## Summary:
Having implemented support for interfaces, it's time to implement
support for fragments. And it turns out there's actually another design
decision I hadn't really thought about when thinking about interfaces!
In this commit I add a sketch of the design -- two proposed designs
really. This one I think will be easier to change later (via a flag),
but opinions are still welcome.
Issue: https://github.com/Khan/genqlient/issues/8
## Test plan:
read it
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/59
## Summary:
We had this setting called "scalars", which said: bind this GraphQL type
to this Go type, rather than the one you would normally use. It's
called that because it's most useful for custom scalars, where "the one
you would normally use" is "error: unknown scalar". But nothing ever
stopped you from using it for a non-scalar type. I was planning on
removing this functionality, because it's sort of a rough edge, but a
discussion with Craig found some good use cases, so instead, in this
commit, I document it better and add some slightly nicer ways to specify
it.
Specifically, here are a few potential non-scalar use cases:
- bind a GraphQL enum to a nonstandard type (or even `string`)
- bind an input type to some type that has exactly the fields you want;
this acts as a sort of workaround for issues #14 and #44
- bind an object type to your own struct, so as to add methods to it
(this is the use case Craig raised)
- bind an object type to your own struct, so as to share it between
multiple queries (I believe named fragments will address this case
better, but it doesn't hurt to have options)
- bind a GraphQL list type to a non-slice type in Go (presumably one
with an UnmarshalJSON method), or any other different structure
The latter three cases still have the sharp edge I was originally
worried about, which is that nothing guarantees that the fields you
request in the query are the ones the type expects to get. But I think
it's worth having the option, with appropriate disclaimers.
The main change to help support that better is that you can now specify
the type inline in the query, as an alternative to specifying it in the
config file; this means you might map a given object to a given struct,
but only in some cases, and when you do you have a chance to look at the
list of fields you're requesting.
Additionally, I renamed the config field from "scalars" to "bindings"
(but mentioned it in a few places where you might go looking for how to
map scalars, most importantly the error message you get for an unknown
(custom) scalar). While I was making a breaking change, I also changed
it to be a `map[string]<struct>` instead of a `map[string]string`,
because I expect to add more fields soon, e.g. to handle issue #38.
Finally, since the feature is now intended/documented, I added some
tests, although it's honestly quite simple on the genqlient side.
## Test plan:
make tesc
Author: benjaminjkraft
Reviewers: csilvers, aberkan, dnerdy, MiguelCastillo
Required Reviewers:
Approved by: csilvers
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/69
🖍 _This is an audit!_ 🖍
## Summary:
We've completed support for interfaces! Fragments are still pending,
but interfaces by themselves should be usable. But I forgot to actually
remove the flag they were behind. In this commit I do!
Issue: https://khanacademy.slack.com/archives/C01120CNCS0/p1630012808004100
## 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/67
## 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:
I've felt for a while that types.go is way too confusing, and as I
started to implement some of the more complex cases of generating
interface-types, the cracks were really starting to show. Luckily, I
also finally realized how to fix it: we need to separate the process of
traversing the GraphQL operation and schema to decide what types to
generate from the process of actually generating those types. This
requires an extra set of intermediate data structures, but I think it
makes things quite a lot easier to understand -- and, importantly, it
means that the code-generation doesn't need to go in the order we
traverse the query/schema.
So in this commit, I did that huge refactor. It's probably best to just
review types.go and traverse.go as if they were new; the old code was
quite hard to understand and the new code will hopefully make a lot more
sense. (And to that end, review comments about what could be organized
better or needs more documentation are very much in order, even for code
that is mostly unchanged.)
This does introduce one bug, sort of, which is that rather than
generating broken code for list-of-interface fields, we generate no code
at all. (A TODO in unmarshal.go describes why.) I'll fix this when I
add support for those fields. (It's all behind the AllowBrokenFeatures
flag, anyway.) Otherwise, the only changes to generated code are that a
few methods are ordered differently, because we now generate the
implements-interface methods with the interface, rather than the
implementations, as it's much simpler that way. (In GraphQL, unlike Go,
we know the list of all possible implementations of each interface, so
this is possible.)
## Test plan:
golangci-lint run ./... && go test ./...
Author: benjaminjkraft
Reviewers: dnerdy, benjaminjkraft, aberkan, csilvers, 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
Pull request URL: https://github.com/Khan/genqlient/pull/51
## Test plan
read it
Author: benjaminjkraft
Reviewers: aberkan, dnerdy, MiguelCastillo
Required Reviewers:
Approved by: aberkan
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/58
## Summary:
As Mark pointed out in a review where I realized that the *example* had
a bogus config key (which I hadn't noticed earlier because it was just
using the default value), we should probably do a strict-unmarshal;
there's no reason you should have random extra keys in your config and
if you do it's probably a mistake. (Or maybe you're running a too-old
version of genqlient for your codebase, which you probably also want to
know.) Now we do.
## Test plan:
- `make check`
- in webapp, `go mod edit -replace github.com/Khan/genqlient=../genqlient`
then `make genqlient` produces no diffs (except go.mod/go.sum).
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/55
## 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
## Summary:
It would be nice to have some linting beyond `go vet`! Now we do. I
started by copying the config from Khan/webapp. I did remove a couple
of staticcheck checks that I didn't feel were useful. (Note also that
exportloopref is the replacement for scopelint in newer golangci-lint.)
Included are all the needed lint fixes; most are stylistic but the
changes in the example are a (minor) bugfix.
Fixes#22.
Issue: https://github.com/Khan/genqlient/issues/22
## 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/49
The design doc talks about the zero value for the `string` and `*string` types and then references `0` and `null`. This PR changes these values to `""` and `nil`.
Craig pointed out this is a bit confusing when you don't have all the
fields. Now we say so, but still include the type's description in case
it's useful.
Fixes#37.
Ben Kraft