From 7f1ef73ee00b82c9b4b1bbfd23f3be10e3a1e176 Mon Sep 17 00:00:00 2001 From: Sylvain Lebresne Date: Tue, 13 Jun 2023 16:58:52 +0200 Subject: [PATCH] Fix issues with named fragment handling code (#2619) The naming of `trimUnsatisfiableBranches` was misleading because it does not just "remove unsatisfiable branches", it more generally remove things that are not-very-optimal (even if they are not full branches). So this rename this method to `normalize`, which more clearly explains then intent (on top of being shorter). Additionally, the method was not actually removing unsatisfiable branches in all cases, which made it a bit error prone to use in practice and led to some small inefficiencies in the `NamedFragmentDefinition.selectionAtType` method. So fixed that, making the method more regular and fixing the aforementioned inefficiency. This also force a normalisation after `expandAllFragments` because we were relying on it in practice so this is less fragile this way. Also, the QP code never truly rely on operation validation as it is assumed the operation passed to it have already been validated prior to being passed to the planner, but having the added validation helps a bit in other testing context when that pre-validation hasn't happened, and since this is fairly cheap... Last but not least, the code for normalizing fields was not taking into account that if we normalize on a more precise type, this may make a field definition "more precise", which can impacts the type we should use to normalize the sub-selection of said field. --- .changeset/nice-cheetahs-yell.md | 10 + internals-js/src/__tests__/operations.test.ts | 598 +++++++++++++-- internals-js/src/definitions.ts | 4 + internals-js/src/operations.ts | 724 ++++++++++++------ internals-js/src/types.ts | 23 +- .../src/__tests__/allFeatures.test.ts | 2 + .../src/__tests__/buildPlan.test.ts | 65 +- .../features/basic/build-query-plan.feature | 10 - query-planner-js/src/buildPlan.ts | 21 +- 9 files changed, 1140 insertions(+), 317 deletions(-) create mode 100644 .changeset/nice-cheetahs-yell.md diff --git a/.changeset/nice-cheetahs-yell.md b/.changeset/nice-cheetahs-yell.md new file mode 100644 index 000000000..b794c9d24 --- /dev/null +++ b/.changeset/nice-cheetahs-yell.md @@ -0,0 +1,10 @@ +--- +"@apollo/query-planner": patch +"@apollo/federation-internals": patch +--- + +Fix issues in code to reuse named fragments. One of the fixed issue would manifest as an assertion error with a message +looking like `Cannot add fragment of condition X (...) to parent type Y (...)`. Another would manifest itself by +generating an invalid subgraph fetch where a field conflicts with another version of that field that is in a reused +named fragment. + \ 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 0df428d13..aad443716 100644 --- a/internals-js/src/__tests__/operations.test.ts +++ b/internals-js/src/__tests__/operations.test.ts @@ -1,12 +1,14 @@ import { defaultRootName, + isCompositeType, Schema, SchemaRootKind, } from '../../dist/definitions'; import { buildSchema } from '../../dist/buildSchema'; -import { MutableSelectionSet, Operation, operationFromDocument, parseOperation } from '../../dist/operations'; +import { MutableSelectionSet, NamedFragmentDefinition, Operation, operationFromDocument, parseOperation, SelectionSetAtType } from '../../dist/operations'; import './matchers'; -import { DocumentNode, FieldNode, GraphQLError, Kind, OperationDefinitionNode, OperationTypeNode, SelectionNode, SelectionSetNode } from 'graphql'; +import { DocumentNode, FieldNode, GraphQLError, Kind, OperationDefinitionNode, OperationTypeNode, parse, SelectionNode, SelectionSetNode, validate } from 'graphql'; +import { assert } from '../utils'; function parseSchema(schema: string): Schema { try { @@ -44,11 +46,7 @@ describe('fragments optimization', () => { expanded: string, }) { const operation = parseOperation(schema, query); - // We call `trimUnsatisfiableBranches` because the selections we care about in the query planner - // will effectively have had gone through that function (and even if that function wasn't called, - // the query planning algorithm would still end up removing unsatisfiable branches anyway), so - // it is a more interesting test. - const withoutFragments = operation.expandAllFragments().trimUnsatisfiableBranches(); + const withoutFragments = operation.expandAllFragments(); expect(withoutFragments.toString()).toMatchString(expanded); @@ -120,7 +118,7 @@ describe('fragments optimization', () => { } `); - const withoutFragments = parseOperation(schema, operation.toString(true, true)); + const withoutFragments = operation.expandAllFragments(); expect(withoutFragments.toString()).toMatchString(` { t { @@ -132,22 +130,18 @@ describe('fragments optimization', () => { x y } - ... on I { - b - } + b u { - ... on U { - ... on I { - b - } - ... on T1 { - a - b - } - ... on T2 { - x - y - } + ... on I { + b + } + ... on T1 { + a + b + } + ... on T2 { + x + y } } } @@ -256,7 +250,7 @@ describe('fragments optimization', () => { } `); - const withoutFragments = parseOperation(schema, operation.toString(true, true)); + const withoutFragments = operation.expandAllFragments(); expect(withoutFragments.toString()).toMatchString(` { t { @@ -264,10 +258,8 @@ describe('fragments optimization', () => { a b me { - ... on I { - b - c - } + b + c } } ... on T2 { @@ -275,35 +267,31 @@ describe('fragments optimization', () => { y } u1 { - ... on U { - ... on I { - b - c - } - ... on T1 { - a - b - } - ... on T2 { - x - y - } + ... on I { + b + c + } + ... on T1 { + a + b + } + ... on T2 { + x + y } } u2 { - ... on U { - ... on I { - b - c - } - ... on T1 { - a - b - } - ... on T2 { - x - y - } + ... on I { + b + c + } + ... on T1 { + a + b + } + ... on T2 { + x + y } } } @@ -1073,6 +1061,346 @@ describe('fragments optimization', () => { }); }); }); + + describe('does not generate invalid operations', () => { + test('due to conflict between selection and reused fragment', () => { + const schema = parseSchema(` + type Query { + t1: T1 + i: I + } + + interface I { + id: ID! + } + + interface WithF { + f(arg: Int): Int + } + + type T1 implements I { + id: ID! + f(arg: Int): Int + } + + type T2 implements I & WithF { + id: ID! + f(arg: Int): Int + } + `); + const gqlSchema = schema.toGraphQLJSSchema(); + + const operation = parseOperation(schema, ` + query { + t1 { + id + f(arg: 0) + } + i { + ...F1 + } + } + + fragment F1 on I { + id + ... on WithF { + f(arg: 1) + } + } + `); + expect(validate(gqlSchema, parse(operation.toString()))).toStrictEqual([]); + + const withoutFragments = operation.expandAllFragments(); + expect(withoutFragments.toString()).toMatchString(` + { + t1 { + id + f(arg: 0) + } + i { + id + ... on WithF { + f(arg: 1) + } + } + } + `); + + // Note that technically, `t1` has return type `T1` which is a `I`, so `F1` can be spread + // within `t1`, and `t1 { ...F1 }` is just `t1 { id }` (because `T!` does not implement `WithF`), + // so that it would appear that it could be valid to optimize this query into: + // { + // t1 { + // ...F1 // Notice the use of F1 here, which does expand to `id` in this context + // f(arg: 0) + // } + // i { + // ...F1 + // } + // } + // And while doing this may look "dumb" in that toy example (we're replacing `id` with `...F1` + // which is longer so less optimal really), it's easy to expand this to example where re-using + // `F1` this way _does_ make things smaller. + // + // But the query above is actually invalid. And it is invalid because the validation of graphQL + // does not take into account the fact that the `... on WithF` part of `F1` is basically dead + // code within `t1`. And so it finds a conflict between `f(arg: 0)` and the `f(arg: 1)` in `F1` + // (even though, again, the later is statically known to never apply, but graphQL does not + // include such static analysis in its validation). + // + // And so this test does make sure we do not generate the query above (do not use `F1` in `t1`). + const optimized = withoutFragments.optimize(operation.fragments!, 1); + expect(validate(gqlSchema, parse(optimized.toString()))).toStrictEqual([]); + + expect(optimized.toString()).toMatchString(` + fragment F1 on I { + id + ... on WithF { + f(arg: 1) + } + } + + { + t1 { + id + f(arg: 0) + } + i { + ...F1 + } + } + `); + }); + + test('due to conflict between the active selection of a reused fragment and the trimmed part of another fragments', () => { + const schema = parseSchema(` + type Query { + t1: T1 + i: I + } + + interface I { + id: ID! + } + + interface WithF { + f(arg: Int): Int + } + + type T1 implements I { + id: ID! + f(arg: Int): Int + } + + type T2 implements I & WithF { + id: ID! + f(arg: Int): Int + } + `); + const gqlSchema = schema.toGraphQLJSSchema(); + + const operation = parseOperation(schema, ` + query { + t1 { + id + ...F1 + } + i { + ...F2 + } + } + + fragment F1 on T1 { + f(arg: 0) + } + + fragment F2 on I { + id + ... on WithF { + f(arg: 1) + } + } + + `); + expect(validate(gqlSchema, parse(operation.toString()))).toStrictEqual([]); + + const withoutFragments = operation.expandAllFragments(); + expect(withoutFragments.toString()).toMatchString(` + { + t1 { + id + f(arg: 0) + } + i { + id + ... on WithF { + f(arg: 1) + } + } + } + `); + + // See the comments on the previous test. The only different here is that `F1` is applied + // first, and then we need to make sure we do not apply `F2` even though it's restriction + // inside `t1` matches its selection set. + const optimized = withoutFragments.optimize(operation.fragments!, 1); + expect(validate(gqlSchema, parse(optimized.toString()))).toStrictEqual([]); + + expect(optimized.toString()).toMatchString(` + fragment F1 on T1 { + f(arg: 0) + } + + fragment F2 on I { + id + ... on WithF { + f(arg: 1) + } + } + + { + t1 { + ...F1 + id + } + i { + ...F2 + } + } + `); + }); + + test('due to conflict between the trimmed parts of 2 fragments', () => { + const schema = parseSchema(` + type Query { + t1: T1 + i1: I + i2: I + } + + interface I { + id: ID! + a: Int + b: Int + } + + interface WithF { + f(arg: Int): Int + } + + type T1 implements I { + id: ID! + a: Int + b: Int + f(arg: Int): Int + } + + type T2 implements I & WithF { + id: ID! + a: Int + b: Int + f(arg: Int): Int + } + `); + const gqlSchema = schema.toGraphQLJSSchema(); + + const operation = parseOperation(schema, ` + query { + t1 { + id + a + b + } + i1 { + ...F1 + } + i2 { + ...F2 + } + } + + fragment F1 on I { + id + a + ... on WithF { + f(arg: 0) + } + } + + fragment F2 on I { + id + b + ... on WithF { + f(arg: 1) + } + } + + `); + expect(validate(gqlSchema, parse(operation.toString()))).toStrictEqual([]); + + const withoutFragments = operation.expandAllFragments(); + expect(withoutFragments.toString()).toMatchString(` + { + t1 { + id + a + b + } + i1 { + id + a + ... on WithF { + f(arg: 0) + } + } + i2 { + id + b + ... on WithF { + f(arg: 1) + } + } + } + `); + + // Here, `F1` in `T1` reduces to `{ id a }` and F2 reduces to `{ id b }`, so theoretically both could be used + // within the first `T1` branch. But they can't both be used because their `... on WithF` part conflict, + // and even though that part is dead in `T1`, this would still be illegal graphQL. + const optimized = withoutFragments.optimize(operation.fragments!, 1); + expect(validate(gqlSchema, parse(optimized.toString()))).toStrictEqual([]); + + expect(optimized.toString()).toMatchString(` + fragment F1 on I { + id + a + ... on WithF { + f(arg: 0) + } + } + + fragment F2 on I { + id + b + ... on WithF { + f(arg: 1) + } + } + + { + t1 { + ...F1 + b + } + i1 { + ...F1 + } + i2 { + ...F2 + } + } + `); + }); + }); }); describe('validations', () => { @@ -1414,8 +1742,8 @@ describe('unsatisfiable branches removal', () => { } `); - const withoutUnsatisfiableBranches = (op: string) => { - return parseOperation(schema, op).trimUnsatisfiableBranches().toString(false, false) + const normalized = (op: string) => { + return parseOperation(schema, op).normalize().toString(false, false) }; @@ -1423,7 +1751,7 @@ describe('unsatisfiable branches removal', () => { '{ i { a } }', '{ i { ... on T1 { a b c } } }', ])('is identity if there is no unsatisfiable branches', (op) => { - expect(withoutUnsatisfiableBranches(op)).toBe(op); + expect(normalized(op)).toBe(op); }); it.each([ @@ -1432,6 +1760,160 @@ describe('unsatisfiable branches removal', () => { { input: '{ i { ... on I { a ... on T2 { d } } } }', output: '{ i { a ... on T2 { d } } }' }, { input: '{ i { ... on T2 { ... on I { a ... on J { b } } } } }', output: '{ i { ... on T2 { a } } }' }, ])('removes unsatisfiable branches', ({input, output}) => { - expect(withoutUnsatisfiableBranches(input)).toBe(output); + expect(normalized(input)).toBe(output); + }); +}); + +describe('named fragment selection set restrictions at type', () => { + const expandAtType = (frag: NamedFragmentDefinition, schema: Schema, typeName: string): SelectionSetAtType => { + const type = schema.type(typeName); + assert(type && isCompositeType(type), `Invalid type ${typeName}`) + // `expandedSelectionSetAtType` assumes it's argument passes `canApplyAtType`, so let's make sure we're + // not typo-ing something in our tests. + assert(frag.canApplyDirectlyAtType(type), `${frag.name} cannot be applied at type ${typeName}`); + return frag.expandedSelectionSetAtType(type); + } + + test('for fragment on interfaces', () => { + const schema = parseSchema(` + type Query { + t1: I1 + } + + interface I1 { + x: Int + } + + interface I2 { + x: Int + } + + interface I3 { + x: Int + } + + interface I4 { + x: Int + } + + type T1 implements I1 & I2 & I4 { + x: Int + } + + type T2 implements I1 & I3 & I4 { + x: Int + } + `); + + const operation = parseOperation(schema, ` + { + t1 { + ...FonI1 + } + } + + fragment FonI1 on I1 { + x + ... on T1 { + x + } + ... on T2 { + x + } + ... on I2 { + x + } + ... on I3 { + x + } + } + `); + + const frag = operation.fragments?.get('FonI1')!; + + let { selectionSet, trimmed } = expandAtType(frag, schema, 'I1'); + expect(selectionSet.toString()).toBe('{ x ... on T1 { x } ... on T2 { x } ... on I2 { x } ... on I3 { x } }'); + expect(trimmed?.toString()).toBeUndefined(); + + ({ selectionSet, trimmed } = expandAtType(frag, schema, 'T1')); + expect(selectionSet.toString()).toBe('{ x }'); + // Note: one could remark that having `... on T1 { x }` in `trimmed` below is a tad weird: this is + // because in this case `normalized` removed that fragment and so when we do `normalized.mins(original)`, + // then it shows up. It a tad difficult to avoid this however and it's ok for what we do (`trimmed` + // is used to check for field conflict and and the only reason we use `trimmed` is to make things faster + // but we could use the `original` instead). + expect(trimmed?.toString()).toBe('{ ... on T1 { x } ... on T2 { x } ... on I2 { x } ... on I3 { x } }'); + }); + + test('for fragment on unions', () => { + const schema = parseSchema(` + type Query { + t1: U1 + } + + union U1 = T1 | T2 + union U2 = T1 + union U3 = T2 + union U4 = T1 | T2 + + type T1 { + x: Int + y: Int + } + + type T2 { + z: Int + w: Int + } + `); + + const operation = parseOperation(schema, ` + { + t1 { + ...FonU1 + } + } + + fragment FonU1 on U1 { + ... on T1 { + x + } + ... on T2 { + z + } + ... on U2 { + ... on T1 { + y + } + } + ... on U3 { + ... on T2 { + w + } + } + } + `); + + const frag = operation.fragments?.get('FonU1')!; + + // Note that with unions, the fragments inside the unions can be "lifted" and so they everyting normalize to just the + // possible runtimes. + + let { selectionSet, trimmed } = expandAtType(frag, schema, 'U1'); + expect(selectionSet.toString()).toBe('{ ... on T1 { x y } ... on T2 { z w } }'); + expect(trimmed?.toString()).toBeUndefined(); + + ({ selectionSet, trimmed } = expandAtType(frag, schema, 'U2')); + expect(selectionSet.toString()).toBe('{ ... on T1 { x y } }'); + expect(trimmed?.toString()).toBe('{ ... on T2 { z w } }'); + + ({ selectionSet, trimmed } = expandAtType(frag, schema, 'U3')); + expect(selectionSet.toString()).toBe('{ ... on T2 { z w } }'); + expect(trimmed?.toString()).toBe('{ ... on T1 { x y } }'); + + ({ selectionSet, trimmed } = expandAtType(frag, schema, 'T1')); + expect(selectionSet.toString()).toBe('{ x y }'); + // Similar remarks that on interfaces + expect(trimmed?.toString()).toBe('{ ... on T1 { x y } ... on T2 { z w } }'); }); }); diff --git a/internals-js/src/definitions.ts b/internals-js/src/definitions.ts index b860b436e..6d3db3224 100644 --- a/internals-js/src/definitions.ts +++ b/internals-js/src/definitions.ts @@ -240,6 +240,10 @@ export function possibleRuntimeTypes(type: CompositeType): readonly ObjectType[] } export function runtimeTypesIntersects(t1: CompositeType, t2: CompositeType): boolean { + if (t1 === t2) { + return true; + } + const rt1 = possibleRuntimeTypes(t1); const rt2 = possibleRuntimeTypes(t2); for (const obj1 of rt1) { diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index a29f70bfe..d6045f990 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -47,10 +47,11 @@ import { Variables, isObjectType, NamedType, + isUnionType, } from "./definitions"; import { isInterfaceObjectType } from "./federation"; import { ERRORS } from "./error"; -import { isSubtype, sameType } from "./types"; +import { isSubtype, sameType, typesCanBeMerged } from "./types"; import { assert, mapKeys, mapValues, MapWithCachedArrays, MultiMap, SetMultiMap } from "./utils"; import { argumentsEquals, argumentsFromAST, isValidValue, valueToAST, valueToString } from "./values"; import { v1 as uuidv1 } from 'uuid'; @@ -115,7 +116,7 @@ export class Field ex constructor( readonly definition: FieldDefinition, - private readonly args?: TArgs, + readonly args?: TArgs, directives?: readonly Directive[], readonly alias?: string, ) { @@ -865,69 +866,81 @@ export class Operation { readonly name?: string) { } - optimize(fragments?: NamedFragments, minUsagesToOptimize: number = 2): Operation { - assert(minUsagesToOptimize >= 1, `Expected 'minUsagesToOptimize' to be at least 1, but got ${minUsagesToOptimize}`) - if (!fragments || fragments.isEmpty()) { - return this; - } - - let optimizedSelection = this.selectionSet.optimize(fragments); - if (optimizedSelection === this.selectionSet) { - return this; - } - - const finalFragments = computeFragmentsToKeep(optimizedSelection, fragments, minUsagesToOptimize); - if (finalFragments === null || finalFragments?.size === fragments.size) { - // This means either that there is no fragment usage whatsoever in `optimizedSelection`, or that - // we're keeping all fragments. In both cases, we need no additional work on `optimizedSelection`. - return new Operation(this.schema, this.rootKind, optimizedSelection, this.variableDefinitions, finalFragments ?? undefined, this.name); - } - - // If we get here, it means some fragments need to be expanded, so we do so. - // Optimizing all fragments to potentially re-expand some is not entirely optimal, but it's unclear - // how to do otherwise, and it probably don't matter too much in practice (we only call this optimization - // on the final computed query plan, so not a very hot path; plus in most cases we won't even reach that - // point either because there is no fragment, or none will have been optimized away so we'll exit above). - optimizedSelection = optimizedSelection.expandFragments(finalFragments); - - // Expanding fragments could create some "inefficiencies" that we wouldn't have if we hadn't re-optimized - // the fragments to de-optimize it later, so we do a final "trim" pass to remove those. - optimizedSelection = optimizedSelection.trimUnsatisfiableBranches(optimizedSelection.parentType); - return new Operation(this.schema, this.rootKind, optimizedSelection, this.variableDefinitions, finalFragments, this.name); - } - - expandAllFragments(): Operation { - const expandedSelections = this.selectionSet.expandFragments(); - if (expandedSelections === this.selectionSet) { + // Returns a copy of this operation with the provided updated selection set. + // Note that this method assumes that the existing `this.fragments` is still appropriate. + private withUpdatedSelectionSet(newSelectionSet: SelectionSet): Operation { + if (this.selectionSet === newSelectionSet) { return this; } return new Operation( this.schema, this.rootKind, - expandedSelections, + newSelectionSet, this.variableDefinitions, - undefined, + this.fragments, this.name ); } - trimUnsatisfiableBranches(): Operation { - const trimmedSelections = this.selectionSet.trimUnsatisfiableBranches(this.selectionSet.parentType); - if (trimmedSelections === this.selectionSet) { + // Returns a copy of this operation with the provided updated selection set and fragments. + private withUpdatedSelectionSetAndFragments(newSelectionSet: SelectionSet, newFragments: NamedFragments | undefined): Operation { + if (this.selectionSet === newSelectionSet && newFragments === this.fragments) { return this; } return new Operation( this.schema, this.rootKind, - trimmedSelections, + newSelectionSet, this.variableDefinitions, - this.fragments, + newFragments, this.name ); } + optimize(fragments?: NamedFragments, minUsagesToOptimize: number = 2): Operation { + assert(minUsagesToOptimize >= 1, `Expected 'minUsagesToOptimize' to be at least 1, but got ${minUsagesToOptimize}`) + if (!fragments || fragments.isEmpty()) { + return this; + } + + let optimizedSelection = this.selectionSet.optimize(fragments); + if (optimizedSelection === this.selectionSet) { + return this; + } + + const finalFragments = computeFragmentsToKeep(optimizedSelection, fragments, minUsagesToOptimize); + + // If there is fragment usages and we're not keeping all fragments, we need to expand fragments. + if (finalFragments !== null && finalFragments?.size !== fragments.size) { + // Note that optimizing all fragments to potentially re-expand some is not entirely optimal, but it's unclear + // how to do otherwise, and it probably don't matter too much in practice (we only call this optimization + // on the final computed query plan, so not a very hot path; plus in most cases we won't even reach that + // point either because there is no fragment, or none will have been optimized away so we'll exit above). + optimizedSelection = optimizedSelection.expandFragments(finalFragments); + + // Expanding fragments could create some "inefficiencies" that we wouldn't have if we hadn't re-optimized + // the fragments to de-optimize it later, so we do a final "normalize" pass to remove those. + optimizedSelection = optimizedSelection.normalize({ parentType: optimizedSelection.parentType }); + } + + return this.withUpdatedSelectionSetAndFragments(optimizedSelection, finalFragments ?? undefined); + } + + expandAllFragments(): Operation { + // We clear up the fragments since we've expanded all. + // Also note that expanding fragment usually generate unecessary fragments/inefficient selections, so it + // basically always make sense to normalize afterwards. Besides, fragment reuse (done by `optimize`) rely + // on the fact that its input is normalized to work properly, so all the more reason to do it here. + const expanded = this.selectionSet.expandFragments(); + return this.withUpdatedSelectionSetAndFragments(expanded.normalize({ parentType: expanded.parentType }), undefined); + } + + normalize(): Operation { + return this.withUpdatedSelectionSet(this.selectionSet.normalize({ parentType: this.selectionSet.parentType })); + } + /** * Returns this operation but potentially modified so all/some of the @defer applications have been removed. * @@ -936,10 +949,7 @@ export class Operation { * applications are removed. */ withoutDefer(labelsToRemove?: Set): Operation { - const updated = this.selectionSet.withoutDefer(labelsToRemove); - return updated == this.selectionSet - ? this - : new Operation(this.schema, this.rootKind, updated, this.variableDefinitions, this.fragments, this.name); + return this.withUpdatedSelectionSet(this.selectionSet.withoutDefer(labelsToRemove)); } /** @@ -965,8 +975,7 @@ export class Operation { const { hasDefers, hasNonLabelledOrConditionalDefers } = normalizer.init(this.selectionSet); let updatedOperation: Operation = this; if (hasNonLabelledOrConditionalDefers) { - const updated = this.selectionSet.withNormalizedDefer(normalizer); - updatedOperation = new Operation(this.schema, this.rootKind, updated, this.variableDefinitions, this.fragments, this.name); + updatedOperation = this.withUpdatedSelectionSet(this.selectionSet.withNormalizedDefer(normalizer)); } return { operation: updatedOperation, @@ -991,6 +1000,8 @@ export class Operation { } } +export type SelectionSetAtType = { selectionSet: SelectionSet, trimmed?: SelectionSet }; + export class NamedFragmentDefinition extends DirectiveTargetElement { private _selectionSet: SelectionSet | undefined; @@ -1000,7 +1011,7 @@ export class NamedFragmentDefinition extends DirectiveTargetElement | undefined; private _includedFragmentNames: Set | undefined; - private readonly expandedSelectionSetsAtTypesCache = new Map(); + private readonly expandedSelectionSetsAtTypesCache = new Map(); constructor( schema: Schema, @@ -1027,7 +1038,7 @@ export class NamedFragmentDefinition extends DirectiveTargetElement= typeRuntimes.length - && typeRuntimes.every((t1) => conditionRuntimes.some((t2) => sameType(t1, t2))); + if (conditionRuntimes.length < typeRuntimes.length + || !typeRuntimes.every((t1) => conditionRuntimes.some((t2) => sameType(t1, t2)))) { + return false; + } + + return isObjectType(type) || isUnionType(this.typeCondition); } /** - * This methods *assumes* that `this.canApplyAtType(type)` is `true` (and may crash if this is not true), and returns + * This methods *assumes* that `this.canApplyDirectlyAtType(type)` is `true` (and may crash if this is not true), and returns * a version fo this named fragment selection set that corresponds to the "expansion" of this named fragment at `type` * * The overall idea here is that if we have an interface I with 2 implementations T1 and T2, and we have a fragment like: @@ -1117,34 +1150,35 @@ export class NamedFragmentDefinition extends DirectiveTargetElement(); @@ -1208,8 +1243,8 @@ export class NamedFragments { } } - maybeApplyingAtType(type: CompositeType): NamedFragmentDefinition[] { - return this.fragments.values().filter(f => f.canApplyAtType(type)); + maybeApplyingDirectlyAtType(type: CompositeType): NamedFragmentDefinition[] { + return this.fragments.values().filter(f => f.canApplyDirectlyAtType(type)); } get(name: string): NamedFragmentDefinition | undefined { @@ -1288,7 +1323,7 @@ export class NamedFragments { mapper: (selectionSet: SelectionSet) => SelectionSet | undefined, ): NamedFragments | undefined { return this.mapInDependencyOrder((fragment, newFragments) => { - const mappedSelectionSet = mapper(fragment.selectionSet.expandFragments().trimUnsatisfiableBranches(fragment.typeCondition)); + const mappedSelectionSet = mapper(fragment.selectionSet.expandFragments().normalize({ parentType: fragment.typeCondition })); if (!mappedSelectionSet) { return undefined; } @@ -1463,6 +1498,22 @@ export class SelectionSet { return fields; } + fieldsByResponseName(): MultiMap { + const byResponseName = new MultiMap(); + this.collectFieldsByResponseName(byResponseName); + return byResponseName; + } + + private collectFieldsByResponseName(collector: MultiMap) { + for (const selection of this.selections()) { + if (selection.kind === 'FieldSelection') { + collector.add(selection.element.responseName(), selection); + } else { + selection.selectionSet.collectFieldsByResponseName(collector); + } + } + } + usedVariables(): Variables { const collector = new VariableCollector(); this.collectVariables(collector); @@ -1523,8 +1574,82 @@ export class SelectionSet { return this.lazyMap((selection) => selection.expandFragments(updatedFragments)); } - trimUnsatisfiableBranches(parentType: CompositeType, options?: { recursive? : boolean }): SelectionSet { - return this.lazyMap((selection) => selection.trimUnsatisfiableBranches(parentType, options), { 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: + * ```graphql + * type Query { + * t1: T1 + * i: I + * } + * + * interface I { + * id: ID! + * } + * + * type T1 implements I { + * id: ID! + * v1: Int + * } + * + * type T2 implements I { + * id: ID! + * v2: Int + * } + * ``` + * + * ``` + * normalize({ + * t1 { + * ... on I { + * id + * } + * } + * i { + * ... on T1 { + * ... on I { + * ... on T1 { + * v1 + * } + * ... on T2 { + * v2 + * } + * } + * } + * ... on T2 { + * ... on I { + * id + * } + * } + * } + * }) === { + * t1 { + * id + * } + * i { + * ... on T1 { + * v1 + * } + * ... on T2 { + * id + * } + * } + * } + * ``` + * + * For this operation to be valid (to not throw), `parentType` must be such this selection set would + * be valid as a subselection of an inline fragment `... on parentType { }` (and + * so `this.normalize(this.parentType)` is always valid and useful, but it is possible to pass a `parentType` + * that is more "restrictive" than the selection current parent type). + * + * Passing the option `recursive == false` makes the normalization only apply at the top-level, removing + * any unecessary top-level inline fragments, possibly multiple layers of them, but we never recurse + * inside the sub-selection of an selection that is not removed by the normalization. + */ + normalize({ parentType, recursive }: { parentType: CompositeType, recursive? : boolean }): SelectionSet { + return this.lazyMap((selection) => selection.normalize({ parentType, recursive }), { parentType }); } /** @@ -1575,17 +1700,26 @@ export class SelectionSet { } /** - * Returns the selection select from filtering out any selection that does not match the provided predicate. + * Returns the selection set resulting from filtering out any of the top-level selection that does not match the provided predicate. * - * Please that this method will expand *ALL* fragments as the result of applying it's filtering. You should - * call `optimize` on the result if you want to re-apply some fragments. + * Please that this method does not recurse within sub-selections. */ filter(predicate: (selection: Selection) => boolean): SelectionSet { - return this.lazyMap((selection) => selection.filter(predicate)); + return this.lazyMap((selection) => predicate(selection) ? selection : undefined); + } + + /** + * Returns the selection set resulting from "recursively" filtering any selection that does not match the provided predicate. + * This method calls `predicate` on every selection of the selection set, not just top-level ones, and apply a "depth-first" + * strategy, meaning that when the predicate is call on a given selection, the it is guaranteed that filtering has happened + * on all the selections of its sub-selection. + */ + filterRecursiveDepthFirst(predicate: (selection: Selection) => boolean): SelectionSet { + return this.lazyMap((selection) => selection.filterRecursiveDepthFirst(predicate)); } withoutEmptyBranches(): SelectionSet | undefined { - const updated = this.filter((selection) => selection.selectionSet?.isEmpty() !== true); + const updated = this.filterRecursiveDepthFirst((selection) => selection.selectionSet?.isEmpty() !== true); return updated.isEmpty() ? undefined : updated; } @@ -1640,51 +1774,6 @@ export class SelectionSet { : ContainsResult.STRICTLY_CONTAINED; } - // Please note that this method assumes that `candidate.canApplyAtType(parentType) === true` but it is left to the caller to - // validate this (`canApplyAtType` is not free, and we want to avoid repeating it multiple times). - diffWithNamedFragmentIfContained( - candidate: NamedFragmentDefinition, - parentType: CompositeType, - fragments: NamedFragments, - ): { contains: boolean, diff?: SelectionSet } { - const that = candidate.expandedSelectionSetAtType(parentType); - // It's possible that while the fragment technically applies at `parentType`, it's "rebasing" on - // `parentType` is empty, or contains only `__typename`. For instance, suppose we have - // a union `U = A | B | C`, and then a fragment: - // ```graphql - // fragment F on U { - // ... on A { - // x - // } - // ... on b { - // y - // } - // } - // ``` - // It is then possible to apply `F` when the parent type is `C`, but this ends up selecting - // nothing at all. - // - // Returning `contains: true` in those cases is, while not 100% incorrect, at least not productive, - // and so we skip right away in that case. This is essentially an optimisation. - if (that.isEmpty() || (that.selections().length === 1 && that.selections()[0].isTypenameField())) { - return { contains: false }; - } - - if (this.contains(that)) { - // One subtlety here is that at "this" sub-selections may already have been optimized with some fragments. It's - // usually ok because `candidate` will also use those fragments, but one fragments that `candidate` can never be - // using is itself (the `contains` check is fine with this, but it's harder to deal in `minus`). So we expand - // the candidate we're currently looking at in "this" to avoid some issues. - let updatedThis = this.expandFragments(fragments.filter((f) => f.name !== candidate.name)); - if (updatedThis !== this) { - updatedThis = updatedThis.trimUnsatisfiableBranches(parentType); - } - const diff = updatedThis.minus(that); - return { contains: true, diff: diff.isEmpty() ? undefined : diff }; - } - return { contains: false }; - } - /** * Returns a selection set that correspond to this selection set but where any of the selections in the * provided selection set have been remove. @@ -1706,6 +1795,28 @@ export class SelectionSet { return updated.toSelectionSet(this.parentType); } + intersectionWith(that: SelectionSet): SelectionSet { + if (this.isEmpty()) { + return this; + } + if (that.isEmpty()) { + return that; + } + + const intersection = new SelectionSetUpdates(); + for (const [key, thisSelection] of this._keyedSelections) { + const thatSelection = that._keyedSelections.get(key); + if (thatSelection) { + const selection = thisSelection.intersectionWith(thatSelection); + if (selection) { + intersection.add(selection); + } + } + } + + return intersection.toSelectionSet(this.parentType); + } + canRebaseOn(parentTypeToTest: CompositeType): boolean { return this.selections().every((selection) => selection.canAddTo(parentTypeToTest)); } @@ -1923,6 +2034,19 @@ export class SelectionSetUpdates { toSelectionSet(parentType: CompositeType, fragments?: NamedFragments): SelectionSet { return makeSelectionSet(parentType, this.keyedUpdates, fragments); } + + toString() { + return '{\n' + + [...this.keyedUpdates.entries()].map(([k, updates]) => { + const updStr = updates.map((upd) => + upd instanceof AbstractSelection + ? upd.toString() + : `${upd.path} -> ${upd.selections}` + ); + return ` - ${k}: ${updStr}`; + }).join('\n') + +'\n\}' + } } function addToKeyedUpdates(keyedUpdates: MultiMap, selections: Selection | SelectionSet | readonly Selection[]) { @@ -2240,7 +2364,7 @@ abstract class AbstractSelection boolean, }): SelectionSet | NamedFragmentDefinition { - let candidates = fragments.maybeApplyingAtType(parentType); + // We limit to fragments whose selection could be applied "directly" at `parentType`, meaning without taking the fragment condition + // into account. The idea being that if the fragment condition would be needed inside `parentType`, then that condition will not + // have been "normalized away" and so we want for this very call to be called on the fragment whose type _is_ the fragment condition (at + // which point, this `maybeApplyingDirectlyAtType` method will apply. + // Also note that this is because we have this restriction that calling `expandedSelectionSetAtType` is ok. + let candidates = fragments.maybeApplyingDirectlyAtType(parentType); // First, we check which of the candidates do apply inside `subSelection`, if any. // If we find a candidate that applies to the whole `subSelection`, then we stop and only return // that one candidate. Otherwise, we cumulate in `applyingFragments` the list of fragments that // applies to a subset of `subSelection`. - const applyingFragments: NamedFragmentDefinition[] = []; + const applyingFragments: { fragment: NamedFragmentDefinition, atType: SelectionSetAtType }[] = []; for (const candidate of candidates) { - const fragmentSSet = candidate.expandedSelectionSetAtType(parentType); + const atType = candidate.expandedSelectionSetAtType(parentType); + const selectionSetAtType = atType.selectionSet; // It's possible that while the fragment technically applies at `parentType`, it's "rebasing" on // `parentType` is empty, or contains only `__typename`. For instance, suppose we have // a union `U = A | B | C`, and then a fragment: @@ -2296,11 +2441,11 @@ abstract class AbstractSelection !applyingFragments.some((o) => o.includes(f.name))); + const filteredApplyingFragments = applyingFragments.filter(({ fragment }) => !applyingFragments.some((o) => o.fragment.includes(fragment.name))) let notCoveredByFragments = subSelection; const optimized = new SelectionSetUpdates(); - // TODO: doing repeated calls to `minus` for every fragment is simple, but a `minusAll` method that - // takes the fragment selections at once would be more efficient in pratice. - for (const fragment of filteredApplyingFragments) { - // Note: we call `expandedSelectionSetAType` twice in this method for the applying fragments, but - // we know it's cached so rely on that fact. - notCoveredByFragments = notCoveredByFragments.minus(fragment.expandedSelectionSetAtType(parentType)); + const addedTrimmedParts: SelectionSet[] = []; + for (const { fragment, atType} of filteredApplyingFragments) { + // At this point, we known that the fragment, restricted to the current parent type, matches a subset of the + // sub-selection. However, there is still one case we we cannot use it that we need to check, and this is + // if using the fragment would create a field "conflict" (in the sense of the graphQL spec + // [`FieldsInSetCanMerge`](https://spec.graphql.org/draft/#FieldsInSetCanMerge())) and thus create an + // invalid selection. To be clear, `atType.selectionSet` cannot create a conflict, since it is a subset + // of `subSelection` and `subSelection` is valid. *But* there may be some part of the fragment that + // is not `atType.selectionSet` due to being "dead branches" for type `parentType`. And while those + // branches _are_ "dead" as far as execution goes, the `FieldsInSetCanMerge` validation does not take + // this into account (it's 1st step says "including visiting fragments and inline fragments" but has + // no logic regarding ignoring any fragment that may not apply due to the intersection of runtimes + // between multiple fragment being empty). + const notCovered = subSelection.minus(atType.selectionSet); + const trimmed = atType.trimmed; + if (trimmed) { + // We need to make sure the trimmed parts of `fragment` merges with the rest of the selection, + // but also that it merge with any of the trimmed parts of any fragment we have added already. + // Note: this last condition means that if 2 fragment conflict on their "trimmed" parts, + // then the choice of which is used is based on the fragment ordering, which may not be optimal. + // This feels niche enough that we keep it simple for now, but we can revise this decision if + // we run into real cases that justify it (but note that making it optimal would be a tad involved + // in general, as in theory you could have complex dependencies of fragments that conflict, + // even cycles, and you need to take the size of fragments into account to know what's best; + // and even then, this could even depend on overall usage, as it can be better ot reuse a + // fragment that is used in other places, than to use one for which it's the only usage (even + // if it save more in theory)). + if (!(selectionSetsDoMerge(notCovered, trimmed) && addedTrimmedParts.every((t) => selectionSetsDoMerge(t, trimmed)))) { + continue; + } + addedTrimmedParts.push(trimmed); + } + notCoveredByFragments = notCoveredByFragments.intersectionWith(notCovered); optimized.add(new FragmentSpreadSelection(parentType, fragments, fragment, [])); } @@ -2386,6 +2558,76 @@ abstract class AbstractSelection, undefined, FieldSelection> { readonly kind = 'FieldSelection' as const; @@ -2409,6 +2651,9 @@ export class FieldSelection extends AbstractSelection, undefined, Fie } withUpdatedComponents(field: Field, selectionSet: SelectionSet | undefined): FieldSelection { + if (this.element === field && this.selectionSet === selectionSet) { + return this; + } return new FieldSelection(field, selectionSet); } @@ -2442,19 +2687,19 @@ export class FieldSelection extends AbstractSelection, undefined, Fie // Then, recurse inside the field sub-selection (note that if we matched some fragments above, // this recursion will "ignore" those as `FragmentSpreadSelection.optimize()` is a no-op). - optimizedSelection = optimizedSelection.optimize(fragments); + optimizedSelection = optimizedSelection.optimizeSelections(fragments); return this.selectionSet === optimizedSelection ? this : this.withUpdatedSelectionSet(optimizedSelection); } - filter(predicate: (selection: Selection) => boolean): FieldSelection | undefined { + filterRecursiveDepthFirst(predicate: (selection: Selection) => boolean): FieldSelection | undefined { if (!this.selectionSet) { return predicate(this) ? this : undefined; } - const updatedSelectionSet = this.selectionSet.filter(predicate); + const updatedSelectionSet = this.selectionSet.filterRecursiveDepthFirst(predicate); const thisWithFilteredSelectionSet = this.selectionSet === updatedSelectionSet ? this : new FieldSelection(this.element, updatedSelectionSet); @@ -2547,28 +2792,41 @@ export class FieldSelection extends AbstractSelection, undefined, Fie return !!this.selectionSet?.hasDefer(); } - trimUnsatisfiableBranches(_: CompositeType, options?: { recursive? : boolean }): FieldSelection { + normalize({ parentType, recursive }: { parentType: CompositeType, recursive? : boolean }): FieldSelection { + // This could be an interface field, and if we're normalizing on one of the implementation of that + // interface, we want to make sure we use the field of the implementation, as it may in particular + // have a more specific type which should propagate to the recursive call to normalize. + + const definition = parentType === this.parentType + ? this.element.definition + : parentType.field(this.element.name); + assert(definition, `Cannot normalize ${this.element} at ${parentType} which does not have that field`) + + const element = this.element.definition === definition ? this.element : this.element.withUpdatedDefinition(definition); if (!this.selectionSet) { - return this; + return this.withUpdatedElement(element); } - const base = this.element.baseType() - assert(isCompositeType(base), () => `Field ${this.element} should not have a sub-selection`); - const trimmed = (options?.recursive ?? true) ? this.mapToSelectionSet((s) => s.trimUnsatisfiableBranches(base)) : this; + const base = element.baseType(); + assert(isCompositeType(base), () => `Field ${element} should not have a sub-selection`); + const normalizedSubSelection = (recursive ?? true) ? this.selectionSet.normalize({ parentType: base }) : this.selectionSet; // In rare caes, it's possible that everything in the sub-selection was trimmed away and so the // sub-selection is empty. Which suggest something may be wrong with this part of the query // intent, but the query was valid while keeping an empty sub-selection isn't. So in that // case, we just add some "non-included" __typename field just to keep the query valid. - if (trimmed.selectionSet?.isEmpty()) { - return trimmed.withUpdatedSelectionSet(selectionSetOfElement( - new Field( - base.typenameField()!, - undefined, - [new Directive('include', { 'if': false })], + if (normalizedSubSelection?.isEmpty()) { + return this.withUpdatedComponents( + element, + selectionSetOfElement( + new Field( + base.typenameField()!, + undefined, + [new Directive('include', { 'if': false })], + ) ) - )); + ); } else { - return trimmed; + return this.withUpdatedComponents(element, normalizedSubSelection); } } @@ -2628,11 +2886,10 @@ export abstract class FragmentSelection extends AbstractSelection boolean): FragmentSelection | undefined { + filterRecursiveDepthFirst(predicate: (selection: Selection) => boolean): FragmentSelection | undefined { // Note that we essentially expand all fragments as part of this. - const selectionSet = this.selectionSet; - const updatedSelectionSet = selectionSet.filter(predicate); - const thisWithFilteredSelectionSet = updatedSelectionSet === selectionSet + const updatedSelectionSet = this.selectionSet.filterRecursiveDepthFirst(predicate); + const thisWithFilteredSelectionSet = updatedSelectionSet === this.selectionSet ? this : new InlineFragmentSelection(this.element, updatedSelectionSet); @@ -2665,6 +2922,9 @@ class InlineFragmentSelection extends FragmentSelection { } withUpdatedComponents(fragment: FragmentElement, selectionSet: SelectionSet): InlineFragmentSelection { + if (fragment === this.element && selectionSet === this.selectionSet) { + return this; + } return new InlineFragmentSelection(fragment, selectionSet); } @@ -2742,7 +3002,7 @@ class InlineFragmentSelection extends FragmentSelection { // some, then: // 1. all it's directives should also be on the current element. // 2. the directives of this element should be the fragment condition. - // because if those 2 conditions are true, we cna replace the whole current inline fragment + // because if those 2 conditions are true, we can replace the whole current inline fragment // with the match spread and directives will still match. return fragment.appliedDirectives.length === 0 || ( @@ -2809,55 +3069,60 @@ class InlineFragmentSelection extends FragmentSelection { : this.withUpdatedComponents(newElement, newSelection); } - trimUnsatisfiableBranches(currentType: CompositeType, options?: { recursive? : boolean }): FragmentSelection | SelectionSet | undefined { - const recursive = options?.recursive ?? true; - + normalize({ parentType, recursive }: { parentType: CompositeType, recursive? : boolean }): FragmentSelection | SelectionSet | undefined { const thisCondition = this.element.typeCondition; - // Note that if the condition has directives, we preserve the fragment no matter what. - if (this.element.appliedDirectives.length === 0) { - if (!thisCondition || currentType === this.element.typeCondition) { - const trimmed = this.selectionSet.trimUnsatisfiableBranches(currentType, options); - return trimmed.isEmpty() ? undefined : trimmed; - } - // If the current type is an object, then we never need to keep the current fragment because: - // - either the fragment is also an object, but we've eliminated the case where the 2 types are the same, - // so this is just an unsatisfiable branch. - // - or it's not an object, but then the current type is more precise and no point in "casting" to a - // less precise interface/union. And if the current type is not even a valid runtime of said interface/union, - // then we should completely ignore the branch (or, since we're eliminating `thisCondition`, we would be - // building an invalid selection). - if (isObjectType(currentType)) { - if (isObjectType(thisCondition) || !possibleRuntimeTypes(thisCondition).includes(currentType)) { - return undefined; - } else { - const trimmed = this.selectionSet.trimUnsatisfiableBranches(currentType, options); - return trimmed.isEmpty() ? undefined : trimmed; - } + // 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; } } - // As we preserve the current fragment, the rest is about recursing. If we don't recurse, we're done - if (!recursive) { - return this; + // 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) { + // There is a number of cases where a fragment is not useful: + // 1. if there is not conditions (remember it also has no directives). + // 2. if it's the same type as the current type: it's not restricting types further. + // 3. if the current type is an object more generally: because in that case too the condition + // cannot be restricting things further (it's typically a less precise interface/union). + if (!thisCondition || parentType === this.element.typeCondition || isObjectType(parentType)) { + const normalized = this.selectionSet.normalize({ parentType, recursive }); + return normalized.isEmpty() ? undefined : normalized; + } } - // In all other cases, we first recurse on the sub-selection. - const trimmedSelectionSet = this.selectionSet.trimUnsatisfiableBranches(this.element.typeCondition ?? this.parentType); + // We preserve the current fragment, so we only recurse within the sub-selection if we're asked to be recusive. + // (note that even if we're not recursive, we may still have some "lifting" to do) + let normalizedSelectionSet: SelectionSet; + if (recursive ?? true) { + normalizedSelectionSet = this.selectionSet.normalize({ parentType: thisCondition ?? parentType }); - // First, could be that everything was unsatisfiable. - if (trimmedSelectionSet.isEmpty()) { - if (this.element.appliedDirectives.length === 0) { - return undefined; - } else { - return this.withUpdatedSelectionSet(selectionSetOfElement( - new Field( - (this.element.typeCondition ?? this.parentType).typenameField()!, - undefined, - [new Directive('include', { 'if': false })], - ) - )); + // It could be that everything was unsatisfiable. + if (normalizedSelectionSet.isEmpty()) { + if (this.element.appliedDirectives.length === 0) { + return undefined; + } else { + return this.withUpdatedComponents( + this.element.rebaseOn(parentType), + selectionSetOfElement( + new Field( + (this.element.typeCondition ?? parentType).typenameField()!, + undefined, + [new Directive('include', { 'if': false })], + ) + ) + ); + } } + } else { + normalizedSelectionSet = this.selectionSet; } // Second, we check if some of the sub-selection fragments can be "lifted" outside of this fragment. This can happen if: @@ -2865,10 +3130,10 @@ class InlineFragmentSelection extends FragmentSelection { // 2. the sub-fragment is an object type, // 3. the sub-fragment type is a valid runtime of the current type. if (this.element.appliedDirectives.length === 0 && isAbstractType(thisCondition!)) { - assert(!isObjectType(currentType), () => `Should not have got here if ${currentType} is an object type`); - const currentRuntimes = possibleRuntimeTypes(currentType); + assert(!isObjectType(parentType), () => `Should not have got here if ${parentType} is an object type`); + const currentRuntimes = possibleRuntimeTypes(parentType); const liftableSelections: Selection[] = []; - for (const selection of trimmedSelectionSet.selections()) { + for (const selection of normalizedSelectionSet.selections()) { if (selection.kind === 'FragmentSelection' && selection.element.typeCondition && isObjectType(selection.element.typeCondition) @@ -2879,8 +3144,8 @@ class InlineFragmentSelection extends FragmentSelection { } // If we can lift all selections, then that just mean we can get rid of the current fragment altogether - if (liftableSelections.length === trimmedSelectionSet.selections().length) { - return trimmedSelectionSet; + if (liftableSelections.length === normalizedSelectionSet.selections().length) { + return normalizedSelectionSet; } // Otherwise, if there is "liftable" selections, we must return a set comprised of those lifted selection, @@ -2889,13 +3154,15 @@ class InlineFragmentSelection extends FragmentSelection { const newSet = new SelectionSetUpdates(); newSet.add(liftableSelections); newSet.add(this.withUpdatedSelectionSet( - trimmedSelectionSet.filter((s) => !liftableSelections.includes(s)), + normalizedSelectionSet.filter((s) => !liftableSelections.includes(s)), )); - return newSet.toSelectionSet(this.parentType); + return newSet.toSelectionSet(parentType); } } - return this.selectionSet === trimmedSelectionSet ? this : this.withUpdatedSelectionSet(trimmedSelectionSet); + return this.parentType === parentType && this.selectionSet === normalizedSelectionSet + ? this + : this.withUpdatedComponents(this.element.rebaseOn(parentType), normalizedSelectionSet); } expandFragments(updatedFragments: NamedFragments | undefined): FragmentSelection { @@ -2956,17 +3223,20 @@ class FragmentSpreadSelection extends FragmentSelection { assert(false, `Unsupported`); } - trimUnsatisfiableBranches(parentType: CompositeType): FragmentSelection { + normalize({ 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 trim using a type from another schema'); + assert(parentType.schema() === this.parentType.schema(), 'Should not try to normalize using a type from another schema'); return this.rebaseOn(parentType, this.fragments); } validate(): void { this.validateDeferAndStream(); - // We don't do anything else because fragment definition are validated when created. + validate( + runtimeTypesIntersects(this.parentType, this.namedFragment.typeCondition), + () => `Fragment "${this.namedFragment.name}" cannot be spread inside type ${this.parentType} as the runtime types do not intersect ${this.namedFragment.typeCondition}` + ); } toSelectionNode(): FragmentSpreadNode { diff --git a/internals-js/src/types.ts b/internals-js/src/types.ts index 4df6dd443..862459e4b 100644 --- a/internals-js/src/types.ts +++ b/internals-js/src/types.ts @@ -3,8 +3,9 @@ */ import { - AbstractType, + AbstractType, InterfaceType, + isCompositeType, isInterfaceType, isListType, isNamedType, @@ -144,3 +145,23 @@ export function isStrictSubtype( && isSubtype(type.ofType, maybeSubType, allowedRules, unionMembershipTester, implementsInterfaceTester); } } + +/** + * This essentially follows the beginning of https://spec.graphql.org/draft/#SameResponseShape(). + * That is, the types cannot be merged unless: + * - they have the same nullability and "list-ability", potentially recursively. + * - their base type is either both composite, or are the same type. + */ +export function typesCanBeMerged(t1: Type, t2: Type): boolean { + if (isNonNullType(t1)) { + return isNonNullType(t2) ? typesCanBeMerged(t1.ofType, t2.ofType) : false; + } + if (isListType(t1)) { + return isListType(t2) ? typesCanBeMerged(t1.ofType, t2.ofType) : false; + } + if (isCompositeType(t1)) { + return isCompositeType(t2); + } + return sameType(t1, t2); +} + diff --git a/query-planner-js/src/__tests__/allFeatures.test.ts b/query-planner-js/src/__tests__/allFeatures.test.ts index 375f63e86..46997ffd3 100644 --- a/query-planner-js/src/__tests__/allFeatures.test.ts +++ b/query-planner-js/src/__tests__/allFeatures.test.ts @@ -10,6 +10,7 @@ import { buildSchema, parseOperation, } from '@apollo/federation-internals'; +import { parse, validate } from 'graphql'; // This test looks over all directories under tests/features and finds "supergraphSdl.graphql" in @@ -52,6 +53,7 @@ for (const directory of directories) { const givenQuery = () => { given(/^query$/im, (operationString: string) => { operation = parseOperation(schema, operationString); + expect(validate(schema.toGraphQLJSSchema(), parse(operationString))).toStrictEqual([]); }); }; diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index 3ccd3e702..d10d856d8 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -5759,7 +5759,7 @@ test('does not error out handling fragments when interface subtyping is involved `); }); -describe("named fragments reuse", () => { +describe("named fragments", () => { test('handles mix of fragments indirection and unions', () => { const subgraph1 = { name: 'Subgraph1', @@ -6002,6 +6002,69 @@ describe("named fragments reuse", () => { } `); }); + + test('handles fragments with interface field subtyping', () => { + const subgraph1 = { + name: 'Subgraph1', + typeDefs: gql` + type Query { + t1: T1! + } + + interface I { + id: ID! + other: I! + } + + type T1 implements I { + id: ID! + other: T1! + } + + + type T2 implements I + { + id: ID! + other: T2! + } + ` + } + + const [api, queryPlanner] = composeAndCreatePlanner(subgraph1); + const operation = operationFromDocument(api, gql` + { + t1 { + ...Fragment1 + } + } + + fragment Fragment1 on I { + other { + ... on T1 { + id + } + ... on T2 { + id + } + } + } + `); + + const plan = queryPlanner.buildQueryPlan(operation); + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Fetch(service: "Subgraph1") { + { + t1 { + other { + id + } + } + } + }, + } + `); + }); }) describe('`debug.maxEvaluatedPlans` configuration', () => { diff --git a/query-planner-js/src/__tests__/features/basic/build-query-plan.feature b/query-planner-js/src/__tests__/features/basic/build-query-plan.feature index b743ce3c5..f6852b663 100644 --- a/query-planner-js/src/__tests__/features/basic/build-query-plan.feature +++ b/query-planner-js/src/__tests__/features/basic/build-query-plan.feature @@ -914,13 +914,6 @@ Scenario: deduplicates fields / selections regardless of adjacency and type cond """ query { body { - ... on Image { - ... on Text { - attributes { - bold - } - } - } ... on Body { ... on Text { attributes { @@ -964,9 +957,6 @@ Scenario: deduplicates fields / selections regardless of adjacency and type cond query { body { - ... on Image { - ...TextFragment - } ... on Body { ...TextFragment } diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts index e06c78332..c0d413361 100644 --- a/query-planner-js/src/buildPlan.ts +++ b/query-planner-js/src/buildPlan.ts @@ -31,7 +31,6 @@ import { NamedFragments, operationToDocument, MapWithCachedArrays, - sameType, FederationMetadata, federationMetadata, entitiesFieldName, @@ -51,7 +50,6 @@ import { mapValues, isInterfaceObjectType, isInterfaceType, - isNonNullType, Type, MutableSelectionSet, SelectionSetUpdates, @@ -60,6 +58,7 @@ import { InterfaceType, FragmentSelection, possibleRuntimeTypes, + typesCanBeMerged, } from "@apollo/federation-internals"; import { advanceSimultaneousPathsWithOperation, @@ -1539,23 +1538,6 @@ function genAliasName(baseName: string, unavailableNames: Map): str return candidate; } -function typesCanBeMerged(t1: Type, t2: Type): boolean { - // This essentially follows the beginning of https://spec.graphql.org/draft/#SameResponseShape(). - // That is, the types cannot be merged unless: - // - they have the same nullability and "list-ability", potentially recursively. - // - their base type is either both composite, or are the same type. - if (isNonNullType(t1)) { - return isNonNullType(t2) ? typesCanBeMerged(t1.ofType, t2.ofType) : false; - } - if (isListType(t1)) { - return isListType(t2) ? typesCanBeMerged(t1.ofType, t2.ofType) : false; - } - if (isCompositeType(t1)) { - return isCompositeType(t2); - } - return sameType(t1, t2); -} - type SelectionSetAtPath = { path: string[], selections: SelectionSet, @@ -2927,7 +2909,6 @@ export class QueryPlanner { // going to expand everything during the algorithm anyway. We'll re-optimize subgraph fetches with fragments // later if possible (which is why we saved them above before expansion). operation = operation.expandAllFragments(); - operation = operation.trimUnsatisfiableBranches(); operation = withoutIntrospection(operation); operation = this.withSiblingTypenameOptimizedAway(operation);