Skip to content

fix: resolve stores from the connection config, not global dj.config - #6

Merged
MilagrosMarin merged 2 commits into
mainfrom
fix/thread-connection-config-through-codec
Sep 3, 2026
Merged

MilagrosMarin merged 2 commits into
mainfrom
fix/thread-connection-config-through-codec

Conversation

@MilagrosMarin

@MilagrosMarin MilagrosMarin commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

encode and decode call _build_path and _get_backend without config=, so both fall back to the module-level dj.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 own dj.Instance, and the global one belongs to none of them. The fetch path already passes the calling connection's config to every codec as key["_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:

DataJointError: Missing S3 configuration: access_key, secret_key

The keyless spec satisfies Config.get_store_spec, which checks key existence, and is rejected later by StorageBackend._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 object and npy codecs, which read key["_config"] and thread it through both helpers.

encode had the same omission on _build_path. That one is write-side, and it matters because _build_path reads schema_prefix off 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:

  • decode resolves the store from key["_config"] rather than the global one
  • decode with no _config still uses the global store, so ambient callers are unaffected
  • encode writes through the connection's backend
  • encode builds its path from the connection's schema_prefix, with backend resolution mocked so it stays red if only _get_backend is threaded

Each of the three config= sites was removed individually and fails its own test.

Versioning

No bump in this PR: pyproject.toml takes the version from the git tag via hatch-vcs, and the build hook writes _version.py — the release tag is the only place it lives. This wants to go out as 0.2.1, a patch: the read path changes behavior only where it was already failing.

The second commit is a drive-by. __init__.py also declared __version__ = "0.2.0" as a literal above the ._version import 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 moment 0.2.1 is cut. Drop that commit if you'd rather keep this PR to the fix alone.

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 ttngu207 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@MilagrosMarin
MilagrosMarin merged commit e379a86 into main Sep 3, 2026
2 checks passed
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.

3 participants