BUG: Insert outline items in front of before and cover more of PdfWriter

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:

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
@@ -567,6 +566,76 @@ 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_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_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_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:
+    """
+    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):

tests/test_writer.py

@@ -4539,6 +4539,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.
@@ -4592,3 +4658,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")