From f41936fb67afa73da55fd6047224e9488140bda0 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Tue, 18 Jul 2023 11:18:45 -0700 Subject: [PATCH] Revert "Revert #2639, an attempt to fix query fragment reuse (#2681)" This reverts commit b6be9f9650a69f6214d806d66b198729560da3dc. --- .../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 +- 5 files changed, 876 insertions(+), 83 deletions(-) diff --git a/gateway-js/src/__tests__/buildQueryPlan.test.ts b/gateway-js/src/__tests__/buildQueryPlan.test.ts index 02c6a900f..6d4160449 100644 --- a/gateway-js/src/__tests__/buildQueryPlan.test.ts +++ b/gateway-js/src/__tests__/buildQueryPlan.test.ts @@ -744,18 +744,21 @@ describe('buildQueryPlan', () => { it(`should not get confused by a fragment spread multiple times`, () => { const operationString = `#graphql - fragment Price on Product { + fragment PriceAndCountry on Product { price + details { + country + } } query { topProducts { __typename ... on Book { - ...Price + ...PriceAndCountry } ... on Furniture { - ...Price + ...PriceAndCountry } } } @@ -770,16 +773,20 @@ describe('buildQueryPlan', () => { topProducts { __typename ... on Book { - ...Price + ...PriceAndCountry } ... on Furniture { - ...Price + ...PriceAndCountry } } } - fragment Price on Product { + fragment PriceAndCountry 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 0d3f530bf..458629385 100644 --- a/internals-js/src/__tests__/operations.test.ts +++ b/internals-js/src/__tests__/operations.test.ts @@ -2758,3 +2758,405 @@ 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 61e5848ac..445ba095b 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -85,7 +85,11 @@ abstract class AbstractOperationElement> e abstract asPathElement(): string | undefined; - abstract rebaseOn(parentType: CompositeType): T; + abstract rebaseOn(args: { parentType: CompositeType, errorIfCannotRebase: boolean }): T | undefined; + + rebaseOnOrError(parentType: CompositeType): T { + return this.rebaseOn({ parentType, errorIfCannotRebase: true })!; + } abstract withUpdatedDirectives(newDirectives: readonly Directive[]): T; @@ -290,7 +294,7 @@ export class Field ex } } - rebaseOn(parentType: CompositeType): Field { + rebaseOn({ parentType, errorIfCannotRebase }: { parentType: CompositeType, errorIfCannotRebase: boolean }): Field | undefined { const fieldParent = this.definition.parent; if (parentType === fieldParent) { return this; @@ -300,12 +304,16 @@ 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); - validate(fieldDef, () => `Cannot add selection of field "${this.definition.coordinate}" to selection set of parent type "${parentType}" (that does not declare that field)`); + 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; + } + return this.withUpdatedDefinition(fieldDef); } @@ -466,7 +474,7 @@ export class FragmentElement extends AbstractOperationElement { return newFragment; } - rebaseOn(parentType: CompositeType): FragmentElement { + rebaseOn({ parentType, errorIfCannotRebase }: { parentType: CompositeType, errorIfCannotRebase: boolean }): FragmentElement | undefined { const fragmentParent = this.parentType; const typeCondition = this.typeCondition; if (parentType === fragmentParent) { @@ -477,10 +485,13 @@ 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); - validate( - canRebase, - () => `Cannot add fragment of condition "${typeCondition}" (runtimes: [${possibleRuntimeTypes(typeCondition!)}]) to parent type "${parentType}" (runtimes: ${possibleRuntimeTypes(parentType)})` - ); + if (!canRebase) { + validate( + !errorIfCannotRebase, + () => `Cannot add fragment of condition "${typeCondition}" (runtimes: [${possibleRuntimeTypes(typeCondition!)}]) to parent type "${parentType}" (runtimes: ${possibleRuntimeTypes(parentType)})` + ); + return undefined; + } return this.withUpdatedTypes(parentType, rebasedCondition); } @@ -697,7 +708,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()) { @@ -1232,7 +1243,7 @@ export class NamedFragmentDefinition extends DirectiveTargetElement { const rebasedType = schema.type(fragment.selectionSet.parentType.name); - 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. + if (!rebasedType || !isCompositeType(rebasedType)) { 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;; }); } @@ -1476,7 +1505,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, @@ -1515,6 +1544,20 @@ 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()) { @@ -1609,7 +1652,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: @@ -1759,14 +1802,25 @@ export class SelectionSet { return updated.isEmpty() ? undefined : updated; } - rebaseOn(parentType: CompositeType, fragments: NamedFragments | undefined): SelectionSet { + rebaseOn({ + parentType, + fragments, + errorIfCannotRebase, + }: { + parentType: CompositeType, + fragments: NamedFragments | undefined + errorIfCannotRebase: boolean, + }): SelectionSet { if (this.parentType === parentType) { return this; } const newSelections = new Map(); for (const selection of this.selections()) { - newSelections.set(selection.key(), selection.rebaseOn(parentType, fragments)); + const rebasedSelection = selection.rebaseOn({ parentType, fragments, errorIfCannotRebase }); + if (rebasedSelection) { + newSelections.set(selection.key(), rebasedSelection); + } } return new SelectionSet(parentType, newSelections); @@ -1790,15 +1844,25 @@ export class SelectionSet { return true; } - contains(that: SelectionSet): ContainsResult { + contains(that: SelectionSet, options?: { ignoreMissingTypename?: boolean }): ContainsResult { + const ignoreMissingTypename = options?.ignoreMissingTypename ?? false; if (that._selections.length > this._selections.length) { - return ContainsResult.NOT_CONTAINED; + // 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; + } } 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); + const selectionResult = thisSelection?.contains(thatSelection, options); if (selectionResult === undefined || selectionResult === ContainsResult.NOT_CONTAINED) { return ContainsResult.NOT_CONTAINED; } @@ -2164,10 +2228,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.rebaseOn(parentType, fragments); + return first.rebaseOnOrError({ parentType, fragments }); } - const element = updateElement(first).rebaseOn(parentType); + const element = updateElement(first).rebaseOnOrError(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. @@ -2215,7 +2279,7 @@ function makeSelectionSet(parentType: CompositeType, keyedUpdates: MultiMap { private computed: SelectionSet | undefined; @@ -2356,7 +2420,11 @@ 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: CompositeType, fragments: NamedFragments | undefined): FieldSelection { + rebaseOn({ + parentType, + fragments, + errorIfCannotRebase, + }: { + parentType: CompositeType, + fragments: NamedFragments | undefined, + errorIfCannotRebase: boolean, + }): FieldSelection | undefined { if (this.element.parentType === parentType) { return this; } - const rebasedElement = this.element.rebaseOn(parentType); + const rebasedElement = this.element.rebaseOn({ parentType, errorIfCannotRebase }); + if (!rebasedElement) { + return undefined; + } + if (!this.selectionSet) { return this.withUpdatedElement(rebasedElement); } @@ -2890,7 +2980,8 @@ 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`); - return this.withUpdatedComponents(rebasedElement, this.selectionSet.rebaseOn(rebasedBase, fragments)); + const rebasedSelectionSet = this.selectionSet.rebaseOn({ parentType: rebasedBase, fragments, errorIfCannotRebase }); + return rebasedSelectionSet.isEmpty() ? undefined : this.withUpdatedComponents(rebasedElement, rebasedSelectionSet); } /** @@ -2997,7 +3088,7 @@ export class FieldSelection extends AbstractSelection, undefined, Fie return !!that.selectionSet && this.selectionSet.equals(that.selectionSet); } - contains(that: Selection): ContainsResult { + contains(that: Selection, options?: { ignoreMissingTypename?: boolean }): ContainsResult { if (!(that instanceof FieldSelection) || !this.element.equals(that.element)) { return ContainsResult.NOT_CONTAINED; } @@ -3007,7 +3098,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); + return this.selectionSet.contains(that.selectionSet, options); } toString(expandFragments: boolean = true, indent?: string): string { @@ -3034,7 +3125,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); @@ -3044,14 +3135,14 @@ export abstract class FragmentSelection extends AbstractSelection `Cannot rebase ${this} if it isn't part of the provided fragments`); + // If we're rebasing on another schema (think a subgraph), then named fragments will have been rebased on that, and some + // of them may not contain anything that is on that subgraph, in which case they will not have been included at all. + // If so, then as long as we're not ask to error if we cannot rebase, then we're happy to skip that spread (since again, + // it expands to nothing that apply on the schema). + if (!namedFragment) { + validate(!errorIfCannotRebase, () => `Cannot rebase ${this.toString(false)} if it isn't part of the provided fragments`); + return undefined; + } return new FragmentSpreadSelection( parentType, newFragments, @@ -3497,7 +3617,7 @@ class FragmentSpreadSelection extends FragmentSelection { && sameDirectiveApplications(this.spreadDirectives, that.spreadDirectives); } - contains(that: Selection): ContainsResult { + contains(that: Selection, options?: { ignoreMissingTypename?: boolean }): ContainsResult { if (this.equals(that)) { return ContainsResult.EQUAL; } @@ -3506,7 +3626,7 @@ class FragmentSpreadSelection extends FragmentSelection { return ContainsResult.NOT_CONTAINED; } - return this.selectionSet.contains(that.selectionSet); + return this.selectionSet.contains(that.selectionSet, options); } 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 5a1d42690..39705355f 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -4002,7 +4002,8 @@ describe('Named fragments preservation', () => { } type V { - v: Int + v1: Int + v2: Int } ` @@ -4026,7 +4027,8 @@ describe('Named fragments preservation', () => { } fragment OnV on V { - v + v1 + v2 } `); @@ -4046,7 +4048,8 @@ describe('Named fragments preservation', () => { } fragment OnV on V { - v + v1 + v2 } }, } @@ -5856,16 +5859,19 @@ describe("named fragments", () => { union U = T1 | T2 interface I { - id: ID! + id1: ID! + id2: ID! } type T1 implements I { - id: ID! + id1: ID! + id2: ID! owner: Owner! } type T2 implements I { - id: ID! + id1: ID! + id2: ID! } ` } @@ -5876,7 +5882,8 @@ describe("named fragments", () => { owner { u { ... on I { - id + id1 + id2 } ...Fragment1 ...Fragment2 @@ -5894,7 +5901,7 @@ describe("named fragments", () => { fragment Fragment2 on T2 { ...Fragment4 - id + id1 } fragment Fragment3 on OItf { @@ -5902,7 +5909,8 @@ describe("named fragments", () => { } fragment Fragment4 on I { - id + id1 + id2 __typename } `); @@ -5930,7 +5938,8 @@ describe("named fragments", () => { fragment Fragment4 on I { __typename - id + id1 + id2 } }, } @@ -5941,7 +5950,8 @@ describe("named fragments", () => { owner { u { ... on I { - id + id1 + id2 } ...Fragment1 ...Fragment2 @@ -5959,7 +5969,7 @@ describe("named fragments", () => { fragment Fragment2 on T2 { ...Fragment4 - id + id1 } fragment Fragment3 on OItf { @@ -5967,7 +5977,8 @@ describe("named fragments", () => { } fragment Fragment4 on I { - id + id1 + id2 } `); @@ -5996,7 +6007,8 @@ describe("named fragments", () => { } fragment Fragment4 on I { - id + id1 + id2 } }, } @@ -6065,6 +6077,258 @@ 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 6fdcf19b3..cafca8ede 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(supergraphItfType, undefined); + keyConditionAsInput = keyConditionAsInput.rebaseOn({ parentType: supergraphItfType, fragments: undefined, errorIfCannotRebase: true }); } fullSelectionSet.updates().add(keyConditionAsInput);