Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(406)

Issue 7139049: code review 7139049: encoding/json: avoid error when decoding unexported fie... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 8 months ago by rick
Modified:
9 years, 8 months ago
Reviewers:
CC:
adg, rsc, bradfitz, golang-dev
Visibility:
Public.

Description

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.

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+24 lines, -10 lines) Patch
M src/pkg/encoding/json/decode.go View 1 2 3 4 5 2 chunks +1 line, -9 lines 0 comments Download
M src/pkg/encoding/json/decode_test.go View 1 2 3 4 2 chunks +23 lines, -1 line 0 comments Download

Messages

Total messages: 19
rick
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
9 years, 8 months ago (2013-01-17 02:11:00 UTC) #1
adg
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 ...
9 years, 8 months ago (2013-01-17 07:15:07 UTC) #2
rick
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 ...
9 years, 8 months ago (2013-01-17 12:23:20 UTC) #3
rick
PTAL The code now ignores unexported fields completely as requested in the issue discussion. I ...
9 years, 8 months ago (2013-01-18 02:24:36 UTC) #4
rsc
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): ...
9 years, 8 months ago (2013-01-18 16:33:14 UTC) #5
rick
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, ...
9 years, 8 months ago (2013-01-18 17:33:09 UTC) #6
bradfitz
LGTM I didn't expect my little regression bug report to result in this much of ...
9 years, 8 months ago (2013-01-18 18:47:48 UTC) #7
rick
On 2013/01/18 18:47:48, bradfitz wrote: > LGTM > > I didn't expect my little regression ...
9 years, 8 months ago (2013-01-18 19:06:39 UTC) #8
rsc
I don't want to special-case zero fields. It's too fussy. Russ
9 years, 8 months ago (2013-01-18 19:12:24 UTC) #9
adg
On 19 January 2013 06:06, <rickarnoldjr@gmail.com> wrote: > Maybe along with that issue we can ...
9 years, 8 months ago (2013-01-18 23:12:51 UTC) #10
adg
On 19 January 2013 06:12, Russ Cox <rsc@golang.org> wrote: > I don't want to special-case ...
9 years, 8 months ago (2013-01-18 23:13:38 UTC) #11
bradfitz
On Fri, Jan 18, 2013 at 3:13 PM, Andrew Gerrand <adg@golang.org> wrote: > > On ...
9 years, 8 months ago (2013-01-19 16:46:41 UTC) #12
rick
Russ, are you ok with this approach? Thanks. On Saturday, January 19, 2013 11:46:39 AM ...
9 years, 8 months ago (2013-01-21 21:27:36 UTC) #13
rsc
LGTM as is. I don't want to introduce special cases. The behavior of type T ...
9 years, 8 months ago (2013-01-22 22:48:31 UTC) #14
rsc
*** Submitted as https://code.google.com/p/go/source/detail?r=9439d60e0f96 *** encoding/json: ignore unexported fields in Unmarshal Go 1.0 behavior was ...
9 years, 8 months ago (2013-01-22 22:49:13 UTC) #15
bradfitz
On Tue, Jan 22, 2013 at 2:48 PM, <rsc@golang.org> wrote: > LGTM as is. > ...
9 years, 8 months ago (2013-01-22 23:13:55 UTC) #16
adg
On 23 January 2013 09:48, <rsc@golang.org> wrote: > I don't want to introduce special cases. ...
9 years, 8 months ago (2013-01-23 01:11:22 UTC) #17
rsc
If you do it for a struct passed directly into Unmarshal/Marshal - not for any ...
9 years, 8 months ago (2013-01-23 02:51:14 UTC) #18
adg
9 years, 8 months ago (2013-01-23 03:01:16 UTC) #19
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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b