diff --git a/docx_editor/comments.py b/docx_editor/comments.py index 8080b3d..e889155 100644 --- a/docx_editor/comments.py +++ b/docx_editor/comments.py @@ -24,6 +24,7 @@ TextMapMatch, _escape_xml, _generate_hex_id, + _require_valid_occurrence, build_text_map, compute_paragraph_hash, count_in_text_map, @@ -155,10 +156,16 @@ def add_comment( ``anchor_text`` matches more than once in the search scope. HashMismatchError: If a paragraph reference's hash is stale. ParagraphIndexError: If a paragraph reference's index is out of range. - CommentError: If ``anchor_text`` is empty. + CommentError: If ``anchor_text`` is not a non-empty string, or + ``comment_text`` is not a string. + ValueError: If ``occurrence`` is negative or not an integer. """ - if not anchor_text: - raise CommentError("anchor_text must not be empty") + if not isinstance(anchor_text, str) or not anchor_text: + raise CommentError(f"anchor_text must be a non-empty string, got {anchor_text!r}") + if not isinstance(comment_text, str): + # Must fail here, before _place_markers mutates document.xml — + # a crash later (html.escape) would leave orphaned range markers. + raise CommentError(f"comment_text must be a string, got {comment_text!r}") _, match = self._locate_anchor(anchor_text, paragraph, occurrence) comment_id = self.next_comment_id @@ -190,11 +197,10 @@ def _locate_anchor( ``_locate_document_wide`` so comment anchors and edit anchors find the same text and fail the same way: ``occurrence=None`` requires a unique anchor (else AmbiguousTextError), an explicit out-of-range occurrence - reports the actual total, and a negative occurrence is rejected with - ValueError. + reports the actual total, and a negative or non-int occurrence is + rejected with ValueError. """ - if occurrence is not None and occurrence < 0: - raise ValueError(f"occurrence must be >= 0, got {occurrence}") + _require_valid_occurrence(occurrence) occ = occurrence if occurrence is not None else 0 if paragraph is not None: diff --git a/docx_editor/document.py b/docx_editor/document.py index d1159ae..f8d1bce 100644 --- a/docx_editor/document.py +++ b/docx_editor/document.py @@ -40,6 +40,14 @@ ) +def _require_ref_string(paragraph: str) -> None: + """Reject non-string paragraph refs before they can silently select the + RevisionManager's document-wide search branch (its ``paragraph=None`` + mode is intentional at that layer, not at this one).""" + if not isinstance(paragraph, str): + raise ValueError(f"'paragraph' must be a paragraph ref string like 'P3#a7b2', got {type(paragraph).__name__}") + + class Document: """Word document with track changes and comment support. @@ -729,6 +737,9 @@ def replace(self, find: str, replace_with: str, *, paragraph: str, occurrence: i for accept_group()/reject_group(). Raises: + ValueError: If ``find`` is not a non-empty string, + ``replace_with`` is not a string, ``paragraph`` is not a ref + string, or ``occurrence`` is negative or not an integer. TextNotFoundError: If ``find`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``find`` @@ -740,6 +751,7 @@ def replace(self, find: str, replace_with: str, *, paragraph: str, occurrence: i doc.replace("other text", "new text", paragraph=new_ref) """ self._ensure_open() + _require_ref_string(paragraph) change_id = self._revision_manager.replace_text(find, replace_with, occurrence=occurrence, paragraph=paragraph) return self._edit_result(paragraph, self._revision_manager.group_id_of(change_id)) @@ -759,6 +771,8 @@ def delete(self, text: str, *, paragraph: str, occurrence: int | None = None) -> revisions this edit created. Raises: + ValueError: If ``text`` is not a non-empty string, ``paragraph`` + is not a ref string, or ``occurrence`` is negative or not an integer. TextNotFoundError: If ``text`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``text`` @@ -769,6 +783,7 @@ def delete(self, text: str, *, paragraph: str, occurrence: int | None = None) -> new_ref = doc.delete("obsolete clause", paragraph="P2#f3c1") """ self._ensure_open() + _require_ref_string(paragraph) change_id = self._revision_manager.suggest_deletion(text, occurrence=occurrence, paragraph=paragraph) return self._edit_result(paragraph, self._revision_manager.group_id_of(change_id)) @@ -789,6 +804,9 @@ def insert_after(self, anchor: str, text: str, *, paragraph: str, occurrence: in revisions this edit created. Raises: + ValueError: If ``anchor`` is not a non-empty string, ``text`` is + not a string, ``paragraph`` is not a ref string, or + ``occurrence`` is negative or not an integer. TextNotFoundError: If ``anchor`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` @@ -799,6 +817,7 @@ def insert_after(self, anchor: str, text: str, *, paragraph: str, occurrence: in new_ref = doc.insert_after("Section 5", " (as amended)", paragraph="P2#f3c1") """ self._ensure_open() + _require_ref_string(paragraph) change_id = self._revision_manager.insert_text_after(anchor, text, occurrence=occurrence, paragraph=paragraph) return self._edit_result(paragraph, self._revision_manager.group_id_of(change_id)) @@ -819,6 +838,9 @@ def insert_before(self, anchor: str, text: str, *, paragraph: str, occurrence: i revisions this edit created. Raises: + ValueError: If ``anchor`` is not a non-empty string, ``text`` is + not a string, ``paragraph`` is not a ref string, or + ``occurrence`` is negative or not an integer. TextNotFoundError: If ``anchor`` is absent or ``occurrence`` is out of range for the paragraph. AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` @@ -829,6 +851,7 @@ def insert_before(self, anchor: str, text: str, *, paragraph: str, occurrence: i new_ref = doc.insert_before("Section 6", "New clause: ", paragraph="P2#f3c1") """ self._ensure_open() + _require_ref_string(paragraph) change_id = self._revision_manager.insert_text_before(anchor, text, occurrence=occurrence, paragraph=paragraph) return self._edit_result(paragraph, self._revision_manager.group_id_of(change_id)) @@ -867,8 +890,9 @@ def batch_edit( Raises: BatchOperationError: The only exception a non-dry-run batch raises - for a failing operation — validation (malformed ref, stale - hash, bad index) and apply (missing text, ambiguous target) + for a failing operation — validation (element is not an + EditOperation, malformed ref, stale hash, bad index) and apply + (missing text, ambiguous target) failures alike. ``operation_index`` names the failing op and ``original`` (also ``__cause__``) holds the underlying typed exception (e.g. a HashMismatchError with ``actual_hash``). @@ -993,6 +1017,9 @@ def add_comment( AmbiguousTextError: If ``occurrence`` is omitted and ``anchor_text`` matches more than once in the search scope. HashMismatchError: If ``paragraph``'s hash is stale. + CommentError: If ``anchor_text`` is not a non-empty string, or + ``comment`` is not a string. + ValueError: If ``occurrence`` is negative or not an integer. Example: doc.add_comment("Section 5", "Please review this section") diff --git a/docx_editor/track_changes.py b/docx_editor/track_changes.py index 76a721a..ba59284 100644 --- a/docx_editor/track_changes.py +++ b/docx_editor/track_changes.py @@ -29,6 +29,7 @@ TextMapMatch, TextPosition, _escape_xml, + _require_valid_occurrence, build_text_map, compute_paragraph_hash, compute_text_hash, @@ -64,8 +65,7 @@ class EditOperation: def _validate_common(constructor: str, paragraph: str, occurrence: int | None) -> None: """Construction-time checks shared by all typed constructors.""" ParagraphRef.parse(paragraph) - if occurrence is not None and occurrence < 0: - raise ValueError(f"EditOperation.{constructor}(): occurrence must be >= 0, got {occurrence}") + _require_valid_occurrence(occurrence, f"EditOperation.{constructor}(): ") @classmethod def replace(cls, find: str, replace_with: str, *, paragraph: str, occurrence: int | None = None) -> "EditOperation": @@ -82,13 +82,19 @@ def replace(cls, find: str, replace_with: str, *, paragraph: str, occurrence: in Raises: ValueError: If the paragraph ref is malformed, ``occurrence`` is - negative, ``find`` is empty, or ``replace_with`` is None. + not a non-negative integer, ``find`` is not a non-empty + string, or ``replace_with`` is not a string. """ cls._validate_common("replace", paragraph, occurrence) - if not find: - raise ValueError("EditOperation.replace(): 'find' must be a non-empty string — the text to search for") - if replace_with is None: - raise ValueError("EditOperation.replace(): 'replace_with' must be a string (empty string is allowed)") + if not isinstance(find, str) or not find: + raise ValueError( + f"EditOperation.replace(): 'find' must be a non-empty string — the text to search for, got {find!r}" + ) + if not isinstance(replace_with, str): + raise ValueError( + f"EditOperation.replace(): 'replace_with' must be a string (empty string is allowed), " + f"got {replace_with!r}" + ) return cls( action="replace", paragraph=paragraph, @@ -110,11 +116,14 @@ def delete(cls, text: str, *, paragraph: str, occurrence: int | None = None) -> Raises: ValueError: If the paragraph ref is malformed, ``occurrence`` is - negative, or ``text`` is empty. + not a non-negative integer, or ``text`` is not a non-empty + string. """ cls._validate_common("delete", paragraph, occurrence) - if not text: - raise ValueError("EditOperation.delete(): 'text' must be a non-empty string — the text to mark as deleted") + if not isinstance(text, str) or not text: + raise ValueError( + f"EditOperation.delete(): 'text' must be a non-empty string — the text to mark as deleted, got {text!r}" + ) return cls(action="delete", paragraph=paragraph, text=text, occurrence=occurrence) @classmethod @@ -127,10 +136,15 @@ def _insert( occurrence: int | None, ) -> "EditOperation": cls._validate_common(action, paragraph, occurrence) - if not anchor: - raise ValueError(f"EditOperation.{action}(): 'anchor' must be a non-empty string — the text to insert near") - if text is None: - raise ValueError(f"EditOperation.{action}(): 'text' must be a string (empty string is allowed)") + if not isinstance(anchor, str) or not anchor: + raise ValueError( + f"EditOperation.{action}(): 'anchor' must be a non-empty string — the text to insert near, " + f"got {anchor!r}" + ) + if not isinstance(text, str): + raise ValueError( + f"EditOperation.{action}(): 'text' must be a string (empty string is allowed), got {text!r}" + ) return cls(action=action, paragraph=paragraph, anchor=anchor, text=text, occurrence=occurrence) @classmethod @@ -148,7 +162,8 @@ def insert_after(cls, anchor: str, text: str, *, paragraph: str, occurrence: int Raises: ValueError: If the paragraph ref is malformed, ``occurrence`` is - negative, ``anchor`` is empty, or ``text`` is None. + not a non-negative integer, ``anchor`` is not a non-empty + string, or ``text`` is not a string. """ return cls._insert("insert_after", anchor, text, paragraph, occurrence) @@ -167,11 +182,21 @@ def insert_before(cls, anchor: str, text: str, *, paragraph: str, occurrence: in Raises: ValueError: If the paragraph ref is malformed, ``occurrence`` is - negative, ``anchor`` is empty, or ``text`` is None. + not a non-negative integer, ``anchor`` is not a non-empty + string, or ``text`` is not a string. """ return cls._insert("insert_before", anchor, text, paragraph, occurrence) +def _not_an_edit_operation_message(op: object) -> str: + """Shared batch_edit/validate_batch message for a non-EditOperation element, + so the raising and never-raises paths cannot drift apart.""" + return ( + f"expected EditOperation, got {type(op).__name__} — build operations with " + "EditOperation.replace()/.delete()/.insert_after()/.insert_before()" + ) + + @dataclass class EditValidationResult: """Outcome of validating one EditOperation in a dry-run batch.""" @@ -608,14 +633,16 @@ def _locate_in_paragraph(self, paragraph, paragraph_ref: str, text: str, occurre be unique within the paragraph. Raises: - ValueError: If ``occurrence`` is negative. + ValueError: If ``occurrence`` is negative or not an integer, or ``text`` is not a + non-empty string. TextNotFoundError: If the text is absent, or ``occurrence`` is out of range (then with ``occurrence``/``total_occurrences`` set). AmbiguousTextError: If ``occurrence`` is None and the text matches more than once in the paragraph. """ - if occurrence is not None and occurrence < 0: - raise ValueError(f"occurrence must be >= 0, got {occurrence}") + _require_valid_occurrence(occurrence) + if not isinstance(text, str) or not text: + raise ValueError(f"search text must be a non-empty string, got {text!r}") text_map = build_text_map(paragraph) total = count_in_text_map(text_map, text) @@ -658,11 +685,12 @@ def batch_edit(self, operations: list[EditOperation]) -> list[int]: List of change IDs, one per operation (in original input order) Raises: - BatchOperationError: If any operation fails — validation (malformed - ref, stale hash, bad index) or apply (missing text, ambiguous - target). Carries ``operation_index`` so the caller knows which - op failed, and ``original`` (also ``__cause__``) with the - underlying typed exception. No edits are applied on failure. + BatchOperationError: If any operation fails — validation (element + is not an EditOperation, malformed ref, stale hash, bad index) + or apply (missing text, ambiguous target). Carries + ``operation_index`` so the caller knows which op failed, and + ``original`` (also ``__cause__``) with the underlying typed + exception. No edits are applied on failure. """ if not operations: return [] @@ -670,6 +698,8 @@ def batch_edit(self, operations: list[EditOperation]) -> list[int]: # Parse and validate all refs upfront parsed: list[tuple[int, ParagraphRef, EditOperation]] = [] for i, op in enumerate(operations): + if not isinstance(op, EditOperation): + raise BatchOperationError(i, _not_an_edit_operation_message(op)) if not op.paragraph: raise BatchOperationError(i, "paragraph reference is required for batch mode") try: @@ -721,23 +751,23 @@ def _resolve_action_target(self, op: EditOperation) -> str: so both paths fail cleanly before the search. Raises: - ValueError: If ``occurrence`` is negative, required arguments for - op.action are missing, or the action is unrecognized. + ValueError: If ``occurrence`` is negative or not an integer, required arguments for + op.action are missing or not strings, or the action is + unrecognized. """ - if op.occurrence is not None and op.occurrence < 0: - raise ValueError(f"occurrence must be >= 0, got {op.occurrence}") + _require_valid_occurrence(op.occurrence) if op.action == "replace": - if not op.find or op.replace_with is None: - raise ValueError("replace requires 'find' and 'replace_with'") + if not op.find or not isinstance(op.replace_with, str): + raise ValueError("replace requires 'find' and a string 'replace_with'") return op.find elif op.action == "delete": if not op.text: raise ValueError("delete requires 'text'") return op.text elif op.action in ("insert_after", "insert_before"): - if not op.anchor or op.text is None: - raise ValueError(f"{op.action} requires 'anchor' and 'text'") + if not op.anchor or not isinstance(op.text, str): + raise ValueError(f"{op.action} requires 'anchor' and a string 'text'") return op.anchor else: raise ValueError(f"Unknown action: {op.action}") @@ -780,10 +810,17 @@ def validate_batch(self, operations: list[EditOperation]) -> list[EditValidation operations: List of EditOperation objects (each should have paragraph set) Returns: - One EditValidationResult per operation, in input order. + One EditValidationResult per operation, in input order. An element + that is not an EditOperation at all comes back as an invalid + result (``paragraph=None``), never as an exception. """ results = [] for i, op in enumerate(operations): + if not isinstance(op, EditOperation): + results.append( + EditValidationResult(index=i, paragraph=None, valid=False, error=_not_an_edit_operation_message(op)) + ) + continue error = self._validate_single(op) results.append( EditValidationResult( @@ -828,7 +865,7 @@ def _validate_single(self, op: EditOperation) -> str | None: try: self._locate_in_paragraph(p, op.paragraph, target, op.occurrence) - except DocxEditError as e: + except (ValueError, DocxEditError) as e: return str(e) return None @@ -1122,14 +1159,16 @@ def _locate_document_wide(self, text: str, occurrence: int | None = None) -> Tex the exact total is part of the error contract. Raises: - ValueError: If ``occurrence`` is negative. + ValueError: If ``occurrence`` is negative or not an integer, or ``text`` is not a + non-empty string. TextNotFoundError: If the text is not found or occurrence doesn't exist; ``total_occurrences`` matches :meth:`count_matches`. AmbiguousTextError: If ``occurrence`` is None and the text matches more than once in the document. """ - if occurrence is not None and occurrence < 0: - raise ValueError(f"occurrence must be >= 0, got {occurrence}") + _require_valid_occurrence(occurrence) + if not isinstance(text, str) or not text: + raise ValueError(f"search text must be a non-empty string, got {text!r}") occ = occurrence if occurrence is not None else 0 match = self._find_across_boundaries(text, occ) if match is None: @@ -1269,11 +1308,15 @@ def replace_text( The change ID of the insertion Raises: + ValueError: If ``find`` is not a non-empty string, ``replace_with`` + is not a string, or ``occurrence`` is negative or not an integer TextNotFoundError: If the text is not found or occurrence doesn't exist AmbiguousTextError: If ``occurrence`` is omitted and ``find`` matches more than once in the search scope HashMismatchError: If the paragraph hash doesn't match """ + if not isinstance(replace_with, str): + raise ValueError(f"'replace_with' must be a string (empty string is allowed), got {replace_with!r}") with self._grouped(): if paragraph is not None: ref = ParagraphRef.parse(paragraph) @@ -1298,6 +1341,8 @@ def suggest_deletion(self, text: str, occurrence: int | None = None, paragraph: The change ID of the deletion Raises: + ValueError: If ``text`` is not a non-empty string, or + ``occurrence`` is negative or not an integer TextNotFoundError: If the text is not found or occurrence doesn't exist AmbiguousTextError: If ``occurrence`` is omitted and ``text`` matches more than once in the search scope @@ -2133,6 +2178,8 @@ def insert_text_after( The change ID of the insertion Raises: + ValueError: If ``anchor`` is not a non-empty string, ``text`` is + not a string, or ``occurrence`` is negative or not an integer TextNotFoundError: If the anchor text is not found or occurrence doesn't exist AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` matches more than once in the search scope @@ -2157,6 +2204,8 @@ def insert_text_before( The change ID of the insertion Raises: + ValueError: If ``anchor`` is not a non-empty string, ``text`` is + not a string, or ``occurrence`` is negative or not an integer TextNotFoundError: If the anchor text is not found or occurrence doesn't exist AmbiguousTextError: If ``occurrence`` is omitted and ``anchor`` matches more than once in the search scope @@ -2173,6 +2222,8 @@ def _insert_text( paragraph: str | None = None, ) -> int: """Insert text before or after anchor with tracked changes.""" + if not isinstance(text, str): + raise ValueError(f"'text' must be a string (empty string is allowed), got {text!r}") with self._grouped(): if paragraph is not None: ref = ParagraphRef.parse(paragraph) diff --git a/docx_editor/xml_editor.py b/docx_editor/xml_editor.py index 0e58908..5ceadcc 100644 --- a/docx_editor/xml_editor.py +++ b/docx_editor/xml_editor.py @@ -60,6 +60,18 @@ class TextMapMatch: spans_boundary: bool # True if match spans different revision contexts +def _require_valid_occurrence(occurrence: int | None, label: str = "") -> None: + """Reject a non-int or negative ``occurrence`` before it can hit an int + comparison (raw TypeError from ``"0" < 0``), TypeError deeper in the + search (float), or be silently misread as 0/1 (bool).""" + if occurrence is None: + return + if isinstance(occurrence, bool) or not isinstance(occurrence, int): + raise ValueError(f"{label}occurrence must be a non-negative integer, got {occurrence!r}") + if occurrence < 0: + raise ValueError(f"{label}occurrence must be >= 0, got {occurrence}") + + def find_in_text_map(text_map: TextMap, search: str, occurrence: int = 0) -> TextMapMatch | None: """Find the nth occurrence of text in a text map. @@ -122,8 +134,13 @@ def parse(cls, ref: str) -> "ParagraphRef": """Parse a paragraph reference string like 'P3#a7b2'. Raises: - ValueError: If the reference format is invalid + ValueError: If the reference is not a string or its format is + invalid """ + if not isinstance(ref, str): + raise ValueError( + f"Invalid paragraph reference {ref!r}: expected a string like 'P3#a7b2', got {type(ref).__name__}" + ) m = _PARAGRAPH_REF_RE.match(ref) if not m: raise ValueError( diff --git a/skills/docx/SKILL.md b/skills/docx/SKILL.md index aab890e..958845d 100644 --- a/skills/docx/SKILL.md +++ b/skills/docx/SKILL.md @@ -306,7 +306,7 @@ doc.close() **Multi-author documents:** Editing inside *another* author's pending insertion preserves their proposal, matching Word: deletions nest a `` under your authorship inside their ``, and replacements/insertions put your text in your own sibling `` (splitting theirs when needed) instead of silently rewriting it. `accept_all(author=...)` / `reject_all(author=...)` then resolve each author's changes independently. Only your own pending insertions are edited in place. -**Raises:** `TextNotFoundError` if the text is not found (or the requested `occurrence` is out of range — the error then reports `total_occurrences`). `AmbiguousTextError` if `occurrence` is omitted and the target matches more than once in the search scope. +**Raises:** `TextNotFoundError` if the text is not found (or the requested `occurrence` is out of range — the error then reports `total_occurrences`). `AmbiguousTextError` if `occurrence` is omitted and the target matches more than once in the search scope. `ValueError` for search/anchor text that is not a non-empty string, replacement/insertion text that is not a string, a non-string `paragraph` ref, or an `occurrence` that is not a non-negative integer — all rejected up front, before any change is made. ### Per-section change report @@ -706,7 +706,7 @@ All LLM-facing errors inherit from `DocxEditError` and (except `RevisionError`, | `TextNotFoundError` | `search_text`, `paragraph_ref`, `paragraph_preview`, `occurrence`, `total_occurrences` | Use `paragraph_preview` to pick a substring that actually appears; if `total_occurrences` is set, retry with an `occurrence` < `total_occurrences`. | | `AmbiguousTextError` | `search_text`, `paragraph_ref`, `paragraph_preview`, `total_occurrences` | The target matched more than once with no `occurrence` given. Enumerate with `find_all()` and pick, or pass an explicit `occurrence` (0-based). | | `ParagraphIndexError` | `index`, `total_paragraphs` | Clamp to `1..total_paragraphs` or call `list_paragraphs()` to pick a valid ref. | -| `BatchOperationError` | `operation_index`, `reason`, `original` | Fix the op at `operations[operation_index]` (or drop it) and retry the batch; `original` is the underlying typed error (e.g. use its `actual_hash` to re-target a stale ref), or None for batch-level rules with no underlying exception (a missing paragraph ref, or a duplicate paragraph in `batch_rewrite` — `batch_edit` allows repeats, applied sequentially). Batch methods never raise the inner types directly. | +| `BatchOperationError` | `operation_index`, `reason`, `original` | Fix the op at `operations[operation_index]` (or drop it) and retry the batch; `original` is the underlying typed error (e.g. use its `actual_hash` to re-target a stale ref), or None for batch-level rules with no underlying exception (a missing paragraph ref, an element that is not an `EditOperation`, or a duplicate paragraph in `batch_rewrite` — `batch_edit` allows repeats, applied sequentially). Batch methods never raise the inner types directly. | | `RevisionError` | (message only) | Unknown `group_id` passed to `accept_group()`/`reject_group()`. Groups are in-memory and per-open-Document: use the `EditResult.group_id` you were handed in this session; after `close()`/reopen resolve by revision id or author instead. | | `DocumentOpenError` | `path`, `owner_file` | **Do not retry blindly.** The destination is open in Word. Stop and tell the user to close it. Only pass `force=True` if the user confirms the `~$` lock is stale (crashed session). | diff --git a/tests/test_batch_edit.py b/tests/test_batch_edit.py index 8ab01ba..a54f166 100644 --- a/tests/test_batch_edit.py +++ b/tests/test_batch_edit.py @@ -1049,6 +1049,34 @@ def test_empty_paragraph_ref_rejected(self): with pytest.raises(ValueError, match="Invalid paragraph reference"): EditOperation.delete("a", paragraph="") + def test_none_paragraph_ref_rejected(self): + """paragraph=None gets the field-specific ValueError, not a raw regex TypeError.""" + with pytest.raises(ValueError, match="Invalid paragraph reference None"): + EditOperation.replace("a", "b", paragraph=None) # type: ignore[arg-type] + + def test_delete_none_paragraph_ref_rejected(self): + with pytest.raises(ValueError, match="Invalid paragraph reference None"): + EditOperation.delete("x", paragraph=None) # type: ignore[arg-type] + + def test_non_string_paragraph_ref_names_the_type(self): + with pytest.raises(ValueError, match="expected a string like 'P3#a7b2', got int"): + EditOperation.insert_after("a", "x", paragraph=3) # type: ignore[arg-type] + + def test_non_string_search_text_rejected(self): + """A truthy non-string target fails at construction, not deep in the search.""" + with pytest.raises(ValueError, match=r"'find' must be a non-empty string"): + EditOperation.replace(123, "x", paragraph="P2#f3c1") # type: ignore[arg-type] + with pytest.raises(ValueError, match=r"'text' must be a non-empty string"): + EditOperation.delete(123, paragraph="P2#f3c1") # type: ignore[arg-type] + with pytest.raises(ValueError, match=r"'anchor' must be a non-empty string"): + EditOperation.insert_after(123, "x", paragraph="P2#f3c1") # type: ignore[arg-type] + + def test_non_string_payload_rejected(self): + with pytest.raises(ValueError, match=r"'replace_with' must be a string"): + EditOperation.replace("a", 123, paragraph="P2#f3c1") # type: ignore[arg-type] + with pytest.raises(ValueError, match=r"'text' must be a string"): + EditOperation.insert_before("a", 123, paragraph="P2#f3c1") # type: ignore[arg-type] + def test_negative_occurrence_rejected(self): with pytest.raises(ValueError, match=r"EditOperation\.replace\(\): occurrence must be >= 0, got -1"): EditOperation.replace("a", "b", paragraph="P2#f3c1", occurrence=-1) diff --git a/tests/test_error_quality.py b/tests/test_error_quality.py index 4786a1c..3f644db 100644 --- a/tests/test_error_quality.py +++ b/tests/test_error_quality.py @@ -235,6 +235,59 @@ def test_inner_missing_field_failure_carries_operation_index(self, doc_path): finally: doc.close() + def test_non_editoperation_element_rejected_with_index(self, doc_path): + """A dict element is a batch-level rule violation, not a raw AttributeError.""" + doc = self._build_batch_doc(doc_path) + try: + before = doc.get_visible_text() + ops = [{"action": "replace", "find": "a", "replace_with": "b"}] + with pytest.raises(BatchOperationError) as exc: + doc.batch_edit(ops) # type: ignore[arg-type] + + err = exc.value + assert err.operation_index == 0 + assert err.original is None + assert "expected EditOperation, got dict" in err.reason + assert "EditOperation.replace()" in err.reason # names the recovery path + assert doc.get_visible_text() == before + finally: + doc.close() + + def test_non_editoperation_mixed_batch_names_offending_index(self, doc_path): + """Atomicity: the valid op at index 0 is not applied when index 1 is rejected.""" + doc = self._build_batch_doc(doc_path) + try: + before = doc.get_visible_text() + refs = doc.list_paragraphs() + valid = EditOperation.replace("Paragraph 1", "x", paragraph=refs[0].split("|")[0]) + with pytest.raises(BatchOperationError) as exc: + doc.batch_edit([valid, "not an op"]) # type: ignore[list-item] + + assert exc.value.operation_index == 1 + assert "expected EditOperation, got str" in exc.value.reason + assert doc.get_visible_text() == before + finally: + doc.close() + + def test_dry_run_reports_non_editoperation_without_raising(self, doc_path): + """validate_batch never raises — a bad element type comes back as an invalid row.""" + doc = self._build_batch_doc(doc_path) + try: + refs = doc.list_paragraphs() + valid = EditOperation.replace("Paragraph 1", "x", paragraph=refs[0].split("|")[0]) + results = doc.batch_edit([valid, {"action": "delete"}], dry_run=True) # type: ignore[list-item] + + assert len(results) == 2 + assert results[0].valid + bad = results[1] + assert bad.index == 1 + assert not bad.valid + assert bad.paragraph is None + assert bad.error is not None + assert "expected EditOperation, got dict" in bad.error + finally: + doc.close() + class TestAmbiguousTextErrorQuality: """ @@ -409,6 +462,302 @@ def test_all_paths_raise_value_error(self, doc_path): doc.close() +class TestEmptySearchTextRejected: + """ + Before: an empty search string slipped into ``find_in_text_map``, where + ``str.find("", start)`` matches at every position. With an explicit + ``occurrence`` the zero-width match carried no DOM positions, so the + apply helpers created zero revisions and the edit silently vanished; + without one, the caller got a meaningless AmbiguousTextError + ("'' matches N times"). + + After: both shared locate paths reject empty (and None) search text up + front with the same ValueError, before any change is made. + (``add_comment`` never had this bug — it pre-rejects empty anchors with + its own CommentError; pinned below so the asymmetry stays deliberate.) + """ + + def test_empty_find_with_occurrence_no_longer_a_silent_no_op(self, doc_path): + """The original regression: explicit occurrence=0 made the edit vanish.""" + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + with pytest.raises(ValueError, match="search text must be a non-empty string"): + doc.replace("", "TEXT", paragraph=ref, occurrence=0) + assert doc.list_revisions() == [] # nothing applied, nothing vanished + finally: + doc.close() + + def test_all_paths_raise_value_error(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + rm = doc._revision_manager + calls = [ + # scoped, no occurrence (was: meaningless AmbiguousTextError) + lambda: doc.replace("", "x", paragraph=ref), + lambda: doc.delete("", paragraph=ref), + lambda: doc.insert_after("", "x", paragraph=ref), + lambda: doc.insert_before("", "x", paragraph=ref), + # scoped, explicit occurrence (was: silent no-op) + lambda: doc.delete("", paragraph=ref, occurrence=0), + # document-wide RM paths, both failure modes + lambda: rm.replace_text("", "x"), + lambda: rm.replace_text("", "x", occurrence=0), + ] + for call in calls: + with pytest.raises(ValueError, match="search text must be a non-empty string"): + call() + assert doc.list_revisions() == [] + finally: + doc.close() + + def test_none_search_text_names_the_value(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + with pytest.raises(ValueError, match="search text must be a non-empty string, got None"): + doc.replace(None, "x", paragraph=ref) # type: ignore[arg-type] + finally: + doc.close() + + def test_add_comment_still_rejects_bad_anchors_its_own_way(self, doc_path): + """add_comment pre-rejects empty AND non-string anchors with CommentError, + not ValueError (its documented type) — and never a raw TypeError.""" + from docx_editor import CommentError + + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + with pytest.raises(CommentError, match="anchor_text must be a non-empty string, got ''"): + doc.add_comment("", "note", paragraph=ref) + with pytest.raises(CommentError, match="anchor_text must be a non-empty string, got 123"): + doc.add_comment(123, "note", paragraph=ref) # type: ignore[arg-type] + with pytest.raises(CommentError, match="anchor_text must be a non-empty string, got b'needle'"): + doc.add_comment(b"needle", "note") # type: ignore[arg-type] + finally: + doc.close() + + def test_add_comment_rejects_non_string_comment_before_mutating(self, doc_path): + """Before: comment_text=123 crashed in html.escape AFTER the range + markers were placed — orphaned commentRangeStart/End corruption. + After: rejected up front with CommentError, document untouched.""" + from docx_editor import CommentError + + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + with pytest.raises(CommentError, match="comment_text must be a string, got 123"): + doc.add_comment("needle", 123, paragraph=ref) # type: ignore[arg-type] + assert doc.list_comments() == [] + assert "commentRangeStart" not in doc._document_editor.dom.toxml() + finally: + doc.close() + + +class TestNonStringParagraphRefRejected: + """ + Before: ``Document.replace(..., paragraph=None)`` didn't error at all — + None slipped through the keyword-only ``paragraph: str`` signature into + the RevisionManager, silently selecting its document-wide search branch. + + After: the four Document edit methods reject non-string paragraph refs + up front. (RevisionManager keeps ``paragraph=None`` as its intended + document-wide mode.) + """ + + def test_all_edit_methods_reject_none(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + calls = [ + lambda: doc.replace("needle", "x", paragraph=None), # type: ignore[arg-type] + lambda: doc.delete("needle", paragraph=None), # type: ignore[arg-type] + lambda: doc.insert_after("needle", "x", paragraph=None), # type: ignore[arg-type] + lambda: doc.insert_before("needle", "x", paragraph=None), # type: ignore[arg-type] + ] + for call in calls: + with pytest.raises(ValueError, match="'paragraph' must be a paragraph ref string"): + call() + assert doc.list_revisions() == [] # the doc-wide fallback never fired + finally: + doc.close() + + +class TestNonIntOccurrenceRejected: + """ + Before: ``occurrence`` was the one input the type-hardening pass skipped. + ``occurrence="0"`` hit ``"0" < 0`` → raw TypeError on every path + (constructors, direct API, batch apply, dry-run, add_comment); + ``occurrence=1.5`` passed the ``< 0`` check and TypeErrored deeper in + the search; ``occurrence=True`` was silently misread as occurrence 1. + + After: a shared guard rejects non-int (including bool) occurrences with + the same ValueError at every boundary, before any comparison or search. + """ + + def test_direct_api_rejects_str_occurrence(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + with pytest.raises(ValueError, match="occurrence must be a non-negative integer, got '0'"): + doc.replace("needle", "x", paragraph=ref, occurrence="0") # type: ignore[arg-type] + assert doc.list_revisions() == [] + finally: + doc.close() + + def test_constructor_rejects_float_and_bool(self): + with pytest.raises( + ValueError, match=r"EditOperation\.replace\(\): occurrence must be a non-negative integer, got 1\.5" + ): + EditOperation.replace("a", "b", paragraph="P2#f3c1", occurrence=1.5) # type: ignore[arg-type] + with pytest.raises( + ValueError, match=r"EditOperation\.delete\(\): occurrence must be a non-negative integer, got True" + ): + EditOperation.delete("a", paragraph="P2#f3c1", occurrence=True) # type: ignore[arg-type] + + def test_batch_wraps_str_occurrence_not_raw_typeerror(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + before = doc.get_visible_text() + ref = doc.list_paragraphs()[0].split("|")[0] + op = EditOperation(action="replace", paragraph=ref, find="needle", replace_with="x", occurrence="0") # type: ignore[arg-type] + with pytest.raises(BatchOperationError) as exc: + doc.batch_edit([op]) + + assert exc.value.operation_index == 0 + assert "occurrence must be a non-negative integer" in exc.value.reason + assert doc.get_visible_text() == before + finally: + doc.close() + + def test_dry_run_reports_str_occurrence_as_invalid(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + op = EditOperation(action="replace", paragraph=ref, find="needle", replace_with="x", occurrence="0") # type: ignore[arg-type] + results = doc.batch_edit([op], dry_run=True) + + assert len(results) == 1 + assert not results[0].valid + assert results[0].error is not None + assert "occurrence must be a non-negative integer" in results[0].error + finally: + doc.close() + + def test_add_comment_rejects_str_occurrence(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + with pytest.raises(ValueError, match="occurrence must be a non-negative integer, got '0'"): + doc.add_comment("needle", "note", paragraph=ref, occurrence="0") # type: ignore[arg-type] + finally: + doc.close() + + +class TestNonStringSearchAndPayloadRejected: + """ + Before: a truthy non-string search target (``find=123``) slipped past + the falsiness checks into the text-map search and surfaced as a raw + ``TypeError`` from ``str.find`` — escaping both documented batch + contracts (``batch_edit`` raises only BatchOperationError; + ``validate_batch`` never raises). A non-string payload + (``replace_with=123``) was worse: dry-run reported the op as *valid*, + then apply crashed with a raw ``AttributeError``. + + After: search/anchor text must be a non-empty ``str`` and payloads must + be ``str`` at every boundary — direct API, typed constructors, batch + apply (wrapped with the op index), and dry-run (invalid row, no raise). + """ + + def test_direct_api_rejects_non_string_search_text(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + with pytest.raises(ValueError, match="search text must be a non-empty string, got 123"): + doc.replace(123, "x", paragraph=ref) # type: ignore[arg-type] + with pytest.raises(ValueError, match="search text must be a non-empty string, got b'needle'"): + doc.delete(b"needle", paragraph=ref) # type: ignore[arg-type] + assert doc.list_revisions() == [] + finally: + doc.close() + + def test_direct_api_rejects_non_string_payload(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + with pytest.raises(ValueError, match="'replace_with' must be a string"): + doc.replace("needle", 123, paragraph=ref) # type: ignore[arg-type] + with pytest.raises(ValueError, match="'text' must be a string"): + doc.insert_after("needle", 123, paragraph=ref) # type: ignore[arg-type] + with pytest.raises(ValueError, match="'text' must be a string"): + doc.insert_before("needle", 123, paragraph=ref) # type: ignore[arg-type] + assert doc.list_revisions() == [] + finally: + doc.close() + + def test_batch_wraps_non_string_target_not_raw_typeerror(self, doc_path): + """Hand-built op (bypassing the typed constructors) with an int find.""" + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + before = doc.get_visible_text() + ref = doc.list_paragraphs()[0].split("|")[0] + op = EditOperation(action="replace", paragraph=ref, find=123, replace_with="x") # type: ignore[arg-type] + with pytest.raises(BatchOperationError) as exc: + doc.batch_edit([op]) + + assert exc.value.operation_index == 0 + assert isinstance(exc.value.original, ValueError) + assert "search text must be a non-empty string" in exc.value.reason + assert doc.get_visible_text() == before + finally: + doc.close() + + def test_batch_wraps_non_string_payload_not_raw_attributeerror(self, doc_path): + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + before = doc.get_visible_text() + ref = doc.list_paragraphs()[0].split("|")[0] + op = EditOperation(action="replace", paragraph=ref, find="needle", replace_with=123) # type: ignore[arg-type] + with pytest.raises(BatchOperationError) as exc: + doc.batch_edit([op]) + + assert exc.value.operation_index == 0 + assert "a string 'replace_with'" in exc.value.reason + assert doc.get_visible_text() == before + finally: + doc.close() + + def test_dry_run_reports_non_string_target_as_invalid(self, doc_path): + """validate_batch's never-raises contract holds for an int find.""" + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + op = EditOperation(action="replace", paragraph=ref, find=123, replace_with="x") # type: ignore[arg-type] + results = doc.batch_edit([op], dry_run=True) + + assert len(results) == 1 + assert not results[0].valid + assert results[0].error is not None + assert "search text must be a non-empty string" in results[0].error + finally: + doc.close() + + def test_dry_run_reports_non_string_payload_as_invalid(self, doc_path): + """Before the fix this came back valid=True, then crashed at apply.""" + doc = _build_doc_with_paragraphs(doc_path, ["one needle only."]) + try: + ref = doc.list_paragraphs()[0].split("|")[0] + op = EditOperation(action="replace", paragraph=ref, find="needle", replace_with=123) # type: ignore[arg-type] + results = doc.batch_edit([op], dry_run=True) + + assert len(results) == 1 + assert not results[0].valid + assert results[0].error is not None + assert "a string 'replace_with'" in results[0].error + finally: + doc.close() + + class TestSharedBaseClass: """All structured errors inherit from `DocxEditError`.