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

applyInPlace does not work when replace root path #188

Open
ruiruige opened this issue Oct 28, 2024 · 3 comments
Open

applyInPlace does not work when replace root path #188

ruiruige opened this issue Oct 28, 2024 · 3 comments

Comments

@ruiruige
Copy link

ruiruige commented Oct 28, 2024

Expected Behavior

do replace when path is root while using applyInPlace

Actual Behavior

did nothing

input: {"throw", true}
diff: [
{
"op": "replace",
"path": "",
"value": {
"throw": false
}
}
]

applyInPlace does not replace anything. after I debug, I found InPlaceApplyProcessor does not change sourceNode when path is root:
image

Specifications

Library Version: 0.4.11
Language (e.g. Java 1.8, Scala, etc):Java 1.8

@holograph
Copy link
Collaborator

While you are correct, the Jackson API (i.e. JsonNode) doesn't actually let you accomplish what you're trying to without some changes to the zjsonpatch API as well. To clarify:

  • JsonNode can be either a ValueNode (scalar) or ContainerNode (array, object)
  • Containers can be edited in place with ContainerNode.removeAll, as long as the replacement is also the same type of container
  • Scalar nodes cannot be edited at all (e.g. DoubleNode has a final value field)

In practice, this means applyInPlace cannot change the root node in many circumstances (replace object with scalar, replace scalar with object, replace object with array, replace array with object...). So we're left with the following options:

  1. Reject patches that replace the root outright;
  2. "Best faith" effort, i.e. only support replacing objects with objects and arrays with arrays;
  3. Return the "replaced" root node from applyInPlace (making it effectively a limited optimization).

I don't really have a strong opinion here, since applyInPlace is an implementation feature of zjsonpatch, rather than anything specced out in the RFC. To my mind a replace on the root path should always result in a runtime exception with applyInPlace.

Thoughts @ruiruige / @vishwakarma ?

@flipkart-incubator flipkart-incubator deleted a comment from tomerg-qm Dec 8, 2024
@holograph
Copy link
Collaborator

See #190

vishwakarma pushed a commit that referenced this issue Dec 13, 2024
* Reimplement full test suite with applyInPlace

* Support exceptions for InPlaceApplyProcessor + tests
@vishwakarma
Copy link
Member

Thanks @holograph

Thanks for the detailed breakdown and suggestions. I agree with your preference to reject patches that replace the root node outright when using applyInPlace. Raising a runtime exception in these cases feels like the most predictable and straightforward solution, and it keeps the behavior of applyInPlace aligned with its intended purpose.

While the other options like "best faith" efforts or returning the replaced root node offer some flexibility, they might lead to inconsistent behavior or make the method more complex than necessary. Rejecting root replacements ensures that users are explicitly handling these scenarios, which I think is the right approach

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

No branches or pull requests

3 participants