Switch from jsonpath_lib to jsonpath-rust
#1345
Conversation
ilya-bobyr
force-pushed
the
pr/switch-to-jsonpath-rust
branch
2 times, most recently
from
49a7d68 to
19d03cb
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1345 +/- ##
=======================================
- Coverage 72.1% 72.1% -0.0%
=======================================
Files 75 75
Lines 6387 6390 +3
=======================================
+ Hits 4604 4606 +2
- Misses 1783 1784 +1
|
ilya-bobyr
force-pushed
the
pr/switch-to-jsonpath-rust
branch
from
19d03cb to
589e7b0
Compare
There was a problem hiding this comment.
Just some small format! nits other than that LGTM 👍
Out of curiosity have you looked into https://docs.rs/serde_json_path/latest/serde_json_path/ - I have used it before and quite liked it.
| let res = parsed_path.find_slice(json); | ||
|
|
||
| let Some(res) = res.into_iter().next() else { | ||
| return Err(Error::AuthExec(format!( | ||
| "Target {context:?} value {path:?} not found" | ||
| ))); | ||
| }; |
There was a problem hiding this comment.
will it always work to use find_slice here? to me that sounds like we expect the query to return a vector of things, but curious if this will work if we do a query returning a single item like '{.items[3].metadata.name}'
There was a problem hiding this comment.
ya so find_slice will return an empty vector if the query doesn't match and will return anything else as elements in an array, including if it results in just a single item. The item will just be returned in the vector. See: https://github.com/besok/jsonpath-rust/blob/main/src/lib.rs#L450-L460.
Test here shows find_slice() returning a single item in a vector: https://github.com/besok/jsonpath-rust/blob/main/src/lib.rs#L1194-L1198
There was a problem hiding this comment.
Existing logic here is a bit suspicious.
If any elements are selected by the JSONPath, it would always use the first element, even if there is more than one result.
I wonder if it would be better to fail with an error, if there are multiple results.
If someone really wants to use the first result, they can always clarify their selector expression.
On the other hand, choosing the first result could hide an error and complicate debugging.
I can send a separate PR to change this behavior.
There was a problem hiding this comment.
I guess this is ultimately only for a small amount of niche behaviour; local auth where the usage from exec based providers needs to grab one key - so it's not as exposed as something like kubectl jsonpath querying. If there's a clear way to improve it feel free to raise a PR, but I don't see this as a big problem atm.
`jsonpath_lib` is unmaintained, which is not ideal. It is also forcing everyone to enable a `preserve_order` feature in the `serde_json` dependency. `preserve_order` is not following the proper feature model, where features are only adding functionality. It is actually changing how `serde_json` works, switching from `BTreeMap` to `IndexMap`. Signed-off-by: Illia Bobyr <[email protected]>
ilya-bobyr
force-pushed
the
pr/switch-to-jsonpath-rust
branch
from
45c8558 to
b4f0002
Compare
|
Replaced format!("{}{}", $, path.clone())with format!("${path}")and rebased on top of the latest |
It seems that both libraries can work directly with I have not used any Rust JSONPath libraries much. And the scope of this PR is that we have a problem with the outdated dependency, that is braking our dependency tree. It is not that I prefer one library over another :) |
needed for #1345 Signed-off-by: clux <[email protected]>
|
Given how little usage there is of the Btw, this needs an MSRV bump to pass CI which is perfectly reasonable for us to do at this stage. Have done this in #1353 |
needed for #1345 Signed-off-by: clux <[email protected]>
|
I think this is the right choice, was just (personally) curious if you explored more alternatives 👍 . |
Motivation
jsonpath_libis unmaintained, which is not ideal. It is also forcing everyone to enable apreserve_orderfeature in theserde_jsondependency.preserve_orderis not following the proper feature model, where features are only adding functionality. It is actually changing howserde_jsonworks, switching fromBTreeMaptoIndexMap.Solution
Switch from
jsonpath_libtojsonpath-rust.Change has been tested with both unit and integration tests.
Based on gregcusack#1