Skip to content
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

Switch from jsonpath_lib to jsonpath-rust #1345

Merged
merged 2 commits into from
Nov 23, 2023

Conversation

ilya-bobyr
Copy link
Contributor

Motivation

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.

Solution

Switch from jsonpath_lib to jsonpath-rust.

Change has been tested with both unit and integration tests.


Based on gregcusack#1

@ilya-bobyr ilya-bobyr force-pushed the pr/switch-to-jsonpath-rust branch 2 times, most recently from 49a7d68 to 19d03cb Compare November 16, 2023 12:13
@clux clux added the changelog-change changelog change category for prs label Nov 17, 2023
Copy link

codecov bot commented Nov 17, 2023

Codecov Report

Merging #1345 (4ab6e54) into main (dde4fc7) will decrease coverage by 0.0%.
The diff coverage is 71.5%.

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     
Files Coverage Δ
kube-client/src/client/auth/mod.rs 50.7% <71.5%> (+0.3%) ⬆️

Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

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

I did not test it / look for functional changes

examples/dynamic_jsonpath.rs Outdated Show resolved Hide resolved
kube-client/src/client/auth/mod.rs Outdated Show resolved Hide resolved
@ilya-bobyr ilya-bobyr force-pushed the pr/switch-to-jsonpath-rust branch from 19d03cb to 589e7b0 Compare November 17, 2023 10:43
@ilya-bobyr ilya-bobyr requested a review from Dav1dde November 17, 2023 10:59
Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

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

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.

examples/dynamic_jsonpath.rs Outdated Show resolved Hide resolved
kube-client/src/client/auth/mod.rs Show resolved Hide resolved
@clux clux added this to the 0.88.0 milestone Nov 17, 2023
Comment on lines +488 to +494
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"
)));
};
Copy link
Member

Choose a reason for hiding this comment

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

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}'

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 <illia.bobyr@gmail.com>
@ilya-bobyr ilya-bobyr force-pushed the pr/switch-to-jsonpath-rust branch from 45c8558 to b4f0002 Compare November 21, 2023 09:32
@ilya-bobyr
Copy link
Contributor Author

Replaced

    format!("{}{}", $, path.clone())

with

    format!("${path}")

and rebased on top of the latest main.

@ilya-bobyr
Copy link
Contributor Author

Out of curiosity have you looked into docs.rs/serde_json_path/latest/serde_json_path - I have used it before and quite liked it.

jsonpath-rust seems to be much more popular, compared to serde_json_path, judging just by the download count.
And, similarly, GitHub stars and fork counts for jsonpath-rust are higher, compared to serde_json_path.

It seems that both libraries can work directly with serde_json::Value objects. But jsonpath-rust was developed several years earlier. Not sure what are the advantages of serde_json_path.

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 :)

clux added a commit that referenced this pull request Nov 21, 2023
needed for #1345

Signed-off-by: clux <sszynrae@gmail.com>
@clux
Copy link
Member

clux commented Nov 21, 2023

Given how little usage there is of the serde_json_path variant, it is probably too fresh for us to bundle anyway. This takes care of the out-of-date problem for now. Happy to take up the discussion of one-vs-the-other again in the future once the ecosystem stabilises (they have both been releasing new versions quite recently).

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

clux added a commit that referenced this pull request Nov 22, 2023
needed for #1345

Signed-off-by: clux <sszynrae@gmail.com>
@Dav1dde
Copy link
Member

Dav1dde commented Nov 22, 2023

I think this is the right choice, was just (personally) curious if you explored more alternatives 👍 .

@clux clux merged commit bdda76e into kube-rs:main Nov 23, 2023
17 checks passed
@ilya-bobyr ilya-bobyr deleted the pr/switch-to-jsonpath-rust branch November 24, 2023 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-change changelog change category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants