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

Inconsistent JObject.SelectNode() behavior and feature loss #15505

Closed
sarahelsaig opened this issue Mar 13, 2024 · 8 comments · Fixed by #15509 or #15524
Closed

Inconsistent JObject.SelectNode() behavior and feature loss #15505

sarahelsaig opened this issue Mar 13, 2024 · 8 comments · Fixed by #15509 or #15524
Labels
Milestone

Comments

@sarahelsaig
Copy link
Contributor

Describe the bug

I have a few concerns with the new JObject.SelectNode() extension method:

If you try to use it on a content part's Content property, it won't work unless you re-serialize the JSON object first, e.g.:

var content = (JsonObject)contentElement.Content;
content.SelectNode(path); // This is null.
JsonSerializer.SerializeToNode(content).SelectNode(path); // This is not null.

Also you can't chain SelectNode calls, the second call will still search from the same position, e.g.:
image

I think when you query a JsonObject, it tries to query from its document root instead of the current element. This is unexpected.

Finally, I'm not sure if anyone is affected, but after the migration to STJ we have lost full compliant JSONPath query support. The documentation of JObject.SelectNode() extension method claims that it searches using "a JSON path", however this is misleading because it's compliant with the JSONPath specification. An example of a query that should work but doesn't (using the same JSON as above):

var data = JsonNode.Parse("{\"SelectedContentItem\":{\"ContentItemIds\":[\"48yrgcwkhv7hkwarvtzpw3ccms\"]},\"Name\":\"My Content\"}");
data.SelectNode("SelectedContentItem.ContentItemIds"); // works, result: [ "48yrgcwkhv7hkwarvtzpw3ccms" ]
data.SelectNode("$.SelectedContentItem.ContentItemIds"); // works, result: [ "48yrgcwkhv7hkwarvtzpw3ccms" ]
data.SelectNode("$..ContentItemIds"); // doesn't work, result is null but it should be the same as above

You can verify that the last one should work using the JSONPath Online Evaluator:
image Also it works with Newtonsoft:

image

Expected behavior

  • SelectNode should query from the current element.
  • Ideally it should keep supporting full JSONPath.

Suggestion

Beyond fixing JObject.SelectNode to search from the current node, the documentation should be updated:

  • The documentation comment I linked above should include a <remarks> section explaining what is and what isn't supported by the current implementation.
  • The 1.9 release notes should mention this breaking change in the "Drop Newtonsoft.Json Support" section.

Consider if there is a JSONPath library we can use instead of a homebrew implementation. I haven't used it yet, but JsonPath.Net of the json-everything project might be suitable? It's for STJ, MIT licensed and it seems to be actively maintained.

@hyzx86
Copy link
Contributor

hyzx86 commented Mar 14, 2024

Good catch,
@hishamco maybe we can add some method on JsonDynamicObject class to resolve some relation problems

hyzx86 pushed a commit to hyzx86/OrchardCore that referenced this issue Mar 14, 2024
@hishamco
Copy link
Member

@hyzx86 if your PR fixes both issues, then I need to close my PR then

@hyzx86
Copy link
Contributor

hyzx86 commented Mar 14, 2024

@hyzx86 if your PR fixes both issues, then I need to close my PR then

Ok , but let me chek first.

@hyzx86
Copy link
Contributor

hyzx86 commented Mar 14, 2024

@hishamco , There seems to be a problem with the logic of the remove menu on the main branch, so let me merge your branches

@hyzx86
Copy link
Contributor

hyzx86 commented Mar 14, 2024

Or you can merge your branches first, and my branch can solve another problem

@sebastienros
Copy link
Member

Can you @sarahelsaig / @hyzx86 fix the SelectNode() behavior by using JsonPath.Net ?

@sebastienros sebastienros added this to the 1.9 milestone Mar 14, 2024
@sarahelsaig
Copy link
Contributor Author

I think so. I will give it a try.

@sarahelsaig
Copy link
Contributor Author

@sebastienros I've replaced the SelectNode() internals with JsonPath.Net (see PR #15524).
Now queries work as I'd expect and it's less code for us to maintain. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants