Validate against conflicting options on the same type #123
Comments
benjaminjkraft
added a commit
that referenced
this issue
Oct 1, 2021
This has been a bit of a thorn since we started using genqlient in production: just as you might want to specify, say, `omitempty` on an argument, you might equally want to specify it on an input-type field. But there's no obvious syntax to do that, because the input-type field does not appear in the query (only the schema) so there's nowhere to put the `# @genqlient` directive. This commit, at last, fixes that problem, via a new option, `for`, which you use in an option applied to the entire operation (or fragment), and says, "actually, apply this directive to the given field, not the entire operation". (It's mainly useful for input types, but I allowed it for output types too; I could imagine it being convenient if you want to say you always use a certain type or type-name for a certain field.) It works basically like you expect: the inline options take precedence over `for` take precedence over query-global options. The implementation was fairly straightforward once I did a little refactoring, mostly in the directive-parsing and directive-merging (which are now combined, since merging is now a bit more complicated). With that in place, and extended to support `for`, we need only add the same wiring to input-fields that we have for other places you can put directives. I did not attempt to solve the issue I've now documented as #123, wherein conflicting options can lead to confusing behavior; the new `for` is a new and perhaps more attractive avenue to cause it but the issue remains the same and requires nontrivial refactoring (described in the issue) to solve. (The breakage isn't horrible for the most part; the option will just apply, or not apply, where you don't expect it to.) But while applying that logic, I noticed a problem, which is that we were inconsistently cascading operation-level options down to input-object fields. (I think this came out of the fact that initially I thought to cascade them, then realized that this could cause problems like #123 and intended to walk them back, but then accidentally only "fixed" it for `omitempty`. I guess until this change, operation-level options were rare enough, and input-field options messy enough, that no one noticed.) So in this commit I bring things back into consistency, by saying that they do cascade: with at least a sketch of a path forward to fix #123 via better validation, I think that's by far the clearest behavior. Issue: #14 Test plan: make check Reviewers: csilvers, marksandstrom, steve, jvoll, adam, miguel, mahtab
benjaminjkraft
added a commit
that referenced
this issue
Oct 1, 2021
## Summary: This has been a bit of a thorn since we started using genqlient in production: just as you might want to specify, say, `omitempty` on an argument, you might equally want to specify it on an input-type field. But there's no obvious syntax to do that, because the input-type field does not appear in the query (only the schema) so there's nowhere to put the `# @genqlient` directive. This commit, at last, fixes that problem, via a new option, `for`, which you use in an option applied to the entire operation (or fragment), and says, "actually, apply this directive to the given field, not the entire operation". (It's mainly useful for input types, but I allowed it for output types too; I could imagine it being convenient if you want to say you always use a certain type or type-name for a certain field.) It works basically like you expect: the inline options take precedence over `for` take precedence over query-global options. The implementation was fairly straightforward once I did a little refactoring, mostly in the directive-parsing and directive-merging (which are now combined, since merging is now a bit more complicated). With that in place, and extended to support `for`, we need only add the same wiring to input-fields that we have for other places you can put directives. I did not attempt to solve the issue I've now documented as #123, wherein conflicting options can lead to confusing behavior; the new `for` is a new and perhaps more attractive avenue to cause it but the issue remains the same and requires nontrivial refactoring (described in the issue) to solve. (The breakage isn't horrible for the most part; the option will just apply, or not apply, where you don't expect it to.) But while applying that logic, I noticed a problem, which is that we were inconsistently cascading operation-level options down to input-object fields. (I think this came out of the fact that initially I thought to cascade them, then realized that this could cause problems like #123 and intended to walk them back, but then accidentally only "fixed" it for `omitempty`. I guess until this change, operation-level options were rare enough, and input-field options messy enough, that no one noticed.) So in this commit I bring things back into consistency, by saying that they do cascade: with at least a sketch of a path forward to solve #123 via better validation, I think that's by far the clearest behavior. Issue: #14 ## Test plan: make check Author: benjaminjkraft Reviewers: csilvers, StevenACoffman, benjaminjkraft, aberkan, dnerdy, jvoll, mahtabsabet, MiguelCastillo Required Reviewers: Approved By: csilvers, StevenACoffman 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: #124
|
Another issue here, surfaced in #247, is that if you're using globbing this can even result in nondeterministic behavior since the order we see the various references to the type potentially depends on the order we get the files from the file system. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Consider the following queries:
In this case, we generate a type
MyInputwhich we use for bothinput1andinput2. But there's actually a conflict here:omitemptyis set to true oninput1and false (by default) oninput2. (This is because directives on operation cascade down to all child fields.) The same problem is even more likely to happen with different queries in a package, and also applies to options set via the forthcoming input-field options setting (see #124), or output type-names reused viatypename.Right now we don't check for that; we just use whichever one we see first. Instead, similar to how we validate that if you use
typenameon several fields, they all have the same selections, we should validate that in any case where we use the same type-name in two places, they have the same options. So in this case, we would error, and advise the user to either setomitempty: trueoninput2, or set a different typename for eitherinput1orinput2.In principle we could do this by extending the existing naming-collision validation (
selectionsMatch). But doing that in the obvious way will require some annoying plumbing as well as parsing directives twice (once in type-generation then again in validation). The best thing to do is probably to parse all the directives in a preprocessing step upfront rather than as we generate types, and somehow attach them to nodes (perhaps just via a global map of node to genqlient directive). Then we can just read from that map in both type-generation and validation. (It would probably simplify the conversion code, too, since we could skip plumbing the options everywhere inline.)Note that this will be a breaking change, but it will be going from silent confusing behavior to an explicit error, so I think that's totally fine and worthwhile.
The text was updated successfully, but these errors were encountered: