code review 6943047: encoding/json: encode map key is of string kind, decode...

encoding/json: encode map key is of string kind, decode only of string type

Allows encoding and decoding of maps with key of string kind, not just string type.
Fixes issue 3519.

[ryanslade, 2012-12-13 17:21:16]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[ryanslade, 2012-12-13 20:59:08]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-12-13 21:24:30]

Thanks for working on this.

https://codereview.appspot.com/6943047/diff/7001/src/pkg/encoding/json/decode.go
File src/pkg/encoding/json/decode.go (right):

https://codereview.appspot.com/6943047/diff/7001/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode.go:441: if t.Key().Kind() !=
reflect.TypeOf("").Kind() {
!= reflect.String

https://codereview.appspot.com/6943047/diff/7001/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode.go:550: kv := reflect.New(v.Type().Key())
kv := reflect.ValueOf(key).Convert(v.Type().Key())
mv.SetMapIndex(kv, subv)

will avoid the allocation of reflect.New.

https://codereview.appspot.com/6943047/diff/7001/src/pkg/encoding/json/decode...
File src/pkg/encoding/json/decode_test.go (right):

https://codereview.appspot.com/6943047/diff/7001/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode_test.go:1008: b :=
bytes.NewBufferString(`{"foo":42}`)
Let's test both encoding and decoding. Please do:

var m1, m2 map[stringKind]int
m1 = map[stringKind]int{
    "foo": 42,
}

and then Marshal m1, Unmarshal into m2, and check that reflect.DeepEqual(m1, m2)
is true.

[ryanslade, 2012-12-13 22:15:49]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[ryanslade, 2012-12-13 22:20:40]

I've made the changes as suggested, thanks for taking a look.

https://codereview.appspot.com/6943047/diff/7001/src/pkg/encoding/json/decode.go
File src/pkg/encoding/json/decode.go (right):

https://codereview.appspot.com/6943047/diff/7001/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode.go:441: if t.Key().Kind() !=
reflect.TypeOf("").Kind() {
On 2012/12/13 21:24:30, rsc wrote:
> != reflect.String

Done.

https://codereview.appspot.com/6943047/diff/7001/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode.go:550: kv := reflect.New(v.Type().Key())
On 2012/12/13 21:24:30, rsc wrote:
> kv := reflect.ValueOf(key).Convert(v.Type().Key())
> mv.SetMapIndex(kv, subv)
> 
> will avoid the allocation of reflect.New.

Done.

[rsc, 2012-12-17 15:57:46]

LGTM

[rsc, 2012-12-17 15:59:14]

Sorry but it looks like someone else made some conflicting changes.
Please hg sync, resolve the changes, and hg mail again.

[dave_cheney.net, 2012-12-21 05:12:08]

@ryanslade, something like this should work against +tip
diff -r 7fe729534c6d src/pkg/encoding/json/decode.go
--- a/src/pkg/encoding/json/decode.go   Fri Dec 21 15:14:38 2012 +1100
+++ b/src/pkg/encoding/json/decode.go   Fri Dec 21 16:11:28 2012 +1100
@@ -432,7 +432,7 @@
        case reflect.Map:
                // map must have string type
                t := v.Type()
-               if t.Key() != reflect.TypeOf("") {
+               if t.Key().Kind() != reflect.String {
                        d.saveError(&UnmarshalTypeError{"object", v.Type()})
                        break
                }
@@ -539,7 +539,8 @@
                // Write value back to map;
                // if using struct, subv points into struct already.
                if v.Kind() == reflect.Map {
-                       v.SetMapIndex(reflect.ValueOf(key), subv)
+                       kv := reflect.ValueOf(key).Convert(v.Type().Key())
+                       v.SetMapIndex(kv, subv)
                }


On Tue, Dec 18, 2012 at 2:59 AM, Russ Cox <rsc@golang.org> wrote:
> Sorry but it looks like someone else made some conflicting changes.
> Please hg sync, resolve the changes, and hg mail again.

[ryanslade, 2012-12-24 18:08:44]

Hello rsc@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2012-12-30 04:41:02]

*** Submitted as https://code.google.com/p/go/source/detail?r=7b6096868871 ***

encoding/json: encode map key is of string kind, decode only of string type

Allows encoding and decoding of maps with key of string kind, not just string
type.
Fixes issue 3519.

R=rsc, dave
CC=golang-dev
https://codereview.appspot.com/6943047

Committer: Dave Cheney <dave@cheney.net>