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

Using JsonPath in JsonPatch operations #209

Closed
andrewjw1995 opened this issue Jan 27, 2022 · 13 comments
Closed

Using JsonPath in JsonPatch operations #209

andrewjw1995 opened this issue Jan 27, 2022 · 13 comments
Labels
question Further information is requested

Comments

@andrewjw1995
Copy link

Hi all,

Would it be possible to make the path handling in the JsonPatch library open for extension, so it could be used with JsonPath syntax? This is my current use case:

[
  {
    "op": "replace",
    "path": "$.inputs[?(name == 'voltage')].threshold",
    "value": 400
  }
]

This is a very useful way to handle concurrent updates to a document. Two users can safely add, remove, and replace items from the same array if they are referring to the items by a stable id instead of the array index.

I realize this would be a breaking change, as the current implementation uses JsonPointer to represent the path. This would need to be changed to a common type, or maybe just a string which is not parsed until the patch is executed. It also raises new problems, like what happens if the path matches multiple elements.

@andrewjw1995 andrewjw1995 added the question Further information is requested label Jan 27, 2022
@gregsdennis
Copy link
Owner

As it stands, JSON Patch uses JSON Pointers to indicate locations within target documents, so this is what I support.

I've actually commented on a proposal for JSON Patch to use JSON Path. While I think that using JSON Path is the correct approach for identifying multiple nodes, I hesitate to implement it without the JSON Patch spec updating to support it.

Two users can safely add, remove, and replace items from the same array if they are referring to the items by a stable id instead of the array index.

How do you envision this working (considering threading, etc.)? What do you mean by "two users?"

It also raises new problems, like what happens if the path matches multiple elements.

This is definitely an open question. The issue I linked suggests that all nodes would be affected. But I think this would need to be defined per operation. For example, what does test do if some of the locations have the right value, but others don't?

Lastly, there's an issue of interoperability. If I support this, any patch document you create that uses paths for location indicators will only be able to be processed by my library. Because JSON is supported in such a wide variety of languages, technologies that built on it are also supported in many of those languages. Thus those technologies must strive to remain language agnostic.

@andrewjw1995
Copy link
Author

Thanks for the prompt response, Greg.

Regarding the 'two users' comment: I'm storing some configuration as json in my system, and I have to force users to stop editing and reload the document if it was modified by another user, even if the changes don't conflict. This is pretty specific to arrays, as the elements are currently selected by index so any change to the order of the array could result in subsequent patches affecting an unexpected element.

Since this is already a request in the JSON Patch 2 repo, I think any discussion regarding the implementation should be there instead. However, that request has been open for 3 years now, with no signs of being accepted any time soon. I think perhaps I'll have to fork this repo and roll my own solution in the meantime.

@gregsdennis
Copy link
Owner

Yeah, it doesn't look like JSON Patch 2 is going anywhere, and I'm not comfortable supporting an unspecified format with this library. I am watching that issue, so I'll see any comments you make. Please post if you come up with anything interesting.

I'm still not certain that supporting JSON Path in the location indicator is a great solution as it opens a lot of edge cases and questions for what to do when the path selects multiple locations.

Specific to your use case, configuration that you want to update at runtime is typically better stored in a database. There are a number of document databases that you could probably adopt pretty easily. If you're set on storing in JSON, I'd suggest using an object instead of an array if you want to be able to access it via a key (e.g. voltage from your example above).

@ejsmith
Copy link

ejsmith commented Mar 26, 2022

I would love for JSON Patches to make use of JSON Path. I implemented support for doing so with the JSON.NET JsonPatch library and I'd love to get that supported in this library as well. It works amazingly well for doing things like a remove operation with a path of $.books[?(@.id == 'ABC123')] instead of having to figure out what the array index is and do a /books/3 and hope that somebody else doesn't add an element to the books array before my patch operation is applied.

@gregsdennis
Copy link
Owner

Thanks @ejsmith, I really do understand the desire for this. Again, my primary concerns are adherence to the specification and interoperability.

As JSON Path continues its journey toward an official specification, I've added an "experiemental features" option to evaluations. I'm also considering doing that for JSON Schema as that spec continues to evolve.

I might be able to work something like this out by abstracting the path resolution. I would only provide an implementation for JSON Pointer, though. I would leave alternate path resolution to the user (you). I'd definitely want to be sure that this is not the default behavior so that some configuration is required to enable it.

I'd like to hear your thoughts on interoperability. For instance, if you send a patch with JSON Path path values, other systems won't be able to understand that patch.

@ejsmith
Copy link

ejsmith commented Mar 26, 2022

@gregsdennis Having some sort of option to pick which resolution mechanism to use would be great and if that is just making it pluggable where we could add the JSON Path based resolution then that would be great as well. I've been digging through the library and trying to see what it would take. Was also looking to change out JsonElement to JsonNode which you had mentioned you were looking to do, but that looks like quite the undertaking since JsonPath is making very extensive use of JsonElement. Are you still looking to change to JsonNode?

@gregsdennis
Copy link
Owner

Are you still looking to change to JsonNode?

I'm not sure I want to do this anymore. (It is a requirement to implement Relative JSON Pointer because it has a parent pointer whereas JsonElement doesn't.) The API for JsonNode is quite different, and when I looked, moving between the two models and strings is cumbersome.

It would also mean breaking changes for every library that I move over. I would need to be sure to coordinate that properly.

I might try to look at it again at some point, but for now I'm sticking with JsonElement.

@ejsmith
Copy link

ejsmith commented Mar 26, 2022

Yeah, I get what you are saying that it's quite different and a ton of breaking changes although I think you could probably make all of the public methods take both JsonElement as well as JsonNode. I just got done converting JsonPointer over to JsonNode to see how it would go and it wasn't too bad, but it seems like JsonPath will probably be a lot more work. It does seem to me like JsonNode is the more appropriate underlying model for this set of libraries, but it would probably be hard to pull off a conversion without a lot of breaking changes.

@gregsdennis
Copy link
Owner

gregsdennis commented Mar 26, 2022

The other problem with using JsonNode is that it's only available starting with .Net 6 (ref). Most of these libraries are .Net Standard. Supporting JsonNode for just .Net 6+ and JsonElement for everything earlier (using compiler directives) will get very ugly very quickly, and I don't want to do that.

@ejsmith
Copy link

ejsmith commented Mar 27, 2022

@gregsdennis that is just what framework it shipped with. You can still target netstandard2.0 and use System.Text.Json 6.0.

@gregsdennis
Copy link
Owner

Updated to this for Json.More and JsonPointer.Net. I've updated those. Hopefully that's a first step to supporting it in the other packages.

@yilmaznaslan
Copy link

Hi,

is there any update on supporting JsonPath as path ?

@gregsdennis
Copy link
Owner

@yilmaznaslan please see the explanation above regarding JSON Patch 2. JSON Path will unlikely be a supported syntax until it is accepted into a specification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants