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: omitempty for arrays is confusing #29310

Open
skriptble opened this issue Dec 17, 2018 · 17 comments
Open

encoding/json: omitempty for arrays is confusing #29310

skriptble opened this issue Dec 17, 2018 · 17 comments
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@skriptble
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.11.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOOS="darwin"

What did you do?

https://play.golang.org/p/NlRBFTZHFYd

func main() {
	type Foo struct {
		ID [16]byte `json:",omitempty"`
	}

	var f Foo
	j, _ := json.Marshal(f)
	fmt.Println(string(j))
}

What did you expect to see?

{}

What did you see instead?

{"ID":[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]}

This is caused by isEmptyValue inside encoding/json checking to see if the length of the array is 0. For people actually using an array (and not a slice), it is unlikely that they would ever use an array of zero length. It is likely, however, that they might want to omit the result if all of the values in the array are empty. For example, if a user is using [16]byte to represent a UUID, they might want to omit encoding it if it is all zeroes. This was brought up in #11939, specifically this comment. It would be nice if we could do this generically for arrays with values we can determine are empty.

@crhntr
Copy link

crhntr commented Dec 18, 2018

An array that has a bunch of 'zero values' is not empty. The list of zeros means something. I would expect the array to be printed as is shown in the "what did you see instead" (for example a zero matrix). I played around with this a little and think this is a real issue.

My first attempt would be to implement the json.Marshaler interface for some type encapsulating [16]byte.

type UUID [16]int

func (uuid UUID) MarshalJSON() ([]byte, error) {
	for _, num := range uuid {
		if num != 0 {
			return json.Marshal([10]int(uuid))
		}
	}
	return json.Marshal(/* some zero value */) // <--- this is the real problem in my opinion.
}

Since it seams MarshalJSON is called after the value is established to not be zero, no matter what you put return from MarshalJSON, the value returned is not considered to be an empty value.

I think a json.ErrValueIsZero would be useful in this situation. So you can override the zero-ness of a type.

The only other solutions that I can think of would be to

  • implement MarshalJSON for all types that embed the [16]byte
  • or make all of those fields pointers to structs *[16]byte

I don't really love either of these options too much.

Notes (my playground): https://play.golang.org/p/GV8rX6f30ba

@agnivade
Copy link
Contributor

IMO, it doesn't seem confusing to me. The array indeed contains 16 zeros (which are totally valid values). So why should it be {} ? It makes sense if it is a slice or a map (which is #27589), but printing zeros sounds like the right behavior to me.

If you want this behavior to be there, this should be a proposal instead of a bug report.

@skriptble
Copy link
Author

This is a bug in the semantics of omitempty, which is why I didn't file it as a proposal. The documentation even points out specifically that there is such a thing as an empty array:

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.

There aren't any caveats added there. For all the other items in the list, any type of those things is considered empty, but with arrays only specific types of arrays can be considered empty and those are arrays that it is doubtful someone would use as a struct field.

What most users who add omitempty as a struct tag to an array field want is for the zero version of an array, e.g. var id [16]byte, to be evaluated as empty.

The confusing part is the definition of an empty array. It doesn't make much sense to provide a struct tag for something that is quite useless in practice and call it out in the documentation. A struct field with [0]byte is always empty and will only ever be empty. There could be a use case for using something like [0]byte as a struct field, but it's likely extremely rare.

@mccolljr
Copy link

It's really not a semantics bug, though. Since an array is not a pointer type,[16]byte is transported around as what is functionally a single value that is 16 bytes in length. "Empty" in the context of that text you cited means len(val) == 0, in which case [16]byte is not an empty array (because it has 16 elements, in which 0 could be considered a valid element value).

What most users who add omitempty as a struct tag to an array field want is for the zero version of an array, e.g. var id [16]byte, to be evaluated as empty.

You may prefer this, but many (for example me) do not. As far as Javascript/JSON is concerned, an array with 16 0-valued elements is not an empty array, so considering it as such would be incorrect, wouldn't it? Why not just use *[16]byte or marshal the value to null if you need to treat the zero value in a special way?

The confusing part is the definition of an empty array. It doesn't make much sense to provide a struct tag for something that is quite useless in practice and call it out in the documentation. A struct field with [0]byte is always empty and will only ever be empty. There could be a use case for using something like [0]byte as a struct field, but it's likely extremely rare.

I don't disagree that you'd probably never have [0]byte in as a struct field... but what about as an interface value? It could be a sentinel value that you ascribe special meaning to. It's not common, but it certainly isn't "useless".

I do agree with @crhntr that a special error to return from json.Marshaler implementations noting that a value should be omitted would be useful.

@bcmills bcmills added Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 19, 2018
@bcmills
Copy link
Contributor

bcmills commented Dec 19, 2018

CC @rsc @dsnet @bradfitz @mvdan for encoding/json

@bcmills bcmills added this to the Go1.13 milestone Dec 19, 2018
@mvdan
Copy link
Member

mvdan commented Dec 19, 2018

Reminds me of #28391 - in that issue, text/template's definition of whether a value is true was a bit too stiff. Seems a bit similar to how users have different expectations of what "empty" means here.

I don't personally have an opinion, but I think the current behavior of omitempty is consistent and well documented. You can argue that omitting an empty array is almost never useful, but making omitempty suddenly mean something different like "omit zero values" would likely be a breaking change.

For example, I presume that a slice with zero length and non-zero capacity is empty, but non-zero. That distinction could break dozens of programs in the wild if we suddenly change the meaning of omitempty.

@adrianbrad
Copy link

@skriptble I also run into this issue using UUID's. Did you find a workaround?
Interesting issue tho, definitely worth looking into.

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@divjotarora
Copy link

divjotarora commented Apr 9, 2020

@andybons I see the milestones on this issue have been updated twice, but it doesn't seem like there was a resolution on what to do going forward. I assume changing the default behavior of omitempty with respect to arrays isn't on the table, but I can think of two ways to address this:

  1. Add an interface like
type Zeroer interface {
    func IsZero() bool
}

that types can implement. The existing isEmptyValue function would check if the type implements this interface and omit it if IsZero returns true.

  1. One of the comments above suggests adding an error that can be returned from json.Marshaler implementations to indicate that the value should be omitted.

Any thoughts as to which solution would be better? I can pick up this task and file a PR once we've decided on a way forward.

@andybons
Copy link
Member

andybons commented Apr 9, 2020

@divjotarora sounds like you want a proposal as you’re looking to add to the API surface.

@mvdan
Copy link
Member

mvdan commented Apr 9, 2020

Also, please search before making either of these suggestions. I'm pretty sure the first one has been made before, for example.

@divjotarora
Copy link

@mvdan Are you referring to #11939? I read through it, and it seems like there are concerns about IsZero, but I don't see any sort of resolution on that thread either. Also, that thread is struct-specific but the use case in this issue is for arrays and potentially other values too. Do you think a new proposal is warranted here or should I wait until there's been more progress on #11939?

@mvdan
Copy link
Member

mvdan commented Apr 9, 2020

@divjotarora yes, that's the issue I mean. I think a new proposal would be so alike, that it would be considered a duplicate. If you have something to add to the original thread, I'd do it there.

@pmahoney
Copy link

I was recently tripped up by this. I think the issue (for me) was that all of "false, 0, a nil pointer, a nil interface value, and any empty array, slice, map, or string" are also the zero value for their type, and it's easy to then (wrongly) think "zero values are omitted".

The confusion comes from "empty array": by itself, "array" can mean an array of 0, 1, or more elements. It took me a second take to realize "empty array" can only ever refer to zero-length arrays.

I suggest changing the documentation to:

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 zero-length array, slice, map, or string.

@mvdan
Copy link
Member

mvdan commented Jan 14, 2021

@pmahoney that seems reasonable - want to send a patch?

@dsnet
Copy link
Member

dsnet commented Oct 6, 2023

Hi all, we kicked off a discussion for a possible "encoding/json/v2" package that addresses the spirit of this proposal.

See the "omitzero" struct tag option under the "Struct tag options" section, which omits a struct field if it is the zero Go value (or has a IsZero method that reports true). Under this semantic, [16]byte{} would be omitted because every element of the array is the zero Go value (i.e., byte(0)).

makew0rld added a commit to starlinglab/integrity-v2 that referenced this issue Jun 10, 2024
Arrays are never considered empty because they are full of zeros by
default. So all attributes were getting encrypted (with a key of zeros)
when set. Switching to a slice fixes this.

golang/go#29310
@callthingsoff
Copy link
Contributor

We can now use omitzero option in this case, see #45669.
Example using this new option works fine: https://go.dev/play/p/BVs9gA3u1bU?v=gotip

callthingsoff added a commit to callthingsoff/go that referenced this issue Oct 22, 2024
This CL is inspired by:
golang#29310 (comment)

When I read omitempty option in encoding/xml package, I find it's
a bit different than encoding/json package.

I think it's more precise to say:
"any array, slice, map, or string of length zero."

Update golang#29310

Change-Id: I64aefea34327c503a9ab33fceca3e02a62cb673a
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/621835 mentions this issue: encoding/json: clarify omitempty option for {array,slice,map,string}

gopherbot pushed a commit that referenced this issue Oct 23, 2024
This CL is inspired by:
#29310 (comment)

When I read omitempty option in encoding/xml package, I find it's
a bit different than encoding/json package.

I think it's more precise to say:
"any array, slice, map, or string of length zero."

Update #29310

Change-Id: Ia77167c3155411640224b349d4b34d0bb91ee11e
GitHub-Last-Rev: a4cf00d
GitHub-Pull-Request: #69984
Reviewed-on: https://go-review.googlesource.com/c/go/+/621835
Auto-Submit: Ian Lance Taylor <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Reviewed-by: Joseph Tsai <[email protected]>
Reviewed-by: Michael Pratt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests