Review — two simpler alternatives to consider

Good work, Kushal. The need is real: third-party packages should be able to register new storage backends without monkey-patching. Before we commit to the ABC approach, I want to explore whether we can achieve the same result with less machinery — or with machinery that also improves the existing code.

Background: what StorageBackend actually does on top of fsspec

Looking at `storage.py`, there are five protocol-specific concerns:

1. Config validation — different required keys per protocol
2. Constructor arg mapping — translating DJ config → fsspec constructor params (e.g., S3 endpoint → `client_kwargs`)
3. Path construction — cloud protocols prepend bucket; file:// uses pathlib
4. URL generation — file:// needs absolute path resolution; clouds are `protocol://path`
5. Atomic writes — file:// uses `safe_copy`/`safe_write`; clouds delegate to fsspec

~80% of I/O already delegates to fsspec directly. The protocol-specific code is concentrated in items 1-4, spread across ~26 `if self.protocol == "..."` branches.

fsspec already has its own protocol discovery — `fsspec.filesystem("s3")` auto-discovers S3FileSystem. So the question: what does DataJoint need to add on top of fsspec's own plugin system?

Alternative A — Generic fsspec passthrough

The lightest option: make `StorageBackend` accept any fsspec-supported protocol by default. No new ABC, no entry points, no registry.

For unknown protocols:

• Pass non-DataJoint config keys directly to `fsspec.filesystem(protocol, **kwargs)`
• Use generic defaults for path (`location/relpath`) and URL (`protocol://path`)

~30 lines of changes. No new module. Works for any fsspec filesystem immediately. Downside: no config validation or custom path/URL logic for unknown protocols.

Alternative B — Unified adapter model for ALL protocols (preferred)

Route all backends through adapters — including the existing four. The 26 hardcoded `if self.protocol == "..."` branches become adapter implementations:

```python
class S3Adapter(StorageAdapter):
protocol = "s3"

def create_filesystem(self, spec):
    endpoint = spec["endpoint"]
    if not endpoint.startswith(("http://", "https://")):
        endpoint_url = f"{'https' if spec.get('secure') else 'http'}://{endpoint}"
    else:
        endpoint_url = endpoint
    return fsspec.filesystem("s3", client_kwargs={"endpoint_url": endpoint_url}, ...)

def validate_spec(self, spec):
    missing = [k for k in ("endpoint", "bucket", "access_key", "secret_key") if k not in spec]
    if missing:
        raise DataJointError(f"S3 store missing: {', '.join(missing)}")

```

Built-in adapters (File, S3, GCS, Azure) registered at import time. Third-party adapters discovered via entry points. `StorageBackend` becomes a thin dispatcher — one code path for all protocols.

This eliminates the 26 hardcoded protocol branches, makes third-party backends first-class citizens alongside built-ins, and gives us the right abstraction. The user-facing API (`dj.config.stores`) doesn't change at all — this is purely internal refactoring. We test against the existing integration test suite and release within the 2.2.x series when confident.

Assessment

Your current PR has the right instincts — ABC, entry points, lazy discovery. But it creates a two-tier system: hardcoded paths for built-in protocols, adapter path for everything else. That means maintaining both code paths going forward.

I'd prefer Alternative B: if we're introducing the adapter abstraction, route everything through it. The existing integration tests validate that the built-in adapters behave correctly. Since the user API doesn't change, this is safe to land in 2.2.x with thorough testing.

@kushalbakshi — would you be up for reworking this PR to route the four existing protocols through adapters as well? The `StorageAdapter` ABC and entry-point discovery from your PR are the right foundation; the additional work is extracting the existing protocol-specific logic from `StorageBackend` into adapter subclasses.

Thanks for the detailed review, Dimitri. I agree the two-tier pattern is a real architectural smell and that routing all backends through a unified interface is the right end state. Happy to own that migration — but I want to push back on bundling it into this PR, for three reasons.

1. Alt A (pure fsspec passthrough) isn't a substitute for the adapter layer. fsspec's entry-point discovery (fsspec.specs group) does work, but StorageBackend adds value on top: config-key conventions (access_key/endpoint → fsspec's key/endpoint_url), path/URL construction semantics, and the atomic-write guarantees from safe_write/safe_copy for the file backend. Pure passthrough regresses on all three. Flagging explicitly so it's on record.

2. The StorageAdapter interface as drafted eliminates only about a third of the protocol branches — the remaining ones are the architecturally hard ones. The ABC (both the one in this PR and the one in your sketch — protocol, create_filesystem, validate_spec, with full_path/get_url added here) collapses the if self.protocol == "..." dispatch in _create_filesystem (storage.py:334–371), _full_path (373–405), get_url (407–452), and _validate_spec (314–324) — 12 of the ~30 protocol-conditional branches in storage.py. The other ~19 live inside the I/O methods themselves and are not addressable via the current interface:

These branches aren't incidental. safe_write and safe_copy (utils.py:131, 153) write to a .saving/.copying temp file and then rename — an atomicity contract (documented in their docstrings) that fsspec.LocalFileSystem.put_file / pipe_file don't provide by default. The shutil-based recursive operations likewise have semantics that don't map 1:1 onto fsspec.put / fsspec.rm. These semantics are load-bearing for how DataJoint users rely on external storage — partial-write corruption or surprising recursive-op behavior would be serious regressions for existing pipelines. There are multiple reasonable ways to unify the interface, each with materially different risk profiles, and that design discussion belongs in a dedicated issue rather than in this PR's review cycle.

3. Unit test coverage on the built-in protocols is thin. The only dedicated unit test touching StorageBackend per-protocol logic is test_settings.py::test_get_store_spec_file_protocol; the rest sits in integration tests against live backends. Refactoring all four built-ins without a per-protocol unit-test harness in place is a real regression risk for existing users — especially for the atomic-write and recursive-op semantics called out in point 2, which integration tests don't typically stress.

Proposal. Land this PR as-is — it's strictly additive (else: extensions in _create_filesystem, _full_path, get_url, and get_store_spec; zero change to any of the four built-in code paths; no regression surface) — and open a tracked follow-up issue for the unified-adapter migration with a concrete target date. That follow-up would take the design questions around the atomic-write and recursive-op semantics seriously, add dedicated per-protocol unit tests before any refactor, and migrate one protocol at a time (file → s3 → gcs → azure) in small reviewable PRs.

I'm happy to own that workstream. Given the scope, I'd want to sequence it against other commitments rather than execute it immediately inside this change — doing it now at the cost of higher-priority items isn't the right trade-off, and bundling it here would force the design decisions under the wrong time pressure. If you're good with the staging, I'll open the follow-up issue today.

Thanks for the detailed pushback, Kushal. I re-read the code and your three claims hold up where it matters most: the atomic-write contract in safe_write/safe_copy is real and load-bearing, and the per-protocol unit-test surface on StorageBackend is genuinely thin (only test_get_store_spec_file_protocol exercises it directly). The branch-count headline is overstated — actual is 22 on master (12 setup + 10 I/O), so the four-method ABC kills ~12, leaving ~10 in I/O, not 19. But that doesn't change the conclusion: full Alt B requires a materially larger interface and a real test scaffold first, and bundling that into this review cycle is the wrong trade-off.

So: I agree with your proposal to land additive + follow-up. I've filed #1440 to track the unified-adapter migration, with Phase 0 (per-protocol unit-test scaffolding) as a hard prerequisite, the atomicity and recursive-op semantics called out as gating design questions, and per-protocol phasing (file → s3 → gcs → azure). No target release is committed there until the design discussion concludes.

Three things I'd like addressed on this PR before merge:

1. Asymmetric error handling in storage.py. The three else: branches behave differently when no adapter is registered:

• _create_filesystem (368): raises DataJointError.
• _full_path (398): silently falls through to file-protocol logic.
• get_url (448): silently falls through to f"{self.protocol}://{full_path}".

In practice _create_filesystem runs first so the inconsistent fallbacks aren't reachable, but a future refactor that calls _full_path before filesystem creation would silently produce wrong paths. Make all three consistent — either factor the lookup into a helper that raises uniformly, or have _full_path/get_url raise too.

2. Default duplication in settings.py. The 7-line setdefault block at the new plugin branch (subfolding, partition_pattern, token_length, hash_prefix, schema_prefix, filepath_prefix, location) re-implements defaults already set by the existing built-in branches. Adding a 5th built-in or changing a default would silently rot the plugin path. Extract a small _apply_common_store_defaults(spec) helper and call it from both paths.

3. Entry-point discovery itself is untested. All 19 tests mutate _adapter_registry directly and bypass _discover_adapters. The discovery code path — the part most likely to break in third-party setups — has no test. One test using monkeypatch.setattr on entry_points with a fake EntryPoint whose .load() returns a stub adapter class would close this.

A nit: please drop the 🤖 trailer from the PR description before merge.

Once those land I'll approve. The two-tier pattern is real but it makes an already-implicit asymmetry explicit, and #1440 is the right place to resolve it properly.

Thanks for the additional pass, Dimitri. Pushed ae0dfb58 addressing all three items.

1. Symmetric error handling — added StorageBackend._require_adapter. _create_filesystem, _full_path, and get_url now all raise the same DataJointError("Unsupported storage protocol: ...") when no adapter is registered, regardless of call order. The previous silent-fallthrough paths in _full_path (file-protocol logic) and get_url (f"{protocol}://{path}") are gone — _full_path got an explicit elif self.protocol == "file": branch so the file path is no longer conflated with "anything unknown."

2. Default consolidation — extracted Config._apply_common_store_defaults and dropped the 7-line block from the plugin branch. Behavior change worth flagging: I also dropped spec.setdefault("location", "") for plugins. The previous code defaulted location to "" before validation, which meant a plugin author who put location in required_keys would never get a missing-key error — the spec would silently pass with empty location and produce wrong paths downstream. Now plugins get a clear DataJointError("plugin store is missing: location") if they require it. Plugins that don't require location are unaffected — the default StorageAdapter.full_path already handles a missing key via spec.get("location", "").

3. Discovery test — added TestEntryPointDiscovery driving _discover_adapters directly via monkeypatch.setattr on importlib.metadata.entry_points. Setup/teardown saves and restores _adapter_registry and _adapters_loaded so the lazy load actually fires and other tests don't inherit state. Two cases covered:

• test_discovery_loads_adapter_from_entry_point — happy path; verifies the adapter ends up in the registry and _adapters_loaded flips to True.
• test_discovery_skips_failing_entry_point — entry point whose .load() raises is logged-and-skipped while a sibling adapter still loads. Exercises the except Exception branch in _discover_adapters that the docstring promises but had no coverage.

I also extended the symmetric-error coverage — test_unsupported_protocol_full_path_raises and test_unsupported_protocol_get_url_raises lock in the _full_path/get_url raise behavior so a future regression there fails loudly.

Plus the nit: dropped the trailer from the PR description.

Test counts: 23 unit tests in test_storage_adapter.py (was 19), 272 unit tests passing repo-wide. CI green pending.

Ready for re-review.