wI2L / jettison

Highly configurable, fast JSON encoder for Go
https://pkg.go.dev/github.com/wI2L/jettison
MIT License
174 stars 13 forks source link

Omitting a value that marshals into `null` isn't possible? #5

Closed Bios-Marcel closed 1 year ago

Bios-Marcel commented 2 years ago

Hey,

I've tried to replace encoding/json with jettison, since jettison has the omitnil tag.

There are certain fields that I am using a custom type for. The idea behind the type is to allow a HTTP request to set a field to null via a PATCH request, but not automatically null all omitted fields. For that to work, null fields must not be see as empty when unmarshalling. However, when marshalling, these null-values have no worth and should be omitted. Is something like that possible?

Let's say you had the following type:

type Thing {
    A *NullableString
    B *NullableString
}

If you now had a resource where both A and B already had a value of Hello and you'd send the following request:

{
    "A": null,

Then field A should be nulled, while B will stay unchanged, since A was explicitly defined, but B was not. The easy solution would be to make two versions of all structs. One for requests and one for replies, however I think that isn't desirable, as it bloats the code and doesn't allow sharing code to work on these structs.

Here's a small example. The second case will panic with json: error calling MarshalJSON for type *flows_service.NullableString: json: invalid value.

package flows_service

import (
    "encoding/json"
    "fmt"
    "testing"

    "github.com/wI2L/jettison"
)

type NullableString struct {
    // Set indicates whether unmarshalled JSON contained the field.
    Set bool
    // This field is only relevant if Set is `true`
    NonNull bool
    // Val; Only relevant if NonNull and Set are `true`.
    Val string
}

// MarshalJSON implements json.Marshaler.
func (v *NullableString) MarshalJSON() ([]byte, error) {
    if !v.Set || !v.NonNull {
        return nil, nil
    }

    return json.Marshal(v.Val)
}

// UnmarshalJSON implements json.Unmarshaler.
func (v *NullableString) UnmarshalJSON(data []byte) error {
    // If this method was called, the value was set.
    v.Set = true

    if string(data) == "null" {
        return nil
    }

    if err := json.Unmarshal(data, &v.Val); err != nil {
        return err
    }

    v.NonNull = true
    return nil
}

type Something struct {
    Field *NullableString `json:"field,omitempty,omitnil"`
}

func TestMarshalling(t *testing.T) {
    cases := []*Something{
        {},
        {
            Field: &NullableString{},
        },
        {
            Field: &NullableString{
                Set: true,
            },
        },
    }

    for i, c := range cases {
        t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
            data, err := jettison.Marshal(c)
            if err != nil {
                panic(err)
            }
            t.Log(string(data))
        })
    }
}
wI2L commented 2 years ago

Hello

To clarify, the error you're getting is returned by this function because the value returned by your json.Marshaler implementation on type NullableString is invalid. It returns nil if the following conditions evaluates to true: if !v.Set || !v.NonNull {.. however, that's invalid JSON. Note that, in that regards, jettison follows the encoding/json behavior.

Now, regarding the underlying question about the omitnil tag, jettison doesn't evaluate the JSON value returned by a MarshalJSON function call. As you can see here, the isNillable function is used to determine if a struct's field type can be nilled. However, since the MarshalJSON can be invoked only on non-nil values, we'd have to check if the JSON-formatted bytes returned by the implementation equals "null" (not nil).

Since the omitnil tag implemented by jettison is not documented/standardized by encoding/json, its behavior could be improved to cover edge cases like the one you reported.

Bios-Marcel commented 2 years ago

Hm, maybe it'd be wise to have this as optional behaviour via encoder options?

wI2L commented 2 years ago

It would make sense to omit values that equals null returned by MarshalJSON implementations, and this should be relatively cheap since it's a constant time comparison. Would you like to work on this and send a PR?

Bios-Marcel commented 2 years ago

Sure, I'll check it out and get back to you.

wI2L commented 1 year ago

@Bios-Marcel see https://github.com/wI2L/jettison/commit/45899e508eba28dad7c22aedbfd7cc51f70d227d

Bios-Marcel commented 1 year ago

Thanks .... I completely forgot about the fact I wanted to do this. Sorry :<