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

encoding/json: add omitzero option #45669

Closed
palsivertsen opened this issue Apr 21, 2021 · 35 comments · Fixed by tailscale/go#108
Closed

encoding/json: add omitzero option #45669

palsivertsen opened this issue Apr 21, 2021 · 35 comments · Fixed by tailscale/go#108

Comments

@palsivertsen
Copy link

The omitempty json tag is kind of confusing to use when working with nested structs. The following example illustrates the most basic case using an empty struct for argument's sake.

type Foo struct {
	EmptyStruct struct{} `json:",omitempty"`
}

The "EmptyStruct" field is a struct without any fields and can be empty, that is equal to it's zero value. But when I try to marshal it to json the field is still included in the resulting json object. Reading the encoding/json documentation about the definition of empty it does not mention empty structs:

The "omitempty" option specifies that the field should be omitted from the encoding if the field has an empty value, defined as false, 0, a nil pointer, a nil interface value, and any empty array, slice, map, or string.

It feels weird that adding the omitempty tag to struct fields is allowed if it will never have the desired effect. If structs will never be considered empty shouldn't there at least be a compiler warning when adding this tag to a struct field?

Working with json time.Time

This behavior causes some confusion when working with time.Time in json structures. Go's zero values for primitive types are fairly reasonable and "guessable" from a non-gopher point of view. But the zero value for time.Time, January 1, year 1, 00:00:00.000000000 UTC, is less common. A more(?) common zero value for time is January 01, 1970 00:00:00 UTC. To avoid confusion when working with json outside the world of go it would be nice to have a way to omit zero value dates.

A commonly suggested workaround to this problem is to use pointers, but pointers might be undesirable for a number of reasons. They are for example cumbersome to assign values to:

type Foo struct {
	T *time.Time
}
_ = Foo{
	T: &time.Now(), // Compile error
}
_ = Foo{
	T: &[]time.Time{time.Now()}[0], // Weird workaround
}

The time.Time documentation also recommends to pass the type as value, not pointer since some methods are not concurrency-safe:

Programs using times should typically store and pass them as values, not pointers. That is, time variables and struct fields should be of type time.Time, not *time.Time.

A Time value can be used by multiple goroutines simultaneously except that the methods GobDecode, UnmarshalBinary, UnmarshalJSON and UnmarshalText are not concurrency-safe.

This playground example illustrates three different uses of empty structs where I'd expect the fields to be excluded from the resulting json. Note that the time.Time type has no exposed fields and uses the time.Time.NarshalJSON() function to marshal itself into json.

Solution

Would it make sense to add a new tag, like omitzero, that also excludes structs?

There's a similar proposal in #11939, but that changes the definition of empty in the omitempty documentation.

@mvdan
Copy link
Member

mvdan commented Apr 21, 2021

Pretty similar to #22480. I think this needs to be put on hold along with #11939. We are aware that the API of encoding/json has issues, and we're working on it.

@seankhliao seankhliao changed the title Omit zero values when encoding json propsoal: encoding/json: add omitzero option Apr 22, 2021
@seankhliao seankhliao changed the title propsoal: encoding/json: add omitzero option proposal: encoding/json: add omitzero option Apr 22, 2021
@gopherbot gopherbot added this to the Proposal milestone Apr 22, 2021
@rsc
Copy link
Contributor

rsc commented May 12, 2021

Hold for #22480.

@rsc
Copy link
Contributor

rsc commented May 12, 2021

Placed on hold.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Jun 1, 2022

Taking off hold after closing #52803 as a duplicate of this one.

The big question is if we add ,omitzero do we also add a method to let "am I zero" be user-controllable?

Or perhaps we should allow MarshalJSON to return nil as in #50480.

@rsc
Copy link
Contributor

rsc commented Jun 1, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@smikulcik
Copy link

Personally I don't like baking the omission policy into struct tags. Things like omitempty, omitnil, omitzero are all limited by that one policy. I kinda liked my proposal for #52803 since it allowed that extra flexibility.

the IsEmpty() proposal also adds this flexibility, but it seems odd to further the abstraction of what it means for a property to be "empty". Arn't we just trying to determine if we want to omit a field or not?

@icholy
Copy link

icholy commented Jun 2, 2022

Or perhaps we should allow MarshalJSON to return nil as in #50480.

Say we go down that path and I write a type like this:

type Null[T any] struct {
	Valid bool
	Value T
}

func (n Null[T]) MarshalJSON() ([]byte, error) {
	if !n.Valid {
		return nil, nil
	}
	return json.Marshal(n.Valid)
}

Now this type can only be used as an omitzero field.

@rsc

@joeshaw
Copy link
Contributor

joeshaw commented Jun 2, 2022

@icholy I believe the return-nil option would be instead of omitzero, not in addition to it.

@icholy
Copy link

icholy commented Jun 2, 2022

@joeshaw having a nil return result in a null in non omitzero or omitempty fields would change the behavior of existing code.

@joeshaw
Copy link
Contributor

joeshaw commented Jun 2, 2022

@icholy Returning nil would not result in null in the JSON (that is done by returning []byte("null")), it would cause the field to be omitted entirely, under the proposal from #50480. Returning nil today causes invalid JSON to be produced.

However, @dsnet points out in #52803 (comment) that nil as a return value might cause existing code which calls MarshalJSON to break because it is not expecting this behavior.

@icholy
Copy link

icholy commented Jun 2, 2022

@joeshaw I want to have a general purpose Null[T] type that will be omitted in the case of omitempty or omitzero and will result in null otherwise.

@mitar
Copy link
Contributor

mitar commented Jun 5, 2022

@icholy So you return string null when you want JSON null, or nil when you want to omit it?

@icholy
Copy link

icholy commented Jun 5, 2022

@mitar my json.Marshaler implementation has no way of knowing if it's being used in an omitempty field.

@mitar
Copy link
Contributor

mitar commented Jun 5, 2022

True. I think this is a broader issue: MarshalJSON not being able to known tags on the field. So I think this is a different issue than this one: how could MarshalJSON know those tags. Maybe something like what Format has (e.g., fmt.State or verb rune). But currently MarshalJSON always overrides and ignores tags. There are simply two ways to "configure" JSON marshaling: or through imperative code (by implementing MarshalJSON) or declaratively through tags. Sadly, those two word do not work together and even more: you cannot implement in MarshalJSON same things you can through tags (see #50480).

@rsc rsc removed the Proposal-Hold label Jun 8, 2022
@inteon
Copy link

inteon commented Jun 14, 2022

  1. Lets start by agreeing on a definition for empty vs zero, here is my proposal:

    • empty JSON-values: false, 0, "", null, [], {}
    • zero JSON-values: empty JSON-values and objects containing only empty JSON-values eg. {a: null, b: 0}
  2. In case we can agree on the definition in 1, we notice that the current JSON encoder implementation is not conforming to this definition.
    Since structs that encode to {} are not skipped for fields with the omitempty tag.
    Now we have to decide if it is desirable to fix this: is this a bug or a feature 😉?
    The https://github.com/go-json-experiment/json project seems to acknowledge this error and fixes this issue.

  3. The omitempty tag is not compatible with a custom MarshalJSON function.
    If MarshalJSON returns null, this value is not omitted.
    In my opinion this is a bug, a field tagged with omitempty should never result in e.g. a null JSON-value.
    .
    Letting MarshalJSON return nil could be an option to generally omit a value, however this is not conditional on whether the omitempty tag is present or not.

  4. Finally, many developers want an option to indicate that a custom object is empty.

    • eg. value true instead of false should be considered empty for your usecase and should be omitted
    • eg. the empty value for a time.Time value should be considered as empty
  5. Other proposals like adding omitzero or an extra ShouldOmit() interface are not discussed here, since I think they are no bugs and less plausible to be implemented.


Bugs in 2 and 3 can be fixed as follows:
encode.go - line 750:

// fast path omitEmpty
if f.omitEmpty && isEmptyValue(fv) {
	continue
}
omitEmptyResetLocation := e.Len()

e.WriteByte(next)
if opts.escapeHTML {
	e.WriteString(f.nameEscHTML)
} else {
	e.WriteString(f.nameNonEsc)
}
opts.quoted = f.quoted

startLen := e.Len()
f.encoder(e, fv, opts)
newLen := e.Len()

if f.omitEmpty && (newLen-startLen) <= 5 {
	// using `Next` and `bytes.NewBuffer` we can modify the end of the
	// underlying slice efficiently (without doing any copying)
	fullBuf := e.Next(newLen) // extract underlying slice from buffer
	switch string(fullBuf[startLen:newLen]) {
	case "false", "0", "\"\"", "null", "[]":
		// reconstruct buffer without zero value
		e.Buffer = *bytes.NewBuffer(fullBuf[:omitEmptyResetLocation])
		continue
	default:
		// reconstruct original buffer
		e.Buffer = *bytes.NewBuffer(fullBuf)
	}
}
next = ','

Feature 4 can be added as follows:
encode.go - line 341:

// Emptyable is the interface implemented by types that
// can provide a function to determine if they are empty.
type Emptyable interface {
	IsEmpty() bool
}

var emptyableType = reflect.TypeOf((*Emptyable)(nil)).Elem()

func isEmptyValue(v reflect.Value) bool {
	// first check via the type if `Emptyable` is implemented
	// this way, we can prevent the more expensive `Interface`
	// conversion if not required
	if v.Type().Implements(emptyableType) {
		if v, ok := v.Interface().(Emptyable); ok {
			return v.IsEmpty()
		}
	}

	switch v.Kind() {
	case reflect.Array, reflect.Map, reflect.Slice, reflect.String:
		return v.Len() == 0
	case reflect.Bool:
		return !v.Bool()
	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
		return v.Int() == 0
	case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
		return v.Uint() == 0
	case reflect.Float32, reflect.Float64:
		return v.Float() == 0
	case reflect.Interface, reflect.Ptr:
		return v.IsNil()
	}
	return false
}

NOTE: Instead of adding a Emptyable interface, the fix for 2 and 3 could be combined with a MarshalJSON function that returns an empty value. Meaning that if we accept the proposed fix for 2 and 3, we also fix 4.

NOTE2 (edit): by replacing case "false", "0", "\"\"", "null", "[]", "{}": with case "false", "0", "\"\"", "null", "[]":; the solution for 2 and 3 becomes a solution for 3 only, this makes it maybe easier to get accepted? -> I'll probably create a PR for this change to gather some extra feedback.

@joeshaw I tried to make this analysis as complete as possible. Please let me know in case I missed something.

aead added a commit to aead/mtls that referenced this issue Sep 11, 2024
This commit adds the `IsZero() bool` method to the `Identity` type.
It returns true if the identity is equal to the Identity zero value.
A 'valid' identity won't be zero since `H(data)` will not produce
a hash value of all zero bits with overwhelming probability if a H
is a collision-resistant hash function.

The `IsZero` method is inline with the accepted Go JSON proposal
adding the struct tag `omitzero`.
Ref: golang/go#45669 (comment)

Signed-off-by: Andreas Auernhammer <[email protected]>
callthingsoff added a commit to callthingsoff/go that referenced this issue Sep 25, 2024
Fixes golang#45669

Change-Id: Idec483a03968cc671c8da27804589008b10864a1
callthingsoff added a commit to callthingsoff/go that referenced this issue Sep 25, 2024
Fixes golang#45669

Change-Id: Idec483a03968cc671c8da27804589008b10864a1
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/615676 mentions this issue: encoding/json: add omitzero option

callthingsoff added a commit to callthingsoff/go that referenced this issue Sep 26, 2024
Fixes golang#45669

Change-Id: Idec483a03968cc671c8da27804589008b10864a1
callthingsoff added a commit to callthingsoff/go that referenced this issue Sep 26, 2024
Fixes golang#45669

Change-Id: Idec483a03968cc671c8da27804589008b10864a1
callthingsoff added a commit to callthingsoff/go that referenced this issue Sep 27, 2024
Fixes golang#45669

Change-Id: Idec483a03968cc671c8da27804589008b10864a1
callthingsoff added a commit to callthingsoff/go that referenced this issue Sep 28, 2024
Fixes golang#45669

Change-Id: Idec483a03968cc671c8da27804589008b10864a1
callthingsoff added a commit to callthingsoff/go that referenced this issue Sep 29, 2024
Fixes golang#45669

Change-Id: Idec483a03968cc671c8da27804589008b10864a1
callthingsoff added a commit to callthingsoff/go that referenced this issue Oct 1, 2024
Fixes golang#45669

Change-Id: Idec483a03968cc671c8da27804589008b10864a1
@dmitshur dmitshur modified the milestones: Backlog, Go1.24 Oct 2, 2024
bradfitz pushed a commit to tailscale/go that referenced this issue Dec 16, 2024
Fixes golang#45669

Change-Id: Ic13523c0b3acdfc5b3e29a717bc62fde302ed8fd
GitHub-Last-Rev: 57030f2
GitHub-Pull-Request: golang#69622
Reviewed-on: https://go-review.googlesource.com/c/go/+/615676
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Reviewed-by: Joseph Tsai <[email protected]>
Reviewed-by: Michael Knyszek <[email protected]>
bradfitz pushed a commit to tailscale/go that referenced this issue Dec 16, 2024
Fixes golang#45669

Change-Id: Ic13523c0b3acdfc5b3e29a717bc62fde302ed8fd
GitHub-Last-Rev: 57030f2
GitHub-Pull-Request: golang#69622
Reviewed-on: https://go-review.googlesource.com/c/go/+/615676
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Reviewed-by: Joseph Tsai <[email protected]>
Reviewed-by: Michael Knyszek <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted