Repository navigation
Conversation
There was a problem hiding this comment.
No issues found across 4 files
Shadow auto-approve: would auto-approve. Fixes a KeyError crash when partition_email encounters a multipart/* attachment by falling back to raw serialized bytes; includes a regression test.
Re-trigger cubic
cragwolfe
left a comment
There was a problem hiding this comment.
Draft review: Unstructured-IO/unstructured PR #4424
Verdict: Request changes
[High] Serialize multipart attachments as standalone documents
unstructured/partition/email.py:443
self._attachment.as_bytes() preserves the part's top-level Content-Disposition: attachment header. When an attachment is named *.eml, auto.partition() routes those serialized bytes back through partition_email(). The serialized part is now the recursive root message, and EmailMessage.get_body() excludes roots whose disposition is attachment, so a valid multipart attachment's body is silently dropped.
I reproduced this with a boundary-bearing multipart/signed attachment: the outer body was returned, but the signed message body was absent. This is introduced by the new serialization path and affects the PGP/MIME forwarded-message case described by the PR.
The added fixture does not exercise that behavior. Its multipart/signed part has no boundary and applies base64 transfer encoding to the multipart entity, so Python parses it as a scalar payload (is_multipart() == False, zero child parts). It proves that the original KeyError no longer escapes, but not that a well-formed multipart attachment is processed correctly.
Please serialize a copy of the multipart part as a standalone document by removing only the copy's top-level Content-Disposition before calling as_bytes(). Preserve the original part for filename metadata and do not mutate the parsed message tree. Add a boundary-bearing multipart/signed regression case that asserts:
- the outer email body is emitted;
- the signed attachment body is emitted;
- the signed body retains
filename == "signed-message.eml"and the enclosingattached_to_filenamemetadata.
The existing malformed fixture can remain as a separate no-crash regression.
(authored by codex)
eeshsaxena
left a comment
There was a problem hiding this comment.
Reasonable fix. Since email.contentmanager genuinely has no get_content() handler registered for any multipart/* type, catching that specific case in _AttachmentPartitioner._file_bytes and falling back to the part's raw serialized bytes is the right call, and it lets partition_email() recover the main body instead of the whole call dying on one signed/forwarded sub-part. The regression .eml with a multipart/signed attachment reproduces the original KeyError path, so the test genuinely guards it. Reads correct.
|
Good catch. Fixed in 1d1ca7e — the fix now strips |
1d1ca7e to
ba4935a
Compare
email.contentmanager has no get_content() handler registered for any
multipart/* content-type (handlers exist for text/*, application/*,
image/*, message/rfc822, etc., but multipart sub-parts are normally
consumed via .iter_parts(), not .get_content()). A multipart sub-part
can still surface as an "attachment" via iter_attachments() though --
e.g. a PGP/MIME-signed forwarded message (multipart/signed) nested
inside a multipart/mixed envelope, or a multipart/mixed forwarded
message itself. Resolving such an attachment's bytes in
_AttachmentPartitioner._file_bytes raised KeyError('multipart/signed')
(or 'multipart/mixed'), crashing the entire partition_email() call
instead of processing or skipping just that one attachment.
Fixed by falling back to the MIME part's raw serialized bytes
(as_bytes()) when its content-type is multipart/*, rather than calling
get_content(). This is a root-cause fix at the actual failure point,
not a broadening of the shared, deliberately narrow
EXPECTED_ATTACHMENT_ERRORS tuple (its own comment: "Intentionally
narrow... we do not catch RuntimeError... would otherwise be silently
skipped") -- a bare KeyError there would mask unrelated bugs elsewhere
in partition().
Fixes Unstructured-IO#3922.
Serializing a multipart/* attachment with as_bytes() preserved its own Content-Disposition: attachment header (that's how it surfaced as an attachment via iter_attachments() in the first place). Since EmailMessage.get_body() skips any candidate part -- including a multipart/* root message -- whose own disposition is "attachment", re-parsing that serialized part as the root of a new message found no body at all: the attachment partitioned to zero elements instead of surfacing its own content (e.g. a PGP-signed forwarded message's body). _serialized_multipart_bytes now serializes a deep copy of the part with that header stripped, leaving the original attachment object (still needed for its filename) untouched. Also fixed the existing regression fixture (mime-attach-multipart-signed.eml): its multipart/signed part had Content-Transfer-Encoding: base64, which is invalid for a multipart entity per RFC 2045 -- Python's parser treated it as an opaque scalar payload with zero child parts, so the fixture could never have exercised this bug regardless of the fix. Rewrote it as a genuine boundary-bearing multipart/signed part and extended the test to assert the attachment's own body and metadata now come through, not just that no exception is raised.
ba4935a to
39df846
Compare
|
@cragwolfe following up here -- the fix landed in 1d1ca7e and eeshsaxena confirmed the approach reads correct on 08-06. Would appreciate a re-review when you get a chance. |
|
@mittalpk , lgtm. please bump the version and update CHANGELOG.md accordingly (most recent changes at top of the file). thanks! |
…-multipart-attachment-keyerror
0.27.2 (and several releases before it) shipped while this PR sat open, so the CHANGELOG entry had drifted into an already-released section instead of a new one. Moved it to a fresh 0.27.3 section at the top and bumped __version__.py to match; make check-version confirms no further sync is needed. Requested by @cragwolfe.
|
Thanks! Bumped to 0.27.3 and moved the CHANGELOG entry -- 0.27.2 (and a few releases before it) shipped while this was open, so it had drifted into an already-released section. |
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval skipped because this PR is from an external contributor.
Re-trigger cubic
…aising AssertionError (Unstructured-IO#4597) ## Summary `partition_email()` processes attachments by default. When a message has a `message/rfc822` part, the whole call fails with a bare `AssertionError` and the body elements are lost too. Common inputs have such a part: - an email forwarded "as attachment" (Outlook, Thunderbird), - a delivery-failure bounce (`multipart/report` with the returned message), - a `multipart/digest`, where every part is `message/rfc822`. The repository's own fixture `example-docs/eml/mime-multipart-digest.eml` fails this way, through `partition()` too. Cause: for a `message/rfc822` part, `get_content()` returns the enclosed `EmailMessage`, not `str` or `bytes`, so `_AttachmentPartitioner._file_bytes` fails its `assert isinstance(content, bytes)`. The assert sits outside the `try`/`except` that skips attachments it cannot partition. Fix, in `unstructured/partition/email.py`: - `_file_bytes` serializes an enclosed message with `as_bytes()`. - For a `message/rfc822` part, `content_type="message/rfc822"` is added to the kwargs of the nested `partition()` call. Without it the bytes go through libmagic, which reports a message as `text/plain` when its first header is e.g. `MIME-Version`, `Message-ID` or `To` and not `From`/`Received`/`Date`. The attachment's raw headers then come out as `Title` elements. Python's `EmailMessage` writes such messages: the forwarded message in the new test starts with `Content-Type`, which libmagic does not recognize either. The hint is merged into the kwargs only for these parts, so a `content_type=` kwarg passed to `partition_email()` directly still reaches the other attachments as before instead of colliding with an explicit keyword. Related: - Unstructured-IO#4424 is the sibling fix in the same `_file_bytes` function (the `KeyError` on a `multipart/*` attachment). Its comment already lists `message/rfc822` among the types `get_content()` handles, but that path still hit the assert. - Unstructured-IO#3604 (closed in October 2024 with no fix linked) reported `process_attachments=True` failing on a nested `.eml`, with a `TypeError` in the implementation of that time. A maintainer asked there for an email that reproduces it so a test could guard the behavior. The tests here use two such emails: the existing digest fixture and a forwarded message built in the test. ## Validation Environment: `main` at c74e6c4, Python 3.12, `uv sync --locked --group test` (no extras; libmagic installed). New tests in `test_unstructured/partition/test_email.py` (attachments section): - `test_partition_email_partitions_the_messages_of_a_multipart_digest`: uses the existing `mime-multipart-digest.eml` fixture. It checks the three messages' text, their `subject` metadata and `attached_to_filename`. - `test_partition_email_partitions_a_message_forwarded_as_an_attachment`: an `EmailMessage` forwarded with `add_attachment()` whose first header is `Content-Type`. It checks the body, the forwarded text, `subject` and `sent_from`. Results: - New tests without the fix: `pytest test_unstructured/partition/test_email.py -k "multipart_digest or forwarded_as_an_attachment"` gives 2 failed, both `AssertionError` at `unstructured/partition/email.py:485` (`assert isinstance(content, bytes)`). The whole file without the fix gives 2 failed, 77 passed, 1 skipped. - New tests with the fix: 2 passed. - With only the `as_bytes()` change and no `content_type` hint, the digest test passes and the forwarded-message test fails with 6 extra `Title` elements (the forwarded message's raw headers). That is why the hint is part of the fix. - `pytest test_unstructured/partition/test_email.py`: 79 passed, 1 skipped. The skip is the WEBP test that needs `unstructured_inference`. - `pytest test_unstructured/file_utils/test_filetype.py test_unstructured/partition/test_msg.py`: 362 passed, 1 failed. The failure is `test_partition_msg_can_process_attachments`, which fails the same way on `main` because the `pptx` extra is not installed. `msg.py` has its own attachment partitioner and is not changed. - Manual check of a bounce (`multipart/report` with `text/plain`, `message/delivery-status` and a `message/rfc822` part whose first header is `MIME-Version`): returns the bounce text, the delivery-status lines and the returned message's body with `subject == "Lunch"`. Before the fix it raised `AssertionError`. A forward of a forward is also partitioned at every level. - `make check` (ruff 0.15.10 from the `lint` group): `All checks passed!`, `355 files already formatted`. - `bash scripts/test-check-new-release-version.sh`: `Passed 27 version guard tests.` - `VERSION_BASE_REF=upstream/main bash scripts/check-new-release-version.sh`: `PASS: unchanged development version 0.27.35-dev (no tag lookup).` - No `CHANGELOG.md` or `unstructured/__version__.py` changes. ## Release note `partition_email()`, and `partition()` on an `.eml` file, now partition an attached email (a message forwarded as an attachment, the returned message in a bounce, each message of a `multipart/digest`) like any other attachment. Before, they raised `AssertionError`. Each element from the attached message carries that message's own `subject`, `sent_from` and other email metadata. ## Checklist - [x] The PR title is a user-facing release-note entry with a conventional commit prefix. - [ ] Apply a category label (`enhancement`, `bug`, or an appropriate existing label), or ask a maintainer to apply it. A maintainer needs to apply `bug`. - [ ] For breaking changes, use `!` in the title and explain impact and migration above. (Not a breaking change.) --------- Co-authored-by: sinnercode228 <[email protected]>
Fixes #3922
What's wrong
partition_email(..., process_attachments=True)crashes with an uncaughtKeyErroron emails containing a PGP-signed (or any othermultipart/*-typed) attachment — matching the traceback in #3922 exactly.Root cause: Python's
email.contentmanagerhas noget_content()handler registered for anymultipart/*content-type (handlers exist fortext/*,application/*,image/*,message/rfc822, etc., but multipart sub-parts are normally consumed via.iter_parts(), not.get_content()). A multipart sub-part can still surface as an "attachment" viaEmailMessage.iter_attachments()though — e.g. a PGP/MIME-signed forwarded message (multipart/signed) nested inside amultipart/mixedenvelope (the linkedtestcase.txtscenario), or amultipart/mixedpart itself (the exactKeyError: 'multipart/mixed'in the original traceback)._AttachmentPartitioner._file_bytes(unstructured/partition/email.py) unconditionally callsself._attachment.get_content(), which raisesKeyError(content_type)for these cases — and since that call happens before thetry/exceptin_iter_elements()that's meant to gracefully skip unpartitionable attachments, the exception propagates out of the wholepartition_email()call instead of being caught.Fix
_file_bytesnow checksself._attachment.get_content_type().startswith("multipart/")and falls back to the part's raw serialized bytes (.as_bytes()) in that case, instead of callingget_content(). This is a root-cause fix at the actual failure point.I deliberately did not widen
EXPECTED_ATTACHMENT_ERRORS(inunstructured/partition/common/__init__.py) to includeKeyErroras an alternative fix — that tuple's own comment says it's "Intentionally narrow" specifically because a bareKeyErrorcatch there would also silently swallow unrelated bugs elsewhere inpartition(). Fixing_file_bytesdirectly avoids that trade-off entirely.How was this tested?
.eml(amultipart/mixedemail with a nested PGP/MIME-signed forwarded message as an attachment, mirroring the issue's real-world scenario) before writing any fix — confirmed the exactKeyError('multipart/signed').test_partition_email_does_not_raise_on_multipart_attachment, using a new fixtureexample-docs/eml/mime-attach-multipart-signed.eml, following the existing pattern oftest_partition_email_silently_skips_attachments_it_cannot_partition(the MP3-attachment test). Confirmed the new test fails against unpatchedemail.py(git stash) with the exact reportedKeyError, and passes after the fix — the email body is still partitioned, and the unpartitionable attachment produces no elements without crashing.test_unstructured/partition/test_email.pysuite: 72 passed, no regressions.ruff check/ruff format --check— clean.scripts/version-sync.sh -c— clean (no version bump needed;CHANGELOG.mdupdated under the existing unreleased0.25.2-dev0section per the contributing checklist).Context on prior attempt
A previous PR (#4277) attempted a fix for this same issue but was self-closed by its author after 3 days with no maintainer engagement, and its fixtures never landed — confirmed the bug is still fully live on current
mainbefore starting this PR. This PR takes a narrower approach than that one (a single-point fix in_file_bytes, no changes to the sharedEXPECTED_ATTACHMENT_ERRORStuple or to_iter_elements's control flow).