Gateway over-merging fields of unioned types #3581
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @trevor-scheer - I think I have a fix for this. Let me know if this passes your test. f2cc553 |
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.
|
Hey @stejas, thanks for taking a stab at this one! In your test, |
jbaxleyiii
approved these changes
Dec 5, 2019
|
Hi @trevor-scheer - Just looked at your fix! If I understand correctly, aren't our fixes effectively same? I am passing |
trevor-scheer
added a commit
that referenced
this pull request
Dec 16, 2019
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>
trevor-scheer
added a commit
that referenced
this pull request
Dec 16, 2019
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>
|
@stejas you're right! My apologies, I was a bit excited to land this fix and could have looked a bit more closely (though I recall running those changes previously against my test and seeing failures 🤔 I'll chalk it up to an error on my end, since your changes pass my test now). I'll await your response on #3616 about landing your simplified changes. |
trevor-scheer
added a commit
that referenced
this pull request
Jan 2, 2020
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>
abernix
pushed a commit
to apollographql/federation
that referenced
this pull request
Sep 4, 2020
…rver#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. Apollo-Orig-Commit-AS: apollographql/apollo-server@c17c7bb
abernix
pushed a commit
to apollographql/federation
that referenced
this pull request
Sep 4, 2020
This commit cleans up and simplifies the approach taken by apollographql/apollo-server#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> Apollo-Orig-Commit-AS: apollographql/apollo-server@61e6941
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds some granularity to how fields are keyed when we group them within a selection set. Previously, the field name was considered sufficient for merging selections (and it almost always is!). However, in the case that the
parentTypeof these same-named fields is different, we most certainly should not group selections as it will result in invalid query plans.The new test illustrates this edge case via two
unioned types that share a same top-level field name. The gateway was incorrectly grouping these separate fields into the sameselectionSetunder themediafield. By keying onparentType:responseNameinstead, the gateway will know better than to group two distinctmediafields of different types.