fix: resolve stores from the connection config, not global dj.config - #6
Conversation
encode and decode called _build_path and _get_backend without config=, so both
fell back to the module-level dj.config. In a process serving many users each
connection carries its own store credentials on dj.Instance.config, while the
global config holds only what the image was built with. On a pod with no
ambient AWS credentials that surfaced as
DataJointError: Missing S3 configuration: access_key, secret_key
on every figpack column, because the keyless spec satisfies
Config.get_store_spec (key existence) and is only rejected later by
StorageBackend._validate_spec (truthiness). encode had the same omission on
_build_path, which reads schema_prefix off the store spec and so wrote under
the wrong prefix.
Matches the built-in object and npy codecs, which thread config through both
helpers. Callers that pass no _config are unaffected and keep resolving
against the global store.
pyproject sets the version from the git tag via hatch-vcs and writes _version.py at build time, which the package imports. The literal above that import was dead — the import overwrites it — so it only ever drifted, and it had to be hand-synced on each release. __all__ also listed __version__ twice.
ttngu207
left a comment
There was a problem hiding this comment.
Approve. The fix is 3 lines matching the built-in object/npy pattern, and the tests are the strong part — four cases, each mutation-checked against removing one of the three config= sites. Test #4 in particular is what makes the _build_path threading a fix and not lint: mocking _get_backend isolates the path-side defect, and schema_prefix comes off the store spec, so a codec threaded only through _get_backend would still write under the wrong prefix silently. Test #2 pins the ambient-caller fallback so workers stay unaffected.
The __init__.py drive-by is a real correctness fix — the ._version import overwrote the literal, so it was dead and would go silently stale on the next release. Happy to see it stand alongside the codec fix.
encodeanddecodecall_build_pathand_get_backendwithoutconfig=, so both fall back to the module-leveldj.config. When a caller holds several connections with different store configurations — a web app serving multiple users, say — each connection's config lives on its owndj.Instance, and the global one belongs to none of them. The fetch path already passes the calling connection's config to every codec askey["_config"]; this codec was discarding it.Where the global config has a store of the same name but no usable credentials, the failure is a raise at decode:
The keyless spec satisfies
Config.get_store_spec, which checks key existence, and is rejected later byStorageBackend._validate_spec, which checks truthiness. Where the global config has a different usable store, there is no error at all — the codec silently reads the wrong one.The fix matches the built-in
objectandnpycodecs, which readkey["_config"]and thread it through both helpers.encodehad the same omission on_build_path. That one is write-side, and it matters because_build_pathreadsschema_prefixoff the store spec, so a global-config lookup builds paths under the wrong prefix. Included here since it's the same defect; say the word if you'd rather it were a separate change.Tests
tests/test_config_threading.py, 4 new, suite 26 → 30:decoderesolves the store fromkey["_config"]rather than the global onedecodewith no_configstill uses the global store, so ambient callers are unaffectedencodewrites through the connection's backendencodebuilds its path from the connection'sschema_prefix, with backend resolution mocked so it stays red if only_get_backendis threadedEach of the three
config=sites was removed individually and fails its own test.Versioning
No bump in this PR:
pyproject.tomltakes the version from the git tag viahatch-vcs, and the build hook writes_version.py— the release tag is the only place it lives. This wants to go out as0.2.1, a patch: the read path changes behavior only where it was already failing.The second commit is a drive-by.
__init__.pyalso declared__version__ = "0.2.0"as a literal above the._versionimport that overwrites it, and__all__listed__version__twice. The literal was dead, but it had to be hand-synced on each release and would go stale the moment0.2.1is cut. Drop that commit if you'd rather keep this PR to the fix alone.