Skip to content

Commit 61efbb7

Browse files
fredbifredbi
committed
test: added tests for edge cases
* fixed panic case when attempting to set a non-assignable value * clarified example and limitation regarding embedded structs Signed-off-by: Frederic BIDON <[email protected]>
1 parent 3f0fe76 commit 61efbb7

File tree

3 files changed

+263
-17
lines changed

3 files changed

+263
-17
lines changed

‎pointer.go‎

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ type JSONSetable interface {
4545
// - a go map[K]V is interpreted as an object, with type K assignable to a string
4646
// - a go slice []T is interpreted as an array
4747
// - a go struct is interpreted as an object, with exported fields interpreted as keys
48+
// - promoted fields from an embedded struct are traversed
4849
// - scalars (e.g. int, float64 ...), channels, functions and go arrays cannot be traversed
4950
//
5051
// For struct s resolved by reflection, key mappings honor the conventional struct tag `json`.
@@ -54,7 +55,7 @@ type JSONSetable interface {
5455
// # Limitations
5556
//
5657
// - Unlike go standard marshaling, untagged fields do not default to the go field name and are ignored.
57-
// - anonymous embedded fields are not traversed
58+
// - anonymous fields are not traversed if untagged
5859
type Pointer struct {
5960
referenceTokens []string
6061
}
@@ -362,7 +363,7 @@ func getSingleImpl(node any, decodedToken string, nameProvider *jsonname.NamePro
362363
case reflect.Slice:
363364
tokenIndex, err := strconv.Atoi(decodedToken)
364365
if err != nil {
365-
return nil, kind, err
366+
return nil, kind, errors.Join(err, ErrPointer)
366367
}
367368
sLength := rValue.Len()
368369
if tokenIndex < 0 || tokenIndex >= sLength {
@@ -378,9 +379,7 @@ func getSingleImpl(node any, decodedToken string, nameProvider *jsonname.NamePro
378379
}
379380

380381
func setSingleImpl(node, data any, decodedToken string, nameProvider *jsonname.NameProvider) error {
381-
rValue := reflect.Indirect(reflect.ValueOf(node))
382-
383-
// Check for nil to prevent panic when calling rValue.Type()
382+
// check for nil to prevent panic when calling rValue.Type()
384383
if isNil(node) {
385384
return fmt.Errorf("cannot set field %q on nil value: %w", decodedToken, ErrPointer)
386385
}
@@ -389,28 +388,44 @@ func setSingleImpl(node, data any, decodedToken string, nameProvider *jsonname.N
389388
return ns.JSONSet(decodedToken, data)
390389
}
391390

391+
rValue := reflect.Indirect(reflect.ValueOf(node))
392+
392393
switch rValue.Kind() {
393394
case reflect.Struct:
394395
nm, ok := nameProvider.GetGoNameForType(rValue.Type(), decodedToken)
395396
if !ok {
396397
return fmt.Errorf("object has no field %q: %w", decodedToken, ErrPointer)
397398
}
399+
398400
fld := rValue.FieldByName(nm)
399-
if fld.IsValid() {
400-
fld.Set(reflect.ValueOf(data))
401+
if !fld.CanSet() {
402+
return fmt.Errorf("can't set struct field %s to %v: %w", nm, data, ErrPointer)
401403
}
404+
405+
value := reflect.ValueOf(data)
406+
valueType := value.Type()
407+
assignedType := fld.Type()
408+
409+
if !valueType.AssignableTo(assignedType) {
410+
return fmt.Errorf("can't set value with type %T to field %s with type %v: %w", data, nm, assignedType, ErrPointer)
411+
}
412+
413+
fld.Set(value)
414+
402415
return nil
403416

404417
case reflect.Map:
405418
kv := reflect.ValueOf(decodedToken)
406419
rValue.SetMapIndex(kv, reflect.ValueOf(data))
420+
407421
return nil
408422

409423
case reflect.Slice:
410424
tokenIndex, err := strconv.Atoi(decodedToken)
411425
if err != nil {
412-
return err
426+
return errors.Join(err, ErrPointer)
413427
}
428+
414429
sLength := rValue.Len()
415430
if tokenIndex < 0 || tokenIndex >= sLength {
416431
return errOutOfBounds(sLength, tokenIndex)
@@ -420,7 +435,17 @@ func setSingleImpl(node, data any, decodedToken string, nameProvider *jsonname.N
420435
if !elem.CanSet() {
421436
return fmt.Errorf("can't set slice index %s to %v: %w", decodedToken, data, ErrPointer)
422437
}
423-
elem.Set(reflect.ValueOf(data))
438+
439+
value := reflect.ValueOf(data)
440+
valueType := value.Type()
441+
assignedType := elem.Type()
442+
443+
if !valueType.AssignableTo(assignedType) {
444+
return fmt.Errorf("can't set value with type %T to slice element %d with type %v: %w", data, tokenIndex, assignedType, ErrPointer)
445+
}
446+
447+
elem.Set(value)
448+
424449
return nil
425450

426451
default:

‎pointer_test.go‎

Lines changed: 203 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,11 @@ package jsonpointer
66
import (
77
"encoding/json"
88
"fmt"
9+
"reflect"
910
"strconv"
1011
"testing"
1112

13+
"github.com/go-openapi/swag/jsonname"
1214
"github.com/go-openapi/testify/v2/assert"
1315
"github.com/go-openapi/testify/v2/require"
1416
)
@@ -246,6 +248,7 @@ func TestPointableInterface(t *testing.T) {
246248

247249
t.Run("with pointable type", func(t *testing.T) {
248250
p := &pointableImpl{"hello"}
251+
249252
result, _, err := GetForToken(p, "some")
250253
require.NoError(t, err)
251254
assert.Equal(t, p.a, result)
@@ -346,6 +349,95 @@ func TestArray(t *testing.T) {
346349
}
347350
}
348351

352+
func TestStruct(t *testing.T) {
353+
t.Parallel()
354+
355+
t.Run("with untagged struct field", func(t *testing.T) {
356+
type Embedded struct {
357+
D int `json:"d"`
358+
}
359+
360+
s := struct {
361+
Embedded
362+
363+
A int `json:"a"`
364+
B int
365+
Anonymous struct {
366+
C int `json:"c"`
367+
}
368+
}{}
369+
370+
{
371+
s.A = 1
372+
s.B = 2
373+
s.Anonymous.C = 3
374+
s.D = 4
375+
}
376+
377+
t.Run(`should resolve field A tagged "a"`, func(t *testing.T) {
378+
pointerA, err := New("/a")
379+
require.NoError(t, err)
380+
381+
value, kind, err := pointerA.Get(s)
382+
require.NoError(t, err)
383+
require.Equal(t, reflect.Int, kind)
384+
require.Equal(t, 1, value)
385+
386+
_, err = pointerA.Set(&s, 9)
387+
require.NoError(t, err)
388+
389+
value, _, err = pointerA.Get(s)
390+
require.NoError(t, err)
391+
require.Equal(t, 9, value)
392+
})
393+
394+
t.Run(`should resolve embedded field D with tag`, func(t *testing.T) {
395+
pointerD, err := New("/d")
396+
require.NoError(t, err)
397+
398+
value, kind, err := pointerD.Get(s)
399+
require.NoError(t, err)
400+
require.Equal(t, reflect.Int, kind)
401+
require.Equal(t, 4, value)
402+
403+
_, err = pointerD.Set(&s, 6)
404+
require.NoError(t, err)
405+
406+
value, _, err = pointerD.Get(s)
407+
require.NoError(t, err)
408+
require.Equal(t, 6, value)
409+
})
410+
411+
t.Run("with known limitations", func(t *testing.T) {
412+
t.Run(`should not resolve field B without tag`, func(t *testing.T) {
413+
pointerB, err := New("/B")
414+
require.NoError(t, err)
415+
416+
_, _, err = pointerB.Get(s)
417+
require.Error(t, err)
418+
require.ErrorContains(t, err, `has no field "B"`)
419+
420+
_, err = pointerB.Set(&s, 8)
421+
require.Error(t, err)
422+
require.ErrorContains(t, err, `has no field "B"`)
423+
})
424+
425+
t.Run(`should not resolve field C with tag, but anonymous`, func(t *testing.T) {
426+
pointerC, err := New("/c")
427+
require.NoError(t, err)
428+
429+
_, _, err = pointerC.Get(s)
430+
require.Error(t, err)
431+
require.ErrorContains(t, err, `has no field "c"`)
432+
433+
_, err = pointerC.Set(&s, 7)
434+
require.Error(t, err)
435+
require.ErrorContains(t, err, `has no field "c"`)
436+
})
437+
})
438+
})
439+
}
440+
349441
func TestOtherThings(t *testing.T) {
350442
t.Parallel()
351443

@@ -367,11 +459,21 @@ func TestOtherThings(t *testing.T) {
367459
})
368460

369461
t.Run("out of bound array index should error", func(t *testing.T) {
370-
p, err := New("/foo/3")
371-
require.NoError(t, err)
462+
t.Run("with index overflow", func(t *testing.T) {
463+
p, err := New("/foo/3")
464+
require.NoError(t, err)
372465

373-
_, _, err = p.Get(testDocumentJSON(t))
374-
require.Error(t, err)
466+
_, _, err = p.Get(testDocumentJSON(t))
467+
require.Error(t, err)
468+
})
469+
470+
t.Run("with index unerflow", func(t *testing.T) {
471+
p, err := New("/foo/-3")
472+
require.NoError(t, err)
473+
474+
_, _, err = p.Get(testDocumentJSON(t))
475+
require.Error(t, err)
476+
})
375477
})
376478

377479
t.Run("referring to a key in an array should error", func(t *testing.T) {
@@ -907,4 +1009,101 @@ func TestEdgeCases(t *testing.T) {
9071009

9081010
require.Equal(t, doc, newDoc)
9091011
})
1012+
1013+
t.Run("with out of bounds index", func(t *testing.T) {
1014+
p, err := New("/foo/10")
1015+
require.NoError(t, err)
1016+
1017+
t.Run("should error on Get", func(t *testing.T) {
1018+
_, _, err := p.Get(testStructJSONDoc(t))
1019+
require.Error(t, err)
1020+
require.ErrorContains(t, err, "index out of bounds")
1021+
})
1022+
1023+
t.Run("should error on Set", func(t *testing.T) {
1024+
_, err := p.Set(testStructJSONPtr(t), "peek-a-boo")
1025+
require.Error(t, err)
1026+
require.ErrorContains(t, err, "index out of bounds")
1027+
})
1028+
})
1029+
1030+
t.Run("Set with invalid pointer token", func(t *testing.T) {
1031+
doc := testStructJSONDoc(t)
1032+
pointer, err := New("/foo/x")
1033+
require.NoError(t, err)
1034+
1035+
_, err = pointer.Set(&doc, "yay")
1036+
require.Error(t, err)
1037+
require.ErrorContains(t, err, `Atoi: parsing "x"`)
1038+
})
1039+
1040+
t.Run("Set with invalid reference in struct", func(t *testing.T) {
1041+
doc := struct {
1042+
A func() `json:"a"`
1043+
B []int `json:"b"`
1044+
}{
1045+
A: func() {},
1046+
B: []int{0, 1},
1047+
}
1048+
1049+
t.Run("should error when attempting to set a struct field value that is not assignable", func(t *testing.T) {
1050+
pointerA, err := New("/a")
1051+
require.NoError(t, err)
1052+
1053+
_, err = pointerA.Set(&doc, "waou")
1054+
require.Error(t, err)
1055+
require.ErrorContains(t, err, `can't set value with type string to field A`)
1056+
})
1057+
1058+
t.Run("should error when attempting to set a slice element value that is not assignable", func(t *testing.T) {
1059+
pointerB, err := New("/b/0")
1060+
require.NoError(t, err)
1061+
1062+
_, err = pointerB.Set(&doc, "waou")
1063+
require.Error(t, err)
1064+
require.ErrorContains(t, err, `can't set value with type string to slice element 0 with type int`)
1065+
})
1066+
1067+
t.Run("should error when attempting to set a value that does not exist", func(t *testing.T) {
1068+
pointerB, err := New("/x")
1069+
require.NoError(t, err)
1070+
1071+
_, _, err = pointerB.Get(&doc)
1072+
require.Error(t, err)
1073+
require.ErrorContains(t, err, `no field`)
1074+
1075+
_, err = pointerB.Set(&doc, "oops")
1076+
require.Error(t, err)
1077+
require.ErrorContains(t, err, `no field`)
1078+
})
1079+
})
1080+
}
1081+
1082+
func TestInternalEdgeCases(t *testing.T) {
1083+
t.Parallel()
1084+
1085+
t.Run("setSingleImpl should error on any node not a struct, map or slice", func(t *testing.T) {
1086+
var node int
1087+
1088+
err := setSingleImpl(&node, 3, "a", jsonname.DefaultJSONNameProvider)
1089+
require.Error(t, err)
1090+
require.ErrorContains(t, err, `invalid token reference "a"`)
1091+
})
1092+
1093+
t.Run("with simulated unsettable", func(t *testing.T) {
1094+
type unsettable struct {
1095+
A string `json:"a"`
1096+
}
1097+
doc := unsettable{
1098+
A: "a",
1099+
}
1100+
1101+
t.Run("setSingleImpl should error on struct field that is not settable", func(t *testing.T) {
1102+
node := doc // doesn't pass a pointer: unsettable
1103+
1104+
err := setSingleImpl(node, "new value", "a", jsonname.DefaultJSONNameProvider)
1105+
require.Error(t, err)
1106+
require.ErrorContains(t, err, `can't set struct field`)
1107+
})
1108+
})
9101109
}

0 commit comments

Comments
 (0)