Use JsonPath.Net for JSONPath selectors (Lombiq Technologies: OCORE-148) #15524
Conversation
All uses of JsonArray should originate from JsonNode.
|
@MikeAlhayek could you please check? |
src/OrchardCore.Modules/OrchardCore.Menu/Controllers/AdminController.cs
Outdated
Show resolved
Hide resolved
| /// specification at https://www.rfc-editor.org/rfc/rfc9535.html or the JsonPath.Net documentation at | ||
| /// https://docs.json-everything.net/path/basics/. | ||
| /// </remarks> | ||
| public static JsonNode? SelectNode(this JsonNode? jsonNode, string? path, PathParsingOptions? options = null) |
There was a problem hiding this comment.
Why all these ? ? If nullability is enabled then it's wrong IMO, if not then it's not necessary, right?
There was a problem hiding this comment.
I realized it was there before, it should still be fixed IMO.
There was a problem hiding this comment.
Nullability was already enabled in this file here. I agree, jsonNode˙and path shouldn't be nullable (but options should, ofc). Fixed.
| } | ||
| path = path?.Trim(); | ||
| if (string.IsNullOrEmpty(path)) return jsonNode; | ||
| if (path[0] != '$') path = "$." + path; |
There was a problem hiding this comment.
is not using $. not supported in JSONPath.Net?
Ideally we would not patch what the caller specifies, but if we are using something that is currently incompatible, and JSONPath.NEt doesn't support it then fine. Add a comment in that case.
There was a problem hiding this comment.
It's not supported. (afaik paths without the leading $ is non-spec in JSONPath) Without that condition JsonPath.TryParse returns false, and JsonPath.Parse(path) throws:
Json.Path.PathParseException: Path must start with '$'
at Json.Path.PathParser.Parse(ReadOnlySpan`1 source, Int32& index, PathParsingOptions options, Boolean requireGlobal)
at Json.Path.JsonPath.Parse(String source, PathParsingOptions options)
Based on the exception message, this patch should be universally safe.
I believe in Newtonsoft does this automatically under the hood. You can see from this example, that the path property behaves as $.property and not as $..property.
I agree that it's not ideal, but without this patch existing functionality in OC would break.
Added a comment.
Co-authored-by: Mike Alhayek <[email protected]>
Piedone
changed the title
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
# Conflicts: # src/docs/community/contributors/images/contributors-map.svg
# Conflicts: # src/OrchardCore.Build/Dependencies.props
# Conflicts: # src/docs/community/contributors/images/contributors-map.svg
# Conflicts: # src/docs/community/contributors/images/contributors-map.svg
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
# Conflicts: # src/OrchardCore.Modules/OrchardCore.Menu/Controllers/AdminController.cs
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
|
@sarahelsaig can you please fix the conflict here so we can merge it? |
# Conflicts: # test/OrchardCore.Tests/Data/ContentItemTests.cs
Fixes #15505