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, encoding/xml: self-defined zero types #4357

Closed
dsymonds opened this Issue Nov 7, 2012 · 39 comments

Comments

Projects
None yet
@dsymonds
Copy link
Member

dsymonds commented Nov 7, 2012

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.
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Nov 7, 2012

Comment 1:

I guess gob does not matter because it has no omitempty?
@rogpeppe

This comment has been minimized.

Copy link
Contributor

rogpeppe commented Nov 7, 2012

Comment 2:

ISTR that gob omits zero fields by default anyway, so this probably does matter there.
@dsymonds

This comment has been minimized.

Copy link
Member Author

dsymonds commented Nov 7, 2012

Comment 3:

Yeah, I guess this would apply to gob too.
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 9, 2012

Comment 4:

Gob should be off the table. No one cares exactly what bytes gob writes.
I forget how this came up. For time.Time most zero times really are a struct full of
zeros, so we could just make omitempty work on structs.

Labels changed: added priority-later, removed priority-soon.

@dsymonds

This comment has been minimized.

Copy link
Member Author

dsymonds commented Dec 9, 2012

Comment 5:

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.
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 9, 2012

Comment 6:

If time.Time is the only case, let's just make struct full of zeros
work for now. That's something we should do anyway, and it avoids
committing to new API.
@extemporalgenome

This comment has been minimized.

Copy link

extemporalgenome commented Dec 12, 2012

Comment 7:

2761 seems related (IsZero may be a solution for that).
@dsymonds

This comment has been minimized.

Copy link
Member Author

dsymonds commented Dec 15, 2012

Comment 8:

Issue #4554 has been merged into this issue.

@minux

This comment has been minimized.

Copy link
Member

minux commented Dec 15, 2012

Comment 9:

Issue #4554 has been merged into this issue.

@minux

This comment has been minimized.

Copy link
Member

minux commented Dec 15, 2012

Comment 10:

Issue #4554 has been merged into this issue.

@minux

This comment has been minimized.

Copy link
Member

minux commented Dec 16, 2012

Comment 11:

Issue #4554 has been merged into this issue.

@robpike

This comment has been minimized.

Copy link
Contributor

robpike commented Mar 7, 2013

Comment 12:

Labels changed: removed go1.1maybe.

@adg

This comment has been minimized.

Copy link
Contributor

adg commented Apr 8, 2013

Comment 13:

Issue #5218 has been merged into this issue.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Jul 17, 2013

Comment 14:

Issue #5902 has been merged into this issue.

@cookieo9

This comment has been minimized.

Copy link
Contributor

cookieo9 commented Jul 21, 2013

Comment 15:

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.
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Jul 21, 2013

Comment 16:

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.
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Jul 30, 2013

Comment 17:

Labels changed: added go1.3maybe.

@robpike

This comment has been minimized.

Copy link
Contributor

robpike commented Aug 20, 2013

Comment 18:

Labels changed: removed go1.3maybe.

@philpennock

This comment has been minimized.

Copy link

philpennock commented Sep 16, 2013

Comment 19:

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
@philpennock

This comment has been minimized.

Copy link

philpennock commented Sep 16, 2013

Comment 20:

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.
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Nov 27, 2013

Comment 21:

Labels changed: added go1.3maybe.

@lukescott

This comment has been minimized.

Copy link

lukescott commented Dec 3, 2013

Comment 22:

I would prefer the IsZero method as time already supports it. The IsZero method is also
found in reflect. It's a convention I've been using in other tag-based reflect packages.
@remyoudompheng

This comment has been minimized.

Copy link
Contributor

remyoudompheng commented Dec 3, 2013

Comment 23:

What do you expect on unmarshalling side ?
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 4, 2013

Comment 24:

Labels changed: added release-none, removed go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 4, 2013

Comment 25:

Labels changed: added repo-main.

@dsymonds

This comment has been minimized.

Copy link
Member Author

dsymonds commented Dec 4, 2013

Comment 26:

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.

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Apr 17, 2014

Comment 27:

Too late for API changes. Punting to Go 1.4.

Labels changed: added release-go1.4, removed release-go1.3maybe.

@niemeyer

This comment has been minimized.

Copy link
Contributor

niemeyer commented Sep 9, 2014

Comment 28:

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.
@nerdatmath

This comment has been minimized.

Copy link
Contributor

nerdatmath commented Sep 9, 2014

Comment 29:

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.
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Sep 15, 2014

Comment 30:

Labels changed: added release-none, removed release-go1.4.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Aug 25, 2015

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

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Aug 28, 2015

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

@dsymonds dsymonds removed their assignment Nov 23, 2015

@bep

This comment has been minimized.

Copy link

bep commented May 12, 2016

Surprised to see this still open. People, like me, gets surprised when omitempty does not omit empty time.Time (aka IsZero).

@joeshaw

This comment has been minimized.

Copy link
Contributor

joeshaw commented May 12, 2016

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.

@bradfitz bradfitz modified the milestones: Go1.8Maybe, Unplanned May 12, 2016

@ericlagergren

This comment has been minimized.

Copy link
Contributor

ericlagergren commented May 14, 2016

@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.)

@dsymonds

This comment has been minimized.

Copy link
Member Author

dsymonds commented May 14, 2016

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.

@OneOfOne

This comment has been minimized.

Copy link
Contributor

OneOfOne commented May 15, 2016

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

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented May 15, 2016

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

@adg

This comment has been minimized.

Copy link
Contributor

adg commented Jul 19, 2016

I'm closing this issue in favor of the newer proposal issue #11939, which covers the same topic.

@adg adg closed this Jul 19, 2016

@golang golang locked and limited conversation to collaborators Jul 19, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.