talksik/genqlient
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix bugs relating to optional fields with custom (un)marshalers (#116)

Browse Source 
## 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
This commit is contained in:
Ben Kraft
2021-09-27 20:35:39 -07:00
committed by GitHub
parent 47e9cea72e
commit 65c3e20ee6
24 changed files with 598 additions and 219 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/integration/generated.go
+173 -46
View File
@@ -40,11 +40,13 @@ func (v *AnimalFields) UnmarshalJSON(b []byte) error {
{
dst := &v.Owner
src := firstPass.Owner
err = __unmarshalAnimalFieldsOwnerBeing(
src, dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal AnimalFields.Owner: %w", err)
if len(src) != 0 && string(src) != "null" {
err = __unmarshalAnimalFieldsOwnerBeing(
src, dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal AnimalFields.Owner: %w", err)
}
}
}
return nil
@@ -314,6 +316,39 @@ func (v *__queryWithCustomMarshalInput) MarshalJSON() ([]byte, error) {
return json.Marshal(&fullObject)
}
// __queryWithCustomMarshalOptionalInput is used internally by genqlient
type __queryWithCustomMarshalOptionalInput struct {
Date *time.Time `json:"-"`
Id *string `json:"id"`
}
func (v *__queryWithCustomMarshalOptionalInput) MarshalJSON() ([]byte, error) {
var fullObject struct {
*__queryWithCustomMarshalOptionalInput
Date json.RawMessage `json:"date"`
graphql.NoMarshalJSON
}
fullObject.__queryWithCustomMarshalOptionalInput = v
{
dst := &fullObject.Date
src := v.Date
if src != nil {
var err error
*dst, err = testutil.MarshalDate(
src)
if err != nil {
return nil, fmt.Errorf(
"Unable to marshal __queryWithCustomMarshalOptionalInput.Date: %w", err)
}
}
}
return json.Marshal(&fullObject)
}
// __queryWithCustomMarshalSliceInput is used internally by genqlient
type __queryWithCustomMarshalSliceInput struct {
Dates []time.Time `json:"-"`
@@ -396,6 +431,51 @@ type failingQueryResponse struct {
Me failingQueryMeUser `json:"me"`
}
// queryWithCustomMarshalOptionalResponse is returned by queryWithCustomMarshalOptional on success.
type queryWithCustomMarshalOptionalResponse struct {
UserSearch []queryWithCustomMarshalOptionalUserSearchUser `json:"userSearch"`
}
// queryWithCustomMarshalOptionalUserSearchUser includes the requested fields of the GraphQL type User.
type queryWithCustomMarshalOptionalUserSearchUser struct {
Id string `json:"id"`
Name string `json:"name"`
Birthdate time.Time `json:"-"`
}
func (v *queryWithCustomMarshalOptionalUserSearchUser) UnmarshalJSON(b []byte) error {
if string(b) == "null" {
return nil
}
var firstPass struct {
*queryWithCustomMarshalOptionalUserSearchUser
Birthdate json.RawMessage `json:"birthdate"`
graphql.NoUnmarshalJSON
}
firstPass.queryWithCustomMarshalOptionalUserSearchUser = v
err := json.Unmarshal(b, &firstPass)
if err != nil {
return err
}
{
dst := &v.Birthdate
src := firstPass.Birthdate
if len(src) != 0 && string(src) != "null" {
err = testutil.UnmarshalDate(
src, dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal queryWithCustomMarshalOptionalUserSearchUser.Birthdate: %w", err)
}
}
}
return nil
}
// queryWithCustomMarshalResponse is returned by queryWithCustomMarshal on success.
type queryWithCustomMarshalResponse struct {
UsersBornOn []queryWithCustomMarshalUsersBornOnUser `json:"usersBornOn"`
@@ -434,11 +514,13 @@ func (v *queryWithCustomMarshalSliceUsersBornOnDatesUser) UnmarshalJSON(b []byte
{
dst := &v.Birthdate
src := firstPass.Birthdate
err = testutil.UnmarshalDate(
src, dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal queryWithCustomMarshalSliceUsersBornOnDatesUser.Birthdate: %w", err)
if len(src) != 0 && string(src) != "null" {
err = testutil.UnmarshalDate(
src, dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal queryWithCustomMarshalSliceUsersBornOnDatesUser.Birthdate: %w", err)
}
}
}
return nil
@@ -472,11 +554,13 @@ func (v *queryWithCustomMarshalUsersBornOnUser) UnmarshalJSON(b []byte) error {
{
dst := &v.Birthdate
src := firstPass.Birthdate
err = testutil.UnmarshalDate(
src, dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal queryWithCustomMarshalUsersBornOnUser.Birthdate: %w", err)
if len(src) != 0 && string(src) != "null" {
err = testutil.UnmarshalDate(
src, dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal queryWithCustomMarshalUsersBornOnUser.Birthdate: %w", err)
}
}
}
return nil
@@ -513,11 +597,13 @@ func (v *queryWithFragmentsBeingsAnimal) UnmarshalJSON(b []byte) error {
{
dst := &v.Owner
src := firstPass.Owner
err = __unmarshalqueryWithFragmentsBeingsAnimalOwnerBeing(
src, dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal queryWithFragmentsBeingsAnimal.Owner: %w", err)
if len(src) != 0 && string(src) != "null" {
err = __unmarshalqueryWithFragmentsBeingsAnimalOwnerBeing(
src, dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal queryWithFragmentsBeingsAnimal.Owner: %w", err)
}
}
}
return nil
@@ -722,11 +808,13 @@ func (v *queryWithFragmentsResponse) UnmarshalJSON(b []byte) error {
len(src))
for i, src := range src {
dst := &(*dst)[i]
err = __unmarshalqueryWithFragmentsBeingsBeing(
src, dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal queryWithFragmentsResponse.Beings: %w", err)
if len(src) != 0 && string(src) != "null" {
err = __unmarshalqueryWithFragmentsBeingsBeing(
src, dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal queryWithFragmentsResponse.Beings: %w", err)
}
}
}
}
@@ -846,11 +934,13 @@ func (v *queryWithInterfaceListFieldResponse) UnmarshalJSON(b []byte) error {
len(src))
for i, src := range src {
dst := &(*dst)[i]
err = __unmarshalqueryWithInterfaceListFieldBeingsBeing(
src, dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal queryWithInterfaceListFieldResponse.Beings: %w", err)
if len(src) != 0 && string(src) != "null" {
err = __unmarshalqueryWithInterfaceListFieldBeingsBeing(
src, dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal queryWithInterfaceListFieldResponse.Beings: %w", err)
}
}
}
}
@@ -970,12 +1060,14 @@ func (v *queryWithInterfaceListPointerFieldResponse) UnmarshalJSON(b []byte) err
len(src))
for i, src := range src {
dst := &(*dst)[i]
*dst = new(queryWithInterfaceListPointerFieldBeingsBeing)
err = __unmarshalqueryWithInterfaceListPointerFieldBeingsBeing(
src, *dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal queryWithInterfaceListPointerFieldResponse.Beings: %w", err)
if len(src) != 0 && string(src) != "null" {
*dst = new(queryWithInterfaceListPointerFieldBeingsBeing)
err = __unmarshalqueryWithInterfaceListPointerFieldBeingsBeing(
src, *dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal queryWithInterfaceListPointerFieldResponse.Beings: %w", err)
}
}
}
}
@@ -1097,11 +1189,13 @@ func (v *queryWithInterfaceNoFragmentsResponse) UnmarshalJSON(b []byte) error {
{
dst := &v.Being
src := firstPass.Being
err = __unmarshalqueryWithInterfaceNoFragmentsBeing(
src, dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal queryWithInterfaceNoFragmentsResponse.Being: %w", err)
if len(src) != 0 && string(src) != "null" {
err = __unmarshalqueryWithInterfaceNoFragmentsBeing(
src, dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal queryWithInterfaceNoFragmentsResponse.Being: %w", err)
}
}
}
return nil
@@ -1262,11 +1356,13 @@ func (v *queryWithNamedFragmentsResponse) UnmarshalJSON(b []byte) error {
len(src))
for i, src := range src {
dst := &(*dst)[i]
err = __unmarshalqueryWithNamedFragmentsBeingsBeing(
src, dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal queryWithNamedFragmentsResponse.Beings: %w", err)
if len(src) != 0 && string(src) != "null" {
err = __unmarshalqueryWithNamedFragmentsBeingsBeing(
src, dst)
if err != nil {
return fmt.Errorf(
"Unable to unmarshal queryWithNamedFragmentsResponse.Beings: %w", err)
}
}
}
}
@@ -1474,6 +1570,37 @@ query queryWithCustomMarshalSlice ($dates: [Date!]!) {
return &retval, err
}
func queryWithCustomMarshalOptional(
ctx context.Context,
client graphql.Client,
date *time.Time,
id *string,
) (*queryWithCustomMarshalOptionalResponse, error) {
__input := __queryWithCustomMarshalOptionalInput{
Date: date,
Id: id,
}
var err error
var retval queryWithCustomMarshalOptionalResponse
err = client.MakeRequest(
ctx,
"queryWithCustomMarshalOptional",
`
query queryWithCustomMarshalOptional ($date: Date, $id: ID) {
userSearch(birthdate: $date, id: $id) {
id
name
birthdate
}
}
`,
&retval,
&__input,
)
return &retval, err
}
func queryWithInterfaceNoFragments(
ctx context.Context,
client graphql.Client,