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.

Sign up for GitHub

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

Jump to bottom

Accidental mutation of old model causes invalid JSON Patch #801

Closed
rmorshea opened this issue Aug 12, 2022 · 5 comments · Fixed by #802
Closed

Accidental mutation of old model causes invalid JSON Patch #801

rmorshea opened this issue Aug 12, 2022 · 5 comments · Fixed by #802
Labels
priority-1-high Should be resolved ASAP. type-bug About something that isn't working

Comments

@rmorshea
Copy link
Collaborator

Current Situation

Unfortunately, the monkey patch applied to jsonpatch doesn't seem to have really fixed the problem. I don't believe that we'll be able to resolve this issue and, even if we were, the library doesn't really seem to be maintained.

Proposed Actions

Instead of actually computing a diff, just use a replace operation where the value is the new render. This will probably have a performance cost in cases where network latency dominates. The alternative though, is getting an error on occasion when JSON patch fails to generate a correct diff.
@rmorshea rmorshea added flag-triage Not prioritized. type-bug About something that isn't working priority-1-high Should be resolved ASAP. and removed flag-triage Not prioritized. labels Aug 12, 2022
@rmorshea
Copy link
Collaborator Author

This issue was discovered to still exist while working with this test component.

This was referenced Aug 12, 2022
Merged
Merged
@rmorshea rmorshea changed the title JsonPatch Still Broken Accidental mutation of old model causes invalid JSON Patch Aug 13, 2022
@rmorshea
Copy link
Collaborator Author

Turns out, the invalid JSON patch was produced because I was mutating something I shouldn't have been.

rmorshea added a commit that referenced this issue Aug 13, 2022
@rmorshea
fix #801
6b2ab39 
accidental mutation of old parent model caused invalid JSON patches
rmorshea added a commit that referenced this issue Aug 13, 2022
@rmorshea
fix #801
d1c6d37 
accidental mutation of old parent model caused invalid JSON patches
rmorshea added a commit that referenced this issue Aug 13, 2022
@rmorshea
fix #801
c8e3f74 
accidental mutation of old parent model caused invalid JSON patches
@Archmonger
Copy link
Contributor

Will we be sticking to using jsonpatch then, or are there additional critical bugs in the library?

@rmorshea
Copy link
Collaborator Author

rmorshea commented Aug 13, 2022 •
edited
Loading

The issue I posted there is still valid, but the hacked fix I provided for it appears to work. So I don't think we need to drop jsonpatch in a hurry.

@rmorshea
Copy link
Collaborator Author

I still think we should plan to remove it eventually though.

rmorshea added a commit that referenced this issue Aug 13, 2022
@rmorshea
Fix: Accidental mutation of old model causes invalid JSON Patch (#802)
2f0bb98 
* fix #801

accidental mutation of old parent model caused invalid JSON patches

* add build_py to cmdclass

distutils has also been deprecated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
priority-1-high Should be resolved ASAP. type-bug About something that isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix: Accidental mutation of old model causes invalid JSON Patch reactive-python/reactpy
2 participants
@rmorshea @Archmonger