jettison icon indicating copy to clipboard operation
jettison copied to clipboard

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

Open Bios-Marcel opened this issue 4 years ago • 4 comments

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))
		})
	}
}

Bios-Marcel avatar Mar 05 '22 12:03 Bios-Marcel

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.

wI2L avatar Mar 08 '22 13:03 wI2L

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

Bios-Marcel avatar Mar 08 '22 15:03 Bios-Marcel

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?

wI2L avatar Mar 08 '22 22:03 wI2L

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

Bios-Marcel avatar Mar 09 '22 15:03 Bios-Marcel

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

wI2L avatar Jan 06 '23 00:01 wI2L

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

Bios-Marcel avatar Jan 06 '23 10:01 Bios-Marcel