encoding/json, encoding/xml: self-defined zero types
dsymonds opened this issue · 39 comments
Both encoding/json and encoding/xml have an "omitempty" struct tag option that
omits the corresponding field if its value is "empty". That's well-defined for
builtin types (e.g. int is 0, string is ""). It doesn't work well for named
types, whether they be time.Time or something a programmer has defined in their own
program.
I propose these two packages use
type isZeroer interface {
IsZero() bool
}
and use that where possible. time.Time already supports this.
I am prepared to do the work to implement this if there's consensus we should do it.struct full of zeros, so we could just make omitempty work on structs. It came up with JSON marshaling, and always getting a time.Time in the output even with omitempty. Zero structs might work, and address the immediate need, but IsZero feels like a more natural solution, just as the MarshalJSON method delegates some control to the type.
I have a CL that omits for a struct of zeros: https://golang.org/cl/11642043/ But it currently breaks an existing encoding/xml test, so I haven't mailed it yet.
I think this will be addressed by the discussions concerning http://golang.org/s/go12encoding and http://golang.org/s/go12xml. Those should let you handle zero types as you wish, with no need to refer to omitempty.
I wrote a fix before finding this issue; the fix is for encoding/json and is intended to be a proof-of-concept to see if the approach is acceptable, before implementing for other encoding types. I don't know how it ties into the go12encoding proposal. The fix (with tests) lets a struct, pointer or interface be omitted under omitempty if it defines IsZero(); package time is our motivation. https://golang.org/cl/13553050
The other approach which I considered, which might fit into go12encoding better, might be to define a sentinel error in encoding. encoding.IsEmpty. If the MarshalText() method returns an value with encoding.IsEmpty as an error, it could be used to indicate that the field can safely be dropped; if "omitempty" is *not* set, then the main return value is used even though encoding.IsEmpty was returned.
Re #16: I don't think http://golang.org/s/go12encoding is sufficient to satisfy this bug. Folk want a way to have "omitempty" do the right thing with a zero time.Time, namely have it not appear at all in the output, just like ints and strings. One would need either something like what #20 describes (there is precedent in path/filepath.SkipDir), or have the relevant packages look for an IsZero method when they support special handling of zero values. The new "encoding" package in Go 1.2 lays some groundwork, and gives control to types to determine how they are marshaled; I think we should seriously consider extending that to support zero values too.
Labels changed: added release-go1.3maybe, removed priority-later, release-none.
There are some suggestions above that testing if a struct is zeroed would solve the case of time.Time. In that regard, please note that time.Time.IsZero disregards the loc field, which means the outcome of omitempty would disagree with the result of IsZero, leading to potential confusion. Putting that aside, I've supported omitempty with the zero-struct check in other packages (bson, etc), and the outcome was positive.
It's easy to get the omitempty behavior desired by making the field a pointer or interface rather than a direct time.Time field. Obviously one would have to ensure the field is nil to omit it. This also works for the builtin types, enabling us to distinguish between values to be omitted and zero values to be emitted.
CL https://golang.org/cl/13914 mentions this issue.
CL https://golang.org/cl/13977 mentions this issue.
Surprised to see this still open. People, like me, gets surprised when omitempty does not omit empty time.Time (aka IsZero).
I wrote up an initial patch for it at https://golang.org/cl/13914. There you'll see comments from @rsc that show it's a surprisingly difficult issue.
See also the proposal in #11939; there was a suggestion in there (which I also made in the CL) that perhaps a sentinel error value for types that implement the Marshaler interfaces would be the best way to go. I didn't get an opportunity to work on that before the Go 1.7 freeze.
@dsymonds is this something you're still working on? At work this is something we'd very much like to have merged in since it's difficult to make omitempty work with NULL database values. We can look at it if needed.
(Can't scan nil into *string, so sql.NullString needs to be used, but that marshals into an object instead of a string, and the custom marshaler breaks the omitempty tag.)
I'm not working on this. I still think sniffing for an IsZero func would be the best option, but that didn't get approved.
I posted a CL @ https://go-review.googlesource.com/#/c/23088/, it's just a PoC, but if the main idea is accepted I'll work on cleaning it up and adding XML/gob support
CL https://golang.org/cl/23088 mentions this issue.