chore(jsonpath): Improve jsonpath package #1024
Draft
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.
Summary
#1022
This PR is a complete rewrite of the jsonpath package. It optimizes several aspects of the current implementation such as:
Test driven approach
To ensure this reimplementation does not break backwards compatibility, I focused on redesigning the current behavior exactly. To accomplish this I first maxed out test coverage for the current implementation to 100%. After the reimplementation I made sure the reimplementation passed all tests that existed up to that point. Then I maxed out test coverage for the reimplementation to 91.8%. 100% coverage can't be achieved with the new version because the uncovered lines handle types JSON can't represent (i.e. default branches in type switching), making lines effectively unreachable with valid JSON input or because
tokenize
rejects malformed paths beforewalk
is called and the tests can't bypass this.To ensure backwards compatibility, I ran the new test suite against the old implementation. Some tweaks were needed in the new implementation to ensure the same behavior but this also revealed some irregularities in the old implementation, namely:
[0
didn't fail but returned a result.data[-1]
caused panic.list.data
for{"list":[1,2,3]}
) didn't fail but returned a result.So the new implementation also improves robustness and fixes some edge-case bugs.
Benchmarks
Old implementation:
BenchmarkEval-8 304356 3809 ns/op 4761 B/op 89 allocs/op
New implementation:
BenchmarkEval-8 353466 3224 ns/op 5544 B/op 77 allocs/op
Performance improvement: The new implementation is faster (15.4% lower latency, 16.2% higher throughput).
Memory trade-off: It uses 16.4% more memory per operation but with 13.5% fewer allocations.
The new version prioritizes speed and allocation efficiency over memory footprint, which I think is a good tradeoff for this use case.
Checklist
Updated documentation inREADME.md
, if applicable.