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: namespaced and non-namespaced attributes conflict #11724

Open
ainar-g opened this Issue Jul 15, 2015 · 9 comments

Comments

Projects
None yet
7 participants
@ainar-g
Copy link
Contributor

ainar-g commented Jul 15, 2015

Go versions:

go version devel +2e4b659 Wed Jul 15 06:04:51 2015 +0000 linux/amd64

go version go1.4.2 linux/amd64

Code: http://play.golang.org/p/S2LU2vfk4i.

Expected: no error and main.Discount{XSIType:"ProgressivePromotion", Type:"Percent", From:"2015-07-28T00:00:00", To:"2015-07-30T00:00:00"}

Got: error: main.Discount field "XSIType" with tag "xsi type,attr" conflicts with field "Type" with tag "type,attr"

Found by Eugene Toropov at golang-ru. May be related to #3703.

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Jul 15, 2015

@rogpeppe

This comment has been minimized.

Copy link
Contributor

rogpeppe commented Jul 15, 2015

This is unfortunate. The existing Go convention is that if no namespace is specified
in the struct tag, it will match any namespace including the empty namespace.

Changing that will break existing code.

One possibility might be to make the tag xml:" name,attr" distinct from xml:"name,attr"
but that seems way too subtle.
Another possibility might be to use a special namespace to signify the empty namespace.
Various portions of the namespace are reserved (for example the xml-prefixed namespace),
so something like:

xml:"xmlempty name,attr"

might be possible, though ugly.

@ainar-g

This comment has been minimized.

Copy link
Contributor Author

ainar-g commented Jul 15, 2015

The existing Go convention is that if no namespace is specified
in the struct tag, it will match any namespace including the empty namespace.

Is this documented anywhere? I might've missed that, but the package docs don't seem to mention that. Might be a documentation issue.

Also, why can't that be changed to "if no namespace is specified in the struct tag, it will match any namespace including the empty namespace but excluding fields with the same name that explicitly specify their namespace" (or something better worded)? This doesn't seem like it would break anyone's code, because right now having xml:"foo,attr" and xml:"bar foo,attr" in the same struct results in an error.

@rogpeppe

This comment has been minimized.

Copy link
Contributor

rogpeppe commented Jul 15, 2015

Is this documented anywhere? I might've missed that, but the package docs don't seem to mention that. Might be a documentation issue.

Yes - this could definitely do with better docs - please feel free to raise an issue.

Also, why can't that be changed to "if no namespace is specified in the struct tag, it will match any
namespace including the empty namespace but excluding fields with the same name that
explicitly specify their namespace
" (or something better worded)? This doesn't seem like it would
break anyone's code, because right now having xml:"foo,attr" and xml:"bar foo,attr" in the same
struct results in an error.

I'm not sure that would work very well. If I'm trying to unmarshal from an attribute
in the empty namespace, I don't want any interference from other attributes that
I never heard of before. For example in the code linked above, if the code just
wants the type attribute and not any others such as xsi:type then there's
no way to do that currently.

This is a particular problem because attributes are in the empty namespace by
default, even if there's a default namespace set up (XML is weird).

@ainar-g

This comment has been minimized.

Copy link
Contributor Author

ainar-g commented Jul 16, 2015

Yes - this could definitely do with better docs - please feel free to raise an issue.

Raised #11735.

If the convention won't be changed (at least, not until Go 2.0), then we could use the form xml:"- name" to explicitly specify an empty namespace. This is more explicit than a single space (which might as well break existing code), looks consistent with xml:"-", and doesn't collide with anything since XML names can't start with "-".

Still, when there's a change to change the convention (e.g. 2.0), I'd suggest to make xml:"name" match only empty namespace, while something like xml:"* name" could be a wildcard to match any namespace.

@mmcdole

This comment has been minimized.

Copy link

mmcdole commented Oct 9, 2015

@rogpeppe just ran into this issue myself trying to unmarshal RSS documents which use the "itunes" namespace. My documents will sometimes have <itunes:author> and <author> elements.

Until Go's behavior is changed, what can we do to work around this?

@ainar-g

This comment has been minimized.

Copy link
Contributor Author

ainar-g commented Oct 9, 2015

@mmcdole Such questions are better off on StackOverflow, golang-nuts, or other forums. Also more people would be able to help you if you provide an MCVE.

The workaround I originally proposed at golang-ru is to replace all namespaced attributes in test with a modified one, i.e. replace <itunes:author> with <itunes:_author> and then unmarshal it into a separate field. I don't know whether it could help you.

@pdw-mb

This comment has been minimized.

Copy link

pdw-mb commented Feb 17, 2016

This looks like a duplicate of #8535 .

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Apr 12, 2018

Change https://golang.org/cl/106575 mentions this issue: encoding/xml : add check of namespaces to detect field names conflicts

@iWdGo

This comment has been minimized.

Copy link
Contributor

iWdGo commented Apr 12, 2018

In the first example, the namespace is not bound. This is mandatory (https://www.w3.org/TR/xml-names/#dt-NSName). If fixed, this issue is a duplicated of #8535 and the same fix submitted applies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment