Commit 72aa757d authored by Russ Cox's avatar Russ Cox Committed by Ian Lance Taylor

encoding/xml: fix incorrect indirect code in chardata, comment, innerxml fields

The new tests in this CL have been checked against Go 1.7 as well
and all pass in Go 1.7, with the one exception noted in a comment
(an intentional change to omitempty already present before this CL).

CL 15684 made the intentional change to omitempty.
This CL fixes bugs introduced along the way.

Most of these are corner cases that are arguably not that important,
but they've always worked all the way back to Go 1, and someone
cared enough to file #19063. The most significant problem found
while adding tests is that in the case of a nil *string field with
`xml:",chardata"`, the existing code silently stops processing not just
that field but the entire remainder of the struct.
Even if #19063 were not worth fixing, this chardata bug would be.

Fixes #19063.

Change-Id: I318cf8f9945e1a4615982d9904e109fde577ebf9
Reviewed-on: https://go-review.googlesource.com/36954
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: 's avatarDaniel Martí <mvdan@mvdan.cc>
Reviewed-by: 's avatarBrad Fitzpatrick <bradfitz@golang.org>
parent eebd8f51
......@@ -775,6 +775,20 @@ func (p *printer) marshalSimple(typ reflect.Type, val reflect.Value) (string, []
var ddBytes = []byte("--")
// indirect drills into interfaces and pointers, returning the pointed-at value.
// If it encounters a nil interface or pointer, indirect returns that nil value.
// This can turn into an infinite loop given a cyclic chain,
// but it matches the Go 1 behavior.
func indirect(vf reflect.Value) reflect.Value {
for vf.Kind() == reflect.Interface || vf.Kind() == reflect.Ptr {
if vf.IsNil() {
return vf
}
vf = vf.Elem()
}
return vf
}
func (p *printer) marshalStruct(tinfo *typeInfo, val reflect.Value) error {
s := parentStack{p: p}
for i := range tinfo.fields {
......@@ -816,17 +830,9 @@ func (p *printer) marshalStruct(tinfo *typeInfo, val reflect.Value) error {
continue
}
}
// Drill into interfaces and pointers.
// This can turn into an infinite loop given a cyclic chain,
// but it matches the Go 1 behavior.
for vf.Kind() == reflect.Interface || vf.Kind() == reflect.Ptr {
if vf.IsNil() {
return nil
}
vf = vf.Elem()
}
var scratch [64]byte
vf = indirect(vf)
switch vf.Kind() {
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
if err := emit(p, strconv.AppendInt(scratch[:0], vf.Int(), 10)); err != nil {
......@@ -861,6 +867,7 @@ func (p *printer) marshalStruct(tinfo *typeInfo, val reflect.Value) error {
if err := s.trim(finfo.parents); err != nil {
return err
}
vf = indirect(vf)
k := vf.Kind()
if !(k == reflect.String || k == reflect.Slice && vf.Type().Elem().Kind() == reflect.Uint8) {
return fmt.Errorf("xml: bad type for comment field of %s", val.Type())
......@@ -901,6 +908,7 @@ func (p *printer) marshalStruct(tinfo *typeInfo, val reflect.Value) error {
continue
case fInnerXml:
vf = indirect(vf)
iface := vf.Interface()
switch raw := iface.(type) {
case []byte:
......
......@@ -386,6 +386,140 @@ func ifaceptr(x interface{}) interface{} {
return &x
}
func stringptr(x string) *string {
return &x
}
type T1 struct{}
type T2 struct{}
type T3 struct{}
type IndirComment struct {
T1 T1
Comment *string `xml:",comment"`
T2 T2
}
type DirectComment struct {
T1 T1
Comment string `xml:",comment"`
T2 T2
}
type IfaceComment struct {
T1 T1
Comment interface{} `xml:",comment"`
T2 T2
}
type IndirChardata struct {
T1 T1
Chardata *string `xml:",chardata"`
T2 T2
}
type DirectChardata struct {
T1 T1
Chardata string `xml:",chardata"`
T2 T2
}
type IfaceChardata struct {
T1 T1
Chardata interface{} `xml:",chardata"`
T2 T2
}
type IndirCDATA struct {
T1 T1
CDATA *string `xml:",cdata"`
T2 T2
}
type DirectCDATA struct {
T1 T1
CDATA string `xml:",cdata"`
T2 T2
}
type IfaceCDATA struct {
T1 T1
CDATA interface{} `xml:",cdata"`
T2 T2
}
type IndirInnerXML struct {
T1 T1
InnerXML *string `xml:",innerxml"`
T2 T2
}
type DirectInnerXML struct {
T1 T1
InnerXML string `xml:",innerxml"`
T2 T2
}
type IfaceInnerXML struct {
T1 T1
InnerXML interface{} `xml:",innerxml"`
T2 T2
}
type IndirElement struct {
T1 T1
Element *string
T2 T2
}
type DirectElement struct {
T1 T1
Element string
T2 T2
}
type IfaceElement struct {
T1 T1
Element interface{}
T2 T2
}
type IndirOmitEmpty struct {
T1 T1
OmitEmpty *string `xml:",omitempty"`
T2 T2
}
type DirectOmitEmpty struct {
T1 T1
OmitEmpty string `xml:",omitempty"`
T2 T2
}
type IfaceOmitEmpty struct {
T1 T1
OmitEmpty interface{} `xml:",omitempty"`
T2 T2
}
type IndirAny struct {
T1 T1
Any *string `xml:",any"`
T2 T2
}
type DirectAny struct {
T1 T1
Any string `xml:",any"`
T2 T2
}
type IfaceAny struct {
T1 T1
Any interface{} `xml:",any"`
T2 T2
}
var (
nameAttr = "Sarah"
ageAttr = uint(12)
......@@ -401,7 +535,9 @@ var marshalTests = []struct {
Value interface{}
ExpectXML string
MarshalOnly bool
MarshalError string
UnmarshalOnly bool
UnmarshalError string
}{
// Test nil marshals to nothing
{Value: nil, ExpectXML: ``, MarshalOnly: true},
......@@ -1133,6 +1269,382 @@ var marshalTests = []struct {
ExpectXML: `<NestedAndCData><A><B></B><B></B></A><![CDATA[test]]></NestedAndCData>`,
Value: &NestedAndCData{AB: make([]string, 2), CDATA: "test"},
},
// Test pointer indirection in various kinds of fields.
// https://golang.org/issue/19063
{
ExpectXML: `<IndirComment><T1></T1><!--hi--><T2></T2></IndirComment>`,
Value: &IndirComment{Comment: stringptr("hi")},
MarshalOnly: true,
},
{
ExpectXML: `<IndirComment><T1></T1><T2></T2></IndirComment>`,
Value: &IndirComment{Comment: stringptr("")},
MarshalOnly: true,
},
{
ExpectXML: `<IndirComment><T1></T1><T2></T2></IndirComment>`,
Value: &IndirComment{Comment: nil},
MarshalError: "xml: bad type for comment field of xml.IndirComment",
},
{
ExpectXML: `<IndirComment><T1></T1><!--hi--><T2></T2></IndirComment>`,
Value: &IndirComment{Comment: nil},
UnmarshalOnly: true,
},
{
ExpectXML: `<IfaceComment><T1></T1><!--hi--><T2></T2></IfaceComment>`,
Value: &IfaceComment{Comment: "hi"},
MarshalOnly: true,
},
{
ExpectXML: `<IfaceComment><T1></T1><!--hi--><T2></T2></IfaceComment>`,
Value: &IfaceComment{Comment: nil},
UnmarshalOnly: true,
},
{
ExpectXML: `<IfaceComment><T1></T1><T2></T2></IfaceComment>`,
Value: &IfaceComment{Comment: nil},
MarshalError: "xml: bad type for comment field of xml.IfaceComment",
},
{
ExpectXML: `<IfaceComment><T1></T1><T2></T2></IfaceComment>`,
Value: &IfaceComment{Comment: nil},
UnmarshalOnly: true,
},
{
ExpectXML: `<DirectComment><T1></T1><!--hi--><T2></T2></DirectComment>`,
Value: &DirectComment{Comment: string("hi")},
},
{
ExpectXML: `<DirectComment><T1></T1><T2></T2></DirectComment>`,
Value: &DirectComment{Comment: string("")},
},
{
ExpectXML: `<IndirChardata><T1></T1>hi<T2></T2></IndirChardata>`,
Value: &IndirChardata{Chardata: stringptr("hi")},
},
{
ExpectXML: `<IndirChardata><T1></T1><![CDATA[hi]]><T2></T2></IndirChardata>`,
Value: &IndirChardata{Chardata: stringptr("hi")},
UnmarshalOnly: true, // marshals without CDATA
},
{
ExpectXML: `<IndirChardata><T1></T1><T2></T2></IndirChardata>`,
Value: &IndirChardata{Chardata: stringptr("")},
},
{
ExpectXML: `<IndirChardata><T1></T1><T2></T2></IndirChardata>`,
Value: &IndirChardata{Chardata: nil},
MarshalOnly: true, // unmarshal leaves Chardata=stringptr("")
},
{
ExpectXML: `<IfaceChardata><T1></T1>hi<T2></T2></IfaceChardata>`,
Value: &IfaceChardata{Chardata: string("hi")},
UnmarshalError: "cannot unmarshal into interface {}",
},
{
ExpectXML: `<IfaceChardata><T1></T1><![CDATA[hi]]><T2></T2></IfaceChardata>`,
Value: &IfaceChardata{Chardata: string("hi")},
UnmarshalOnly: true, // marshals without CDATA
UnmarshalError: "cannot unmarshal into interface {}",
},
{
ExpectXML: `<IfaceChardata><T1></T1><T2></T2></IfaceChardata>`,
Value: &IfaceChardata{Chardata: string("")},
UnmarshalError: "cannot unmarshal into interface {}",
},
{
ExpectXML: `<IfaceChardata><T1></T1><T2></T2></IfaceChardata>`,
Value: &IfaceChardata{Chardata: nil},
UnmarshalError: "cannot unmarshal into interface {}",
},
{
ExpectXML: `<DirectChardata><T1></T1>hi<T2></T2></DirectChardata>`,
Value: &DirectChardata{Chardata: string("hi")},
},
{
ExpectXML: `<DirectChardata><T1></T1><![CDATA[hi]]><T2></T2></DirectChardata>`,
Value: &DirectChardata{Chardata: string("hi")},
UnmarshalOnly: true, // marshals without CDATA
},
{
ExpectXML: `<DirectChardata><T1></T1><T2></T2></DirectChardata>`,
Value: &DirectChardata{Chardata: string("")},
},
{
ExpectXML: `<IndirCDATA><T1></T1><![CDATA[hi]]><T2></T2></IndirCDATA>`,
Value: &IndirCDATA{CDATA: stringptr("hi")},
},
{
ExpectXML: `<IndirCDATA><T1></T1>hi<T2></T2></IndirCDATA>`,
Value: &IndirCDATA{CDATA: stringptr("hi")},
UnmarshalOnly: true, // marshals with CDATA
},
{
ExpectXML: `<IndirCDATA><T1></T1><T2></T2></IndirCDATA>`,
Value: &IndirCDATA{CDATA: stringptr("")},
},
{
ExpectXML: `<IndirCDATA><T1></T1><T2></T2></IndirCDATA>`,
Value: &IndirCDATA{CDATA: nil},
MarshalOnly: true, // unmarshal leaves CDATA=stringptr("")
},
{
ExpectXML: `<IfaceCDATA><T1></T1><![CDATA[hi]]><T2></T2></IfaceCDATA>`,
Value: &IfaceCDATA{CDATA: string("hi")},
UnmarshalError: "cannot unmarshal into interface {}",
},
{
ExpectXML: `<IfaceCDATA><T1></T1>hi<T2></T2></IfaceCDATA>`,
Value: &IfaceCDATA{CDATA: string("hi")},
UnmarshalOnly: true, // marshals with CDATA
UnmarshalError: "cannot unmarshal into interface {}",
},
{
ExpectXML: `<IfaceCDATA><T1></T1><T2></T2></IfaceCDATA>`,
Value: &IfaceCDATA{CDATA: string("")},
UnmarshalError: "cannot unmarshal into interface {}",
},
{
ExpectXML: `<IfaceCDATA><T1></T1><T2></T2></IfaceCDATA>`,
Value: &IfaceCDATA{CDATA: nil},
UnmarshalError: "cannot unmarshal into interface {}",
},
{
ExpectXML: `<DirectCDATA><T1></T1><![CDATA[hi]]><T2></T2></DirectCDATA>`,
Value: &DirectCDATA{CDATA: string("hi")},
},
{
ExpectXML: `<DirectCDATA><T1></T1>hi<T2></T2></DirectCDATA>`,
Value: &DirectCDATA{CDATA: string("hi")},
UnmarshalOnly: true, // marshals with CDATA
},
{
ExpectXML: `<DirectCDATA><T1></T1><T2></T2></DirectCDATA>`,
Value: &DirectCDATA{CDATA: string("")},
},
{
ExpectXML: `<IndirInnerXML><T1></T1><hi/><T2></T2></IndirInnerXML>`,
Value: &IndirInnerXML{InnerXML: stringptr("<hi/>")},
MarshalOnly: true,
},
{
ExpectXML: `<IndirInnerXML><T1></T1><T2></T2></IndirInnerXML>`,
Value: &IndirInnerXML{InnerXML: stringptr("")},
MarshalOnly: true,
},
{
ExpectXML: `<IndirInnerXML><T1></T1><T2></T2></IndirInnerXML>`,
Value: &IndirInnerXML{InnerXML: nil},
},
{
ExpectXML: `<IndirInnerXML><T1></T1><hi/><T2></T2></IndirInnerXML>`,
Value: &IndirInnerXML{InnerXML: nil},
UnmarshalOnly: true,
},
{
ExpectXML: `<IfaceInnerXML><T1></T1><hi/><T2></T2></IfaceInnerXML>`,
Value: &IfaceInnerXML{InnerXML: "<hi/>"},
MarshalOnly: true,
},
{
ExpectXML: `<IfaceInnerXML><T1></T1><hi/><T2></T2></IfaceInnerXML>`,
Value: &IfaceInnerXML{InnerXML: nil},
UnmarshalOnly: true,
},
{
ExpectXML: `<IfaceInnerXML><T1></T1><T2></T2></IfaceInnerXML>`,
Value: &IfaceInnerXML{InnerXML: nil},
},
{
ExpectXML: `<IfaceInnerXML><T1></T1><T2></T2></IfaceInnerXML>`,
Value: &IfaceInnerXML{InnerXML: nil},
UnmarshalOnly: true,
},
{
ExpectXML: `<DirectInnerXML><T1></T1><hi/><T2></T2></DirectInnerXML>`,
Value: &DirectInnerXML{InnerXML: string("<hi/>")},
MarshalOnly: true,
},
{
ExpectXML: `<DirectInnerXML><T1></T1><hi/><T2></T2></DirectInnerXML>`,
Value: &DirectInnerXML{InnerXML: string("<T1></T1><hi/><T2></T2>")},
UnmarshalOnly: true,
},
{
ExpectXML: `<DirectInnerXML><T1></T1><T2></T2></DirectInnerXML>`,
Value: &DirectInnerXML{InnerXML: string("")},
MarshalOnly: true,
},
{
ExpectXML: `<DirectInnerXML><T1></T1><T2></T2></DirectInnerXML>`,
Value: &DirectInnerXML{InnerXML: string("<T1></T1><T2></T2>")},
UnmarshalOnly: true,
},
{
ExpectXML: `<IndirElement><T1></T1><Element>hi</Element><T2></T2></IndirElement>`,
Value: &IndirElement{Element: stringptr("hi")},
},
{
ExpectXML: `<IndirElement><T1></T1><Element></Element><T2></T2></IndirElement>`,
Value: &IndirElement{Element: stringptr("")},
},
{
ExpectXML: `<IndirElement><T1></T1><T2></T2></IndirElement>`,
Value: &IndirElement{Element: nil},
},
{
ExpectXML: `<IfaceElement><T1></T1><Element>hi</Element><T2></T2></IfaceElement>`,
Value: &IfaceElement{Element: "hi"},
MarshalOnly: true,
},
{
ExpectXML: `<IfaceElement><T1></T1><Element>hi</Element><T2></T2></IfaceElement>`,
Value: &IfaceElement{Element: nil},
UnmarshalOnly: true,
},
{
ExpectXML: `<IfaceElement><T1></T1><T2></T2></IfaceElement>`,
Value: &IfaceElement{Element: nil},
},
{
ExpectXML: `<IfaceElement><T1></T1><T2></T2></IfaceElement>`,
Value: &IfaceElement{Element: nil},
UnmarshalOnly: true,
},
{
ExpectXML: `<DirectElement><T1></T1><Element>hi</Element><T2></T2></DirectElement>`,
Value: &DirectElement{Element: string("hi")},
},
{
ExpectXML: `<DirectElement><T1></T1><Element></Element><T2></T2></DirectElement>`,
Value: &DirectElement{Element: string("")},
},
{
ExpectXML: `<IndirOmitEmpty><T1></T1><OmitEmpty>hi</OmitEmpty><T2></T2></IndirOmitEmpty>`,
Value: &IndirOmitEmpty{OmitEmpty: stringptr("hi")},
},
{
// Note: Changed in Go 1.8 to include <OmitEmpty> element (because x.OmitEmpty != nil).
ExpectXML: `<IndirOmitEmpty><T1></T1><OmitEmpty></OmitEmpty><T2></T2></IndirOmitEmpty>`,
Value: &IndirOmitEmpty{OmitEmpty: stringptr("")},
MarshalOnly: true,
},
{
ExpectXML: `<IndirOmitEmpty><T1></T1><OmitEmpty></OmitEmpty><T2></T2></IndirOmitEmpty>`,
Value: &IndirOmitEmpty{OmitEmpty: stringptr("")},
UnmarshalOnly: true,
},
{
ExpectXML: `<IndirOmitEmpty><T1></T1><T2></T2></IndirOmitEmpty>`,
Value: &IndirOmitEmpty{OmitEmpty: nil},
},
{
ExpectXML: `<IfaceOmitEmpty><T1></T1><OmitEmpty>hi</OmitEmpty><T2></T2></IfaceOmitEmpty>`,
Value: &IfaceOmitEmpty{OmitEmpty: "hi"},
MarshalOnly: true,
},
{
ExpectXML: `<IfaceOmitEmpty><T1></T1><OmitEmpty>hi</OmitEmpty><T2></T2></IfaceOmitEmpty>`,
Value: &IfaceOmitEmpty{OmitEmpty: nil},
UnmarshalOnly: true,
},
{
ExpectXML: `<IfaceOmitEmpty><T1></T1><T2></T2></IfaceOmitEmpty>`,
Value: &IfaceOmitEmpty{OmitEmpty: nil},
},
{
ExpectXML: `<IfaceOmitEmpty><T1></T1><T2></T2></IfaceOmitEmpty>`,
Value: &IfaceOmitEmpty{OmitEmpty: nil},
UnmarshalOnly: true,
},
{
ExpectXML: `<DirectOmitEmpty><T1></T1><OmitEmpty>hi</OmitEmpty><T2></T2></DirectOmitEmpty>`,
Value: &DirectOmitEmpty{OmitEmpty: string("hi")},
},
{
ExpectXML: `<DirectOmitEmpty><T1></T1><T2></T2></DirectOmitEmpty>`,
Value: &DirectOmitEmpty{OmitEmpty: string("")},
},
{
ExpectXML: `<IndirAny><T1></T1><Any>hi</Any><T2></T2></IndirAny>`,
Value: &IndirAny{Any: stringptr("hi")},
},
{
ExpectXML: `<IndirAny><T1></T1><Any></Any><T2></T2></IndirAny>`,
Value: &IndirAny{Any: stringptr("")},
},
{
ExpectXML: `<IndirAny><T1></T1><T2></T2></IndirAny>`,
Value: &IndirAny{Any: nil},
},
{
ExpectXML: `<IfaceAny><T1></T1><Any>hi</Any><T2></T2></IfaceAny>`,
Value: &IfaceAny{Any: "hi"},
MarshalOnly: true,
},
{
ExpectXML: `<IfaceAny><T1></T1><Any>hi</Any><T2></T2></IfaceAny>`,
Value: &IfaceAny{Any: nil},
UnmarshalOnly: true,
},
{
ExpectXML: `<IfaceAny><T1></T1><T2></T2></IfaceAny>`,
Value: &IfaceAny{Any: nil},
},
{
ExpectXML: `<IfaceAny><T1></T1><T2></T2></IfaceAny>`,
Value: &IfaceAny{Any: nil},
UnmarshalOnly: true,
},
{
ExpectXML: `<DirectAny><T1></T1><Any>hi</Any><T2></T2></DirectAny>`,
Value: &DirectAny{Any: string("hi")},
},
{
ExpectXML: `<DirectAny><T1></T1><Any></Any><T2></T2></DirectAny>`,
Value: &DirectAny{Any: string("")},
},
{
ExpectXML: `<IndirFoo><T1></T1><Foo>hi</Foo><T2></T2></IndirFoo>`,
Value: &IndirAny{Any: stringptr("hi")},
UnmarshalOnly: true,
},
{
ExpectXML: `<IndirFoo><T1></T1><Foo></Foo><T2></T2></IndirFoo>`,
Value: &IndirAny{Any: stringptr("")},
UnmarshalOnly: true,
},
{
ExpectXML: `<IndirFoo><T1></T1><T2></T2></IndirFoo>`,
Value: &IndirAny{Any: nil},
UnmarshalOnly: true,
},
{
ExpectXML: `<IfaceFoo><T1></T1><Foo>hi</Foo><T2></T2></IfaceFoo>`,
Value: &IfaceAny{Any: nil},
UnmarshalOnly: true,
},
{
ExpectXML: `<IfaceFoo><T1></T1><T2></T2></IfaceFoo>`,
Value: &IfaceAny{Any: nil},
UnmarshalOnly: true,
},
{
ExpectXML: `<IfaceFoo><T1></T1><T2></T2></IfaceFoo>`,
Value: &IfaceAny{Any: nil},
UnmarshalOnly: true,
},
{
ExpectXML: `<DirectFoo><T1></T1><Foo>hi</Foo><T2></T2></DirectFoo>`,
Value: &DirectAny{Any: string("hi")},
UnmarshalOnly: true,
},
{
ExpectXML: `<DirectFoo><T1></T1><Foo></Foo><T2></T2></DirectFoo>`,
Value: &DirectAny{Any: string("")},
UnmarshalOnly: true,
},
}
func TestMarshal(t *testing.T) {
......@@ -1142,9 +1654,19 @@ func TestMarshal(t *testing.T) {
}
data, err := Marshal(test.Value)
if err != nil {
if test.MarshalError == "" {
t.Errorf("#%d: marshal(%#v): %s", idx, test.Value, err)
continue
}
if !strings.Contains(err.Error(), test.MarshalError) {
t.Errorf("#%d: marshal(%#v): %s, want %q", idx, test.Value, err, test.MarshalError)
}
continue
}
if test.MarshalError != "" {
t.Errorf("#%d: Marshal succeeded, want error %q", idx, test.MarshalError)
continue
}
if got, want := string(data), test.ExpectXML; got != want {
if strings.Contains(want, "\n") {
t.Errorf("#%d: marshal(%#v):\nHAVE:\n%s\nWANT:\n%s", idx, test.Value, got, want)
......@@ -1268,8 +1790,16 @@ func TestUnmarshal(t *testing.T) {
}
if err != nil {
t.Errorf("#%d: unexpected error: %#v", i, err)
} else if got, want := dest, test.Value; !reflect.DeepEqual(got, want) {
if test.UnmarshalError == "" {
t.Errorf("#%d: unmarshal(%#v): %s", i, test.ExpectXML, err)
continue
}
if !strings.Contains(err.Error(), test.UnmarshalError) {
t.Errorf("#%d: unmarshal(%#v): %s, want %q", i, test.ExpectXML, err, test.UnmarshalError)
}
continue
}
if got, want := dest, test.Value; !reflect.DeepEqual(got, want) {
t.Errorf("#%d: unmarshal(%q):\nhave %#v\nwant %#v", i, test.ExpectXML, got, want)
}
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment