If you do `UPDATE_SNAPSHOTS=1 go test ./...` that will:
1. run the snapshot tests, updating any changed snapshots
2. run the integration tests and (if you have tokens) example tests
3. check that the code for the integration tests and example tests is
up-to-date
Step 3 is not strictly a snapshot test; the generated code is actually
checked in and used. But, I mean, it's basically the same! So now, we
also update it if you asked to update snapshots, which is hopefully a
little more convenient.
Fixes#212.
Test plan:
Make a trivial change to `example/generated.go`; `go test ./...` should
now fail. `UPDATE_SNAPSHOTS=1 go test ./...` should also fail, but say
it updated the snapshot, and the change should be reverted. Run `go test
./...` again; it should pass again.
This allows client code to see the operation (query or mutation) exactly
as genqlient sends it over the wire. This data was already available in
the generated safelist.json file, but now it's easily available from Go
code as well.
Fixes#236
I have:
- [x] Written a clear PR title and description (above)
- [x] Signed the [Khan Academy CLA](https://www.khanacademy.org/r/cla)
- [x] Added tests covering my changes, if applicable
- [x] Included a link to the issue fixed, if applicable
- [x] Included documentation, for new features
- [x] Added an entry to the changelog
Lint is failing with some inscrutable panic (on a commit with no code
changes). Let's try bumping the version in case they fixed it.
Additionally, the new version's github action uses Go 1.19, which means
it pulls in gofmt updates to match the new [doc-comment formatting rules][1].
So I added Go 1.19 to our list of versions to test (fixes#216)
and updated some of our doc-comments to format better in the
new world (mostly using the new link syntax).
[1]: https://go.dev/doc/comment
Test plan: make lint
We noticed that the output from genqlient can be non-deterministic
when a graphql query queries a field that's an interface. This PR
fixes that by sorting the types as soon as we extract them from
the schema.
Current implementation always uses POST as the transport mechanism. Adding GET support enables usage of GET queries for caching simply via URL.
Some notes:
- I left the existing API for creating a new client as is, but the implementation could be much cleaner by introducing some sort of configuration struct when creating a new client
- The construction of the query parameters follows the logic from Apollo's client implementation, which can be found here https://github.com/apollographql/apollo-client/blob/8beb4820edc6352996e08f7f73bde3573f1eb666/src/link/http/rewriteURIForGET.ts
- Updated integration tests to use both sets of clients. Updating the tests to use a test suite would be cleaner
This enables accessing the Extensions field, as defined in the response
format: https://spec.graphql.org/October2021/#sec-Response-Format
Extensions can be enabled using configuration option use_extensions.
This will change the return parameters of the generated client
functions. Making it a breaking change if enabled.
Since extensions are untyped as defined in the spec, the Client will
return an interface of type map[string]interface{}.
`Code generated by` disclaimer should be at the top of the file, as it is demonstrated in [this article](https://go.dev/blog/generate) on The Go Blog.
The current placing breaks integration with some code formatters and linters which do no skip files generated by `genqlient`.
Multiple schema-files are now supported as of #134, but the support was
a bit different from how we did multiple operation-files. Before anyone
starts to depend on the ways the syntaxes differ, let's just make them
the same. Since it's easy, I also added support for having just a
single operations-file.
I also realized while writing this that the type-change is technically
breaking (if you call from Go), so documented it as such. I think
this is unlikely to affect many people.
Test plan: make check
## Summary:
If your type implements an interface, we add getter methods for the
shared fields, so that those may be accessed via the interface. But it
turns out occasionally it's useful to have these getter methods when
they don't implement a GraphQL interface, so you can use two
genqlient-generated types in the same function if they have the same
fields. (This comes up most often when you have a GraphQL union that
maybe should really be an interface, or if you don't yet support
interfaces implementing other interfaces (indeed our parser doesn't
either). But one can imagine other use cases.)
We can't predict how you want to do that, so we can't generate the
interface, but we can generate the methods, so you can define the
interface and do a type assertion from there. Since these methods are
pretty simple to generate, we just do it always. (As with #120, if
binary size becomes an issue we could later add an option to only
generate methods that are truly needed but including them seems like the
better default.)
This also fixes a subtle and rare bug, which would have become much more
common (indeed existing tests caught it). Specifically, if you have a
query like
```graphql
fragment FragmentOne on T { id }
fragment FragmentTwo on T { id }
query Q {
f { # interface type T
...FragmentOne
...FragmentTwo
}
}
```
since both `FragmentOne` and `FragmentTwo` request some common field,
say `id`, we generate a method `GetId` on each one. But since
`FragmentOne` and `FragmentTwo` are both on `T`, we also include their
interfaces in the interface we generate for the type of `f`, `QFT`. So
`QFT` includes a method `GetId`. But on the implementations, the two
methods conflict, and neither gets promoted; this causes various code to
fail to compile. With this change, this would have happened much more
frequently -- even if only one of the two fragments is on `T`, as long
as both request the field. Anyway, we now generate explicit methods on
each struct for all of its recursively emebedded fields -- using the
logic from #120 to compute them -- so that we don't need to rely on
method-promotion.
## Test plan:
make tesc
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/126
## 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:
When genqlient generates output types, it generates whatever code is
necessary to unmarshal them. Conversely, when it generates input types,
it generates whatever code is necessary to marshal. This is all that's
needed for genqlient itself: it never needs to marshal output types or
unmarshal input types.
But maybe you do! (For example, to put the responses in a cache, which
is the use case that @csilvers hit at Khan, although there are others
one can imagine.) While we can't support every serialization format you
might want (at least not without adding plugins or some such), it's not
unreasonable to expect that since genqlient can read JSON, it can write
it too. Sadly, in the past this was not true for types requiring custom
unmarshaling logic, for several reasons.
In this commit I implement logic to always write both marshalers and
unmarshalers whenever they're needed to be able to correctly round-trip
the types, even though genqlient doesn't do so. I wasn't starting from
scratch, since of course we already write both marshalers and
unmarshalers in some cases. But this ended up requiring surprisingly
large changes on the marshaling side, mostly to correctly support
embedding (which we use for named fragments).
Specifically, as the comments in `types.go` discuss, the most difficult
issue is spreads with duplicate fields, which translate to Go embedded
fields which end up hidden from the json-marshaler. Ultimately, I had
to do things quite differently from unmarshaling, and essentially
flatten the type when we write marshaler. But in the end it's not so
ugly -- indeed arguably it's cleaner! Mainly it's just different.
One thing to note is that we do marshal `__typename` based on
what we know about the types; users need not fill it in (and if they
do we'll ignore it). This seemed to me to be a better UX, and
didn't add much complexity.
In general, I begin to wonder whether using `encoding/json` at all is
really right for genqlient: we're doing a lot of work to appease it,
despite knowing what our types look like. I think it would still be a
significant increase in lines of code to roll our own, but that code
would perhaps be simpler, and would surely be faster (although if we
just want the speed gains we could use another JSON-generator library,
see also #47). Anyway, something to think about in the future.
## Test plan:
make tesc
Author: benjaminjkraft
Reviewers: csilvers, StevenACoffman, benjaminjkraft, 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/120
## Summary:
There were a few bugs here, one of which Craig came across when pulling
the custom-unmarshaler change into webapp:
1. If you have an optional field with a custom unmarshaler, and the
server omits the field from the response entirely (i.e. does not
write `"myField": null`), we would still call your unmarshaler with
an input of `[]byte(nil)`. This is just wrong; it's our job to do
the nil-check. (This is the one Craig found; in practice gqlgen
servers do not do this and I think the spec says not to although it's
a bit fuzzy on the matter of serialization. But in practice we have
mocks that do it -- for required fields even! -- and it seems better
to handle it than pass you data on which you'll probably err or even
panic.)
2. If you have an optional field with a custom unmarshaler, and the
server returns an explicit null (i.e. `"myField": null`), we would
call your unmarshaler with `[]byte("null")`. In principle the intent
was you're supposed to implement that, as [`json.Unmarshaler`
advises][1]. But (a) I forgot to document that, and (b) in practice
`json.Unmarshal` [does *not* call you in that case][2], i.e. its
advice is unnecessary. So I think it's better for us to just match
it, and not call you. (And in that case I see no reason to bother
documenting the advice.)
3. If you have an optional, `pointer: true` field with a custom
marshaler, the reverse of (2) applies: if the pointer is nil, we
shouldn't really call you. (Indeed if you were a real
`json.Marshaler` with a value-method rather than a pointer-method,
trying to call you might panic!) Note we don't need to explicitly
write "null"; we just leave the `json.RawMessage` as nil, and
`json.Marshal` [handles that][3].
4. We handle interface types effectively the same as custom
unmarshalers, just we generate the unmarshaler. So if you have an
optional field with interface type, (1) would also apply there; our
generated unmarshaler returns an error in this case.
5. While (2) doesn't apply to such optional interface fields (because we
do the customary `if string(b) == "null"` check -- this I at least
thought to test), if you set `pointer: true` on the field, we would
still call the unmarshaler on the value, and it would no-op, but only
*after* we initialized the pointer. Put more simply, we'd return a
non-nil pointer to nil interface, rather than a nil pointer; this is
wrong since the whole point of `pointer: true` is you only get a
non-nil pointer if your value is nil! Of course, in practice there's
little reason to use `pointer: true` on interface fields, and indeed
this stuff gets so confusing my test was even wrong.
In this commit I fix all the bugs, by adding appropriate nil-checks to
wrap the unmarshaler-calls. The templates are, as always, a bit
confusing, but the generated code makes it clear what changed.
Note we'll want to land this before cutting a release with custom
marshaler/unmarshaler support, because the first three bugs are
potentially quite noticeable. (The latter two are in `v0.1.0`, but
presumably quite rare.)
[1]: https://pkg.go.dev/encoding/json#Unmarshaler
[2]: https://play.golang.org/p/Pw6zNN8trGO
[3]: https://play.golang.org/p/crTfnT7ePte
Issue: https://phabricator.khanacademy.org/D74453#inline-558571
## Test plan:
make tesc
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: https://github.com/Khan/genqlient/pull/116
## Summary:
This is useful if you want to bind to a type you don't control (or use
for other things) but need different serialization than its default.
This is a feature gqlgen has and we've found it very useful. For
example, in webapp we want to bind `DateTime` to `time.Time`, but its
default serialization is not compatible with Python, so currently we
have to bind to a wrapper type and cast all over the place, which is
exactly the sort of boilerplate genqlient is supposed to avoid.
For unmarshaling, the implementation basically just follows the existing
support for abstract types; instead of calling our own generated
helper, we now call your specified function. This required some
refactoring to abstract the handling of custom unmarshalers generally
from abstract types specifically, and to wire in not only the
unmarshaler-name but also the `generator` (in order to compute the right
import alias).
For marshaling, I had to implement all that stuff over again; it's
mostly parallel to unmarshaling (and I made a few minor changes to
unmarshaling to make the two more parallel). Luckily, after #103 I at
least only had to do it once, rather than implementing the same
functionality for arguments and for input-type fields. It was still
quite a bit of code; I didn't try to be quite as completionist about the
tests as with unmarshal but still had to add a few.
Issue: https://github.com/Khan/genqlient/issues/38
## Test plan:
make check
Author: benjaminjkraft
Reviewers: StevenACoffman, dnerdy, benjaminjkraft, 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/104
## Summary:
In this commit I refactor the argument-generation logic to move most of
the code out of the template and into the type-generator. This logic
predates #51, and I didn't think to update it there, but I think it
benefits from similar treatment, for similar reasons.
Specifically, the main change is to treat variables as another struct
type we can generate, rather than handling them inline as a
`map[string]interface{}`. Users still pass them the same way, but
instead of putting them into a `map[string]interface{}` and JSONifying
that, we generate a struct and put them there.
This turns out to simplify things quite a lot, because we already have a
lot of code to generate types. Notably, the omitempty code goes from a
dozen lines to basically two, and fixes a bug (#43) in the process,
because now that we have a struct, `json.Marshal` will do our work for
us! (And, once we have syntax for it (#14), we'll be able to handle
field-level omitempty basically for free.) More importantly, it will
simplify custom marshalers (#38, forthcoming) significantly, since we do
all that logic at the containing-struct level, but will need to apply it
to arguments.
It does require two breaking changes:
1. For folks implementing the `graphql.Client` API (rather than just
calling `NewClient`): we now pass them variables as an `interface{}`
rather than a `map[string]interface{}`. For most callers, including
Khan/webapp, this is basically a one-line change to the signature of
their `MakeRequest`, and it should be a lot more future-proof.
2. genqlient's handling of the `omitempty` option has changed to match
that of `encoding/json`, in particular it now never considers structs
"empty". The difference was never intentional (I just didn't realize
that behavior of `encoding/json`); arguably our behavior was more
useful but I think that's outweighed by the value of consistency with
`encoding/json` as well as the simpler and more correct
implementation (fixing #43 is actually quite nontrivial otherwise).
Once we have custom unmarshaler support (#38), users will be able to
map a zero value to JSON null if they wish, which is mostly if not
entirely equivalent for GraphQL's purposes.
Issue: https://github.com/Khan/genqlient/issues/38
Issue: https://github.com/Khan/genqlient/issues/43
## 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, ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14)
Pull Request URL: https://github.com/Khan/genqlient/pull/103
## Summary:
We guarantee that we never return a nil response, so you can safely do
```
resp, err := myQuery(...)
return resp.Field.SubField, err
```
And furthermore, if the error was a GraphQL error, `resp` may even be
nonzero; other, non-failing fields may be set. (This depends on the
server, of course.) But we weren't testing either of those. Now we do.
## Test plan:
make check
Author: benjaminjkraft
Reviewers: jvoll, aberkan, dnerdy, MiguelCastillo, mahtabsabet
Required Reviewers:
Approved By: 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/83
## Summary:
In previous commits I added support to genqlient for interfaces,
inline fragments, and, most recently, named fragments of concrete
(object) type. This leaves only named fragments of interface type!
Like other named fragments, these are useful for code-sharing,
especially if you want some code that can handle the same fields of
several different types.
As seems to be inevitable with genqlient, this was mostly pretty
straightforward, although there turned out to be surprisingly many
places we needed to add some handling; almost anywhere that touches
interfaces *or* named fragments needed some updates. But it's all
hopefully fairly clear code.
As a part of this change I made three semi-related improvements:
1. I refactored the handling of descriptions (i.e. GoDoc), because it
was getting more and more confusing and duplicative. I'm still not
sure how much of it it makes sense to inline vs. separate, but I
think this is better than it was. This resulted in some minor
changes to descriptions, generally in the direction of making things
more consistent.
2. I bumped the minimum Go version to 1.14 so we can guarantee support
for duplicate interface methods. These are useful for
abstract-in-absstract spreads; we generate an interface for the
fragment, and (if the fragment-type implements the scope-type) we
embed it into the interface we generate for its spread-context, and
if the two have a duplicated field we thus duplicate the method. It
wouldn't be impossible to support this on 1.13 (maybe just by
omitting said embed) but it didn't seem worth it. This also removes
a few special-cases in tests.
3. I added a bunch of code to better format syntax errors in the
generated code (which we see from `gofmt`). This is mostly just an
internal improvement; I wrote it because I got annoyed while hunting
down a few such errors..
Fixes, at last, #8.
Issue: https://github.com/Khan/genqlient/issues/8
## Test plan:
make check
Author: benjaminjkraft
Reviewers: dnerdy, benjaminjkraft, aberkan, MiguelCastillo
Required Reviewers:
Approved By: dnerdy
Checks: ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint
Pull Request URL: https://github.com/Khan/genqlient/pull/79
## Summary:
In previous commits I added support to genqlient for interfaces and
inline fragments. This means the only query structures that remain are
named fragments and their spreads, e.g.
```
fragment MyFragment on MyType { myField }
query MyQuery { getMyType { ...MyFragment } }
```
Other than mere completionism, these are potentially useful for code
sharing: you can spread the same fragment multiple places; and then
genqlient can notice that and generate the same type for each. (They
can even be shared between different queries in the same package.)
In this commit I add support for named fragments of concrete
(object/struct, not interface) type, spread into either concrete or
abstract scope. For genqlient's purposes, these are a new "root"
type-name, just like each operation, and are then embedded into the
appropriate struct. (Using embeds allows their fields to be referenced
as fields of the containing type, if convenient. Further design
considerations are discussed in DESIGN.md.)
This requires new code in two main places (plus miscellaneous glue),
both nontrivial but neither particularly complex:
- We need to actually traverse both structures and generate the types
(in `convert.go`).
- We need to decide which fragments from this package to send to the
server, both for good hyigene and because GraphQL requires we send
only ones this query uses (in `generate.go`).
- We need a little new wiring for options -- because fragments can be
shared between queries they get their own toplevel options, rather
than inheriting the query's options.
Finally, this required slightly subtler changes to how we do
unmarshaling (in `types.go` and `unmarshal.go.tmpl`). Basically,
because embedded fields' methods, including `UnmarshalJSON`, get
promoted to the parent type, and because the JSON library ignores their
fields when shadowed by those of the parent type, we need a little bit
of special logic in each such parent type to do its own unmarshal and
then delegate to each embed. This is similar (and much simpler) to
what we did for interfaces, although it required some changes to the
"method-hiding" trick (used for both). It's only really necessary in
certain specific cases (namely when an embedded type has an
`UnmarshalJSON` method or a field with the same name as the embedder),
but it's easier to just generate it always. This is all described in
more detail inline.
This does not support fragments of abstract type, which have their own
complexities. I'll address those, which are now the only remaining
piece of #8, in a future commit.
Issue: https://github.com/Khan/genqlient/issues/8
## Test plan:
make check
Author: benjaminjkraft
Reviewers: dnerdy, benjaminjkraft, aberkan, MiguelCastillo
Required Reviewers:
Approved By: dnerdy
Checks: ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ 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/75
## Summary:
ContextType is in use at Khan as a part of our ka-context system; it
basically just lets you configure the type to pass as the `ctx` argument
to genqlient helpers (or say to omit such an argument). ClientGetter I
wrote thinking we might use it; then we didn't (because we have a few
different clients we may use) but it's not much code and may be helpful
to others. In this commit I clean up, document, and add tests for both
options.
The cleanup is mainly for ClientGetter, which was kind of broken before
because it was a Go snippet but couldn't specify imports. I was
thinking maybe you want to be able to write `ctx.Something()`, but I
just don't see how to make it work, so I made it a function of context,
which is probably the better idea anyway.
Additionally, I improved the documentation for both, and added tests for
those and several other config options that weren't completely tested.
Fixes#5.
Issue: https://github.com/Khan/genqlient/issues/5
## 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/77
## Summary:
When adding support for interfaces, I did not do the type-names as I
intended: they came out to be `MyFieldMyType`, not
`MyInterfaceMyFieldMyType`, which is inconsistent, but not strictly
wrong. But once supporting fragments, this is also now incorrect.
(Exactly why is described in the comments inline.) In this commit, in
any case, I fix it.
To do that, I finally did the last of the refactors I've been hoping to
do but unable to successfully implement, which is to make the type-name
and type-name-prefix management clearer. In the past it was kind of
spread out, and each caller would have to pass the right name into
`convertDefinition`, which go quite unwieldy. Now, the case that really
wanted that -- the operation toplevel -- just does it own thing; and the
main name-generation code is factored out into a separate file with
tests, and with a long comment that goes into all the details of the
algorithm that the design-doc didn't cover. (I even had some fun using
a linked list to implement the prefix-stack!)
This allowed me to fix the above bug fairly easily -- actually the fix
was pretty much automatic once I understood how to organize things.
There is one change which is that if your query name is unexported, we
no longer do the same with the input-type names; it's unclear to me if
anyone will actually care about this behavior (Khan always makes the
queries exported) but if they did it was very inconsistent (only at the
query toplevel, and only for input-objects, not enums), so we can
reimplement it properly if that comes up. As a bonus fix, we now better
handle the case where your type-names are lowercase, which is legal if
nonstandard GraphQL.
Issue: https://github.com/Khan/genqlient/issues/8
## Test plan:
make tesc
Author: benjaminjkraft
Reviewers: dnerdy, benjaminjkraft, 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/71
🖍 _This is an audit!_ 🖍
## Summary:
These got broken by the merge.
## Test plan:
make check
Author: benjaminjkraft
Auditors: aberkan, csilvers, dnerdy, MiguelCastillo
Required Reviewers:
Approved by:
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/72
## Summary:
In practice, at Khan at least, this is easy to mess up when writing
mocks, because you write the mock by looking at the query, and the query
doesn't say it's asking for `__typename` (because genqlient
automatically adds that). A sufficiently-smart mocking library might be
able to fix that, or detect it at least, but in any case, we can give a
clearer error.
I also removed an unrelated TODO that was done.
Issue: https://khanacademy.slack.com/archives/C01120CNCS0/p1630019788014000
## 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/68
## 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:
Right now, if you make a query like `{ myInterface { field } }`, you
have to type-switch on all the possible implementations of `myInterface`
to get at `field`. Now, we generate getter-methods (e.g. `GetField`),
to make that access easier. Of course this only applies to shared
fields (which for now are the only ones, but once we support fragments
will no longer be).
This also includes a small change to the way we generate type-names for
interfaces: we no longer include the name of the concrete type in the
interface we propagate forward, so we generate
`MyInterfaceMyFieldMyType`, not `MyInterfaceMyImplMyFieldMyType`, in the
case where you have an interface `MyInterface` implemented by `MyImpl`
(and maybe other types) with field `myField: MyType`. This is necessary
so the getter method returns a well-defined type, and also probably
convenient for calling code. It will have to get a little bit more
complicated once we support fragments, where you could have two
implementing types with identically-named fields of different types, but
I think it'll be easiest to figure out how to deal with that when
implementing fragments.
While I was in the area, I added to the interface doc-comment a list of
the implementations. (In GraphQL, we're guaranteed to know them all
assuming our schema is up to date.)
Issue: https://github.com/Khan/genqlient/issues/8
## Test plan:
make check
Author: benjaminjkraft
Reviewers: benjaminjkraft, 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/57
## 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 remove one of the limitations of our support for
interfaces, from #52, by adding support for list-of-interface fields.
This was surprisingly complex! The issue is that, as before, it's the
containing type that has to do all the glue work -- and it's that glue
work that is complicated by list-of-interface fields.
All in all, it's not that much new code, and by far the hard part is
just 20 lines in the UnmarshalJSON template (which come with almost
twice as many lines of comments to explain them). It may be easiest to
start by reading some of the generated code, and then read the template.
I also added support for such fields with `pointer: true` specified,
such that the type is `[][]...[]*MyInterface`, although I don't know why
you would want that. This does *not* allow e.g. `*[]*[][]*MyInterface`;
that would require a way to specify it (see #16) but also add some extra
complexity (as we'd have to actually walk the type-unwrap chain
properly, instead of just counting the number of slices and whether
there's a pointer).
Issue: https://github.com/Khan/genqlient/issues/8
## Test plan:
make check
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, ⌛ Lint, ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13)
Pull request URL: https://github.com/Khan/genqlient/pull/54
## 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
## Summary:
We have lots of tests covering codegen, but not a lot that actually run
the code. For things where all we do is generate types, that's (mostly)
fine (especially now that we actually build the code), but as we
generate more nontrivial non-type code we need to actually run it.
So I wrote some integration tests that spin up a little gqlgen
server, and make calls to it; we can add more over time especially as
the JSON marshalling logic gets complex (to support fragments).
They're more work to write than the snapshot tests, but of course they
can test a lot more.
In addition to gqlgen, I pulled in testify assert/require, because I
really wanted to be able to use assert.Equal and such for these. I
didn't bother converting existing tests, although I assume they will
become useful elsewhere in time. Both gqlgen and testify are of course
only used in tests.
Fixes#21 and #24.
Issue: https://github.com/Khan/genqlient/issues/21
Issue: https://github.com/Khan/genqlient/issues/24
## Test plan:
make check
Author: benjaminjkraft
Reviewers: aberkan, dnerdy, benjaminjkraft, csilvers, MiguelCastillo
Required Reviewers:
Approved by: aberkan, 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/50
Ben Kraft