Skip to content

Commit

Permalink
encoding/xml: fix printing of namespace prefix in tag names
Browse files Browse the repository at this point in the history 
Prefix displays in XML when defined in tag names.
The URL of the name space is also returned for an End Token as
documentation requires to improve idempotency of Marshal/Unmarshal.

Translating the prefix is popping the NS which was unavailable
for translation. Translate for an End Token has been moved inside
the pop element part.

Fixes golang#9519

Change-Id: Id7a14d07c106a76a487b5c4e28d9d563fe061c60
  • Loading branch information
@ydnar
ydnar committed Sep 27, 2021
1 parent 8ac9e1a commit 7280904
Show file tree
Hide file tree
Showing 4 changed files with 197 additions and 104 deletions.
113 changes: 81 additions & 32 deletions src/encoding/xml/marshal.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,8 +305,8 @@ type printer struct {
*bufio.Writer
encoder *Encoder
seq int
indent string
prefix string
indent string // line identation
prefix string // line prefix
depth int
indentedIn bool
putNewline bool
Expand Down Expand Up @@ -366,7 +366,7 @@ func (p *printer) createAttrPrefix(url string) string {

p.attrPrefix[url] = prefix
p.attrNS[prefix] = url

/* prints a prefix definition for the URL which had no prefix */
p.WriteString(`xmlns:`)
p.WriteString(prefix)
p.WriteString(`="`)
Expand All @@ -384,7 +384,9 @@ func (p *printer) deleteAttrPrefix(prefix string) {
delete(p.attrNS, prefix)
}

func (p *printer) markPrefix() { // This is why prefix are never showing
// p.prefixes contains prefixes separated by the empty string between tags
// as several prefixes can be defined at any level
func (p *printer) markPrefix() {
p.prefixes = append(p.prefixes, "")
}

Expand All @@ -393,11 +395,34 @@ func (p *printer) popPrefix() {
prefix := p.prefixes[len(p.prefixes)-1]
p.prefixes = p.prefixes[:len(p.prefixes)-1]
if prefix == "" {
break
break // end of tag is reached
}
p.deleteAttrPrefix(prefix)
}
}
// No prefix is returned if the first prefix of the list for this tag is not assigned to a namespace
func (p *printer) tagPrefix() string {
prefix := p.prefixes[len(p.prefixes)-1] // last prefix relates to current tag
i := len(p.prefixes) - 1
for i >= 0 && i < len(p.prefixes){
if prefix == "" { // end of list of prefixes for this tag is reached
// check that previous prefix is the one of the tag
if i+1 < len(p.prefixes) { // list is not empty
if p.attrNS[p.prefixes[i+1]] != "" {
// prefix has been created, i.e. no prefix for the tag
return ""
} else {
return p.prefixes[i+1]
}
} else {
return ""
}
}
i--
prefix = p.prefixes[i]
}
return ""
}

var (
marshalerType = reflect.TypeOf((*Marshaler)(nil)).Elem()
Expand Down Expand Up @@ -713,52 +738,69 @@ func (p *printer) writeStart(start *StartElement) error {
return fmt.Errorf("xml: start tag with no name")
}

// pushes the value of the namespace and not the eventual prefix
// Pushes the value (url) of the namespace and not the eventual prefix
p.tags = append(p.tags, start.Name)
p.markPrefix() // pushes an empty prefix
p.markPrefix() // pushes an empty prefix to allow pop to locate the end of any prefix

p.writeIndent(1) // Handling relative depth of a tag
p.writeIndent(1) // handling relative depth of a tag
p.WriteByte('<')
p.WriteString(start.Name.Local) // if prefix exists, it is not printed
/* The attribute was not added if no XMLName field existed. */
var tagSpaceAttr Attr // the attribute will not be printed to avoid repetition
if start.Name.Space != "" { // tag starts with <.Space:.Local
// The tag prefix is not the default name space and it is a mistake to print it if not bound by an attribute.
// But this is used in some cases which are unrelated to XML standards. Invalid XML can then be produced.
dontPrintTagSpace := false
// Locate an eventual prefix. If none, the XML is <tag name and no short notation is used.
prefixToPrint := ""
for _, attr := range start.Attr {
// Name.Space only contains a namespace xmlns=".Space" or a .Value xmlns:(unavailable)=.Space
// tag .Space only contains a namespace URL and the prefix is unavailable
// (xmlns:(unavailable)=.Space)
// Attributes values which are namespaces are searched to avoid reprinting the domain
dontPrintTagSpace = (start.Name.Space == attr.Value && attr.Name.Space == xmlnsPrefix && attr.Name.Local != "") ||
(attr.Name.Space == "" && attr.Name.Local == xmlnsPrefix && attr.Value == start.Name.Space)
if dontPrintTagSpace {
if attr.Name.Space == xmlnsPrefix {
p.tags[len(p.tags)-1].Space = attr.Name.Local // Overriding with prefix
// The first attribute with a value == to start tag token will be the prefix used
// Print the space definition if the prefix is used
if start.Name.Space == attr.Value && attr.Name.Space == xmlnsPrefix && attr.Name.Local != "" {
// this is not enough if you return End tag with url in space and not the prefix
prefixToPrint = attr.Name.Local // if you skip printing, the value of the prefix does not display
p.prefixes = append(p.prefixes, prefixToPrint) // xmlns(.Space):(.Local)=(.Value)
break // the first prefix found is used
}
}
if prefixToPrint == "" { // no prefix was found for the space of the tag name
// the tag name Space is then considered as the default xmlns=".Space"
for _, attr := range start.Attr {
// attributes values which are namespaces are searched to avoid reprinting the default
if start.Name.Space == attr.Value && attr.Name.Space == "" && attr.Name.Local == xmlnsPrefix {
tagSpaceAttr = attr
}
break
break // the first attribute which is a default declaration that matches is kept
}
}
if !dontPrintTagSpace {
p.WriteString(` xmlns="`)
if prefixToPrint != "" {
// print the prefix and not the namespace value
p.WriteString(prefixToPrint)
p.WriteByte(':')
p.WriteString(start.Name.Local) // tag name
} else {
p.WriteString(start.Name.Local) // no prefix found
p.WriteString(` xmlns="`) // printing the namespace taken as default without prefix
p.EscapeString(start.Name.Space)
p.WriteByte('"')
}
} else {
p.WriteString(start.Name.Local) // tag name
}

// Attributes
for _, attr := range start.Attr {
name := attr.Name
if name.Local == "" {
continue
if attr.Name.Local == "" || attr == tagSpaceAttr {
continue // attribute already printed
}
p.WriteByte(' ')
if name.Space == xmlnsPrefix { // printing prefix name.Local xmlns:{.Local}={.Value}
if attr.Name.Space == xmlnsPrefix { // printing prefix name.Local xmlns:{.Local}={.Value}
p.WriteString(xmlnsPrefix)
p.WriteByte(':')
} else if name.Space != "" { // not a name space {.Space}:{.Local}={.Value}
p.WriteString(p.createAttrPrefix(name.Space)) // name.Space is not a prefix
p.WriteByte(':')
} else if attr.Name.Space != "" { // not a name space {.Space}:{.Local}={.Value} and .Local is not xmlns
p.WriteString(p.createAttrPrefix(attr.Name.Space)) // name.Space is not a prefix but nothing should be done
p.WriteByte(':') // about it if another one exists
}
// When space is empty, only writing .Local=.Value
// When space is empty, only writing .Local=.Value which will also be xmlns=".Value"
p.WriteString(attr.Name.Local)
p.WriteString(`="`)
p.EscapeString(attr.Value)
Expand All @@ -775,17 +817,24 @@ func (p *printer) writeEnd(name Name) error {
if len(p.tags) == 0 || p.tags[len(p.tags)-1].Local == "" {
return fmt.Errorf("xml: end tag </%s> without start tag", name.Local)
}
if top := p.tags[len(p.tags)-1]; top != name {
if top := p.tags[len(p.tags)-1]; top != name { // the end tag must check the prefix value when there is one
if top.Local != name.Local { // Tag names do not match
return fmt.Errorf("xml: end tag </%s> does not match start tag <%s>", name.Local, top.Local)
} // Namespaces do not match
return fmt.Errorf("xml: end tag </%s> in namespace %s does not match start tag <%s> in namespace %s", name.Local, name.Space, top.Local, top.Space)
if top.Space != name.Space { // tag prefixes do not match
return fmt.Errorf("xml: end space </%s> does not match start space <%s>", name.Space, top.Space)
}
}
p.tags = p.tags[:len(p.tags)-1]

p.writeIndent(-1)
p.WriteByte('<')
p.WriteByte('/')
endPrefix := p.tagPrefix()
if name.Space != "" && endPrefix != "" { // a prefix is available
p.WriteString(endPrefix) // print the prefix and not its value
p.WriteByte(':')
} // otherwise, xmlns=".Space" has no prefix and nothing should be printed
p.tags = p.tags[:len(p.tags)-1]
p.WriteString(name.Local)
p.WriteByte('>')
p.popPrefix()
Expand Down
32 changes: 16 additions & 16 deletions src/encoding/xml/marshal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2072,8 +2072,8 @@ var encodeTokenTests = []struct {
toks: []Token{
StartElement{Name{"space", "foo"}, nil},
EndElement{Name{"another", "foo"}},
},
err: "xml: end tag </foo> in namespace another does not match start tag <foo> in namespace space",
}, // #15
err: "xml: end space </another> does not match start space <space>",
want: `<foo xmlns="space">`,
}, {
desc: "start element with explicit namespace",
Expand All @@ -2082,8 +2082,8 @@ var encodeTokenTests = []struct {
{Name{"xmlns", "x"}, "space"},
{Name{"space", "foo"}, "value"},
}},
},// #16
want: `<local xmlns:x="space" xmlns:space="space" space:foo="value">`,
},// #16 <x:local xmlns:x="space" space:foo="value">
want: `<x:local xmlns:x="space" xmlns:space="space" space:foo="value">`,
}, {
desc: "start element with explicit namespace and colliding prefix",
toks: []Token{
Expand All @@ -2094,7 +2094,7 @@ var encodeTokenTests = []struct {
}},
},
// #17 Removed version was not well-formed as x is bound to "space" and to "x"
want: `<local xmlns:x="space" xmlns:space="space" space:foo="value" xmlns:x="x" x:bar="other">`,
want: `<x:local xmlns:x="space" xmlns:space="space" space:foo="value" xmlns:x="x" x:bar="other">`,
}, {
desc: "start element using previously defined namespace",
toks: []Token{
Expand All @@ -2105,7 +2105,7 @@ var encodeTokenTests = []struct {
{Name{"space", "x"}, "y"}, // space:x="y"
}},
},
// #18 The well-formed prefix is the only one appearing
// #18 The well-formed prefix is the only one appearing and the prefix is not in the tag as .Space is empty
want: `<local xmlns:x="space"><foo xmlns="space" xmlns:space="space" space:x="y">`,
}, {
desc: "nested name space with same prefix",
Expand Down Expand Up @@ -2137,7 +2137,7 @@ var encodeTokenTests = []struct {
{Name{"space", "x"}, "value"}, // space:x="value"
}},
},// #20
want: `<foo xmlns:a="space" xmlns:b="space" xmlns:space="space" space:x="value">`,
want: `<a:foo xmlns:a="space" xmlns:b="space" xmlns:space="space" space:x="value">`,
}, {
desc: "nested element redefines name space",
toks: []Token{
Expand All @@ -2149,7 +2149,7 @@ var encodeTokenTests = []struct {
{Name{"space", "a"}, "value"}, // space:a="value"
}},
}, // #21
want: `<foo xmlns:x="space"><foo xmlns:y="space" xmlns:space="space" space:a="value">`,
want: `<foo xmlns:x="space"><y:foo xmlns:y="space" xmlns:space="space" space:a="value">`,
}, {
desc: "nested element creates alias for default name space",
toks: []Token{
Expand All @@ -2161,7 +2161,7 @@ var encodeTokenTests = []struct {
{Name{"space", "a"}, "value"}, // space:a="value"
}},
}, // #22 Invalid duplication removed. Space oddities for non-XML attributes still produced
want: `<foo xmlns="space"><foo xmlns:y="space" xmlns:space="space" space:a="value">`,
want: `<foo xmlns="space"><y:foo xmlns:y="space" xmlns:space="space" space:a="value">`,
}, {
desc: "nested element defines default name space with existing prefix",
toks: []Token{
Expand Down Expand Up @@ -2266,7 +2266,7 @@ var encodeTokenTests = []struct {
want: `<foo xmlns="some/space" attr="value" xmlns:space="some/space" space:other="other value">`,
}, {
desc: "default name space should not be used by attributes",
toks: []Token{
toks: []Token{ // space is the value in space:foo and not the prefix
StartElement{Name{"space", "foo"}, []Attr{ // <space:foo
{Name{"", "xmlns"}, "space"}, // xmlns="space"
{Name{"xmlns", "bar"}, "space"}, // xmlns:bar="space"
Expand All @@ -2275,19 +2275,19 @@ var encodeTokenTests = []struct {
StartElement{Name{"space", "baz"}, nil}, // <space:baz
EndElement{Name{"space", "baz"}}, // space:baz>
EndElement{Name{"space", "foo"}}, // space:foo>
},
want: `<foo xmlns="space" xmlns:bar="space" xmlns:space="space" space:baz="foo"><baz xmlns="space"></baz></foo>`,
}, // #34 space has a prefix bar and is defined and should not appear again in baz tag
want: `<bar:foo xmlns="space" xmlns:bar="space" xmlns:space="space" space:baz="foo"><baz xmlns="space"></baz></bar:foo>`,
}, {
desc: "default name space not used by attributes, not explicitly defined",
toks: []Token{
StartElement{Name{"space", "foo"}, []Attr{
{Name{"", "xmlns"}, "space"},
{Name{"space", "baz"}, "foo"},
{Name{"", "xmlns"}, "space"}, // xmlns="space"
{Name{"space", "baz"}, "foo"}, // space:baz="foo"
}},
StartElement{Name{"space", "baz"}, nil},
EndElement{Name{"space", "baz"}},
EndElement{Name{"space", "foo"}},
}, // #35
}, // #35 space in the start element is a URL without prefix. It means that Space is default and no prefix is created
want: `<foo xmlns="space" xmlns:space="space" space:baz="foo"><baz xmlns="space"></baz></foo>`,
}, {
desc: "impossible xmlns declaration",
Expand All @@ -2298,7 +2298,7 @@ var encodeTokenTests = []struct {
StartElement{Name{"space", "bar"}, []Attr{
{Name{"space", "attr"}, "value"},
}},
},
}, // #36
want: `<foo xmlns="space"><bar xmlns="space" xmlns:space="space" space:attr="value">`,
}, {
desc: "reserved namespace prefix -- all lower case",
Expand Down
6 changes: 4 additions & 2 deletions src/encoding/xml/xml.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,6 @@ func (d *Decoder) Token() (Token, error) {
if !d.popElement(&t1) { // Popping the element with its eventual prefix for appropriate comparison
return nil, d.err
}
d.translate(&t1.Name, true)
t = t1
}
return t, err
Expand Down Expand Up @@ -512,10 +511,13 @@ func (d *Decoder) popElement(t *EndElement) bool {
return false
case s.name.Space != name.Space:
d.err = d.syntaxError("element <" + s.name.Local + "> in space " + s.name.Space +
"closed by </" + name.Local + "> in space " + name.Space)
" closed by </" + name.Local + "> in space " + name.Space)
return false
}

// translating before removal of ns
d.translate(&t.Name, true) // returning a namespace and not its prefix as doc says

// Pop stack until a Start or EOF is on the top, undoing the
// translations that were associated with the element we just closed.
for d.stk != nil && d.stk.kind != stkStart && d.stk.kind != stkEOF {
Expand Down
Loading

0 comments on commit 7280904

Please sign in to comment.