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

Handle op:add for same path in patch #2370

Merged
merged 6 commits into from
Feb 7, 2024
Merged

Handle op:add for same path in patch #2370

merged 6 commits into from
Feb 7, 2024

Conversation

omarismail94
Copy link
Contributor

@omarismail94 omarismail94 commented Dec 12, 2023

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #2342

Description
When merging JSON patches:
* "replace" and "remove" operations from the second patch will overwrite any existing operations for the same path.
* "add" operations from the second patch will be added to the list of operations for that path, even if operations already exist for that path.

Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?

Type
Choose one: (Bug fix | Feature | Documentation | Testing | Code health | Builds | Releases | Other)

Screenshots (if applicable)

Checklist

  • I have read and acknowledged the Code of conduct.
  • I have read the Contributing page.
  • I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
  • I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
  • I have built and run the demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

@kelvin-ngure
Copy link

kelvin-ngure commented Dec 13, 2023

Edit: Below the *** was a concern had but I was wrong since the code is extracting the object from the list. And at the end, only the objects are inserted into the output array. So the structure during the reduction stays the same.


@omarismail94 since mergePatches is being called in a reduce function, the first time it runs, the parameters will receive strings with this sort of structure (the below are patch strings)

[{"op":"add","path":"/member/-","value":{"entity":{"reference":"Patient/dbe7936d-a1bc-4593-b219-025f34a8a8bf"}}}]
[{"op":"add","path":"/member/-","value":{"entity":{"reference":"Patient/e4ebe735-cbf5-4365-b8ad-0169466d51r9"}}}]

The output of the mergePatches function has the below structure since it is created from an Array Node (I use the word Patch String in place of the above objects)

[
Patch string,
Patch String,
]

Therefore, the next time the merge patches function is run (e.g when there are more than 2 LocalChange), the parameter for firstPatch will receive the correct data type but not the same structure. The first time, it was like this => [{}] but subsequent times it will be like this => [[{}], [{}]].

Will the op.get("path") still work? I assume that due to the nesting, path won't be recognized as a key. Thoughts?

@omarismail94
Copy link
Contributor Author

Edit: Below the *** was a concern had but I was wrong since the code is extracting the object from the list. And at the end, only the objects are inserted into the output array. So the structure during the reduction stays the same.

@omarismail94 since mergePatches is being called in a reduce function, the first time it runs, the parameters will receive strings with this sort of structure (the below are patch strings)

[{"op":"add","path":"/member/-","value":{"entity":{"reference":"Patient/dbe7936d-a1bc-4593-b219-025f34a8a8bf"}}}] [{"op":"add","path":"/member/-","value":{"entity":{"reference":"Patient/e4ebe735-cbf5-4365-b8ad-0169466d51r9"}}}]

The output of the mergePatches function has the below structure since it is created from an Array Node (I use the word Patch String in place of the above objects)

[ Patch string, Patch String, ]

Therefore, the next time the merge patches function is run (e.g when there are more than 2 LocalChange), the parameter for firstPatch will receive the correct data type but not the same structure. The first time, it was like this => [{}] but subsequent times it will be like this => [[{}], [{}]].

Will the op.get("path") still work? I assume that due to the nesting, path won't be recognized as a key. Thoughts?

@kelvin-ngure thanks for clarifying! i assume the change fixes the issue you have then?

@kelvin-ngure
Copy link

kelvin-ngure commented Dec 14, 2023

@omarismail94 yes it looks like it will work for my issue

Copy link
Collaborator

@MJ1998 MJ1998 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any latest version of jsonpatch ?

I think for each activity the path generated should be unique and not activity/-

Update: We are using the latest library!
The idea to overwrite patches when op = replace / remove is scary to me. Let's say there are 2 activities added. and then 1 is updated. This would mean the patch will contain 2 adds and 1 replace. Will the path be same for all these 3 operations ? If yes then again we lose the second activity in the patch.

Can we add a test to check that on replace / remove the path is always different ?!

LZRS added a commit to opensrp/android-fhir that referenced this pull request Jan 8, 2024
Includes:
- Handle op:add for same path in patch (google#2370)
@jingtang10 jingtang10 enabled auto-merge (squash) February 7, 2024 07:58
@jingtang10 jingtang10 merged commit 81c435b into google:master Feb 7, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

Error with Json Patch
5 participants