Add option to generate `test` operations in diff by RobertCochran · Pull Request #154 · stefankoegl/python-json-patch

RobertCochran · 2024-01-17T17:39:25Z

Hi,

This PR adds a new option for generating patches - generate_test_ops - that adds a test operation before every remove and replace operation. This allows for generating patches that have stronger data integrity characteristics. It is also a useful option if you care about knowing what the data was pre-patch (this is important for our use-case). The flag defaults to False to keep the default behavior the same.

I have modified the test suite to run the MakePatchTestCase twice - once with generate_test_ops=True and once with generate_test_ops=False. Patches are still applying properly in both cases so I am confident that all is well.

Please let me know if there's anything else that needs to be done as part of this PR.

Thanks!

This reverts commit 13e9ab0.

RobertCochran · 2024-03-25T20:14:18Z

Hi, any news on this?

Copilot

Pull Request Overview

This PR adds a new generate_test_ops option to the JSON patch generation that creates test operations before every remove and replace operation. This enhances data integrity by capturing the previous values before modifications are applied.

Adds generate_test_ops parameter to make_patch() and JsonPatch.from_diff() methods with default value False
Modifies DiffBuilder to generate test operations when the flag is enabled
Updates test suite to run all existing tests with both generate_test_ops=True and generate_test_ops=False

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.

File	Description
jsonpatch.py	Adds `generate_test_ops` parameter support and logic to generate test operations in the diff builder
tests.py	Updates all test cases to use the new parameter and adds new test classes to verify behavior with test operations enabled

Copilot · 2025-08-08T06:37:37Z


        return obj

+    def _on_undo_add(self, path, key):


These empty methods _on_undo_add and _on_undo_remove in the base PatchOperation class appear to be default implementations that don't perform any logic. Consider adding docstrings to explain their purpose or making them abstract methods if they're meant to be overridden by subclasses.

Copilot · 2025-08-08T06:37:38Z

+    :param generate_test_ops: While :const:`True` generate test operations
+                              capturing previous values of `replace`/`remove`
+                              operations. By default do not generate these


The docstring uses 'While :const:True' which is grammatically incorrect. It should be 'When :const:True' or 'If :const:True'.

Suggested change

:param generate_test_ops: While :const:`True` generate test operations

capturing previous values of `replace`/`remove`

operations. By default do not generate these

:param generate_test_ops: If :const:`True`, generate test operations

capturing previous values of `replace`/`remove`

operations. By default, do not generate these

Copilot · 2025-08-08T06:37:38Z

        :param dst: Data source document object.
        :type dst: dict

+        :param generate_test_ops: While :const:`True` generate test operations


Same grammatical error as in the make_patch function. The docstring uses 'While :const:True' which should be 'When :const:True' or 'If :const:True'.

Suggested change

:param generate_test_ops: While :const:`True` generate test operations

:param generate_test_ops: If :const:`True`, generate test operations

Copilot · 2025-08-08T06:37:38Z


+            if self.generate_test_ops:
+                prev_op_index = index[0]
+                if isinstance(prev_op_index[2], TestOperation):


[nitpick] The code accesses prev_op_index[2] directly using a magic index. Consider using a named constant or a more descriptive way to access the operation object to improve code readability.

Copilot · 2025-08-08T06:37:38Z

        exp = [{'op': 'remove', 'path': '/foo/0/baz'}]
+
+        if self.generate_test_ops:
+            exp.insert(0, {'path': '/foo/0/baz', 'value': 2, 'op': 'test'})


[nitpick] The expected result is defined before the conditional logic that modifies it. Consider moving the base expectation definition closer to where it's used, or make the conditional logic more explicit about what's being added.

Suggested change

exp = [{'op': 'remove', 'path': '/foo/0/baz'}]

if self.generate_test_ops:

exp.insert(0, {'path': '/foo/0/baz', 'value': 2, 'op': 'test'})

if self.generate_test_ops:

exp = [

{'path': '/foo/0/baz', 'value': 2, 'op': 'test'},

{'op': 'remove', 'path': '/foo/0/baz'}

]

else:

exp = [{'op': 'remove', 'path': '/foo/0/baz'}]

RobertCochran · 2025-10-29T19:31:01Z

Hi, checking in on this. Is there anything that needs to be done to get this merged?

stefankoegl

Thanks for working on this; recording the before-state with test ops is a useful feature. A few things need fixing before it can be merged:

Generated patches can fail on their own source. Test ops aren't shifted when list indices change, so about 19% of random list diffs produce a patch that fails when applied to its own source document (details inline).
Breaking API changes. make_patch, JsonPatch.from_diff and DiffBuilder change their positional signatures, and from_diff loses its optimization parameter.
Test values aren't deep-copied. Mutating the source after diffing changes the patch.

I've also suggested an alternative design (emit test ops in execute() from each op's final path) that would avoid the stale-index problem altogether.

Generated by Claude Code

stefankoegl · 2026-10-19T11:01:01Z

+    def _on_undo_add(self, path, key):
+        return key
+
+    def _on_undo_remove(self, path, key):
+        return key


Bug: test ops keep stale array indices. When an earlier list add/remove is undone (paired into a move), _on_undo_add/_on_undo_remove shift the index of the following remove/replace op, but these no-op versions leave the test op in front of it pointing at the old index. The test then checks the wrong element.

Reproducer:

p = jsonpatch.make_patch({'a': [1, 3, 0]}, {'a': [4, 1, 3, 3, 4]}, generate_test_ops=True) # [add /a/0, test /a/2 value 0, replace /a/3, add /a/4] p.apply({'a': [1, 3, 0]}) # JsonPatchTestFailed: 3 is not equal to tested value 0

In a fuzz run of 20,000 random small list pairs, 3,773 generated patches failed when applied to their own source with generate_test_ops=True, and none failed with it off.

Generated by Claude Code

stefankoegl · 2026-10-19T11:01:04Z



-def make_patch(src, dst, pointer_cls=JsonPointer):
+def make_patch(src, dst, generate_test_ops=False, pointer_cls=JsonPointer):


Backwards-incompatible signature change. Putting generate_test_ops before pointer_cls changes what positional callers get. make_patch(src, dst, CustomPointer) was valid before this PR. Now it silently turns on test-op generation (the class is truthy) and falls back to JsonPointer, with no error.

Suggest adding the new parameter after pointer_cls, or making it keyword-only.

Generated by Claude Code

stefankoegl · 2026-10-19T11:01:07Z

    @classmethod
    def from_diff(
-            cls, src, dst, optimization=True, dumps=None,
+            cls, src, dst, generate_test_ops=False, dumps=None,


Removes the public optimization parameter. generate_test_ops replaces optimization in the same position, so:

JsonPatch.from_diff(src, dst, optimization=False) now raises TypeError: unexpected keyword argument 'optimization'.

from_diff(src, dst, True, mydumps) now turns on test-op generation instead.

Suggest keeping optimization and adding generate_test_ops as a new keyword argument at the end.

Generated by Claude Code

stefankoegl · 2026-10-19T11:01:09Z

 class DiffBuilder(object):

-    def __init__(self, src_doc, dst_doc, dumps=json.dumps, pointer_cls=JsonPointer):
+    def __init__(self, src_doc, dst_doc, dumps=json.dumps, generate_test_ops=False, pointer_cls=JsonPointer):


Same positional break in DiffBuilder. DiffBuilder(src, dst, json.dumps, MyPointer) now sets generate_test_ops=MyPointer and uses the default JsonPointer.

Related: _item_replaced now requires a fourth argument, old_item. A subclass that overrides _item_replaced(self, path, key, item) will get a TypeError when _compare_values calls it with four arguments.

Generated by Claude Code

stefankoegl · 2026-10-19T11:01:13Z

+            if self.generate_test_ops:
+                prev_op_index = index[0]
+                if isinstance(prev_op_index[2], TestOperation):
+                    self.remove(prev_op_index)


Design suggestion. Removing the paired test op depends on it being the node directly before the remove in the linked list. Because test ops are separate nodes, every path that rewrites indices (_on_undo_*, the remove/add pairing, the replace collapse in execute()) needs special handling, and the stale-index bug above comes from that.

A sturdier approach: store the old value on the RemoveOperation/ReplaceOperation and emit the test op in execute(), right before each final remove/replace, using that op's final path. That fixes the index problem and lets you drop this adjacency check and the TestOperation._on_undo_* stubs.

Generated by Claude Code

stefankoegl · 2026-10-19T11:01:15Z

+        if self.generate_test_ops:
+            test_index = self.insert(TestOperation({
+                'op': 'test',
+                'path': _path_join(path, key),
+                'value': item,
+            }, pointer_cls=self.pointer_cls))


Minor: this always builds a TestOperation (including parsing its pointer) and links it into the list, and both paired branches below then remove it again. Calling take_index first and inserting the test op only when index is None does the same thing with less work and one fewer branch.

Generated by Claude Code

stefankoegl · 2026-10-19T11:01:19Z

+            test_index = self.insert(TestOperation({
+                'op': 'test',
+                'path': _path_join(path, key),
+                'value': item,


The test value isn't copied. The test op keeps a reference to the source sub-object, so mutating the source after diffing also changes the patch:

src = {'a': [1, 2]} p = jsonpatch.make_patch(src, {}, generate_test_ops=True) src['a'].append(3) p.patch # test /a value [1, 2, 3]

Applying p to a clean copy of the original document then fails, and the recorded before-state (the point of this feature) is wrong. Suggest copy.deepcopy(item) here, and for old_item in _item_replaced.

Generated by Claude Code

stefankoegl · 2026-10-19T11:01:22Z

+            self.insert(TestOperation({
+                'op': 'test',
+                'path': _path_join(path, key),
+                'value': old_item,


The integrity check misses JSON type changes. The diff detects changes using dumps(), which treats 1, True and 1.0 as different, but TestOperation compares with Python !=, which treats them as equal. So make_patch({'a': 1}, {'a': True}, generate_test_ops=True) produces test /a value 1, and that test also passes against {'a': True} or {'a': 1.0}.

This may be out of scope for this PR (it's existing TestOperation behaviour), but it weakens the integrity guarantee the feature is meant to give, so it's worth documenting at least.

Generated by Claude Code

stefankoegl · 2026-10-19T11:01:25Z

        self.assertEqual(new, new_from_patch)


+class MakePatchWithTestOpsTestCase(MakePatchTestCase):


Coverage gap. The test-ops variants only rerun the existing hand-picked cases, and none of them shift list indices in a way that exposes the stale-index bug. Please add a case like src={'a': [1, 3, 0]}, dst={'a': [4, 1, 3, 3, 4]} that applies the generated patch to the source, and consider running property_tests.py/randomized list diffs with generate_test_ops=True.

Generated by Claude Code

stefankoegl · 2026-10-19T11:01:27Z

        src = {'foo': 'bar', 'boo': 'qux'}
        dst = {'baz': 'qux', 'foo': 'boo'}
-        patch = jsonpatch.make_patch(src, dst)
+        patch = jsonpatch.make_patch(src, dst, generate_test_ops=self.generate_test_ops)


Nit: generate_test_ops=self.generate_test_ops is repeated in about 30 calls. A small helper such as def _make_patch(self, src, dst): return jsonpatch.make_patch(src, dst, generate_test_ops=self.generate_test_ops) would remove the repetition, and new tests in these classes would cover the test-ops variant automatically.

Generated by Claude Code

RobertCochran added 7 commits January 16, 2024 14:47

Add generate_test_ops flag

2cb66b9

Extend test suite

597b76e

Do not print error warning over pandoc

13e9ab0

Revert "Do not print error warning over pandoc"

35734fb

This reverts commit 13e9ab0.

Remove generated test op when transforming remove to move

e1de7e4

Extend OptimizationTests to be dual-tested with generate_test_ops

821ed0a

Remove pregenerated test op when remove is converted to move

ee5040f

stefankoegl requested a review from Copilot August 8, 2025 06:36

Copilot AI reviewed Aug 8, 2025

View reviewed changes

stefankoegl reviewed Oct 19, 2026

View reviewed changes

	:param generate_test_ops: While :const:`True` generate test operations
	:param generate_test_ops: If :const:`True`, generate test operations



		def make_patch(src, dst, pointer_cls=JsonPointer):
		def make_patch(src, dst, generate_test_ops=False, pointer_cls=JsonPointer):

		self.assertEqual(new, new_from_patch)


		class MakePatchWithTestOpsTestCase(MakePatchTestCase):

Conversation

RobertCochran commented Jan 17, 2024

Uh oh!

RobertCochran commented Mar 25, 2024

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

RobertCochran commented Oct 29, 2025

Uh oh!

stefankoegl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants