Repository navigation
Add opt-in JsonPatchWithAppend for string appends - #171
jeonsworld wants to merge 5 commits into
Conversation
- Add new `append` operation as an optional extension to RFC 6902 - Introduce `use_append_ops` parameter to enable/disable append operations - Support automatic detection of string append scenarios in make_patch() - Maintain full backward compatibility with RFC 6902 by default - Enable optimized notation for consecutive append operations in streaming The append operation is disabled by default to maintain RFC 6902 compliance. Users can opt-in by setting use_append_ops=True in apply_patch(), make_patch(), and JsonPatch methods. This feature is particularly useful for chat applications and streaming scenarios where text is appended incrementally, reducing payload size by ~2.8x compared to replace operations.
|
@stefankoegl This PR is resubmitted with the append operation made optional for RFC 6902 compliance. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new append operation to python-json-patch specifically optimized for text streaming in chat applications. When streaming text character-by-character or word-by-word, this reduces payload size by only sending incremental text instead of the entire string with each update.
Key changes include:
- New
AppendOperationclass for string concatenation - Optional RFC 6902 extension controlled via
use_append_opsparameter - Smart patch generation that automatically detects append scenarios when enabled
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| else: | ||
| self._item_replaced(path, key, dst) | ||
| # Check if this is a string append operation (only if append ops are enabled) | ||
| if self.use_append_ops and isinstance(src, basestring) and isinstance(dst, basestring) and dst.startswith( | ||
| src): | ||
| appended_text = dst[len(src):] | ||
| if appended_text: # Only create append op if there's actual text to append | ||
| self.insert(AppendOperation({ | ||
| 'op': 'append', | ||
| 'path': _path_join(path, key), | ||
| 'value': appended_text, | ||
| }, pointer_cls=self.pointer_cls)) | ||
| else: | ||
| self._item_replaced(path, key, dst) | ||
| else: | ||
| self._item_replaced(path, key, dst) |
There was a problem hiding this comment.
The logic flow is incorrect - there's an unnecessary else: block at line 1018 that creates unreachable code. The else: clause at line 1031 will never execute because it's nested inside the outer else: block, making the string append detection logic unreachable in most cases.
| raise JsonPatchConflict("Cannot append to root document") | ||
|
|
||
| try: | ||
| if isinstance(subobj[part], basestring): |
There was a problem hiding this comment.
The append operation should validate that the 'value' parameter is a string before attempting concatenation. Currently, if 'value' is not a string, the += operation could succeed but produce unexpected results (e.g., concatenating a list to a string).
| if isinstance(subobj[part], basestring): | |
| if isinstance(subobj[part], basestring): | |
| if not isinstance(self.operation['value'], basestring): | |
| raise JsonPatchConflict("Cannot append non-string value to string") |
|
I don't like to idea of mixing in features that do not appear in RFC 6902 in the main interfaces. Instead I would prefer a solution that would keep them separate, eg a What do you think? |
796ec31 to
1014d15
Compare
jeonsworld
changed the title
|
@stefankoegl Updated the PR to introduce |
Adds
JsonPatchWithAppend, a drop-in subclass for opt-in string appends, while preserving the standardJsonPatch, module-level helpers, and CLI behavior.op,path, andvaluefields with existing parsing, serialization, and atomic rollback behavior.Local validation: 311 unit/doctest tests and 26 property tests passed, along with Flake8, Sphinx, distribution checks, and installed-wheel type checks.