diff --git a/.changeset/invalid-reused-fragment.md b/.changeset/invalid-reused-fragment.md new file mode 100644 index 000000000..6fc78c270 --- /dev/null +++ b/.changeset/invalid-reused-fragment.md @@ -0,0 +1,7 @@ +--- +"@apollo/query-planner": patch +"@apollo/composition": patch +"@apollo/federation-internals": patch +--- + +Fix a query planning bug where invalid subgraph queries are generated with `reuseQueryFragments` set true. ([#2952](https://github.com/apollographql/federation/issues/2952)) diff --git a/internals-js/src/__tests__/operations.test.ts b/internals-js/src/__tests__/operations.test.ts index fe6f9fee2..7e34d83d5 100644 --- a/internals-js/src/__tests__/operations.test.ts +++ b/internals-js/src/__tests__/operations.test.ts @@ -3100,6 +3100,38 @@ describe('named fragment selection set restrictions at type', () => { return frag.expandedSelectionSetAtType(type); } + test('for fragment on object types', () => { + const schema = parseSchema(` + type Query { + t1: T1 + } + + type T1 { + x: Int + y: Int + } + `); + + const operation = parseOperation(schema, ` + { + t1 { + ...FonT1 + } + } + + fragment FonT1 on T1 { + x + y + } + `); + + const frag = operation.fragments?.get('FonT1')!; + + const { selectionSet, validator } = expandAtType(frag, schema, 'T1'); + expect(selectionSet.toString()).toBe('{ x y }'); + expect(validator?.toString()).toBeUndefined(); + }); + test('for fragment on interfaces', () => { const schema = parseSchema(` type Query { @@ -3159,10 +3191,24 @@ describe('named fragment selection set restrictions at type', () => { let { selectionSet, validator } = expandAtType(frag, schema, 'I1'); expect(selectionSet.toString()).toBe('{ x ... on T1 { x } ... on T2 { x } ... on I2 { x } ... on I3 { x } }'); - expect(validator?.toString()).toBeUndefined(); + // Note: Due to `FieldsInSetCanMerge` rule, we can't use trimmed validators for + // fragments on non-object types. + expect(validator?.toString()).toMatchString(` + { + x: [ + I1.x + T1.x + T2.x + I2.x + I3.x + ] + } + `); ({ selectionSet, validator } = expandAtType(frag, schema, 'T1')); expect(selectionSet.toString()).toBe('{ x }'); + // Note: Fragments on non-object types always have the full validator and thus + // results in the same validator regardless of the type they are expanded at. // Note: one could remark that having `T1.x` in the `validator` below is a tad weird: this is // because in this case `normalized` removed that fragment and so when we do `normalized.minus(original)`, // then it shows up. It a tad difficult to avoid this however and it's ok for what we do (`validator` @@ -3170,6 +3216,7 @@ describe('named fragment selection set restrictions at type', () => { expect(validator?.toString()).toMatchString(` { x: [ + I1.x T1.x T2.x I2.x @@ -3239,8 +3286,16 @@ describe('named fragment selection set restrictions at type', () => { // this happens due to the "lifting" of selection mentioned above, is a bit hard to avoid, // and is essentially harmess (it may result in a bit more cpu cycles in some cases but // that is likely negligible). + // Note: Due to `FieldsInSetCanMerge` rule, we can't use trimmed validators for + // fragments on non-object types. expect(validator?.toString()).toMatchString(` { + x: [ + T1.x + ] + z: [ + T2.z + ] y: [ T1.y ] @@ -3252,8 +3307,13 @@ describe('named fragment selection set restrictions at type', () => { ({ selectionSet, validator } = expandAtType(frag, schema, 'U2')); expect(selectionSet.toString()).toBe('{ ... on T1 { x y } }'); + // Note: Fragments on non-object types always have the full validator and thus + // results in the same validator regardless of the type they are expanded at. expect(validator?.toString()).toMatchString(` { + x: [ + T1.x + ] z: [ T2.z ] @@ -3268,11 +3328,16 @@ describe('named fragment selection set restrictions at type', () => { ({ selectionSet, validator } = expandAtType(frag, schema, 'U3')); expect(selectionSet.toString()).toBe('{ ... on T2 { z w } }'); + // Note: Fragments on non-object types always have the full validator and thus + // results in the same validator regardless of the type they are expanded at. expect(validator?.toString()).toMatchString(` { x: [ T1.x ] + z: [ + T2.z + ] y: [ T1.y ] diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index 12d54d13a..dd2b2699f 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -1224,6 +1224,14 @@ export class NamedFragmentDefinition extends DirectiveTargetElement { } `); }); + + it('validates fragments on non-object types across spreads', () => { + const subgraph1 = { + name: 'subgraph1', + typeDefs: gql` + type Query { + theEntity: AnyEntity + } + + union AnyEntity = EntityTypeA | EntityTypeB + + type EntityTypeA @key(fields: "unifiedEntityId") { + unifiedEntityId: ID! + unifiedEntity: UnifiedEntity + } + + type EntityTypeB @key(fields: "unifiedEntityId") { + unifiedEntityId: ID! + unifiedEntity: UnifiedEntity + } + + interface UnifiedEntity { + id: ID! + } + + type Generic implements UnifiedEntity @key(fields: "id") { + id: ID! + } + + type Movie implements UnifiedEntity @key(fields: "id") { + id: ID! + } + + type Show implements UnifiedEntity @key(fields: "id") { + id: ID! + } + `, + }; + + const subgraph2 = { + name: 'subgraph2', + typeDefs: gql` + interface Video { + videoId: Int! + taglineMessage(uiContext: String): String + } + + interface UnifiedEntity { + id: ID! + } + + type Generic implements UnifiedEntity @key(fields: "id") { + id: ID! + taglineMessage(uiContext: String): String + } + + type Movie implements UnifiedEntity & Video @key(fields: "id") { + videoId: Int! + id: ID! + taglineMessage(uiContext: String): String + } + + type Show implements UnifiedEntity & Video @key(fields: "id") { + videoId: Int! + id: ID! + taglineMessage(uiContext: String): String + } + `, + }; + + const [api, queryPlanner] = composeAndCreatePlannerWithOptions( + [subgraph1, subgraph2], + { reuseQueryFragments: true }, + ); + + const query = gql` + query Search { + theEntity { + ... on EntityTypeA { + unifiedEntity { + ... on Generic { + taglineMessage(uiContext: "Generic") + } + } + } + ... on EntityTypeB { + unifiedEntity { + ...VideoSummary + } + } + } + } + + fragment VideoSummary on Video { + videoId # Note: This extra field selection is necessary, so this fragment is not ignored. + taglineMessage(uiContext: "Video") + } + `; + const operation = operationFromDocument(api, query, { validate: true }); + + const plan = queryPlanner.buildQueryPlan(operation); + const validationErrors = validateSubFetches(plan.node, subgraph2); + expect(validationErrors).toHaveLength(0); + }); }); +/** + * For each fetch in a PlanNode validate the generated operation is actually spec valid against its subgraph schema + * @param plan + * @param subgraphs + */ +function validateSubFetches( + plan: PlanNode | SubscriptionNode | undefined, + subgraphDef: ServiceDefinition, +): { + errors: readonly GraphQLError[]; + serviceName: string; + fetchNode: FetchNode; +}[] { + if (!plan) { + return []; + } + const fetches = findFetchNodes(subgraphDef.name, plan); + const results: { + errors: readonly GraphQLError[]; + serviceName: string; + fetchNode: FetchNode; + }[] = []; + for (const fetch of fetches) { + const subgraphName: string = fetch.serviceName; + const operation: string = fetch.operation; + const subgraph = buildSubgraph( + subgraphName, + 'http://subgraph', + subgraphDef.typeDefs, + ); + const gql_errors = validate( + subgraph.schema.toGraphQLJSSchema(), + parse(operation), + ); + if (gql_errors.length > 0) { + results.push({ + errors: gql_errors, + serviceName: subgraphName, + fetchNode: fetch, + }); + } + } + return results; +} + describe('Fragment autogeneration', () => { const subgraph = { name: 'Subgraph1',