From 1bb7c5129c7b07627ea33684b538fda8a83b8da8 Mon Sep 17 00:00:00 2001 From: Sylvain Lebresne Date: Thu, 6 Jul 2023 17:41:37 +0200 Subject: [PATCH] Fix normalization of fragment spreads (#2659) The code that "normalize" selection sets to remove unecessary fragments is also used to remove impossibe (and thus invalid) branches when some reused fragments gets expanded away due to only be used once. However, while the code for inline fragments was correctly removing impossible branches, the similar code for named spreads was not, which in some rare cases could result in an invalid subgraph fetch. --- .changeset/rotten-ants-clean.md | 10 ++ internals-js/src/__tests__/operations.test.ts | 92 +++++++++++++++++++ internals-js/src/operations.ts | 36 +++++--- 3 files changed, 124 insertions(+), 14 deletions(-) create mode 100644 .changeset/rotten-ants-clean.md diff --git a/.changeset/rotten-ants-clean.md b/.changeset/rotten-ants-clean.md new file mode 100644 index 000000000..f1f0f4a5b --- /dev/null +++ b/.changeset/rotten-ants-clean.md @@ -0,0 +1,10 @@ +--- +"@apollo/federation-internals": patch +--- + +Fix issue in the code to reuse fragments that, in some rare circumstances, could led to invalid queries where a named +spread was use in an invalid position. If triggered, this resulted in an subgraph fetch whereby a named spread was +used inside a sub-selection even though the spread condition did not intersect the parent type (the exact error message +would depend on the client library used to handle subgraph fetches, but with GraphQL-js, the error message had the +form "Fragment cannot be spread here as objects of type can never be of type "). + \ No newline at end of file diff --git a/internals-js/src/__tests__/operations.test.ts b/internals-js/src/__tests__/operations.test.ts index 455e2f3a0..f9c8298e5 100644 --- a/internals-js/src/__tests__/operations.test.ts +++ b/internals-js/src/__tests__/operations.test.ts @@ -1812,6 +1812,98 @@ describe('fragments optimization', () => { } `); }); + + test('when a spread inside an expanded fragment should be "normalized away"', () => { + const schema = parseSchema(` + type Query { + t1: T1 + i: I + } + + interface I { + id: ID! + } + + type T1 implements I { + id: ID! + a: Int + } + + type T2 implements I { + id: ID! + b: Int + c: Int + } + `); + const gqlSchema = schema.toGraphQLJSSchema(); + + const operation = parseOperation(schema, ` + { + t1 { + ...GetAll + } + i { + ...GetT2 + } + } + + fragment GetAll on I { + ... on T1 { + a + } + ...GetT2 + ... on T2 { + c + } + } + + fragment GetT2 on T2 { + b + } + `); + expect(validate(gqlSchema, parse(operation.toString()))).toStrictEqual([]); + + const withoutFragments = operation.expandAllFragments(); + expect(withoutFragments.toString()).toMatchString(` + { + t1 { + a + } + i { + ... on T2 { + b + } + } + } + `); + + // As we re-optimize, we will initially generated the initial query. But + // as we ask to only optimize fragments used more than once, the `GetAll` + // fragment will be re-expanded (`GetT2` will not because the code will say + // that it is used both in the expanded `GetAll` but also inside `i`). + // But because `GetAll` is within `t1: T1`, that expansion should actually + // get rid of anything `T2`-related. + // This test exists because a previous version of the code was not correctly + // "getting rid" of the `...GetT2` spread, keeping in the query, which is + // invalid (we cannot have `...GetT2` inside `t1`). + const optimized = withoutFragments.optimize(operation.fragments!, 2); + expect(validate(gqlSchema, parse(optimized.toString()))).toStrictEqual([]); + + expect(optimized.toString()).toMatchString(` + fragment GetT2 on T2 { + b + } + + { + t1 { + a + } + i { + ...GetT2 + } + } + `); + }); }); test('does not leave unused fragments', () => { diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index e80e68935..9487760c6 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -3137,6 +3137,26 @@ export abstract class FragmentSelection extends AbstractSelection typeRuntimes.includes(t))) { + return undefined; + } + } + + return this.normalizeKnowingItIntersects({ parentType, recursive }); + } + + protected abstract normalizeKnowingItIntersects({ parentType, recursive }: { parentType: CompositeType, recursive? : boolean }): FragmentSelection | SelectionSet | undefined; } class InlineFragmentSelection extends FragmentSelection { @@ -3317,21 +3337,9 @@ class InlineFragmentSelection extends FragmentSelection { : this.withUpdatedComponents(newElement, newSelection); } - normalize({ parentType, recursive }: { parentType: CompositeType, recursive? : boolean }): FragmentSelection | SelectionSet | undefined { + protected normalizeKnowingItIntersects({ parentType, recursive }: { parentType: CompositeType, recursive? : boolean }): FragmentSelection | SelectionSet | undefined { const thisCondition = this.element.typeCondition; - // This method assumes by contract that `parentType` runtimes intersects `this.parentType`'s, but `parentType` - // runtimes may be a subset. So first check if the selection should not be discarded on that account (that - // is, we should not keep the selection if its condition runtimes don't intersect at all with those of - // `parentType` as that would ultimately make an invalid selection set). - if (thisCondition && parentType !== this.parentType) { - const conditionRuntimes = possibleRuntimeTypes(thisCondition); - const typeRuntimes = possibleRuntimeTypes(parentType); - if (!conditionRuntimes.some((t) => typeRuntimes.includes(t))) { - return undefined; - } - } - // We know the condition is "valid", but it may not be useful. That said, if the condition has directives, // we preserve the fragment no matter what. if (this.element.appliedDirectives.length === 0) { @@ -3472,7 +3480,7 @@ class FragmentSpreadSelection extends FragmentSelection { assert(false, `Unsupported`); } - normalize({ parentType }: { parentType: CompositeType }): FragmentSelection { + normalizeKnowingItIntersects({ parentType }: { parentType: CompositeType }): FragmentSelection { // We must update the spread parent type if necessary since we're not going deeper, // or we'll be fundamentally losing context. assert(parentType.schema() === this.parentType.schema(), 'Should not try to normalize using a type from another schema');