If requested, validate binding-types get the right fields #70
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
One sharp edge of the new `bindings` setting (when used for composite types) is this: the (presumably struct) type to which you're binding may expect to have particular fields, but it's GraphQL so you could have requested some other set of fields. Now, if you ask us, we check. Specifically, I've added a new setting under the `bindings` items, which says: everywhere we query this must select these fields. (Or use its own inline `# @genqlient(bind: ...)`.) It must select exactly those fields, in order, no more, no less. This was fairly easy to implement; actually comparing the selections was surprisingly much code but it's all pretty straightforward. Test plan: make check Reviewers: csilvers, marksandstrom, miguel, adam
benjaminjkraft
requested review from
aberkan,
csilvers,
dnerdy and
MiguelCastillo
dnerdy
approved these changes
Aug 28, 2021
There was a problem hiding this comment.
The implementation looks straight forward. 👍
I was wondering if json.Decoder with DisallowUnknownFields set could work for this. The validation wouldn't be as strong, but maybe it would be good enough? (Also, the expectation for UnmarshalJSON would be that the mention returns an error if the value is somehow incompatible.)
|
I think it's better for us to validate, because we can do it at compile-time. DisallowUnknownFields might be a good idea, although I worry that some GraphQL server will be badly-behaved and it will do more harm than good. |
benjaminjkraft
added a commit
that referenced
this pull request
Sep 15, 2021
In this commit I add two related features to genqlient: conflict-detection to avoid generating two distinct types with the same name, and an option to specify the type-name genqlient should use for some type. The conflict-detection was pretty simple once I realized I had already written all the code to do it in #70. There was a bunch of wiring, since we now need to keep track of the GraphQL type/selection-set that each type corresponds to, but it was pretty straightforward. This allows us to: - detect and reject if you have really sneaky type-names (there are some examples documented in `names.go`) - more clearly crash if genqlient accidentally generates two conflicting types, and - avoid stack-overflow when handing recursive (input) types (although sadly the poor support for options on input types (#14) makes them difficult to use in many cases; you really need to be able to set `pointer: true`) And with that all set up, the type-naming was also easy! (It doesn't have to get into the core of the type-generator, just plug in where we choose names. The desire for conflict detection was the main reason I hadn't set it up already.) Note that the existing limitation of #70 that the fields have to be in exactly the same order remains (and is now documented as #93); it's not deeply hard to fix but it's surprisingly much work. Issue: #60 Issue: #12 Test plan: make check Reviewers: csilvers, marksandstrom, adam, miguel, jvoll, mahtab
benjaminjkraft
added a commit
that referenced
this pull request
Sep 16, 2021
In this commit I add two related features to genqlient: conflict-detection to avoid generating two distinct types with the same name, and an option to specify the type-name genqlient should use for some type. The conflict-detection was pretty simple once I realized I had already written all the code to do it in #70. There was a bunch of wiring, since we now need to keep track of the GraphQL type/selection-set that each type corresponds to, but it was pretty straightforward. This allows us to: - detect and reject if you have really sneaky type-names (there are some examples documented in `names.go`) - more clearly crash if genqlient accidentally generates two conflicting types, and - avoid stack-overflow when handing recursive (input) types (although sadly the poor support for options on input types (#14) makes them difficult to use in many cases; you really need to be able to set `pointer: true`) And with that all set up, the type-naming was also easy! (It doesn't have to get into the core of the type-generator, just plug in where we choose names. The desire for conflict detection was the main reason I hadn't set it up already.) Note that the existing limitation of #70 that the fields have to be in exactly the same order remains (and is now documented as #93); it's not deeply hard to fix but it's surprisingly much work. Issue: #60 Issue: #12 Test plan: make check Reviewers: csilvers, marksandstrom, adam, miguel, jvoll, mahtab
benjaminjkraft
added a commit
that referenced
this pull request
Sep 16, 2021
## Summary: In this commit I add two related features to genqlient: conflict-detection to avoid generating two distinct types with the same name, and an option to specify the type-name genqlient should use for some type. The conflict-detection was pretty simple once I realized I had already written all the code to do it in #70. There was a bunch of wiring, since we now need to keep track of the GraphQL type/selection-set that each type corresponds to, but it was pretty straightforward. This allows us to: - detect and reject if you have really sneaky type-names (there are some examples documented in `names.go`) - more clearly crash if genqlient accidentally generates two conflicting types, and - avoid stack-overflow when handing recursive (input) types (although sadly the poor support for options on input types (#14) makes them difficult to use in many cases; you really need to be able to set `pointer: true`) And with that all set up, the type-naming was also easy! (It doesn't have to get into the core of the type-generator, just plug in where we choose names. The desire for conflict detection was the main reason I hadn't set it up already.) Note that the existing limitation of #70 that the fields have to be in exactly the same order remains (and is now documented as #93); it's not deeply hard to fix but it's surprisingly much work. Issue: #60 Issue: #12 ## Test plan: make check Author: benjaminjkraft Reviewers: StevenACoffman, jvoll, benjaminjkraft, aberkan, csilvers, dnerdy, mahtabsabet, MiguelCastillo Required Reviewers: Approved By: StevenACoffman, jvoll Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint Pull Request URL: #94
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary:
One sharp edge of the new
bindingssetting (when used for compositetypes) is this: the (presumably struct) type to which you're binding
may expect to have particular fields, but it's GraphQL so you could have
requested some other set of fields. Now, if you ask us, we check.
Specifically, I've added a new setting under the
bindingsitems, whichsays: everywhere we query this must select these fields. (Or use its
own inline
# @genqlient(bind: ...).) It must select exactly thosefields, in order, no more, no less. This was fairly easy to implement;
actually comparing the selections was surprisingly much code but it's
all pretty straightforward.
Test plan:
make check