Skip to content

Preserve one-shot operation iterators during patch validation - #234

Open
Apeksha9389 wants to merge 2 commits into
stefankoegl:masterfrom
Apeksha9389:fix/preserve-operation-iterators
Open

Apeksha9389 wants to merge 2 commits into
stefankoegl:masterfrom
Apeksha9389:fix/preserve-operation-iterators

Conversation

@Apeksha9389

Copy link
Copy Markdown
Contributor

JsonPatch(iter(operations)).apply(document) currently applies no operations: construction validates and exhausts the iterator before application can read it. Empty iterators also produce a truthy patch, and serializing an iterator-backed patch fails.

Materialize one-shot iterators before validation. Existing list and tuple inputs retain their container behavior. Regression tests cover list iterators and generators, repeated application and serialization, empty iterators, invalid operations, and preservation of the original list container.

Validation on Python 3.12.14: 217 unit tests and 20 Hypothesis property tests pass. The iterator and empty-iterator regressions produce three failures without the fix. Ruff syntax/undefined-name checks and git diff --check pass. Windows and the other Python versions were not run locally.

AI disclosure: This patch, tests, and description were prepared with OpenAI Codex assistance. The reported checks were executed locally.

@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 100.0%. remained the same — Apeksha9389:fix/preserve-operation-iterators into stefankoegl:master

@stefankoegl stefankoegl left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the fix! Materializing one-shot input before validation is the right approach, and plain iterators and generators now work correctly. The test suite passes on this head.

Before merging, I'd ask for the change suggested on jsonpatch.py:519: check for Sequence instead of Iterator, so that one-shot iterables without __next__ are covered too. Inline comments have the details. Two more points that aren't on changed lines:

  • Docs: the apply_patch docstring still says :type patch: list or str, and the JsonPatch class docstring and doc/tutorial.rst only describe building a patch from a list. If iterators and generators are now supported, it would help to say so.
  • Optional, not blocking: __init__ builds every operation object only to validate it, then throws them away, and _ops rebuilds them on every apply, __eq__ and __hash__. Since iterator input is now copied anyway, the validated operations could be stored and reused. This behavior predates this PR, so it's fine to leave it for a follow-up.

Generated by Claude Code

Comment thread jsonpatch.py Outdated
Comment thread tests.py Outdated
Comment thread tests.py Outdated

@stefankoegl stefankoegl left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the quick turnaround! 504f40e addresses all the earlier comments:

  • Sequence check instead of Iterator ✅, with a test for a one-shot iterable that has no __next__ ✅
  • the assertIs identity test is replaced by content and application checks for lists and tuples ✅
  • named subtests ✅
  • apply_patch and JsonPatch docstrings and doc/tutorial.rst updated ✅

Still blocking: CI is red on every Python version. That isn't a flaw in this commit itself. #236 (merged to master yesterday) restructured the same constructor, and the merged code skips validation for one-shot inputs. Details and a tested fix are inline. Once master is merged in and the copy moved below the type checks, this looks ready to me.

Optional nit: the collections.abc import is now only rewrapped in parentheses with the same names, so it could go back to its original single line to keep the diff minimal.


Generated by Claude Code

Comment thread jsonpatch.py

def __init__(self, patch, pointer_cls=JsonPointer):
self.patch = patch
self.patch = patch if isinstance(patch, Sequence) else list(patch)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This line is right on its own, but it now clashes with #236, which merged into master after this branch was created. CI tests the merge with master, and that's why every build job is red.

On master, __init__ now checks the input type after self.patch = patch and then validates by iterating the original patch:

self.patch = patch
...
if isinstance(patch, (str, bytes, Mapping)):
    raise InvalidJsonPatch(...)
try:
    patch_iter = iter(patch)
except TypeError:
    raise InvalidJsonPatch(...)
for op in patch_iter:
    ...

Git merges the two without a conflict, but the result has two problems:

  1. list(patch) runs before the type checks, so non-iterable input like JsonPatch.from_string('1') raises TypeError instead of InvalidJsonPatch. That accounts for the 18 errors in test_serialized_patch_rejects_non_array_documents.
  2. More seriously, list(patch) has already used up a one-shot input, so iter(patch) yields nothing and no operation gets validated. test_invalid_operation_iterator_is_rejected catches this (good test!). An invalid iterator is accepted, and the error only appears later in apply().

Moving the copy after the type checks and validating self.patch fixes both:

    def __init__(self, patch, pointer_cls=JsonPointer):
        self.pointer_cls = pointer_cls

        if isinstance(patch, (str, bytes, Mapping)):
            raise InvalidJsonPatch("Document is expected to be sequence of operations")
        try:
            patch_iter = iter(patch)
        except TypeError:
            raise InvalidJsonPatch("Document is expected to be sequence of operations")

        # One-shot iterables are materialized, so that validating them does
        # not leave nothing to apply
        self.patch = patch if isinstance(patch, Sequence) else list(patch_iter)

        # Verify that the structure of the patch document
        # ...
        for op in self.patch:

I tried this on top of this branch merged with current master (e1d055d). All 227 tests and the module doctests pass. Could you merge master into the branch and apply it?


Generated by Claude Code

This branch has not been deployed

No deployments
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.

3 participants