diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index f108f4395..2cd978de7 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -951,7 +951,6 @@ export class SelectionSet extends Freezable { if (names && names.length === 0) { return this; } - const newFragments = updateSelectionSetFragments ? (names ? this.fragments?.without(names) : undefined) : this.fragments; @@ -1605,6 +1604,8 @@ export abstract class FragmentSelection extends Freezable { abstract withNormalizedDefer(normalizer: DeferNormalizer): FragmentSelection | SelectionSet; + abstract updateForAddingTo(selectionSet: SelectionSet): FragmentSelection; + protected us(): FragmentSelection { return this; } @@ -1624,30 +1625,6 @@ export abstract class FragmentSelection extends Freezable { return mergeVariables(this.element().variables(), this.selectionSet.usedVariables()); } - updateForAddingTo(selectionSet: SelectionSet): FragmentSelection { - const updatedFragment = this.element().updateForAddingTo(selectionSet); - if (this.element() === updatedFragment) { - return this.cloneIfFrozen(); - } - - // Like for fields, we create a new selection that not only uses the updated fragment, but also ensures - // the underlying selection set uses the updated type as parent type. - const updatedCastedType = updatedFragment.castedType(); - let updatedSelectionSet : SelectionSet | undefined; - if (this.selectionSet.parentType !== updatedCastedType) { - updatedSelectionSet = new SelectionSet(updatedCastedType); - // Note that re-adding every selection ensures that anything frozen will be cloned as needed, on top of handling any knock-down - // effect of the type change. - for (const selection of this.selectionSet.selections()) { - updatedSelectionSet.add(selection); - } - } else { - updatedSelectionSet = this.selectionSet?.cloneIfFrozen(); - } - - return new InlineFragmentSelection(updatedFragment, updatedSelectionSet); - } - filter(predicate: (selection: Selection) => boolean): FragmentSelection | undefined { if (!predicate(this)) { return undefined; @@ -1713,6 +1690,31 @@ class InlineFragmentSelection extends FragmentSelection { this.selectionSet.validate(); } + updateForAddingTo(selectionSet: SelectionSet): FragmentSelection { + const updatedFragment = this.element().updateForAddingTo(selectionSet); + if (this.element() === updatedFragment) { + return this.cloneIfFrozen(); + } + + // Like for fields, we create a new selection that not only uses the updated fragment, but also ensures + // the underlying selection set uses the updated type as parent type. + const updatedCastedType = updatedFragment.castedType(); + let updatedSelectionSet : SelectionSet | undefined; + if (this.selectionSet.parentType !== updatedCastedType) { + updatedSelectionSet = new SelectionSet(updatedCastedType); + // Note that re-adding every selection ensures that anything frozen will be cloned as needed, on top of handling any knock-down + // effect of the type change. + for (const selection of this.selectionSet.selections()) { + updatedSelectionSet.add(selection); + } + } else { + updatedSelectionSet = this.selectionSet?.cloneIfFrozen(); + } + + return new InlineFragmentSelection(updatedFragment, updatedSelectionSet); + } + + get selectionSet(): SelectionSet { return this._selectionSet; } @@ -1785,7 +1787,6 @@ class InlineFragmentSelection extends FragmentSelection { if (updatedSubSelections === this.selectionSet && !hasDeferToRemove) { return this; } - const newFragment = hasDeferToRemove ? this.fragmentElement.withoutDefer() : this.fragmentElement; if (!newFragment) { return updatedSubSelections; @@ -1877,6 +1878,16 @@ class FragmentSpreadSelection extends FragmentSelection { return this; } + updateForAddingTo(_selectionSet: SelectionSet): FragmentSelection { + // This is a little bit iffy, because the fragment could link to a schema (typically the supergraph API one) + // that is different from the one of `_selectionSet` (say, a subgraph fetch selection in which we're trying to + // reuse a user fragment). But in practice, we expand all fragments when we do query planning and only re-add + // fragments back at the very end, so this should be fine. Importantly, we don't want this method to mistakenly + // expand the spread, as that would compromise the code that optimize subgraph fetches to re-use named + // fragments. + return this; + } + expandFragments(names?: string[], updateSelectionSetFragments: boolean = true): FragmentSelection | readonly Selection[] { if (names && !names.includes(this.namedFragment.name)) { return this; diff --git a/query-planner-js/CHANGELOG.md b/query-planner-js/CHANGELOG.md index 40084f4c3..5b3fb4915 100644 --- a/query-planner-js/CHANGELOG.md +++ b/query-planner-js/CHANGELOG.md @@ -2,6 +2,9 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The Federation v0.x equivalent for this package can be found [here](https://github.com/apollographql/federation/blob/version-0.x/query-planner-js/CHANGELOG.md) on the `version-0.x` branch of this repo. + +- Fix issue with fragment reusing code something mistakenly re-expanding fragments [PR #2098](https://github.com/apollographql/federation/pull/2098). + ## 2.1.0-alpha.4 - Update peer dependency `graphql` to `^16.5.0` to use `GraphQLErrorOptions` [PR #2060](https://github.com/apollographql/federation/pull/2060) diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index 7da042f76..8b9fbcd1b 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -3723,4 +3723,70 @@ describe('Named fragments preservation', () => { expect(plan).toMatchInlineSnapshot(reuseQueryFragments ? withReuse : withoutReuse); }); + + it('works with nested fragments when only the nested fragment gets preserved', () => { + const subgraph1 = { + name: 'Subgraph1', + typeDefs: gql` + type Query { + t: T + } + + type T @key(fields : "id") { + id: ID! + a: V + b: V + } + + type V { + v: Int + } + + ` + } + + const [api, queryPlanner] = composeAndCreatePlanner(subgraph1); + let operation = operationFromDocument(api, gql` + { + t { + ...OnT + } + } + + fragment OnT on T { + a { + ...OnV + } + b { + ...OnV + } + } + + fragment OnV on V { + v + } + `); + + const plan = queryPlanner.buildQueryPlan(operation); + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Fetch(service: "Subgraph1") { + { + t { + a { + ...OnV + } + b { + ...OnV + } + } + } + + fragment OnV on V { + v + } + }, + } + `); + }); });