Skip to content

Conversation

@meerumschlungen
Copy link

@meerumschlungen meerumschlungen commented Mar 10, 2025

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 before walk 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:

go test -cover ./jsonpath
--- FAIL: TestEval (0.00s)
    --- FAIL: TestEval/no-path-non-array (0.00s)
        jsonpath_test.go:406: Expected output length to be 15, but was 16
        jsonpath_test.go:409: Expected output to be {"key":"value"}, but was {"key": "value"}
    --- FAIL: TestEval/malformed-path-missing-close-bracket (0.00s)
        jsonpath_test.go:400: Expected error, got '<nil>'
        jsonpath_test.go:406: Expected output length to be 0, but was 2
        jsonpath_test.go:409: Expected output to be , but was [1 2]
    --- FAIL: TestEval/array-negative-index (0.00s)
panic: runtime error: index out of range [-1] [recovered]
        panic: runtime error: index out of range [-1]
    --- FAIL: TestEval/key-on-array (0.00s)
        jsonpath_test.go:400: Expected error, got '<nil>'
        jsonpath_test.go:406: Expected output length to be 0, but was 3
        jsonpath_test.go:409: Expected output to be , but was [1 2 3]
FAIL    github.com/TwiN/gatus/v5/jsonpath       0.220s
FAIL
  • TestEval/no-path-non-array: This is trivial and does not need treatment.
  • TestEval/malformed-path-missing-close-bracket: Invalid paths like [0 didn't fail but returned a result.
  • TestEval/array-negative-index: Negative indexes like data[-1] caused panic.
  • TestEval/key-on-array: Evaluating a path segment on an array (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

  • Tested and/or added tests to validate that the changes work as intended, if applicable.
  • Updated documentation in README.md, if applicable.

Copy link
Contributor

@ImTheCurse ImTheCurse left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just a bit of a nit pick, perhaps make the code a bit more modular - separate the different cases into helper functions

switch current.Type {
case tokenKey:
if obj, ok := value.(map[string]any); ok {
if nextVal, exists := obj[current.Value]; exists {
Copy link
Contributor

Choose a reason for hiding this comment

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

nextVal might be a bit confusing since where are getting the current value(even if it is a part of the tree that we are iterating over), I suggest something along the lines of currPathVal.
might be a nit pick but I think it could improve readability.

@dennisoderwald
Copy link

@meerumschlungen Are you still actively working on this? @TwiN Is it realistic to achieve a merge here in the near future? I think it would also help with my problem #1238.

@meerumschlungen
Copy link
Author

I was basically done with the implementation as far as I remember. I use the changed code in my personal fork. It got stale then.
@TwiN Are you interested in this contribution and are committed to perform the review? Then I would take the time to clean up, resolve conflicts, fix tests if necessary and implement review suggestions.
If you're not, please be honest, then let's close this. I gained some learnings in the design.

@meerumschlungen
Copy link
Author

@meerumschlungen Are you still actively working on this? @TwiN Is it realistic to achieve a merge here in the near future? I think it would also help with my problem #1238.

Without having tested this, I think that the reimplementation might solve your problem. I could create a unit test for this in this branch.

@TwiN
Copy link
Owner

TwiN commented Sep 2, 2025

Not against reviewing this, but I'm currently working on a large change, and I want to merge that first.

@dennisoderwald
Copy link

@TwiN Thanks so far. Is there an ETA for this large change? Just so @meerumschlungen can plan ahead, it would be helpful for him to know when to schedule it.

@joaojacome
Copy link

random idea: would it make sense to use yqlib to parse both json and yaml files?

https://pkg.go.dev/github.com/mikefarah/yq/[email protected]/pkg/yqlib

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.

5 participants