From 14ef75d8ddfd0fdaea0a731143e553ef8ffa2e05 Mon Sep 17 00:00:00 2001 From: Sylvain Lebresne Date: Fri, 23 Sep 2022 11:18:18 +0200 Subject: [PATCH] Fix building subgraph selections using the wrong underlying schema (#2155) In some case, we were building the selections for a subgraph based on the "operation path", which meant the selections were incorrectly linked to the supergraph API schema. This could lead to case where, as we tried to add a field to such selection, the field was not found due to being `@inacessible` and thus not part of the type of the API schema. This commit ensures we instead always "add" the "operation path" to a selection set properly based on the subgraph schema we're building the selection for. Fixes #2152 --- gateway-js/CHANGELOG.md | 2 + internals-js/src/operations.ts | 78 ++++++++++++------- query-planner-js/CHANGELOG.md | 3 + .../src/__tests__/buildPlan.test.ts | 76 ++++++++++++++++++ query-planner-js/src/buildPlan.ts | 11 ++- 5 files changed, 138 insertions(+), 32 deletions(-) diff --git a/gateway-js/CHANGELOG.md b/gateway-js/CHANGELOG.md index 4730505a0..fe0dbd611 100644 --- a/gateway-js/CHANGELOG.md +++ b/gateway-js/CHANGELOG.md @@ -4,6 +4,8 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The ## vNext +- Fix building subgraph selections using the wrong underlying schema [PR #2155](https://github.com/apollographql/federation/pull/2155). + ## 2.1.2 - Allow fields with arguments in `@requires` [PR #2120](https://github.com/apollographql/federation/pull/2120). diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index 724880cc3..b17080739 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -26,7 +26,6 @@ import { isInterfaceType, isLeafType, isNullableType, - isObjectType, isUnionType, ObjectType, runtimeTypesIntersects, @@ -75,6 +74,9 @@ abstract class AbstractOperationElement> e return mergeVariables(this.variablesInElement, this.variablesInAppliedDirectives()); } + /** + * See `FielSelection.updateForAddingTo` for a discussion of why this method exists and what it does. + */ abstract updateForAddingTo(selection: SelectionSet): T; addAttachement(key: string, value: string) { @@ -198,6 +200,9 @@ export class Field ex } } + /** + * See `FielSelection.updateForAddingTo` for a discussion of why this method exists and what it does. + */ updateForAddingTo(selectionSet: SelectionSet): Field { const selectionParent = selectionSet.parentType; const fieldParent = this.definition.parent; @@ -209,19 +214,16 @@ export class Field ex return this.withUpdatedDefinition(selectionParent.typenameField()!); } - // We accept adding a selection of an interface field to a selection of one of its subtype. But otherwise, it's invalid. - // Do note that the field might come from a supergraph while the selection is on a subgraph, so we avoid relying on isDirectSubtype (because - // isDirectSubtype relies on the subtype knowing which interface it implements, but the one of the subgraph might not declare implementing - // the supergraph interface, even if it does in the subgraph). + // There is 2 valid cases were we could get here: + // 1. either `selectionParent` and `fieldParent` are the same underlying type (same name) but from different underlying schema. Typically, + // happens when we're building subgraph queries but using selections from the original query which is against the supergraph API schema. + // 2. or they are not the same underlying type, and we only accept this if we're adding an interface field to a selection of one of its + // subtype, and this for convenience. Note that in that case too, `selectinParent` and `fieldParent` may or may be from the same exact + // underlying schema, and so we avoid relying on `isDirectSubtype` in the check. + // In both cases, we just get the field from `selectionParent`, ensuring the return field parent _is_ `selectionParent`. validate( selectionParent.name == fieldParent.name - || ( - !isUnionType(selectionParent) - && ( - (isInterfaceType(fieldParent) && fieldParent.allImplementations().some(i => i.name == selectionParent.name)) - || (isObjectType(fieldParent) && fieldParent.name == selectionParent.name) - ) - ), + || (isInterfaceType(fieldParent) && fieldParent.allImplementations().some(i => i.name == selectionParent.name)), () => `Cannot add selection of field "${this.definition.coordinate}" to selection set of parent type "${selectionSet.parentType}"` ); const fieldDef = selectionParent.field(this.name); @@ -301,6 +303,9 @@ export class FragmentElement extends AbstractOperationElement { return newFragment; } + /** + * See `FielSelection.updateForAddingTo` for a discussion of why this method exists and what it does. + */ updateForAddingTo(selectionSet: SelectionSet): FragmentElement { const selectionParent = selectionSet.parentType; const fragmentParent = this.parentType; @@ -1112,7 +1117,7 @@ export class SelectionSet extends Freezable { return toAdd; } - addPath(path: OperationPath) { + addPath(path: OperationPath, onPathEnd?: (finalSelectionSet: SelectionSet | undefined) => void) { let previousSelections: SelectionSet = this; let currentSelections: SelectionSet | undefined = this; for (const element of path) { @@ -1121,6 +1126,9 @@ export class SelectionSet extends Freezable { previousSelections = currentSelections; currentSelections = mergedSelection.selectionSet; } + if (onPathEnd) { + onPathEnd(currentSelections); + } } addSelectionSetNode( @@ -1382,19 +1390,6 @@ export function selectionOfElement(element: OperationElement, subSelection?: Sel return element.kind === 'Field' ? new FieldSelection(element, subSelection) : new InlineFragmentSelection(element, subSelection); } -export function selectionSetOfPath(path: OperationPath, onPathEnd?: (finalSelectionSet: SelectionSet | undefined) => void): SelectionSet { - validate(path.length > 0, () => `Cannot create a selection set from an empty path`); - const last = selectionSetOfElement(path[path.length - 1]); - let current = last; - for (let i = path.length - 2; i >= 0; i--) { - current = selectionSetOfElement(path[i], current); - } - if (onPathEnd) { - onPathEnd(last.selections()[0].selectionSet); - } - return current; -} - export type Selection = FieldSelection | FragmentSelection; export class FieldSelection extends Freezable { @@ -1526,6 +1521,34 @@ export class FieldSelection extends Freezable { this.selectionSet?.validate(); } + /** + * Returns a field selection "equivalent" to the one represented by this object, but such that: + * 1. its parent type is the exact one of the provided selection set (same type of same schema object). + * 2. it is not frozen (which might involve cloning). + * + * This method assumes that such a thing is possible, meaning that the parent type of the provided + * selection set does have a field that correspond to this selection (which can support any sub-selection). + * If that is not the case, an assertion will be thrown. + * + * Note that in the simple cases where this selection parent type is already the one of the provide + * `selectionSet`, then this method is mostly a no-op, except for the potential cloning if this selection + * is frozen. But this method mostly exists to make working with multiple "similar" schema easier. + * That is, `Selection` and `SelectionSet` are intrinsically linked to a particular `Schema` object since + * their underlying `OperationElement` points to fields and types of a particular `Schema`. And we want to + * make sure that _everything_ within a particular `SelectionSet` does link to the same `Schema` object, + * or things could get really confusing (nor would it make much sense; a selection set is that of a particular + * schema fundamentally). In many cases, when we work with a single schema (when we parse an operation string + * against a given schema for instance), this problem is moot, but as we do query planning for instance, we + * end up building queries over subgraphs _based_ on some selections from the supergraph API schema, and so + * we need to deal with the fact that the code can easily mix selection from different schema. One option + * could be to simply hard-reject such mixing, meaning that `SelectionSet.add(Selection)` could error out + * if the provided selection is not of the same schema of that of the selection set we add to, thus forcing + * the caller to first ensure the selection is properly "rebased" on the same schema. But this would be a + * bit inconvenient and so this this method instead provide a sort of "automatic rebasing": that is, it + * allows `this` selection not be of the same schema as the provided `selectionSet` as long as both are + * "compatible", and as long as it's the case, it return an equivalent selection that is suitable to be + * added to `selectionSet` (it's against the same fundamental schema). + */ updateForAddingTo(selectionSet: SelectionSet): FieldSelection { const updatedField = this.field.updateForAddingTo(selectionSet); if (this.field === updatedField) { @@ -1650,6 +1673,9 @@ export abstract class FragmentSelection extends Freezable { abstract withNormalizedDefer(normalizer: DeferNormalizer): FragmentSelection | SelectionSet; + /** + * See `FielSelection.updateForAddingTo` for a discussion of why this method exists and what it does. + */ abstract updateForAddingTo(selectionSet: SelectionSet): FragmentSelection; abstract withUpdatedSubSelection(newSubSelection: SelectionSet | undefined): FragmentSelection; diff --git a/query-planner-js/CHANGELOG.md b/query-planner-js/CHANGELOG.md index c23ed101b..f710b1973 100644 --- a/query-planner-js/CHANGELOG.md +++ b/query-planner-js/CHANGELOG.md @@ -4,7 +4,10 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The ## vNext +- Fix building subgraph selections using the wrong underlying schema [PR #2155](https://github.com/apollographql/federation/pull/2155). + ## 2.1.2 + - Fix issue with path #2137 (optimization for `__typename`) [PR #2140](https://github.com/apollographql/federation/pull/2140). - Fix potential inefficient planning due to `__typename` [PR #2137](https://github.com/apollographql/federation/pull/2137). - Fix potential assertion during query planning [PR #2133](https://github.com/apollographql/federation/pull/2133). diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index 6b631242a..b201e7773 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -2566,6 +2566,82 @@ describe('@requires', () => { `); }); }); + + it('can require @inaccessible fields', () => { + const subgraph1 = { + name: 'Subgraph1', + typeDefs: gql` + type Query { + one: One + onlyIn1: Int + } + + type One @key(fields: "id") { + id: ID! + a: String @inaccessible + onlyIn1: Int + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + typeDefs: gql` + type Query { + onlyIn2: Int + } + + type One @key(fields: "id") { + id: ID! + a: String @external + b: String @requires(fields: "a" ) + onlyIn2: Int + } + ` + } + + const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2); + const operation = operationFromDocument(api, gql` + { + one { + b + } + } + `); + + const plan = queryPlanner.buildQueryPlan(operation); + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Sequence { + Fetch(service: "Subgraph1") { + { + one { + __typename + id + a + } + } + }, + Flatten(path: "one") { + Fetch(service: "Subgraph2") { + { + ... on One { + __typename + id + a + } + } => + { + ... on One { + b + } + } + }, + }, + }, + } + `); + }); }); describe('fetch operation names', () => { diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts index b5dce4abe..25663f22d 100644 --- a/query-planner-js/src/buildPlan.ts +++ b/query-planner-js/src/buildPlan.ts @@ -22,7 +22,6 @@ import { Selection, SelectionSet, selectionSetOf, - selectionSetOfPath, Type, Variable, VariableDefinition, @@ -804,8 +803,8 @@ class FetchGroup { } } - addSelection(path: OperationPath) { - this._selection.forWrite().addPath(path); + addSelection(path: OperationPath, onPathEnd?: (finalSelectionSet: SelectionSet | undefined) => void) { + this._selection.forWrite().addPath(path, onPathEnd); } addSelections(selection: SelectionSet) { @@ -930,7 +929,8 @@ class FetchGroup { if (path.length === 0) { selectionSet = merged.selection; } else { - selectionSet = selectionSetOfPath(path, (endOfPathSet) => { + selectionSet = new SelectionSet(this.selection.parentType); + selectionSet.addPath(path, (endOfPathSet) => { assert(endOfPathSet, () => `Merge path ${path} ends on a non-selectable type`); for (const selection of merged.selection.selections()) { const withoutUneededFragments = removeRedundantFragments(selection, endOfPathSet.parentType, mergePathConditionalDirectives); @@ -3344,11 +3344,10 @@ function addPostRequireInputs( if (keyInputs) { // It could be the key used to resume fetching after the @require is already fetched in the original group, but we cannot // guarantee it, so we add it now (and if it was already selected, this is a no-op). - const addedSelection = selectionSetOfPath(requirePath, (endOfPathSet) => { + preRequireGroup.addSelection(requirePath, (endOfPathSet) => { assert(endOfPathSet, () => `Merge path ${requirePath} ends on a non-selectable type`); endOfPathSet.addAll(keyInputs.selections()); }); - preRequireGroup.addSelections(addedSelection); } return updatedPath; }