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

proposal: encoding/json, encoding/xml: support zero values of structs with omitempty #11939

Open
joeshaw opened this issue Jul 30, 2015 · 81 comments
Open

Comments

@joeshaw
Copy link
Contributor

@joeshaw joeshaw commented Jul 30, 2015

Support zero values of structs with omitempty in encoding/json and encoding/xml.

This bites people a lot, especially with time.Time. Open bugs include #4357 (which has many dups) and #10648. There may be others.

Proposal

Check for zero struct values by adding an additional case to the isEmptyValue function:

case reflect.Struct:
        return reflect.Zero(v.Type()).Interface() == v.Interface()

This will solve the vast majority of cases.

(Optional) Introduce a new encoding.IsZeroer interface, and use this to check for emptiness:

Update: I am dropping this part of the proposal, see below.

type IsZeroer interface {
        IsZero() bool
}

Visit this playground link and note that the unmarshaled time.Time value does not have a nil Location field. This prevents the reflection-based emptiness check from working. IsZero() already exists on time.Time, has the correct semantics, and has been adopted as a convention by Go code outside the standard library.

An additional check can be added to the isEmptyValue() functions before checking the value's Kind:

if z, ok := v.Interface().(encoding.IsZeroer); ok {
        return z.IsZero()
}

Compatibility

The encoding.IsZeroer interface could introduce issues with existing non-struct types that may have implemented IsZero() without consideration of omitempty. If this is undesirable, the encoding.IsZeroer interface check could be moved only within the struct case:

case reflect.Struct:
        val := v.Interface()
        if z, ok := val.(encoding.IsZeroer); ok {
                return z.IsZero()
        }
        return reflect.Zero(v.Type()).Interface() == val

Otherwise, this change is backward-compatible with existing valid uses of omitempty. Users who have applied omitempty to struct fields incorrectly will get their originally intended behavior for free.

Implementation

I (@joeshaw) have implemented and tested this change locally, and will send the CL when the Go 1.6 tree opens.

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Jul 30, 2015
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 25, 2015

CL https://golang.org/cl/13914 mentions this issue.

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 28, 2015

CL https://golang.org/cl/13977 mentions this issue.

@joeshaw
Copy link
Contributor Author

@joeshaw joeshaw commented Sep 18, 2015

The empty struct approach is implemented in CL 13914 and the IsZeroer interface is implemented in CL 13977.

In order for them to be reviewable separately they conflict a bit -- mostly in the documentation -- but I will fix for one if the other is merged.

@adg adg added Proposal and removed Proposal labels Sep 25, 2015
@joeshaw
Copy link
Contributor Author

@joeshaw joeshaw commented Oct 19, 2015

In the CLs @rsc said,

I'd really like to stop adding to these packages. I think we need to leave well enough alone at some point.

I see what he's getting at. CL 13977, which implements the IsZeroer interface is clearly an enhancement and adds API to the standard library that needs to be maintained forever. So, I am abandoning that CL and that part of the proposal.

However, I still feel strongly about omitempty with empty structs, and I want to push for CL 13914 to land for Go 1.6.

I use the JSON encoding in Go a lot, as my work is mostly writing services that communicate with other services, in multiple languages, over HTTP. The fact that structs don't obey omitempty is a frequent source of confusion (see #4357 and its many dups and references, and #10648) and working around it is really annoying. Other programming languages do not conform to Go's ideal "zero value" idea, and as a result encoding a zero value is semantically different in JSON than omitting it or encoding it as null. People run into this most commonly with time.Time. (There is also the issue that decoding a zero time.Time does not result in an empty struct, see #4357 (comment) for background on that.)

I think it should be considered a bug that Go does not support omitempty for these types, and although it adds a small amount of additional code, it fixes a bug.

@rsc rsc modified the milestones: Proposal, Unplanned Oct 24, 2015
@rsc rsc changed the title proposal: encoding: Support zero values of structs with omitempty in encoding/json and encoding/xml proposal: encoding/json, encoding/xml: support zero values of structs with omitempty Oct 24, 2015
@jeromenerf
Copy link

@jeromenerf jeromenerf commented Mar 27, 2016

This proposal is marked as unplanned, yet the related bug report #10648 is marked as go1.7.
Is it still being worked /thought on?

@rsc
Copy link
Contributor

@rsc rsc commented Mar 28, 2016

To my knowledge, it is not being worked on. Honestly this seems fairly low
priority and will likely miss Go 1.7.

On Sun, Mar 27, 2016 at 12:22 PM Jérôme Andrieux notifications@github.com
wrote:

This proposal is marked as unplanned, yet the related bug report #10648
#10648 is marked as go1.7.
Is it still being worked /thought on?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#11939 (comment)

@jeromenerf
Copy link

@jeromenerf jeromenerf commented Mar 28, 2016

OK.

This is more of a convenience than a priority indeed.

It can be a pain point when dealing with libs that don't support "embedded structs" as pointer though.

@Perelandric
Copy link

@Perelandric Perelandric commented Mar 29, 2016

I wonder if a low-impact alternative to the IsZeroer interface would be to allow one to return an error called json.CanOmit (or similar) from an implementation of the Marshaler interface. That way the dev is in control of determining what constitutes a zero value, and it doesn't impact other code.

It's not a perfect solution, since one can't add methods to types defined in another package, but this can be worked around to a degree.

Taking the time.Time example:

type MyTime struct {
  time.Time
}

// Implement the Marshaler interface
func (mt MyTime) MarshalJSON() ([]byte, error) {
  res, err := json.Marshal(mt.Time)

  if err == nil && mt.IsZero() {
    return res, json.CanOmit // Exclude zero value from fields with `omitempty`
  }
  return res, err
}

I haven't looked into implementation, but on the surface it would seem like a low-overhead solution, assuming the only work would be to check if an error returned equals json.CanOmit on fields where omitempty was included.

Using errors as a flag is not without precedent in the standard library, e.g. filepath#WalkFunc allows one to return filepath.SkipDir to skip recursion into a directory.

@joeshaw
Copy link
Contributor Author

@joeshaw joeshaw commented May 12, 2016

@Perelandric I mentioned a possible sentinel error value in https://golang.org/cl/13914 but I didn't get feedback on the idea or an opportunity to implement it before the Go 1.7 freeze. After Russ's comments on my original CL (and showing the unexpected difficulty in implementing this) I think that's the better way to go.

@gopherbot
Copy link

@gopherbot gopherbot commented May 15, 2016

CL https://golang.org/cl/23088 mentions this issue.

@adg
Copy link
Contributor

@adg adg commented Jul 19, 2016

While it's clear that we can do this, it's not clear that we want to. I'd like to see a formal proposal document that weighs the advantages and drawbacks of this feature addition. In particular, I am concerned about compatibility and maintenance burden.

@joeshaw
Copy link
Contributor Author

@joeshaw joeshaw commented Jul 19, 2016

thanks Andrew. I worked on this a little bit at GopherCon. I will look into putting together a formal proposal.

@albrow
Copy link
Contributor

@albrow albrow commented Aug 2, 2016

@joeshaw we ran into this issue at my place of work and I'm eagerly awaiting your proposal. Feel free to contact me if you would like any help. Email is on my profile.

@Perelandric
Copy link

@Perelandric Perelandric commented Sep 8, 2016

@joeshaw Is the proposal you're considering based on the sentinel object idea, or are you considering a different approach? Do you think you'll have time for this before the next release?

@joeshaw
Copy link
Contributor Author

@joeshaw joeshaw commented Sep 9, 2016

@Perelandric Yes, I think the sentinel object idea is the most straightforward way to go.

Other options include:

  • The IsZeroer interface (but I think this has potential backward compatibility issues)
  • "Backspacing" over objects that serialize to {} (but I think this requires too big a change to the JSON encoder code, and doesn't handle json.Marshaler implementers like time.Time)

I don't think I will be able to do this (proposal + implementation) before Go 1.8. If someone else wants to take it on for 1.8, I will gladly pass along my knowledge and partial implementation.

@Perelandric
Copy link

@Perelandric Perelandric commented Sep 10, 2016

Thanks @joeshaw. I created an implementation using the sentinel error for the encoding/json package and will start to work on the proposal in a bit. I think I'll focus primarily on this approach.

The Marshaler interface in encoding/xml is different from that in encoding/json, and seems as though a custom zero-value can already be established without needing to return anything special. Did you find that to be true?

After I make a little more progress, I'll post a link to a branch in case you, @albrow or anyone else wishes to review and contribute.

If you have any additional thoughts or info in the meantime, please let me know. Thank you!

@Perelandric
Copy link

@Perelandric Perelandric commented Sep 19, 2016

Change of heart on this. If there's resistance to adding to packages, then this won't fly. Maybe someone else wishes to advocate for this.

@pschultz
Copy link

@pschultz pschultz commented Dec 14, 2016

Mentioned in one of the duplicate issues was the idea to let MarshalJSON return (nil, nil) to skip the field. Borrowing your earlier example:

type MyTime struct { time.Time }

func (mt MyTime) MarshalJSON() ([]byte, error) {
    if mt.IsZero() {
        return nil, nil // Exclude zero value from fields with `omitempty`
    }

    return json.Marshal(mt.Time)
}

In Go 1.7, returning nil is not a valid implementation for MarshalJSON and leads to "unexpected end of JSON input" errors. This approach doesn't require any visible change to the encoding package (not even adding an error value).

For what it's worth, I just intuitively wrote a MarshalJSON method like that, expecting a field to be omitted from the JSON output.

@joeshaw
Copy link
Contributor Author

@joeshaw joeshaw commented Dec 14, 2016

@pschultz That approach seems reasonable to me, but it can't be used with omitempty.

The reason you get that error is because the JSON encoder checks for the validity of the JSON coming out of MarshalJSON and the result of returning a nil byte slice is (something like) "key":,. If returning a nil byte slice indicated that it should be omitted that'd be a different way to omit something from being encoded in the JSON than omitempty. (That might be fine, it seems ok to me.)

The benefit of the error value is that it could fit in with the existing omitempty because you'd return something like []byte(""), ErrOmitEmpty and it'd obey omitempty yet still return a valid value ("") if not set.

@Perelandric
Copy link

@Perelandric Perelandric commented Dec 14, 2016

@pschultz: A nil return wouldn't be able to be used as a flag for omitempty without causing undesired errors when omitempty is not present, since the implementation of MarshalJSON doesn't know when omitempty is actually there.

I don't know if having nil as an alternative to omitempty would be the best either. Seems like the user of a type should be the one deciding when it is omitted. IMO, the implementation of MarshalJSON should always return some useful representation, or an error when that's impossible.

oneumyvakin added a commit to oneumyvakin/jirardeau that referenced this issue Jan 17, 2017
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 7, 2020

Change https://golang.org/cl/241179 mentions this issue: encoding/json: Enable the json encoder to distinguish between 0, null, and empty values.

clarketm added a commit to clarketm/json that referenced this issue Jul 10, 2020
wking added a commit to wking/cincinnati-operator that referenced this issue Aug 4, 2020
Users must point the update service at a repository so the graph
builder can scrape for primary release metadata.

Also require replicas.  It's possible that we could make this optional
going forward, with the operator supplying a reasonable default.  But
there's no operator-side code for defaults today.

Also require graphDataImage.  It's possible that we could make this
optional going forward, with the operator disabling the graph-data
plugins in the policy engine when it is not set.  But there's no
operator-side code for that today, and
docs/graph-data-init-container.md documents how to build a graph-data
image pretty easily today.

Also require the spec, but leave status optional.  This allows users
to upload a manifest, while leaving the status empty for the
controller to fill in later.

The new metadata docstring is based on wording from [1].

I've dropped some 'omitempty' from struct properties, because Go's
encoding/json, etc., do not support omitempty for structs [2].  And
there's no need to set omitempty for required properties anyway.

I've pivoted some existing '// +optional' marker to
'+kubebuilder:validation:Optional', to make the consumer more obvious
(operator-sdk is in charge of invoking the kubebuilder logic).  Docs
for the kubebuilder markers are in [4].

Generated changes with:

  $ operator-sdk generate crds

using:

  $ operator-sdk version
  operator-sdk version: "v0.16.0", commit: "55f1446c5f472e7d8e308dcdf36d0d7fc44fc4fd", go version: "go1.13.8 linux/amd64"

[1]: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#pod-v1-core
[2]: golang/go#11939
[3]: https://book.kubebuilder.io/reference/markers/crd-validation.html
clarketm added a commit to clarketm/json that referenced this issue Aug 12, 2020
arithx added a commit to arithx/fcct that referenced this issue Oct 22, 2020
The  "github.com/clarketm/json"  package supports zero values of structs
with omitempty markers when marshaling them.
See [1] for context on this.

Closes: coreos#78

[1]  golang/go#11939
@zosmac
Copy link

@zosmac zosmac commented Jan 5, 2021

Intuitively, a struct "feels" like a complex type, while "time" feels like a simple value. I believe that it is not necessary to generalize the notion of an "empty" struct. However, an "empty time" is already defined, with the IsZero() method. Add to encoding/json/encode.go/isEmptyValue():

case reflect.Struct:
	switch v := val.Interface().(type) {
        case time.Time:
    		if v.IsZero() {
			return true
		}
	}
}
@Dynom
Copy link

@Dynom Dynom commented Jan 5, 2021

Intuitively, a struct "feels" like a complex type, while "time" feels like a simple value. I believe that it is not necessary to generalize the notion of an "empty" struct. However, an "empty time" is already defined, with the IsZero() method. Add to encoding/json/encode.go/isEmptyValue():

case reflect.Struct:
	switch v := val.Interface().(type) {
        case time.Time:
    		if v.IsZero() {
			return true
		}
	}
}

That could be improved to also support aliased time.Time types, so it'll need to include a step to see if it's: val.Type().ComparableTo(reflect.TypeOf((*time.Time)(nil)).Elem()).

Adding a Zeroer interface would make it a lot simpler, and you can easily do this in your own application, but I agree that being able to distinguish during unmarshalling should be part of stdlib.

Are there libraries that do solve this issue? I went through a couple, but most initiatives appear to be around Encoding, rather than the reverse.

@zosmac
Copy link

@zosmac zosmac commented Jan 6, 2021

With Mr. van der Velden's advice, I added the following case to the switch in encoding/json/encode.go:IsEmptyValue() to my Go deployment.

	case reflect.Struct:
		if v.Type().ConvertibleTo(reflect.TypeOf(time.Time{})) {
			return v.IsZero()
		}

I added the following type alttime, Optionals fields, and optionalsExpected lines to encoding/json/encode_test.go for the TestOmitEmpty test:

// issue golang.org/issues/11939
type alttime time.Time

func (t alttime) MarshalText() ([]byte, error) {
	return time.Time(t).MarshalText()
}
	// issue golang.org/issues/11939
	Tmr time.Time `json:"tmr"`
	Tmo time.Time `json:"tmo,omitempty"`
	Atr alttime   `json:"atr"`
	Ato alttime   `json:"ato,omitempty"`
 "tmr": "0001-01-01T00:00:00Z",
 "atr": "0001-01-01T00:00:00Z"

The test passes:

cd $GOROOT/src/encoding/json
go test -v
=== RUN   TestOmitEmpty
--- PASS: TestOmitEmpty (0.00s)
@zosmac
Copy link

@zosmac zosmac commented Jan 6, 2021

The omitempty tag option does not influence unmarshaling. Decoding is driven by the incoming JSON. It might be of value to add that. Here is an example, though the current behavior allows a structures fields to be updated with several Unmarshal calls, perhaps if one is initializing a structure from several JSON objects.

package main

import (
	"fmt"
	"encoding/json"
	"time"
)

func main() {
	type example struct {
		Word string    `json:",omitempty"`
		Time time.Time `json:",omitempty"`
	}

	x := example{Word: "abc", Time: time.Now()}
	buf, _ := json.MarshalIndent(x, "", "  ")
	fmt.Printf("%s\n", buf)

	buf = []byte(`{"Time": "2021-01-02T12:10:25.527-05:00"}`)
	json.Unmarshal(buf, &x)

	fmt.Printf("%+v\n", x)
} 

{
  "Word": "abc",
  "Time": "2009-11-10T23:00:00Z"
}
{**Word:abc** Time:2021-01-02 12:10:25.527 -0500 -0500}

Note that the x.Word value remains "abc". The omitempty option does not influence unmarshaling behavior.

Unmarshaling that would be sensitive to all the "json" tags in a structure requires adding a pass through the structure. I am not sure what I would even want unmarshaling to do in this case. If we want an existing structure value retained if not in the JSON, do we add a new tag option "ignoreabsent"?

For Marshaling however, I hope that the suggestion for time.Time handling of omitempty is reasonable. Documenting how to interpret "0001-01-01T00:00:00Z", and why, in REST API's does cause occasional confusion for my non-Go acquaintances. :)

@EarlWare
Copy link

@EarlWare EarlWare commented Apr 5, 2021

So does anyone know if there has been any official word/movement on this issue? I see theres been a number of proposals but they all seems to have just been abandoned over the years?

It definitely feels like if we can tag fields as "omitempty" it should work for both marshaling and unmarshaling equally (ie, if a struct has a nill/zero field and tagged "omitempty" it should not produce a JSON field, just as a JSON field that is null/omitted will not populate a value into a struct field.)

If there was an interface like IsNiller that was very simple, just one function like IsNil() bool. Then add a tiny bit of logic into marshal.go / encode.go:

func isEmptyValue(v reflect.Value) bool {
...
	case reflect.Struct:
 		if sv, ok := v.interface().(nillerPackage.IsNiller); ok {
			return sv.IsNill()
		}
...
}

Then any struct that wanted to be skipped just implements that interface. And would pretty much take care of the issue from what I can see? Plus it leaves the exact behavior up to the developer who is implementing the struct's behavior. (ie. if time.Time doesn't want this to be the default behavior, it doesn't need to implement it, but it is still easy enough to wrap and implement it in your own struct, win win)

I might be missing something here, but it feels like a relatively small change for something that would be so helpful for everyone that works with Go for web APIs/ JSON tasks in general.

@zwass
Copy link

@zwass zwass commented Apr 7, 2021

This feels like one of those issues which is causing real pain for the community, but sees no traction from @rsc, @adg, and others due to "not a problem at Google".

What would it take to move this proposal forward?

@Xe
Copy link

@Xe Xe commented Apr 7, 2021

If there was an interface like IsNiller that was very simple, just one function like IsNil() bool. Then add a tiny bit of logic into marshal.go / encode.go:

I'd suggest something like this instead:

type IsZeroer interface {
  IsZero() bool
}

This matches time#Time.IsZero and semantically aligns with the Go idea that zero values are the default.

I would be willing to champion this proposal (or whatever equivalent metaphor is most accurate), I am not a Googler or on the Go team but I am willing to do the legwork needed to make this happen.

@EarlWare
Copy link

@EarlWare EarlWare commented Apr 7, 2021

The original post(back mid 2015) suggested the IsZeroer interface, but noted there may be some compatibility issues:

"The encoding.IsZeroer interface could introduce issues with existing non-struct types that may have implemented IsZero() without consideration of omitempty."

Which is the only real reason I figured a change to IsNil would be appropriate, especially where in the cases Im thinking its useful for, will be used for omitting fields that are nil structs (or that wish to be considered nil, for example if they keep track of if they have been modified since creation or something?). Plus, if you need the new behavior for time.Time, you can very easily wrap it as a field in your own struct and implement whatever IsNil behavior you wish.

Essentially avoiding any backwards compatibility issues, and leaving the exact behavior in the hands of the developer who is defining the structs.

@lavalamp
Copy link

@lavalamp lavalamp commented Apr 7, 2021

An approach I don't see listed in this issue is:

  • Add to the Encoder type a func (e *Encoder) SetEmptyPredicates(predicates map[reflect.Type]func(reflect.Value)bool) (see note at end)
  • Pass this map in the options used during encoding (obviously)
  • During encoding, we only call the given function for fields marked as omitempty which have a type in the map.

The advantage of this approach is:

  • We avoid a potentially expensive interface check in the vast majority of cases
  • There's no need to define an interface for people to implement (which might collide)
  • Users can decide if something is "empty" however they please, including on objects declared outside of their package (e.g. time.Time)

(The disadvantage of this solution is that it's hideous.)

I'd hesitate to let users modify the global encoding options, but maybe adapters for things in the standard library (e.g. time.Time) could be pre-added.

Note: of course we don't have to make users pass a map, they could pass one reflect.Type, func (reflect.Value) bool at a time. I recommend the function takes reflect.Value because the user can convert it to a usable thing in the most efficient way -- they might not need to convert it.

@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Apr 8, 2021

#5901 (accepted) is custom per encoder/decoder matshal/unmarshal functions

@lavalamp
Copy link

@lavalamp lavalamp commented Apr 8, 2021

#5901 (accepted) is custom per encoder/decoder matshal/unmarshal functions

Thanks for that reference, it makes me think this approach might not get laughed all the way out of town-- however note that the omit-or-not decision is (at least in the current code) made by the caller of the marshal/unmarshal function, so it's not exactly the same problem.

@as
Copy link
Contributor

@as as commented Apr 16, 2021

I think this is a two part issue. The first part is a bug report and the second is a proposal.

Over the last ten years, it has been inconsistent and surprising that an empty struct value whose type has an omitempty tag is still not treated as empty when marshaled by default. Special cases for time.Time and opt-in interfaces are a red herring and distract from the bug present in this package.

If this bug is not going to be fixed due to perceived incompatibility (such as what happened to #31924), that should be clearly stated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.