When we see that @provides specifies an overridden field, remove it from the field selection.

[clenfest]

I moved the logic that checked to ensure all @external directives were used from the subgraph validations into the merge validations. Other than that, I'm part of me is unsure about the assert inside of removeOverriddenFieldFromFieldSet and wants to just return undefined, but I think the test harness should catch if I've created any problems there

[svc-apollo-docs]

✅ Docs Preview Ready

No new or changed pages found.

[changeset-bot]

🦋 Changeset detected

Latest commit: 12158af

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@apollo/composition	Patch
@apollo/federation-internals	Patch
@apollo/gateway	Patch
@apollo/query-planner	Patch
@apollo/query-graphs	Patch
@apollo/subgraph	Patch
apollo-federation-integration-testsuite	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[clenfest]

This test is essentially moved from subgraphValidation.test.ts

[dariuszkuc]

nit: I think the naming is somewhat confusing as this is specifically for @provides but function name doesnt mention it at all

[dariuszkuc]

would it be possible to have some overriddenCoordinates that are applicable to different @provides? (so there would be no field set filtering required for this @provides)

[sachindshinde]

This kind of error is not a mismatch, so instead of using mismatch reporter, you should use this.errors.push().

[sachindshinde]

When this was a subgraph validation, the addSubgraphToError() function would prefix the message with the subgraph name (which you've done above), but it would do other things as well (e.g. add the subgraph name as a key in the AST nodes). It would be easier to call that function here instead of manually copying its logic.

[sachindshinde]

I'm a bit confused why this particular validation was moved from subgraph validation to (the constructor of) merge validation. My understanding of @external is that the meaning and validity of it is subgraph-local. Is there a specific problem we're trying to solve here?

[sachindshinde]

Two things here:

• It looks like you're only looking for top-level fields in the@provides, when we really want to check all the fields.
• @override can only be on (non-interface-object) object fields, and the fields being overridden, if they exist, must also be object fields (if the field doesn't exist, it can't be "effectively" implemented by an interface object either).
  • The effect of this is that when looking at fields f in @provides, you want to find the possible runtime types of that field's parent type P, and then for each such possible runtime type T, you want to check if T.f is in overriddenCoordinates. It's not sufficient to just check P.f.

In terms of how to do this, I would suggest a similar approach to removeInactiveApplications(), which modifies the given directive to exclude non-external leaf fields (potentially removing the directive entirely). The gist is:

1. Call this.getFieldSet(source, sourceMeta.providesDirective()), as addJoinField() does, to get the (string) field set given to @provides.
2. Call parseSelectionSet() to parse that field set into a SelectionSet (you can use validate: false, since it should already be done by this point).
3. You'll want to create an analog to selectsNonExternalLeafField(), e.g. selectsOverriddenField(), to check whether the @provides selection set uses an overridden field at all.
   • If it doesn't, then you should just return the original @provides string. This is in part to avoid unnecessary selection set construction, and in part to avoid unnecessary diffs in the supergraph schema.
4. When it does contain an overridden field, you'll need to call an analog to withoutNonExternalLeafFields() that removes overridden fields.
   • It'll help here to create an analog to isExternalOrHasExternalImplementations() that checks whether the field (or one of its implementers) is in overriddenCoordinates. You can call that in the analog for selectsNonExternalLeafField() as well.
   • Note that the behavior when an empty selection set occurs is to remove the parent selection, which should be fine. When the top-level selection set is empty, this results in the @provides being effectively dropped entirely.

[sachindshinde]

Note that:

1. Since the result of overridden field removal may be to remove the @provides entirely, this could result in the @join__field becoming unnecessary. This means the computation of the "updated" @provides needs to be done before that, so that needsJoinField() can be aware of it. (Might want to put it in FieldMergeContextProperties.)
2. In @join__field, the usedOverridden argument is set based on whether the overridden field is "used" (i.e. is in @key/@requires/@provides/@fromContext or is an object field that implements some interface field in the subgraph). If it's used, then the field still needs to be in the overridden subgraph, but otherwise (for non-progressive overrides) it is effectively dropped from supergraph schema metadata.
   • This definition of "used" should account for the @provides effectively omitting (non-progressive) overridden fields. My suggestion here is that the collectUsedFields() function should collect two separate sets of FieldDefinitions: one for @provides, and one for the rest. Both of these should be stored in FederationMetadata.
   • The FederationMetadata.isFieldUsed() method should check both sets. You should add a new method to FederationMetadata called e.g. isFieldUsedIgnoringProvides() that only checks the non-@provides set.
   • In validateOverride(), when computing overriddenFieldIsReferenced, use isFieldUsedIgnoringProvides() instead of isFieldUsed() if overrideLabel is unset.