Skip to content
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.

Sign up for GitHub

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

Jump to bottom

encoding/xml: serialization of interface{} attribute is unsupported, even when dynamic type implements relevant interfaces #48624

Open
silkeh opened this issue Sep 25, 2021 · 4 comments · May be fixed by #48625
Open

encoding/xml: serialization of interface{} attribute is unsupported, even when dynamic type implements relevant interfaces #48624

silkeh opened this issue Sep 25, 2021 · 4 comments · May be fixed by #48625
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@silkeh
Copy link
Contributor

silkeh commented Sep 25, 2021

What version of Go are you using (go version)?

$ go version
go version go1.17.1 linux/amd64

Also reproduces on the playground (see below).

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/silex/.cache/go-build"
GOENV="/home/silex/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/silex/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/silex/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib64/golang"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib64/golang/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17.1"
GCCGO="gccgo"
AR="ar"
CC="x86_64-solus-linux-gcc"
CXX="x86_64-solus-linux-g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build858878585=/tmp/go-build -gno-record-gcc-switches"

What did you do?

When marshalling a struct with an interface field to an XML attribute, an error is returned. Simple example, extended example.

What did you expect to see?

No error, and for the simple example:

XML: <Test a="2009-11-10T23:00:00Z"></Test>

The extended example should return:

<Test a="hello" b="there"><a>hello</a><b>there</b></Test>    (err: %!s(<nil>))
<ITest><a>hello</a><b>there</b></ITest>                      (err: %!s(<nil>))
<ITest a="hello" b="there"><a>hello</a><b>there</b></ITest>  (err: %!s(<nil>))

What did you see instead?

Error: xml: unsupported type: time.Time

This error is incorrect (time.Time implements encoding.TextMarshaler), so should be supported.

Additionally, this does work with values instead of attributes, as shown in the extended example:

<Test a="hello" b="there"><a>hello</a><b>there</b></Test>    (err: %!s(<nil>))
<ITest><a>hello</a><b>there</b></ITest>                      (err: %!s(<nil>))
                                                             (err: xml: unsupported type: main.A)
@silkeh silkeh linked a pull request Sep 25, 2021 that will close this issue
Open
@gopherbot
Copy link

Change https://golang.org/cl/352229 mentions this issue: encoding/xml: fix 'unsupported type' error on interface{} attributes

@mknyszek
Copy link
Contributor

mknyszek commented Oct 4, 2021

If your type actually implements the XML marshaller interface, why not just make the field of type encoding.TextMarshaler or xml.MarshalerAttr? If it could be one or the other, why not define a new interface like

type myInterface interface{
    xml.MarshalerAttr
    encoding.TextMarshaler
}

?

It might be a usability improvement to just have the code check if the empty interface implements one of these, but should it check this for any interface type? What's special about empty interface here?

CC @rsc via https://dev.golang.org/owners

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 4, 2021
@mknyszek mknyszek added this to the Backlog milestone Oct 4, 2021
@mknyszek mknyszek changed the title XML serialization of interface{} attribute results in error encoding/xml: serialization of interface{} attribute is unsupported, even when dynamic type implements relevant interfaces Oct 4, 2021
@silkeh
Copy link
Contributor Author

silkeh commented Oct 4, 2021

If your type actually implements the XML marshaller interface, why not just make the field of type encoding.TextMarshaler or xml.MarshalerAttr? If it could be one or the other, why not define a new interface like

type myInterface interface{
    xml.MarshalerAttr
    encoding.TextMarshaler
}

Well, my type does, but types like int and float don't implement those interfaces. Note that those do work properly, because the de-reference of the interface happens before trying to marshal them.

It might be a usability improvement to just have the code check if the empty interface implements one of these, but should it check this for any interface type? What's special about empty interface here?

What's special about the empty interface is that the behaviour is different depending on if it is a value (which works) or an attribute, and then also if the attribute is a basic type (also works) or not. This seems to be caused by the fact that the marshalling of XML values and attributes happens somewhat differently: marshalValue de-references any interfaces before checking if the supported interfaces are implemented (here), while marshalAttr does it after. The PR I submitted (#48625) simply moves this check up, which fixes this specific inconsistency (the de-referencing still works differently, so the behaviour is not identical).

Also note that the given error is incorrect, which seems like a proper bug to me. This is caused by the error being created after de-referencing the interface.

@mknyszek
Copy link
Contributor

mknyszek commented Oct 4, 2021

Oof. That does seem like an inconsistency, given that it checks for both basic types or if it's a value. Thanks for the additional info.

@mknyszek mknyszek added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 4, 2021
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 4, 2021
@mihaitodor mihaitodor mentioned this issue Nov 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

encoding/xml: fix 'unsupported type' error on interface{} attributes silkeh/go
3 participants
@mknyszek @silkeh @gopherbot