Skip to content

fix: prevent panic on nil intermediate values during Set operations #49

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 9, 2025

Conversation

mfleader
Copy link
Contributor

@mfleader mfleader commented Aug 5, 2025

Fix panic on nil intermediate values during Set operations

Problem

The library panics with reflect: call of reflect.Value.Type on zero Value when setting values through JSON paths containing nil intermediate values. This violates RFC 6901 Section 7, which requires implementations to raise error conditions for unresolvable paths.

Reproduction

data := map[string]any{
    "level1": map[string]any{
        "level2": map[string]any{
            "level3": nil, // Causes panic
        },
    },
}

ptr, _ := jsonpointer.New("/level1/level2/level3/value")
ptr.Set(data, "test-value") // PANIC before fix, ERROR after fix

Solution

Added isNil() checks in two locations:

  • setSingleImpl() (line 183): Before calling rValue.Type() on potentially zero reflect.Value
  • set() traversal loop (line 294): Before reflection operations during path traversal

Returns descriptive errors like "cannot set field X on nil value" instead of panicking.

RFC 6901 Compliance

Per RFC 6901 Section 7: implementations must "raise an error condition" for unresolvable paths rather than crash.

Testing

Added comprehensive test cases covering direct nil traversal, multi-level paths, and path creation scenarios. All tests pass with proper error handling.

Impact

  • No breaking changes - existing valid operations unchanged
  • Crashes become recoverable errors
  • Better compliance with JSON Pointer specification

Add nil checks in setSingleImpl() and traversal loop to handle nil
intermediate values gracefully. Per RFC 6901 Section 7, implementations
must raise error conditions for unresolvable paths instead of panicking.

- Add isNil() check before reflection operations in setSingleImpl()
- Add nil validation in set() method traversal loop
- Return descriptive errors: "cannot set field X on nil value"
- Add comprehensive test cases for nil traversal scenarios

Fixes panic: "reflect: call of reflect.Value.Type on zero Value" when
traversing JSON paths containing nil intermediate values.

Signed-off-by: Matthew F Leader <[email protected]>
@mfleader
Copy link
Contributor Author

mfleader commented Aug 6, 2025

@fredbi, would you have time to review my changes?

Copy link

codecov bot commented Aug 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.57%. Comparing base (76476c2) to head (0dd4df8).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
+ Coverage   69.97%   70.57%   +0.60%     
==========================================
  Files           2        2              
  Lines         343      350       +7     
==========================================
+ Hits          240      247       +7     
  Misses         78       78              
  Partials       25       25              
Flag Coverage Δ
oldstable-macos-latest 70.57% <100.00%> (+0.60%) ⬆️
oldstable-ubuntu-latest 70.57% <100.00%> (+0.60%) ⬆️
oldstable-windows-latest 70.57% <100.00%> (+0.60%) ⬆️
stable-macos-latest 70.57% <100.00%> (+0.60%) ⬆️
stable-ubuntu-latest 70.57% <100.00%> (+0.60%) ⬆️
stable-windows-latest 70.57% <100.00%> (+0.60%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@fredbi fredbi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! Thank you

@fredbi fredbi merged commit 2247499 into go-openapi:master Aug 9, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants