BUG: Fix outline merge ordering when inserting at specific position by adityamoolya · Pull Request #3965 · py-pdf/pypdf

adityamoolya · 2026-08-14T06:49:00Z

Closes TODO at pypdf/_writer.py line 2792.

The TODO was about using the before parameter when inserting outlines during merge(). Previously, when pages from another PDF were merged at a specific position, their bookmarks were always added to the end of the outline instead of being inserted at the correct position.

For example, merging Doc B into Doc A at position=1 should result in [A1, B1, B2, A2, A3], but previously produced [A1, A2, A3, B1, B2].

The main issue was in TreeObject.insert_child() in _data_structures.py. The traversal started from /Last and followed /Next. Since /Last has no /Next, the traversal stopped immediately and the code always fell back to appending the new outline items.

The fix changes the traversal to start from /First and follow /Next, allowing the correct insertion point to be found. The before parameter is also passed through to _insert_filtered_outline() so the bookmarks are inserted at the same position as the merged pages.

I used Claude Opus 4.6 to write tests for the same

When merging a source PDF into a target at a given page position, bookmarks were unconditionally appended to the end of the outline tree instead of being inserted at the corresponding position. This was caused by three distinct bugs: 1. TreeObject.insert_child() traversal bug (_data_structures.py): The linked-list walk started from /Last and followed /Next. Since the tail node has no /Next, the loop terminated immediately and always fell back to appending. Fixed by traversing from /First via /Next, with correct handling for insertion at the head, middle, or tail. 2. Page identity corruption (_writer.py): _get_page_number_by_indirect() relied on PageObject.page_number which used list.index(). Because PageObject inherits from dict, identical blank pages compared equal under dict.__eq__, causing page number resolution to falsely return 0. Fixed by comparing indirect_reference .idnum directly, bypassing content-based equality. 3. Position mutation in merge() (_writer.py): merge() incremented the position variable in the page-insertion loop, so by the time outline processing ran, position pointed past the inserted pages. Fixed by capturing initial_position before the loop and using it for outline insertion point calculation. Added _find_outline_item_before_page() helper to locate the correct insertion point in the existing outline tree, and wired it into _insert_filtered_outline() via the 'before' parameter. Added test_merge_outline_ordering_position() verifying that merging Doc B at position 1 of Doc A produces outline order [A1, B1, B2, A2, A3].

- Fix ruff S112 (try-except-continue) in _find_outline_item_before_page by logging the exception via logger_warning before continuing. - Fix test_tree_object__insert_child__cycle: the cycle must be reachable from /First (not /Last) since insert_child now traverses forward.

codecov · 2026-08-14T07:11:42Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.04%. Comparing base (799f2d8) to head (7e011b6).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3965      +/-   ##
==========================================
+ Coverage   97.98%   98.04%   +0.06%     
==========================================
  Files          57       57              
  Lines       11106    11153      +47     
  Branches     2080     2098      +18     
==========================================
+ Hits        10882    10935      +53     
+ Misses        125      122       -3     
+ Partials       99       96       -3

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

stefan6419846

Thanks for the PR, although I have not fully reviewed it due to its complexity.

At the moment, coverage is quite bad. Could you please review it and make sure that all relevant lines are covered correctly.

- Restructure insert_child to use early-return style per review feedback - Add test coverage for insert_child append-at-end branch - Add tests for merge outline ordering at start, end, and with /Dest arrays - Update stale comment referencing old /Last traversal

stefan6419846

Please check the updated remarks and the merge conflict.

adityamoolya · 2026-08-25T09:13:27Z

Addressed all review comments , moved _find_outline_item_before_page to pypdf/generic/_outline.py and added a proper docstring, and resolved the merge conflict. CI is passing as well .

stefan6419846 · 2026-08-25T13:55:18Z

+        if page_ref is None:
+            continue
+        try:
+            pn = writer._get_page_number_by_indirect(page_ref)


Please avoid such abbreviations.

stefan6419846 · 2026-08-25T13:59:30Z

            indirect_reference = IndirectObject(indirect_reference, 0, self)
+        target_id = getattr(indirect_reference, "idnum", None)
+        if target_id is not None:
+            for idx, page in enumerate(self.pages):


This will quickly become a bottleneck for files with lots of outlines. I guess we need some sort of cache object numbers to page indizes.

stefan6419846 · 2026-08-25T13:59:57Z

+        target_id = getattr(indirect_reference, "idnum", None)
+        if target_id is not None:
+            for idx, page in enumerate(self.pages):
+                if page.indirect_reference is not None and page.indirect_reference.idnum == target_id:


Isn't this behavior incorrect if we are dealing with multiple generations?

stefan6419846 · 2026-08-25T14:00:14Z

        if isinstance(obj, PageObject):
+            if obj.indirect_reference is not None:
+                target_id = obj.indirect_reference.idnum
+                for idx, page in enumerate(self.pages):


This is the same as above. Can we avoid the repetition?

stefan6419846 · 2026-08-25T14:02:11Z

+                exc=exc,
+            )
+            continue
+        if pn is not None and pn >= page_number:


Doesn't this assume that outline items are ordered by destination page number, which we cannot assume?

stefan6419846 · 2026-08-25T14:03:58Z

+        The child's ``IndirectObject`` (suitable for the *before* parameter of
+        ``TreeObject.insert_child``), or ``None`` if no such child exists.
+    """
+    for child in parent.children():


Why can we omit nested outlines?

stefan6419846 · 2026-08-25T14:04:52Z

+            continue
+        try:
+            pn = writer._get_page_number_by_indirect(page_ref)
+        except Exception as exc:


Which exceptions do we actually expect here?

stefan6419846 · 2026-08-25T14:09:05Z

+    assert titles == ["B1", "A_Dest"]
+
+
+def test_merge_outline_ordering_coverage_edge_cases():


Please use focused tests and consider moving them to tests/generic/test_outline.py. The current test setup is rather complex and very implementation-driven.

adityamoolya force-pushed the fix/outline-merge-ordering branch from 107c65f to c3b4099 Compare August 14, 2026 06:55

adityamoolya added 2 commits August 14, 2026 12:42

FIX: mypy type casts for insert_child and outline helper

6e3cd8e

STY: remove trailing newline in test_merger.py

449097b

stefan6419846 requested changes Aug 14, 2026

View reviewed changes

Comment thread pypdf/generic/_data_structures.py Outdated

adityamoolya force-pushed the fix/outline-merge-ordering branch 3 times, most recently from 456f87b to 1bccccb Compare August 18, 2026 07:22

stefan6419846 added needs-test A test should be added before this PR is merged. needs-change The PR/issue cannot be handled as issue and needs to be improved labels Aug 19, 2026

adityamoolya force-pushed the fix/outline-merge-ordering branch 3 times, most recently from 6ba9138 to bd49ffa Compare August 21, 2026 10:35

adityamoolya force-pushed the fix/outline-merge-ordering branch from bd49ffa to 586ecb8 Compare August 21, 2026 10:53

adityamoolya requested a review from stefan6419846 August 21, 2026 12:47

stefan6419846 requested changes Aug 24, 2026

View reviewed changes

Comment thread pypdf/_writer.py Outdated

Comment thread pypdf/_writer.py Outdated

Resolve merge conflicts and address PR reviews

7e011b6

adityamoolya force-pushed the fix/outline-merge-ordering branch 3 times, most recently from 57f31de to 7e011b6 Compare August 25, 2026 09:00

adityamoolya requested a review from stefan6419846 August 25, 2026 09:13

stefan6419846 removed needs-test A test should be added before this PR is merged. needs-change The PR/issue cannot be handled as issue and needs to be improved labels Aug 25, 2026

stefan6419846 requested changes Aug 25, 2026

View reviewed changes

		assert titles == ["B1", "A_Dest"]


		def test_merge_outline_ordering_coverage_edge_cases():

Uh oh!

Conversation

adityamoolya commented Aug 14, 2026

Uh oh!

codecov Bot commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stefan6419846 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stefan6419846 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adityamoolya commented Aug 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Aug 14, 2026 •

edited

Loading