Skip to content

Commit 65c3e20

Browse files
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: #116
1 parent 47e9cea commit 65c3e20

24 files changed

+598
-219
lines changed

docs/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ When releasing a new version:
3434

3535
- The `omitempty` option now works correctly for struct- and map-typed variables, matching `encoding/json`, which is to say it never omits structs, and omits empty maps. (#43)
3636
- Generated type-names now abbreviate across multiple components; for example if the path to a type is `(MyOperation, Outer, Outer, Inner, OuterInner)`, it will again be called `MyOperationOuterInner`. (This regressed in a pre-v0.1.0 refactor.) (#109)
37+
- Previously, interface fields with `# @genqlient(pointer: true)` would be unmarshaled to `(*MyInterface)(*<nil>)`, i.e. a pointer to the untyped-nil of the interface type. Now they are unmarshaled as `(*MyInterface)(<nil>)`, i.e. a nil pointer of the pointer-to-interface type, as you would expect.
3738

3839
## v0.1.0
3940

generate/marshal.go.tmpl

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,15 @@ func (v *{{.GoName}}) MarshalJSON() ([]byte, error) {
3030
for i, src := range src {
3131
dst := &(*dst)[i]
3232
{{end -}}
33+
{{/* src now has type <GoType>; dst is json.RawMessage */ -}}
34+
{{if $field.GoType.IsPointer -}}
35+
{{/* If you passed a pointer, and it's nil, don't call the
36+
marshaler. This matches json.Marshal's behavior. */ -}}
37+
if src != nil {
38+
{{end -}}
3339
var err error
3440
*dst, err = {{$field.Marshaler $.Generator}}(
35-
{{/* src is a pointer to the struct-field (or field-element, etc.).
41+
{{/* src is the struct-field (or field-element, etc.).
3642
We want to pass a pointer to the type you specified, so if
3743
there's a pointer on the field that's exactly what we want,
3844
and if not we need to take the address. */ -}}
@@ -41,6 +47,9 @@ func (v *{{.GoName}}) MarshalJSON() ([]byte, error) {
4147
return nil, fmt.Errorf(
4248
"Unable to marshal {{$.GoName}}.{{$field.GoName}}: %w", err)
4349
}
50+
{{if $field.GoType.IsPointer -}}
51+
}{{/* end if src != nil */}}
52+
{{end -}}
4453
{{range $i := intRange $field.GoType.SliceDepth -}}
4554
}
4655
{{end -}}

generate/testdata/snapshots/TestGenerate-ComplexInlineFragments.graphql-ComplexInlineFragments.graphql.go

Lines changed: 42 additions & 30 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

generate/testdata/snapshots/TestGenerate-ComplexNamedFragments.graphql-ComplexNamedFragments.graphql.go

Lines changed: 28 additions & 20 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

generate/testdata/snapshots/TestGenerate-CustomMarshal.graphql-CustomMarshal.graphql.go

Lines changed: 7 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

generate/testdata/snapshots/TestGenerate-CustomMarshalSlice.graphql-CustomMarshalSlice.graphql.go

Lines changed: 8 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

generate/testdata/snapshots/TestGenerate-InterfaceListField.graphql-InterfaceListField.graphql.go

Lines changed: 14 additions & 10 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

generate/testdata/snapshots/TestGenerate-InterfaceListOfListsOfListsField.graphql-InterfaceListOfListsOfListsField.graphql.go

Lines changed: 15 additions & 11 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

generate/testdata/snapshots/TestGenerate-InterfaceNesting.graphql-InterfaceNesting.graphql.go

Lines changed: 14 additions & 10 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)