Khan / Khan/genqlient

Validate against conflicting options on the same type

Open
#123 1 comment 0 reactions 0 assignees View on GitHub
bug
Dominant language
Go
Stars
1.3k
Forks
143
Avg merge
4h 50m
Merged PRs (30d)
1

Description

Consider the following queries:
```graphql
# @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.

Contributor guide

Open the contributing guide

Research direction

Start with the existing naming-collision validation, especially selectionsMatch, and trace where directives are parsed during type generation. Determine how directives can be preprocessed and attached to nodes for both generation and validation. Done means reused type names with conflicting options produce an explicit error that suggests matching the options or choosing a different typename.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
tooling
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.