feat: PDF attachment extraction (metadata, bytes, streaming) - #313
Open
yonikremer wants to merge 4 commits into
Open
feat: PDF attachment extraction (metadata, bytes, streaming)#313yonikremer wants to merge 4 commits into
yonikremer wants to merge 4 commits into
Conversation
Expose embedded-file attachments end to end: - C++: extract_attachment_records walks the EmbeddedFiles name tree and FileAttachment annotations, deduplicating by QPDFObjGen; metadata only, stream bytes decoded on demand via get_attachment_data with max_size guard. - qpdf quirk: stream handles don't proxy getKeys()/hasKey() on this qpdf version, so /Subtype, /Params and /Length are read via getDict(). - pybind: get_attachments / get_attachment_data on the parser. - Python: PdfAttachment/FileAttachmentAnnotation models, get_attachments(), get_attachment_data(max_size=...) and get_attachment_stream(max_size=...). - tests/test_attachments.py: synthetic PDFs covering name/mime/size extraction, annotation anchoring, ID dedup and the streaming API. Signed-off-by: yoni kremer <yoni.kremer@gmail.com>
- document.h: the raw /Length filter-bomb guard was dead code — stream handles don't proxy hasKey/getKey on this qpdf version, so read it via getDict(); replace the catch-all swallows with WARNING logs, drop the fallback comment that described unimplemented code, and document the 10x slack heuristic. - attachments.h: extract _make_attachment_annotation to remove the duplicated (and already diverged) annotation-append blocks; document the stream_id==0 sentinel on AttachmentRecord. - pdf_parser.py: get_attachment_stream now returns the NamedTemporaryFile itself (delete on close) instead of a leaked delete=False reopen, fixes the docstring/behavior drift, drops leftover working notes and the now-unused os import; 8 MB threshold becomes a named constant. Signed-off-by: yoni kremer <yoni.kremer@gmail.com>
- attachments.h: add SPDX/MIT header (CONTRIBUTING.md:41-50 hard violation); extract getStreamDict() helper to dedupe mime/size extraction; rename _extract_*/_make_* helpers (Mysterious Name); bundle stream_id/gen into QPDFObjGen obj_gen (Data Clumps); extend EmbeddedFiles name-tree walk to handle /Nums (and /Limits via Kids recursion); fix dedup to gate on isIndirect() for direct streams; remove internal stream_id/stream_gen leak from attachments_to_json (scope creep). - document.h: switch to obj_gen/isIndirect()/getObjectByObjGen; add write_attachment_data() via Pipeline::pipeStreamData with LimitedFilePipeline enforcing max_size incrementally (fixes OOM before spill for large attachments). - docling_parser.h/pybind_parse.cpp: expose write_attachment_data; unify get_attachments error contract to throw on missing key. - pdf_parser.py: get_attachments no longer swallows null; add os import; rewrite get_attachment_stream to be metadata-driven and truly streaming via mkstemp+write_attachment_data (no 2x memory), with BytesIO fallback and legacy spill. - tests/test_attachments.py: strengthen test_one_file_two_annots to assert exact Rect bbox values and polygon. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: yoni kremer <yoni.kremer@gmail.com>
- document.h: extract require_attachment_record/require_attachment_stream/warn_if_raw_length_exceeds to remove duplicated /Length guard and lookup prelude between get_attachment_data and write_attachment_data - pdf_parser.py: lift per-call _DeletingFileWrapper to module-level _AttachmentDeletingFile Signed-off-by: yoni kremer <yoni.kremer@gmail.com>
Contributor
|
✅ DCO Check Passed Thanks @yonikremer, all your commits are properly signed off. 🎉 |
Contributor
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
Contributor
Author
|
@PeterStaar-IBM Hey, can you review the PR? I think it would be a great feature for docling. |
PeterStaar-IBM
requested changes
Aug 10, 2026
|
|
||
|
|
||
| class FileAttachmentAnnotation(BaseModel): | ||
| """Position of a FileAttachment annotation on a page (0-based). |
Member
There was a problem hiding this comment.
I think the convention is that in C++ we use 0-based (since vectors are zero based and all memory is stored there), however the python interface should be 1-based (following the PDF convention). The goal of the python is to translate the 1 based to the 0 based.
| @@ -0,0 +1,343 @@ | |||
| /* | |||
| Copyright IBM Inc. All rights reserved. | |||
Member
There was a problem hiding this comment.
no need for this, all these lines need to be removed...
Member
|
@yonikremer Thanks for the addition, can you:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Exposes PDF embedded-file attachments end-to-end through the native parser. Before this change
docling-parsesilently ignored/EmbeddedFilesand/FileAttachmentannotations; consumers had no way to discover or extract attached payloads (spreadsheets, source docs, annotations) without a second PDF library. This PR adds a metadata-only first pass and an on-demand, size-gated byte path so attachments can be listed cheaply and fetched safely even for large files.Why this is needed
Catalog/Names/EmbeddedFilesand page-levelFileAttachmentannotations anchored to aRect. Both are common in enterprise PDFs (invoice ZUGFeRD/Factur-X, portfolio PDFs, review markup).get_annotations/get_pagenever walked those structures; callers fell back topypdf/qpdfout-of-band, losing the thread-safe decode cache and timing infrastructure.Core design
1. C++ extraction —
src/parse/qpdf/attachments.h(new, MIT/SPDX)extract_attachment_records(QPDF&, QPDFObjectHandle root)walks:Root/Names/EmbeddedFilesname-tree recursively: handles both/Namesand/Numsarrays and/Kidssub-trees (/Limitsimplicit via recursion) — the original/Names-only walk missed producer variants.Annots[]whereSubtype == /FileAttachment, resolvingFS→EFstream.QPDFObjGen(object + generation). A single embedded file referenced from the name-tree and two annotations (or multiple pages) collapses to oneAttachmentRecordwith multipleAttachmentAnnotations. Direct (non-indirect)EFstreams — inline dicts with sentinel(0,0)— are never deduped (each is a distinct inline object) and are flagged!isIndirect()so the byte path can fail loudly rather than chasing a free-list object 0.namefrom/UF→/F(UTF-8 sanitized),mime_typefrom streamSubtype,sizefromParams/Size→DL→Lengthpriority.getStreamDict()helper hides the qpdf quirk on this version where stream handles do not proxygetKeys()/hasKey()— all dict lookups go throughgetDict().attachments_to_json()emits{name, mime_type, size, annotations: [{page_no, bbox:[x0,y0,x1,y1]}]}; internals likeobj_gennever leak to Python (scope-creep fix in round 2).2. Decoder —
src/parse/pdf_decoders/document.hensure_attachments_loaded()runs once perpdf_decoder<DOCUMENT>lifetime, storesvector<AttachmentRecord>, reset onprocess_document_from_bytesio.require_attachment_record(index, max_size),require_attachment_stream(index, max_size),warn_if_raw_length_exceeds(stream, max_size)factor the bounds/size/isIndirect/getObjectByObjGen/isStreamprelude and theLengthfilter-bomb warning (10× slack — compression can legitimately expand) that was duplicated between the two byte paths.get_attachment_data(index, max_size) → Buffer—stream.getStreamData(qpdf_dl_all)then post-decode size check vsmax_size.write_attachment_data(index, max_size, path)—stream.pipeStreamData(&LimitedFilePipeline)with an incrementalwritten > max_sizethrow insidePipeline::write()so a 9 MB zip with lying/LengthOOMs before it spills;finish()flushes. Both paths pre-checkrec.size > max_sizeand the raw/Lengthheuristic before any decode.RuntimeError("key not found")(unified acrossget_attachments/get_attachment_data), OOB index →out_of_range, oversize/direct-stream/missing-stream →RuntimeErrorwith human message. No silent swallowing — failures areLOG_S(WARNING)+ throw.3. Python —
docling_parse/pdf_parser.py+src/pybind/*PdfAttachment(name, mime_type?, size, annotations: List[FileAttachmentAnnotation]),FileAttachmentAnnotation(page_no: int, bbox: BoundingRectangle)— 0-based pages, bottom-left origin,BoundingRectanglepolygons match page geometry.PdfDocument.get_attachments() -> List[PdfAttachment]— does not decode bytes.PdfDocument.get_attachment_data(index, *, max_size: int) -> bytes—max_sizerequired (TypeError if omitted) to force caller policy.PdfDocument.get_attachment_stream(index, *, max_size: int) -> BinaryIO— metadata-driven: ifeffective_size ≤ 8 MBreturnsBytesIO; otherwisemkstemp+ nativewrite_attachment_data(no 2× memory), wrapped in module-level_AttachmentDeletingFile(munlinks onclose(), supportswith).size==0(unknown) falls back tomax_size; under-estimates spill after a memory fetch.docling_parser::get_attachments/get_attachment_data/write_attachment_datakeyed by internal doc key, same throw-on-missing-key contract.4. Key trade-offs
processMemoryFilefast.Bufferpath remains for small files (fast, no fd); pipe avoids the C++Bufferallocation for large payloads — measured +9 MB spill no longer holds 2×.Compatibility with docling-core
This PR is complementary to, not dependent on, the unmrged docling-project/docling-core#713
PdfAttachment/FileAttachmentAnnotation(docling_parse/pdf_parser.py:FileAttachmentAnnotation) deliberately stays parser-level and only re-uses the stable core typeBoundingRectangle.AttachmentItemis not imported here somain/CIstay green while your branch is unreleased.name/mime_type/sizematchAttachmentItem1:1, sodocling(the converter) can later doAttachmentItem(name=a.name, mime_type=a.mime_type, size=a.size, target=..., status=...)with zero translation. Parser-onlyannotations{page_no, bbox}stays on the parse side (core modelstarget/statusinstead).docling-parsewill bump todocling-core>=2.92,<3and can ship an optional helperPdfAttachment.to_attachment_item() -> AttachmentItem(no API break). The actualDoclingDocument.attachmentspopulation belongs indocling, not here.A maintainer can merge this PR without waiting for the core branch; the integration point is
docling.Testing
tests/test_attachments.py, 5 tests):_build_pdfcrafts minimal xref PDFs covering no-annot, one-file-two-annots (ID dedup → 2 annotations on pages 0/2 with exactRect→BoundingRectangle+ polygon asserts), same name/different bytes (must stay 2 records, payload differs), large size-gate (max_sizeviolation) and>8 MBspill (returns file notBytesIO, auto-unlink).Files changed (this PR only)
src/parse/qpdf/attachments.h(new),src/parse/pdf_decoders/document.h,src/pybind/docling_parser.h,app/pybind_parse.cpp,docling_parse/pdf_parser.py,tests/test_attachments.pyExcluded from this PR:
27c9397(threaded-parse Windows key fix) and8c9eaa1(CLAUDE.md build notes) — separate PRs if needed.Co-authored-by: Claude noreply@anthropic.com
Signed-off-by: yoni kremer yoni.kremer@gmail.com