make patch works wrong for lists

[kxepal]

In short, there is very simple case that fails:

from jsonpatch import apply_patch, make_patch
src = {'foo': [1, 3]}
dst = {'foo': [0, 3, 3, 2]}
print apply_patch(src, make_patch(src, dst))

That's possible because of reversed statement at https://github.com/stefankoegl/python-json-patch/blob/master/jsonpatch.py#L244
If we remove it code above would work perfectly, but we'll break next case:

from jsonpatch import apply_patch, make_patch
src = {'foo': [1, 3]}
dst = {'foo': [0, 3, 3, 2]}
print apply_patch(dst, make_patch(dst, src))

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.

[kxepal]

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)))

[kxepal]

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)))

[stefankoegl]

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?

[stefankoegl]

Merged, thanks!