Repository navigation
Conversation
cragwolfe
left a comment
There was a problem hiding this comment.
Requesting changes for four production blockers:
-
[P1] Preserve valid UTF-8 when no encoding is supplied (
unstructured/partition/json.py:87,90,unstructured/partition/ndjson.py:84,87). The old paths decoded bytes strictly as UTF-8. The new paths callread_txt_file(..., encoding=None), which accepts a sufficiently confidentcharset_normalizer.detect()guess before attempting UTF-8. With the lockedcharset-normalizer==3.4.7, valid UTF-8 JSON containing a literal zero-width space can be selected as CP932 and still parse successfully, changing the element text (for example,RAGbecomesR窶帰G). This is silent content corruption. Please make explicit encodings authoritative and, when encoding is omitted, preserve valid UTF-8 before falling back to non-UTF-8 detection. Add assertions on exact Unicode content for JSON and NDJSON through both filenames and binary streams; the current ASCII assertions do not cover this regression. -
[P1] Preserve the existing file-like contract (
unstructured/partition/json.py:90-91,unstructured/partition/ndjson.py:87-88). The removed implementation accepted any readable object and preserved already-decodedstrvalues. The new no-encoding path routes the live object throughconvert_to_bytes(), which only accepts a few concrete types, may reopen a stream by.name, and usesBytesIO.getvalue()rather than reading from the current cursor. Exact-head CI already demonstrates the regression: bothtest_unit (3.13)andtest_dockerfilefail intest_formskeysvalues_reads_savesbecausepartition_json(file=io.StringIO(...))now raisesValueError: Invalid file-like object type. Read the supplied stream once from its current position, passstrthrough, and decode buffered bytes without reopening or taking ownership of the caller's stream. Cover text streams, generic seekable binary streams, temporary-file wrappers, and nonzero cursor positions for both formats. -
[P1] Forward
encodingthrough the public dispatcher (unstructured/partition/auto.py:266-269).partition()bindsencodingas a named parameter and uses it for detection, but its JSON/NDJSON special case calls the selected partitioner with onlyfilename,file, and**kwargs. Because the bound value is not inkwargs,partition(..., encoding=...)still discards the caller's requested codec before parsing, leaving the stated top-level behavior unfixed and potentially producing different text or error behavior from a directpartition_json()/partition_ndjson()call. Passencoding=encodingexplicitly and add dispatcher-level tests for both formats using bytes that have different meanings or validity under competing codecs; a BOM-bearing UTF-16 success alone can pass through autodetection while the argument is still dropped. -
[P2] Keep the existing fourth positional argument compatible (
unstructured/partition/json.py:36-42,unstructured/partition/ndjson.py:36-42). Before this change,metadata_last_modifiedwas the fourth positional parameter. Insertingencodingahead of it silently rebinds existing calls: binary input attempts to use the timestamp as a codec and raisesLookupError, whiletext=input can succeed but lose the requested timestamp. Preserve the original parameter order and place the new parameter afterward (or otherwise make only the new parameter keyword-only), with regression coverage for four-positional-argument calls in both partitioners.
The branch also currently conflicts with main in CHANGELOG.md; that is operational follow-up after the content blockers above are addressed.
(authored by codex)
Both partitioners read the source with a hard-coded UTF-8:
with open(filename, encoding="utf8") as f:
file_text = f.read()
and, for a file-like object, a bare .decode() (also UTF-8). Neither
accepted an `encoding` parameter, so an `encoding=` passed by the caller
-- directly or via partition() -- landed in **kwargs and was discarded
without a warning. A UTF-16 document raised UnicodeDecodeError even when
its encoding was stated explicitly:
partition_json(filename="utf16.json", encoding="utf-16")
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 0
partition_text() and partition_csv() both take `encoding` and read
through read_txt_file(), which uses the given encoding when present and
detects it otherwise. Route the two JSON partitioners through the same
helper and give them the same parameter.
Reading a non-UTF-8 document without stating its encoding now works too,
since read_txt_file() falls back to detection.
…rward encoding Four fixes from review of Unstructured-IO#4483: 1. Valid UTF-8 is preserved when no encoding is supplied. Routing through `read_txt_file(..., encoding=None)` made charset detection authoritative, and `charset_normalizer` can report high confidence in a legacy codec for bytes that are also valid UTF-8 -- a run of zero-width spaces is detected as CP932 at 0.907 and still parses as JSON, so the text was silently rewritten. Decoding now tries UTF-8 first and falls back to detection only for bytes that are not valid UTF-8. An explicit `encoding` stays authoritative and is used as given. 2. The file-like contract is restored. `convert_to_bytes()` accepts only a few concrete types, may reopen a stream by `.name`, and reads via `BytesIO.getvalue()` rather than from the cursor, which broke `partition_json(file=io.StringIO(...))`. The stream is again read once from its current position, an already-decoded `str` passes through, and the object is never reopened. 3. `partition()` forwards `encoding` to the JSON/NDJSON partitioners. It is bound as a named parameter, so it never reached them through `**kwargs` and the dispatcher silently dropped the caller's codec. 4. `metadata_last_modified` is the fourth positional parameter again; `encoding` follows it, so existing four-positional-argument calls keep binding as before. Both partitioners now share one `read_json_text()` helper so the two paths cannot drift apart.
21ca66c to
fb8f5bb
Compare
|
Thanks — all four are addressed, and the branch is rebased onto current I reproduced each one before changing anything, against 1. Valid UTF-8 is preserved when no encoding is supplied. Confirmed, and worth recording how it reproduces: my first attempt did not show corruption because Decoding now tries UTF-8 first and only falls back to detection for bytes that are not valid UTF-8. An explicit 2. The file-like contract is restored. Reproduced exactly as CI showed — 3. 4. Positional order is preserved. Both partitioners now share one Verification
One note on test placement: the dispatcher tests live in |
|
@cragwolfe Requesting a re-review when you have a moment — all four P1s from your 09-15 review were addressed in the 09-16 push (head Not re-litigating the detail here since it is in my 09-16 reply, just the state: CI on this head is green across the board, and there are no unresolved review threads left. The only thing holding If anything in the responses did not land the way you intended, I would rather hear that than have it sit — happy to keep iterating. |
|
Gentle follow-up: head |
cragwolfe
left a comment
There was a problem hiding this comment.
Re-reviewed exact head fb8f5bb74cc3f3f3ec2f8bf660e48b62e2b9cdf8 against fresh main (0b7f806e0f6ef540df22fad5627756a72663eb11), with a complete production-context Oracle review using server-confirmed GPT-6 Pro. No actionable introduced source defects remain. The four September 15 blockers are addressed: valid UTF-8 is preserved before fallback detection, caller streams are read from their current position without reopening or taking ownership, explicit encodings reach both partitioners through partition(), and the fourth positional argument remains metadata_last_modified.
This approval covers the reviewed source change. The PR still has substantive conflicts in unstructured/partition/json.py and test_unstructured/partition/test_json.py; CHANGELOG.md merges cleanly. Resolve using main's JSON identity/metadata processing from #4504, then integrate the encoding parameter and reader without restoring the old processing decorators.
Main's #4550 now supplies a BOM-aware, NUL-aware shared decoding policy. Keep the new read-once/string-pass-through stream acquisition and authoritative explicit decoding; prefer that shared policy for omitted-encoding buffered bytes. Do not pass the live caller stream into the detector. Main still lacks the JSON/NDJSON input-encoding support and dispatcher forwarding, so it does not supersede the whole PR. If default decoding is consolidated, make the dispatcher tests assert forwarding deterministically: BOM-less UTF-16 may now succeed through detection even if forwarding is removed.
Exact-head CI has 53 completed checks: 52 success and one neutral, including all six required checks. They completed September 16 and do not validate today's main integration. Preserve both sets of regression tests and obtain fresh CI on the conflict-resolved head before merging. No local tests were executed for this review.
(authored by codex)
…for omitted encoding
# Conflicts: # test_unstructured/partition/test_json.py # unstructured/partition/json.py
cragwolfe
left a comment
There was a problem hiding this comment.
Fresh Oracle Pro production re-review of exact head 240b72904eede76d206f23deb490e49a662cef63 against current main (0b7f806e0f6ef540df22fad5627756a72663eb11). One verified P2 compatibility regression remains: the new named encoding parameter is omitted from JSON's manual chunker dispatch. The inline comment gives the failure case and fix. This is limited to registered chunkers that accept encoding; built-in chunkers are unaffected. No other actionable source defect was established.
The integration has resolved the prior JSON/source-test conflicts, preserved #4504's JSON identity/metadata processing, and adopted #4550's shared BOM/NUL-aware detector on buffered bytes. Explicit byte decoding, stream acquisition/ownership, fourth-position metadata_last_modified, and dispatcher forwarding are intact. I fetched all current issue comments, reviews, inline comments and thread replies; no posted discussion newer than the previous approval was returned. The subsequent integration commits were reviewed afresh.
Coverage follow-up: the two dispatcher tests still assume BOM-less UTF-16 cannot autodetect. That premise no longer proves forwarding with the shared detector. Add a deterministic assertion on the received codec, or make fallback decoding fail in the explicit-encoding case. Current dispatcher forwarding is correct, so this is not a separate source defect.
Merge readiness remains unvalidated: CI, CodeQL, Security and Partition Benchmark are action_required, with zero jobs on this head. The three passing external checks do not supply unit/integration execution; Cubic explicitly skipped AI review for this merge commit. Historical September CI does not cover this integration. The current merge tree is clean. After the chunker fix, obtain workflow execution and review on the new exact head before merging.
Oracle was available as server-confirmed gpt-6-pro, with Pro verified before submission and while streaming. Its complete final answer was recovered from the same conversation after CLI capture detached. Findings were verified against source and tests as code; no local tests or source changes were made.
(authored by codex)
| file: Optional[IO[bytes]] = None, | ||
| text: Optional[str] = None, | ||
| metadata_last_modified: Optional[str] = None, | ||
| encoding: Optional[str] = None, |
There was a problem hiding this comment.
[P2] Preserve encoding when dispatching registered JSON chunkers
Binding encoding here removes it from kwargs, but _apply_chunking() only reconstructs filename, file, text, metadata_last_modified and the remaining kwargs. Both calls therefore drop the supplied codec. On current main, an explicit encoding stayed in kwargs and reached a registered chunker that declared it. For example, a chunker with a required keyword-only encoding now raises TypeError for partition_json(text='{}', encoding='utf-8', chunking_strategy='custom'); an optional codec parameter silently uses its default. NDJSON's decorator still forwards named arguments.
Pass the bound codec into _apply_chunking() and its argument map from both the rehydration and arbitrary-JSON paths, while retaining #4504's manual finalization. Extend the existing complete-call-arguments capture test to declare and assert encoding for rehydrated, arbitrary and empty input. Built-in chunkers need no behavior change.
(authored by codex)
The manual _apply_chunking dispatch built its call arguments from filename/file/text/metadata_last_modified only, dropping the named encoding parameter, so registered chunkers that declare encoding never received the caller's codec (built-in chunkers are unaffected). Forward encoding in both the rehydration and arbitrary-JSON paths. Extend the complete-call-arguments capture test to assert encoding for rehydrated, arbitrary and empty input, and make the dispatcher encoding tests deterministic by patching fallback detection to fail: the shared BOM/NUL-aware detector can recognize BOM-less UTF-16 on its own, so a successful decode no longer proves the explicit codec was forwarded. Reformat the _decode_bytes docstring so ruff format --check passes.
|
Follow-up on the 10-14 P2 note (chunker dispatch / coverage follow-up), addressed on head
Local: |
cragwolfe
left a comment
There was a problem hiding this comment.
Re-reviewed exact head c4cebcf3a5a7507f28ea3f9f18ec32ee9a13f448 against current main (3253c2b0f6b0a5f0808910c085675672aff718f3), with a complete production-context GPT Pro review. No actionable introduced source defects remain.
The prior P2 is fixed: JSON forwards encoding into registered chunkers through both rehydration and arbitrary/empty-JSON finalization. The two dispatcher tests now make fallback detection raise, so their successful explicit-codec reads prove forwarding. The integration preserves authoritative explicit decoding, the shared BOM/NUL-aware default detector, read-once acquisition from caller streams, fourth-position metadata_last_modified, and main's JSON identity/metadata processing. Findings and test coverage were checked against source; no local tests were run.
All 51 exact-head checks now pass, including all six required checks and the CI, Security, CodeQL, and Partition Benchmark workflows. This approval covers the reviewed source. The PR remains behind main, and branch protection requires it to be current before merge. After updating the branch, obtain checks on the resulting head before merging.
(authored by codex)
…-IO#4483) Resolve CHANGELOG.md and __version__.py against current main; reviewed encoding source and regression blobs are unchanged.
|
Integration note (no review or merge action taken): merged the latest Local checks:
The same 3 failures reproduce on a clean Fork CI required jobs still need workflow approval on this head. I have not approved, requested changes, resolved a thread, or merged. |
Drops the `-dev6` suffix so 0.27.34 can be tagged and published. There are no code changes. Since 0.27.33, this release contains: - Free decode buffers sooner when restoring original elements (Unstructured-IO#4524) - Free image buffers sooner during figure extraction (Unstructured-IO#4523) - Honor `encoding=` in `partition_json` and `partition_ndjson` (Unstructured-IO#4483) - Read a PowerPoint line break as a newline (Unstructured-IO#4571) - Drop the empty entry of an email address group (Unstructured-IO#4576) - Read CSV and TSV cells as the file holds them (Unstructured-IO#4574) - Classify text as an email address only when all of it is one (Unstructured-IO#4577) After merge, create the `0.27.34` GitHub release from the merge commit. That publishes to PyPI. <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/Unstructured-IO/unstructured/pull/4582?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="View guided diff" src="https://www.cubic.dev/buttons/review-in-cubic-light.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
Problem
partition_json()andpartition_ndjson()read the source with a hard-codedUTF-8, and neither accepts an
encodingparameter:Because both signatures end in
**kwargs: Any, anencoding=passed by thecaller — directly, or forwarded by
partition()— is swallowed silently. Onmain(ee2b3a35):The same call succeeds for the sibling text formats, which is where the
inconsistency shows:
encodingpartition_textread_txt_file()partition_csvpartition_jsonopen(..., encoding="utf8")partition_ndjsonopen(..., encoding="utf8")UTF-16 JSON is not exotic — it is what several Windows tools emit by default,
and
json.dumpto a file opened in a non-UTF-8 encoding produces it too.Fix
Add
encodingto both signatures and read through the existingread_txt_file()helper, exactly aspartition_text()does:read_txt_file()uses the given encoding when one is supplied and falls back todetect_file_encoding()when it is not, so this also makes a non-UTF-8 documentreadable without naming its encoding. The
text=path is untouched, andfile.seek(0)is preserved so the detect-then-partition sequence over one handlestill works.
Source change is +11/-8 across the two files; no new helper, no behaviour change
for UTF-8 input.
Verification
The 35 failures are pre-existing on
mainin this environment — they come fromunstructured/nlp/tokenize.pyfailing to import an optional dependency(
ModuleNotFoundError: No module named 'installer'), unrelated to these files.I compared the failure sets with
git stash: identical before and after, andthe pass count goes 70 → 75, which is exactly the five tests added here.
Five regression tests, following the existing naming style in both files:
encoding="utf-16"forfilename=(both partitioners)encoding="utf-16"forfile=(both partitioners)encodinggiven, UTF-16 detected (json)Rollback proof. Restoring just the two read blocks to the hard-coded form —
leaving the new parameter in place — fails exactly those five and nothing else:
each with
UnicodeDecodeError.ruff checkon the four changed files is clean,and a CHANGELOG entry is added under the current
0.27.8-dev0section.Notes
This is the parsing side. There are open PRs (#4397, #4398, #4447) addressing
non-UTF-8 crashes during file-type detection in
file_utils/filetype.py; Ichecked all currently open PRs and none touches
partition/json.pyorpartition/ndjson.py, so this does not overlap with them.I kept the scope to honoring the parameter. I did not change what happens when a
document really is undecodable, nor touch the
text=path.