code review 101420043: encoding/json: UnmarshalTypeError now tells what key yo...

encoding/json: UnmarshalTypeError now tells what key you were unmarshalling.

Fixes issue 8254.

[satran, 2014-06-21 17:49:16]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2014-06-23 17:55:38]

R=rsc



On Sat, Jun 21, 2014 at 10:49 AM, <s@ranjeev.in> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> encoding/json: UnmarshalTypeError now tells what key you were
> unmarshalling.
>
> Fixes issue 8254.
>
> Please review this at https://codereview.appspot.com/101420043/
>
> Affected files (+27, -19 lines):
>   M src/pkg/encoding/json/decode.go
>   M src/pkg/encoding/json/decode_test.go
>
>
> Index: src/pkg/encoding/json/decode.go
> ===================================================================
> --- a/src/pkg/encoding/json/decode.go
> +++ b/src/pkg/encoding/json/decode.go
> @@ -92,10 +92,15 @@
>  type UnmarshalTypeError struct {
>         Value string       // description of JSON value - "bool", "array",
> "number -5"
>         Type  reflect.Type // type of Go value it could not be assigned to
> +       Key   string       // stores the most recent decoded key
>  }
>
>  func (e *UnmarshalTypeError) Error() string {
> -       return "json: cannot unmarshal " + e.Value + " into Go value of
> type " + e.Type.String()
> +       err := "json: cannot unmarshal " + e.Value + " into Go value of
> type " + e.Type.String()
> +       if e.Key != "" {
> +               err += " in key " + e.Key
> +       }
> +       return err
>  }
>
>  // An UnmarshalFieldError describes a JSON object key that
> @@ -175,6 +180,7 @@
>         savedError error
>         tempstr    string // scratch space to avoid some allocations
>         useNumber  bool
> +       key        string // stores the key whose values are being decoded
>  }
>
>  // errPhase is used for errors that should not happen unless
> @@ -352,7 +358,7 @@
>                 return
>         }
>         if ut != nil {
> -               d.saveError(&UnmarshalTypeError{"array", v.Type()})
> +               d.saveError(&UnmarshalTypeError{"array", v.Type(), d.key})
>                 d.off--
>                 d.next()
>                 return
> @@ -371,7 +377,7 @@
>                 // Otherwise it's invalid.
>                 fallthrough
>         default:
> -               d.saveError(&UnmarshalTypeError{"array", v.Type()})
> +               d.saveError(&UnmarshalTypeError{"array", v.Type(), d.key})
>                 d.off--
>                 d.next()
>                 return
> @@ -458,7 +464,7 @@
>                 return
>         }
>         if ut != nil {
> -               d.saveError(&UnmarshalTypeError{"object", v.Type()})
> +               d.saveError(&UnmarshalTypeError{"object", v.Type(),
> d.key})
>                 d.off--
>                 d.next() // skip over { } in input
>                 return
> @@ -477,7 +483,7 @@
>                 // map must have string kind
>                 t := v.Type()
>                 if t.Key().Kind() != reflect.String {
> -                       d.saveError(&UnmarshalTypeError{"object",
> v.Type()})
> +                       d.saveError(&UnmarshalTypeError{"object",
> v.Type(), d.key})
>                         break
>                 }
>                 if v.IsNil() {
> @@ -486,7 +492,7 @@
>         case reflect.Struct:
>
>         default:
> -               d.saveError(&UnmarshalTypeError{"object", v.Type()})
> +               d.saveError(&UnmarshalTypeError{"object", v.Type(),
> d.key})
>                 d.off--
>                 d.next() // skip over { } in input
>                 return
> @@ -512,6 +518,8 @@
>                 key, ok := unquoteBytes(item)
>                 if !ok {
>                         d.error(errPhase)
> +               } else {
> +                       d.key = string(key)
>                 }
>
>                 // Figure out field corresponding to key.
> @@ -612,7 +620,7 @@
>         }
>         f, err := strconv.ParseFloat(s, 64)
>         if err != nil {
> -               return nil, &UnmarshalTypeError{"number " + s,
> reflect.TypeOf(0.0)}
> +               return nil, &UnmarshalTypeError{"number " + s,
> reflect.TypeOf(0.0), d.key}
>         }
>         return f, nil
>  }
> @@ -645,7 +653,7 @@
>                         if fromQuoted {
>                                 d.saveError(fmt.Errorf("json: invalid use
> of ,string struct tag, trying to unmarshal %q into %v", item, v.Type()))
>                         } else {
> -                               d.saveError(&UnmarshalTypeError{"string",
> v.Type()})
> +                               d.saveError(&UnmarshalTypeError{"string",
> v.Type(), d.key})
>                         }
>                 }
>                 s, ok := unquoteBytes(item)
> @@ -679,7 +687,7 @@
>                         if fromQuoted {
>                                 d.saveError(fmt.Errorf("json: invalid use
> of ,string struct tag, trying to unmarshal %q into %v", item, v.Type()))
>                         } else {
> -                               d.saveError(&UnmarshalTypeError{"bool",
> v.Type()})
> +                               d.saveError(&UnmarshalTypeError{"bool",
> v.Type(), d.key})
>                         }
>                 case reflect.Bool:
>                         v.SetBool(value)
> @@ -687,7 +695,7 @@
>                         if v.NumMethod() == 0 {
>                                 v.Set(reflect.ValueOf(value))
>                         } else {
> -                               d.saveError(&UnmarshalTypeError{"bool",
> v.Type()})
> +                               d.saveError(&UnmarshalTypeError{"bool",
> v.Type(), d.key})
>                         }
>                 }
>
> @@ -702,10 +710,10 @@
>                 }
>                 switch v.Kind() {
>                 default:
> -                       d.saveError(&UnmarshalTypeError{"string",
> v.Type()})
> +                       d.saveError(&UnmarshalTypeError{"string",
> v.Type(), d.key})
>                 case reflect.Slice:
>                         if v.Type() != byteSliceType {
> -                               d.saveError(&UnmarshalTypeError{"string",
> v.Type()})
> +                               d.saveError(&UnmarshalTypeError{"string",
> v.Type(), d.key})
>                                 break
>                         }
>                         b := make([]byte, base64.StdEncoding.DecodedLen(
> len(s)))
> @@ -721,7 +729,7 @@
>                         if v.NumMethod() == 0 {
>                                 v.Set(reflect.ValueOf(string(s)))
>                         } else {
> -                               d.saveError(&UnmarshalTypeError{"string",
> v.Type()})
> +                               d.saveError(&UnmarshalTypeError{"string",
> v.Type(), d.key})
>                         }
>                 }
>
> @@ -743,7 +751,7 @@
>                         if fromQuoted {
>                                 d.error(fmt.Errorf("json: invalid use of
> ,string struct tag, trying to unmarshal %q into %v", item, v.Type()))
>                         } else {
> -                               d.error(&UnmarshalTypeError{"number",
> v.Type()})
> +                               d.error(&UnmarshalTypeError{"number",
> v.Type(), d.key})
>                         }
>                 case reflect.Interface:
>                         n, err := d.convertNumber(s)
> @@ -752,7 +760,7 @@
>                                 break
>                         }
>                         if v.NumMethod() != 0 {
> -                               d.saveError(&UnmarshalTypeError{"number",
> v.Type()})
> +                               d.saveError(&UnmarshalTypeError{"number",
> v.Type(), d.key})
>                                 break
>                         }
>                         v.Set(reflect.ValueOf(n))
> @@ -760,7 +768,7 @@
>                 case reflect.Int, reflect.Int8, reflect.Int16,
> reflect.Int32, reflect.Int64:
>                         n, err := strconv.ParseInt(s, 10, 64)
>                         if err != nil || v.OverflowInt(n) {
> -                               d.saveError(&UnmarshalTypeError{"number "
> + s, v.Type()})
> +                               d.saveError(&UnmarshalTypeError{"number "
> + s, v.Type(), d.key})
>                                 break
>                         }
>                         v.SetInt(n)
> @@ -768,7 +776,7 @@
>                 case reflect.Uint, reflect.Uint8, reflect.Uint16,
> reflect.Uint32, reflect.Uint64, reflect.Uintptr:
>                         n, err := strconv.ParseUint(s, 10, 64)
>                         if err != nil || v.OverflowUint(n) {
> -                               d.saveError(&UnmarshalTypeError{"number "
> + s, v.Type()})
> +                               d.saveError(&UnmarshalTypeError{"number "
> + s, v.Type(), d.key})
>                                 break
>                         }
>                         v.SetUint(n)
> @@ -776,7 +784,7 @@
>                 case reflect.Float32, reflect.Float64:
>                         n, err := strconv.ParseFloat(s, v.Type().Bits())
>                         if err != nil || v.OverflowFloat(n) {
> -                               d.saveError(&UnmarshalTypeError{"number "
> + s, v.Type()})
> +                               d.saveError(&UnmarshalTypeError{"number "
> + s, v.Type(), d.key})
>                                 break
>                         }
>                         v.SetFloat(n)
> Index: src/pkg/encoding/json/decode_test.go
> ===================================================================
> --- a/src/pkg/encoding/json/decode_test.go
> +++ b/src/pkg/encoding/json/decode_test.go
> @@ -231,7 +231,7 @@
>         {in: `"g-clef: \uD834\uDD1E"`, ptr: new(string), out: "g-clef:
> \U0001D11E"},
>         {in: `"invalid: \uD834x\uDD1E"`, ptr: new(string), out: "invalid:
> \uFFFDx\uFFFD"},
>         {in: "null", ptr: new(interface{}), out: nil},
> -       {in: `{"X": [1,2,3], "Y": 4}`, ptr: new(T), out: T{Y: 4}, err:
> &UnmarshalTypeError{"array", reflect.TypeOf("")}},
> +       {in: `{"X": [1,2,3], "Y": 4}`, ptr: new(T), out: T{Y: 4}, err:
> &UnmarshalTypeError{"array", reflect.TypeOf(""), "X"}},
>         {in: `{"x": 1}`, ptr: new(tx), out: tx{}},
>         {in: `{"F1":1,"F2":2,"F3":3}`, ptr: new(V), out: V{F1: float64(1),
> F2: int32(2), F3: Number("3")}},
>         {in: `{"F1":1,"F2":2,"F3":3}`, ptr: new(V), out: V{F1:
> Number("1"), F2: int32(2), F3: Number("3")}, useNumber: true},
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[satran, 2014-06-28 11:40:24]

Hi,

I'm sure you guys must be caught up with other serious bugs and
implementations. This is my first contribution to the language and I am not
quite sure how things work. I just wanted to know if there is something
that I should do while I wait for the review to be complete.

Thanks.



On Mon, Jun 23, 2014 at 11:25 PM, Brad Fitzpatrick <bradfitz@golang.org>
wrote:

> R=rsc
>
>
>
> On Sat, Jun 21, 2014 at 10:49 AM, <s@ranjeev.in> wrote:
>
>> Reviewers: golang-codereviews,
>>
>> Message:
>> Hello golang-codereviews@googlegroups.com,
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go
>>
>>
>> Description:
>> encoding/json: UnmarshalTypeError now tells what key you were
>> unmarshalling.
>>
>> Fixes issue 8254.
>>
>> Please review this at https://codereview.appspot.com/101420043/
>>
>> Affected files (+27, -19 lines):
>>   M src/pkg/encoding/json/decode.go
>>   M src/pkg/encoding/json/decode_test.go
>>
>>
>> Index: src/pkg/encoding/json/decode.go
>> ===================================================================
>> --- a/src/pkg/encoding/json/decode.go
>> +++ b/src/pkg/encoding/json/decode.go
>> @@ -92,10 +92,15 @@
>>  type UnmarshalTypeError struct {
>>         Value string       // description of JSON value - "bool",
>> "array", "number -5"
>>         Type  reflect.Type // type of Go value it could not be assigned to
>> +       Key   string       // stores the most recent decoded key
>>  }
>>
>>  func (e *UnmarshalTypeError) Error() string {
>> -       return "json: cannot unmarshal " + e.Value + " into Go value of
>> type " + e.Type.String()
>> +       err := "json: cannot unmarshal " + e.Value + " into Go value of
>> type " + e.Type.String()
>> +       if e.Key != "" {
>> +               err += " in key " + e.Key
>> +       }
>> +       return err
>>  }
>>
>>  // An UnmarshalFieldError describes a JSON object key that
>> @@ -175,6 +180,7 @@
>>         savedError error
>>         tempstr    string // scratch space to avoid some allocations
>>         useNumber  bool
>> +       key        string // stores the key whose values are being decoded
>>  }
>>
>>  // errPhase is used for errors that should not happen unless
>> @@ -352,7 +358,7 @@
>>                 return
>>         }
>>         if ut != nil {
>> -               d.saveError(&UnmarshalTypeError{"array", v.Type()})
>> +               d.saveError(&UnmarshalTypeError{"array", v.Type(),
>> d.key})
>>                 d.off--
>>                 d.next()
>>                 return
>> @@ -371,7 +377,7 @@
>>                 // Otherwise it's invalid.
>>                 fallthrough
>>         default:
>> -               d.saveError(&UnmarshalTypeError{"array", v.Type()})
>> +               d.saveError(&UnmarshalTypeError{"array", v.Type(),
>> d.key})
>>                 d.off--
>>                 d.next()
>>                 return
>> @@ -458,7 +464,7 @@
>>                 return
>>         }
>>         if ut != nil {
>> -               d.saveError(&UnmarshalTypeError{"object", v.Type()})
>> +               d.saveError(&UnmarshalTypeError{"object", v.Type(),
>> d.key})
>>                 d.off--
>>                 d.next() // skip over { } in input
>>                 return
>> @@ -477,7 +483,7 @@
>>                 // map must have string kind
>>                 t := v.Type()
>>                 if t.Key().Kind() != reflect.String {
>> -                       d.saveError(&UnmarshalTypeError{"object",
>> v.Type()})
>> +                       d.saveError(&UnmarshalTypeError{"object",
>> v.Type(), d.key})
>>                         break
>>                 }
>>                 if v.IsNil() {
>> @@ -486,7 +492,7 @@
>>         case reflect.Struct:
>>
>>         default:
>> -               d.saveError(&UnmarshalTypeError{"object", v.Type()})
>> +               d.saveError(&UnmarshalTypeError{"object", v.Type(),
>> d.key})
>>                 d.off--
>>                 d.next() // skip over { } in input
>>                 return
>> @@ -512,6 +518,8 @@
>>                 key, ok := unquoteBytes(item)
>>                 if !ok {
>>                         d.error(errPhase)
>> +               } else {
>> +                       d.key = string(key)
>>                 }
>>
>>                 // Figure out field corresponding to key.
>> @@ -612,7 +620,7 @@
>>         }
>>         f, err := strconv.ParseFloat(s, 64)
>>         if err != nil {
>> -               return nil, &UnmarshalTypeError{"number " + s,
>> reflect.TypeOf(0.0)}
>> +               return nil, &UnmarshalTypeError{"number " + s,
>> reflect.TypeOf(0.0), d.key}
>>         }
>>         return f, nil
>>  }
>> @@ -645,7 +653,7 @@
>>                         if fromQuoted {
>>                                 d.saveError(fmt.Errorf("json: invalid use
>> of ,string struct tag, trying to unmarshal %q into %v", item, v.Type()))
>>                         } else {
>> -                               d.saveError(&UnmarshalTypeError{"string",
>> v.Type()})
>> +                               d.saveError(&UnmarshalTypeError{"string",
>> v.Type(), d.key})
>>                         }
>>                 }
>>                 s, ok := unquoteBytes(item)
>> @@ -679,7 +687,7 @@
>>                         if fromQuoted {
>>                                 d.saveError(fmt.Errorf("json: invalid use
>> of ,string struct tag, trying to unmarshal %q into %v", item, v.Type()))
>>                         } else {
>> -                               d.saveError(&UnmarshalTypeError{"bool",
>> v.Type()})
>> +                               d.saveError(&UnmarshalTypeError{"bool",
>> v.Type(), d.key})
>>                         }
>>                 case reflect.Bool:
>>                         v.SetBool(value)
>> @@ -687,7 +695,7 @@
>>                         if v.NumMethod() == 0 {
>>                                 v.Set(reflect.ValueOf(value))
>>                         } else {
>> -                               d.saveError(&UnmarshalTypeError{"bool",
>> v.Type()})
>> +                               d.saveError(&UnmarshalTypeError{"bool",
>> v.Type(), d.key})
>>                         }
>>                 }
>>
>> @@ -702,10 +710,10 @@
>>                 }
>>                 switch v.Kind() {
>>                 default:
>> -                       d.saveError(&UnmarshalTypeError{"string",
>> v.Type()})
>> +                       d.saveError(&UnmarshalTypeError{"string",
>> v.Type(), d.key})
>>                 case reflect.Slice:
>>                         if v.Type() != byteSliceType {
>> -                               d.saveError(&UnmarshalTypeError{"string",
>> v.Type()})
>> +                               d.saveError(&UnmarshalTypeError{"string",
>> v.Type(), d.key})
>>                                 break
>>                         }
>>                         b := make([]byte, base64.StdEncoding.DecodedLen(
>> len(s)))
>> @@ -721,7 +729,7 @@
>>                         if v.NumMethod() == 0 {
>>                                 v.Set(reflect.ValueOf(string(s)))
>>                         } else {
>> -                               d.saveError(&UnmarshalTypeError{"string",
>> v.Type()})
>> +                               d.saveError(&UnmarshalTypeError{"string",
>> v.Type(), d.key})
>>                         }
>>                 }
>>
>> @@ -743,7 +751,7 @@
>>                         if fromQuoted {
>>                                 d.error(fmt.Errorf("json: invalid use of
>> ,string struct tag, trying to unmarshal %q into %v", item, v.Type()))
>>                         } else {
>> -                               d.error(&UnmarshalTypeError{"number",
>> v.Type()})
>> +                               d.error(&UnmarshalTypeError{"number",
>> v.Type(), d.key})
>>                         }
>>                 case reflect.Interface:
>>                         n, err := d.convertNumber(s)
>> @@ -752,7 +760,7 @@
>>                                 break
>>                         }
>>                         if v.NumMethod() != 0 {
>> -                               d.saveError(&UnmarshalTypeError{"number",
>> v.Type()})
>> +                               d.saveError(&UnmarshalTypeError{"number",
>> v.Type(), d.key})
>>                                 break
>>                         }
>>                         v.Set(reflect.ValueOf(n))
>> @@ -760,7 +768,7 @@
>>                 case reflect.Int, reflect.Int8, reflect.Int16,
>> reflect.Int32, reflect.Int64:
>>                         n, err := strconv.ParseInt(s, 10, 64)
>>                         if err != nil || v.OverflowInt(n) {
>> -                               d.saveError(&UnmarshalTypeError{"number
>> " + s, v.Type()})
>> +                               d.saveError(&UnmarshalTypeError{"number
>> " + s, v.Type(), d.key})
>>                                 break
>>                         }
>>                         v.SetInt(n)
>> @@ -768,7 +776,7 @@
>>                 case reflect.Uint, reflect.Uint8, reflect.Uint16,
>> reflect.Uint32, reflect.Uint64, reflect.Uintptr:
>>                         n, err := strconv.ParseUint(s, 10, 64)
>>                         if err != nil || v.OverflowUint(n) {
>> -                               d.saveError(&UnmarshalTypeError{"number
>> " + s, v.Type()})
>> +                               d.saveError(&UnmarshalTypeError{"number
>> " + s, v.Type(), d.key})
>>                                 break
>>                         }
>>                         v.SetUint(n)
>> @@ -776,7 +784,7 @@
>>                 case reflect.Float32, reflect.Float64:
>>                         n, err := strconv.ParseFloat(s, v.Type().Bits())
>>                         if err != nil || v.OverflowFloat(n) {
>> -                               d.saveError(&UnmarshalTypeError{"number
>> " + s, v.Type()})
>> +                               d.saveError(&UnmarshalTypeError{"number
>> " + s, v.Type(), d.key})
>>                                 break
>>                         }
>>                         v.SetFloat(n)
>> Index: src/pkg/encoding/json/decode_test.go
>> ===================================================================
>> --- a/src/pkg/encoding/json/decode_test.go
>> +++ b/src/pkg/encoding/json/decode_test.go
>> @@ -231,7 +231,7 @@
>>         {in: `"g-clef: \uD834\uDD1E"`, ptr: new(string), out: "g-clef:
>> \U0001D11E"},
>>         {in: `"invalid: \uD834x\uDD1E"`, ptr: new(string), out: "invalid:
>> \uFFFDx\uFFFD"},
>>         {in: "null", ptr: new(interface{}), out: nil},
>> -       {in: `{"X": [1,2,3], "Y": 4}`, ptr: new(T), out: T{Y: 4}, err:
>> &UnmarshalTypeError{"array", reflect.TypeOf("")}},
>> +       {in: `{"X": [1,2,3], "Y": 4}`, ptr: new(T), out: T{Y: 4}, err:
>> &UnmarshalTypeError{"array", reflect.TypeOf(""), "X"}},
>>         {in: `{"x": 1}`, ptr: new(tx), out: tx{}},
>>         {in: `{"F1":1,"F2":2,"F3":3}`, ptr: new(V), out: V{F1:
>> float64(1), F2: int32(2), F3: Number("3")}},
>>         {in: `{"F1":1,"F2":2,"F3":3}`, ptr: new(V), out: V{F1:
>> Number("1"), F2: int32(2), F3: Number("3")}, useNumber: true},
>>
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "golang-codereviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-codereviews+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.
>>
>
>

[rsc, 2014-09-15 16:28:47]

This is allocating (d.key) just for the error case. I understand the motivation
but I don't think this is the right solution. I don't know what the right
solution is.

[satran, 2014-09-16 10:33:11]

On 2014/09/15 16:28:47, rsc wrote:
> This is allocating (d.key) just for the error case. I understand the
motivation
> but I don't think this is the right solution. I don't know what the right
> solution is.


There is another approach I did think of. Taking the following as the input json
{
   "a": {
      "b": [1, 3x]
   }
}

We can see that 3x would cause an exception. Thus we can respond with an
exception 

 json: cannot unmarshal 1st value into Go value of type ... occurring in `a.b`.

The gives the user a more detailed explanation at the cost of tracking down
every key. I don't know exactly how I could implement the position in the array
yet. But if you think this seems to be in the right direction I could give it
another shot.

[gobot, 2014-12-19 05:06:09]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/101420043 is best) in the description in your
new CL.

Thanks very much.