andodeki
00f1dfbc12
Three defects found by auditing the ADR merge criteria against the code rather than against memory. 1. The scheduled fuzz job ran five hardcoded targets. Four had been added since and were never fuzzed: parse_revision_chain, decrypt, eval_function and parse_colorspace. eval_function is the sharpest of those - it executes PostScript taken verbatim from an untrusted file. The list now comes from `cargo fuzz list` and the job fails rather than passing vacuously if it comes back empty. `cargo fuzz list` reads the manifest, so a target file added without its [[bin]] entry would still be skipped silently. The engine job, which runs on every push, now checks the two agree. Both directions of that guard were exercised before committing. 2. ADR 0004 rule 3 promises an annotation whose appearance cannot be generated "keeps its original /AP and is reported as skipped". The keeping worked - to_dict clones the source dictionary - but nothing reported it: SaveReport only tracked skipped appearances for form fields. A caller who moved a stamp was never told its artwork still showed the old position. Adds SaveReport::annotation_appearances_skipped and appearance_is_generated, which enumerates the out-of-scope types explicitly so a new AnnotationType fails to compile until classified. 3. SetContents and SetFlags had no round-trip test. Both were implemented and unit-tested against the in-memory model, but neither was ever reparsed from written bytes - the assertion ADR 0004 calls central. New fixture annotations/stamp.pdf carries real /AP artwork for a Stamp (undrawable: must be preserved and reported) beside a Square (drawable: must not be reported), so the reporting cannot pass by reporting everything. The stamp test was mutation-checked: it fails when the reporting line is removed. ADR 0003 and 0004 merge criteria are now ticked. 0003's were genuine paperwork - every box traced to an existing named test. 0004's were not, and its ADR now records what was missing rather than implying it always worked. TEST_TARGET=pdf 447 -> 451 passing. rustfmt and clippy -D warnings clean.
155 lines
7 KiB
Markdown
155 lines
7 KiB
Markdown
# ADR 0003: PDF incremental save — append revisions, never rewrite
|
||
|
||
- **Status:** Accepted
|
||
- **Date:** 2026-07-28
|
||
- **Review item:** Phase 8, `DART_PDF_VS_MAKEPAD_PDF_GAP_ANALYSIS.md`
|
||
- **Supersedes:** nothing
|
||
- **Related:** `REVIEWS/DART_PDF_VS_MAKEPAD_PDF_GAP_ANALYSIS.md` phases 3, 4 and 7
|
||
|
||
## Context
|
||
|
||
Phases 0–7 are complete. The viewer parses documents, renders them, and edits
|
||
form fields through `DocumentFormEditor` with real appearance regeneration.
|
||
|
||
None of that can be saved. There is no public API anywhere in the four PDF
|
||
crates that writes an edited document back out:
|
||
|
||
```
|
||
$ grep -rn "pub fn" pdf-document/src/*.rs | grep -iE "save|write|serial"
|
||
(no matches)
|
||
```
|
||
|
||
`PdfWriter` and `PdfDocBuilder` can only author a whole new file from
|
||
scratch, which is how the test fixtures are produced. They cannot represent
|
||
"this existing document, plus a change".
|
||
|
||
So the product currently ships an editor whose edits are discarded. That is
|
||
the same class of defect Phase 0 was created to remove — an API that claims
|
||
a capability it does not deliver — and it makes the Phase 3 and Phase 4 work
|
||
demonstrably useful only inside a single session.
|
||
|
||
Phase 8 lists ten independent features. Incremental save is chosen first for
|
||
three reasons:
|
||
|
||
1. **Its prerequisites are already met.** The review lists "writer/serializer,
|
||
byte-preserving tests, signature preservation". The writer exists, the
|
||
test infrastructure exists (Phase 6), and there are no signatures to
|
||
preserve yet precisely because signing is not implemented.
|
||
2. **It unblocks two other Phase 8 features.** Both *annotation editing* and
|
||
*AcroForm full support* list incremental save as a prerequisite. Neither
|
||
can start until this exists.
|
||
3. **Nothing else closes the credibility gap.** Encryption and advanced
|
||
colour add new capability; this one makes existing capability real.
|
||
|
||
## Decision
|
||
|
||
Implement saving as an **incremental update** — the original bytes are copied
|
||
verbatim and a new revision is appended — rather than as a full rewrite.
|
||
|
||
A PDF incremental update appends only the changed objects, a new
|
||
cross-reference section, and a trailer whose `/Prev` points at the previous
|
||
xref offset. Readers follow the chain backwards, so unchanged objects resolve
|
||
to their original bytes.
|
||
|
||
### Why append rather than rewrite
|
||
|
||
A full rewrite is simpler to implement and wrong for this product:
|
||
|
||
- **It destroys anything the parser does not understand.** This parser does
|
||
not yet model optional content groups, structure trees, embedded files or
|
||
digital signatures. A rewrite would silently drop all of them. An append
|
||
physically cannot: the original bytes are still there.
|
||
- **It invalidates every existing signature.** A signature covers a byte
|
||
range of the file. Appending preserves that range; rewriting does not.
|
||
Signing is not implemented today, but a save format that structurally
|
||
precludes it is a dead end.
|
||
- **It is unauditable.** With an append, the previous revision is still in
|
||
the file and the diff is the appended bytes. A reviewer can see exactly
|
||
what an edit changed.
|
||
|
||
The cost is file growth on repeated saves. That is the correct trade: a
|
||
larger file is recoverable, a destroyed one is not.
|
||
|
||
### Scope of this ADR
|
||
|
||
**In scope:**
|
||
|
||
- Append a revision containing modified objects.
|
||
- Update form field values (`/V`) and appearance streams (`/AP`).
|
||
- Emit a valid xref section chained through `/Prev`.
|
||
- Preserve the original bytes byte-for-byte.
|
||
- Round-trip: the saved file reparses and observes the edit.
|
||
|
||
**Explicitly out of scope**, to be separate work:
|
||
|
||
- Cross-reference *streams* (`/Type /XRef`). Only classic xref tables are
|
||
written. A document that uses xref streams can still be saved, because a
|
||
classic table appended to it remains readable, but this is recorded as a
|
||
known limitation rather than glossed over.
|
||
- Object streams (`/ObjStm`) for the appended objects.
|
||
- Encryption. Saving an encrypted document is **refused**, not attempted:
|
||
writing plaintext objects into an encrypted file would silently corrupt it
|
||
and could leak content that the document's own security policy protects.
|
||
- Signature creation or re-signing.
|
||
- Compaction or garbage collection of superseded objects.
|
||
|
||
## Non-negotiable rules for this feature
|
||
|
||
These extend the review's existing rules and are enforced by tests:
|
||
|
||
1. **The original bytes are a prefix of the saved file.** Asserted directly,
|
||
not by inspection.
|
||
2. **A save with no pending edits is a no-op**, returning the input
|
||
unchanged. Appending an empty revision would grow the file for nothing
|
||
and churn its modification date.
|
||
3. **An unsupported document is refused with a typed error**, never saved
|
||
partially. Encryption is the first such case.
|
||
4. **Every appended object is reachable.** A revision that writes an object
|
||
no trailer points at is a leak and a bug.
|
||
5. **Round-trip is the acceptance test.** Save, reparse from bytes, and
|
||
assert the edit is observable. Anything less tests the writer against
|
||
itself.
|
||
|
||
## Merge criteria
|
||
|
||
This feature merges when all of the following hold:
|
||
|
||
- [x] `DocumentFormEditor` edits can be saved and reparsed, with the new
|
||
value observable through the normal `acroform()` path.
|
||
- [x] The original document's bytes are an exact prefix of the output.
|
||
- [x] Saving with no edits returns the input unchanged.
|
||
- [x] Saving an encrypted document returns `SaveError::Encrypted`.
|
||
- [x] Appearance streams regenerated in Phase 3 are written as real stream
|
||
objects and referenced from their widgets.
|
||
- [x] Corpus round-trip: every `forms/` fixture survives edit → save →
|
||
reparse.
|
||
- [x] A saved file survives a second edit-and-save cycle, producing a chain
|
||
of three revisions that still reparses.
|
||
- [x] Malformed and truncated corpus fixtures either save correctly or
|
||
return a typed error; none panic.
|
||
- [x] `TEST_TARGET=pdf ./tools/test-rust-clean.sh` passes, rustfmt and
|
||
clippy `-D warnings` clean.
|
||
|
||
All criteria met. These boxes went unticked when the feature shipped; they
|
||
were audited afterwards against the code and each maps to a named test:
|
||
byte-prefix to `save_roundtrip.rs` and `incremental.rs`
|
||
`the_original_bytes_are_a_prefix_of_the_output`, the no-op save to
|
||
`report.unchanged`, appearance streams to
|
||
`a_regenerated_appearance_is_written_as_a_real_stream`, and the corpus
|
||
round trip to `every_form_fixture_round_trips`.
|
||
|
||
## Consequences
|
||
|
||
**Positive.** Form editing becomes a real feature. Annotation editing and
|
||
full AcroForm support are unblocked. The document model gains a serialisation
|
||
path that a future signature implementation can build on without redesign.
|
||
|
||
**Negative.** Files grow on every save. The parser must handle its own
|
||
output, which means the xref chain code is now load-bearing in both
|
||
directions. Documents using xref streams get a hybrid file — valid, but
|
||
inelegant — until that limitation is lifted.
|
||
|
||
**Risk.** The most likely defect is an offset error in the appended xref,
|
||
which produces a file that opens in this viewer but not in Acrobat. The
|
||
mitigation is a round-trip test that reparses from bytes rather than reusing
|
||
in-memory state, plus byte-prefix assertions that catch accidental rewrites.
|