|
|
Created:
10 years, 8 months ago by rick Modified:
10 years, 8 months ago Reviewers:
CC:
adg, rsc, bradfitz, golang-dev Visibility:
Public. |
Descriptionencoding/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.
Patch Set 1 #Patch Set 2 : diff -r 9a3e56fe880c https://code.google.com/p/go #Patch Set 3 : diff -r 9a3e56fe880c https://code.google.com/p/go #
Total comments: 4
Patch Set 4 : diff -r 9a3e56fe880c https://code.google.com/p/go #Patch Set 5 : diff -r 9a3e56fe880c https://code.google.com/p/go #
Total comments: 2
Patch Set 6 : diff -r 9a3e56fe880c https://code.google.com/p/go #MessagesTotal messages: 19
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.)
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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?
Sign in to reply to this message.
I don't want to special-case zero fields. It's too fussy. Russ
Sign in to reply to this message.
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.
Sign in to reply to this message.
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).
Sign in to reply to this message.
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.
Sign in to reply to this message.
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. > > >
Sign in to reply to this message.
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.
Sign in to reply to this message.
*** 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>
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
|