Skip to content

Keep short non-ASCII UTF-8 text from being guessed as UTF-16 - #2674

Open
Franco Ceballos Rastello (xThreeh) wants to merge 1 commit into
microsoft:mainfrom
xThreeh:fix/prefer-valid-utf8
Open

Franco Ceballos Rastello (xThreeh) wants to merge 1 commit into
microsoft:mainfrom
xThreeh:fix/prefer-valid-utf8

Conversation

@xThreeh

Copy link
Copy Markdown

A short UTF-8 text file can be decoded as UTF-16 and come out garbled:

import io
from markitdown import MarkItDown, StreamInfo

data = "AAAㇰ".encode("utf-8")
MarkItDown().convert_stream(io.BytesIO(data), stream_info=StreamInfo(extension=".txt")).markdown
# '䅁䇣螰' (expected 'AAAㇰ')

_get_stream_info_guesses takes the charset from charset_normalizer.from_bytes(sample).best(), which reads this 6-byte sample as utf_16_be. The plain text converter then decodes with that charset. Longer text such as "Hi café" is detected correctly, so this shows up on short files and short ZIP members.

When the sample contains non-ASCII bytes and decodes as UTF-8, this uses utf-8. ASCII-only samples keep the detector's answer (ascii, as the existing vectors expect), and other encodings such as the cp932 vector still fail the UTF-8 decode and keep it too.

Tests: test_short_utf8_text_is_not_guessed_as_utf16 fails on main and passes here. The full suite has the same results with and without the change on my machine (Windows): the 6 failures in test_cli_vectors.py also fail on main. black 23.7.0 (the pre-commit version) leaves both files unchanged.

charset_normalizer can read a short UTF-8 sample as UTF-16-BE, so a .txt
file holding "ok ✓" converts to "潫⃢鲓". When the sample has non-ASCII
bytes that decode as UTF-8, the stream is UTF-8; ASCII-only samples and
other encodings still use the detector's answer.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a community review. It does not clear the merge gate.

I read the diff and ran it. The change looks correct to me.

What I ran (Windows, Python 3.13, PR head b0550a3 against main 4cc9fa1):

  • convert_stream with StreamInfo(extension=".txt") on short UTF-8 inputs. On main, "AAAㇰ" comes out as "䅁䇣螰" and "ok ✓" as "潫⃢鲓". On the PR head both come back correct.
  • Inputs that were already fine on main stay fine on the PR: "日本語", "Hi café", "é", a BOM-less UTF-16-LE and UTF-16-BE Japanese string, UTF-16 with a BOM, and cp932 "こんにちは".
  • tests/test_module_misc.py on the PR head: 171 passed, 1 failed, 1 skipped. The failure is test_speech_transcription, which also fails on main (169 passed, 1 failed, 1 skipped there, before the new tests). I did not look into why it fails. I assume it is the missing ffmpeg/ffprobe warning on this machine, but I did not confirm that.
  • I copied the PR's test_short_utf8_text_is_not_guessed_as_utf16 onto main. Both parametrized cases fail there and pass on the PR.

I did not run test_cli_vectors.py or the rest of the suite.

Notes, none blocking:

  • The override reads stream_page, which is the 64 KiB sample from _read_charset_sample. That helper already completes a split UTF-8 character at the boundary, so a large UTF-8 file will not fall into the except UnicodeDecodeError branch only because the sample was cut mid-character. I read this and did not test a file over 64 KiB.
  • The override also runs when charset_result is None, and it replaces any charset the detector returned, not just UTF-16. I think that is intended. A short cp1252 or cp1251 sample that happens to be valid UTF-8 would now be called UTF-8, which seems the right call.
  • A possible overlap with #2679 (cp1252 tie-breaking) since both touch charset choice in _get_stream_info_guesses. I did not check whether they conflict.
  • Separate from this PR: short single-byte text such as "héllo wörld" in cp1252 and "привет мир" in cp1251 is still garbled, on main and on the PR. I only mention it so nobody expects this PR to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants