From 89cdcfd60da0ce4fce4b9fa56ff3b802a14a7f93 Mon Sep 17 00:00:00 2001
From: Trevor Scheer <trevor.scheer@gmail.com>
Date: Mon, 16 Dec 2019 10:01:12 -0800
Subject: [PATCH] Gateway over-merging unions cleanup

This commit cleans up and simplifies the approach taken by #3581
to prevent the gateway from over-merging fields of unioned types.

The correct information and groupings were already present in the
affected code, they just needed to be utilized correctly. Specifically,
the internal for loop yielding `fieldsForParentType` was not being
used - the less specifically grouped `fieldsForResponseName` was.

Co-authored-by: Tejas Shikhare <tshikhare@netflix.com>
---
 packages/apollo-gateway/src/FieldSet.ts       | 23 +++----------------
 packages/apollo-gateway/src/buildQueryPlan.ts |  8 +++----
 2 files changed, 7 insertions(+), 24 deletions(-)

diff --git a/packages/apollo-gateway/src/FieldSet.ts b/packages/apollo-gateway/src/FieldSet.ts
index 1757872dd26..02cf29a2ff5 100644
--- a/packages/apollo-gateway/src/FieldSet.ts
+++ b/packages/apollo-gateway/src/FieldSet.ts
@@ -64,25 +64,8 @@ function groupBy<T, U>(keyFunction: (element: T) => U) {
   };
 }
 
-// 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 groupByResponseName = groupBy<Field, string>(field =>
+  getResponseName(field.fieldNode)
 );
 
 export const groupByParentType = groupBy<Field, GraphQLCompositeType>(
@@ -98,7 +81,7 @@ export function selectionSetFromFieldSet(
     selections: Array.from(groupByParentType(fields)).flatMap(
       ([typeCondition, fieldsByParentType]: [GraphQLCompositeType, FieldSet]) =>
         wrapInInlineFragmentIfNeeded(
-          Array.from(groupByParentTypeAndResponseName(fieldsByParentType).values()).map(
+          Array.from(groupByResponseName(fieldsByParentType).values()).map(
             fieldsByResponseName => {
               return combineFields(fieldsByResponseName)
                 .fieldNode;
diff --git a/packages/apollo-gateway/src/buildQueryPlan.ts b/packages/apollo-gateway/src/buildQueryPlan.ts
index fe6ae3396fc..1b255cbd95c 100644
--- a/packages/apollo-gateway/src/buildQueryPlan.ts
+++ b/packages/apollo-gateway/src/buildQueryPlan.ts
@@ -31,7 +31,7 @@ import {
   Field,
   FieldSet,
   groupByParentType,
-  groupByParentTypeAndResponseName,
+  groupByResponseName,
   matchesField,
   selectionSetFromFieldSet,
   Scope,
@@ -373,7 +373,7 @@ function splitFields(
   fields: FieldSet,
   groupForField: (field: Field<GraphQLObjectType>) => FetchGroup,
 ) {
-  for (const fieldsForResponseName of groupByParentTypeAndResponseName(fields).values()) {
+  for (const fieldsForResponseName of groupByResponseName(fields).values()) {
     for (const [parentType, fieldsForParentType] of groupByParentType(
       fieldsForResponseName,
     )) {
@@ -413,7 +413,7 @@ function splitFields(
             scope as Scope<typeof parentType>,
             group,
             path,
-            fieldsForResponseName,
+            fieldsForParentType,
           ),
         );
       } else {
@@ -452,7 +452,7 @@ function splitFields(
               field.fieldNode,
             );
 
-            const fieldsWithRuntimeParentType = fieldsForResponseName.map(field => ({
+            const fieldsWithRuntimeParentType = fieldsForParentType.map(field => ({
               ...field,
               fieldDef,
             }));