Gateway over-merging fields of unioned types (#3581)

Group fields by response name AND parent type The groupByResponseName function was insufficient for determining the merge condition for selections. The parent type name is also required for the merge condition, as it's possible for them to differ.
apollographql · Dec 5, 2019 · c17c7bb · c17c7bb
1 parent e3d3b90
commit c17c7bb
Show file tree

Hide file tree

Showing 4 changed files with 103 additions and 5 deletions.
diff --git a/packages/apollo-gateway/CHANGELOG.md b/packages/apollo-gateway/CHANGELOG.md
@@ -4,6 +4,8 @@
 
 > The changes noted within this `vNEXT` section have not been released yet.  New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development.  When a release is being prepared, a new header will be (manually) created below and the the appropriate changes within that release will be moved into the new section.
 
+ * Gateway over-merging fields of unioned types [#3581](https://github.com/apollographql/apollo-server/pull/3581)
+
 # v0.11.0
 
 > [See complete versioning details.](https://github.com/apollographql/apollo-server/commit/93002737d53dd9a50b473ab9cef14849b3e539aa)

diff --git a/packages/apollo-gateway/src/FieldSet.ts b/packages/apollo-gateway/src/FieldSet.ts
@@ -64,8 +64,25 @@ function groupBy<T, U>(keyFunction: (element: T) => U) {
   };
 }
 
-export const groupByResponseName = groupBy<Field, string>(field =>
-  getResponseName(field.fieldNode),
+// The response name isn't sufficient for determining uniqueness. In the case of
+// unions, for example, we can see a response name collision where the parent type
+// is different. In this case, these should not be merged (media)!
+// query {
+//   content {
+//     ... on Audio {
+//       media {
+//         url
+//       }
+//     }
+//     ... on Video {
+//       media {
+//         aspectRatio
+//       }
+//     }
+//   }
+// }
+export const groupByParentTypeAndResponseName = groupBy<Field, string>(field =>
+  `${field.scope.parentType}:${getResponseName(field.fieldNode)}`,
 );
 
 export const groupByParentType = groupBy<Field, GraphQLCompositeType>(
@@ -81,7 +98,7 @@ export function selectionSetFromFieldSet(
     selections: Array.from(groupByParentType(fields)).flatMap(
       ([typeCondition, fieldsByParentType]: [GraphQLCompositeType, FieldSet]) =>
         wrapInInlineFragmentIfNeeded(
-          Array.from(groupByResponseName(fieldsByParentType).values()).map(
+          Array.from(groupByParentTypeAndResponseName(fieldsByParentType).values()).map(
             fieldsByResponseName => {
               return combineFields(fieldsByResponseName)
                 .fieldNode;

diff --git a/packages/apollo-gateway/src/__tests__/integration/unions.test.ts b/packages/apollo-gateway/src/__tests__/integration/unions.test.ts
@@ -0,0 +1,79 @@
+import gql from 'graphql-tag';
+import { astSerializer, queryPlanSerializer } from '../../snapshotSerializers';
+import { execute } from '../execution-utils';
+
+expect.addSnapshotSerializer(astSerializer);
+expect.addSnapshotSerializer(queryPlanSerializer);
+
+it('handles multiple union type conditions that share a response name (media)', async () => {
+  const query = gql`
+    query {
+      content {
+        ...Audio
+        ... on Video {
+          media {
+            aspectRatio
+          }
+        }
+      }
+    }
+    fragment Audio on Audio {
+      media {
+        url
+      }
+    }
+  `;
+
+  const { queryPlan, errors } = await execute(
+    [
+      {
+        name: 'contentService',
+        typeDefs: gql`
+          extend type Query {
+            content: Content
+          }
+          union Content = Audio | Video
+          type Audio {
+            media: AudioURL
+          }
+          type AudioURL {
+            url: String
+          }
+          type Video {
+            media: VideoAspectRatio
+          }
+          type VideoAspectRatio {
+            aspectRatio: String
+          }
+        `,
+        resolvers: {
+          Query: {},
+        },
+      },
+    ],
+    { query },
+  );
+
+  expect(errors).toBeUndefined();
+  expect(queryPlan).toMatchInlineSnapshot(`
+    QueryPlan {
+      Fetch(service: "contentService") {
+        {
+          content {
+            __typename
+            ... on Audio {
+              media {
+                url
+              }
+            }
+            ... on Video {
+              media {
+                aspectRatio
+              }
+            }
+          }
+        }
+      },
+    }
+  `);
+});
diff --git a/packages/apollo-gateway/src/buildQueryPlan.ts b/packages/apollo-gateway/src/buildQueryPlan.ts
@@ -31,7 +31,7 @@ import {
   Field,
   FieldSet,
   groupByParentType,
-  groupByResponseName,
+  groupByParentTypeAndResponseName,
   matchesField,
   selectionSetFromFieldSet,
   Scope,
@@ -373,7 +373,7 @@ function splitFields(
   fields: FieldSet,
   groupForField: (field: Field<GraphQLObjectType>) => FetchGroup,
 ) {
-  for (const fieldsForResponseName of groupByResponseName(fields).values()) {
+  for (const fieldsForResponseName of groupByParentTypeAndResponseName(fields).values()) {
     for (const [parentType, fieldsForParentType] of groupByParentType(
       fieldsForResponseName,
     )) {