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
make patch works wrong for lists #4
Comments
Proposed patch to fix problem, but is it ok to use inner stack? diff --git a/jsonpatch.py b/jsonpatch.py
index 1240586..f8dd536 100644
--- a/jsonpatch.py
+++ b/jsonpatch.py
@@ -241,7 +241,8 @@ class JsonPatch(object):
def compare_list(path, src, dst):
lsrc, ldst = len(src), len(dst)
- for idx in reversed(range(max(lsrc, ldst))):
+ stack = []
+ for idx in range(max(lsrc, ldst)):
if idx < lsrc and idx < ldst:
current = path + [str(idx)]
for operation in compare_values(current, src[idx], dst[idx]):
@@ -250,7 +251,9 @@ class JsonPatch(object):
yield {'add': '/'.join(path + [str(idx)]),
'value': dst[idx]}
elif idx < lsrc:
- yield {'remove': '/'.join(path + [str(idx)])}
+ stack.append({'remove': '/'.join(path + [str(idx)])})
+ for item in reversed(stack):
+ yield item
return cls(list(compare_dict([''], src, dst))) |
After some thoughts, I suppose next solution would be better: diff --git a/jsonpatch.py b/jsonpatch.py
index 1240586..1865737 100644
--- a/jsonpatch.py
+++ b/jsonpatch.py
@@ -241,15 +241,15 @@ class JsonPatch(object):
def compare_list(path, src, dst):
lsrc, ldst = len(src), len(dst)
- for idx in reversed(range(max(lsrc, ldst))):
- if idx < lsrc and idx < ldst:
- current = path + [str(idx)]
- for operation in compare_values(current, src[idx], dst[idx]):
- yield operation
- elif idx < ldst:
- yield {'add': '/'.join(path + [str(idx)]),
- 'value': dst[idx]}
- elif idx < lsrc:
+ for idx in range(min(lsrc, ldst)):
+ current = path + [str(idx)]
+ for operation in compare_values(current, src[idx], dst[idx]):
+ yield operation
+ if lsrc < ldst:
+ for idx in range(lsrc, ldst):
+ yield {'add': '/'.join(path + [str(idx)]), 'value': dst[idx]}
+ elif ldst > lsrc:
+ for idx in reversed(range(ldst, lsrc)):
yield {'remove': '/'.join(path + [str(idx)])}
return cls(list(compare_dict([''], src, dst))) |
Sorry for my late response. GitHub didn't notify me when you added the proposed solutions.... The 2nd solution looks good at first glance. Could you put that in a pull request together with the initial test? |
kxepal
added a commit
to kxepal/python-json-patch
that referenced
this issue
Jul 15, 2012
Patch creation from diff of two JSON documents was able to produce invalid instance if there was need to add more than 1 array element. In this situation, elements been tryed be added since higher index that is far outside of target. That's correct order if we removing array elements (from higher index to lower one), but we should add elements in reversed order (from lower index to higher).
Merged, thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In short, there is very simple case that fails:
That's possible because of
reversed
statement at https://github.com/stefankoegl/python-json-patch/blob/master/jsonpatch.py#L244If we remove it code above would work perfectly, but we'll break next case:
In other words, we have to add items in ascending order and remove them in descending one. That's my code and my fault, I'll try to figure how to fix it in better way, just creating issue there to let you know about problem.
The text was updated successfully, but these errors were encountered: