Prevent field argument collision in diffTypeNodes #1298
Conversation
|
@stevenschobert: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
|
CLA is signed! |
|
Thanks @stevenschobert! I've got a lot on my plate atm but I'll try to get to this as soon as I'm able. Might be tough with the holidays coming up - don't be afraid to ping me after the new year if I haven't given you a review. |
|
Hey @trevor-scheer! Just giving a friendly reminder about this PR, after the new year. Hope you had a wonderful break! |
There was a problem hiding this comment.
This is excellent, thanks for all the work you put into this @stevenschobert. Definitely appreciate all the new tests as well!
Would you mind adding a CHANGELOG entry to the federation package?
If you don't get to this in the next couple days, I'll take care of the rest and get this landed.
For #1100 - previously, arguments were being diff-ed across every field, without tracking which field the argument belonged to. This made it possible for certain types to produce false positives, if that type contained 2 fields that both used the same argument name, but different types on that argument. Additionally, the `inputValues` entry in the return object would also contain field diffs for input objects, due to the fact that the InputValueDefinition gets called on BOTH field arguments, and fields on input objects. Both these problems have been addressed by diffing the field arguments on a per-field basis, and by treating fields on input objects the same as fields on regular types.
|
Thanks for the review! I've added a CHANGELOG entry, and corrected that negative test case. |
|
LGTM! In case you're curious, I pushed up an empty commit to trigger CI - for some reason none of the circle checks ran on your most recent commit, or so it seemed. Looks good now. Thanks again! Been a pleasure working with you on this. 🙏 |
|
@stevenschobert this fix is now released in the |
This MR addresses the bug outlined in #1100, where value types with similarly named field arguments result in a composition error.
I outlined some of the implementation details in my issue comment, but the summary of the changes in this PR are:
diffTypeNodeshas been adjusted to consider arguments on a per-field basisdiffTypeNodes's return value has the following changes:{ name: [], kind: [], fields: {}, - inputValues: {}, + fieldArgs: {}, unionTypes: {}, locations: [], - args: {}, + directiveArgs: {} }diffTypeNodeshave been updated to handle the new return value's shape. In cases where the call site previously used theinputValuesentry in the diff, a loop has been added to handlefieldArgsholding nested diffs per-field name.diffTypeNodes