From c0d6d332f68a38bc13825279d2048a16fffe87f9 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Thu, 23 Jul 2015 10:28:27 -0400 Subject: [PATCH] encoding/xml: restore Go 1.4 name space behavior There is clearly work to do here with respect to xml name spaces, but I don't believe the changes in this cycle are clearly correct. The changes in this cycle have visible impact on the generated xml, possibly breaking existing programs, and yet it's not clear that they are the end of the story: there is still significant confusion about how name spaces work or should work (see #9519, #9775, #8167, #7113). I would like to wait to make breaking changes until we completely understand what the behavior should be and can evaluate the benefit of those breaking changes. My main concern here is that we will break programs in Go 1.5 for the sake of name space adjustments and then while trying to fix those other bugs we'll break programs in Go 1.6 too. Let's wait until we know all the changes we want to make before we decide whether or how to break existing programs. This CL reverts: 5ae822b encoding/xml: minor changes bb7e665 encoding/xml: fix xmlns= behavior 9f9d66d encoding/xml: fix default namespace of tags b69ea01 encoding/xml: fix namespaces in a>b tags 3be158d encoding/xml: encoding name spaces correctly and adjusts tests from a9dddb5 encoding/xml: add more EncodeToken tests. to expect Go 1.4 behavior. I have confirmed that the name space parts of the test suite as of this CL passes against the Go 1.4 encoding/xml package, indicating that this CL successfully restores the Go 1.4 behavior. (Other tests do not, but that's because there were some real bug fixes in this cycle that are being kept. Specifically, the tests that don't pass in Go 1.4 are TestMarshal's NestedAndComment case, TestEncodeToken's encoding of newlines, and TestSimpleUseOfEncodeToken returning an error for invalid token types.) I also checked that the Go 1.4 tests pass when run against this copy of the sources. Fixes #11841. Change-Id: I97de06761038b40388ef6e3a55547ff43edee7cb Reviewed-on: https://go-review.googlesource.com/12570 Reviewed-by: Nigel Tao --- src/encoding/xml/marshal.go | 556 +++++++++---------------------- src/encoding/xml/marshal_test.go | 257 +++----------- src/encoding/xml/read_test.go | 32 +- src/encoding/xml/typeinfo.go | 8 - src/encoding/xml/xml.go | 43 +-- 5 files changed, 209 insertions(+), 687 deletions(-) diff --git a/src/encoding/xml/marshal.go b/src/encoding/xml/marshal.go index 3c3b6aca586f5e..b8dad494fb3d06 100644 --- a/src/encoding/xml/marshal.go +++ b/src/encoding/xml/marshal.go @@ -180,24 +180,17 @@ var ( ) // EncodeToken writes the given XML token to the stream. -// It returns an error if StartElement and EndElement tokens are not -// properly matched. +// It returns an error if StartElement and EndElement tokens are not properly matched. // -// EncodeToken does not call Flush, because usually it is part of a -// larger operation such as Encode or EncodeElement (or a custom -// Marshaler's MarshalXML invoked during those), and those will call -// Flush when finished. Callers that create an Encoder and then invoke -// EncodeToken directly, without using Encode or EncodeElement, need to -// call Flush when finished to ensure that the XML is written to the -// underlying writer. +// EncodeToken does not call Flush, because usually it is part of a larger operation +// such as Encode or EncodeElement (or a custom Marshaler's MarshalXML invoked +// during those), and those will call Flush when finished. +// Callers that create an Encoder and then invoke EncodeToken directly, without +// using Encode or EncodeElement, need to call Flush when finished to ensure +// that the XML is written to the underlying writer. // -// EncodeToken allows writing a ProcInst with Target set to "xml" only -// as the first token in the stream. -// -// When encoding a StartElement holding an XML namespace prefix -// declaration for a prefix that is not already declared, contained -// elements (including the StartElement itself) will use the declared -// prefix when encoding names with matching namespace URIs. +// EncodeToken allows writing a ProcInst with Target set to "xml" only as the first token +// in the stream. func (enc *Encoder) EncodeToken(t Token) error { p := &enc.p @@ -308,27 +301,19 @@ type printer struct { depth int indentedIn bool putNewline bool - defaultNS string attrNS map[string]string // map prefix -> name space attrPrefix map[string]string // map name space -> prefix - prefixes []printerPrefix + prefixes []string tags []Name } -// printerPrefix holds a namespace undo record. -// When an element is popped, the prefix record -// is set back to the recorded URL. The empty -// prefix records the URL for the default name space. -// -// The start of an element is recorded with an element -// that has mark=true. -type printerPrefix struct { - prefix string - url string - mark bool -} +// createAttrPrefix finds the name space prefix attribute to use for the given name space, +// defining a new prefix if necessary. It returns the prefix. +func (p *printer) createAttrPrefix(url string) string { + if prefix := p.attrPrefix[url]; prefix != "" { + return prefix + } -func (p *printer) prefixForNS(url string, isAttr bool) string { // The "http://www.w3.org/XML/1998/namespace" name space is predefined as "xml" // and must be referred to that way. // (The "http://www.w3.org/2000/xmlns/" name space is also predefined as "xmlns", @@ -336,97 +321,12 @@ func (p *printer) prefixForNS(url string, isAttr bool) string { if url == xmlURL { return "xml" } - if !isAttr && url == p.defaultNS { - // We can use the default name space. - return "" - } - return p.attrPrefix[url] -} - -// defineNS pushes any namespace definition found in the given attribute. -// If ignoreNonEmptyDefault is true, an xmlns="nonempty" -// attribute will be ignored. -func (p *printer) defineNS(attr Attr, ignoreNonEmptyDefault bool) error { - var prefix string - if attr.Name.Local == "xmlns" { - if attr.Name.Space != "" && attr.Name.Space != "xml" && attr.Name.Space != xmlURL { - return fmt.Errorf("xml: cannot redefine xmlns attribute prefix") - } - } else if attr.Name.Space == "xmlns" && attr.Name.Local != "" { - prefix = attr.Name.Local - if attr.Value == "" { - // Technically, an empty XML namespace is allowed for an attribute. - // From http://www.w3.org/TR/xml-names11/#scoping-defaulting: - // - // The attribute value in a namespace declaration for a prefix may be - // empty. This has the effect, within the scope of the declaration, of removing - // any association of the prefix with a namespace name. - // - // However our namespace prefixes here are used only as hints. There's - // no need to respect the removal of a namespace prefix, so we ignore it. - return nil - } - } else { - // Ignore: it's not a namespace definition - return nil - } - if prefix == "" { - if attr.Value == p.defaultNS { - // No need for redefinition. - return nil - } - if attr.Value != "" && ignoreNonEmptyDefault { - // We have an xmlns="..." value but - // it can't define a name space in this context, - // probably because the element has an empty - // name space. In this case, we just ignore - // the name space declaration. - return nil - } - } else if _, ok := p.attrPrefix[attr.Value]; ok { - // There's already a prefix for the given name space, - // so use that. This prevents us from - // having two prefixes for the same name space - // so attrNS and attrPrefix can remain bijective. - return nil - } - p.pushPrefix(prefix, attr.Value) - return nil -} -// createNSPrefix creates a name space prefix attribute -// to use for the given name space, defining a new prefix -// if necessary. -// If isAttr is true, the prefix is to be created for an attribute -// prefix, which means that the default name space cannot -// be used. -func (p *printer) createNSPrefix(url string, isAttr bool) { - if _, ok := p.attrPrefix[url]; ok { - // We already have a prefix for the given URL. - return - } - switch { - case !isAttr && url == p.defaultNS: - // We can use the default name space. - return - case url == "": - // The only way we can encode names in the empty - // name space is by using the default name space, - // so we must use that. - if p.defaultNS != "" { - // The default namespace is non-empty, so we - // need to set it to empty. - p.pushPrefix("", "") - } - return - case url == xmlURL: - return + // Need to define a new name space. + if p.attrPrefix == nil { + p.attrPrefix = make(map[string]string) + p.attrNS = make(map[string]string) } - // TODO If the URL is an existing prefix, we could - // use it as is. That would enable the - // marshaling of elements that had been unmarshaled - // and with a name space prefix that was not found. - // although technically it would be incorrect. // Pick a name. We try to use the final element of the path // but fall back to _. @@ -451,100 +351,41 @@ func (p *printer) createNSPrefix(url string, isAttr bool) { } } - p.pushPrefix(prefix, url) -} + p.attrPrefix[url] = prefix + p.attrNS[prefix] = url -// writeNamespaces writes xmlns attributes for all the -// namespace prefixes that have been defined in -// the current element. -func (p *printer) writeNamespaces() { - for i := len(p.prefixes) - 1; i >= 0; i-- { - prefix := p.prefixes[i] - if prefix.mark { - return - } - p.WriteString(" ") - if prefix.prefix == "" { - // Default name space. - p.WriteString(`xmlns="`) - } else { - p.WriteString("xmlns:") - p.WriteString(prefix.prefix) - p.WriteString(`="`) - } - EscapeText(p, []byte(p.nsForPrefix(prefix.prefix))) - p.WriteString(`"`) - } -} + p.WriteString(`xmlns:`) + p.WriteString(prefix) + p.WriteString(`="`) + EscapeText(p, []byte(url)) + p.WriteString(`" `) -// pushPrefix pushes a new prefix on the prefix stack -// without checking to see if it is already defined. -func (p *printer) pushPrefix(prefix, url string) { - p.prefixes = append(p.prefixes, printerPrefix{ - prefix: prefix, - url: p.nsForPrefix(prefix), - }) - p.setAttrPrefix(prefix, url) + p.prefixes = append(p.prefixes, prefix) + + return prefix } -// nsForPrefix returns the name space for the given -// prefix. Note that this is not valid for the -// empty attribute prefix, which always has an empty -// name space. -func (p *printer) nsForPrefix(prefix string) string { - if prefix == "" { - return p.defaultNS - } - return p.attrNS[prefix] +// deleteAttrPrefix removes an attribute name space prefix. +func (p *printer) deleteAttrPrefix(prefix string) { + delete(p.attrPrefix, p.attrNS[prefix]) + delete(p.attrNS, prefix) } -// markPrefix marks the start of an element on the prefix -// stack. func (p *printer) markPrefix() { - p.prefixes = append(p.prefixes, printerPrefix{ - mark: true, - }) + p.prefixes = append(p.prefixes, "") } -// popPrefix pops all defined prefixes for the current -// element. func (p *printer) popPrefix() { for len(p.prefixes) > 0 { prefix := p.prefixes[len(p.prefixes)-1] p.prefixes = p.prefixes[:len(p.prefixes)-1] - if prefix.mark { + if prefix == "" { break } - p.setAttrPrefix(prefix.prefix, prefix.url) + p.deleteAttrPrefix(prefix) } } -// setAttrPrefix sets an attribute name space prefix. -// If url is empty, the attribute is removed. -// If prefix is empty, the default name space is set. -func (p *printer) setAttrPrefix(prefix, url string) { - if prefix == "" { - p.defaultNS = url - return - } - if url == "" { - delete(p.attrPrefix, p.attrNS[prefix]) - delete(p.attrNS, prefix) - return - } - if p.attrPrefix == nil { - // Need to define a new name space. - p.attrPrefix = make(map[string]string) - p.attrNS = make(map[string]string) - } - // Remove any old prefix value. This is OK because we maintain a - // strict one-to-one mapping between prefix and URL (see - // defineNS) - delete(p.attrPrefix, p.attrNS[prefix]) - p.attrPrefix[url] = prefix - p.attrNS[prefix] = url -} - var ( marshalerType = reflect.TypeOf((*Marshaler)(nil)).Elem() marshalerAttrType = reflect.TypeOf((*MarshalerAttr)(nil)).Elem() @@ -580,23 +421,23 @@ func (p *printer) marshalValue(val reflect.Value, finfo *fieldInfo, startTemplat // Check for marshaler. if val.CanInterface() && typ.Implements(marshalerType) { - return p.marshalInterface(val.Interface().(Marshaler), p.defaultStart(typ, finfo, startTemplate)) + return p.marshalInterface(val.Interface().(Marshaler), defaultStart(typ, finfo, startTemplate)) } if val.CanAddr() { pv := val.Addr() if pv.CanInterface() && pv.Type().Implements(marshalerType) { - return p.marshalInterface(pv.Interface().(Marshaler), p.defaultStart(pv.Type(), finfo, startTemplate)) + return p.marshalInterface(pv.Interface().(Marshaler), defaultStart(pv.Type(), finfo, startTemplate)) } } // Check for text marshaler. if val.CanInterface() && typ.Implements(textMarshalerType) { - return p.marshalTextInterface(val.Interface().(encoding.TextMarshaler), p.defaultStart(typ, finfo, startTemplate)) + return p.marshalTextInterface(val.Interface().(encoding.TextMarshaler), defaultStart(typ, finfo, startTemplate)) } if val.CanAddr() { pv := val.Addr() if pv.CanInterface() && pv.Type().Implements(textMarshalerType) { - return p.marshalTextInterface(pv.Interface().(encoding.TextMarshaler), p.defaultStart(pv.Type(), finfo, startTemplate)) + return p.marshalTextInterface(pv.Interface().(encoding.TextMarshaler), defaultStart(pv.Type(), finfo, startTemplate)) } } @@ -623,13 +464,8 @@ func (p *printer) marshalValue(val reflect.Value, finfo *fieldInfo, startTemplat // 3. type name var start StartElement - // explicitNS records whether the element's name space has been - // explicitly set (for example an XMLName field). - explicitNS := false - if startTemplate != nil { start.Name = startTemplate.Name - explicitNS = true start.Attr = append(start.Attr, startTemplate.Attr...) } else if tinfo.xmlname != nil { xmlname := tinfo.xmlname @@ -638,14 +474,9 @@ func (p *printer) marshalValue(val reflect.Value, finfo *fieldInfo, startTemplat } else if v, ok := xmlname.value(val).Interface().(Name); ok && v.Local != "" { start.Name = v } - explicitNS = true } if start.Name.Local == "" && finfo != nil { - start.Name.Local = finfo.name - if finfo.xmlns != "" { - start.Name.Space = finfo.xmlns - explicitNS = true - } + start.Name.Space, start.Name.Local = finfo.xmlns, finfo.name } if start.Name.Local == "" { name := typ.Name() @@ -655,38 +486,87 @@ func (p *printer) marshalValue(val reflect.Value, finfo *fieldInfo, startTemplat start.Name.Local = name } - // defaultNS records the default name space as set by a xmlns="..." - // attribute. We don't set p.defaultNS because we want to let - // the attribute writing code (in p.defineNS) be solely responsible - // for maintaining that. - defaultNS := p.defaultNS - // Attributes for i := range tinfo.fields { finfo := &tinfo.fields[i] if finfo.flags&fAttr == 0 { continue } - attr, err := p.fieldAttr(finfo, val) - if err != nil { - return err + fv := finfo.value(val) + name := Name{Space: finfo.xmlns, Local: finfo.name} + + if finfo.flags&fOmitEmpty != 0 && isEmptyValue(fv) { + continue } - if attr.Name.Local == "" { + + if fv.Kind() == reflect.Interface && fv.IsNil() { continue } - start.Attr = append(start.Attr, attr) - if attr.Name.Space == "" && attr.Name.Local == "xmlns" { - defaultNS = attr.Value + + if fv.CanInterface() && fv.Type().Implements(marshalerAttrType) { + attr, err := fv.Interface().(MarshalerAttr).MarshalXMLAttr(name) + if err != nil { + return err + } + if attr.Name.Local != "" { + start.Attr = append(start.Attr, attr) + } + continue } + + if fv.CanAddr() { + pv := fv.Addr() + if pv.CanInterface() && pv.Type().Implements(marshalerAttrType) { + attr, err := pv.Interface().(MarshalerAttr).MarshalXMLAttr(name) + if err != nil { + return err + } + if attr.Name.Local != "" { + start.Attr = append(start.Attr, attr) + } + continue + } + } + + if fv.CanInterface() && fv.Type().Implements(textMarshalerType) { + text, err := fv.Interface().(encoding.TextMarshaler).MarshalText() + if err != nil { + return err + } + start.Attr = append(start.Attr, Attr{name, string(text)}) + continue + } + + if fv.CanAddr() { + pv := fv.Addr() + if pv.CanInterface() && pv.Type().Implements(textMarshalerType) { + text, err := pv.Interface().(encoding.TextMarshaler).MarshalText() + if err != nil { + return err + } + start.Attr = append(start.Attr, Attr{name, string(text)}) + continue + } + } + + // Dereference or skip nil pointer, interface values. + switch fv.Kind() { + case reflect.Ptr, reflect.Interface: + if fv.IsNil() { + continue + } + fv = fv.Elem() + } + + s, b, err := p.marshalSimple(fv.Type(), fv) + if err != nil { + return err + } + if b != nil { + s = string(b) + } + start.Attr = append(start.Attr, Attr{name, s}) } - if !explicitNS { - // Historic behavior: elements use the default name space - // they are contained in by default. - start.Name.Space = defaultNS - } - // Historic behaviour: an element that's in a namespace sets - // the default namespace for all elements contained within it. - start.setDefaultNamespace() if err := p.writeStart(&start); err != nil { return err @@ -715,68 +595,9 @@ func (p *printer) marshalValue(val reflect.Value, finfo *fieldInfo, startTemplat return p.cachedWriteError() } -// fieldAttr returns the attribute of the given field. -// If the returned attribute has an empty Name.Local, -// it should not be used. -// The given value holds the value containing the field. -func (p *printer) fieldAttr(finfo *fieldInfo, val reflect.Value) (Attr, error) { - fv := finfo.value(val) - name := Name{Space: finfo.xmlns, Local: finfo.name} - if finfo.flags&fOmitEmpty != 0 && isEmptyValue(fv) { - return Attr{}, nil - } - if fv.Kind() == reflect.Interface && fv.IsNil() { - return Attr{}, nil - } - if fv.CanInterface() && fv.Type().Implements(marshalerAttrType) { - attr, err := fv.Interface().(MarshalerAttr).MarshalXMLAttr(name) - return attr, err - } - if fv.CanAddr() { - pv := fv.Addr() - if pv.CanInterface() && pv.Type().Implements(marshalerAttrType) { - attr, err := pv.Interface().(MarshalerAttr).MarshalXMLAttr(name) - return attr, err - } - } - if fv.CanInterface() && fv.Type().Implements(textMarshalerType) { - text, err := fv.Interface().(encoding.TextMarshaler).MarshalText() - if err != nil { - return Attr{}, err - } - return Attr{name, string(text)}, nil - } - if fv.CanAddr() { - pv := fv.Addr() - if pv.CanInterface() && pv.Type().Implements(textMarshalerType) { - text, err := pv.Interface().(encoding.TextMarshaler).MarshalText() - if err != nil { - return Attr{}, err - } - return Attr{name, string(text)}, nil - } - } - // Dereference or skip nil pointer, interface values. - switch fv.Kind() { - case reflect.Ptr, reflect.Interface: - if fv.IsNil() { - return Attr{}, nil - } - fv = fv.Elem() - } - s, b, err := p.marshalSimple(fv.Type(), fv) - if err != nil { - return Attr{}, err - } - if b != nil { - s = string(b) - } - return Attr{name, s}, nil -} - // defaultStart returns the default start element to use, // given the reflect type, field info, and start template. -func (p *printer) defaultStart(typ reflect.Type, finfo *fieldInfo, startTemplate *StartElement) StartElement { +func defaultStart(typ reflect.Type, finfo *fieldInfo, startTemplate *StartElement) StartElement { var start StartElement // Precedence for the XML element name is as above, // except that we do not look inside structs for the first field. @@ -793,12 +614,6 @@ func (p *printer) defaultStart(typ reflect.Type, finfo *fieldInfo, startTemplate // since it has the Marshaler methods. start.Name.Local = typ.Elem().Name() } - // Historic behaviour: elements use the name space of - // the element they are contained in by default. - if start.Name.Space == "" { - start.Name.Space = p.defaultNS - } - start.setDefaultNamespace() return start } @@ -843,44 +658,29 @@ func (p *printer) writeStart(start *StartElement) error { p.tags = append(p.tags, start.Name) p.markPrefix() - // Define any name spaces explicitly declared in the attributes. - // We do this as a separate pass so that explicitly declared prefixes - // will take precedence over implicitly declared prefixes - // regardless of the order of the attributes. - ignoreNonEmptyDefault := start.Name.Space == "" - for _, attr := range start.Attr { - if err := p.defineNS(attr, ignoreNonEmptyDefault); err != nil { - return err - } - } - // Define any new name spaces implied by the attributes. - for _, attr := range start.Attr { - name := attr.Name - // From http://www.w3.org/TR/xml-names11/#defaulting - // "Default namespace declarations do not apply directly - // to attribute names; the interpretation of unprefixed - // attributes is determined by the element on which they - // appear." - // This means we don't need to create a new namespace - // when an attribute name space is empty. - if name.Space != "" && !name.isNamespace() { - p.createNSPrefix(name.Space, true) - } - } - p.createNSPrefix(start.Name.Space, false) p.writeIndent(1) p.WriteByte('<') - p.writeName(start.Name, false) - p.writeNamespaces() + p.WriteString(start.Name.Local) + + if start.Name.Space != "" { + p.WriteString(` xmlns="`) + p.EscapeString(start.Name.Space) + p.WriteByte('"') + } + + // Attributes for _, attr := range start.Attr { name := attr.Name - if name.Local == "" || name.isNamespace() { - // Namespaces have already been written by writeNamespaces above. + if name.Local == "" { continue } p.WriteByte(' ') - p.writeName(name, true) + if name.Space != "" { + p.WriteString(p.createAttrPrefix(name.Space)) + p.WriteByte(':') + } + p.WriteString(name.Local) p.WriteString(`="`) p.EscapeString(attr.Value) p.WriteByte('"') @@ -889,16 +689,6 @@ func (p *printer) writeStart(start *StartElement) error { return nil } -// writeName writes the given name. It assumes -// that p.createNSPrefix(name) has already been called. -func (p *printer) writeName(name Name, isAttr bool) { - if prefix := p.prefixForNS(name.Space, isAttr); prefix != "" { - p.WriteString(prefix) - p.WriteByte(':') - } - p.WriteString(name.Local) -} - func (p *printer) writeEnd(name Name) error { if name.Local == "" { return fmt.Errorf("xml: end tag with no name") @@ -917,7 +707,7 @@ func (p *printer) writeEnd(name Name) error { p.writeIndent(-1) p.WriteByte('<') p.WriteByte('/') - p.writeName(name, false) + p.WriteString(name.Local) p.WriteByte('>') p.popPrefix() return nil @@ -979,7 +769,7 @@ func (p *printer) marshalStruct(tinfo *typeInfo, val reflect.Value) error { switch finfo.flags & fMode { case fCharData: - if err := s.setParents(&noField, reflect.Value{}); err != nil { + if err := s.trim(finfo.parents); err != nil { return err } if vf.CanInterface() && vf.Type().Implements(textMarshalerType) { @@ -1025,7 +815,7 @@ func (p *printer) marshalStruct(tinfo *typeInfo, val reflect.Value) error { continue case fComment: - if err := s.setParents(&noField, reflect.Value{}); err != nil { + if err := s.trim(finfo.parents); err != nil { return err } k := vf.Kind() @@ -1079,22 +869,25 @@ func (p *printer) marshalStruct(tinfo *typeInfo, val reflect.Value) error { } case fElement, fElement | fAny: - if err := s.setParents(finfo, vf); err != nil { + if err := s.trim(finfo.parents); err != nil { return err } + if len(finfo.parents) > len(s.stack) { + if vf.Kind() != reflect.Ptr && vf.Kind() != reflect.Interface || !vf.IsNil() { + if err := s.push(finfo.parents[len(s.stack):]); err != nil { + return err + } + } + } } if err := p.marshalValue(vf, finfo, nil); err != nil { return err } } - if err := s.setParents(&noField, reflect.Value{}); err != nil { - return err - } + s.trim(nil) return p.cachedWriteError() } -var noField fieldInfo - // return the bufio Writer's cached write error func (p *printer) cachedWriteError() error { _, err := p.Write(nil) @@ -1133,64 +926,37 @@ func (p *printer) writeIndent(depthDelta int) { } type parentStack struct { - p *printer - xmlns string - parents []string + p *printer + stack []string } -// setParents sets the stack of current parents to those found in finfo. -// It only writes the start elements if vf holds a non-nil value. -// If finfo is &noField, it pops all elements. -func (s *parentStack) setParents(finfo *fieldInfo, vf reflect.Value) error { - xmlns := s.p.defaultNS - if finfo.xmlns != "" { - xmlns = finfo.xmlns - } - commonParents := 0 - if xmlns == s.xmlns { - for ; commonParents < len(finfo.parents) && commonParents < len(s.parents); commonParents++ { - if finfo.parents[commonParents] != s.parents[commonParents] { - break - } +// trim updates the XML context to match the longest common prefix of the stack +// and the given parents. A closing tag will be written for every parent +// popped. Passing a zero slice or nil will close all the elements. +func (s *parentStack) trim(parents []string) error { + split := 0 + for ; split < len(parents) && split < len(s.stack); split++ { + if parents[split] != s.stack[split] { + break } } - // Pop off any parents that aren't in common with the previous field. - for i := len(s.parents) - 1; i >= commonParents; i-- { - if err := s.p.writeEnd(Name{ - Space: s.xmlns, - Local: s.parents[i], - }); err != nil { + for i := len(s.stack) - 1; i >= split; i-- { + if err := s.p.writeEnd(Name{Local: s.stack[i]}); err != nil { return err } } - s.parents = finfo.parents - s.xmlns = xmlns - if commonParents >= len(s.parents) { - // No new elements to push. - return nil - } - if (vf.Kind() == reflect.Ptr || vf.Kind() == reflect.Interface) && vf.IsNil() { - // The element is nil, so no need for the start elements. - s.parents = s.parents[:commonParents] - return nil - } - // Push any new parents required. - for _, name := range s.parents[commonParents:] { - start := &StartElement{ - Name: Name{ - Space: s.xmlns, - Local: name, - }, - } - // Set the default name space for parent elements - // to match what we do with other elements. - if s.xmlns != s.p.defaultNS { - start.setDefaultNamespace() - } - if err := s.p.writeStart(start); err != nil { + s.stack = parents[:split] + return nil +} + +// push adds parent elements to the stack and writes open tags. +func (s *parentStack) push(parents []string) error { + for i := 0; i < len(parents); i++ { + if err := s.p.writeStart(&StartElement{Name: Name{Local: parents[i]}}); err != nil { return err } } + s.stack = append(s.stack, parents...) return nil } diff --git a/src/encoding/xml/marshal_test.go b/src/encoding/xml/marshal_test.go index 5dc78e748b306f..66675d7abc409e 100644 --- a/src/encoding/xml/marshal_test.go +++ b/src/encoding/xml/marshal_test.go @@ -174,11 +174,6 @@ type XMLNameWithTag struct { Value string `xml:",chardata"` } -type XMLNameWithNSTag struct { - XMLName Name `xml:"ns InXMLNameWithNSTag"` - Value string `xml:",chardata"` -} - type XMLNameWithoutTag struct { XMLName Name Value string `xml:",chardata"` @@ -307,7 +302,8 @@ func (m *MyMarshalerTest) MarshalXML(e *Encoder, start StartElement) error { return nil } -type MyMarshalerAttrTest struct{} +type MyMarshalerAttrTest struct { +} var _ MarshalerAttr = (*MyMarshalerAttrTest)(nil) @@ -315,22 +311,10 @@ func (m *MyMarshalerAttrTest) MarshalXMLAttr(name Name) (Attr, error) { return Attr{name, "hello world"}, nil } -type MyMarshalerValueAttrTest struct{} - -var _ MarshalerAttr = MyMarshalerValueAttrTest{} - -func (m MyMarshalerValueAttrTest) MarshalXMLAttr(name Name) (Attr, error) { - return Attr{name, "hello world"}, nil -} - type MarshalerStruct struct { Foo MyMarshalerAttrTest `xml:",attr"` } -type MarshalerValueStruct struct { - Foo MyMarshalerValueAttrTest `xml:",attr"` -} - type InnerStruct struct { XMLName Name `xml:"testns outer"` } @@ -366,34 +350,6 @@ type NestedAndComment struct { Comment string `xml:",comment"` } -type XMLNSFieldStruct struct { - Ns string `xml:"xmlns,attr"` - Body string -} - -type NamedXMLNSFieldStruct struct { - XMLName struct{} `xml:"testns test"` - Ns string `xml:"xmlns,attr"` - Body string -} - -type XMLNSFieldStructWithOmitEmpty struct { - Ns string `xml:"xmlns,attr,omitempty"` - Body string -} - -type NamedXMLNSFieldStructWithEmptyNamespace struct { - XMLName struct{} `xml:"test"` - Ns string `xml:"xmlns,attr"` - Body string -} - -type RecursiveXMLNSFieldStruct struct { - Ns string `xml:"xmlns,attr"` - Body *RecursiveXMLNSFieldStruct `xml:",omitempty"` - Text string `xml:",omitempty"` -} - func ifaceptr(x interface{}) interface{} { return &x } @@ -680,23 +636,17 @@ var marshalTests = []struct { C string `xml:"space x>c"` C1 string `xml:"space1 x>c"` D1 string `xml:"space1 x>d"` - E1 string `xml:"x>e"` }{ A: "a", B: "b", C: "c", C1: "c1", D1: "d1", - E1: "e1", }, ExpectXML: `` + - `abc` + - `` + - `c1` + - `d1` + - `` + - `` + - `e1` + + `abc` + + `c1` + + `d1` + `` + ``, }, @@ -720,11 +670,10 @@ var marshalTests = []struct { D1: "d1", }, ExpectXML: `` + - `ab` + - `c` + - `` + - `c1` + - `d1` + + `ab` + + `c` + + `c1` + + `d1` + `` + ``, }, @@ -738,8 +687,8 @@ var marshalTests = []struct { B1: "b1", }, ExpectXML: `` + - `b` + - `b1` + + `b` + + `b1` + ``, }, @@ -1033,10 +982,6 @@ var marshalTests = []struct { ExpectXML: ``, Value: &MarshalerStruct{}, }, - { - ExpectXML: ``, - Value: &MarshalerValueStruct{}, - }, { ExpectXML: ``, Value: &OuterStruct{IntAttr: 10}, @@ -1061,39 +1006,6 @@ var marshalTests = []struct { ExpectXML: ``, Value: &NestedAndComment{AB: make([]string, 2), Comment: "test"}, }, - { - ExpectXML: `hello world`, - Value: &XMLNSFieldStruct{Ns: "http://example.com/ns", Body: "hello world"}, - }, - { - ExpectXML: `hello world`, - Value: &NamedXMLNSFieldStruct{Ns: "http://example.com/ns", Body: "hello world"}, - }, - { - ExpectXML: `hello world`, - Value: &NamedXMLNSFieldStruct{Ns: "", Body: "hello world"}, - }, - { - ExpectXML: `hello world`, - Value: &XMLNSFieldStructWithOmitEmpty{Body: "hello world"}, - }, - { - // The xmlns attribute must be ignored because the - // element is in the empty namespace, so it's not possible - // to set the default namespace to something non-empty. - ExpectXML: `hello world`, - Value: &NamedXMLNSFieldStructWithEmptyNamespace{Ns: "foo", Body: "hello world"}, - MarshalOnly: true, - }, - { - ExpectXML: `hello world`, - Value: &RecursiveXMLNSFieldStruct{ - Ns: "foo", - Body: &RecursiveXMLNSFieldStruct{ - Text: "hello world", - }, - }, - }, } func TestMarshal(t *testing.T) { @@ -1207,6 +1119,15 @@ func TestUnmarshal(t *testing.T) { if _, ok := test.Value.(*Plain); ok { continue } + if test.ExpectXML == ``+ + `b`+ + `b1`+ + `` { + // TODO(rogpeppe): re-enable this test in + // https://go-review.googlesource.com/#/c/5910/ + continue + } + vt := reflect.TypeOf(test.Value) dest := reflect.New(vt.Elem()).Interface() err := Unmarshal([]byte(test.ExpectXML), dest) @@ -1316,100 +1237,6 @@ func TestMarshalFlush(t *testing.T) { } } -var encodeElementTests = []struct { - desc string - value interface{} - start StartElement - expectXML string -}{{ - desc: "simple string", - value: "hello", - start: StartElement{ - Name: Name{Local: "a"}, - }, - expectXML: `hello`, -}, { - desc: "string with added attributes", - value: "hello", - start: StartElement{ - Name: Name{Local: "a"}, - Attr: []Attr{{ - Name: Name{Local: "x"}, - Value: "y", - }, { - Name: Name{Local: "foo"}, - Value: "bar", - }}, - }, - expectXML: `hello`, -}, { - desc: "start element with default name space", - value: struct { - Foo XMLNameWithNSTag - }{ - Foo: XMLNameWithNSTag{ - Value: "hello", - }, - }, - start: StartElement{ - Name: Name{Space: "ns", Local: "a"}, - Attr: []Attr{{ - Name: Name{Local: "xmlns"}, - // "ns" is the name space defined in XMLNameWithNSTag - Value: "ns", - }}, - }, - expectXML: `hello`, -}, { - desc: "start element in name space with different default name space", - value: struct { - Foo XMLNameWithNSTag - }{ - Foo: XMLNameWithNSTag{ - Value: "hello", - }, - }, - start: StartElement{ - Name: Name{Space: "ns2", Local: "a"}, - Attr: []Attr{{ - Name: Name{Local: "xmlns"}, - // "ns" is the name space defined in XMLNameWithNSTag - Value: "ns", - }}, - }, - expectXML: `hello`, -}, { - desc: "XMLMarshaler with start element with default name space", - value: &MyMarshalerTest{}, - start: StartElement{ - Name: Name{Space: "ns2", Local: "a"}, - Attr: []Attr{{ - Name: Name{Local: "xmlns"}, - // "ns" is the name space defined in XMLNameWithNSTag - Value: "ns", - }}, - }, - expectXML: `hello world`, -}} - -func TestEncodeElement(t *testing.T) { - for idx, test := range encodeElementTests { - var buf bytes.Buffer - enc := NewEncoder(&buf) - err := enc.EncodeElement(test.value, test.start) - if err != nil { - t.Fatalf("enc.EncodeElement: %v", err) - } - err = enc.Flush() - if err != nil { - t.Fatalf("enc.Flush: %v", err) - } - if got, want := buf.String(), test.expectXML; got != want { - t.Errorf("#%d(%s): EncodeElement(%#v, %#v):\nhave %#q\nwant %#q", idx, test.desc, test.value, test.start, got, want) - } - } -} - func BenchmarkMarshal(b *testing.B) { b.ReportAllocs() for i := 0; i < b.N; i++ { @@ -1466,7 +1293,7 @@ var encodeTokenTests = []struct { toks: []Token{ StartElement{Name{"space", "local"}, nil}, }, - want: ``, + want: ``, }, { desc: "start element with no name", toks: []Token{ @@ -1560,7 +1387,7 @@ var encodeTokenTests = []struct { EndElement{Name{"another", "foo"}}, }, err: "xml: end tag in namespace another does not match start tag in namespace space", - want: ``, + want: ``, }, { desc: "start element with explicit namespace", toks: []Token{ @@ -1569,7 +1396,7 @@ var encodeTokenTests = []struct { {Name{"space", "foo"}, "value"}, }}, }, - want: ``, + want: ``, }, { desc: "start element with explicit namespace and colliding prefix", toks: []Token{ @@ -1579,7 +1406,7 @@ var encodeTokenTests = []struct { {Name{"x", "bar"}, "other"}, }}, }, - want: ``, + want: ``, }, { desc: "start element using previously defined namespace", toks: []Token{ @@ -1590,7 +1417,7 @@ var encodeTokenTests = []struct { {Name{"space", "x"}, "y"}, }}, }, - want: ``, + want: ``, }, { desc: "nested name space with same prefix", toks: []Token{ @@ -1611,7 +1438,7 @@ var encodeTokenTests = []struct { {Name{"space2", "b"}, "space2 value"}, }}, }, - want: ``, + want: ``, }, { desc: "start element defining several prefixes for the same name space", toks: []Token{ @@ -1621,7 +1448,7 @@ var encodeTokenTests = []struct { {Name{"space", "x"}, "value"}, }}, }, - want: ``, + want: ``, }, { desc: "nested element redefines name space", toks: []Token{ @@ -1633,7 +1460,7 @@ var encodeTokenTests = []struct { {Name{"space", "a"}, "value"}, }}, }, - want: ``, + want: ``, }, { desc: "nested element creates alias for default name space", toks: []Token{ @@ -1645,7 +1472,7 @@ var encodeTokenTests = []struct { {Name{"space", "a"}, "value"}, }}, }, - want: ``, + want: ``, }, { desc: "nested element defines default name space with existing prefix", toks: []Token{ @@ -1657,7 +1484,7 @@ var encodeTokenTests = []struct { {Name{"space", "a"}, "value"}, }}, }, - want: ``, + want: ``, }, { desc: "nested element uses empty attribute name space when default ns defined", toks: []Token{ @@ -1668,7 +1495,7 @@ var encodeTokenTests = []struct { {Name{"", "attr"}, "value"}, }}, }, - want: ``, + want: ``, }, { desc: "redefine xmlns", toks: []Token{ @@ -1676,7 +1503,7 @@ var encodeTokenTests = []struct { {Name{"foo", "xmlns"}, "space"}, }}, }, - err: `xml: cannot redefine xmlns attribute prefix`, + want: ``, }, { desc: "xmlns with explicit name space #1", toks: []Token{ @@ -1684,7 +1511,7 @@ var encodeTokenTests = []struct { {Name{"xml", "xmlns"}, "space"}, }}, }, - want: ``, + want: ``, }, { desc: "xmlns with explicit name space #2", toks: []Token{ @@ -1692,7 +1519,7 @@ var encodeTokenTests = []struct { {Name{xmlURL, "xmlns"}, "space"}, }}, }, - want: ``, + want: ``, }, { desc: "empty name space declaration is ignored", toks: []Token{ @@ -1700,7 +1527,7 @@ var encodeTokenTests = []struct { {Name{"xmlns", "foo"}, ""}, }}, }, - want: ``, + want: ``, }, { desc: "attribute with no name is ignored", toks: []Token{ @@ -1716,7 +1543,7 @@ var encodeTokenTests = []struct { {Name{"/34", "x"}, "value"}, }}, }, - want: `<_:foo xmlns:_="/34" _:x="value">`, + want: ``, }, { desc: "nested element resets default namespace to empty", toks: []Token{ @@ -1729,7 +1556,7 @@ var encodeTokenTests = []struct { {Name{"space", "x"}, "value"}, }}, }, - want: ``, + want: ``, }, { desc: "nested element requires empty default name space", toks: []Token{ @@ -1738,7 +1565,7 @@ var encodeTokenTests = []struct { }}, StartElement{Name{"", "foo"}, nil}, }, - want: ``, + want: ``, }, { desc: "attribute uses name space from xmlns", toks: []Token{ @@ -1747,7 +1574,7 @@ var encodeTokenTests = []struct { {Name{"some/space", "other"}, "other value"}, }}, }, - want: ``, + want: ``, }, { desc: "default name space should not be used by attributes", toks: []Token{ @@ -1760,7 +1587,7 @@ var encodeTokenTests = []struct { EndElement{Name{"space", "baz"}}, EndElement{Name{"space", "foo"}}, }, - want: ``, + want: ``, }, { desc: "default name space not used by attributes, not explicitly defined", toks: []Token{ @@ -1772,7 +1599,7 @@ var encodeTokenTests = []struct { EndElement{Name{"space", "baz"}}, EndElement{Name{"space", "foo"}}, }, - want: ``, + want: ``, }, { desc: "impossible xmlns declaration", toks: []Token{ @@ -1783,7 +1610,7 @@ var encodeTokenTests = []struct { {Name{"space", "attr"}, "value"}, }}, }, - want: ``, + want: ``, }} func TestEncodeToken(t *testing.T) { diff --git a/src/encoding/xml/read_test.go b/src/encoding/xml/read_test.go index 02f1e10c330add..7d004dc488cdfe 100644 --- a/src/encoding/xml/read_test.go +++ b/src/encoding/xml/read_test.go @@ -5,8 +5,6 @@ package xml import ( - "bytes" - "fmt" "io" "reflect" "strings" @@ -486,34 +484,6 @@ func TestUnmarshalNS(t *testing.T) { } } -func TestRoundTrip(t *testing.T) { - // From issue 7535 - const s = `` - in := bytes.NewBufferString(s) - for i := 0; i < 10; i++ { - out := &bytes.Buffer{} - d := NewDecoder(in) - e := NewEncoder(out) - - for { - t, err := d.Token() - if err == io.EOF { - break - } - if err != nil { - fmt.Println("failed:", err) - return - } - e.EncodeToken(t) - } - e.Flush() - in = out - } - if got := in.String(); got != s { - t.Errorf("have: %q\nwant: %q\n", got, s) - } -} - func TestMarshalNS(t *testing.T) { dst := Tables{"hello", "world"} data, err := Marshal(&dst) @@ -637,7 +607,7 @@ func TestMarshalNSAttr(t *testing.T) { if err != nil { t.Fatalf("Marshal: %v", err) } - want := `` + want := `` str := string(data) if str != want { t.Errorf("Marshal:\nhave: %#q\nwant: %#q\n", str, want) diff --git a/src/encoding/xml/typeinfo.go b/src/encoding/xml/typeinfo.go index c9a6421f2806b6..22248d20a6d669 100644 --- a/src/encoding/xml/typeinfo.go +++ b/src/encoding/xml/typeinfo.go @@ -194,14 +194,6 @@ func structFieldInfo(typ reflect.Type, f *reflect.StructField) (*fieldInfo, erro return finfo, nil } - if finfo.xmlns == "" && finfo.flags&fAttr == 0 { - // If it's an element no namespace specified, get the default - // from the XMLName of enclosing struct if possible. - if xmlname := lookupXMLName(typ); xmlname != nil { - finfo.xmlns = xmlname.xmlns - } - } - // Prepare field name and parents. parents := strings.Split(tag, ">") if parents[0] == "" { diff --git a/src/encoding/xml/xml.go b/src/encoding/xml/xml.go index ffab4a70c9dc06..0a21c930531034 100644 --- a/src/encoding/xml/xml.go +++ b/src/encoding/xml/xml.go @@ -35,24 +35,15 @@ func (e *SyntaxError) Error() string { return "XML syntax error on line " + strconv.Itoa(e.Line) + ": " + e.Msg } -// A Name represents an XML name (Local) annotated with a name space -// identifier (Space). In tokens returned by Decoder.Token, the Space -// identifier is given as a canonical URL, not the short prefix used in -// the document being parsed. -// -// As a special case, XML namespace declarations will use the literal -// string "xmlns" for the Space field instead of the fully resolved URL. -// See Encoder.EncodeToken for more information on namespace encoding -// behaviour. +// A Name represents an XML name (Local) annotated +// with a name space identifier (Space). +// In tokens returned by Decoder.Token, the Space identifier +// is given as a canonical URL, not the short prefix used +// in the document being parsed. type Name struct { Space, Local string } -// isNamespace reports whether the name is a namespace-defining name. -func (name Name) isNamespace() bool { - return name.Local == "xmlns" || name.Space == "xmlns" -} - // An Attr represents an attribute in an XML element (Name=Value). type Attr struct { Name Name @@ -81,30 +72,6 @@ func (e StartElement) End() EndElement { return EndElement{e.Name} } -// setDefaultNamespace sets the namespace of the element -// as the default for all elements contained within it. -func (e *StartElement) setDefaultNamespace() { - if e.Name.Space == "" { - // If there's no namespace on the element, don't - // set the default. Strictly speaking this might be wrong, as - // we can't tell if the element had no namespace set - // or was just using the default namespace. - return - } - // Don't add a default name space if there's already one set. - for _, attr := range e.Attr { - if attr.Name.Space == "" && attr.Name.Local == "xmlns" { - return - } - } - e.Attr = append(e.Attr, Attr{ - Name: Name{ - Local: "xmlns", - }, - Value: e.Name.Space, - }) -} - // An EndElement represents an XML end element. type EndElement struct { Name Name