Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/twenty-plants-rescue.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@apollo/composition": patch
"@apollo/federation-internals": patch
---

When `@provides` specifies an overridden field, remove it from the supergraph's selection set so that data is retrieved from the correct subgraph
42 changes: 38 additions & 4 deletions composition-js/src/__tests__/compose.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ describe('composition', () => {
expect(result.supergraphSdl).toMatchString(`
schema
@link(url: "https://specs.apollo.dev/link/v1.0")
@link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION)
@link(url: "https://specs.apollo.dev/join/v0.6", for: EXECUTION)
{
query: Query
}
Expand All @@ -79,7 +79,7 @@ describe('composition', () => {

directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE

directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!], originalProvides: String) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION

directive @join__graph(name: String!, url: String!) on ENUM_VALUE

Expand Down Expand Up @@ -231,7 +231,7 @@ describe('composition', () => {
expect(result.supergraphSdl).toMatchString(`
schema
@link(url: "https://specs.apollo.dev/link/v1.0")
@link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION)
@link(url: "https://specs.apollo.dev/join/v0.6", for: EXECUTION)
{
query: Query
}
Expand All @@ -240,7 +240,7 @@ describe('composition', () => {

directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE

directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!], originalProvides: String) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION

directive @join__graph(name: String!, url: String!) on ENUM_VALUE

Expand Down Expand Up @@ -5290,3 +5290,37 @@ describe('@source* directives', () => {
assertCompositionSuccess(result);
});
});

it('errors on unused @external', () => {
const subgraphA = {
name: 'S',
typeDefs: gql`
type Query {
T: T!
}

type T {
f: Int @external
}
`,
};

const subgraphB = {
name: 'T',
typeDefs: gql`
type Query {
a: Int!
}

type T {
f: Int
}
`,
};

const result = composeAsFed2Subgraphs([subgraphA, subgraphB]);
expect(result.errors).toBeDefined();
expect(errors(result)).toStrictEqual([
['EXTERNAL_UNUSED', '[S] Field "T.f" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external).']
]);
});
21 changes: 17 additions & 4 deletions composition-js/src/__tests__/override.compose.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ describe("composition involving @override directive", () => {
`);
});

// @provides may not provide a value when the field is completely overridden in the local subgraph
// when that happens, the selection should be removed from the field set
it("override field in a @provides", () => {
const subgraph1 = {
name: "Subgraph1",
Expand All @@ -125,7 +127,10 @@ describe("composition involving @override directive", () => {
type A @key(fields: "id") {
id: ID!
b: B @override(from: "Subgraph2")
z: String! @shareable
z2: String! @shareable
}

type B @key(fields: "id") {
id: ID!
v: String @shareable
Expand All @@ -143,11 +148,15 @@ describe("composition involving @override directive", () => {
typeDefs: gql`
type T @key(fields: "k") {
k: ID
a: A @shareable @provides(fields: "b { v }")
a: A @shareable @provides(fields: "b { v } z")
a2: A @shareable @provides(fields: "b { v }")
a3: A @shareable @provides(fields: "z b { v } z2")
}
type A @key(fields: "id") {
id: ID!
b: B
z: String! @external
z2: String! @external
}
type B @key(fields: "id") {
id: ID!
Expand All @@ -168,6 +177,8 @@ describe("composition involving @override directive", () => {
{
id: ID!
b: B @join__field(graph: SUBGRAPH1, override: \\"Subgraph2\\") @join__field(graph: SUBGRAPH2, usedOverridden: true)
z: String! @join__field(graph: SUBGRAPH1) @join__field(graph: SUBGRAPH2, external: true)
z2: String! @join__field(graph: SUBGRAPH1) @join__field(graph: SUBGRAPH2, external: true)
}"
`);

Expand All @@ -179,7 +190,9 @@ describe("composition involving @override directive", () => {
@join__type(graph: SUBGRAPH2, key: \\"k\\")
{
k: ID
a: A @join__field(graph: SUBGRAPH1) @join__field(graph: SUBGRAPH2, provides: \\"b { v }\\")
a: A @join__field(graph: SUBGRAPH1) @join__field(graph: SUBGRAPH2, provides: \\"z\\", originalProvides: \\"b { v } z\\")
a2: A @join__field(graph: SUBGRAPH2, originalProvides: \\"b { v }\\")
a3: A @join__field(graph: SUBGRAPH2, provides: \\"z z2\\", originalProvides: \\"z b { v } z2\\")
}"
`);
});
Expand Down Expand Up @@ -974,7 +987,7 @@ describe("composition involving @override directive", () => {
expect(result.supergraphSdl).toMatchInlineSnapshot(`
"schema
@link(url: \\"https://specs.apollo.dev/link/v1.0\\")
@link(url: \\"https://specs.apollo.dev/join/v0.5\\", for: EXECUTION)
@link(url: \\"https://specs.apollo.dev/join/v0.6\\", for: EXECUTION)
{
query: Query
}
Expand All @@ -983,7 +996,7 @@ describe("composition involving @override directive", () => {

directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE

directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!], originalProvides: String) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION

directive @join__graph(name: String!, url: String!) on ENUM_VALUE

Expand Down
120 changes: 108 additions & 12 deletions composition-js/src/merging/merge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@
FeatureDefinition,
CoreImport,
inaccessibleIdentity,
parseSelectionSet,
isUnionType,
} from "@apollo/federation-internals";
import { ASTNode, GraphQLError, DirectiveLocation } from "graphql";
import {
Expand Down Expand Up @@ -379,15 +381,21 @@
private schemaToImportNameToFeatureUrl = new Map<Schema, Map<string, FeatureUrl>>();
private fieldsWithFromContext: Set<string>;
private fieldsWithOverride: Set<string>;

// a map from the subgraph index to a list of coordinates that are completely overridden by another subgraph
private completelyOverriddenFieldMap: Map<number, Set<string>>;

constructor(readonly subgraphs: Subgraphs, readonly options: CompositionOptions) {
this.names = subgraphs.names();
this.latestFedVersionUsed = this.getLatestFederationVersionUsed();
this.joinSpec = JOIN_VERSIONS.getMinimumRequiredVersion(this.latestFedVersionUsed);
this.linkSpec = LINK_VERSIONS.getMinimumRequiredVersion(this.latestFedVersionUsed);
this.fieldsWithFromContext = this.getFieldsWithFromContextDirective();
this.fieldsWithOverride = this.getFieldsWithOverrideDirective();

this.names = subgraphs.names();

const { overriddenFieldMap, fieldsWithOverride } = this.getFieldsWithOverrideDirective();
this.fieldsWithOverride = fieldsWithOverride;
this.completelyOverriddenFieldMap = overriddenFieldMap;

this.composeDirectiveManager = new ComposeDirectiveManager(
this.subgraphs,
(error: GraphQLError) => { this.errors.push(error) },
Expand Down Expand Up @@ -2045,10 +2053,16 @@
const external = this.isExternal(idx, source);
const sourceMeta = this.subgraphs.values()[idx].metadata();
const name = this.joinSpecName(idx);

// fields in this subgraph that are no longer viable because they've been overridden
const overriddenFields = this.completelyOverriddenFieldMap.get(idx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const originalProvides = this.getFieldSet(source, sourceMeta.providesDirective());
const providesFieldSet = this.removeOverriddenFieldFromFieldSet(source, overriddenFields, sourceMeta);
dest.applyDirective(joinFieldDirective, {
graph: name,
requires: this.getFieldSet(source, sourceMeta.requiresDirective()),
provides: this.getFieldSet(source, sourceMeta.providesDirective()),
provides: providesFieldSet,
originalProvides: providesFieldSet === originalProvides ? undefined : originalProvides,
override: source.appliedDirectivesOf(sourceMeta.overrideDirective()).pop()?.arguments()?.from,
type: allTypesEqual ? undefined : source.type?.toString(),
external: external ? true : undefined,
Expand All @@ -2058,6 +2072,69 @@
});
}
}

private removeOverriddenFieldFromFieldSet <T extends FieldDefinition<ObjectType | InterfaceType> | InputFieldDefinition>(
source: T,
overriddenCoordinates: Set<string> | undefined,
sourceMeta: FederationMetadata,
) {
const providesDirective = sourceMeta.providesDirective();
const applications = source.appliedDirectivesOf(providesDirective);
assert(applications.length <= 1, () => `Found more than one application of ${providesDirective} on ${source}`);
if (applications.length === 0) {
return undefined;
}
const fields: string = applications[0].arguments().fields;
const parent = applications[0].parent;
const parentType = baseType(parent.type!);

if (!overriddenCoordinates) {
return fields;
}

if (!isCompositeType(parentType)) {
return undefined;

Check warning on line 2096 in composition-js/src/merging/merge.ts

View check run for this annotation

Codecov / codecov/patch

composition-js/src/merging/merge.ts#L2096

Added line #L2096 was not covered by tests
}

// when we parse the selection set, we will have a list of selections. Anything that eventually goes to a overridden field
// cannot be considered valid in the supgraph
let validSelectionsIndex: boolean[] = [];

const selectionSet = parseSelectionSet({
Copy link
Contributor

@sachindshinde sachindshinde Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

parentType,
source: fields,
fieldAccessor: (t, f) => {
const field = t.field(f);

// Every time we get back to to seeing the parentType, we know that it is a new selection.
// Assume it's valid until we see a contradiction
if (t.name === parentType.name) {
validSelectionsIndex.push(true);
} else if (isInterfaceType(parentType)) {
if (parentType.allImplementations().find(member => member.name === t.name)) {
validSelectionsIndex.push(true);

Check warning on line 2115 in composition-js/src/merging/merge.ts

View check run for this annotation

Codecov / codecov/patch

composition-js/src/merging/merge.ts#L2115

Added line #L2115 was not covered by tests
}
} else if (isUnionType(parentType)) {
if (parentType.members().find(member => t.name === member.type.name)) {
validSelectionsIndex.push(true);

Check warning on line 2119 in composition-js/src/merging/merge.ts

View check run for this annotation

Codecov / codecov/patch

composition-js/src/merging/merge.ts#L2119

Added line #L2119 was not covered by tests
}
}
assert(field, 'field should exist on type');
if (overriddenCoordinates.has(field.coordinate)) {
validSelectionsIndex[validSelectionsIndex.length-1] = false;

Check warning on line 2124 in composition-js/src/merging/merge.ts

View check run for this annotation

Apollo SecOps / Static App Security Check

rules.providers.gitlab.security.eslint.detect-object-injection

Bracket object notation with user input is present, this might allow an attacker to access all properties of the object and even it's prototype, leading to possible code execution.
}
return field;
}
});

// now we should have a SelectionSet with an array of _selections the same length as `validSelectionsIndex`
// We should be able to filter out the ones that are invalid and we'll be left with the valid selection string
const selections = selectionSet.selections();
assert(selections.length === validSelectionsIndex.length, 'selection parsing failed');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

const filteredSelections = selections.filter((_, index) => validSelectionsIndex[index]);

Check warning on line 2134 in composition-js/src/merging/merge.ts

View check run for this annotation

Apollo SecOps / Static App Security Check

rules.providers.gitlab.security.eslint.detect-object-injection

Bracket object notation with user input is present, this might allow an attacker to access all properties of the object and even it's prototype, leading to possible code execution.
return filteredSelections.length > 0 ? filteredSelections.map(selection => selection.toString()).join(' ') : undefined;
}

private getFieldSet(element: SchemaElement<any, any>, directive: DirectiveDefinition<{fields: string}>): string | undefined {
const applications = element.appliedDirectivesOf(directive);
assert(applications.length <= 1, () => `Found more than one application of ${directive} on ${element}`);
Expand Down Expand Up @@ -3531,15 +3608,34 @@
);
}

private getFieldsWithOverrideDirective(): Set<string> {
return this.getFieldsWithAppliedDirective(
(subgraph: Subgraph) => subgraph.metadata().overrideDirective(),
(application: Directive<SchemaElement<any,any>>) => {
const field = application.parent;
assert(isFieldDefinition(field), () => `Expected ${application.parent} to be a field`);
return field;
private getFieldsWithOverrideDirective(): { fieldsWithOverride: Set<string>, overriddenFieldMap: Map<number, Set<string>> } {
const overriddenFieldMap = new Map<number, Set<string>>();
const fieldsWithOverride = new Set<string>();
for (const subgraph of this.subgraphs) {
const directive = subgraph.metadata().overrideDirective();
if (isFederationDirectiveDefinedInSchema(directive)) {
for (const application of directive.applications()) {
const field = application.parent;
assert(isFieldDefinition(field), () => `Expected ${application.parent} to be a field`);
const coordinate = field.coordinate;
const { from: fromSubgraphName, label } = application.arguments();
fieldsWithOverride.add(coordinate);

// we only want fields that are completely overridden (i.e. progressive overrides will have a label and we should ignore them)
if (!label) {
const fromSubgraphIndex = this.names.indexOf(fromSubgraphName);
if (!overriddenFieldMap.has(fromSubgraphIndex)) {
overriddenFieldMap.set(fromSubgraphIndex, new Set());
}
overriddenFieldMap.get(fromSubgraphIndex)?.add(coordinate);
}
}
}
);
}
return {
fieldsWithOverride,
overriddenFieldMap,
};
}

private getFieldsWithAppliedDirective(
Expand Down
2 changes: 1 addition & 1 deletion gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ describe('lifecycle hooks', () => {
// the supergraph (even just formatting differences), this ID will change
// and this test will have to updated.
expect(secondCall[0]!.compositionId).toMatchInlineSnapshot(
`"6dc1bde2b9818fabec62208c5d8825abaa1bae89635fa6f3a5ffea7b78fc6d82"`,
`"2d9e498fd22c9fab2bda597f91b67d289b7edb01ec5c245f0527d3699d15bddb"`,
);
// second call should have previous info in the second arg
expect(secondCall[1]!.compositionId).toEqual(expectedFirstId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -821,7 +821,7 @@ test('contextual arguments can be extracted', () => {
const supergraph = `
schema
@link(url: "https://specs.apollo.dev/link/v1.0")
@link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION)
@link(url: "https://specs.apollo.dev/join/v0.6", for: EXECUTION)
@link(url: "https://specs.apollo.dev/context/v0.1")
{
query: Query
Expand All @@ -833,7 +833,7 @@ directive @join__graph(name: String!, url: String!) on ENUM_VALUE

directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR

directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!], originalProvides: String) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION

directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE

Expand Down
2 changes: 1 addition & 1 deletion internals-js/src/extractSubgraphsFromSupergraph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ function addSubgraphField({
subgraphField.applyDirective(subgraph.metadata().requiresDirective(), {'fields': joinFieldArgs.requires});
}
if (joinFieldArgs?.provides) {
subgraphField.applyDirective(subgraph.metadata().providesDirective(), {'fields': joinFieldArgs.provides});
subgraphField.applyDirective(subgraph.metadata().providesDirective(), {'fields': joinFieldArgs.originalProvides ?? joinFieldArgs.provides});
}
if (joinFieldArgs?.contextArguments) {
const fromContextDirective = subgraph.metadata().fromContextDirective();
Expand Down
8 changes: 7 additions & 1 deletion internals-js/src/specs/joinSpec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
graph?: string,
requires?: string,
provides?: string,
originalProvides?: string,
override?: string,
type?: string,
external?: boolean,
Expand Down Expand Up @@ -176,6 +177,10 @@

joinField.addArgument('contextArguments', new ListType(new NonNullType(contextArgumentsType)));
}

if (this.version.gte(new FeatureVersion(0, 6))) {
joinField.addArgument('originalProvides', schema.stringType());

Check warning on line 182 in internals-js/src/specs/joinSpec.ts

View check run for this annotation

Codecov / codecov/patch

internals-js/src/specs/joinSpec.ts#L182

Added line #L182 was not covered by tests
}

if (this.isV01()) {
const joinOwner = this.addDirective(schema, 'owner').addLocations(DirectiveLocation.OBJECT);
Expand Down Expand Up @@ -289,6 +294,7 @@
.add(new JoinSpecDefinition(new FeatureVersion(0, 2)))
.add(new JoinSpecDefinition(new FeatureVersion(0, 3), new FeatureVersion(2, 0)))
.add(new JoinSpecDefinition(new FeatureVersion(0, 4), new FeatureVersion(2, 7)))
.add(new JoinSpecDefinition(new FeatureVersion(0, 5), new FeatureVersion(2, 8)));
.add(new JoinSpecDefinition(new FeatureVersion(0, 5), new FeatureVersion(2, 8)))
.add(new JoinSpecDefinition(new FeatureVersion(0, 6), new FeatureVersion(2, 9)));

registerKnownFeature(JOIN_VERSIONS);
Loading