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

Use jsonptr to represent paths #34

Merged
merged 3 commits into from
Mar 19, 2024
Merged

Use jsonptr to represent paths #34

merged 3 commits into from
Mar 19, 2024

Conversation

asmello
Copy link
Contributor

@asmello asmello commented Mar 19, 2024

RFC 6902 references RFC 6901 where it chooses to define the paths in the patch operations as JSON Pointers. JSON Pointers can be encoded as strings, but they are conceptually a separate type, with very particular set of invariants that have to be maintained. This motivates defining a separate Rust type to represent them, as was done in the jsonptr crate, so that manipulations over these paths can be performed safely and with reusable code.

While use of Strings is sufficient to implement the required JSON Patch logic in this crate with minimal boilerplate, it makes it inconvenient for crate users to manipulate the patch operations outside of the diff and merge APIs. For example, a manual implementation of merge for a type other than a serde_json::Value would have to either implement its own string manipulation utilities for extracting tokens from the paths for traversal, or convert to a jsonptr::Pointer, which requires a clone. The reverse may also be required if the user crate intends to produce its own JSON Patches.

By embracing the jsonptr::Pointer type as the representation for the operation paths in this crate, not only we make things simpler and more efficient at the consumer side, we also avoid implementing some error-prone logic in this crate, as it can take advantage of the useful APIs provided by the jsonptr crate. The resulting code ends up being simpler, more elegant and easier to reason about on both sides.

In short, why reinvent the wheel? If there's a perfectly good representation for JSON Pointers in the Rust open-source ecosystem, may as well converge on it.

Side changes

  • Adding a PatchOperation::path method here for user convenience, since every operation has this field and it's useful for directing the operation to its appropriate location without need for matching explicitly on the operation variant.
  • Changed the error message of PatchError to start with a lowercase character (as is conventional in std, and to match the jsonptr messages),

@idubrov idubrov merged commit 1e6ae7b into idubrov:main Mar 19, 2024
3 checks passed
@asmello
Copy link
Contributor Author

asmello commented Mar 19, 2024

Wow that was quick! Thanks @idubrov

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

Successfully merging this pull request may close these issues.

2 participants