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

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

Closed
Bios-Marcel opened this issue Mar 5, 2022 · 6 comments
Closed

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

Bios-Marcel opened this issue Mar 5, 2022 · 6 comments
Assignees
Labels
enhancement New feature or request question Further information is requested

Comments

@Bios-Marcel
Copy link

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
Copy link
Owner

wI2L commented Mar 8, 2022

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
Copy link
Author

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

@wI2L
Copy link
Owner

wI2L commented Mar 8, 2022

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
Copy link
Author

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

@wI2L wI2L added enhancement New feature or request question Further information is requested labels Mar 17, 2022
@wI2L wI2L closed this as completed in 45899e5 Jan 6, 2023
@wI2L
Copy link
Owner

wI2L commented Jan 6, 2023

@Bios-Marcel see 45899e5

@Bios-Marcel
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants