genqlient icon indicating copy to clipboard operation
genqlient copied to clipboard

Validate against conflicting options on the same type

Open benjaminjkraft opened this issue 4 years ago • 1 comments

Consider the following queries:

# @genqlient(omitempty: true)
query Q1(input1: MyInput) { ... }

query Q2(input2: MyInput) { ... }

In this case, we generate a type MyInput which we use for both input1 and input2. But there's actually a conflict here: omitempty is set to true on input1 and false (by default) on input2. (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 via typename.

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 typename on 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 set omitempty: true on input2, or set a different typename for either input1 or input2.

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.

benjaminjkraft avatar Oct 01 '21 00:10 benjaminjkraft

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.

benjaminjkraft avatar Jan 22 '23 22:01 benjaminjkraft