From b6be9f9650a69f6214d806d66b198729560da3dc Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Mon, 17 Jul 2023 14:45:32 -0700 Subject: [PATCH] Revert #2639, an attempt to fix query fragment reuse (#2681) --- .changeset/metal-cheetahs-add.md | 10 + .../src/__tests__/buildQueryPlan.test.ts | 19 +- internals-js/src/__tests__/operations.test.ts | 402 ------------------ internals-js/src/operations.ts | 244 +++-------- .../src/__tests__/buildPlan.test.ts | 292 +------------ query-planner-js/src/buildPlan.ts | 2 +- 6 files changed, 93 insertions(+), 876 deletions(-) create mode 100644 .changeset/metal-cheetahs-add.md diff --git a/.changeset/metal-cheetahs-add.md b/.changeset/metal-cheetahs-add.md new file mode 100644 index 000000000..20224d62f --- /dev/null +++ b/.changeset/metal-cheetahs-add.md @@ -0,0 +1,10 @@ +--- +"@apollo/query-planner": patch +"@apollo/federation-internals": patch +"@apollo/gateway": patch +--- + +Revert #2639 from v2.4.9 + +PR #2639 attempts to resolve issues with query fragment reuse, but we've since turned up multiple issues (at least 1 of which is a regression - see #2680. For now, this reverts it until we resolve the regression for a future patch release. + \ No newline at end of file diff --git a/gateway-js/src/__tests__/buildQueryPlan.test.ts b/gateway-js/src/__tests__/buildQueryPlan.test.ts index 6d4160449..02c6a900f 100644 --- a/gateway-js/src/__tests__/buildQueryPlan.test.ts +++ b/gateway-js/src/__tests__/buildQueryPlan.test.ts @@ -744,21 +744,18 @@ describe('buildQueryPlan', () => { it(`should not get confused by a fragment spread multiple times`, () => { const operationString = `#graphql - fragment PriceAndCountry on Product { + fragment Price on Product { price - details { - country - } } query { topProducts { __typename ... on Book { - ...PriceAndCountry + ...Price } ... on Furniture { - ...PriceAndCountry + ...Price } } } @@ -773,20 +770,16 @@ describe('buildQueryPlan', () => { topProducts { __typename ... on Book { - ...PriceAndCountry + ...Price } ... on Furniture { - ...PriceAndCountry + ...Price } } } - fragment PriceAndCountry on Product { + fragment Price on Product { price - details { - __typename - country - } } }, } diff --git a/internals-js/src/__tests__/operations.test.ts b/internals-js/src/__tests__/operations.test.ts index 458629385..0d3f530bf 100644 --- a/internals-js/src/__tests__/operations.test.ts +++ b/internals-js/src/__tests__/operations.test.ts @@ -2758,405 +2758,3 @@ describe('named fragment selection set restrictions at type', () => { `); }); }); - -describe('named fragment rebasing on subgraphs', () => { - test('it skips unknown fields', () => { - const schema = parseSchema(` - type Query { - t: T - } - - type T { - v0: Int - v1: Int - v2: Int - u1: U - u2: U - } - - type U { - v3: Int - v4: Int - v5: Int - } - `); - - const operation = parseOperation(schema, ` - query { - t { - ...FragOnT - } - } - - fragment FragOnT on T { - v0 - v1 - v2 - u1 { - v3 - v4 - v5 - } - u2 { - v4 - v5 - } - } - `); - - const fragments = operation.fragments; - assert(fragments, 'Should have some fragments'); - - const subgraph = parseSchema(` - type Query { - _: Int - } - - type T { - v1: Int - u1: U - } - - type U { - v3: Int - v5: Int - } - `); - - const rebased = fragments.rebaseOn(subgraph); - expect(rebased?.toString('')).toMatchString(` - fragment FragOnT on T { - v1 - u1 { - v3 - v5 - } - } - `); - }); - - test('it skips unknown type (on condition)', () => { - const schema = parseSchema(` - type Query { - t: T - u: U - } - - type T { - x: Int - y: Int - } - - type U { - x: Int - y: Int - } - `); - - const operation = parseOperation(schema, ` - query { - t { - ...FragOnT - } - u { - ...FragOnU - } - } - - fragment FragOnT on T { - x - y - } - - fragment FragOnU on U { - x - y - } - `); - - const fragments = operation.fragments; - assert(fragments, 'Should have some fragments'); - - const subgraph = parseSchema(` - type Query { - t: T - } - - type T { - x: Int - y: Int - } - `); - - const rebased = fragments.rebaseOn(subgraph); - expect(rebased?.toString('')).toMatchString(` - fragment FragOnT on T { - x - y - } - `); - }); - - test('it skips unknown type (used inside fragment)', () => { - const schema = parseSchema(` - type Query { - i: I - } - - interface I { - id: ID! - otherId: ID! - } - - type T1 implements I { - id: ID! - otherId: ID! - x: Int - } - - type T2 implements I { - id: ID! - otherId: ID! - y: Int - } - `); - - const operation = parseOperation(schema, ` - query { - i { - ...FragOnI - } - } - - fragment FragOnI on I { - id - otherId - ... on T1 { - x - } - ... on T2 { - y - } - } - `); - - const fragments = operation.fragments; - assert(fragments, 'Should have some fragments'); - - const subgraph = parseSchema(` - type Query { - i: I - } - - interface I { - id: ID! - } - - type T2 implements I { - id: ID! - y: Int - } - `); - - const rebased = fragments.rebaseOn(subgraph); - expect(rebased?.toString('')).toMatchString(` - fragment FragOnI on I { - id - ... on T2 { - y - } - } - `); - }); - - test('it skips fragments with no selection or trivial ones applying', () => { - const schema = parseSchema(` - type Query { - t: T - } - - type T { - a: Int - b: Int - c: Int - d: Int - } - `); - - const operation = parseOperation(schema, ` - query { - t { - ...F1 - ...F2 - ...F3 - } - } - - fragment F1 on T { - a - b - } - - fragment F2 on T { - __typename - a - b - } - - fragment F3 on T { - __typename - a - b - c - d - } - `); - - const fragments = operation.fragments; - assert(fragments, 'Should have some fragments'); - - const subgraph = parseSchema(` - type Query { - t: T - } - - type T { - c: Int - d: Int - } - `); - - // F1 reduces to nothing, and F2 reduces to just __typename so we shouldn't keep them. - const rebased = fragments.rebaseOn(subgraph); - expect(rebased?.toString('')).toMatchString(` - fragment F3 on T { - __typename - c - d - } - `); - }); - - test('it handles skipped fragments used by other fragments', () => { - const schema = parseSchema(` - type Query { - t: T - } - - type T { - x: Int - u: U - } - - type U { - y: Int - z: Int - } - `); - - const operation = parseOperation(schema, ` - query { - ...TheQuery - } - - fragment TheQuery on Query { - t { - x - ...GetU - } - } - - fragment GetU on T { - u { - y - z - } - } - `); - - const fragments = operation.fragments; - assert(fragments, 'Should have some fragments'); - - const subgraph = parseSchema(` - type Query { - t: T - } - - type T { - x: Int - } - `); - - const rebased = fragments.rebaseOn(subgraph); - expect(rebased?.toString('')).toMatchString(` - fragment TheQuery on Query { - t { - x - } - } - `); - }); - - test('it handles fields whose type is a subtype in the subgarph', () => { - const schema = parseSchema(` - type Query { - t: I - } - - interface I { - x: Int - y: Int - } - - type T implements I { - x: Int - y: Int - z: Int - } - `); - - const operation = parseOperation(schema, ` - query { - ...TQuery - } - - fragment TQuery on Query { - t { - x - y - ... on T { - z - } - } - } - `); - - const fragments = operation.fragments; - assert(fragments, 'Should have some fragments'); - - const subgraph = parseSchema(` - type Query { - t: T - } - - type T { - x: Int - y: Int - z: Int - } - `); - - const rebased = fragments.rebaseOn(subgraph); - expect(rebased?.toString('')).toMatchString(` - fragment TQuery on Query { - t { - x - y - ... on T { - z - } - } - } - `); - }); -}); diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index 445ba095b..61e5848ac 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -85,11 +85,7 @@ abstract class AbstractOperationElement> e abstract asPathElement(): string | undefined; - abstract rebaseOn(args: { parentType: CompositeType, errorIfCannotRebase: boolean }): T | undefined; - - rebaseOnOrError(parentType: CompositeType): T { - return this.rebaseOn({ parentType, errorIfCannotRebase: true })!; - } + abstract rebaseOn(parentType: CompositeType): T; abstract withUpdatedDirectives(newDirectives: readonly Directive[]): T; @@ -294,7 +290,7 @@ export class Field ex } } - rebaseOn({ parentType, errorIfCannotRebase }: { parentType: CompositeType, errorIfCannotRebase: boolean }): Field | undefined { + rebaseOn(parentType: CompositeType): Field { const fieldParent = this.definition.parent; if (parentType === fieldParent) { return this; @@ -304,16 +300,12 @@ export class Field ex return this.withUpdatedDefinition(parentType.typenameField()!); } + validate( + this.canRebaseOn(parentType), + () => `Cannot add selection of field "${this.definition.coordinate}" to selection set of parent type "${parentType}"` + ); const fieldDef = parentType.field(this.name); - const canRebase = this.canRebaseOn(parentType) && fieldDef; - if (!canRebase) { - validate( - !errorIfCannotRebase, - () => `Cannot add selection of field "${this.definition.coordinate}" to selection set of parent type "${parentType}"` - ); - return undefined; - } - + validate(fieldDef, () => `Cannot add selection of field "${this.definition.coordinate}" to selection set of parent type "${parentType}" (that does not declare that field)`); return this.withUpdatedDefinition(fieldDef); } @@ -474,7 +466,7 @@ export class FragmentElement extends AbstractOperationElement { return newFragment; } - rebaseOn({ parentType, errorIfCannotRebase }: { parentType: CompositeType, errorIfCannotRebase: boolean }): FragmentElement | undefined { + rebaseOn(parentType: CompositeType): FragmentElement { const fragmentParent = this.parentType; const typeCondition = this.typeCondition; if (parentType === fragmentParent) { @@ -485,13 +477,10 @@ export class FragmentElement extends AbstractOperationElement { // to update the source type of the fragment, but also "rebase" the condition to the selection set // schema. const { canRebase, rebasedCondition } = this.canRebaseOn(parentType); - if (!canRebase) { - validate( - !errorIfCannotRebase, - () => `Cannot add fragment of condition "${typeCondition}" (runtimes: [${possibleRuntimeTypes(typeCondition!)}]) to parent type "${parentType}" (runtimes: ${possibleRuntimeTypes(parentType)})` - ); - return undefined; - } + validate( + canRebase, + () => `Cannot add fragment of condition "${typeCondition}" (runtimes: [${possibleRuntimeTypes(typeCondition!)}]) to parent type "${parentType}" (runtimes: ${possibleRuntimeTypes(parentType)})` + ); return this.withUpdatedTypes(parentType, rebasedCondition); } @@ -708,7 +697,7 @@ export type RootOperationPath = { path: OperationPath } -// Computes for every fragment, which other fragments use it (so the reverse of it's dependencies, the other fragment it uses). +// Computes for every fragment, which other fragments use it (so the reverse of it's dependencies, the other fragment it uses). function computeFragmentsDependents(fragments: NamedFragments): SetMultiMap { const reverseDeps = new SetMultiMap(); for (const fragment of fragments.definitions()) { @@ -1243,7 +1232,7 @@ export class NamedFragmentDefinition extends DirectiveTargetElement { const rebasedType = schema.type(fragment.selectionSet.parentType.name); - if (!rebasedType || !isCompositeType(rebasedType)) { + try { + if (!rebasedType || !isCompositeType(rebasedType)) { + return undefined; + } + + const rebasedSelection = fragment.selectionSet.rebaseOn(rebasedType, newFragments); + return new NamedFragmentDefinition(schema, fragment.name, rebasedType).setSelectionSet(rebasedSelection); + } catch (e) { + // This means we cannot rebase this selection on the schema and thus cannot reuse that fragment on that + // particular schema. return undefined; } - - const rebasedSelection = fragment.selectionSet.rebaseOn({ parentType: rebasedType, fragments: newFragments, errorIfCannotRebase: false }); - return this.selectionSetIsWorthUsing(rebasedSelection) - ? new NamedFragmentDefinition(schema, fragment.name, rebasedType).setSelectionSet(rebasedSelection) - : undefined;; }); } @@ -1505,7 +1476,7 @@ class DeferNormalizer { } export enum ContainsResult { - // Note: enum values are numbers in the end, and 0 means false in JS, so we should keep `NOT_CONTAINED` first + // Note: enum values are numbers in the end, and 0 means false in JS, so we should keep `NOT_CONTAINED` first // so that using the result of `contains` as a boolean works. NOT_CONTAINED, STRICTLY_CONTAINED, @@ -1544,20 +1515,6 @@ export class SelectionSet { return this._keyedSelections.has(typenameFieldName); } - withoutTopLevelTypenameField(): SelectionSet { - if (!this.hasTopLevelTypenameField) { - return this; - } - - const newKeyedSelections = new Map(); - for (const [key, selection] of this._keyedSelections) { - if (key !== typenameFieldName) { - newKeyedSelections.set(key, selection); - } - } - return new SelectionSet(this.parentType, newKeyedSelections); - } - fieldsInSet(): CollectedFieldsInSet { const fields = new Array<{ path: string[], field: FieldSelection }>(); for (const selection of this.selections()) { @@ -1652,7 +1609,7 @@ export class SelectionSet { } /** - * Applies some normalization rules to this selection set in the context of the provided `parentType`. + * Applies some normalization rules to this selection set in the context of the provided `parentType`. * * Normalization mostly removes unecessary/redundant inline fragments, so that for instance, with * schema: @@ -1802,25 +1759,14 @@ export class SelectionSet { return updated.isEmpty() ? undefined : updated; } - rebaseOn({ - parentType, - fragments, - errorIfCannotRebase, - }: { - parentType: CompositeType, - fragments: NamedFragments | undefined - errorIfCannotRebase: boolean, - }): SelectionSet { + rebaseOn(parentType: CompositeType, fragments: NamedFragments | undefined): SelectionSet { if (this.parentType === parentType) { return this; } const newSelections = new Map(); for (const selection of this.selections()) { - const rebasedSelection = selection.rebaseOn({ parentType, fragments, errorIfCannotRebase }); - if (rebasedSelection) { - newSelections.set(selection.key(), rebasedSelection); - } + newSelections.set(selection.key(), selection.rebaseOn(parentType, fragments)); } return new SelectionSet(parentType, newSelections); @@ -1844,25 +1790,15 @@ export class SelectionSet { return true; } - contains(that: SelectionSet, options?: { ignoreMissingTypename?: boolean }): ContainsResult { - const ignoreMissingTypename = options?.ignoreMissingTypename ?? false; + contains(that: SelectionSet): ContainsResult { if (that._selections.length > this._selections.length) { - // If `that` has more selections but we're ignoring missing __typename, then in the case where - // `that` has a __typename but `this` does not, then we need the length of `that` to be at - // least 2 more than that of `this` to be able to conclude there is no contains. - if (!ignoreMissingTypename || that._selections.length > this._selections.length + 1 || this.hasTopLevelTypenameField() || !that.hasTopLevelTypenameField()) { - return ContainsResult.NOT_CONTAINED; - } + return ContainsResult.NOT_CONTAINED; } let isEqual = true; for (const [key, thatSelection] of that._keyedSelections) { - if (key === typenameFieldName && ignoreMissingTypename) { - continue; - } - const thisSelection = this._keyedSelections.get(key); - const selectionResult = thisSelection?.contains(thatSelection, options); + const selectionResult = thisSelection?.contains(thatSelection); if (selectionResult === undefined || selectionResult === ContainsResult.NOT_CONTAINED) { return ContainsResult.NOT_CONTAINED; } @@ -2228,10 +2164,10 @@ function makeSelection(parentType: CompositeType, updates: SelectionUpdate[], fr // Optimize for the simple case of a single selection, as we don't have to do anything complex to merge the sub-selections. if (updates.length === 1 && first instanceof AbstractSelection) { - return first.rebaseOnOrError({ parentType, fragments }); + return first.rebaseOn(parentType, fragments); } - const element = updateElement(first).rebaseOnOrError(parentType); + const element = updateElement(first).rebaseOn(parentType); const subSelectionParentType = element.kind === 'Field' ? element.baseType() : element.castedType(); if (!isCompositeType(subSelectionParentType)) { // This is a leaf, so all updates should correspond ot the same field and we just use the first. @@ -2279,7 +2215,7 @@ function makeSelectionSet(parentType: CompositeType, keyedUpdates: MultiMap { private computed: SelectionSet | undefined; @@ -2420,11 +2356,7 @@ abstract class AbstractSelection, undefined, Fie } /** - * Returns a field selection "equivalent" to the one represented by this object, but such that its parent type + * Returns a field selection "equivalent" to the one represented by this object, but such that its parent type * is the one provided as argument. * * Obviously, this operation will only succeed if this selection (both the field itself and its subselections) * make sense from the provided parent type. If this is not the case, this method will throw. */ - rebaseOn({ - parentType, - fragments, - errorIfCannotRebase, - }: { - parentType: CompositeType, - fragments: NamedFragments | undefined, - errorIfCannotRebase: boolean, - }): FieldSelection | undefined { + rebaseOn(parentType: CompositeType, fragments: NamedFragments | undefined): FieldSelection { if (this.element.parentType === parentType) { return this; } - const rebasedElement = this.element.rebaseOn({ parentType, errorIfCannotRebase }); - if (!rebasedElement) { - return undefined; - } - + const rebasedElement = this.element.rebaseOn(parentType); if (!this.selectionSet) { return this.withUpdatedElement(rebasedElement); } @@ -2980,8 +2890,7 @@ export class FieldSelection extends AbstractSelection, undefined, Fie } validate(isCompositeType(rebasedBase), () => `Cannot rebase field selection ${this} on ${parentType}: rebased field base return type ${rebasedBase} is not composite`); - const rebasedSelectionSet = this.selectionSet.rebaseOn({ parentType: rebasedBase, fragments, errorIfCannotRebase }); - return rebasedSelectionSet.isEmpty() ? undefined : this.withUpdatedComponents(rebasedElement, rebasedSelectionSet); + return this.withUpdatedComponents(rebasedElement, this.selectionSet.rebaseOn(rebasedBase, fragments)); } /** @@ -3088,7 +2997,7 @@ export class FieldSelection extends AbstractSelection, undefined, Fie return !!that.selectionSet && this.selectionSet.equals(that.selectionSet); } - contains(that: Selection, options?: { ignoreMissingTypename?: boolean }): ContainsResult { + contains(that: Selection): ContainsResult { if (!(that instanceof FieldSelection) || !this.element.equals(that.element)) { return ContainsResult.NOT_CONTAINED; } @@ -3098,7 +3007,7 @@ export class FieldSelection extends AbstractSelection, undefined, Fie return ContainsResult.EQUAL; } assert(that.selectionSet, '`this` and `that` have the same element, so if one has sub-selection, the other one should too') - return this.selectionSet.contains(that.selectionSet, options); + return this.selectionSet.contains(that.selectionSet); } toString(expandFragments: boolean = true, indent?: string): string { @@ -3125,7 +3034,7 @@ export abstract class FragmentSelection extends AbstractSelection boolean): FragmentSelection | undefined { // Note that we essentially expand all fragments as part of this. const updatedSelectionSet = this.selectionSet.filterRecursiveDepthFirst(predicate); @@ -3135,14 +3044,14 @@ export abstract class FragmentSelection extends AbstractSelection `Cannot rebase ${this.toString(false)} if it isn't part of the provided fragments`); - return undefined; - } + assert(namedFragment, () => `Cannot rebase ${this} if it isn't part of the provided fragments`); return new FragmentSpreadSelection( parentType, newFragments, @@ -3617,7 +3497,7 @@ class FragmentSpreadSelection extends FragmentSelection { && sameDirectiveApplications(this.spreadDirectives, that.spreadDirectives); } - contains(that: Selection, options?: { ignoreMissingTypename?: boolean }): ContainsResult { + contains(that: Selection): ContainsResult { if (this.equals(that)) { return ContainsResult.EQUAL; } @@ -3626,7 +3506,7 @@ class FragmentSpreadSelection extends FragmentSelection { return ContainsResult.NOT_CONTAINED; } - return this.selectionSet.contains(that.selectionSet, options); + return this.selectionSet.contains(that.selectionSet); } toString(expandFragments: boolean = true, indent?: string): string { diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index 39705355f..5a1d42690 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -4002,8 +4002,7 @@ describe('Named fragments preservation', () => { } type V { - v1: Int - v2: Int + v: Int } ` @@ -4027,8 +4026,7 @@ describe('Named fragments preservation', () => { } fragment OnV on V { - v1 - v2 + v } `); @@ -4048,8 +4046,7 @@ describe('Named fragments preservation', () => { } fragment OnV on V { - v1 - v2 + v } }, } @@ -5859,19 +5856,16 @@ describe("named fragments", () => { union U = T1 | T2 interface I { - id1: ID! - id2: ID! + id: ID! } type T1 implements I { - id1: ID! - id2: ID! + id: ID! owner: Owner! } type T2 implements I { - id1: ID! - id2: ID! + id: ID! } ` } @@ -5882,8 +5876,7 @@ describe("named fragments", () => { owner { u { ... on I { - id1 - id2 + id } ...Fragment1 ...Fragment2 @@ -5901,7 +5894,7 @@ describe("named fragments", () => { fragment Fragment2 on T2 { ...Fragment4 - id1 + id } fragment Fragment3 on OItf { @@ -5909,8 +5902,7 @@ describe("named fragments", () => { } fragment Fragment4 on I { - id1 - id2 + id __typename } `); @@ -5938,8 +5930,7 @@ describe("named fragments", () => { fragment Fragment4 on I { __typename - id1 - id2 + id } }, } @@ -5950,8 +5941,7 @@ describe("named fragments", () => { owner { u { ... on I { - id1 - id2 + id } ...Fragment1 ...Fragment2 @@ -5969,7 +5959,7 @@ describe("named fragments", () => { fragment Fragment2 on T2 { ...Fragment4 - id1 + id } fragment Fragment3 on OItf { @@ -5977,8 +5967,7 @@ describe("named fragments", () => { } fragment Fragment4 on I { - id1 - id2 + id } `); @@ -6007,8 +5996,7 @@ describe("named fragments", () => { } fragment Fragment4 on I { - id1 - id2 + id } }, } @@ -6077,258 +6065,6 @@ describe("named fragments", () => { } `); }); - - test('can reuse fragments in subgraph where they only partially apply in root fetch', () => { - const subgraph1 = { - name: 'Subgraph1', - typeDefs: gql` - type Query { - t1: T - t2: T - } - - type T @key(fields: "id") { - id: ID! - v0: Int - v1: Int - v2: Int - } - ` - } - - const subgraph2 = { - name: 'Subgraph2', - typeDefs: gql` - type T @key(fields: "id") { - id: ID! - v3: Int - } - ` - } - - const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2); - const operation = operationFromDocument(api, gql` - { - t1 { - ...allTFields - } - t2 { - ...allTFields - } - } - - fragment allTFields on T { - v0 - v1 - v2 - v3 - } - `); - - const plan = queryPlanner.buildQueryPlan(operation); - expect(plan).toMatchInlineSnapshot(` - QueryPlan { - Sequence { - Fetch(service: "Subgraph1") { - { - t1 { - __typename - ...allTFields - id - } - t2 { - __typename - ...allTFields - id - } - } - - fragment allTFields on T { - v0 - v1 - v2 - } - }, - Parallel { - Flatten(path: "t1") { - Fetch(service: "Subgraph2") { - { - ... on T { - __typename - id - } - } => - { - ... on T { - v3 - } - } - }, - }, - Flatten(path: "t2") { - Fetch(service: "Subgraph2") { - { - ... on T { - __typename - id - } - } => - { - ... on T { - v3 - } - } - }, - }, - }, - }, - } - `); - }); - - test('can reuse fragments in subgraph where they only partially apply in entity fetch', () => { - const subgraph1 = { - name: 'Subgraph1', - typeDefs: gql` - type Query { - t: T - } - - type T @key(fields: "id") { - id: ID! - } - ` - } - - const subgraph2 = { - name: 'Subgraph2', - typeDefs: gql` - type T @key(fields: "id") { - id: ID! - u1: U - u2: U - - } - - type U @key(fields: "id") { - id: ID! - v0: Int - v1: Int - } - ` - } - - const subgraph3 = { - name: 'Subgraph3', - typeDefs: gql` - type U @key(fields: "id") { - id: ID! - v2: Int - v3: Int - } - ` - } - - const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2, subgraph3); - const operation = operationFromDocument(api, gql` - { - t { - u1 { - ...allUFields - } - u2 { - ...allUFields - } - } - } - - fragment allUFields on U { - v0 - v1 - v2 - v3 - } - `); - - const plan = queryPlanner.buildQueryPlan(operation); - expect(plan).toMatchInlineSnapshot(` - QueryPlan { - Sequence { - Fetch(service: "Subgraph1") { - { - t { - __typename - id - } - } - }, - Flatten(path: "t") { - Fetch(service: "Subgraph2") { - { - ... on T { - __typename - id - } - } => - { - ... on T { - u1 { - __typename - ...allUFields - id - } - u2 { - __typename - ...allUFields - id - } - } - } - - fragment allUFields on U { - v0 - v1 - } - }, - }, - Parallel { - Flatten(path: "t.u1") { - Fetch(service: "Subgraph3") { - { - ... on U { - __typename - id - } - } => - { - ... on U { - v2 - v3 - } - } - }, - }, - Flatten(path: "t.u2") { - Fetch(service: "Subgraph3") { - { - ... on U { - __typename - id - } - } => - { - ... on U { - v2 - v3 - } - } - }, - }, - }, - }, - } - `); - }); }) describe('`debug.maxEvaluatedPlans` configuration', () => { diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts index cafca8ede..6fdcf19b3 100644 --- a/query-planner-js/src/buildPlan.ts +++ b/query-planner-js/src/buildPlan.ts @@ -4541,7 +4541,7 @@ function inputsForRequire( assert(supergraphItfType && isInterfaceType(supergraphItfType), () => `Type ${entityType} should be an interface in the supergraph`); // Note: we are rebasing on another schema below, but we also known that we're working on a full expanded // selection set (no spread), so passing undefined is actually correct. - keyConditionAsInput = keyConditionAsInput.rebaseOn({ parentType: supergraphItfType, fragments: undefined, errorIfCannotRebase: true }); + keyConditionAsInput = keyConditionAsInput.rebaseOn(supergraphItfType, undefined); } fullSelectionSet.updates().add(keyConditionAsInput);