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: custom unmarshaler is ignored for map #34437

Closed
trung opened this issue Sep 20, 2019 · 6 comments

Comments

@trung
Copy link

commented Sep 20, 2019

If i use a custom type for string implementing Unmarshaler interface and use it as key type in a map, the unmarshaling seems not working as expected.

package main

import (
	"encoding/json"
	"fmt"
	"strings"
)

type MyString string

func (m *MyString) UnmarshalJSON(data []byte) error {
	var v string
	if err := json.Unmarshal(data, &v); err != nil {
		return err
	}
	*m = MyString(strings.ToLower(v))
	return nil
}

func main() {
	var m MyString
	_ = json.Unmarshal([]byte(`"HELLO WORLD"`), &m)
	fmt.Println(m) // CORRECT output: hello world

	var p map[MyString]string
	_ = json.Unmarshal([]byte(`
{
	"KEY1": "1",
	"KEY2": "2"
}
`), &p)
	fmt.Println(p) // INCORRECT output: map[KEY1:1 KEY2:2] but expected: map[key1:1 key2:2]
}
@toothrot

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

This sounds related to @mvdan's last comment on a similar issue: #28189 (comment)

In this case, MyString is a string as far as reflect.ValueOf.Kind is concerned:

        var m MyString
	v := reflect.ValueOf(m)
	fmt.Println(v.Kind()) // string

        // Even though MyString implements a custom TextUnmarshaler, the ValueOf.Kind check is used first.
	var textUnmarshalerType = reflect.TypeOf((*encoding.TextUnmarshaler)(nil)).Elem()
	p1 := make(map[MyString]string)
	v2 := reflect.ValueOf(p1)
	fmt.Println(reflect.PtrTo(v2.Type().Key()).Implements(textUnmarshalerType)) // true

See related code here:

case reflect.Map:
// Map key must either have string kind, have an integer kind,
// or be an encoding.TextUnmarshaler.
switch t.Key().Kind() {
case reflect.String,

And here:

case kt.Kind() == reflect.String:
kv = reflect.ValueOf(key).Convert(kt)

Because the string key case is checked first, your custom UnmarshalText or UnmarshalJSON on your map key type will not be used.

As a workaround, you can define a custom map type that defines UnmarshalJSON and does the work you want, but it will not be as clean as you expect in your example. I'll let the domain experts take over from here.

/cc @rsc @mvdan @dsnet @bradfitz

@toothrot toothrot added this to the Go1.14 milestone Sep 20, 2019
@trung

This comment has been minimized.

Copy link
Author

commented Sep 20, 2019

Playing around with reflect package. I found:

var m MyString
var x string
fmt.Println(reflect.ValueOf(m).Type().Name()) // returns "MyString"
fmt.Println(reflect.ValueOf(x).Type().Name()) // returns "string"

It could be a bit hacky ...

@mvdan

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

I'm pretty sure this is working as intended:

To unmarshal a JSON object into a map, Unmarshal first establishes a map to use. If the map is nil, Unmarshal allocates a new map. Otherwise Unmarshal reuses the existing map, keeping existing entries. Unmarshal then 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 it mentions encoding.TextUnmarshaler for map keys, but not json.Unmarshaler. Why do you think your code should be doing something different?

@mvdan mvdan added the WaitingForInfo label Oct 8, 2019
@trung

This comment has been minimized.

Copy link
Author

commented Oct 9, 2019

@mvdan thanks for pointing out the doc.

However, it doesn't seem working per the below code.

package main

import (
	"encoding/json"
	"fmt"
	"strings"
)

type MyString string

func (m *MyString) UnmarshalText(text []byte) error {
	fmt.Println("not invoked")
	*m = MyString(strings.ToLower(string(text)))
	return nil
}

func main() {

	var p map[MyString]string
	_ = json.Unmarshal([]byte(`
{
	"KEY1": "1",
	"KEY2": "2"
}
`), &p)
	fmt.Println(p) // INCORRECT output: map[KEY1:1 KEY2:2] but expected: map[key1:1 key2:2]
}

Am i missing anything?

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@cuonglm

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

The problem is that Kind can not distinguish between static type and underlying type, in:

var x MyString = "x"
t := reflect.TypeOf(x)

Even x has type MyString, t.Kind() will return string.

@gopherbot

This comment has been minimized.

Copy link

commented Oct 10, 2019

Change https://golang.org/cl/200237 mentions this issue: encoding/json: fix unmarshal to a map with key is user defined type

@gopherbot gopherbot closed this in fb9af84 Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.