From 152ae7c9380bf3a3061776609e4a609f124f5c8e Mon Sep 17 00:00:00 2001 From: Martin Thoma Date: Thu, 12 Nov 2026 18:42:20 +0100 Subject: [PATCH 1/3] BUG: Insert outline items in front of `before` and cover more of PdfWriter PdfWriter.add_outline_item(before=item) appended the new item at the end unless `item` was the last one of its level, and inserting in front of the first item lost the new item: it was counted in /Count, but not linked into the outline, as /First of the parent was not updated (ISO 32000-2:2020, 12.3.3, Table 150). Closes #4340. TreeObject.insert_child() looked for `before` starting at /Last and following /Next, so it only found the last child. It now starts at /First when `before` is given and updates /First when inserting in front of the first child. Appending without `before` still starts at /Last, so it stays O(1). A `before` that is no child of the parent still appends at the end. Add tests for PdfWriter branches without coverage so far: * add_outline_item() with `before` being None, the first, a middle, the last or the only item, without a page (no /A action), and with a PageObject, * the page_mode getter, add_annotation() with an invalid page type and clean_page() converting a named destination inside an /A action, * merge() with excluded_fields=None and with form fields that are not on any merged page, * reset_translation() for a reader the writer never copied from and for an IndirectObject, checked by appending the reader again and comparing the copied /Resources. Where the expected behavior comes from ISO 32000-2:2020, the docstring names the section. Co-Authored-By: Claude Opus 5.5 --- pypdf/generic/_data_structures.py | 74 ++++++---- tests/generic/test_data_structures.py | 5 +- tests/test_writer.py | 197 ++++++++++++++++++++++++++ 3 files changed, 242 insertions(+), 34 deletions(-) diff --git a/pypdf/generic/_data_structures.py b/pypdf/generic/_data_structures.py index 76498d7fe1..31bda14d79 100644 --- a/pypdf/generic/_data_structures.py +++ b/pypdf/generic/_data_structures.py @@ -895,7 +895,6 @@ def insert_child( assert child.indirect_reference is not None, "mypy" child_reference: IndirectObject = child.indirect_reference - prev: DictionaryObject | None if "/First" not in self: # no child yet self[NameObject("/First")] = child_reference self[NameObject("/Count")] = NumberObject(0) @@ -907,41 +906,54 @@ def insert_child( if "/Prev" in child_obj: del child_obj["/Prev"] return child_reference - prev = cast("DictionaryObject", self["/Last"]) + child_obj[NameObject("/Parent")] = self.indirect_reference - visited: set[int] = set() - while prev.indirect_reference != before: - prev_id = id(prev) - if prev_id in visited: + if before is not None: + # The children are linked from /First to /Last by /Next (ISO 32000-2:2020, 12.3.3). + node = cast("DictionaryObject", self["/First"]) + visited: set[int] = set() + while True: + if id(node) in visited: + raise LimitReachedError("Detected cycle in tree structure.") + visited.add(id(node)) + if node.indirect_reference == before: + self._link_child_before(child_obj, child_reference, node) + inc_parent_counter(self, child_obj.get("/Count", 1)) + return child_reference + if "/Next" not in node: + break + node = cast("DictionaryObject", node["/Next"]) + # `before` is no child of this node: append at the end. + + last = cast("DictionaryObject", self["/Last"]) + visited = set() + while "/Next" in last: + if id(last) in visited: raise LimitReachedError("Detected cycle in tree structure.") - visited.add(prev_id) - if "/Next" in prev: - prev = cast("TreeObject", prev["/Next"]) - continue - - # append at the end - prev[NameObject("/Next")] = cast("TreeObject", child_reference) - child_obj[NameObject("/Prev")] = prev.indirect_reference - child_obj[NameObject("/Parent")] = self.indirect_reference - if "/Next" in child_obj: - del child_obj["/Next"] - self[NameObject("/Last")] = child_reference - inc_parent_counter(self, child_obj.get("/Count", 1)) - return child_reference - try: # insert as first or in the middle - assert isinstance(prev["/Prev"], DictionaryObject) - prev["/Prev"][NameObject("/Next")] = child_reference - child_obj[NameObject("/Prev")] = prev["/Prev"] - except MemoryError: # pragma: no cover # TODO: BLE001 - raise - except Exception: # it means we are inserting in first position - child_obj.pop("/Next", None) - child_obj[NameObject("/Next")] = prev - prev[NameObject("/Prev")] = child_reference - child_obj[NameObject("/Parent")] = self.indirect_reference + visited.add(id(last)) + last = cast("DictionaryObject", last["/Next"]) + last[NameObject("/Next")] = child_reference + child_obj[NameObject("/Prev")] = last.indirect_reference + child_obj.pop("/Next", None) + self[NameObject("/Last")] = child_reference inc_parent_counter(self, child_obj.get("/Count", 1)) return child_reference + def _link_child_before( + self, child_obj: DictionaryObject, child_reference: IndirectObject, node: DictionaryObject + ) -> None: + """Link ``child_obj`` into the list of children directly in front of ``node``.""" + previous_reference = node.raw_get("/Prev") if "/Prev" in node else None + if previous_reference is None or is_null_or_none(previous_reference): + self[NameObject("/First")] = child_reference + child_obj.pop("/Prev", None) + else: + previous = cast("DictionaryObject", previous_reference.get_object()) + previous[NameObject("/Next")] = child_reference + child_obj[NameObject("/Prev")] = previous_reference + child_obj[NameObject("/Next")] = node.indirect_reference + node[NameObject("/Prev")] = child_reference + def _remove_node_from_tree( self, *, previous: Any, previous_reference: Any, current: Any, last: Any ) -> None: diff --git a/tests/generic/test_data_structures.py b/tests/generic/test_data_structures.py index f84fec3d68..24cad3b716 100644 --- a/tests/generic/test_data_structures.py +++ b/tests/generic/test_data_structures.py @@ -179,9 +179,6 @@ def test_tree_object__insert_child_in_first_position_with_next() -> None: child_a_ref = writer._add_object(child_a) tree.add_child(child_a_ref, writer) - # prev = tree["/Last"] = A, prev == before, so no while loop. - # try: prev["/Prev"] — A has no /Prev → KeyError → except block fires. - # Create child C with a /Next pointer pointing to something child_c = DictionaryObject() child_c[NameObject("/Next")] = child_a_ref # C -> A @@ -195,6 +192,8 @@ def test_tree_object__insert_child_in_first_position_with_next() -> None: c_obj = child_c_ref.get_object() assert isinstance(c_obj, DictionaryObject) assert c_obj["/Next"] == child_a + assert tree["/First"] == c_obj + assert "/Prev" not in c_obj @pytest.mark.enable_socket diff --git a/tests/test_writer.py b/tests/test_writer.py index 4a43f5430e..295075ca0a 100644 --- a/tests/test_writer.py +++ b/tests/test_writer.py @@ -4442,6 +4442,72 @@ def test_generate_file_identifiers__update() -> None: assert writer._ID[1] != id_changing +def _get_outline_titles(writer: PdfWriter) -> list[str]: + return [item["/Title"] for item in PdfReader(_write_to_bytes(writer)).outline] + + +def _write_to_bytes(writer: PdfWriter) -> BytesIO: + output = BytesIO() + writer.write(output) + return output + + +@pytest.mark.parametrize( + ("titles", "before", "expected"), + [ + pytest.param(["first", "middle", "last"], None, ["first", "middle", "last", "new"], id="none"), + pytest.param(["first", "middle", "last"], "last", ["first", "middle", "new", "last"], id="last"), + pytest.param(["first", "middle", "last"], "middle", ["first", "new", "middle", "last"], id="middle"), + pytest.param(["first", "middle", "last"], "first", ["new", "first", "middle", "last"], id="first"), + pytest.param(["only"], "only", ["new", "only"], id="only"), + ], +) +def test_add_outline_item__before(titles: list[str], before: str | None, expected: list[str]) -> None: + """ + add_outline_item(before=...) inserts the new item in front of `before`, or at the end without it. + + The items of one outline level are displayed in the order of their linked + list /First, /Next, ... (ISO 32000-2:2020, 12.3.3), which is the order in + which PdfReader.outline returns them. + """ + writer = PdfWriter() + writer.add_blank_page(72, 72) + items = {title: writer.add_outline_item(title=title, page_number=0) for title in titles} + + writer.add_outline_item(title="new", page_number=0, before=None if before is None else items[before]) + + assert _get_outline_titles(writer) == expected + + +def test_add_outline_item__without_page() -> None: + """ + add_outline_item() with page_number=None creates an item without an action. + + An outline item needs neither a destination nor an action, /Dest and /A are + both optional (ISO 32000-2:2020, 12.3.3, Table 151). + """ + writer = PdfWriter() + writer.add_blank_page(72, 72) + + item = writer.add_outline_item(title="no destination", page_number=None).get_object() + + assert "/A" not in item + assert "/Dest" not in item + + +def test_add_outline_item__page_object() -> None: + """add_outline_item() accepts a PageObject of the writer as page.""" + writer = PdfWriter() + page = writer.add_blank_page(72, 72) + + item = writer.add_outline_item(title="page object", page_number=page).get_object() + + # A go-to action (ISO 32000-2:2020, 12.6.4.2, Table 202) to the destination + # [page /Fit], where page is an indirect reference to the page (12.3.2.2, Table 149). + assert item["/A"]["/S"] == "/GoTo" + assert item["/A"]["/D"] == [page.indirect_reference, "/Fit"] + + def test_add_outline_item__positional_arguments() -> None: """ add_outline_item() warns about, but still accepts, positional arguments in the order of its signature. @@ -4495,3 +4561,134 @@ def test_remove_images__legacy_boolean_parameter() -> None: writer.remove_images(True) assert not writer.pages[0].images + + +def test_page_mode__getter() -> None: + """page_mode is None without a /PageMode entry and returns the mode that was set.""" + writer = PdfWriter() + # /PageMode is optional (ISO 32000-2:2020, 7.7.2, Table 29). pypdf returns None + # instead of the default /UseNone. + assert writer.page_mode is None + + writer.page_mode = "/UseOutlines" + + assert writer.page_mode == "/UseOutlines" + + +def test_add_annotation__invalid_page_type() -> None: + """add_annotation() raises a TypeError if the page is neither an index nor a PageObject.""" + writer = PdfWriter() + writer.add_blank_page(72, 72) + annotation = Link(rect=(0, 0, 10, 10), target_page_index=0) + + with pytest.raises(TypeError, match=r"^page: invalid type$"): + writer.add_annotation("0", annotation) + + +def test_clean_page__named_destination_in_action() -> None: + """ + clean_page() converts the name of a named destination in a go-to action to a string. + + A named destination can be referred to by a name object or by a byte string + (ISO 32000-2:2020, 12.3.2.4). merge() writes all named destinations into the + /Dests name tree, whose keys are strings, so the name has to become a string. + The key keeps the leading slash, as merge() uses "/chapter1" as key for the + name /chapter1 of a /Dests dictionary in the catalog. + """ + writer = PdfWriter() + page = writer.add_blank_page(72, 72) + # A link to the named destination /chapter1 (12.6.4.2, Table 202). + action = DictionaryObject({NameObject("/S"): NameObject("/GoTo"), NameObject("/D"): NameObject("/chapter1")}) + page[NameObject("/Annots")] = ArrayObject([DictionaryObject({NameObject("/A"): action})]) + + writer.clean_page(page) + + assert isinstance(action["/D"], TextStringObject) + assert action["/D"] == "/chapter1" + + +def _create_reader_with_article_thread() -> PdfReader: + """A one-page document with an article thread of one bead (ISO 32000-2:2020, 12.4.3).""" + writer = PdfWriter() + page = writer.add_blank_page(72, 72) + thread = DictionaryObject({NameObject("/I"): DictionaryObject({NameObject("/Title"): TextStringObject("story")})}) + thread_reference = writer._add_object(thread) + bead = DictionaryObject() + bead_reference = writer._add_object(bead) + # Table 163: the only bead is its own next and previous bead. + bead.update({ + NameObject("/T"): thread_reference, + NameObject("/N"): bead_reference, + NameObject("/V"): bead_reference, + NameObject("/P"): page.indirect_reference, + NameObject("/R"): RectangleObject((0, 0, 72, 72)), + }) + thread[NameObject("/F")] = bead_reference + page[NameObject("/B")] = ArrayObject([bead_reference]) + writer.root_object[NameObject("/Threads")] = writer._add_object(ArrayObject([thread_reference])) + return PdfReader(_write_to_bytes(writer)) + + +def test_merge__excluded_fields_none() -> None: + """merge() with excluded_fields=None excludes nothing, so it copies the article threads.""" + reader = _create_reader_with_article_thread() + writer = PdfWriter() + + writer.merge(None, reader, excluded_fields=None) + + assert len(writer.threads) == 1 + + +def test_merge__form_field_without_merged_widget() -> None: + """ + merge() does not add a form field to /AcroForm that is on none of the merged pages. + + merge() only takes over the fields whose widget annotations it copied with the pages. + """ + source = PdfWriter() + source.add_blank_page(72, 72) + # A text field without widget annotation, so it is on no page. /Fields lists + # the root fields of the form (ISO 32000-2:2020, 12.7.3, Table 224). + field = source._add_object( + DictionaryObject({NameObject("/FT"): NameObject("/Tx"), NameObject("/T"): TextStringObject("orphan")}) + ) + source.root_object[NameObject("/AcroForm")] = DictionaryObject({NameObject("/Fields"): ArrayObject([field])}) + + writer = PdfWriter() + writer.merge(None, PdfReader(_write_to_bytes(source))) + + assert writer.root_object["/AcroForm"]["/Fields"] == [] + + +def test_reset_translation__other_reader() -> None: + """ + reset_translation() for another reader, or an object of it, keeps the objects copied from the first reader. + + Appending the first reader again therefore reuses them. + """ + reader = PdfReader(RESOURCE_ROOT / "crazyones.pdf") + other = PdfReader(RESOURCE_ROOT / "crazyones.pdf") + writer = PdfWriter() + writer.append(reader) + + writer.reset_translation(other) + writer.reset_translation(IndirectObject(1, 0, other)) + writer.append(reader) + + assert writer.pages[0].raw_get("/Resources") == writer.pages[1].raw_get("/Resources") + + +def test_reset_translation__indirect_object() -> None: + """ + reset_translation() for an object of a reader makes the writer copy the objects of that reader again. + + Appending the reader again therefore creates new objects instead of reusing the copied ones. + """ + reader = PdfReader(RESOURCE_ROOT / "crazyones.pdf") + writer = PdfWriter() + writer.append(reader) + + writer.reset_translation(IndirectObject(1, 0, reader)) + writer.append(reader) + + assert writer.pages[0].raw_get("/Resources") != writer.pages[1].raw_get("/Resources") From a6751d28457517cd951f01104718cd26b08aed7a Mon Sep 17 00:00:00 2001 From: Martin Thoma Date: Fri, 13 Nov 2026 14:21:42 +0100 Subject: [PATCH 2/3] TST: Cover the remaining branches of TreeObject.insert_child() --- tests/generic/test_data_structures.py | 68 +++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/tests/generic/test_data_structures.py b/tests/generic/test_data_structures.py index 24cad3b716..2a0285f8c8 100644 --- a/tests/generic/test_data_structures.py +++ b/tests/generic/test_data_structures.py @@ -566,6 +566,74 @@ def test_tree_object__insert_child__cycle() -> None: ) +def test_tree_object__insert_child__before_with_cycle() -> None: + """insert_child() raises a LimitReachedError if the children it searches for `before` form a cycle.""" + writer = PdfWriter() + tree = TreeObject() + writer._add_object(tree) + # /First -> first -> second -> first -> ...: the /Next links of the children form a cycle. + first = DictionaryObject() + second = DictionaryObject() + first_reference = writer._add_object(first) + first[NameObject("/Next")] = writer._add_object(second) + second[NameObject("/Next")] = first_reference + tree[NameObject("/First")] = first_reference + tree[NameObject("/Last")] = first.raw_get("/Next") + not_a_child = writer._add_object(DictionaryObject()) + + with pytest.raises(LimitReachedError, match=r"^Detected cycle in tree structure\.$"): + tree.insert_child(writer._add_object(DictionaryObject()), not_a_child, writer) + + +def test_tree_object__insert_child__before_is_not_a_child() -> None: + """ + insert_child() appends the child at the end if `before` is not one of the children. + + The children are linked from /First to /Last by /Next and /Prev + """ + writer = PdfWriter() + tree = TreeObject() + writer._add_object(tree) + first = writer._add_object(DictionaryObject()) + last = writer._add_object(DictionaryObject()) + tree.add_child(first, writer) + tree.add_child(last, writer) + not_a_child = writer._add_object(DictionaryObject()) + new = writer._add_object(DictionaryObject()) + + tree.insert_child(new, not_a_child, writer) + + assert tree.raw_get("/First") == first + assert tree.raw_get("/Last") == new + assert last.get_object().raw_get("/Next") == new + assert new.get_object().raw_get("/Prev") == last + assert "/Next" not in new.get_object() + + +def test_tree_object__insert_child__first_child_drops_previous_links() -> None: + """ + insert_child() removes the /Next and /Prev entries a child brings along when it becomes the only child. + + The first child has no /Prev and the last child no /Next. + """ + writer = PdfWriter() + tree = TreeObject() + writer._add_object(tree) + # A child that was linked into another list before. + child = DictionaryObject({ + NameObject("/Next"): writer._add_object(DictionaryObject()), + NameObject("/Prev"): writer._add_object(DictionaryObject()), + }) + child_reference = writer._add_object(child) + + tree.insert_child(child_reference, None, writer) + + assert tree.raw_get("/First") == child_reference + assert tree.raw_get("/Last") == child_reference + assert "/Next" not in child + assert "/Prev" not in child + + def _outlined_pdf(nested: bool = False) -> BytesIO: writer = PdfWriter() for _ in range(4): From 4ce577b2997dba263d8927b752d7571a0b57777e Mon Sep 17 00:00:00 2001 From: Martin Thoma Date: Fri, 13 Nov 2026 14:40:37 +0100 Subject: [PATCH 3/3] TST: Satisfy mypy in the insert_child tests --- tests/generic/test_data_structures.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/generic/test_data_structures.py b/tests/generic/test_data_structures.py index 2a0285f8c8..d2b370e2d2 100644 --- a/tests/generic/test_data_structures.py +++ b/tests/generic/test_data_structures.py @@ -595,19 +595,21 @@ def test_tree_object__insert_child__before_is_not_a_child() -> None: tree = TreeObject() writer._add_object(tree) first = writer._add_object(DictionaryObject()) - last = writer._add_object(DictionaryObject()) + last_item = DictionaryObject() + last = writer._add_object(last_item) tree.add_child(first, writer) tree.add_child(last, writer) not_a_child = writer._add_object(DictionaryObject()) - new = writer._add_object(DictionaryObject()) + new_item = DictionaryObject() + new = writer._add_object(new_item) tree.insert_child(new, not_a_child, writer) assert tree.raw_get("/First") == first assert tree.raw_get("/Last") == new - assert last.get_object().raw_get("/Next") == new - assert new.get_object().raw_get("/Prev") == last - assert "/Next" not in new.get_object() + assert last_item.raw_get("/Next") == new + assert new_item.raw_get("/Prev") == last + assert "/Next" not in new_item def test_tree_object__insert_child__first_child_drops_previous_links() -> None: