code review 6460044: encoding/json: handle anonymous fields

encoding/json: handle anonymous fields

Fixes issue 3069.

[rsc, 2012-08-06 20:32:30]

Hello golang-dev@googlegroups.com,

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

[rsc, 2012-08-06 20:36:41]

+gustavo

[niemeyer, 2012-08-31 21:26:45]

Thanks for handling this Russ. Just a couple of questions:

http://codereview.appspot.com/6460044/diff/4001/src/pkg/encoding/json/decode.go
File src/pkg/encoding/json/decode.go (right):

http://codereview.appspot.com/6460044/diff/4001/src/pkg/encoding/json/decode....
src/pkg/encoding/json/decode.go:504: if strings.EqualFold(ff.name, key) {
This is now picking the last match rather than the first one. Is this ok?  No
tests seem to cover this either.

http://codereview.appspot.com/6460044/diff/4001/src/pkg/encoding/json/encode.go
File src/pkg/encoding/json/encode.go (right):

http://codereview.appspot.com/6460044/diff/4001/src/pkg/encoding/json/encode....
src/pkg/encoding/json/encode.go:89: // An anonymous struct field with a name
given in its JSON tag is treated as
That's nice.

http://codereview.appspot.com/6460044/diff/4001/src/pkg/encoding/json/encode....
src/pkg/encoding/json/encode.go:594: current, next = next, current[:0]
Clever reuse.

http://codereview.appspot.com/6460044/diff/4001/src/pkg/encoding/json/encode....
src/pkg/encoding/json/encode.go:628: if count[f.typ] > 1 {
// Force a name collision.

Is that the idea?  The index information is of course not correct for the
multiple fields, so it's a bit surprising on first sight.

It'd also be nice to have a test that breaks when this is commented out. Right
now we can take all of the count logic out and tests still pass.

http://codereview.appspot.com/6460044/diff/4001/src/pkg/encoding/json/encode....
src/pkg/encoding/json/encode.go:636: if ft.Name() == "" {
// Must be a pointer.

[rsc, 2012-09-05 15:15:04]

PTAL

http://codereview.appspot.com/6460044/diff/4001/src/pkg/encoding/json/decode.go
File src/pkg/encoding/json/decode.go (right):

http://codereview.appspot.com/6460044/diff/4001/src/pkg/encoding/json/decode....
src/pkg/encoding/json/decode.go:504: if strings.EqualFold(ff.name, key) {
On 2012/08/31 21:26:45, niemeyer wrote:
> This is now picking the last match rather than the first one. Is this ok?  No
> tests seem to cover this either.

Fixed and added test.

http://codereview.appspot.com/6460044/diff/4001/src/pkg/encoding/json/encode.go
File src/pkg/encoding/json/encode.go (right):

http://codereview.appspot.com/6460044/diff/4001/src/pkg/encoding/json/encode....
src/pkg/encoding/json/encode.go:628: if count[f.typ] > 1 {
On 2012/08/31 21:26:45, niemeyer wrote:
> // Force a name collision.
> 
> Is that the idea?  The index information is of course not correct for the
> multiple fields, so it's a bit surprising on first sight.
> 
> It'd also be nice to have a test that breaks when this is commented out. Right
> now we can take all of the count logic out and tests still pass.

Done.

http://codereview.appspot.com/6460044/diff/4001/src/pkg/encoding/json/encode....
src/pkg/encoding/json/encode.go:636: if ft.Name() == "" {
On 2012/08/31 21:26:45, niemeyer wrote:
> // Must be a pointer.

Done.

[niemeyer, 2012-09-11 02:21:59]

LGTM, thanks.

[rsc, 2012-09-11 03:31:46]

*** Submitted as http://code.google.com/p/go/source/detail?r=a9fc9baa621b ***

encoding/json: handle anonymous fields

Fixes issue 3069.

R=golang-dev, n13m3y3r
CC=golang-dev
http://codereview.appspot.com/6460044

[knieriem, 2012-11-26 21:17:10]

In case I want to encode a struct containing an anonymous
field based on a predeclared type, the encoder will panic
in typeFields():
	panic: reflect: NumField of non-struct type [recovered]

If the condition in line 621 of file encode.go,

	... || !sf.Anonymous,

would be relaxed to 

	t := sf.Type
	... || !(sf.Anonymous && (t.Kind() == reflect.Struct || t.Kind() == reflect.Ptr
&& t.Elem().Kind() == reflect.Struct)),

anonymous fields that have an underlying predeclared type could be
encoded and decoded as well (and the above-mentioned panic would
be avoided).

Michael

For an example program see: http://play.golang.org/p/XVWosi6Zhj

https://codereview.appspot.com/6460044/diff/13002/src/pkg/encoding/json/encod...
File src/pkg/encoding/json/encode.go (right):

https://codereview.appspot.com/6460044/diff/13002/src/pkg/encoding/json/encod...
src/pkg/encoding/json/encode.go:621: if name != "" || !sf.Anonymous {
The THEN-block will be skipped not only for anonymous structs, but also in case
sf is an anonymous field based on a predeclared type like uint16. This will
result in a panic later, when NumField() is called on such an (anonymous)
non-struct field.

[rsc, 2012-12-11 17:38:58]

On Mon, Nov 26, 2012 at 4:17 PM,  <mt4swm@googlemail.com> wrote:
> In case I want to encode a struct containing an anonymous
> field based on a predeclared type, the encoder will panic
> in typeFields():
>         panic: reflect: NumField of non-struct type [recovered]

Thanks. Looks like this is issue 4474.

Russ