Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Sep 28, 2021

Conversation

benjaminjkraft
Copy link
Collaborator

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
    . But (a) I forgot to document that, and (b) in practice
    json.Unmarshal does not call you in that case, 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.
  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.)

Issue: https://phabricator.khanacademy.org/D74453#inline-558571

Test plan:

make tesc

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

Reviewers: marksandstrom, steve, csilvers, mahtab, adam, jvoll, miguel
Copy link
Member

@csilvers csilvers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks right to me!

@@ -36,10 +36,6 @@ func MarshalDate(t *time.Time) ([]byte, error) {

func UnmarshalDate(b []byte, t *time.Time) error {
// (modified from time.Time.UnmarshalJSON)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this case is likely caught by the template code above now, but if someone calls this public method directly, won't they need these lines as in https://cs.opensource.google/go/go/+/refs/tags/go1.17.1:src/time/time.go;l=1283

Copy link
Collaborator Author

@benjaminjkraft benjaminjkraft Sep 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean on this style of function in general, or on this specific function? In general, folks can include such code if they intend to call the method directly. In this specific case, the package is internal so no one except us can call it!

Copy link
Member

@StevenACoffman StevenACoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a minor nit which probably does not matter. Otherwise, I quite like this.

@benjaminjkraft benjaminjkraft merged commit 65c3e20 into main Sep 28, 2021
@benjaminjkraft benjaminjkraft deleted the benkraft.custom-unmarshal-fix branch September 28, 2021 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants