Files
genqlient/generate/unmarshal.go.tmpl
T
Ben KraftandGitHub 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
2021-09-27 20:35:39 -07:00

134 lines
6.1 KiB
Cheetah

{{/* (the blank lines at the start are intentional, to separate
UnmarshalJSON from the function it follows) */}}
func (v *{{.GoName}}) UnmarshalJSON(b []byte) error {
{{/* Standard convention for unmarshalers is to no-op on null. */}}
if string(b) == "null" {
return nil
}
{{/* We want to specially handle certain fields, but unmarshal everything
else normally. To handle abstract fields and fields with custom
unmarshalers, first we unmarshal them into a json.RawMessage, and then
handle those further, below. Embedded fields don't need the
json.RawMessage; we just use our input again. Either way, we first
want to call json.Unmarshal on the receiver (v). But if we do that
naively on a value of type `.Type`, it will call this function again,
and recurse infinitely. So we make a wrapper type which embeds both
this type and NoUmnarshalJSON, which prevents either's UnmarshalJSON
method from being promoted. For more on why this is so difficult, see
https://github.com/benjaminjkraft/notes/blob/master/go-json-interfaces.md.
(Note there are a few different ways "hide" the method, but this one
seems to be the best option that works if this type has embedded types
with UnmarshalJSON methods.)
*/}}
{{/* TODO(benkraft): Omit/simplify the first pass if all fields are
embedded/abstract. */ -}}
var firstPass struct{
*{{.GoName}}
{{range .Fields -}}
{{if and .NeedsUnmarshaler (not .IsEmbedded) -}}
{{.GoName}} {{repeat .GoType.SliceDepth "[]"}}{{ref "encoding/json.RawMessage"}} `json:"{{.JSONName}}"`
{{end -}}
{{end -}}
{{/* TODO(benkraft): In principle you might have a field-name that
conflicts with this one; avoid that. */ -}}
{{ref "github.com/Khan/genqlient/graphql.NoUnmarshalJSON"}}
}
firstPass.{{.GoName}} = v
err := {{ref "encoding/json.Unmarshal"}}(b, &firstPass)
if err != nil {
return err
}
{{/* Now, handle the fields needing special handling. */}}
{{range $field := .Fields -}}
{{if $field.NeedsUnmarshaler -}}
{{if $field.IsEmbedded -}}
{{/* Embedded fields are easier: we just unmarshal the same input into
them. (They're also easier because they can't be lists, since they
arise from GraphQL fragment spreads.) */ -}}
err = {{$field.Unmarshaler $.Generator}}(
b, &v.{{$field.GoType.Unwrap.Reference}})
if err != nil {
return err
}
{{else -}}
{{/* For other fields (abstract or custom unmarshaler), first, call the
unmarshaler (our unmarshal-helper, or the user-specified one,
respectively). This gets a little complicated because we may have
a slice field. So what we do is basically, for each field of type
`[][]...[]MyType`:
dst := &v.MyField // *[][]...[]MyType
src := firstPass.MyField // [][]...[]json.RawMessage
// repeat the following three lines n times; each time, inside
// the loop we have one less layer of slice on src and dst
*dst = make([][]...[]MyType, len(src))
for i, src := range src {
// We need the &(*dst)[i] because at each stage we want to
// keep dst as a pointer. (It only really has to be a
// pointer at the innermost level, but it's easiest to be
// consistent.)
dst := &(*dst)[i]
// (now we have `dst *MyType` and `src json.RawMessage`)
__unmarshalMyType(dst, src)
} // (also n times)
Note that if the field also uses a pointer (`[][]...[]*MyType`), we
now pass around `*[][]...[]*MyType`; again in principle
`[][]...[]*MyType` would work but require more special-casing. Thus
in the innermost loop, `dst` is of type `**MyType`, so we have to
pass `*dst` to the unmarshaler. Of course, since MyType is an
interface, I'm not sure why you'd any of that anyway.
One additional trick is we wrap everything above in a block ({ ... }),
so that the variables dst and src may take on different types for
each field we are handling, which would otherwise conflict. (We could
instead suffix the names, but that makes things much harder to read.)
*/}}
{
dst := &v.{{$field.GoName}}
src := firstPass.{{$field.GoName}}
{{range $i := intRange $field.GoType.SliceDepth -}}
*dst = make(
{{repeat (sub $field.GoType.SliceDepth $i) "[]"}}{{if $field.GoType.IsPointer}}*{{end}}{{$field.GoType.Unwrap.Reference}},
len(src))
for i, src := range src {
dst := &(*dst)[i]
{{end -}}
{{/* dst now has type *<GoType>; dst is json.RawMessage */ -}}
{{/* If the field is null in the input, skip calling unmarshaler.
(This matches json.Unmarshal.) If the field is missing entirely
from the input, we will have an uninitialized json.RawMessage;
handle that likewise. */ -}}
if len(src) != 0 && string(src) != "null" {
{{if $field.GoType.IsPointer -}}
{{/* In this case, the parent for loop did `make([]*MyType, ...)`
and we have a pointer into that list. But we actually still
need to initialize the *elements* of the list. */ -}}
*dst = new({{$field.GoType.Unwrap.Reference}})
{{end -}}
err = {{$field.Unmarshaler $.Generator}}(
src, {{if $field.GoType.IsPointer}}*{{end}}dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal {{$.GoName}}.{{$field.GoName}}: %w", err)
}
}
{{range $i := intRange $field.GoType.SliceDepth -}}
}
{{end -}}
}
{{end -}}{{/* end if/else .IsEmbedded */ -}}
{{end -}}{{/* end if .NeedsUnmarshaler */ -}}
{{end}}{{/* end range .Fields */ -}}
return nil
}