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/xml: inconsistent omitempty compared to encoding/json #5452

Closed
gopherbot opened this Issue May 13, 2013 · 20 comments

Comments

Projects
None yet
7 participants
@gopherbot
Copy link

gopherbot commented May 13, 2013

by krolaw:

Both packages define empty as being:
"The empty values are false, 0, any nil pointer or interface value, and any array,
slice, map, or string of length zero."

However, json does not look at the value behind pointers, while xml does.  This
difference is made obvious when wanting to omit nil pointers, but retain empty values.

Without omitempty:
xml will omit tags with nil pointers (not documented)
json will display null for nil pointers.

With omitempty:
xml will omit pointer fields pointing to empty values.
json will DISPLAY pointer fields pointing to empty values.

My current solution is: have omitempty on for json and off for xml.

http://play.golang.org/p/hANosIIkM_

This is probably a documentation issue.  If it's a bug, please retain ability to omit
nil pointers, but not look at values beyond the pointer. 

Thanks.
@rogpeppe

This comment has been minimized.

Copy link
Contributor

rogpeppe commented Jun 13, 2013

Comment 2:

A related issue: I believe this program should print {}, not {"X":null}.
It may be strictly speaking within the letter of the documentation
(the interface is not nil, but holds a nil value) but
I think it would be more in the spirit of the existing marshalling
(and consistent with encoding/xml) for the value inside the interface
to be considered when deciding about omitempty.
package main
import (
    "encoding/json"
    "fmt"
)
type Foo struct {
    X interface{} `json:",omitempty"`
}
func main() {
    data, err := json.Marshal(Foo{[]int(nil)})
    if err != nil {
        fmt.Println("err: %v", err)
    } else {
        fmt.Printf("%s\n", data)
    }
}
@gopherbot

This comment has been minimized.

Copy link
Author

gopherbot commented Jun 14, 2013

Comment 3 by krolaw:

I like that json does not look inside the value of the pointer.  Otherwise, I wouldn't
be able to set the difference between a zero (zero availability) and null value (no
update), which is important in the motel management solution I am writing.
Maybe omitempty is too general.  Maybe we need an omitnil as well.
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Jul 30, 2013

Comment 4:

Labels changed: added priority-later, go1.2maybe, removed priority-triage.

Status changed to Accepted.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Jul 30, 2013

Comment 5:

Labels changed: added feature.

@robpike

This comment has been minimized.

Copy link
Contributor

robpike commented Aug 30, 2013

Comment 6:

Not for 1.2.

Labels changed: removed go1.2maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Nov 27, 2013

Comment 7:

Labels changed: added go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Nov 27, 2013

Comment 8:

Labels changed: removed feature.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 4, 2013

Comment 9:

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

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 4, 2013

Comment 10:

Labels changed: added repo-main.

stripecodahale added a commit to aws/aws-sdk-go that referenced this issue Dec 20, 2014

Only use omitempty for XML lists.
Otherwise, because of golang/go#5452, we can't encode zero values at all.

So until that's fixed, you're stuck being unable to use default values in most requests.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015

@allan-simon

This comment has been minimized.

Copy link
Contributor

allan-simon commented Oct 9, 2015

What's the status on this ticket ? it's really hurting development , as it basically means for all google APIs (that use SOAP, and hence XML) we have to throw away the default xml marshaling and roll out our own :/

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Oct 9, 2015

@allan-simon, the status is that nobody is working on it.

@allan-simon

This comment has been minimized.

Copy link
Contributor

allan-simon commented Oct 9, 2015

@bradfitz ok ^^' , if I was to try my hand on it, would it have chances to get merge (admitting i follow the correct contributing procedure ofc) ? By that I mean, would there be backward compatibility concerns, even though it's not a documented "feature" ?

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Oct 9, 2015

I think it has a good chance of being accepted. If not it would've been closed a long time ago as "Unfortunate". The fact that it's still open implies that some sort of fix is possible. It's also not labeled as a "Documentation" fix. So, give it a shot. Complications often arise during development, but try and see.

@allan-simon

This comment has been minimized.

Copy link
Contributor

allan-simon commented Oct 9, 2015

@bradfitz , thanks for the clarification , if things get complicated I will expose them here

@allan-simon

This comment has been minimized.

Copy link
Contributor

allan-simon commented Oct 9, 2015

after investigation the problem seems to be with https://github.com/golang/go/blob/master/src/encoding/xml/marshal.go#L762-L768
which mean that when we got a struct with a field which is a pointer to an empty string, we already call "marshalValue" with the empty string, rather than the pointer

So when we arrive in the function, we early return https://github.com/golang/go/blob/master/src/encoding/xml/marshal.go#L405-L407 as we got an empty string
but deferencing should be the job of https://github.com/golang/go/blob/master/src/encoding/xml/marshal.go#L409-L417

@allan-simon

This comment has been minimized.

Copy link
Contributor

allan-simon commented Oct 9, 2015

I've added in marshal_test for OmitFieldTest struct a field "PStr" which is a *string, and in the test, initialized it to a pointer to an empty string, with current code it fails, by removing lines 762-768 it works

but now one test fail

--- FAIL: TestMarshal (0.00s)
    marshal_test.go:1030: #76: marshal(&xml.PointerFieldsTest{XMLName:xml.Name{Space:"", Local:""}, Name:(*string)(0x703100), Age:(*uint)(0x6f5158), Empty:(*string)(nil), Contents:(*string)(0x7030e0)}):
        have `<dummy name="Sarah" age="12"></dummy>`
        want `<dummy name="Sarah" age="12">lorem ipsum</dummy

I will dig more in some hours after some sleep ^^

@allan-simon

This comment has been minimized.

Copy link
Contributor

allan-simon commented Oct 10, 2015

got it, it was because the chardata / attributes part were relying on the fact that pointer were already dereferenced by the calling function, so it was simply ignoring pointers

all test are passing now, I will complete the test suite that was lacking some cases and i will issue a patch

@allan-simon

This comment has been minimized.

Copy link
Contributor

allan-simon commented Oct 10, 2015

@gopherbot

This comment has been minimized.

Copy link
Author

gopherbot commented Oct 10, 2015

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

@AlekSi

This comment has been minimized.

Copy link
Contributor

AlekSi commented May 11, 2016

However, json does not look at the value behind pointers, while xml does. This
difference is made obvious when wanting to omit nil pointers, but retain empty values.

Oh yes. The most important consequence of this for me is a problem with bool types. If I have type Bool bool, then MarshalXML defined on Bool is called for &Bool(true), but not for &Bool(false).

@gopherbot gopherbot closed this in daa1211 Oct 13, 2016

@golang golang locked and limited conversation to collaborators Oct 13, 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.