Skip to content

Add opt-in atomic application of in-place patches - #251

Merged
stefankoegl merged 15 commits into
masterfrom
claude/hopeful-brown-vzvsj7
Nov 4, 2026
Merged

stefankoegl merged 15 commits into
masterfrom
claude/hopeful-brown-vzvsj7

Conversation

@stefankoegl

@stefankoegl stefankoegl commented Nov 3, 2026 •

Copy link
Copy Markdown
Owner

Fixes #115.

With in_place=True, a patch that fails part-way leaves the changes of the preceding operations in the document, and the caller can't tell which ones were applied. in_place=False avoids this, but deep-copies the whole document.

This adds an opt-in atomic parameter to JsonPatch.apply and apply_patch:

jsonpatch.apply_patch(doc, patch, in_place=True, atomic=True)

If an operation fails, the changes of the preceding operations are undone before the exception is re-raised, so the document is either fully patched or left unchanged. The default behaviour is unchanged.

How it works

  • The built-in operations make their changes through a journal (_Journal). Before each change it records the changed container and the old value; on failure the steps are undone newest first. Because they are undone in reverse order, each container is in the same state as right after its change, so list indices stay valid. The document is not copied; only the values the patch removes or replaces, and the key order of objects that lose a member, are kept until the patch is applied.
  • Without atomic, a _NullJournal makes the changes directly, so the default path records nothing.
  • Removed members of a dict (including subclasses such as OrderedDict or Counter) are restored at their former position, so the member order is unchanged after a rollback (not just equal). The keys of each dict are recorded once, before its first removal; after all steps are undone, the members from the first misplaced one on are moved back into order with pop() and item assignment, so applying and rolling back stay linear even for many removals from one large object. Other mappings get their members back, but their order isn't rebuilt.
  • A ChainMap keeps its layering: setting a member it takes from another of its mappings adds it to the first one, and undoing that removes it again.
  • Appending to a full bounded deque drops its first item; the journal records it and puts it back when undoing the append.
  • The original containers are put back, so references into the document stay valid.
  • Rollback runs on any exception, including JsonPointerException.
  • atomic has no effect without in_place=True, as the document is copied anyway.

Custom operations

Operations registered through JsonPatch.operations may override apply and change the document directly, so their changes can't be undone. If a patch contains such an operation (a subclass of a built-in operation that changes how it is applied, or an operation object that sets apply itself), atomic=True copies the document, applies the patch in place, and if it fails restores the document's contents from the copy: mutable mappings and sequences (including e.g. deque) are refilled in place, each mapping of a ChainMap is restored on its own, and immutable containers such as tuples have their mutable descendants restored. That is still all-or-nothing, but costs a copy like in_place=False, and nested containers are then the copies rather than the originals.

Internally, PatchOperation.apply(obj) now delegates to _apply(obj, journal); the built-in operations implement _apply. The public apply(obj) signature of operations is unchanged, custom operations that override apply keep working, and AddOperation subclasses that override _add(obj, value) are still called that way.

What is guaranteed

Exact restoration, with the same values, member order and container objects, is guaranteed for documents of plain dict and list objects, which includes every document parsed from JSON. Other containers are restored through their own item operations, which is exact as long as these only change the item they are given. The tutorial documents two limits:

  • Containers that change when they are merely read, such as collections.defaultdict, are not supported: pointer lookups, test, and the from of move/copy can add members to them that remain after a rollback (they also do so without atomic).
  • The copy that patches with custom operations are restored from only restores the items of mappings and sequences, not other state a custom operation can change, such as attributes of objects in the document or the maps of a ChainMap.

Tests

  • tests.py: new AtomicApplyTests — the example from the issue, the default still leaving a partial result, failures after each operation type on objects and arrays (including move within a list, copy, nested changes, members added before a removal), original containers restored, member order of dict, OrderedDict, defaultdict and Counter, ChainMap (removals, replacing inherited members, and restoring from a copy), duck-typed and defaultdict mappings, bounded deques (including maxlen=0), JSON pointer and unexpected errors, root replacement, success, reapplying a patch, custom operations (including one that only fails on the original document, one that sets apply on the instance, and deque, tuple and read-only Mapping roots), and _add overrides without the journal parameter.
  • property_tests.py:
    • test_atomic_in_place_gives_same_result: for random patches, the outcome equals copy-based application, and on failure the document is unchanged, compared via json.dumps so member order counts.
    • test_atomic_rollback_restores_document: uses docs_with_failing_patches, which draws each operation against the document as the preceding ones left it (so later operations act on what earlier ones added, moved or replaced), keeps the ones that succeed, and ends with one that fails. 84% of its cases roll back at least one change and 56% more than one (vs. 29% / 14% for the random patches). It checks that the error matches copy-based application, that the document is unchanged including member order and the identity of every container, and that the patch without its failing operation gives the same result as on a copy. With a fresh Hypothesis database it detects, in 3 of 3 runs each, a missing rollback, missing order restoration, wrong undo indices for list removals and appends, a replace whose old value isn't restored, restoring copies instead of the original values, and the add-before-remove bug found in review, which the random patches did not find.
  • Disabling the rollback, or the member-order restoration, makes the new tests fail; each review fix comes with a test that fails without it.
  • tests.py, property_tests.py (100% line and branch coverage), the tutorial doctests, mypy.stubtest and mypy on typetests/usage.py pass; the new property also passes 5,000 examples (HYPOTHESIS_PROFILE=thorough).
  • 20,000 additions plus 20,000 removals on one object and a rollback take 0.27 s with atomic=True.

Docs: new paragraphs in doc/tutorial.rst under "When an Operation Fails", including what is guaranteed; atomic added to the type stubs and typetests/usage.py.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PewzpjP8bGvTXBq5LST6Xh

With in_place=True, a patch that fails part-way left the changes of the
preceding operations in the document. JsonPatch.apply and apply_patch
now take atomic=True, which undoes those changes before re-raising, so
the document is either fully patched or left unchanged.

The built-in operations make their changes through a journal that
records how to undo each one (the changed container and the old value),
so this does not copy the document. Removed object members are restored
at their former position, and the original containers are put back, so
references into the document stay valid. Patches with custom operations,
whose changes can't be undone, are tried on a copy first instead.

The default behaviour is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PewzpjP8bGvTXBq5LST6Xh
@coveralls

coveralls commented Nov 3, 2026 •

Copy link
Copy Markdown

Coverage Status

coverage: 100.0%. remained the same — claude/hopeful-brown-vzvsj7 into master

Test rolling back a removal from a duck-typed mapping, and drop the
unreachable fallback of _key_index: the key was just read from the
mapping, so iterating over it finds the key.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PewzpjP8bGvTXBq5LST6Xh

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Failed atomic patches can leave partial changes in the original document, existing AddOperation subclasses can raise TypeError even with atomic mode disabled, and removing missing mapping members can incorrectly succeed.

3 open issues
What changed in this PR

Adds opt-in atomic in-place patching to address #115, using a change journal to undo failed patches without copying the entire document for built-in operations.

Changes:

  • Adds atomic to both patch-application APIs and routes built-in mutations through a rollback journal.
  • Adds rollback, compatibility, and property-based tests.
  • Documents atomic application and updates type stubs and usage checks.
File Description
typetests/​usage.py Checks typed calls with atomic=True.
tests.py Adds atomic application and rollback test cases.
property_tests.py Checks outcome equivalence and document restoration on failure.
jsonpatch.py Implements journaling, rollback, and custom-operation preflight.
jsonpatch-stubs/​__init__.pyi Adds atomic to public API signatures.
doc/​tutorial.rst Explains atomic usage, memory behavior, and custom operations.

Review effort: Balanced

Comment thread jsonpatch.py Resolved
Comment thread jsonpatch.py Resolved
Comment thread jsonpatch.py Resolved
- Call AddOperation._add without the journal when not applying
  atomically, so that subclasses overriding _add(obj, value) keep
  working.
- Don't read a missing mapping member before removing it, as that
  creates it in a defaultdict, and the removal then succeeded.
- Patches with custom operations were tried on a copy first, but the
  application to the document itself was not undone if it failed
  there. Restore the document's contents from the copy instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PewzpjP8bGvTXBq5LST6Xh

Copy link
Copy Markdown
Owner Author

Failing status: Coveralls - ubuntu-latest-python-pypy3.11 ("Coverage decreased (-2.0%) to 90.143%") on 7431016.

All GitHub Actions jobs, including test (ubuntu-latest, pypy3.11), pass. The failing status is only about coverage, and I don't think it reflects untested code:

  • CI measures branch coverage of jsonpatch.py only (.coveragerc: source = jsonpatch, branch = True).
  • On the same commit, every CPython job (3.10–3.14, Linux/macOS/Windows) reports 100.0%, unchanged; locally make test also reports 100% of lines and branches.
  • On master, the pypy job was already at ~92% while CPython was at 100%, so pypy's tracer counts some lines/arcs of jsonpatch.py as missed that are executed. The new journal code (loops, generator expressions, try/except BaseException) adds more such constructs, which lowers the pypy-only number further.

What blocks fixing it from here: this session can't reach coveralls.io, the raw job logs, or a pypy download, so I can't see which lines pypy reports as missed, and re-running the job would give the same deterministic result. If you want the pypy number unchanged, the per-file report from that job (coverage report -m under pypy) would show which constructs to restructure; otherwise this status can be ignored like the pypy/CPython gap that already exists on master.


Generated by Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Atomic rollback can leave documents mutated when immutable roots contain mutable children or defaultdict lookups create missing members before a later operation fails.

1 open issue
1 previously missed issue

In code that hasn't changed since last review

Medium severity Atomic rollback leaves mapping entries created by source lookup

jsonpatch.py:523

With doc = defaultdict(int, a=1), moving /missing to /b and then running a failing remove leaves 'missing': 0 after atomic rollback: the source read creates that member before the journal records any change. Copy has the same problem at jsonpatch.py:600, and missing intermediate members can also be created during pointer resolution. Guard missing mapping members during source lookup and pointer traversal, or journal those read-induced insertions, so failed atomic patches leave the document unchanged.

3 resolved since last review

Review effort: Balanced

Comment thread jsonpatch.py Resolved
When a patch with custom operations fails with atomic=True, the document
is restored from a copy. Only mutable mappings and sequences were
restored, so changes to mutable members of an immutable root, such as a
dict inside a tuple, remained. Walk immutable containers and restore
their mutable descendants.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PewzpjP8bGvTXBq5LST6Xh
Resolve the conflict in the test suite registration by running both
AtomicApplyTests and PathNotFoundTests.

master added JsonPatchPathNotFound without a stub, which fails the
stubtest check, so add it to jsonpatch-stubs as well.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PewzpjP8bGvTXBq5LST6Xh

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Atomic rollback can leave documents modified because implicit insertions during pointer traversal bypass journaling, while deque restoration can fail and mask the original exception.

2 open issues
1 resolved since last review

Review effort: Balanced

Comment thread jsonpatch.py Resolved
Comment thread jsonpatch.py Resolved
deque is a MutableSequence but doesn't support obj[:] = saved, so
restoring a deque root raised TypeError, which hid the original
exception and left the deque changed. Use clear() and extend() instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PewzpjP8bGvTXBq5LST6Xh

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Human assessment is needed because container-specific rollback failures can leave documents mutated or mask original errors, while repeated key scans introduce quadratic deletion overhead.

1 open issue
1 resolved since last review

Review effort: Balanced

Containers such as defaultdict add members when they are only read,
e.g. while a pointer is resolved, a value is tested or the 'from' of a
move or copy is read. These additions are not journaled, so they remain
after a rollback. Document this rather than resolving pointers without
the pointer class, as documents parsed from JSON are not affected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PewzpjP8bGvTXBq5LST6Xh
To restore the order of an object's members, every removal looked up
the position of the removed key, and undoing it re-added all members
that followed. Removing many members from one large object was
therefore quadratic: 20,000 removals from the end took 9 s instead of
0.05 s without atomic.

Instead, record the keys of each mapping once, before its first removal,
and restore that order once after all steps have been undone. Applying
and rolling back are now linear (0.09 s for the case above).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PewzpjP8bGvTXBq5LST6Xh

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Rollback after an add-before-remove failure can raise KeyError, masking the original exception and leaving dictionary keys in the wrong order.

1 open issue
1 resolved since last review

Review effort: Balanced

Comment thread jsonpatch.py Resolved
The keys of an object are recorded before its first removal, so they
include members that the patch added before. The rollback removes these
again, and restoring the recorded order then raised KeyError, which hid
the original exception and left the members out of order. Leave out
the keys that are no longer present.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PewzpjP8bGvTXBq5LST6Xh

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Human assessment is needed because an unpublished critical finding shows bounded-deque rollback can lose data or mask the original patch exception.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

A deque with a maxlen drops its first item when appending to it while
it is full. The undo step only deleted the appended item, so the dropped
one was lost, and after further changes the recorded indices could be
out of range, so that the rollback raised IndexError and hid the
original exception. Record the dropped item and put it back at the
front when undoing such an append.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PewzpjP8bGvTXBq5LST6Xh

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Human review is needed because atomic rollback can corrupt or fail to restore supported mappings, while per-instance custom operation overrides may be silently bypassed.

No open issues 🎉

Review effort: Balanced

Restoring the order of a mapping's members with clear() and update()
added the members that a ChainMap takes from its other mappings to its
first one. Only restore the order of dicts (and dict subclasses), whose
order is the order of insertion; other mappings keep the members that
the rollback restores in the order they then have.

Operations that set apply, _apply or _add on the instance were treated
as journal-aware, so atomic=True bypassed the instance's apply. Use the
copy-and-restore path for them, which calls it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PewzpjP8bGvTXBq5LST6Xh
The patches of docs_with_patches draw all their locations from the
original document, and most of them fail at an early operation, so in
only about 30% of the cases rolling back undid any change, and in 14%
more than one.

docs_with_failing_patches draws each operation against the document as
the preceding ones left it, mixing in operations that refer to existing
locations, keeps only operations that succeed, and ends with one that
fails. 84% of its cases undo changes, 56% more than one. The new
property checks that the document is unchanged afterwards, including
member order and the identity of its containers, and that the patch
without its failing operation gives the same result as on a copy.

It detects the add-before-remove bug fixed in 3fad0a5 without an
explicit example, which test_atomic_in_place_gives_same_result did not.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PewzpjP8bGvTXBq5LST6Xh

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Human assessment is needed because critical rollback failures can corrupt Counter contents and alter ChainMap layering, even when a failing operation makes no changes.

No open issues 🎉

Review effort: Balanced

Restoring the member order of a dict passed its items to update(),
which a Counter uses to count them, so its contents were replaced by
counts of the (key, value) pairs. Move the members from the first
misplaced one on to the end with pop() and item assignment instead,
the operations the rollback itself uses.

Setting a member that a ChainMap takes from another of its mappings
adds it to the first one; undoing it set the old value there instead of
removing it again. When restoring a ChainMap from a copy, restore each
of its mappings, so that the first one doesn't take over the others'
members.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PewzpjP8bGvTXBq5LST6Xh

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Atomic rollback fails when a custom operation removes a ChainMap layer and raises, leaving the document incompletely restored.

1 open issue

Review effort: Balanced

Comment thread jsonpatch.py Resolved
Exact restoration, with the same values, member order and container
objects, is guaranteed for documents of plain dicts and lists, which
includes every document parsed from JSON. Other containers are restored
through their own item operations. Besides containers that change when
they are read, document that the copy that patches with custom
operations are restored from only restores the items of mappings and
sequences, not other state such as attributes or a ChainMap's maps.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PewzpjP8bGvTXBq5LST6Xh

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Human assessment is needed because the published review omits a moderate-severity finding about incomplete state restoration during nested ChainMap rollback.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@stefankoegl
stefankoegl merged commit 36e54f0 into master Nov 4, 2026
23 checks passed
@johanneswilm

Copy link
Copy Markdown

great!

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

Successfully merging this pull request may close these issues.

atomic patch application

5 participants