Repository navigation
PPF-2099: Repair selected-page ownership and excluded-content retention - #2111
Conversation
- Build every destination graph through a new `ClonePlan` class in `PyPDFForm/lib/clone.py`, used by page extraction, merging, and the watermark copy behind draw(), generate_coordinate_grid(), and bulk_create_fields(). Before cloning, it walks the objects reachable from the copied annotations in a throwaway reader and edits them. - Point every reference to a copied page, including each annotation's own /P, at its output page, and references to the source catalog at the output catalog, so cloning no longer copies the original page tree. - Keep only the copied part of shared and parent fields: /Kids drop annotations that are not copied and fields whose widgets are all on excluded pages. - Keep dropping links to excluded pages, and remove GoTo actions whose target page is excluded while keeping their button or field. Raise ValueError naming the page and entry for any other reference to excluded content instead of copying it. - Extract each page from its own reader in one pass, without its /Annots or article beads, replacing the remove-widgets, split, and re-clone pipeline and the single-page branch of copy_watermark_widgets. - Describe the behavior in the extraction warning of the User Guide and in the affected docstrings, and add regression tests for the appendix A reproduction, cross-page fields and actions, rejected references, and the other clone paths. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Annotations cloned by extraction, merging, draw(), coordinate grids, and bulk field creation no longer carry copies of their source pages, and their /P now points at their own page. Page counts, fields, values, appearance states, and page contents are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Reviewed at
Validation: |
- When pruning /Kids of a button field whose /Opt has one entry per kid, keep only the remaining kids' /Opt entries and renumber positional on-states (/0, /1, ...) in their /AS and appearance dictionaries and in the field's /V and /DV. A /V or /DV naming a removed position becomes /Off. Choice fields keep their /Opt. - Resolve fields that ResetForm and SubmitForm /Fields or Hide /T name by fully qualified name against the source field hierarchy, including kids already pruned, and reject excluded ones like references. - Prune /Kids once per field instead of while walking the array. - Add tests for named targets in each action, names of copied and unknown fields, and positional radio states, with a choice field whose option count matches its widgets. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks. Both findings reproduced at
Tests: four named-target cases (strings and arrays, in all three actions), a test that names of copied and unknown fields are kept, and a positional-radio test. The new reject cases and the radio test fail against Still not handled, as noted in the description: numeric states that don't match their own position (radios in unison), and field names inside JavaScript. 🤖 Generated with Claude Code |
|
Follow-up review at
Validation: all four cases were reproduced separately and compared with the previous PR revision. |
- Rename each distinct appearance dictionary once, so one shared by /N and /D, or by several kids, is not renumbered twice. - Renumber rollover (/R) appearances along with normal and down ones. - Resolve /Opt, /V, and /DV through field inheritance and write the renumbered values on the pruned field itself, only for fields whose kids are widgets. - Delete the field's own /V or /DV when it names a removed button, and write /Off only when an ancestor would still supply a value. Writing /DV /Off made PyPDFForm read the remaining buttons as checkboxes (PPF-2112). - Resolve indirect strings before checking named ResetForm, SubmitForm, and Hide targets. - Add tests for a shared appearance dictionary, rollover appearances, inherited /Opt and /V with an unchanged parent, an own /DV, a group without /DV, and indirect named targets. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks. All four reproduced at
One behavior change came out of this round. Writing Tests: indirect-string cases in the reject test, and a new test for a shared 🤖 Generated with Claude Code |
|
Reviewed at Isolate appearance state dictionaries shared between different fields before renumbering. ( The
Before extraction, The mutable state dictionaries need to be isolated for each field before renumbering, while the underlying appearance streams can remain shared. A global visited set alone would still allow a pruned field to change an unpruned field's dictionary. Please add a regression test that shares the dictionary between two groups with different retained widget sets and checks that the wholly retained group's selection survives. Validation: replayed all four previous reproductions and independently reproduced this case. |
- Give each remaining kid of a renumbered field its own /AP dictionary with renamed copies of its normal, rollover, and down state dictionaries, so a state dictionary shared with another field is not changed. Appearance streams stay shared, and kids or entries of the same field that shared a state dictionary share its copy. - Keep appearance entries that are a single stream as they are, and do not add entries a kid did not have. - Add a test where a group split across pages and a group wholly on the extracted page share a normal state dictionary, and check that a single-stream rollover appearance is kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks. Reproduced at Renumbering no longer edits state dictionaries in place. Each remaining button of a renumbered field gets its own Test: 🤖 Generated with Claude Code |
|
Reviewed at Preserve action targets when pruning a shared
Using person = pdf.Root.AcroForm.Fields[1]
person.Kids = pdf.make_indirect(Array(person.Kids))
pdf.pages[0].Annots[7].A = Dictionary(
S=Name.ResetForm, Fields=person.Kids
)Extracting page 1 succeeds and silently changes the action's targets from Assign a new array to the field's Validation: replayed the prior reported reproductions and independently reproduced both forms of this case. |
- Assign a new /Kids array to a pruned field instead of mutating the original. An action whose /Fields refers to the same array kept only the remaining kids, so its excluded target was dropped instead of rejected. The original array is now checked wherever else it is used. - Add a test that requires a ResetForm action to reject the excluded child whether /Fields is the field's indirect /Kids array or a copy, and that the field is still pruned without such an action. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks. Reproduced at A pruned field now gets a new I also checked the other edits the guard makes in place: remapped page and catalog references, removed GoTo actions, Test: 🤖 Generated with Claude Code |
Describe pruning as giving fields new /Kids arrays in the ClonePlan docstring, and say that get_page_streams clones each page's annotations with its own ClonePlan rather than through clone_page_annotations. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Describe the final behavior once instead of appending each review round, summarize the review findings, count all nine tests and which fail on master, and re-measure extraction times against master on the committed samples. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolves #2099 (action plan item E3-01, review R01).
Baseline
At
db35aa0a(pypdf 6.21.4), the appendix A reproduction still turned a 4,099-byte source into a 4,296-byte one-page output that containedEXCLUDED_SECRET_SECOND_PAGE. The widget's/Pwas object(8, 0), not the output page(3, 0), and that page's/Parent /Countwas 2.A three-page test form leaked every excluded marker into
pages[0]: page 2's text and image, a field value only on page 2, a sibling's value under a shared parent, and other pages' appearances of a shared text field and a radio group. There were three leak paths:/Pcopied the source page tree/Parentcopied its other/Kidsand their pages/Aand/AAcopied their target pagespypdf's
PdfWriter.append(pages=...)leaks through those actions too. The other clone paths (merge,draw(), coordinate grid, bulk field creation) also copied each widget's source page and page tree as stray objects.Changes
One clone plan: a new
ClonePlanclass inPyPDFForm/lib/clone.pybuilds the destination graph for every clone path. It is used throughclone_page_annotationsandget_page_streamsinutils.py. Before cloning, it walks everything reachable from the copied annotations in a throwaway reader and edits it. No pypdf private state is used.Ownership: references to a copied page, including each annotation's own
/P, point at its output page. References to the source catalog point at the output catalog.Shared fields: a field gets a new
/Kidsarray without annotations that aren't copied and fields whose widgets are all on excluded pages, so a shared or parent field keeps only its copied part. The original array stays intact, so an action that refers to it is still checked. When a button field's kids are widgets and its/Opt, inherited or not, has one entry per kid, the field gets its own trimmed/Opt. Positional on-states (/0,/1, ...) are renumbered in/AS,/Vand/DV(inherited or not), and the normal, rollover, and down appearances. Each remaining button gets its own/APwith renamed copies of its state dictionaries, so dictionaries shared with other fields are unchanged; appearance streams stay shared. A/Vor/DVnaming a removed button is deleted from the field, or set to/Offwhen a parent would still supply a value. Choice fields keep their/Opt.Cross-page policy (confirmed):
ValueErrornaming the page and entry, including fields that ResetForm/SubmitForm/Fieldsor Hide/Tname by fully qualified name (direct or indirect strings), e.g.Cannot copy page 1: /Annots[7]/A/Fields[0] refers to content that is not being copied, ....Because
.pagesextracts every page at once, one rejected reference fails the whole extraction, andcreate extractexits 1 without writing output.Extraction: each page is copied from its own reader in one pass, without
/Annotsand article beads, then its checked annotations are cloned onto it. This replaces the remove-widgets, split, and re-clone pipeline, along with the single-page branch andpage_numargument ofcopy_watermark_widgets.Docs: the User Guide warning on page extraction, the
pagesandcreate extractdocstrings, and the E3-01 record in the action plan (separate commit).Expected PDFs: 54 regenerated (separate commit) and visually compared by the maintainer.
Validation
tests/test_functional.py: nine tests, 19 cases. 18 fail against master. The exception istest_pages_keep_actions_naming_copied_fields, which guards against rejecting too much. Each review fix added cases that fail against the revision it fixed.test_pages_exclude_other_page_content: the appendix A reproductiontest_pages_keep_only_own_part_of_cross_page_fields: a three-page pikepdf form with a shared field, parent fields, a cross-page radio group, a page 2 image and value, a popup, navigation buttons, a page action, and a link. Each page,pages[0:2], and a merge of pages 1 and 3 are checked across every serialized object for each page's markers, page ownership,/Kids, values, and actions.test_pages_reject_other_cross_page_references: ResetForm, Hide, and/IRTreferences to page 2 and 3 content, and the same targets by name in ResetForm, SubmitForm, and Hide, as strings, arrays, and indirect stringstest_pages_check_targets_sharing_pruned_kids: a ResetForm/Fieldsthat is the field's own/Kidsarray or a copy of it, both rejectedtest_pages_keep_actions_naming_copied_fields: names of copied, shared, and unknown fields are kepttest_pages_renumber_positional_radio_states: positional radio states and/Optafter extracting either page, a single-stream rollover, a choice field whose option count matches its widgets, and no change afterdraw()test_pages_renumber_shared_and_inherited_radio_states: a shared/Nand/Dstate dictionary, rollover appearances, and/Optand/Vinherited from a parent whose other kids are radio groupstest_pages_renumber_without_changing_other_fields: a state dictionary shared by a group split across pages and a group wholly on the extracted page, which keeps its selectiontest_clone_paths_keep_page_ownership: page ownership and remapped GoTo targets afterdraw(), the coordinate grid, merging, and bulk field creation/Kidscounts, decoded page contents, AcroForm field order, and reopeneddata/schemaare identical. Stray page objects fall from 124 to 0, annotations with a foreign/Pfrom 3,786 to 0, and total size from 29.6 MB to 27.9 MB.create extracton a rejected PDF exits 1 without output./Datareference to the catalog points at the output catalog after extraction,draw(), and merging.test_bulk_create_fields_stress_max.pdf, 0.12 s vs 0.31 s forsample_template_with_full_key.pdf.make test-all(487 passed, 100% coverage),make linting,ruff format --check, a strict MkDocs build, andgit diff --checkoutside the expected PDFs (whose xref lines always end in a space) all passed, using local./venvwith standardzlib.Not addressed
/DVnaming a removed button still becomes a local/Off, which PyPDFForm reads incorrectly until PPF-2112: Radio buttons are read as checkboxes when the group's default value is Off #2112 (radios with a default of Off read as checkboxes) is fixed. JavaScript that names fields is not checked. An/AAdictionary whose actions were all removed is left empty./DA,/DR, and widgetless fields. A field shared by pages extracted together becomes two fields when they are merged./Vwhen it names a button on an excluded page by a non-positional state.🤖 Generated with Claude Code