Skip to content

Commit 8a1cae1

Browse files
authored
Merge pull request #158 from fredbi/copilot/fix-nil-pointer-dereference
Copilot/fix nil pointer dereference
2 parents 2109391 + 280ae48 commit 8a1cae1

10 files changed

Lines changed: 696 additions & 64 deletions

File tree

‎.claude/CLAUDE.md‎

Lines changed: 49 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,39 +8,76 @@ Go implementation of [JSON Pointer (RFC 6901)](https://datatracker.ietf.org/doc/
88
and mutating JSON documents represented as Go values. Unlike most implementations, it works not only with
99
`map[string]any` and slices, but also with Go structs (resolved via `json` struct tags and reflection).
1010

11-
See [docs/MAINTAINERS.md](../docs/MAINTAINERS.md) for CI/CD, release process, and repo structure details.
11+
### Package layout
1212

13-
### Package layout (single package)
13+
Two packages in one module (`github.com/go-openapi/jsonpointer`):
1414

1515
| File | Contents |
1616
|------|----------|
17-
| `pointer.go` | Core types (`Pointer`, `JSONPointable`, `JSONSetable`), `New`, `Get`, `Set`, `Offset`, `Escape`/`Unescape` |
18-
| `errors.go` | Sentinel errors: `ErrPointer`, `ErrInvalidStart`, `ErrUnsupportedValueType` |
17+
| `pointer.go` | `Pointer`, `New`, `Get`, `Set`, `Offset`, `GetForToken`, `SetForToken`, `Escape`/`Unescape`, and the reflection helpers |
18+
| `ifaces.go` | `JSONPointable`, `JSONSetable`, `NameProvider` interfaces |
19+
| `options.go` | `Option`, `WithNameProvider`, `SetDefaultNameProvider`, `UseGoNameProvider`, `DefaultNameProvider` |
20+
| `errors.go` | Sentinel errors and their constructors |
21+
| `jsonname/name_provider.go` | `NameProvider` — the default, tag-only resolver (`DefaultJSONNameProvider`) |
22+
| `jsonname/go_name_provider.go` | `GoNameProvider` — the alternative resolver, follows `encoding/json` rules |
1923

2024
### Key API
2125

2226
- `New(string) (Pointer, error)` — parse a JSON pointer string (e.g. `"/foo/0/bar"`)
23-
- `Pointer.Get(document any) (any, reflect.Kind, error)` — retrieve a value
24-
- `Pointer.Set(document, value any) (any, error)` — set a value (document must be pointer/map/slice)
27+
- `Pointer.Get(document any, opts ...Option) (any, reflect.Kind, error)` — retrieve a value
28+
- `Pointer.Set(document, value any, opts ...Option) (any, error)` — set a value (document must be a pointer, map or slice)
2529
- `Pointer.Offset(jsonString string) (int64, error)` — byte offset of token in raw JSON
2630
- `GetForToken` / `SetForToken` — single-level convenience helpers
2731
- `Escape` / `Unescape` — RFC 6901 token escaping (`~0` ↔ `~`, `~1` ↔ `/`)
2832

2933
Custom types can implement `JSONPointable` (for Get) or `JSONSetable` (for Set) to bypass reflection.
3034

35+
Sentinel errors: `ErrPointer`, `ErrInvalidStart`, `ErrUnsupportedValueType`, `ErrDashToken`.
36+
Errors raised by the package wrap `ErrPointer`. Errors returned by a caller's `JSONLookup` or
37+
`JSONSet` are passed through unchanged, so `errors.Is(err, ErrPointer)` is false for those.
38+
39+
### Name providers
40+
41+
Two implementations of the `NameProvider` interface resolve go field names to json names. They
42+
differ, and the difference is load-bearing for most "why is my field unreachable" questions:
43+
44+
- `jsonname.NameProvider` (the default, `jsonname.DefaultJSONNameProvider`): only fields carrying a
45+
non-empty `json` tag are reachable.
46+
- `jsonname.GoNameProvider` (opt in with `UseGoNameProvider()` or `WithNameProvider`): follows
47+
`encoding/json`, so untagged exported fields are reachable under their go name.
48+
3149
### Dependencies
3250

33-
- `github.com/go-openapi/swag/jsonname` — struct tag → JSON field name resolution
3451
- `github.com/go-openapi/testify/v2` — test-only assertions
3552

36-
### Notable historical design decisions
53+
The module has no non-test dependency. `jsonname` is a subpackage of this repository, not the
54+
similarly named `github.com/go-openapi/swag/jsonname`.
3755

38-
See also .claude/plans/ROADMAP.md.
56+
### Notable design decisions
3957

40-
- Struct fields **must** have a `json` tag to be reachable; untagged fields are ignored
41-
(differs from `encoding/json` which defaults to the Go field name).
42-
- Anonymous embedded struct fields are traversed only if tagged.
58+
- Under the default provider, struct fields **must** have a `json` tag to be reachable; untagged
59+
fields are ignored (differs from `encoding/json`, which defaults to the go field name). Use
60+
`UseGoNameProvider()` for `encoding/json` semantics.
61+
- An anonymous (embedded) field is walked for the fields it promotes, and its own `json` tag is
62+
ignored — tagged or not, the tag never becomes a reachable key.
63+
- Embedding is traversed through a pointer (`*Base`). Resolving a promoted field on a value whose
64+
embedded pointer is nil reports an error; it does not allocate the intermediate struct.
65+
- An anonymous field that is not a struct (e.g. an embedded named slice) promotes nothing and is
66+
unreachable.
67+
- Maps must be keyed by a string type. A named string type is converted; `map[int]V` and friends
68+
cannot be addressed by a pointer and report an error.
69+
- `Set` with a nil value (what `encoding/json` decodes a JSON null into) writes the zero value when
70+
the target can hold nil — interface, pointer, map, slice, chan, func. On a target that cannot
71+
represent null (a `string` field, an element of `[]int`) it reports an error rather than
72+
substituting a zero value. Setting a map member to nil keeps the member; it does not delete it.
4373
- The RFC 6901 `"-"` array suffix is supported on `Pointer.Set` as an append
4474
operation (RFC 6902 convention). On `Pointer.Get` / `Pointer.Offset` it is
4575
always an error per RFC 6901 §4.
4676

77+
### Conventions
78+
79+
See `.claude/rules/` for contribution, linting, go and technical-writing conventions.
80+
Format with `golangci-lint fmt`, and check a change with `golangci-lint run --new-from-rev master`.
81+
82+
Tests run with `-race`. CI runs `{ubuntu, macos, windows} x {stable, oldstable}`, via the shared
83+
`go-openapi/ci-workflows/.github/workflows/go-test.yml`.

‎.claude/rules/contributions.md‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@ paths:
55

66
# Contribution rules (go-openapi)
77

8-
Read `.github/CONTRIBUTING.md` before opening a pull request.
8+
Read the go-openapi contribution guide before opening a pull request, plus the sibling rules in
9+
this directory: go conventions, linting, testing, github workflows and technical writing.
910

1011
## Commit hygiene
1112

@@ -41,12 +42,15 @@ go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest
4142
- Aim for at least 80% coverage of your patch.
4243
- Run the full test suite before submitting:
4344

44-
For mono-repos:
4545
```sh
46-
go test work ./...
46+
go test ./...
4747
```
4848

49-
For single module repos:
49+
In a mono-repo, run it once per module (a Go workspace does not give `go test` a whole-workspace
50+
target).
51+
52+
Run the race detector too, since CI does:
53+
5054
```sh
51-
go test ./...
55+
go test -race ./...
5256
```

‎.claude/rules/linting.md‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,17 @@ golangci-lint run
1010
```
1111

1212
Config: `.golangci.yml` — posture is `default: all` with explicit disables.
13-
See `docs/STYLE.md` for the rationale behind each disabled linter.
13+
Each disable is listed there under `linters.disable`.
14+
15+
Format with `golangci-lint fmt` (not `gofmt` or `gofumpt` directly — the config drives which
16+
formatters run).
17+
18+
Check a change against the base branch before pushing, so pre-existing reports in untouched code
19+
do not drown out your own:
20+
21+
```sh
22+
golangci-lint run --new-from-rev master
23+
```
1424

1525
Key rules:
1626
- Every `//nolint` directive **must** have an inline comment explaining why.

‎.github/copilot-instructions.md‎

Lines changed: 50 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,42 +6,81 @@ Go implementation of [JSON Pointer (RFC 6901)](https://datatracker.ietf.org/doc/
66
and mutating JSON documents represented as Go values. Works with `map[string]any`, slices, and Go structs
77
(resolved via `json` struct tags and reflection).
88

9-
## Package Layout (single package)
9+
## Package Layout
10+
11+
Two packages in one module (`github.com/go-openapi/jsonpointer`):
1012

1113
| File | Contents |
1214
|------|----------|
13-
| `pointer.go` | Core types (`Pointer`, `JSONPointable`, `JSONSetable`), `New`, `Get`, `Set`, `Offset`, `Escape`/`Unescape` |
14-
| `errors.go` | Sentinel errors: `ErrPointer`, `ErrInvalidStart`, `ErrUnsupportedValueType` |
15+
| `pointer.go` | `Pointer`, `New`, `Get`, `Set`, `Offset`, `GetForToken`, `SetForToken`, `Escape`/`Unescape`, and the reflection helpers |
16+
| `ifaces.go` | `JSONPointable`, `JSONSetable`, `NameProvider` interfaces |
17+
| `options.go` | `Option`, `WithNameProvider`, `SetDefaultNameProvider`, `UseGoNameProvider`, `DefaultNameProvider` |
18+
| `errors.go` | Sentinel errors and their constructors |
19+
| `jsonname/name_provider.go` | `NameProvider` — the default, tag-only resolver (`DefaultJSONNameProvider`) |
20+
| `jsonname/go_name_provider.go` | `GoNameProvider` — the alternative resolver, follows `encoding/json` rules |
1521

1622
## Key API
1723

1824
- `New(string) (Pointer, error)` — parse a JSON pointer string (e.g. `"/foo/0/bar"`)
19-
- `Pointer.Get(document any) (any, reflect.Kind, error)` — retrieve a value
20-
- `Pointer.Set(document, value any) (any, error)` — set a value (document must be pointer/map/slice)
25+
- `Pointer.Get(document any, opts ...Option) (any, reflect.Kind, error)` — retrieve a value
26+
- `Pointer.Set(document, value any, opts ...Option) (any, error)` — set a value (document must be a pointer, map or slice)
2127
- `Pointer.Offset(jsonString string) (int64, error)` — byte offset of token in raw JSON
2228
- `GetForToken` / `SetForToken` — single-level convenience helpers
2329
- `Escape` / `Unescape` — RFC 6901 token escaping (`~0` ↔ `~`, `~1` ↔ `/`)
2430

2531
Custom types can implement `JSONPointable` (for Get) or `JSONSetable` (for Set) to bypass reflection.
2632

33+
Sentinel errors: `ErrPointer`, `ErrInvalidStart`, `ErrUnsupportedValueType`, `ErrDashToken`.
34+
Errors raised by the package wrap `ErrPointer`. Errors returned by a caller's `JSONLookup` or
35+
`JSONSet` are passed through unchanged, so `errors.Is(err, ErrPointer)` is false for those.
36+
37+
## Name providers
38+
39+
Two implementations of the `NameProvider` interface resolve go field names to json names. The
40+
difference drives most "why is my field unreachable" questions:
41+
42+
- `jsonname.NameProvider` (the default, `jsonname.DefaultJSONNameProvider`): only fields carrying a
43+
non-empty `json` tag are reachable.
44+
- `jsonname.GoNameProvider` (opt in with `UseGoNameProvider()` or `WithNameProvider`): follows
45+
`encoding/json`, so untagged exported fields are reachable under their go name.
46+
2747
## Design Decisions
2848

29-
- Struct fields **must** have a `json` tag to be reachable; untagged fields are ignored.
30-
- Anonymous embedded struct fields are traversed only if tagged.
31-
- The RFC 6901 `"-"` array suffix (append) is **not** implemented.
49+
- Under the default provider, struct fields **must** have a `json` tag to be reachable; untagged
50+
fields are ignored. Use `UseGoNameProvider()` for `encoding/json` semantics.
51+
- An anonymous (embedded) field is walked for the fields it promotes, and its own `json` tag is
52+
ignored — tagged or not, the tag never becomes a reachable key.
53+
- Embedding is traversed through a pointer (`*Base`). Resolving a promoted field on a value whose
54+
embedded pointer is nil reports an error; it does not allocate the intermediate struct.
55+
- An anonymous field that is not a struct (e.g. an embedded named slice) promotes nothing and is
56+
unreachable.
57+
- Maps must be keyed by a string type. A named string type is converted; `map[int]V` and friends
58+
cannot be addressed by a pointer and report an error.
59+
- `Set` with a nil value (what `encoding/json` decodes a JSON null into) writes the zero value when
60+
the target can hold nil — interface, pointer, map, slice, chan, func. On a target that cannot
61+
represent null (a `string` field, an element of `[]int`) it reports an error rather than
62+
substituting a zero value. Setting a map member to nil keeps the member; it does not delete it.
63+
- The RFC 6901 `"-"` array suffix **is** supported on `Pointer.Set` as an append operation
64+
(RFC 6902 convention). On `Pointer.Get` and `Pointer.Offset` it is always an error per
65+
RFC 6901 §4. See `ErrDashToken`.
3266

3367
## Dependencies
3468

35-
- `github.com/go-openapi/swag/jsonname` — struct tag to JSON field name resolution
3669
- `github.com/go-openapi/testify/v2` — test-only assertions (zero-dep fork of `stretchr/testify`)
3770

71+
The module has no non-test dependency. `jsonname` is a subpackage of this repository, not the
72+
similarly named `github.com/go-openapi/swag/jsonname`.
73+
3874
## Conventions
3975

4076
- All `.go` files must have SPDX license headers (Apache-2.0).
4177
- Commits require DCO sign-off (`git commit -s`).
42-
- Linting: `golangci-lint run` — config in `.golangci.yml` (posture: `default: all` with explicit disables).
78+
- Linting: `golangci-lint run` — config in `.golangci.yml` (posture: `default: all` with explicit
79+
disables). Format with `golangci-lint fmt`, and check a change with
80+
`golangci-lint run --new-from-rev master`.
4381
- Every `//nolint` directive **must** have an inline comment explaining why.
44-
- Tests: `go test ./...` with `-race`. CI runs on `{ubuntu, macos, windows} x {stable, oldstable}`.
82+
- Tests: `go test ./...` with `-race`. CI runs `{ubuntu, macos, windows} x {stable, oldstable}`,
83+
via the shared `go-openapi/ci-workflows/.github/workflows/go-test.yml`.
4584
- Test framework: `github.com/go-openapi/testify/v2` (not `stretchr/testify`).
4685

4786
See `.github/copilot/` (symlinked to `.claude/rules/`) for detailed rules on Go conventions, linting, testing, and contributions.

‎errors.go‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,10 @@
33

44
package jsonpointer
55

6-
import "fmt"
6+
import (
7+
"fmt"
8+
"reflect"
9+
)
710

811
type pointerError string
912

@@ -47,6 +50,24 @@ func errInvalidReference(token string) error {
4750
return fmt.Errorf("invalid token reference %q: %w", token, ErrPointer)
4851
}
4952

53+
func errNoField(token string) error {
54+
return fmt.Errorf("object has no field %q: %w", token, ErrPointer)
55+
}
56+
57+
// errUnreachableField reports a field that exists on the type but cannot be reached on this value,
58+
// because it is promoted through a nil embedded pointer.
59+
func errUnreachableField(name string, cause error) error {
60+
return fmt.Errorf("cannot reach field %q: %w: %w", name, cause, ErrPointer)
61+
}
62+
63+
// errMapKey reports a reference token that cannot serve as a key of the target map type.
64+
//
65+
// JSON object member names are strings, so only maps keyed by a string type (or by an interface
66+
// type a string satisfies) can be addressed by a JSON pointer.
67+
func errMapKey(token string, mapType reflect.Type) error {
68+
return fmt.Errorf("can't use token %q as a key of map type %v: %w", token, mapType, ErrPointer)
69+
}
70+
5071
func errDashOnGet() error {
5172
return fmt.Errorf("cannot resolve %q token on get: %w: %w", dashToken, ErrDashToken, ErrPointer)
5273
}

‎jsonname/go_name_provider.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ func (n *GoNameProvider) GetJSONNames(subject any) []string {
4646
n.lock.Lock()
4747
defer n.lock.Unlock()
4848

49-
tpe := reflect.Indirect(reflect.ValueOf(subject)).Type()
49+
tpe := typeOfSubject(subject)
5050
names := n.nameIndexFor(tpe)
5151

5252
res := make([]string, 0, len(names.jsonNames))
@@ -59,7 +59,7 @@ func (n *GoNameProvider) GetJSONNames(subject any) []string {
5959

6060
// GetJSONName gets the json name for a go property name.
6161
func (n *GoNameProvider) GetJSONName(subject any, name string) (string, bool) {
62-
tpe := reflect.Indirect(reflect.ValueOf(subject)).Type()
62+
tpe := typeOfSubject(subject)
6363

6464
return n.GetJSONNameForType(tpe, name)
6565
}
@@ -77,7 +77,7 @@ func (n *GoNameProvider) GetJSONNameForType(tpe reflect.Type, name string) (stri
7777

7878
// GetGoName gets the go name for a json property name.
7979
func (n *GoNameProvider) GetGoName(subject any, name string) (string, bool) {
80-
tpe := reflect.Indirect(reflect.ValueOf(subject)).Type()
80+
tpe := typeOfSubject(subject)
8181

8282
return n.GetGoNameForType(tpe, name)
8383
}
@@ -132,7 +132,7 @@ func buildGoNameIndex(tpe reflect.Type) nameIndex {
132132
//
133133
//nolint:gocognit // everything is inlined to help the compiler determine what escapes and what doesn't
134134
func collectGoFields(tpe reflect.Type) []fieldEntry {
135-
if tpe.Kind() != reflect.Struct {
135+
if tpe == nil || tpe.Kind() != reflect.Struct {
136136
return nil
137137
}
138138

‎jsonname/name_provider.go‎

Lines changed: 49 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,13 @@ func buildnameIndex(tpe reflect.Type, idx, reverseIdx map[string]string) {
4747
}
4848

4949
if targetDes.Anonymous { // walk embedded structures tree down first
50-
buildnameIndex(targetDes.Type, idx, reverseIdx)
50+
// An embedded field is not necessarily a struct: it may be a pointer to a struct, or a named
51+
// non-struct type (e.g. an embedded named slice or map). Only struct shapes carry promoted
52+
// fields worth indexing; anything else contributes no json name and must not be walked.
53+
if embedded := structTypeOf(targetDes.Type); embedded != nil {
54+
buildnameIndex(embedded, idx, reverseIdx)
55+
}
56+
5157
continue
5258
}
5359

@@ -73,18 +79,57 @@ func buildnameIndex(tpe reflect.Type, idx, reverseIdx map[string]string) {
7379
}
7480

7581
func newNameIndex(tpe reflect.Type) nameIndex {
82+
tpe = structTypeOf(tpe)
83+
if tpe == nil {
84+
// only struct shapes carry json names: anything else indexes to nothing.
85+
return nameIndex{jsonNames: map[string]string{}, goNames: map[string]string{}}
86+
}
87+
7688
idx := make(map[string]string, tpe.NumField())
7789
reverseIdx := make(map[string]string, tpe.NumField())
7890

7991
buildnameIndex(tpe, idx, reverseIdx)
8092
return nameIndex{jsonNames: idx, goNames: reverseIdx}
8193
}
8294

95+
// structTypeOf reduces tpe to the struct type it designates, dereferencing a pointer type.
96+
//
97+
// It returns nil when tpe is nil or does not designate a struct, so that callers may treat
98+
// "nothing to index here" uniformly instead of panicking in [reflect.Type.NumField].
99+
func structTypeOf(tpe reflect.Type) reflect.Type {
100+
if tpe == nil {
101+
return nil
102+
}
103+
104+
if tpe.Kind() == reflect.Pointer {
105+
tpe = tpe.Elem()
106+
}
107+
108+
if tpe.Kind() != reflect.Struct {
109+
return nil
110+
}
111+
112+
return tpe
113+
}
114+
115+
// typeOfSubject returns the type of a document subject, dereferencing pointers.
116+
//
117+
// It returns nil for an untyped nil subject, which resolves to an empty name index rather than
118+
// panicking in [reflect.Value.Type].
119+
func typeOfSubject(subject any) reflect.Type {
120+
rValue := reflect.Indirect(reflect.ValueOf(subject))
121+
if !rValue.IsValid() {
122+
return nil
123+
}
124+
125+
return rValue.Type()
126+
}
127+
83128
// GetJSONNames gets all the json property names for a type.
84129
func (n *NameProvider) GetJSONNames(subject any) []string {
85130
n.lock.Lock()
86131
defer n.lock.Unlock()
87-
tpe := reflect.Indirect(reflect.ValueOf(subject)).Type()
132+
tpe := typeOfSubject(subject)
88133
names, ok := n.index[tpe]
89134
if !ok {
90135
names = n.makeNameIndex(tpe)
@@ -99,7 +144,7 @@ func (n *NameProvider) GetJSONNames(subject any) []string {
99144

100145
// GetJSONName gets the json name for a go property name.
101146
func (n *NameProvider) GetJSONName(subject any, name string) (string, bool) {
102-
tpe := reflect.Indirect(reflect.ValueOf(subject)).Type()
147+
tpe := typeOfSubject(subject)
103148
return n.GetJSONNameForType(tpe, name)
104149
}
105150

@@ -117,7 +162,7 @@ func (n *NameProvider) GetJSONNameForType(tpe reflect.Type, name string) (string
117162

118163
// GetGoName gets the go name for a json property name.
119164
func (n *NameProvider) GetGoName(subject any, name string) (string, bool) {
120-
tpe := reflect.Indirect(reflect.ValueOf(subject)).Type()
165+
tpe := typeOfSubject(subject)
121166
return n.GetGoNameForType(tpe, name)
122167
}
123168

0 commit comments

Comments
 (0)