Code review - Issue 7139049: code review 7139049: encoding/json: avoid error when decoding unexported fie...https://codereview.appspot.com/2013-01-23T03:01:16+00:00rietveld
Message from unknown
2013-01-17T02:10:10+00:00rickurn:md5:917c517a77b5e502963a619911c2a8c0
Message from unknown
2013-01-17T02:10:14+00:00rickurn:md5:cee1bcea5a8f48b8129c6d6cf9d9ef15
Message from unknown
2013-01-17T02:10:54+00:00rickurn:md5:50d984b51c0c1db214cd965bdaeeeb6d
Message from rickarnoldjr@gmail.com
2013-01-17T02:11:00+00:00rickurn:md5:d7a66676e8b5a34019fd20bb3b168990
Hello golang-dev@googlegroups.com,
I'd like you to review this change to
https://code.google.com/p/go
Message from adg@golang.org
2013-01-17T07:15:07+00:00adgurn:md5:2c9f387b7af0ac4897995e81620e9d2f
https://codereview.appspot.com/7139049/diff/5001/src/pkg/encoding/json/decode_test.go
File src/pkg/encoding/json/decode_test.go (right):
https://codereview.appspot.com/7139049/diff/5001/src/pkg/encoding/json/decode_test.go#newcode1081
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_test.go#newcode1087
src/pkg/encoding/json/decode_test.go:1087: t.Errorf("expected field error on m2; got: %v", err)
ditto
Message from unknown
2013-01-17T12:22:06+00:00rickurn:md5:0aa11921d26f2b438ab5c1e1259e013a
Message from rickarnoldjr@gmail.com
2013-01-17T12:23:20+00:00rickurn:md5:911fc57842ea6f792dc75dcd407177d3
PTAL
https://codereview.appspot.com/7139049/diff/5001/src/pkg/encoding/json/decode_test.go
File src/pkg/encoding/json/decode_test.go (right):
https://codereview.appspot.com/7139049/diff/5001/src/pkg/encoding/json/decode_test.go#newcode1081
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_test.go#newcode1087
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.
Message from unknown
2013-01-18T02:20:48+00:00rickurn:md5:0663538ff6611ad7df1e9874c78d89fb
Message from rickarnoldjr@gmail.com
2013-01-18T02:24:36+00:00rickurn:md5:de4a9f853ed6c79414be3e96b8abf118
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.
Message from rsc@golang.org
2013-01-18T16:33:14+00:00rscurn:md5:cdb5a0ef7eecc933d27592dc8712b44e
LGTM
Okay I guess. Would like to hear from Brad too.
https://codereview.appspot.com/7139049/diff/13001/src/pkg/encoding/json/decode.go
File src/pkg/encoding/json/decode.go (right):
https://codereview.appspot.com/7139049/diff/13001/src/pkg/encoding/json/decode.go#newcode109
src/pkg/encoding/json/decode.go:109: // (Deprecated; kept for compatibility.)
(No longer used; kept for compatibility.)
Message from unknown
2013-01-18T17:29:06+00:00rickurn:md5:2cbbbe90504167f7c2cdcdb20697e372
Message from rickarnoldjr@gmail.com
2013-01-18T17:33:09+00:00rickurn:md5:4b4c53156961b306ac5da303517242c5
PTAL
https://codereview.appspot.com/7139049/diff/13001/src/pkg/encoding/json/decode.go
File src/pkg/encoding/json/decode.go (right):
https://codereview.appspot.com/7139049/diff/13001/src/pkg/encoding/json/decode.go#newcode109
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.
Message from bradfitz@golang.org
2013-01-18T18:47:48+00:00bradfitzurn:md5:0c7be073e86295615a9be5ffb549bcea
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.
Message from rickarnoldjr@gmail.com
2013-01-18T19:06:39+00:00rickurn:md5:cf8c6ca8c33631397c096261994b7d4b
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?
Message from rsc@golang.org
2013-01-18T19:12:24+00:00rscurn:md5:bcd9300c301410f543fe3868bd095847
I don't want to special-case zero fields. It's too fussy.
Russ
Message from adg@golang.org
2013-01-18T23:12:51+00:00adgurn:md5:c05b66b676bd3961040434c1d1da7fd5
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.
Message from adg@golang.org
2013-01-18T23:13:38+00:00adgurn:md5:d9352879769a8ce7d1cc371c6988a250
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).
Message from bradfitz@golang.org
2013-01-19T16:46:41+00:00bradfitzurn:md5:c5047a1be288106a48294c912277b2e1
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.
Message from rickarnoldjr@gmail.com
2013-01-21T21:27:36+00:00rickurn:md5:30538bba14a9683eb3bdc27e71b52234
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.
>
>
>
Message from rsc@golang.org
2013-01-22T22:48:31+00:00rscurn:md5:76c379e2ba58d072a906ea5de0315472
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.
Message from rsc@golang.org
2013-01-22T22:49:13+00:00rscurn:md5:bcc996d84fdd81bbe4e7bc4186d1564e
*** 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>
Message from bradfitz@golang.org
2013-01-22T23:13:55+00:00bradfitzurn:md5:61a8f8ba920ea4601d0dbfaee8522e47
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.
Message from adg@golang.org
2013-01-23T01:11:22+00:00adgurn:md5:2ee774b0bbd28e5c4ae07a2f4bc6c167
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.
Message from rsc@golang.org
2013-01-23T02:51:14+00:00rscurn:md5:60a10ee2e4ba06f816d1311f820f6eaa
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.
Message from adg@golang.org
2013-01-23T03:01:16+00:00adgurn:md5:d894edb820d7f454511c8d8ed83b83b0
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.