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.

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: Map key marshals with MarshalText but unmarshals with UnmarshalJSON #29732

Open
bmoylan opened this issue Jan 14, 2019 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@bmoylan
Copy link

bmoylan commented Jan 14, 2019

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

$ go version
go version go1.11.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

playgound: https://play.golang.org/p/4g3sHaRjh7z

  1. Declare a struct type that implements all of MarshalJSON, UnmarshalJSON, MarshalText, UnmarshalText, where the JSON form is an object, not string.
  2. Use that type as a map key in a map.
  3. json.Marshal the map. Notice the MarshalText behavior is used to convert the map keys to strings in the resulting JSON object.
  4. json.Unmarshal the JSON bytes from (3) into a new map.

What did you expect to see?

json.Unmarshal should use the map key's UnmarshalText method, because it is forced to be a JSON string.

What did you see instead?

json.Unmarshal used the type's UnmarshalJSON method on the map key data.

Debugging

TextMarshaler map keys were introduced in ffbd31e. The core issue is that literalStore always prefers UnmarshalJSON (decode.go#L855) before attempting UnmarshalText because indirect only returns one of the interfaces and prefers JSON to Text. This is correct in most cases, but for map keys we're calling literalStore explicitly because the value implements UnmarshalText (decode.go#L776).

@mvdan
Copy link
Member

mvdan commented Jan 14, 2019

Perhaps a duplicate of #23455?

@bmoylan
Copy link
Author

bmoylan commented Jan 14, 2019

Thanks for the link, @mvdan. That issue seems to be describing a type which implements UnmarshalJSON but not UnmarshalText. In my case, my type implements both interfaces. Decode correctly checks if the map key type implements UnmarshalText, but if yes it eventually attempts UnmarshalJSON instead (if both are implemented).

@bcmills
Copy link
Contributor

bcmills commented Jan 30, 2019

CC @rsc @dsnet @bradfitz for encoding/json

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 30, 2019
@bradfitz bradfitz added this to the Go1.13 milestone Jan 30, 2019
@bcmills
Copy link
Contributor

bcmills commented Jan 30, 2019

From the current documentation for encoding/json:

Map values encode as JSON objects. The map's key type must either be a string, an integer type, or implement encoding.TextMarshaler.

Unmarshal […] stores key-value pairs from the JSON object into the map. The map's key type must either be a string, an integer, or implement encoding.TextUnmarshaler.

Note that in your example, the map's key type itself does not implement encoding.TextUnmarshaler: the pointer to that key type does. However, I think that's just a documentation issue: I don't see any plausible way that a value type that is sensible as a map key could also implement TextUnmarshaler.

@bcmills
Copy link
Contributor

bcmills commented Jan 30, 2019

At any rate, removing the UnmarshalJSON method fixes the linked example, so there is clearly an asymmetry here that should be fixed.

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants