fix: fixed invalid subgraph queries when reuseQueryFragments=true

- updated computeExpandedSelectionSetAtType function not to trim fragment's validators if the fragment's type condition is not an object type.
- This change is necessary because `FieldsInSetCanMerge` rule is more restrictive in that case.
- The untrimmed validator should avoid generating invalid queries, but it may be less efficient.

apollographql#2952

.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:/apollographql/federation/issues/2952))

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')!;
+
+ let { 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,17 +3191,32 @@ 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`
  // is used to check for field conflict and save for efficiency, we could use the `original` instead).
  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
  ]

internals-js/src/operations.ts

@@ -1224,6 +1224,14 @@ export class NamedFragmentDefinition extends DirectiveTargetElement<NamedFragmen
  const expandedSelectionSet = this.expandedSelectionSet();
  const selectionSet = expandedSelectionSet.normalize({ parentType: type });
 
+ if( ! isObjectType(this.typeCondition) ) {
+ // When the type condition of the fragment is not an object type, the `FieldsInSetCanMerge` rule is more
+ // restrictive and any fields can create conflicts. Thus, we have to use the full validator in this case.
+ // (see https:/graphql/graphql-spec/issues/1085 for details.)
+ const validator = FieldsConflictValidator.build(expandedSelectionSet);
+ return { selectionSet, validator };
+ }
+
  // Note that `trimmed` is the difference of 2 selections that may not have been normalized on the same parent type,
  // so in practice, it is possible that `trimmed` contains some of the selections that `selectionSet` contains, but
  // that they have been simplified in `selectionSet` in such a way that the `minus` call does not see it. However,

query-planner-js/src/__tests__/buildPlan.test.ts

@@ -4,6 +4,7 @@ import {
  operationFromDocument,
  ServiceDefinition,
  Supergraph,
+ buildSubgraph,
 } from '@apollo/federation-internals';
 import gql from 'graphql-tag';
 import {
@@ -12,7 +13,7 @@ import {
  SequenceNode,
  serializeQueryPlan,
 } from '../QueryPlan';
-import { FieldNode, OperationDefinitionNode, parse } from 'graphql';
+import { FieldNode, OperationDefinitionNode, parse, validate, GraphQLError } from 'graphql';
 import {
  composeAndCreatePlanner,
  composeAndCreatePlannerWithOptions,
@@ -5041,8 +5042,139 @@ describe('Named fragments preservation', () => {
  }
  `);
  });
+
+ 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});
+
+ let 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);
+ let 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: any, subgraphDef: ServiceDefinition):
+ { errors: readonly GraphQLError[], serviceName: string, fetchNode: FetchNode }[]
+{
+ if (!plan) {
+ return [];
+ }
+ let fetches = findFetchNodes(subgraphDef.name, plan);
+ var results: { errors: readonly GraphQLError[], serviceName: string, fetchNode: FetchNode }[] = [];
+ for (let fetch of fetches) {
+ let subgraphName: string = fetch.serviceName;
+ let operation: string = fetch.operation;
+ let subgraph = buildSubgraph(subgraphName, 'http://subgraph', subgraphDef.typeDefs);
+ let 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',