Skip to content

Commit

Permalink
Gateway over-merging unions cleanup
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
trevor-scheer and Tejas Shikhare committed Dec 16, 2019
1 parent 33fa165 commit 45428a2
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 25 deletions.
1 change: 0 additions & 1 deletion packages/apollo-gateway/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
> [See complete versioning details.](https://github.com/apollographql/apollo-server/commit/a0a60e73e04e913d388de8324f7d17e4406deea2)
* 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)
Expand Down
23 changes: 3 additions & 20 deletions packages/apollo-gateway/src/FieldSet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>(
Expand All @@ -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;
Expand Down
8 changes: 4 additions & 4 deletions packages/apollo-gateway/src/buildQueryPlan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import {
Field,
FieldSet,
groupByParentType,
groupByParentTypeAndResponseName,
groupByResponseName,
matchesField,
selectionSetFromFieldSet,
Scope,
Expand Down Expand Up @@ -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,
)) {
Expand Down Expand Up @@ -413,7 +413,7 @@ function splitFields(
scope as Scope<typeof parentType>,
group,
path,
fieldsForResponseName,
fieldsForParentType,
),
);
} else {
Expand Down Expand Up @@ -452,7 +452,7 @@ function splitFields(
field.fieldNode,
);

const fieldsWithRuntimeParentType = fieldsForResponseName.map(field => ({
const fieldsWithRuntimeParentType = fieldsForParentType.map(field => ({
...field,
fieldDef,
}));
Expand Down

0 comments on commit 45428a2

Please sign in to comment.