c61d7acaa59c965701e602cca1f6cbb7cc21814f
7 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Dylan R. Johnston
|
c61d7acaa5 |
Add "generic" option to the "optional" configuration for handling nullable types (#252)
This is an implementation for #251, it adds a new `"generic"` option for the `"optional"` configuration, and a companion type `"optional_generic_type"` which is a fully qualified type with a placeholder `%` for the generic parameter. Co-authored-by: Dylan R. Johnston <dylan.johnston@familyzone.com> Co-authored-by: Ben Kraft <ben@benkraft.org> |
||
|
Ben Kraft
|
f4c981031e |
Allow genqlient types to be marshaled safely (#120)
## 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 |
||
|
Ben Kraft
|
65c3e20ee6 |
Fix bugs relating to optional fields with custom (un)marshalers (#116)
## 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
|
||
|
Ben Kraft
|
8de55d352e |
Add support for binding with a custom marshal/unmarshal function (#104)
## 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 |
||
|
Ben Kraft
|
6c86eed770 |
Clean up, test, and document ContextType and ClientGetter options (#77)
## 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 |
||
|
Ben Kraft
|
222d6f191a |
Document and improve support for binding non-scalars to a specific type (#69)
## 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 |
||
|
Ben Kraft
|
3c11f07e62 |
Make test ID type actually exist
This way I can start to test that the code compiles. |