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.

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

encoding/json: clarify Marshal behavior for string keys that implement encoding.TextMarshaler #28827

Open
reinerRubin opened this Issue Nov 16, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@reinerRubin

reinerRubin commented Nov 16, 2018

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

go version go1.10.3 linux/amd64

Does this issue reproduce with the latest release?

yes

What did you do?

https://play.golang.org/p/uBmsW13USTc

tldr:
Before checking for encoding.TextMarshaler implementation the resolve method makes this check

if w.v.Kind() == reflect.String {

so if out type of key is something like:

type MyEnumString string

resolve ignores the encoding.TextMarshaler implementation.

func (w *reflectWithString) resolve() error {

What did you expect to see?

encoding.TextMarshaler is called for string types.

What did you see instead?

Just base string value.

/I have made small proof for myself:
https://play.golang.org/p/CFRVA4yDUlA

@reinerRubin

This comment has been minimized.

reinerRubin commented Nov 16, 2018

If it's a valid bug, I can fix it. I think about or checking for interface implementation beforehand, or mv Kind() to Type(). But the second variant seems quite dangerous.

@bontibon

This comment has been minimized.

Contributor

bontibon commented Nov 16, 2018

cc @rsc

@bontibon

This comment has been minimized.

Contributor

bontibon commented Nov 16, 2018

See the discussion when TextMarshaler support was added for map keys: #12146. There was concerns about backwards compatibility for existing kind strings that implement TextMarshaler.

This may have to be a Go2 request.

@reinerRubin

This comment has been minimized.

reinerRubin commented Nov 16, 2018

Oh, I have tried to find discussion by "TextMarshaler" keyword, but failed. Thanks!

So what shoud I do now? Add a go2 request explicitly?

@bcmills bcmills changed the title from encoding/json for maps keys: encoding.TextMarshaler interface is ignored for string based types by reflectWithString.resolve to encoding/json: encoding.TextMarshaler interface is ignored for string types Nov 19, 2018

@bcmills bcmills added this to the Go1.13 milestone Nov 19, 2018

@bcmills

This comment has been minimized.

Member

bcmills commented Nov 19, 2018

This is mentioned in the documentation (https://tip.golang.org/pkg/encoding/json/#Marshal):

The map keys are sorted and used as JSON object keys by applying the following rules, subject to the UTF-8 coercion described for string values above:

  • string keys are used directly
  • encoding.TextMarshalers are marshaled
  • integer keys are converted to strings

We could certainly be more explicit about the fact that “string keys” means “keys of any string type” rather than “keys of type string exactly”.

@bcmills bcmills changed the title from encoding/json: encoding.TextMarshaler interface is ignored for string types to encoding/json: clarify Marshal behavior for string keys that implement encoding.TextMarshaler Nov 19, 2018

@reinerRubin

This comment has been minimized.

reinerRubin commented Nov 19, 2018

I think it would help. Because basically you can not put your string based type instead a string without an explicit conversion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment