GraphQL schemas have some builtin types, like `String`. The spec says
your SDL must not include those, but in practice some schemas do. (This
is probably because introspection must include them, and some tools that
create SDL from introspection don't know they're supposed to filter them
out.) Anyway, we've since #145 had logic to handle this; we just parse
with and without the prelude that defines them and see which works.
The problem is that this makes for very confusing error messages if you
have an invalid schema. (Or if you have a schema that you think is valid
but gqlparser doesn't, which is the more common case in the wild; see
for example #200.) Right now if both ways error we take the
without-prelude error, which if you didn't define the builtins is just
`undefined type String`; if we took the with-prelude error then if you
did define the builtins you'd just get `type String defined twice`. So
we actually have to be smart if we want good error messages for
everyone.
So in this commit we are smart: we check if your schema defines
`String`, and include the prelude only if it does not. To do this I
basically inlined `gqlparser.LoadSchema` (twice), so that in between
parsing and validation we can check if you have `String` and if not add
the prelude. This should in theory be both more efficient (we don't
have to do everything twice) and give better error messages,
although it's a bit more code.
Fixes#175.
Test plan: make check
While writing tests at some point I came across an invalid query that
gqlparser doesn't catch, and which causes a panic for us. Now we
validate for it and return a nice error instead of panicing.
Fixes#176.
Test plan: make check
Some of the errors tests need to have their own schema, so the schema
can do something weird (or even be entirely invalid!). But most can
still share a schema. In this commit I have those indeed share a
schema, to avoid having to have a bunch of copies of mostly the same
schema. While doing so I noticed one error whose location wasn't very
useful, and fixed it.
Test plan: make check
GraphQL is pretty restrictive about its identifiers, so for the most
part we can and do safely use GraphQL identifiers in the Go we generate
with attention only to conflicts with other such identifiers. But we do
need to check one thing, which is that the identifier isn't a Go
keyword. (If it is, the generated code will almost certainly fail to
compile, but often with a confusing error message.) In this commit I add
such checks.
The most likely place to run into trouble here is argument names, which
are often one word and are used as-is. Operation names, if unexported,
can also be keywords, although in practice they're usually multiword.
Field names are always exported, thus safe. Generated type names are
always prefixed, camel-cased, with the operation name, so they always
contain an uppercase letter (even if the operation name is lowercase),
but type-names specified by `typename` may collide, so we check those.
In theory we could check type-names specified by `bind`, but these must
be defined by the user, so their code will already fail to compile, so I
didn't bother. I think that's all the places to consider, although it's
hard to be sure. In summary, we check argument names, operation names,
and user-specified type names.
Test plan: make check
In #133, Craig added support for a new use of typename, where it applies
to a scalar and means that genqlient should generate a named type, e.g.
`# @genqlient(typename: "MyString")` on a node of type string will
generate and use `type MyString string`. But this gets a bit confusing
if you mix it with `bind`; should
`typename: "MyString", bind: "int32"` generate `type MyString int32`, or
should one override the other, or what? Of course in practice you're
not likely to write that all in one place, but you could via a global
binding, or a `for` directive, and in that case probably it was a
mistake. In #138, we looked at making them work together correctly, but
it added complexity and got even more confusing.
So instead, here, we just ban it; we can always add it back if it proves
useful. (Or, you can make the `typename` win over a global binding by
locally unbinding it via `bind: "-"`.) This required changes in
surprisingly many places; I already knew the directive-validation code
was due for a refactor but that will happen some other day. The tests
show that it works, in any case.
Interestingly, this problem actually could have arisen for a struct
binding already, before #133. But all the same reasons it's confusing
seem to apply, so I just banned it there too. This is technically a
breaking change although I doubt anyone will hit it.
Test plan: make check
## Summary:
This lets you write code like:
```
query x {
# @genqlient(typename: "MyString")
someStringField
}
```
and genqlient will do
```
typename MyString string
type x struct {
someStringField MyString
}
```
This was not difficult to implement, though it required introducing a
new identifier type. The main difficulty I had was weird test
failures, that it turns out was due to the tests putting a bunch of
fields on the same line, so that the genqlient directive on the
previous line applied to all of them, accidentally. This became a
problem when `typename` suddenly started being respected for builtin
types! I fixed it by just spreading out the queries a bit.
Fixes#130
## Test plan:
make check
Author: csilvers
Reviewers: dnerdy, StevenACoffman, benjaminjkraft
Required Reviewers:
Approved By: 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: https://github.com/Khan/genqlient/pull/133
## Summary:
One common use of fragment spreads is as the entirety of a field's
selection, e.g.
```graphql
query MyQuery {
myField {
...MyFragment
}
}
```
In this case, by default, genqlient generates a wrapper type
`MyQueryMyFieldMyType`, which just embeds `MyFragment`. This makes
sense if you later want to add more fields in addition to the fragment
spread. But if you don't -- and you did the fragment because you want
to share types, it's an extra layer of indirection. (Which becomes
especially onerous if `myField` has list type (`[MyType!]`), such that
it's not just an extra attribute-access to get to `MyFragment`.)
The new option `# @genqlient(flatten: true)` simplifies this situation:
if applied to `myField` is skips the wrapper type;
`MyQueryResponse.MyField` will simply have type `MyFragment` (or
`[]MyFragment`, or whatever). This should hopefully make the `typename`
option, which has more limitations, less necessary.
Note that in #30 the initial idea was to support this for fields as
well. This would require significant additional complexity in the
JSON-(un)marshaling code, and has proven less necessary, so I
implemented this option only for fragment-spreads for now. With that
restriction, it was shockingly simple; we have to hook into a bunch of
different places, but they're all quite simple, since the structure of
the Go types still matches the structure in GraphQL.
Issue: https://github.com/Khan/genqlient/issues/30
## Test plan:
make check
Author: benjaminjkraft
Reviewers: csilvers, dnerdy, aberkan, jvoll, mahtabsabet, MiguelCastillo, StevenACoffman
Required Reviewers:
Approved By: csilvers, dnerdy
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: https://github.com/Khan/genqlient/pull/121
## Summary:
Previously, we actually allowed you to put several genqlient directives
on the same node, but the semantics were undocumented (and somewhat
confusing, when it comes to `typename`). In order to support directives
on input options, we're actually going to be encouraging this usage (see
notes in #14), so it's time to fix it. To avoid confusion, I just had
conflicting directives be an error, rather than defining which one
"wins". The same applies to specifying the same option several
times in one directive.
I also fixed two small bugs:
- `typename` on an operation would incorrectly cascade down to
all input types in a query (causing conflicts).
- directive parse errors had useless positions, now they're correct
## Test plan:
make check
Author: benjaminjkraft
Reviewers: StevenACoffman, dnerdy, aberkan, jvoll, mahtabsabet, MiguelCastillo
Required Reviewers:
Approved By: StevenACoffman, dnerdy
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: https://github.com/Khan/genqlient/pull/105
## 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: https://github.com/Khan/genqlient/issues/60
Issue: https://github.com/Khan/genqlient/issues/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: https://github.com/Khan/genqlient/pull/94
## Summary:
The basic idea here is if you only request interface fields (no
fragments) you may not care about the concrete type, and so we could
just generate a struct as if it were an object. I don't think it's a
good idea to do that by default, because then if you later add a
fragment all your code totally changes, but it's quite reasonable as an
option!
Most of the code involved is just wiring and validation; the
core implementation is literally just: treat it like an object.
Issue: https://github.com/Khan/genqlient/issues/85
## Test plan:
make check
Author: benjaminjkraft
Reviewers: csilvers, StevenACoffman, benjaminjkraft, aberkan, dnerdy, jvoll, mahtabsabet, MiguelCastillo
Required Reviewers:
Approved By: 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: https://github.com/Khan/genqlient/pull/97
## Summary:
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
Author: benjaminjkraft
Reviewers: dnerdy, aberkan, csilvers, MiguelCastillo
Required Reviewers:
Approved by: dnerdy
Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint
Pull request URL: https://github.com/Khan/genqlient/pull/70
## Summary:
In this commit I add support for inline fragments
(`... on MyType { fields }`) to genqlient. This will make interfaces a
lot more useful! In future commits I'll add named fragments, for which
we'll generate slightly different types, as discussed in DESIGN.md.
In general, implementing the flattening approach described in DESIGN.md
was... surprisingly easy. All we have to do is recurse on applicable
fragments when generating our selection-set. The refactor to
selection-set handling this encouraged was, I think, quite beneficial.
It did reveal two tricky pre-existing issues.
One issue is that GraphQL allows for duplicate selections, as long as
they match. (In practice, this is only useful in the context of
fragments, although GraphQL allows it even without.) I decided to handle
the simple case (duplicate leaf fields; we just deduplicate) but leave
to the future the complex cases where we need to merge different
sub-selections (now #64). For now we just forbid that; we can see how
much it comes up.
The other issue is that we are generating type-names incorrectly for
interface types; I had intended to do `MyInterfaceMyFieldMyType` for
shared fields and `MyImplMyFieldMyType` for non-shared ones, but instead
I did `MyFieldMyType`, which is inconsistent already and can result in
conflicts in the presence of fragments. I'm going to fix this in a
separate commit, though, because it's going to require some refactoring
and is irrelevant to the main logic of this commit; I left some TODOs in
the tests related to this.
Issue: https://github.com/Khan/genqlient/issues/8
## Test plan:
make check
Author: benjaminjkraft
Reviewers: dnerdy, aberkan, MiguelCastillo
Required Reviewers:
Approved by: dnerdy
Checks: ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13), ✅ Lint, ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13), ✅ Lint
Pull request URL: https://github.com/Khan/genqlient/pull/65
## Summary:
In #52, I added support for interface types, but with the simplifying
restriction (among others) that the user must request the field
`__typename`. In this commit, I remove this restriction.
The basic idea is simple: we preprocess the query to add `__typename`.
The implementation isn't much more complicated! Although it required
some new wiring in a few places.
Issue: https://github.com/Khan/genqlient/issues/8
## Test plan:
make check
Author: benjaminjkraft
Reviewers: dnerdy, aberkan, MiguelCastillo
Required Reviewers:
Approved by: dnerdy
Checks: ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13), ⌛ Lint, ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13), ⌛ Lint
Pull request URL: https://github.com/Khan/genqlient/pull/56
## Summary:
In this commit I begin the journey to add the long-awaited support for
interfaces (part of #8). Well, it's not the beginning: I already had
some half-written broken code around. But it's the first fully
functional support, and especially, the first *tested* support; it's
probably best to review the nontrivially-changed code as if it were new.
Conceptually, the code so far is pretty simple: we generate an interface
type, and the implementations. (That code is in fact mostly unchanged.)
The complexity comes in because encoding/json doesn't know how to
unmarshal that. So we have to add an UnmarshalJSON method, which
actually has to be on the types with interface-type fields, that knows
how. I factored it into two methods, such that that UnmarshalJSON
method is just glue, and then there's a separate function, corresponding
to each interface-type, that actually does all the work. (If only one
could just write it as an actual method!) The method uses the same
trick suggested to me by a few others in another context to deserialize
all but one field, then handle that field specially, which is discussed
in the code.
This still has some limitations, which will be lifted in future commits:
- it doesn't allow for list-of-interface fields
- it requires that you manually ask for `__typename`
- it doesn't support fragments, i.e. you can only query for interface
fields, not concrete-type-specific ones
But it works, even in integration tests, which is progress!
As a part of this, I added a proper config option for the "allow broken
features" flag, since I need to be able to set it from the integration
tests which are in a separate package (and actually shell out via `go
generate`). I also renamed what was to be the first case
(InterfaceNoFragments), and replaced it with a further-simplified
version (avoiding list-of-interface fields.
[1] https://github.com/benjaminjkraft/notes/blob/master/go-json-interfaces.md
Issue: https://github.com/Khan/genqlient/issues/8
## Test plan:
make tesc
Author: benjaminjkraft
Reviewers: dnerdy, benjaminjkraft, aberkan, csilvers, MiguelCastillo
Required Reviewers:
Approved by: dnerdy
Checks: ⌛ Test (1.17), ✅ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13), ✅ Lint, ✅ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13), ✅ Lint
Pull request URL: https://github.com/Khan/genqlient/pull/52
Ben Kraft