[BUGFIX] TupleFilesystemStoreBackend reads with ambient locale encoding - #12125
[BUGFIX] TupleFilesystemStoreBackend reads with ambient locale encoding#12125nanjeshramesh wants to merge 1 commit into
Conversation
TupleFilesystemStoreBackend wrote store values as UTF-8 explicitly but read them back with whatever encoding the process's locale happened to resolve to. Same story for the project YAML: InlineStoreBackend and FileDataContext wrote it under the ambient locale and read it back the same way, so a project written under one locale broke when reloaded under another. On Unix this mostly stays hidden because PEP 538 coerces the locale to UTF-8, but that coercion is POSIX only, so a non-UTF-8 Windows codepage hits this with no special setup at all. Pinned encoding="utf-8" on the four unpinned text-mode open() calls: the read side of TupleFilesystemStoreBackend, and both the read and write sides of the project YAML in InlineStoreBackend and FileDataContext. Nothing else changes, on-disk format and public signatures are the same as before. Left config_variables.yml in abstract_data_context.py alone, that's already called out as a separate concern. Also noticed a handful of similarly unpinned open() calls in serializable_data_context.py, but that file isn't part of what this issue asked for, so I didn't touch it.
👷 Deploy request for niobium-lead-7998 pending review.Visit the deploys page to approve it
|
joshua-stauffer
left a comment
joshua-stauffer
left a comment
There was a problem hiding this comment.
hi @nanjeshramesh, thanks for this PR, too! i've requested some additional test coverage, but otherwise this is close to shippable.
There was a problem hiding this comment.
Two of the four call sites this PR pins to UTF-8 have no regression test here. Reverting each pin individually and re-running this file: dropping encoding= from tuple_store_backend.py:318 fails test_tuple_filesystem_store_backend_reads_own_writes_under_non_utf8_locale, and dropping it from file_data_context.py:194 fails test_file_data_context_reloads_non_ascii_project_yaml_under_non_utf8_locale. Both read paths are genuinely covered. But dropping it from either write site, file_data_context.py:168 or inline_store_backend.py:222, leaves both tests passing — for two different reasons.
file_data_context.py:168 does execute: the second test's write subprocess reaches _save_project_config. It's masked because that subprocess runs with the inherited environment rather than env=NON_UTF8_ENV, so only the reload leg is locale-forced — an unpinned write still emits correct UTF-8 bytes under the ambient UTF-8 locale, leaving nothing for the assertion to catch. Forcing the non-UTF-8 locale on the write leg too would close this: a value GX writes should round-trip when both ends are non-UTF-8, not only when the reader is.
inline_store_backend.py:222 is different — it never executes here, under any environment. It's the file's only open(), and neither test reaches it: add_pandas persists fluent datasources through _save_project_config's own to_yaml, not through InlineStoreBackend._set/_save_changes. Closing this needs a test that actually drives that path — e.g. mutating a DataContextVariables-backed config value with a non-ASCII value and asserting it survives a save/reload round-trip.
The current shape is reasonable: it models the issue's own cross-host scenario — a project written on a UTF-8 host, read on a non-UTF-8 one — and fails pre-fix for both read paths. The gap is that a later refactor dropping encoding= from either write call would go uncaught.
Drafted by an automated review pass against this diff; verify before treating any claim as authoritative.
2 participants
Closes #12120.
Pinned encoding="utf-8" on the four text-mode open() calls the issue
named: the read side of TupleFilesystemStoreBackend
(tuple_store_backend.py), and both directions of the project YAML in
InlineStoreBackend (inline_store_backend.py) and FileDataContext
(file_data_context.py). The write side of the store backend was
already writing UTF-8 bytes explicitly in binary mode, so that one's
untouched.
Tests force a genuinely non-UTF-8 encoding the same way the issue's
own repro does, by spawning a subprocess with LC_ALL=C, LANG=C,
PYTHONCOERCECLOCALE=0, and PYTHONUTF8=0, rather than relying on the
ambient locale (which is UTF-8 in CI either way, so that wouldn't
actually test anything). Confirmed both new tests fail against
unpatched develop with the exact UnicodeDecodeError from the issue,
and pass with the fix.
Ran the full data_context test suite locally, 306 passed, 27 skipped,
1 xfailed, nothing broken.
Two things I noticed but left alone, both already flagged as out of
scope in the issue:
serializable_data_context.py, which isn't one of the three classes
this issue's requirements name
New test file:
tests/data_context/test_file_backed_encoding.py