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

Wrong Patch Generated #138

Open
rmorshea opened this issue Dec 24, 2021 · 7 comments
Open

Wrong Patch Generated #138

rmorshea opened this issue Dec 24, 2021 · 7 comments

Comments

@rmorshea
Copy link

The following sample of code ought to work, but instead it raises an error:

from jsonpatch import apply_patch, make_patch

old = [
    {"x": ["a", {"y": ["b"]}], "z": "a"},
    {"x": ["c", {"d": ["d"]}], "z": "c"},
    {},
]

new = [
    {"x": ["c", {"y": ["d"]}], "z": "c"},
    {},
]

patch = make_patch(old, new)

assert apply_patch(old, patch) == new
Traceback (most recent call last):
  File "/venv/lib/python3.9/site-packages/jsonpointer.py", line 275, in walk
    return doc[part]
IndexError: list index out of range

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/temp.py", line 18, in <module>
    assert apply_patch(old, patch) == new
  File "/venv/lib/python3.9/site-packages/jsonpatch.py", line 151, in apply_patch
    return patch.apply(doc, in_place)
  File "/venv/lib/python3.9/site-packages/jsonpatch.py", line 669, in apply
    obj = operation.apply(obj)
  File "/venv/lib/python3.9/site-packages/jsonpatch.py", line 324, in apply
    subobj, part = self.pointer.to_last(obj)
  File "/venv/lib/python3.9/site-packages/jsonpointer.py", line 194, in to_last
    doc = self.walk(doc, part)
  File "/venv/lib/python3.9/site-packages/jsonpointer.py", line 278, in walk
    raise JsonPointerException("index '%s' is out of bounds" % (part, ))
jsonpointer.JsonPointerException: index '1' is out of bounds

The generated patch is:

[
    {"op": "remove", "path": "/0/x/0"},
    {"op": "replace", "path": "/0/x/1/y/0", "value": "d"},
    {"op": "replace", "path": "/0/z", "value": "c"},
    {"op": "remove", "path": "/1/x"},
    {"op": "move", "from": "/1/z", "path": "/0/x/0"},
    {"op": "remove", "path": "/2"},
]

Which is clearly the result of the error above.

@rmorshea
Copy link
Author

It looks like swapping these two items fixes the patch:

{"op": "remove", "path": "/0/x/0"}
{"op": "replace", "path": "/0/x/1/y/0", "value": "d"}

So that the remove operation happens second:

{"op": "replace", "path": "/0/x/1/y/0", "value": "d"}
{"op": "remove", "path": "/0/x/0"}

@rmorshea
Copy link
Author

Removing this section of code (which I assume is some optimization heuristic) fixes the assertion: https://github.com/stefankoegl/python-json-patch/blob/master/jsonpatch.py#L828-L851

@rmorshea
Copy link
Author

rmorshea commented Jan 4, 2022

For those who need to work around this until a patch is released, the following code will fix this issue (though probably with a hit to performance):

from jsonpatch import _ST_REMOVE
from jsonpatch import DiffBuilder as _DiffBuilder
from jsonpatch import JsonPatch as _JsonPatch
from jsonpatch import JsonPointer, RemoveOperation, _path_join


def apply_patch(doc, patch, in_place=False, pointer_cls=JsonPointer):
    if isinstance(patch, (str, bytes)):
        patch = JsonPatch.from_string(patch, pointer_cls=pointer_cls)
    else:
        patch = JsonPatch(patch, pointer_cls=pointer_cls)
    return patch.apply(doc, in_place)


def make_patch(src, dst, pointer_cls=JsonPointer):
    return JsonPatch.from_diff(src, dst, pointer_cls=pointer_cls)


class JsonPatch(_JsonPatch):
    @classmethod
    def from_diff(
        cls,
        src,
        dst,
        optimization=True,
        dumps=None,
        pointer_cls=JsonPointer,
    ):
        json_dumper = dumps or cls.json_dumper
        builder = DiffBuilder(src, dst, json_dumper, pointer_cls=pointer_cls)
        builder._compare_values("", None, src, dst)
        ops = list(builder.execute())
        return cls(ops, pointer_cls=pointer_cls)


class DiffBuilder(_DiffBuilder):
    def _item_removed(self, path, key, item):
        new_op = RemoveOperation(
            {
                "op": "remove",
                "path": _path_join(path, key),
            },
            pointer_cls=self.pointer_cls,
        )
        new_index = self.insert(new_op)
        self.store_index(item, new_index, _ST_REMOVE)

@rmorshea
Copy link
Author

rmorshea commented Jun 26, 2022

@stefankoegl any ideas on what's going wrong here? I've narrowed down the issue about as much as I can without having to put in a bunch more time to figure out the underlying issue.

@rmorshea
Copy link
Author

@stefankoegl do you have any hints you could give me on how to fix this?

It turns out that my patch hasn't complete fixed this bug and I'm in desperate need of a solution.

@stefankoegl
Copy link
Owner

Unfortunately, I don't have anything I can suggest. I haven't been actively working on the project for quite some time now, so I'm not up-to-speed at the moment.

@rmorshea
Copy link
Author

rmorshea commented Aug 13, 2022

Turns out my hacked fix still works. The new error I was experiencing was caused by something else.

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

2 participants