code review 6868044: encoding/xml: namespace handling

encoding/xml: namespace handling
        This is a revised patch for 6856092, issue 3526. This fixes various
        aspects of XML namespace handling, especially attributes in namespaces
        other than their containing elements'.

[rsc, 2012-12-09 17:21:59]

Just found this linked in issue 3526. Please run hg mail to send to golang-dev.
Thanks.

[chris_cjones.org, 2012-12-10 01:29:58]

maxwell$ hg stat
M src/pkg/encoding/xml/atom_test.go
M src/pkg/encoding/xml/marshal.go
M src/pkg/encoding/xml/marshal_test.go
M src/pkg/encoding/xml/read.go
M src/pkg/encoding/xml/read_test.go
M src/pkg/encoding/xml/typeinfo.go
M src/pkg/encoding/xml/xml.go
M src/pkg/encoding/xml/xml_test.go
A src/pkg/encoding/xml/namespace.go
? patch.3526.2
maxwell$ hg mail
abort: no files changed

On 12/9/2012 10:21 AM, rsc@golang.org wrote:
> Just found this linked in issue 3526. Please run hg mail to send to
> golang-dev. Thanks.
>
>
> https://codereview.appspot.com/6868044/

[cjyar, 2012-12-10 02:06:03]

Hello golang-dev@googlegroups.com, rsc@golang.org, chris@cjones.org (cc:
golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[rsc, 2012-12-11 18:31:27]

I'd like to see what Gustavo thinks if he's around.
There's a lot of new API here.

[cjyar, 2012-12-14 06:36:09]

Hello rsc@golang.org, chris@cjones.org, n13m3y3r@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[cjyar, 2012-12-14 06:41:14]

Hello rsc@golang.org, chris@cjones.org, n13m3y3r@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[cjyar, 2012-12-16 22:26:26]

Hello rsc@golang.org, chris@cjones.org, n13m3y3r@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[cjyar, 2012-12-17 02:54:11]

Hello rsc@golang.org, chris@cjones.org, n13m3y3r@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-12-17 16:19:47]

I would like to drop Context from the API. Can we do that?
It seems to me that even if you're in the middle of a document you can
just introduce new prefixes as needed.

Russ

[rsc, 2012-12-17 18:54:36]

Perhaps, but perhaps not.

I think that if you need to send a <foo> in the jabber:client name
space, you can always send

<foo xmlns="jabber:client">

instead of needing to know what the outer version is called.

Russ

[rsc, 2012-12-22 14:57:10]

It's easy to handle the parsing half. Use the xml Decoder to read the
first element, and it will have the appropriate context.

For the printing half, I'd still like to avoid adding new API until we
have a demonstrated need. Any XMPP implementation must parse XML, and
what I've suggested is valid XML. It's true that it adds the 22 bytes
or so to each message, but I've seen what XMPP messages look like: no
one will notice 22 extra bytes. Also, if you're doing XMPP over TLS
it's all getting compressed anyway.

Russ

[cjyar, 2012-12-28 22:12:45]

Hello rsc@golang.org, chris@cjones.org, n13m3y3r@gmail.com (cc:
golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[rsc, 2013-01-18 22:24:56]

Can we please split this into two CLs, one for encoding and one for decoding?
Perhaps move the decoding to the new CL.

I don't believe the decoder should need any changes at all, except perhaps to
set the default xml and xmlns meanings.

https://codereview.appspot.com/6868044/diff/23001/src/pkg/encoding/xml/xml.go
File src/pkg/encoding/xml/xml.go (right):

https://codereview.appspot.com/6868044/diff/23001/src/pkg/encoding/xml/xml.go...
src/pkg/encoding/xml/xml.go:165: // context provides the namespace context, for
use in the
I'm a little confused. The current decoder already tracks name spaces. Does the
current code not work? I believe it does, in which case we can leave this file
alone.

[cjyar, 2013-01-21 04:46:08]

It would be awkward to split this into two CLs, but I'll do it if you really
want me to. Both Marshal and Unmarshal need to be fixed, and some of the fixes
are common to both (typeinfo.go).

Having said that, I need to clean this up a bit more and submit yet another
revision of it. The CL has evolved since I originally wrote it, and I haven't
been keeping it all neat.

https://codereview.appspot.com/6868044/diff/23001/src/pkg/encoding/xml/xml.go
File src/pkg/encoding/xml/xml.go (right):

https://codereview.appspot.com/6868044/diff/23001/src/pkg/encoding/xml/xml.go...
src/pkg/encoding/xml/xml.go:165: // context provides the namespace context, for
use in the
Issue 3526 isn't very clear on this: The lack of support for attribute
namespaces is common to both unmarshal and marshal.

[rsc, 2013-01-22 22:15:15]

I believe that Unmarshal is handling name spaces just fine and does not
need changes. If you disagree, please point out specific things that it is
doing wrong. This CL appears to be replacing correct code with new (perhaps
correct, but perhaps not) code. Instead of wholesale code replacement I
would prefer to fix the code that is there, unless it is truly
unsalvageable. I haven't seen any indication of that yet. I suggest to move
the Unmarshal changes - if any are needed! - to a separate CL.

Marshal, on the other hand, has never attempted to do something useful with
name spaces. I can believe that it needs help.

Russ

[cjyar, 2013-01-31 05:33:44]

Hello rsc@golang.org, n13m3y3r@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[mendel, 2013-11-27 15:41:02]

On 2013/01/31 05:33:44, cjyar wrote:
> Hello mailto:rsc@golang.org, mailto:n13m3y3r@gmail.com (cc:
mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Hi! I have a bug to report.

The scenario is: two distinct structs (therefore two distinct XML documents)
sharing a struct.
The first call to Unmarshal will be OK with all contents being retrieved from
the document.
The second call to Unmarshal won't retrieve the contents from the shared
structure.

If tinfoMap is cleaned right after the call to Unmarshal, everything works but
this doesn't seem to be OK as there's a performance penalty.

I'm not familiar with Google Code and Rietveld Code Review Tool, so I created a
gist with a test to demonstrate the issue:

https://gist.github.com/MendelGusmao/7677446

Thanks.

[dave_cheney.net, 2013-11-30 01:16:11]

@mendel	this is not the appropriate forum for a bug report.

Please log issues here, http://golang.org/issue

On 2013/11/27 15:41:02, mendel wrote:
> On 2013/01/31 05:33:44, cjyar wrote:
> > Hello mailto:rsc@golang.org, mailto:n13m3y3r@gmail.com (cc:
> mailto:golang-dev@googlegroups.com),
> > 
> > Please take another look.
> 
> Hi! I have a bug to report.
> 
> The scenario is: two distinct structs (therefore two distinct XML documents)
> sharing a struct.
> The first call to Unmarshal will be OK with all contents being retrieved from
> the document.
> The second call to Unmarshal won't retrieve the contents from the shared
> structure.
> 
> If tinfoMap is cleaned right after the call to Unmarshal, everything works but
> this doesn't seem to be OK as there's a performance penalty.
> 
> I'm not familiar with Google Code and Rietveld Code Review Tool, so I created
a
> gist with a test to demonstrate the issue:
> 
> https://gist.github.com/MendelGusmao/7677446
> 
> Thanks.

[gobot, 2013-12-20 16:21:42]

Replacing golang-dev with golang-codereviews.

[rsc, 2014-04-09 01:57:35]

R=close

The referenced issue is marked fixed.