Improve JSON decode error messages with path context #4288
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit improves error messages when JSON decoding fails by adding context about where in the JSON structure the error occurred. Changes: - Added pathContext field to Decoder to track the current location - Added path management helper methods (pushPath, popPath, getPathString) - Added *WithContext versions of jsonObject methods that use path context - Updated key decode methods to track and use path context For example, instead of: missing property: type Users now see: missing property: type (at array[0]) missing property: type (at field:myField.type) Closes #4209
devin-ai-integration
bot
requested review from
RZhang05,
SupunS and
turbolent
as code owners
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Benchstat comparison
Results
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Keeping a path makes sense. However, keep the path as "elements" instead of strings. This avoids string formatting / allocation during the happy path. Instead of type pathElement interface {
Append(io.Writer)
}then for example instead of d.pushPath("array[" + strconv.Itoa(i) + "]")d.pushPath(indexPathElement(i))and type indexPathElement int
var _ pathElement = indexPathElement(0)
func (e indexPathElement) Append(w io.Writer) {
_, _ = fmt.Fprintf("array:%d", int(e))
}Also, in |
Follow-up to address code review feedback from @turbolent. Changes: - Introduced pathElement interface with Append(io.Writer) method - Created specific path element types: - indexPathElement: for array indices - fieldPathElement: for struct/resource/event field names - staticPathElement: for static strings like 'type' - Updated getPathString() to use strings.Builder for efficient concatenation - Changed pathContext from []string to []pathElement This optimization avoids string formatting and allocation during the happy path (successful decoding), only building the path string when an error occurs. Addresses: #4288 (comment)
|
Thanks for the feedback! I've optimized the implementation to avoid string allocations during the happy path. Changes made:
This way, path construction only happens when an error occurs, not during successful decoding. |
|
Add tests |
Added comprehensive tests to verify that path context is correctly included in error messages when JSON decoding fails. Test cases cover: - Missing property at root level - Missing property in array elements - Missing property in nested array elements - Missing property in struct fields - Missing property in type definitions - Missing property in complex nested structures (array[0].field:nested) All tests verify that the error message contains both the property name and the path context showing where in the JSON structure the error occurred.
|
Added comprehensive tests for the error context feature in The tests cover various error scenarios:
All tests verify that error messages correctly include the path context. |
|
@bjartek How does this look? Would this be helpful? Does the path format look useful? |
|
Yes wonderful! |
|
Refactor all decode methods like this (dictionary, parameters, etc.) to use the context versions. |
Comprehensive refactoring to consistently use GetWithContext, GetStringWithContext, GetSliceWithContext, GetBoolWithContext, and GetValueWithContext throughout all decode methods. This ensures path context is available for all decoding errors, not just in a few select locations. Now any missing property error will include context about where in the JSON structure the error occurred. Updated methods: - decodeKeyValuePair - decodeComposite - decodeInclusiveRange - decodePath - decodeFunction - decodeTypeParameter - decodeParameter - decodeFieldType - decodeAuthorization - decodeNominalType - decodeType - decodeTypeValue - decodeCapability Addresses: #4288 (review)
|
Refactored all decode methods to consistently use the All This ensures that any missing property error in any decode method will include path context showing where in the JSON structure the error occurred. All existing tests pass with the changes. |
turbolent
reviewed
Oct 15, 2025
- Add path tracking system using pathElement interface - Implement indexPathElement, fieldPathElement, staticPathElement - Refactor all decode methods to use *WithContext helpers - Remove non-context Get* helper methods - Add comprehensive error context tests Closes #4209
6 tasks
|
Replaced by #4302 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #4209
Description
This PR improves JSON decode error messages by adding path context showing where in the JSON structure errors occur.
Before:
After:
Implementation
Added a path tracking system using a
pathElementinterface to avoid string allocation during successful decoding:pathContext []pathElementfield on Decoder withpushPath()/popPath()methodsindexPathElement(array indices),fieldPathElement(struct fields),staticPathElement(static strings like "type")strings.Builderwhen error occurs*WithContextversions of alljsonObjecthelper methods and migrated decode methods to use themFiles to Review
Critical:
encoding/json/decode.go:38-66: Path element interface and implementationsencoding/json/decode.go:195-217: Path management methodsencoding/json/decode.go:1536-1558: GetWithContext error handlingencoding/json/decode.go:850-861: Example of path tracking indecodeCompositeFieldTesting:
encoding/json/encoding_test.go:4027-4142: Comprehensive error context testsKnown Issues
decodeAuthorizationmethod still uses oldGet()on nested entitlement objects (lines 1150, 1156, 1164). These specific errors won't include path context. Reviewer should verify if this is acceptable or needs fixing.Performance Considerations
Path tracking adds overhead to every decode operation (pushPath/popPath calls), but string building only occurs on error. Pre-allocated slice capacity of 8 minimizes reallocations for typical JSON depths.
Link to Devin run: https://app.devin.ai/sessions/5e6d0a0c3c03459d8c6aaa8d5feae034
Requested by: [email protected]
masterbranchFiles changedin the Github PR explorer