Skip to content

Commit f494a68

Browse files
refactor(storage): drop builder prefix fallbacks — settings is the single source
Review finding: build_hash_path/build_object_path carried default prefix parameters (DEFAULT_HASH_PREFIX/DEFAULT_SCHEMA_PREFIX) duplicating the defaults that settings already applies. _apply_common_store_defaults runs for EVERY store spec — built-in and plugin protocols alike — before get_store_spec returns, so the fallbacks were dead code and a second place for the default to drift. The prefixes are now required keyword-only parameters; both constants are deleted; all production callers index the spec directly (spec['hash_prefix'] / spec['schema_prefix']) with a loud KeyError, rather than a silent wrong-layout write, as the failure mode for any spec that bypassed get_store_spec. Direct builder calls in tests pass the prefix explicitly, which is the honest contract. Suites: hash-storage/object/unit 377 passed; gc/npy/chaining green.
1 parent c74a830 commit f494a68

7 files changed

Lines changed: 28 additions & 34 deletions

File tree

src/datajoint/builtin_codecs/schema.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ def _build_path(
150150
spec = config.get_store_spec(store_name)
151151
partition_pattern = spec.get("partition_pattern")
152152
token_length = spec.get("token_length", 8)
153-
schema_prefix = spec.get("schema_prefix", "_schema")
153+
schema_prefix = spec["schema_prefix"] # always present: settings applies the default
154154

155155
return build_object_path(
156156
schema=schema,

src/datajoint/gc.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@
4141
import logging
4242
from typing import TYPE_CHECKING, Any
4343

44-
from .hash_registry import DEFAULT_HASH_PREFIX, delete_path, get_store_backend
44+
from .hash_registry import delete_path, get_store_backend
4545
from .errors import DataJointError
4646

4747
if TYPE_CHECKING:
@@ -286,7 +286,7 @@ def list_stored_hashes(store_name: str | None = None, config=None) -> dict[str,
286286
# value remain readable (their metadata stores full paths) but are not
287287
# candidates for reclamation until the setting is restored.
288288
_spec = config.get_store_spec(store_name)
289-
hash_prefix = _spec.get("hash_prefix", DEFAULT_HASH_PREFIX).strip("/") + "/"
289+
hash_prefix = _spec["hash_prefix"].strip("/") + "/" # settings applies the "_hash" default
290290
# Base32 pattern: 26 lowercase alphanumeric chars
291291
base32_pattern = re.compile(r"^[a-z2-7]{26}$")
292292

@@ -363,7 +363,7 @@ def list_schema_paths(store_name: str | None = None, config=None) -> dict[str, i
363363
_spec = config.get_store_spec(store_name)
364364
_fp = _spec.get("filepath_prefix")
365365
filepath_prefix = (_fp.strip("/") + "/") if _fp else None
366-
_hp = _spec.get("hash_prefix", DEFAULT_HASH_PREFIX).strip("/")
366+
_hp = _spec["hash_prefix"].strip("/") # settings applies the "_hash" default
367367
hash_section = _hp + "/" if _hp else None
368368

369369
try:

src/datajoint/hash_registry.py

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -48,14 +48,6 @@
4848
logger = logging.getLogger(__name__.split(".")[0])
4949

5050

51-
# Default section prefix for hash-addressed storage. The authoritative value
52-
# is the per-store `hash_prefix` setting (default "_hash", see
53-
# settings.get_store_spec and docs/how-to/configure-storage) — writers, GC,
54-
# and <filepath@> validation all consume the same setting so the layout can
55-
# be relocated per store without the components drifting apart.
56-
DEFAULT_HASH_PREFIX = "_hash"
57-
58-
5951
def compute_hash(data: bytes) -> str:
6052
"""
6153
Compute Base32-encoded MD5 hash of content.
@@ -105,7 +97,8 @@ def build_hash_path(
10597
content_hash: str,
10698
schema_name: str,
10799
subfolding: tuple[int, ...] | None = None,
108-
hash_prefix: str = DEFAULT_HASH_PREFIX,
100+
*,
101+
hash_prefix: str,
109102
) -> str:
110103
"""
111104
Build the storage path for hash-addressed storage.
@@ -126,9 +119,10 @@ def build_hash_path(
126119
Database/schema name for isolation.
127120
subfolding : tuple[int, ...], optional
128121
Subfolding pattern from store config. None means flat (no subfolding).
129-
hash_prefix : str, optional
130-
Section prefix from the store's ``hash_prefix`` setting
131-
(default ``"_hash"``).
122+
hash_prefix : str
123+
Section prefix from the store's ``hash_prefix`` setting. There is no
124+
fallback here by design: settings (``get_store_spec``) is the single
125+
source of the ``"_hash"`` default, applied to every store spec.
132126
133127
Returns
134128
-------
@@ -240,7 +234,7 @@ def put_hash(
240234
content_hash,
241235
schema_name,
242236
subfolding,
243-
hash_prefix=spec.get("hash_prefix", DEFAULT_HASH_PREFIX),
237+
hash_prefix=spec["hash_prefix"], # always present: settings applies the default
244238
)
245239

246240
backend = get_store_backend(store_name, config=config)

src/datajoint/staged_insert.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ def _resolve_field(self, field: str, ext: str) -> tuple[str, "StorageBackend"]:
120120
ext=ext if ext else None,
121121
partition_pattern=partition_pattern,
122122
token_length=token_length,
123-
schema_prefix=spec.get("schema_prefix", "_schema"),
123+
schema_prefix=spec["schema_prefix"], # always present: settings applies the default
124124
)
125125

126126
self._staged_objects[field] = {

src/datajoint/storage.py

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -188,12 +188,6 @@ def encode_pk_value(value: Any) -> str:
188188
return s
189189

190190

191-
# Default section prefix for schema-addressed storage. The authoritative value
192-
# is the per-store `schema_prefix` setting (default "_schema", see
193-
# settings.get_store_spec and docs/how-to/configure-storage).
194-
DEFAULT_SCHEMA_PREFIX = "_schema"
195-
196-
197191
def build_object_path(
198192
schema: str,
199193
table: str,
@@ -202,7 +196,8 @@ def build_object_path(
202196
ext: str | None,
203197
partition_pattern: str | None = None,
204198
token_length: int = 8,
205-
schema_prefix: str = DEFAULT_SCHEMA_PREFIX,
199+
*,
200+
schema_prefix: str,
206201
) -> tuple[str, str]:
207202
"""
208203
Build the storage path for an object attribute.
@@ -223,9 +218,10 @@ def build_object_path(
223218
Partition pattern with ``{attr}`` placeholders.
224219
token_length : int, optional
225220
Length of random token suffix. Default 8.
226-
schema_prefix : str, optional
227-
Section prefix from the store's ``schema_prefix`` setting
228-
(default ``"_schema"``).
221+
schema_prefix : str
222+
Section prefix from the store's ``schema_prefix`` setting. No fallback
223+
by design: settings (``get_store_spec``) is the single source of the
224+
``"_schema"`` default, applied to every store spec.
229225
230226
Returns
231227
-------

tests/integration/test_hash_storage.py

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -58,33 +58,33 @@ class TestBuildHashPath:
5858
def test_builds_flat_path(self):
5959
"""Test that path is built as _hash/{schema}/{hash}."""
6060
test_hash = "abcdefghijklmnopqrstuvwxyz"[:26] # 26 char base32
61-
result = build_hash_path(test_hash, "my_schema")
61+
result = build_hash_path(test_hash, "my_schema", hash_prefix="_hash")
6262

6363
assert result == f"_hash/my_schema/{test_hash}"
6464

6565
def test_builds_subfolded_path(self):
6666
"""Test path with subfolding."""
6767
test_hash = "abcdefghijklmnopqrstuvwxyz"[:26]
68-
result = build_hash_path(test_hash, "my_schema", subfolding=(2, 2))
68+
result = build_hash_path(test_hash, "my_schema", subfolding=(2, 2), hash_prefix="_hash")
6969

7070
assert result == f"_hash/my_schema/ab/cd/{test_hash}"
7171

7272
def test_rejects_invalid_hash(self):
7373
"""Test that invalid hash raises error."""
7474
with pytest.raises(DataJointError, match="Invalid content hash"):
75-
build_hash_path("not-a-hash", "my_schema")
75+
build_hash_path("not-a-hash", "my_schema", hash_prefix="_hash")
7676

7777
with pytest.raises(DataJointError, match="Invalid content hash"):
78-
build_hash_path("a" * 64, "my_schema") # Too long
78+
build_hash_path("a" * 64, "my_schema", hash_prefix="_hash") # Too long
7979

8080
with pytest.raises(DataJointError, match="Invalid content hash"):
81-
build_hash_path("ABCDEFGHIJKLMNOPQRSTUVWXYZ"[:26], "my_schema") # Uppercase
81+
build_hash_path("ABCDEFGHIJKLMNOPQRSTUVWXYZ"[:26], "my_schema", hash_prefix="_hash") # Uppercase
8282

8383
def test_real_hash_path(self):
8484
"""Test path building with a real computed hash."""
8585
data = b"test content"
8686
content_hash = compute_hash(data)
87-
path = build_hash_path(content_hash, "test_schema")
87+
path = build_hash_path(content_hash, "test_schema", hash_prefix="_hash")
8888

8989
# Verify structure: _hash/{schema}/{hash}
9090
parts = path.split("/")

tests/integration/test_object.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ def test_build_object_path_basic(self):
8080
field="data_file",
8181
primary_key={"id": 123},
8282
ext=".dat",
83+
schema_prefix="_schema",
8384
)
8485
assert "myschema" in path
8586
assert "MyTable" in path
@@ -96,6 +97,7 @@ def test_build_object_path_no_extension(self):
9697
field="data_folder",
9798
primary_key={"id": 456},
9899
ext=None,
100+
schema_prefix="_schema",
99101
)
100102
assert not path.endswith(".")
101103
assert "data_folder_" in path
@@ -108,6 +110,7 @@ def test_build_object_path_multiple_pk(self):
108110
field="raw_data",
109111
primary_key={"subject_id": 1, "session_id": 2},
110112
ext=".zarr",
113+
schema_prefix="_schema",
111114
)
112115
assert "subject_id=1" in path
113116
assert "session_id=2" in path
@@ -121,6 +124,7 @@ def test_build_object_path_with_partition(self):
121124
primary_key={"subject_id": 1, "session_id": 2},
122125
ext=".dat",
123126
partition_pattern="{subject_id}",
127+
schema_prefix="_schema",
124128
)
125129
# subject_id should be at the beginning due to partition
126130
# section prefix first, then partition attrs (schema_prefix default "_schema")

0 commit comments

Comments
 (0)