Repository navigation
fix: stop silently returning mojibake from encoding detection - #4550
Conversation
…ctured-IO#4466) detect_file_encoding() accepted any confident charset_normalizer result and, on inconclusive detection, took the first COMMON_ENCODINGS codec that decoded, which is almost always iso_8859_1. Both paths turned text into mojibake without raising. - Honor Unicode BOMs before statistical detection. - Accept bytes that are valid UTF-8 before consulting the detector, which reports mac_iceland / cp850 at confidence 1.0 when the only non-ASCII content is a single U+200B or U+00AD. - Rank inconclusive candidates with from_bytes() instead of taking the first decodable codec, and raise when no text candidate survives. - Emit EncodingDetectionWarning for statistically inferred legacy or BOM-less UTF-16/32 results so callers can reject them with a warnings filter. - Pass partition_md(encoding=...) through to local and URL byte decoding.
make docker-test runs pytest -n auto with --no-install-project, and the xdist controller there cannot import unstructured, so it crashed unserializing an EncodingDetectionWarning reported by a worker. Ignore it by message on its builtin base class; tests that check it use pytest.warns().
|
Independent verification, on the environment where Method: PyPI
One thing I would pin with a test: Unmodified, the same inputs are correct and do warn (identical to
A one-line test on any of those rows would keep a later simplification of that condition from silently reopening it. Scope note: the two partition-level test modules need the rest of the patch. Against the partial tree above they fail identically with |
cragwolfe
left a comment
There was a problem hiding this comment.
GPT Pro review of c294f4a found two actionable Unicode decoding defects, detailed inline:
- BOM-less UTF-16/32 can pass the new UTF-8 shortcut and return corrupted text without the inferred-encoding warning.
- Strict BOM decoding can reject a valid UTF-16 upload when file-type detection splits a surrogate pair at its 4,096-byte read boundary.
Both findings were verified against the changed code and its callers. All 52 reported checks on this exact head are successful, including CI unit tests for Python 3.11–3.13. Validation here was source and checked-in test inspection; no local tests were run. These cases need fixes and focused regression coverage before approval.
(authored by codex)
| f"File encoding detection failed: detected '{encoding}' but decode failed. " | ||
| f"File may be corrupted or in an unsupported format." | ||
| ) from None | ||
| byte_data.decode("utf-8") |
There was a problem hiding this comment.
[P2] Keep BOM-less UTF-16/32-shaped data out of the unconditional UTF-8 shortcut
For example, "Meeting notes: \u8acb\n".encode("utf-16-le") has no BOM, contains non-ASCII bytes, and is also syntactically valid UTF-8: ASCII characters contribute a trailing NUL, while U+8ACB contributes CB 8A, which UTF-8 interprets as U+02CA. This branch therefore selects UTF-8 before a correct UTF-16 detector result can be considered. The returned text contains NUL padding and the wrong character; the warning at lines 148–155 is also suppressed. BOM-less UTF-32LE has the same overlap. read_txt_file() and partition_text() consume the corrupted result, and warning-as-error callers cannot reject it.
Guard against the NUL-padding patterns of UTF-16/32 before taking this shortcut, so these buffers can reach statistical detection and retain the inferred-encoding warning. Add filename/bytes/stream coverage for this overlap, including a supplied correct detector result and warning-as-error behavior, while retaining the genuine UTF-8 precedence tests.
(authored by codex)
There was a problem hiding this comment.
Confirmed and fixed in bf795c2. On c294f4a, "Meeting notes: 請\n" in BOM-less UTF-16LE and UTF-32LE came back as utf-8 with NUL padding and no warning. main gets both right, so this was a regression from this PR.
The UTF-8 check now also requires b"\x00" not in byte_data. BOM-less UTF-16/32 always NUL-pads ASCII characters, while real UTF-8 text essentially never contains NUL. A NUL-bearing buffer that really is UTF-8 just falls back to the detector, as on main. After the fix, all four of UTF-16LE/BE and UTF-32LE/BE, for both that string and pure-ASCII "hello world", come back with the right codec and text and warn.
Tests in test_encoding_detection_policy.py:
test_bom_less_utf16_and_utf32_are_not_claimed_by_the_utf8_check: both strings × 4 codecs × filename/bytes/stream, with the correct detector result supplied, asserting the text and the warning.test_callers_can_reject_bom_less_utf16_and_utf32: warning-as-error raises.- The existing UTF-8 precedence tests (
test_valid_utf8_is_not_overridden_by_a_confident_detector, the BOM tests) are unchanged and pass.
|
|
||
| # Do not expose a UnicodeError's input payload in the public exception. | ||
| try: | ||
| file_text = byte_data.decode(encoding) |
There was a problem hiding this comment.
[P2] Preserve the incomplete-prefix distinction when decoding a detected BOM
_FileTypeDetectionContext.text_head reads only 4,096 bytes from streams (filetype.py:685–709). With the default UTF-8 encoding, a UTF-16 BOM makes both initial decoding attempts fail, so the bounded prefix reaches this strict whole-buffer decode without its eof_reached flag.
A valid payload b"\xff\xfe" + ("A" * 2046 + "\U0001f600" + "\n").encode("utf-16-le") places the emoji's high surrogate at the end of that prefix and its low surrogate in the remaining stream. The new forced UTF-16 decode raises UnprocessableEntityError, even though the complete document is valid. On the textual MIME path, file-type detection accesses text_head before the .txt extension shortcut, so this can stop valid uploads before partitioning. The filename path reads the complete document and avoids this artificial truncation.
Use BOM-aware incremental decoding for prefix inspection with final=eof_reached, while keeping complete-document decoding strict. Cover UTF-16LE/BE surrogate splits, stream-position restoration, and genuinely truncated EOF input. The existing boundary test covers UTF-8 under the initial decoder, not this BOM fallback.
(authored by codex)
There was a problem hiding this comment.
Confirmed and fixed in bf795c2. With b"\xff\xfe" + ("A" * 2046 + "\U0001f600\n").encode("utf-16-le") (and the BE equivalent), text_head raised UnprocessableEntityError on c294f4a.
detect_file_encoding() takes a new keyword-only final: bool = True. With final=False, the decode, and the UTF-8 check before it, use the codec's incremental decoder with final=False, so a multi-byte sequence cut off at the end is left out. An invalid byte anywhere else still fails. text_head passes final=eof_reached, the same distinction its own incremental fallback already makes one step earlier. Every other caller decodes a complete document and keeps the strict default. This covers the BOM case and also any detected multi-byte codec whose sequence the prefix splits.
Tests:
test_filetype.py,and_it_does_not_mistake_a_boundary_split_surrogate_pair_for_a_decode_error: UTF-16LE/BE, assertstext_head == "a" * 2046and that the stream position is restored to 0.test_filetype.py,but_it_rejects_a_utf16_stream_that_ends_inside_a_surrogate_pair: a genuinely truncated stream at EOF still raises.test_encoding_detection_policy.py:final=Falseon UTF-16LE/BE, UTF-8-with-BOM and BOM-less UTF-8 prefixes, plus a check thatfinal=Falsestill rejects an invalid byte before the end.
# Conflicts: # CHANGELOG.md # unstructured/__version__.py
- Skip the strict UTF-8 shortcut when the bytes contain NUL. BOM-less UTF-16/32 pads ASCII with NUL, and with a non-ASCII character such as U+8ACB (`CB 8A` in UTF-16LE) the whole buffer is also valid UTF-8, so it was decoded as UTF-8 with NUL padding and no EncodingDetectionWarning. - Add a keyword-only `final` to detect_file_encoding(). With final=False a multi-byte sequence cut off at the end is left out instead of failing. _FileTypeDetectionContext.text_head passes final=eof_reached, so a UTF-16 surrogate pair split at the 4,096-byte stream prefix after a BOM no longer raises UnprocessableEntityError. Complete-document decoding stays strict.
|
@cdcailf thanks for the independent run on 3.5.2. Your ASCII-only UTF-16/32 rows are now pinned: The review above found the case your counterfactual points toward, and it already fails on this branch with the @cragwolfe both P2 findings are addressed in bf795c2 (replies inline). The branch also merges current |
|
@huuufu reran bf795c2 locally — PyPI
One boundary note, information only — no change requested. The gap the guard leaves is the no-NUL buffer that is itself valid UTF-8: |
cragwolfe
left a comment
There was a problem hiding this comment.
GPT Pro review of bf795c2 found no actionable defects in the updated change.
Both findings from the previous review are resolved for their reported failure scenarios:
- NUL-bearing BOM-less UTF-16/32 input now reaches encoding detection instead of being claimed by the UTF-8 shortcut. Regression tests cover both byte orders, filename/bytes/stream inputs, and warning-as-error behavior.
- File-type inspection now passes the EOF distinction to incremental decoding, so a UTF-16 surrogate pair split at the 4,096-byte boundary does not reject a valid upload. Tests cover both byte orders, stream-position restoration, and rejection of genuinely truncated EOF input. Complete-document decoding remains strict.
Validation was inspection of the full updated implementation, relevant callers, and checked-in tests; no local tests were run. At publication, this head has 44 successful checks, seven still in progress, and one skipped. This approval covers the code review; the remaining CI results are still pending.
(authored by codex)
Summary
Fixes #4466.
detect_file_encoding()(unstructured/file_utils/encoding.py) turns text into mojibake without raising, by two routes:charset_normalizer.detect()returns once confidence clears 0.8. That includes valid UTF-8 whose only non-ASCII content is one invisible character, which the detector reports as a single-byte codec at confidence 1.0 (case from @cdcailf in bug/encoding-detection-returns-mojibake #4466).COMMON_ENCODINGSentry that decodes.iso_8859_1is at index 1 and decodes every byte, so it always wins and the CJK codecs behind it are unreachable.Everything that reads text through
read_txt_file()is affected:partition_text,partition_md,partition_html,partition_xml, and file-type detection.detect_file_encoding(file=...),mainvs this PR:main"…unique."cp1125,'unique.тАЛ'utf-8, correct"…unique."big5,'unique.簫'utf-8, correct髙﨑様と﨑山様…cp932, correct, silentcp932, correct, warnswindows-1254, correct, silentwindows-1254, correct, warns会議室予約(issue)big5hkscsmojibake, silentwindows-1250mojibake, silentThe detector result for very short inputs varies by
charset_normalizerversion: with 3.4.7 the zero-width case comes back ascp1125, and #4466 reportsmac_icelandwith 3.5.2. Either way, the result is wrong onmain.Change
Order of decisions in
detect_file_encoding()when no encoding is supplied:ascii.COMMON_ENCODINGS(e.g.cp932,cp1254). The issue's "Additional context" shows that turning that list into an allowlist breaks correctly detected Turkish.from_bytes(byte_data, enable_fallback=False).best()instead of taking the first codec that decodes. If nothing survives, the result isUnprocessableEntityErrorinstead of Latin-1 text.EncodingDetectionWarning(UserWarning)is emitted whenever a legacy encoding or BOM-less UTF-16/32 was inferred statistically. The message contains the codec, never document text.On the policy question left open in #4466 (which way to be wrong), this PR keeps the existing best-effort output and makes it observable instead of picking a new winner. Correct legacy detections keep working. Callers who prefer failure to possibly wrong text can promote the warning to an exception:
Callers who know the source encoding pass
encoding=, which is authoritative and decoded strictly.partition_md()now passesencoding=through toread_txt_file()forfilename/fileand uses it to decode URL response bytes. Before this change it was ignored on all three paths.Compatibility
UnprocessableEntityError. This happens whenfrom_bytes()finds no text candidate, i.e. binary-looking input.encoding=or filterEncodingDetectionWarning.Testing
test_unstructured/file_utils/test_encoding_detection_policy.py(new). Covers the issue samples via filename, bytes and stream; nine further legacy code pages; valid UTF-8 not overridden by a confident detector (the five characters from bug/encoding-detection-returns-mojibake #4466's table); BOM precedence and malformed BOM payloads; explicit encoding is authoritative; inconclusive ranking; binary rejection; path/stream parity including\r\n/\r; empty input; no warning for UTF-8/ASCII aliases.test_unstructured/partition/test_encoding_policy.py(new). Exercises explicitencoding=, the warning and warning-as-error throughpartition_text,partition_mdandpartition_html, pluspartition_md(url=..., encoding=...).test_charset_detection_failureno longer patchesCOMMON_ENCODINGS, so the real fallback path is exercised.pyproject.tomlignores the warning in the test suite by message, on its builtinUserWarningbase class. Existing tests on legacy fixtures (e.g.fake-text-utf-16-le.txt) now emit it. Inmake docker-testthe xdist controller cannot importunstructured(--no-install-project), so it crashed unserializing the worker'sEncodingDetectionWarning. This is the package's first custom warning class. Tests that check the warning usepytest.warns(), which overrides the filter.The 2 failures (
test_partition_text_from_filename_raises_econding_error[fake-text.txt-utf-16-…],test_partition_html_on_ideas_page) fail identically onmain(e2f22b38) on the same Windows machine and are unrelated.ruff check .andruff format --check .are clean.