From b81735924936291303559fd71dabaa1aa88f57c5 Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Fri, 21 Jul 2017 16:53:54 -0700 Subject: encoding/json: ignore embedded fields of pointers to unexported non-structs https://golang.org/cl/33773 fixes the JSON marshaler to avoid serializing embedded fields on unexported types of non-struct types. However, Go allows embedding pointer to types, so the check for whether the field is a non-struct type must first dereference the pointer to get at the underlying type. Furthermore, due to a edge-case in the behavior of StructField.PkgPath not being a reliable indicator of whether the field is unexported (see #21122), we use our own logic to determine whether the field is exported or not. The logic in this CL may be simplified depending on what happens in #21122. Fixes #21121 Updates #21122 Change-Id: I8dfd1cdfac8a87950df294a566fb96dfd04fd749 Reviewed-on: https://go-review.googlesource.com/50711 Reviewed-by: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Gobot Gobot --- src/encoding/json/encode.go | 23 +++- src/encoding/json/encode_test.go | 229 +++++++++++++++++++++++++++------------ 2 files changed, 182 insertions(+), 70 deletions(-) diff --git a/src/encoding/json/encode.go b/src/encoding/json/encode.go index 6fcea4735f..0371f0a24d 100644 --- a/src/encoding/json/encode.go +++ b/src/encoding/json/encode.go @@ -1093,7 +1093,22 @@ func typeFields(t reflect.Type) []field { // Scan f.typ for fields to include. for i := 0; i < f.typ.NumField(); i++ { sf := f.typ.Field(i) - if sf.PkgPath != "" && (!sf.Anonymous || sf.Type.Kind() != reflect.Struct) { // unexported + if sf.Anonymous { + t := sf.Type + if t.Kind() == reflect.Ptr { + t = t.Elem() + } + // If embedded, StructField.PkgPath is not a reliable + // indicator of whether the field is exported. + // See https://golang.org/issue/21122 + if !isExported(t.Name()) && t.Kind() != reflect.Struct { + // Ignore embedded fields of unexported non-struct types. + // Do not ignore embedded fields of unexported struct types + // since they may have exported fields. + continue + } + } else if sf.PkgPath != "" { + // Ignore unexported non-embedded fields. continue } tag := sf.Tag.Get("json") @@ -1211,6 +1226,12 @@ func typeFields(t reflect.Type) []field { return fields } +// isExported reports whether the identifier is exported. +func isExported(id string) bool { + r, _ := utf8.DecodeRuneInString(id) + return unicode.IsUpper(r) +} + // dominantField looks through the fields, all of which are known to // have the same name, to find the single field that dominates the // others using Go's embedding rules, modified by the presence of diff --git a/src/encoding/json/encode_test.go b/src/encoding/json/encode_test.go index d5f5f0a691..3fda6a0c71 100644 --- a/src/encoding/json/encode_test.go +++ b/src/encoding/json/encode_test.go @@ -253,76 +253,167 @@ func TestMarshalerEscaping(t *testing.T) { } } -type IntType int - -type MyStruct struct { - IntType -} - -func TestAnonymousNonstruct(t *testing.T) { - var i IntType = 11 - a := MyStruct{i} - const want = `{"IntType":11}` - - b, err := Marshal(a) - if err != nil { - t.Fatalf("Marshal: %v", err) - } - if got := string(b); got != want { - t.Errorf("got %q, want %q", got, want) - } -} - -type unexportedIntType int - -type MyStructWithUnexportedIntType struct { - unexportedIntType -} - -func TestAnonymousNonstructWithUnexportedType(t *testing.T) { - a := MyStructWithUnexportedIntType{11} - const want = `{}` - - b, err := Marshal(a) - if err != nil { - t.Fatalf("Marshal: %v", err) - } - if got := string(b); got != want { - t.Errorf("got %q, want %q", got, want) - } -} - -type MyStructContainingUnexportedStruct struct { - unexportedStructType1 - unexportedIntType -} - -type unexportedStructType1 struct { - ExportedIntType1 - unexportedIntType - unexportedStructType2 -} - -type unexportedStructType2 struct { - ExportedIntType2 - unexportedIntType -} - -type ExportedIntType1 int -type ExportedIntType2 int - -func TestUnexportedAnonymousStructWithExportedType(t *testing.T) { - s2 := unexportedStructType2{3, 4} - s1 := unexportedStructType1{1, 2, s2} - a := MyStructContainingUnexportedStruct{s1, 6} - const want = `{"ExportedIntType1":1,"ExportedIntType2":3}` +func TestAnonymousFields(t *testing.T) { + tests := []struct { + label string // Test name + makeInput func() interface{} // Function to create input value + want string // Expected JSON output + }{{ + // Both S1 and S2 have a field named X. From the perspective of S, + // it is ambiguous which one X refers to. + // This should not serialize either field. + label: "AmbiguousField", + makeInput: func() interface{} { + type ( + S1 struct{ x, X int } + S2 struct{ x, X int } + S struct { + S1 + S2 + } + ) + return S{S1{1, 2}, S2{3, 4}} + }, + want: `{}`, + }, { + label: "DominantField", + // Both S1 and S2 have a field named X, but since S has an X field as + // well, it takes precedence over S1.X and S2.X. + makeInput: func() interface{} { + type ( + S1 struct{ x, X int } + S2 struct{ x, X int } + S struct { + S1 + S2 + x, X int + } + ) + return S{S1{1, 2}, S2{3, 4}, 5, 6} + }, + want: `{"X":6}`, + }, { + // Unexported embedded field of non-struct type should not be serialized. + label: "UnexportedEmbeddedInt", + makeInput: func() interface{} { + type ( + myInt int + S struct{ myInt } + ) + return S{5} + }, + want: `{}`, + }, { + // Exported embedded field of non-struct type should be serialized. + label: "ExportedEmbeddedInt", + makeInput: func() interface{} { + type ( + MyInt int + S struct{ MyInt } + ) + return S{5} + }, + want: `{"MyInt":5}`, + }, { + // Unexported embedded field of pointer to non-struct type + // should not be serialized. + label: "UnexportedEmbeddedIntPointer", + makeInput: func() interface{} { + type ( + myInt int + S struct{ *myInt } + ) + s := S{new(myInt)} + *s.myInt = 5 + return s + }, + want: `{}`, + }, { + // Exported embedded field of pointer to non-struct type + // should be serialized. + label: "ExportedEmbeddedIntPointer", + makeInput: func() interface{} { + type ( + MyInt int + S struct{ *MyInt } + ) + s := S{new(MyInt)} + *s.MyInt = 5 + return s + }, + want: `{"MyInt":5}`, + }, { + // Exported fields of embedded structs should have their + // exported fields be serialized regardless of whether the struct types + // themselves are exported. + label: "EmbeddedStruct", + makeInput: func() interface{} { + type ( + s1 struct{ x, X int } + S2 struct{ y, Y int } + S struct { + s1 + S2 + } + ) + return S{s1{1, 2}, S2{3, 4}} + }, + want: `{"X":2,"Y":4}`, + }, { + // Exported fields of pointers to embedded structs should have their + // exported fields be serialized regardless of whether the struct types + // themselves are exported. + label: "EmbeddedStructPointer", + makeInput: func() interface{} { + type ( + s1 struct{ x, X int } + S2 struct{ y, Y int } + S struct { + *s1 + *S2 + } + ) + return S{&s1{1, 2}, &S2{3, 4}} + }, + want: `{"X":2,"Y":4}`, + }, { + // Exported fields on embedded unexported structs at multiple levels + // of nesting should still be serialized. + label: "NestedStructAndInts", + makeInput: func() interface{} { + type ( + MyInt1 int + MyInt2 int + myInt int + s2 struct { + MyInt2 + myInt + } + s1 struct { + MyInt1 + myInt + s2 + } + S struct { + s1 + myInt + } + ) + return S{s1{1, 2, s2{3, 4}}, 6} + }, + want: `{"MyInt1":1,"MyInt2":3}`, + }} - b, err := Marshal(a) - if err != nil { - t.Fatalf("Marshal: %v", err) - } - if got := string(b); got != want { - t.Errorf("got %q, want %q", got, want) + for _, tt := range tests { + t.Run(tt.label, func(t *testing.T) { + b, err := Marshal(tt.makeInput()) + if err != nil { + t.Fatalf("Marshal() = %v, want nil error", err) + } + if string(b) != tt.want { + t.Fatalf("Marshal() = %q, want %q", b, tt.want) + } + }) } } -- cgit v1.2.3-54-g00ecf