code review 7139049: encoding/json: avoid error when decoding unexported fie...

encoding/json: ignore unexported fields in Unmarshal

Go 1.0 behavior was to create an UnmarshalFieldError when a json value name matched an unexported field name. This error will no longer be created and the field will be skipped instead.

Fixes issue 4660.

[rick, 2013-01-17 02:11:00]

Hello golang-dev@googlegroups.com,

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

[adg, 2013-01-17 07:15:07]

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

https://codereview.appspot.com/7139049/diff/5001/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode_test.go:1081: t.Errorf("expected no error; got:
%v", err)
reverse the error message:

"got error %v, expected nil"

https://codereview.appspot.com/7139049/diff/5001/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode_test.go:1087: t.Errorf("expected field error on m2;
got: %v", err)
ditto

[rick, 2013-01-17 12:23:20]

PTAL

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

https://codereview.appspot.com/7139049/diff/5001/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode_test.go:1081: t.Errorf("expected no error; got:
%v", err)
On 2013/01/17 07:15:07, adg wrote:
> reverse the error message:
> 
> "got error %v, expected nil"

Done.

https://codereview.appspot.com/7139049/diff/5001/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode_test.go:1087: t.Errorf("expected field error on m2;
got: %v", err)
On 2013/01/17 07:15:07, adg wrote:
> ditto

Done.

[rick, 2013-01-18 02:24:36]

PTAL

The code now ignores unexported fields completely as requested in the issue
discussion.

I left the UnmarshalFieldError and noted that it is deprecated in the comments
since removing it could break existing code. Do you think that's the correct
approach?

Thanks.

[rsc, 2013-01-18 16:33:14]

LGTM

Okay I guess. Would like to hear from Brad too.

https://codereview.appspot.com/7139049/diff/13001/src/pkg/encoding/json/decod...
File src/pkg/encoding/json/decode.go (right):

https://codereview.appspot.com/7139049/diff/13001/src/pkg/encoding/json/decod...
src/pkg/encoding/json/decode.go:109: // (Deprecated; kept for compatibility.)
(No longer used; kept for compatibility.)

[rick, 2013-01-18 17:33:09]

PTAL

https://codereview.appspot.com/7139049/diff/13001/src/pkg/encoding/json/decod...
File src/pkg/encoding/json/decode.go (right):

https://codereview.appspot.com/7139049/diff/13001/src/pkg/encoding/json/decod...
src/pkg/encoding/json/decode.go:109: // (Deprecated; kept for compatibility.)
On 2013/01/18 16:33:15, rsc wrote:
> (No longer used; kept for compatibility.)

Done.

[bradfitz, 2013-01-18 18:47:48]

LGTM

I didn't expect my little regression bug report to result in this much of a
change, but I'm fine with it.  I found the old behavior too scary.

If we wanted to keep UnmarshalFieldError for pedagogical purposes, perhaps we
only return it when the Unmarshal would otherwise decode no fields.

So a new user doing:

type T struct {
    foo string
}

and trying to decode `{"foo": "bar"}` would get the error, but NOT a user trying
to decode:

type T struct {
    Name string `json:"name"`
    foo  string
}

... trying to decode `{"foo": "bar", "name": "Bob"}`

because in that case, some progress was made.

I haven't thought through the negative consequences of that too much, though,
and I imagine people could just read the docs or show up on the mailing list
with questions.

[rick, 2013-01-18 19:06:39]

On 2013/01/18 18:47:48, bradfitz wrote:
> LGTM
> 
> I didn't expect my little regression bug report to result in this much of a
> change, but I'm fine with it.  I found the old behavior too scary.
> 
> If we wanted to keep UnmarshalFieldError for pedagogical purposes, perhaps we
> only return it when the Unmarshal would otherwise decode no fields.
> 
> So a new user doing:
> 
> type T struct {
>     foo string
> }
> 
> and trying to decode `{"foo": "bar"}` would get the error, but NOT a user
trying
> to decode:
> 
> type T struct {
>     Name string `json:"name"`
>     foo  string
> }
> 
> ... trying to decode `{"foo": "bar", "name": "Bob"}`
> 
> because in that case, some progress was made.
> 
> I haven't thought through the negative consequences of that too much, though,
> and I imagine people could just read the docs or show up on the mailing list
> with questions.

I did notice that there is a somewhat related issue on documentation: 4664.
Seems like Unmarshal field mapping is a common area of confusion. Maybe along
with that issue we can document that non-exported fields are always ignored?

[rsc, 2013-01-18 19:12:24]

I don't want to special-case zero fields. It's too fussy.

Russ

[adg, 2013-01-18 23:12:51]

On 19 January 2013 06:06, <rickarnoldjr@gmail.com> wrote:

> Maybe along with that issue we can document that non-exported fields are
> always ignored?
>

Yes, the docs need updating.

[adg, 2013-01-18 23:13:38]

On 19 January 2013 06:12, Russ Cox <rsc@golang.org> wrote:

> I don't want to special-case zero fields. It's too fussy.


Are you sure? I think it makes sense to give an error if the user provides
a type that is unusable by encoding/json (no exported fields or
UnmarshalJSON method).

[bradfitz, 2013-01-19 16:46:41]

On Fri, Jan 18, 2013 at 3:13 PM, Andrew Gerrand <adg@golang.org> wrote:

>
> On 19 January 2013 06:12, Russ Cox <rsc@golang.org> wrote:
>
>> I don't want to special-case zero fields. It's too fussy.
>
>
> Are you sure? I think it makes sense to give an error if the user provides
> a type that is unusable by encoding/json (no exported fields or
> UnmarshalJSON method).
>

Oh, I hadn't even thought of that.  That's much better than what I was
proposing.

Yes, if the user pass *T to Unmarshal but T is:

type T struct {
   onlyLower string
   etc           int
}

... there's no way it'll ever work.  That seems like a good place to teach.

[rick, 2013-01-21 21:27:36]

Russ, are you ok with this approach?

Thanks.


On Saturday, January 19, 2013 11:46:39 AM UTC-5, Brad Fitzpatrick wrote:
>
>
>
> On Fri, Jan 18, 2013 at 3:13 PM, Andrew Gerrand <a...@golang.org<javascript:>
> > wrote:
>
>>
>> On 19 January 2013 06:12, Russ Cox <r...@golang.org <javascript:>> wrote:
>>
>>> I don't want to special-case zero fields. It's too fussy.
>>
>>
>> Are you sure? I think it makes sense to give an error if the user 
>> provides a type that is unusable by encoding/json (no exported fields or 
>> UnmarshalJSON method).
>>
>
> Oh, I hadn't even thought of that.  That's much better than what I was 
> proposing.
>
> Yes, if the user pass *T to Unmarshal but T is:
>
> type T struct {
>    onlyLower string
>    etc           int
> }
>
> ... there's no way it'll ever work.  That seems like a good place to teach.
>  
>
>

[rsc, 2013-01-22 22:48:31]

LGTM as is.

I don't want to introduce special cases. The behavior of

type T struct {
   X int 
   Y int
   Z int
   w int
}

should not change if you remove X, Y, or Z individually, and it does not. If you
remove all three, it should not change either.

[rsc, 2013-01-22 22:49:13]

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

encoding/json: ignore unexported fields in Unmarshal

Go 1.0 behavior was to create an UnmarshalFieldError when a json value name
matched an unexported field name. This error will no longer be created and the
field will be skipped instead.

Fixes issue 4660.

R=adg, rsc, bradfitz
CC=golang-dev
https://codereview.appspot.com/7139049

Committer: Russ Cox <rsc@golang.org>

[bradfitz, 2013-01-22 23:13:55]

On Tue, Jan 22, 2013 at 2:48 PM, <rsc@golang.org> wrote:

> LGTM as is.
>
> I don't want to introduce special cases. The behavior of
>
> type T struct {
>    X int
>    Y int
>    Z int
>    w int
> }
>
> should not change if you remove X, Y, or Z individually, and it does
> not. If you remove all three, it should not change either.
>

I guess.

But if there are no exported fields, why are they unmarshalling in the
first place?

I actually don't really care much either way.

[adg, 2013-01-23 01:11:22]

On 23 January 2013 09:48, <rsc@golang.org> wrote:

> I don't want to introduce special cases. The behavior of
>
> type T struct {
>    X int
>    Y int
>    Z int
>    w int
> }
>
> should not change if you remove X, Y, or Z individually, and it does
> not. If you remove all three, it should not change either.
>

I understand the desire not to introduce special cases.

However, calling unmarshal on a data structure that can never receive any
of the data seems like it is always a mistake to me. And it's a mistake
that pretty much only newbies will make.

I'm not averse to improving this with better docs though.

[rsc, 2013-01-23 02:51:14]

If you do it for a struct passed directly into Unmarshal/Marshal - not for
any of its subfields, and not if it has methods - then that's probably
okay. What I want to avoid is bombs in the code that only go off if you
happen to get to the right place in a complex data structure.

[adg, 2013-01-23 03:01:16]

LGTM

On 23 January 2013 13:51, Russ Cox <rsc@golang.org> wrote:

> If you do it for a struct passed directly into Unmarshal/Marshal - not for
> any of its subfields, and not if it has methods - then that's probably
> okay. What I want to avoid is bombs in the code that only go off if you
> happen to get to the right place in a complex data structure.


Good call. Let's do this in a follow up CL, if at all.