Skip to content

Commit

Permalink
Output field path in strict errors
Browse files Browse the repository at this point in the history
  • Loading branch information
@liggitt
liggitt committed Nov 20, 2021
1 parent c049b76 commit be5ad23
Show file tree
Hide file tree
Showing 5 changed files with 106 additions and 19 deletions.
45 changes: 40 additions & 5 deletions internal/golang/encoding/json/decode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ type decodeState struct {

savedStrictErrors []error
seenStrictErrors map[string]struct{}
strictFieldStack []string

caseSensitive bool

Expand Down Expand Up @@ -261,6 +262,8 @@ func (d *decodeState) init(data []byte) *decodeState {
// Reuse the allocated space for the FieldStack slice.
d.errorContext.FieldStack = d.errorContext.FieldStack[:0]
}
// Reuse the allocated space for the strict FieldStack slice.
d.strictFieldStack = d.strictFieldStack[:0]
return d
}

Expand Down Expand Up @@ -555,6 +558,12 @@ func (d *decodeState) array(v reflect.Value) error {
break
}

origStrictFieldStackLen := len(d.strictFieldStack)
defer func() {
// Reset to original length and reuse the allocated space for the strict FieldStack slice.
d.strictFieldStack = d.strictFieldStack[:origStrictFieldStackLen]
}()

i := 0
for {
// Look ahead for ] - can only happen on first iteration.
Expand All @@ -580,6 +589,7 @@ func (d *decodeState) array(v reflect.Value) error {
}
}

d.appendStrictFieldStackIndex(i)
if i < v.Len() {
// Decode into element.
if err := d.value(v.Index(i)); err != nil {
Expand All @@ -591,6 +601,8 @@ func (d *decodeState) array(v reflect.Value) error {
return err
}
}
// Reset to original length and reuse the allocated space for the strict FieldStack slice.
d.strictFieldStack = d.strictFieldStack[:origStrictFieldStackLen]
i++

// Next token must be , or ].
Expand Down Expand Up @@ -683,7 +695,7 @@ func (d *decodeState) object(v reflect.Value) error {
seenKeys = map[string]struct{}{}
}
if _, seen := seenKeys[fieldName]; seen {
d.saveStrictError(fmt.Errorf("duplicate field %q", fieldName))
d.saveStrictError(d.newFieldError("duplicate field", fieldName))
} else {
seenKeys[fieldName] = struct{}{}
}
Expand All @@ -699,7 +711,7 @@ func (d *decodeState) object(v reflect.Value) error {
var seenKeys uint64
checkDuplicateField = func(fieldNameIndex int, fieldName string) {
if seenKeys&(1<<fieldNameIndex) != 0 {
d.saveStrictError(fmt.Errorf("duplicate field %q", fieldName))
d.saveStrictError(d.newFieldError("duplicate field", fieldName))
} else {
seenKeys = seenKeys | (1 << fieldNameIndex)
}
Expand All @@ -712,7 +724,7 @@ func (d *decodeState) object(v reflect.Value) error {
seenIndexes = make([]bool, len(fields.list))
}
if seenIndexes[fieldNameIndex] {
d.saveStrictError(fmt.Errorf("duplicate field %q", fieldName))
d.saveStrictError(d.newFieldError("duplicate field", fieldName))
} else {
seenIndexes[fieldNameIndex] = true
}
Expand All @@ -732,6 +744,7 @@ func (d *decodeState) object(v reflect.Value) error {
if d.errorContext != nil {
origErrorContext = *d.errorContext
}
origStrictFieldStackLen := len(d.strictFieldStack)

for {
// Read opening " of string key or closing }.
Expand Down Expand Up @@ -768,6 +781,7 @@ func (d *decodeState) object(v reflect.Value) error {
if checkDuplicateField != nil {
checkDuplicateField(0, string(key))
}
d.appendStrictFieldStackKey(string(key))
} else {
var f *field
if i, ok := fields.nameIndex[string(key)]; ok {
Expand Down Expand Up @@ -820,8 +834,9 @@ func (d *decodeState) object(v reflect.Value) error {
}
d.errorContext.FieldStack = append(d.errorContext.FieldStack, f.name)
d.errorContext.Struct = t
d.appendStrictFieldStackKey(f.name)
} else if d.disallowUnknownFields {
d.saveStrictError(fmt.Errorf("unknown field %q", key))
d.saveStrictError(d.newFieldError("unknown field", string(key)))
}
}

Expand Down Expand Up @@ -905,6 +920,8 @@ func (d *decodeState) object(v reflect.Value) error {
d.errorContext.FieldStack = d.errorContext.FieldStack[:len(origErrorContext.FieldStack)]
d.errorContext.Struct = origErrorContext.Struct
}
// Reset to original length and reuse the allocated space for the strict FieldStack slice.
d.strictFieldStack = d.strictFieldStack[:origStrictFieldStackLen]
if d.opcode == scanEndObject {
break
}
Expand Down Expand Up @@ -1141,6 +1158,12 @@ func (d *decodeState) valueInterface() (val interface{}) {

// arrayInterface is like array but returns []interface{}.
func (d *decodeState) arrayInterface() []interface{} {
origStrictFieldStackLen := len(d.strictFieldStack)
defer func() {
// Reset to original length and reuse the allocated space for the strict FieldStack slice.
d.strictFieldStack = d.strictFieldStack[:origStrictFieldStackLen]
}()

var v = make([]interface{}, 0)
for {
// Look ahead for ] - can only happen on first iteration.
Expand All @@ -1149,7 +1172,10 @@ func (d *decodeState) arrayInterface() []interface{} {
break
}

d.appendStrictFieldStackIndex(len(v))
v = append(v, d.valueInterface())
// Reset to original length and reuse the allocated space for the strict FieldStack slice.
d.strictFieldStack = d.strictFieldStack[:origStrictFieldStackLen]

// Next token must be , or ].
if d.opcode == scanSkipSpace {
Expand All @@ -1167,6 +1193,12 @@ func (d *decodeState) arrayInterface() []interface{} {

// objectInterface is like object but returns map[string]interface{}.
func (d *decodeState) objectInterface() map[string]interface{} {
origStrictFieldStackLen := len(d.strictFieldStack)
defer func() {
// Reset to original length and reuse the allocated space for the strict FieldStack slice.
d.strictFieldStack = d.strictFieldStack[:origStrictFieldStackLen]
}()

m := make(map[string]interface{})
for {
// Read opening " of string key or closing }.
Expand Down Expand Up @@ -1199,12 +1231,15 @@ func (d *decodeState) objectInterface() map[string]interface{} {

if d.disallowDuplicateFields {
if _, exists := m[key]; exists {
d.saveStrictError(fmt.Errorf("duplicate field %q", key))
d.saveStrictError(d.newFieldError("duplicate field", key))
}
}

// Read value.
d.appendStrictFieldStackKey(key)
m[key] = d.valueInterface()
// Reset to original length and reuse the allocated space for the strict FieldStack slice.
d.strictFieldStack = d.strictFieldStack[:origStrictFieldStackLen]

// Next token must be , or }.
if d.opcode == scanSkipSpace {
Expand Down
2 changes: 1 addition & 1 deletion internal/golang/encoding/json/decode_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -899,7 +899,7 @@ var unmarshalTests = []unmarshalTest{
"Q": 18
}`,
ptr: new(Top),
err: fmt.Errorf("json: unknown field \"extra\""),
err: fmt.Errorf("json: unknown field \"e.extra\""),
disallowUnknownFields: true,
},
// issue 26444
Expand Down
28 changes: 28 additions & 0 deletions internal/golang/encoding/json/kubernetes_patch.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ package json

import (
gojson "encoding/json"
"fmt"
"strconv"
"strings"
)

Expand Down Expand Up @@ -69,6 +71,14 @@ func (d *Decoder) DisallowDuplicateFields() {
d.d.disallowDuplicateFields = true
}

func (d *decodeState) newFieldError(msg, field string) error {
if len(d.strictFieldStack) > 0 {
return fmt.Errorf("%s %q", msg, strings.Join(d.strictFieldStack, "")+"."+field)
} else {
return fmt.Errorf("%s %q", msg, field)
}
}

// saveStrictError saves a strict decoding error,
// for reporting at the end of the unmarshal if no other errors occurred.
func (d *decodeState) saveStrictError(err error) {
Expand All @@ -90,6 +100,24 @@ func (d *decodeState) saveStrictError(err error) {
d.savedStrictErrors = append(d.savedStrictErrors, err)
}

func (d *decodeState) appendStrictFieldStackKey(key string) {
if !d.disallowDuplicateFields && !d.disallowUnknownFields {
return
}
if len(d.strictFieldStack) > 0 {
d.strictFieldStack = append(d.strictFieldStack, ".", key)
} else {
d.strictFieldStack = append(d.strictFieldStack, key)
}
}

func (d *decodeState) appendStrictFieldStackIndex(i int) {
if !d.disallowDuplicateFields && !d.disallowUnknownFields {
return
}
d.strictFieldStack = append(d.strictFieldStack, "[", strconv.Itoa(i), "]")
}

// UnmarshalStrictError holds errors resulting from use of strict disallow___ decoder directives.
// If this is returned from Unmarshal(), it means the decoding was successful in all other respects.
type UnmarshalStrictError struct {
Expand Down
46 changes: 35 additions & 11 deletions internal/golang/encoding/json/kubernetes_patch_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,12 @@ func TestUnmarshalWithOptions(t *testing.T) {
}

func TestStrictErrors(t *testing.T) {
type SubType struct {
}
type Typed struct {
A int `json:"a"`
A int `json:"a"`
B map[string]SubType `json:"b"`
C []SubType `json:"c"`
}

testcases := []struct {
Expand All @@ -111,21 +115,25 @@ func TestStrictErrors(t *testing.T) {
name: "malformed 1",
in: `{`,
expectStrictErr: false,
expectErr: `unexpected end of JSON input`,
},
{
name: "malformed 2",
in: `{}}`,
expectStrictErr: false,
expectErr: `invalid character '}' after top-level value`,
},
{
name: "malformed 3",
in: `{,}`,
expectStrictErr: false,
expectErr: `invalid character ',' looking for beginning of object key string`,
},
{
name: "type error",
in: `{"a":true}`,
expectStrictErr: false,
expectErr: `json: cannot unmarshal bool into Go struct field Typed.a of type int`,
},
{
name: "unknown",
Expand All @@ -139,15 +147,23 @@ func TestStrictErrors(t *testing.T) {
expectStrictErr: true,
expectErr: `json: unknown field "unknown", unknown field "unknown2"`,
},
{
name: "nested unknowns",
in: `{"a":1,"unknown":true,"unknown2":true,"unknown":true,"unknown2":true,"b":{"a":{"unknown":true}},"c":[{"unknown":true},{"unknown":true}]}`,
expectStrictErr: true,
expectErr: `json: unknown field "unknown", unknown field "unknown2", unknown field "b.a.unknown", unknown field "c[0].unknown", unknown field "c[1].unknown"`,
},
{
name: "unknowns and type error",
in: `{"unknown":true,"a":true}`,
expectStrictErr: false,
expectErr: `json: cannot unmarshal bool into Go struct field Typed.a of type int`,
},
{
name: "unknowns and malformed error",
in: `{"unknown":true}}`,
expectStrictErr: false,
expectErr: `invalid character '}' after top-level value`,
},
}

Expand All @@ -161,8 +177,8 @@ func TestStrictErrors(t *testing.T) {
if tc.expectStrictErr != isStrictErr {
t.Fatalf("expected strictErr=%v, got %v: %v", tc.expectStrictErr, isStrictErr, err)
}
if !strings.Contains(err.Error(), tc.expectErr) {
t.Fatalf("expected error containing %q, got %q", tc.expectErr, err)
if err.Error() != tc.expectErr {
t.Fatalf("expected error\n%s\n%s", tc.expectErr, err)
}
t.Log(err)
})
Expand Down Expand Up @@ -569,11 +585,16 @@ func TestPreserveInts(t *testing.T) {
}

func TestDisallowDuplicateFields(t *testing.T) {
type SubType struct {
F int `json:"f"`
G int `json:"g"`
}
type MixedObj struct {
A int `json:"a"`
B int `json:"b"`
C int
D map[string]string
D map[string]string `json:"d"`
E []SubType `json:"e"`
}
type SmallObj struct {
F01 int
Expand Down Expand Up @@ -718,21 +739,21 @@ func TestDisallowDuplicateFields(t *testing.T) {
}{
{
name: "duplicate typed",
in: `{"a":1,"a":2,"a":3,"b":4,"b":5,"b":6,"C":7,"C":8,"C":9}`,
in: `{"a":1,"a":2,"a":3,"b":4,"b":5,"b":6,"C":7,"C":8,"C":9,"e":[{"f":1,"f":1}]}`,
to: &MixedObj{},
expectErr: `json: duplicate field "a", duplicate field "b", duplicate field "C"`,
expectErr: `json: duplicate field "a", duplicate field "b", duplicate field "C", duplicate field "e[0].f"`,
},
{
name: "duplicate typed map field",
in: `{"d":{"a":"","b":"","c":"","a":"","b":"","c":""}}`,
to: &MixedObj{},
expectErr: `json: duplicate field "a", duplicate field "b", duplicate field "c"`,
expectErr: `json: duplicate field "d.a", duplicate field "d.b", duplicate field "d.c"`,
},
{
name: "duplicate untyped map",
in: `{"a":"","b":"","a":"","b":"","c":{"c":"","c":""}}`,
in: `{"a":"","b":"","a":"","b":"","c":{"c":"","c":""},"d":{"e":{"f":1,"f":1}},"e":[{"g":1,"g":1}]}`,
to: &map[string]interface{}{},
expectErr: `json: duplicate field "a", duplicate field "b", duplicate field "c"`,
expectErr: `json: duplicate field "a", duplicate field "b", duplicate field "c.c", duplicate field "d.e.f", duplicate field "e[0].g"`,
},
{
name: "small obj",
Expand All @@ -754,8 +775,11 @@ func TestDisallowDuplicateFields(t *testing.T) {
if (len(tc.expectErr) > 0) != (err != nil) {
t.Fatalf("expected err=%v, got %v", len(tc.expectErr) > 0, err)
}
if len(tc.expectErr) > 0 && !strings.Contains(err.Error(), tc.expectErr) {
t.Fatalf("expected err containing '%s', got %v", tc.expectErr, err)
if err == nil {
return
}
if err.Error() != tc.expectErr {
t.Fatalf("expected err\n%s\ngot\n%s", tc.expectErr, err.Error())
}
})
}
Expand Down
4 changes: 2 additions & 2 deletions json_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,8 @@ func TestUnmarshal(t *testing.T) {
name: "duplicate map field",
in: `{"c":{"a":"1","a":"2","b":"1","b":"2"}}`,
to: func() interface{} { return &Obj{} },
expect: &Obj{C: map[string]string{"a": "2", "b": "2"}}, // last duplicates win
expectStrictErrs: []string{`duplicate field "a"`, `duplicate field "b"`}, // multiple strict errors are returned
expect: &Obj{C: map[string]string{"a": "2", "b": "2"}}, // last duplicates win
expectStrictErrs: []string{`duplicate field "c.a"`, `duplicate field "c.b"`}, // multiple strict errors are returned
},
{
name: "unknown fields",
Expand Down

0 comments on commit be5ad23

Please sign in to comment.