From a65a2a40c93aef53d69d2daaeba28339851067a6 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Mon, 10 Aug 2026 09:30:59 +0100 Subject: [PATCH 1/2] docs: v1 redesign architecture + ADRs 0001-0009 Design-only. No code changes. Adds misc/docs/architecture.md (four-layer target design) and nine ADRs covering layering, engine choice, provider presets/capabilities, error taxonomy, large-object I/O, key scoping, naming/compat, testing, and scope. Claude-Session: https://claude.ai/code/session_01LioNGNe7Yts1FC3247veKp --- misc/docs/README.md | 42 ++++ misc/docs/architecture.md | 189 ++++++++++++++++++ .../decisions/0001-layered-architecture.md | 77 +++++++ misc/docs/decisions/0002-boto3-as-engine.md | 73 +++++++ .../0003-provider-presets-and-capabilities.md | 160 +++++++++++++++ misc/docs/decisions/0004-error-taxonomy.md | 131 ++++++++++++ misc/docs/decisions/0005-large-object-io.md | 164 +++++++++++++++ .../0006-key-scoping-and-dol-fixes.md | 146 ++++++++++++++ .../0007-naming-and-compatibility.md | 121 +++++++++++ .../decisions/0008-testing-architecture.md | 115 +++++++++++ .../decisions/0009-scope-and-deferrals.md | 141 +++++++++++++ 11 files changed, 1359 insertions(+) create mode 100644 misc/docs/README.md create mode 100644 misc/docs/architecture.md create mode 100644 misc/docs/decisions/0001-layered-architecture.md create mode 100644 misc/docs/decisions/0002-boto3-as-engine.md create mode 100644 misc/docs/decisions/0003-provider-presets-and-capabilities.md create mode 100644 misc/docs/decisions/0004-error-taxonomy.md create mode 100644 misc/docs/decisions/0005-large-object-io.md create mode 100644 misc/docs/decisions/0006-key-scoping-and-dol-fixes.md create mode 100644 misc/docs/decisions/0007-naming-and-compatibility.md create mode 100644 misc/docs/decisions/0008-testing-architecture.md create mode 100644 misc/docs/decisions/0009-scope-and-deferrals.md diff --git a/misc/docs/README.md b/misc/docs/README.md new file mode 100644 index 0000000..13070eb --- /dev/null +++ b/misc/docs/README.md @@ -0,0 +1,42 @@ +# s3dol design docs + +Hand-written design material. (Generated API docs go elsewhere — `docs/` is Sphinx/epythet +output; don't put prose here that a build step will overwrite.) + +| Document | Contents | +|---|---| +| [architecture.md](architecture.md) | **Start here.** The v1 four-layer design, module layout, and the contracts each layer enforces. | +| [decisions/](decisions/) | ADRs — one defaulted choice per document, with the evidence behind it. | + +## ADR index + +| # | Decision | Read it when | +|---|---|---| +| [0001](decisions/0001-layered-architecture.md) | Four layers, adopted from `azuredol` | adding any class or module | +| [0002](decisions/0002-boto3-as-engine.md) | boto3 stays the engine; alternatives are optional and later | tempted by obstore / minio / async | +| [0003](decisions/0003-provider-presets-and-capabilities.md) | Providers are config rows, not subclasses | adding a backend, or hitting "works on AWS, breaks on X" | +| [0004](decisions/0004-error-taxonomy.md) | One error seam; a taxonomy that never lies | touching exception handling | +| [0005](decisions/0005-large-object-io.md) | Value refs + injected transfer strategies | anything about big objects, streaming, or `s[k] = v` types | +| [0006](decisions/0006-key-scoping-and-dol-fixes.md) | **Prefix scoping — read before writing key code** | always, if you touch keys | +| [0007](decisions/0007-naming-and-compatibility.md) | Names, public API, deprecation path | renaming anything, or planning the release | +| [0008](decisions/0008-testing-architecture.md) | Four tiers, shipped fake, exported conformance | writing a test | +| [0009](decisions/0009-scope-and-deferrals.md) | v1 scope, deferrals, the `s3dol`/`botodol` line | proposing a feature | + +## The three things most likely to bite you + +1. **`dol`'s prefix machinery silently corrupts non-matching keys.** `mk_relative_path_store`, + `KeyCodecs.prefixed` and `prefixless_view` turn a sibling key `ab/x` into `/x` and a + non-matching key `z` into `''`. Only `Pipe(filt_iter.prefixes(p), KeyCodecs.prefixed(p))` + is safe. [ADR-0006 §1](decisions/0006-key-scoping-and-dol-fixes.md). +2. **`url_for` through a `dol` key-wrap returns a URL for the wrong object, silently**, and + `isinstance(store, SupportsUrlFor)` still says `True`. + [ADR-0006 §2](decisions/0006-key-scoping-and-dol-fixes.md). +3. **botocore ≥1.36 sends checksums by default**, and several S3-compatible providers either + reject them loudly or persist the `aws-chunked` framing *into the object body*. The fix is + client config plus `s3transfer>=0.11.2`. + [ADR-0003 §3](decisions/0003-provider-presets-and-capabilities.md). + +## Convention + +When you change a default, update the ADR that documents it **in the same PR**. When a +decision is reversed, add a new ADR that supersedes the old one rather than editing history. diff --git a/misc/docs/architecture.md b/misc/docs/architecture.md new file mode 100644 index 0000000..003fbd7 --- /dev/null +++ b/misc/docs/architecture.md @@ -0,0 +1,189 @@ +# s3dol — Architecture (v1 target) + +`s3dol` exposes **S3 and S3-compatible object storage** as `dol`-style `Mapping` / +`MutableMapping` interfaces. This document is the single source of truth for the package's +layering. The *why* behind each defaulted choice lives in [decisions/](decisions/). + +> **Status: design.** This describes the v1 target, not the shipped code. v0.1.x has a +> different (and in places broken) shape — see +> [decisions/0009-scope-and-deferrals.md](decisions/0009-scope-and-deferrals.md) for what +> lands when, and [decisions/0007-naming-and-compatibility.md](decisions/0007-naming-and-compatibility.md) +> for the migration. + +--- + +## Goals + +1. **Pythonic.** `s[k]`, `s[k] = v`, `del s[k]`, `k in s`, `for k in s:` is the surface. + Everything else is opt-in. +2. **`dol` is the base.** Key transforms, prefix scoping, codecs, caching and filtering come + from `dol`. s3dol writes S3 knowledge only. +3. **Portable across S3-compatible backends** — AWS, MinIO, R2, Scaleway, Hetzner, Backblaze, + Wasabi, Ceph, Supabase, DigitalOcean, Tigris — with AWS as the reference semantics. +4. **Never silently wrong.** A wrong answer is worse than an error in every use case we + serve. No operation returns empty-on-failure, and no explicit argument is silently + ignored. +5. **Big objects are ordinary.** Reading and writing must never require the whole value in + memory, and that must be reachable *through the Mapping interface*. +6. **Testable without a cloud**, by us and by our users. + +## Non-goals for v1 + +Async, an fsspec filesystem adapter, an obstore engine, and Mapping interfaces over +non-blob S3 resources (versions, tags, bucket config, in-flight uploads). All are tracked; +see [decisions/0009](decisions/0009-scope-and-deferrals.md). + +--- + +## The four layers + +``` +┌────────────────────────────────────────────────────────────────────┐ +│ Layer D — recipes (s3dol.recipes) │ +│ s3_store(...) / s3(...) factories, codec stacks (S3Jsons, ...) │ +│ Built ONLY by wrap_kvs / Pipe composition. Never by subclassing. │ +├────────────────────────────────────────────────────────────────────┤ +│ Layer C — relative-key stores (s3dol.stores) │ +│ Prefix scoping via dol. Sub-stores. This is what users hold. │ +├────────────────────────────────────────────────────────────────────┤ +│ Layer B — close-to-metal (s3dol.base) │ +│ BucketCollection/Reader/Store, Buckets, ObjectHandle. │ +│ ABSOLUTE keys, bytes in / bytes out, one error seam, no codecs. │ +├────────────────────────────────────────────────────────────────────┤ +│ Layer A — connection (s3dol.connection) │ +│ S3Connection: the credential + endpoint SSOT. Lazy, picklable, │ +│ redacting. The dependency-injection seam. │ +├────────────────────────────────────────────────────────────────────┤ +│ boto3 / botocore │ +└────────────────────────────────────────────────────────────────────┘ +``` + +Every public class belongs to exactly one layer. No mixing. Two rules make the layering +load-bearing rather than decorative: + +- **Layer B keys are absolute.** All prefix arithmetic happens in Layer C, in `dol`. A + Layer B store addresses the bucket's real keyspace, which is what makes `url_for`, + `info`, and the transfer strategies correct by construction — they operate on the key S3 + actually sees. +- **Layer D never subclasses.** If a recipe cannot be expressed as a composition of Layer C + plus `dol` wrappers, that is a signal the capability belongs in Layer B as a parameter, + not in Layer D as a subclass. This is the rule that keeps per-vendor classes + (`SupabaseS3BucketDol`) from reappearing. + +### Layer A — `s3dol.connection` + +Owns the expensive resource (the botocore client) and *all* credential/endpoint +resolution. See [decisions/0002](decisions/0002-boto3-as-engine.md) and +[decisions/0003](decisions/0003-provider-presets-and-capabilities.md). + +```python +@dataclass(frozen=True) +class S3Connection: + preset: str | Preset | None = None # 'aws' | 'minio' | 'r2' | ... + profile_name: str | None = None + endpoint_url: str | None = None + region_name: str | None = None + credentials: Credentials | None = None # explicit; None => resolve the chain + anon: bool | Literal['auto'] = False + client_config: dict = field(default_factory=dict) +``` + +Three properties matter and are each tested: + +- **Lazy.** The client is a `cached_property`; constructing a connection performs no I/O and + never raises for missing credentials. +- **Picklable.** The connection carries a *spec*, not a client. `__getstate__` drops the + cached client so stores survive `ProcessPoolExecutor` and Dask. +- **Redacting.** No secret appears in `repr`, `str`, or any surviving local. + +### Layer B — `s3dol.base` + +Follows `dol.filesys`' triangle, in S3 vocabulary: + +``` +BucketCollection (Collection — __iter__ over object keys) + └── BucketReader (+ __getitem__ -> bytes, url_for, info, handle) + └── BucketStore (+ __setitem__ / __delitem__) + +BucketsCollection (Collection — __iter__ over bucket names) + └── BucketsReader (+ __getitem__ -> BucketReader) + └── Buckets (+ __setitem__ / __delitem__ for buckets) +``` + +plus `ObjectHandle` — the escape hatch for one object, which is **not** a Mapping and is +where ranged reads, streaming, multipart and object metadata live. + +| Operation | Contract | +|---|---| +| `__getitem__(k)` | Returns `bytes`. `KeyError` iff absent. Auth/config errors re-raised untouched. | +| `__setitem__(k, v)` | `v` in a closed, documented union (see [0005](decisions/0005-large-object-io.md)). Replaces. | +| `__delitem__(k)` | `KeyError` iff absent. | +| `__contains__(k)` | One `HeadObject`. `False` iff absent; **raises** on auth/config failure. | +| `__iter__()` | Lazy paginated `ListObjectsV2`. **Raises** if the bucket is missing or unlistable — never yields empty. | +| `__len__()` | **Not implemented.** Raises `TypeError` with guidance. See [0008](decisions/0008-testing-architecture.md) §cost model. | +| `__repr__` | bucket, prefix, endpoint host, mode. No secrets, no addresses. | +| `url_for(k, ...)` | Presigned URL. Zero object requests. Prefix-aware. | + +### Layer C — `s3dol.stores` + +Prefix scoping, delegated to `dol` — but **only in its safe composition**. This is a +correctness requirement, not an optimization; see +[decisions/0006](decisions/0006-key-scoping-and-dol-fixes.md), which is the most important +document here. + +### Layer D — `s3dol.recipes` + +```python +s3_store(bucket, *, prefix='', preset=None, connection=None, codec=None, ...) -> BucketStore +S3Jsons = wrap_kvs(BucketStore, value_codec=ValueCodecs.json()) +S3Texts = wrap_kvs(BucketStore, value_codec=ValueCodecs.str_to_bytes()) +``` + +--- + +## Module layout + +``` +s3dol/ + __init__.py lazy PEP-562 __getattr__, explicit __all__, TYPE_CHECKING imports + connection.py S3Connection, credential/endpoint resolution + precedence + presets.py Preset + Capabilities frozen dataclasses; the provider registry + errors.py translate_s3_errors seam; the exception taxonomy + values.py Filepath / Chunks / Streamable refs; as_fileobj singledispatch + writes.py write strategies (simple / transfer / multipart) + reads.py read strategies (bytes / stream / ranged / to-file) + base.py Layer B + stores.py Layer C + Layer D codec facades + recipes.py s3_store(...) and friends + store.py COMPAT SHIM — legacy S3Store, DeprecationWarning, removed in v2 + testing.py in-memory fake + the exported conformance suite + tests/ + util.py KEEP — py2store imports two functions from here +``` + +`store.py` must survive as an importable module: both external dependents do +`from s3dol.store import S3Store`, not `from s3dol import S3Store`. + +--- + +## What we explicitly do NOT do + +- **Per-vendor subclasses.** A provider is a row in `presets.py`, never a class. The v0 + `SupabaseS3BucketDol` is the anti-pattern; its behaviour becomes client configuration + ([0003](decisions/0003-provider-presets-and-capabilities.md) §Supabase). +- **`type(self)(**self.__dict__)` for sub-stores.** Fragile the moment any attribute isn't + an `__init__` arg. Sub-stores come from `dol`. +- **Probe-then-act.** No `head_bucket` before a write. Bucket policy is decided once, at + construction. +- **Cascading deletes as a side effect.** `del buckets[name]` refuses a non-empty bucket; + `buckets.delete(name, force=True)` is the explicit form. +- **`__len__` on a bucket store.** Unbounded pagination cost. +- **Silent empties.** Anywhere. + +## Prior art + +`azuredol` went through this refactor first; its +[design_decisions.md](https://github.com/i2mint/azuredol) is the direct ancestor of this +layout, and several of its sections cite s3dol v0 as the pattern being rejected. Where the +two packages face the same question, **we deliberately give the same answer** — the +`*dol` family's value is that one adapter reads like the next. diff --git a/misc/docs/decisions/0001-layered-architecture.md b/misc/docs/decisions/0001-layered-architecture.md new file mode 100644 index 0000000..577533d --- /dev/null +++ b/misc/docs/decisions/0001-layered-architecture.md @@ -0,0 +1,77 @@ +# ADR-0001: Four-layer architecture, adopted from `azuredol` + +- **Status:** Accepted +- **Date:** 2026-08-10 +- **Supersedes:** the v0.1.x `base.py` / `store.py` / `utility.py` split + +## Context + +v0.1.x has three modules and a class hierarchy that mixes concerns at every level: +`BaseS3BucketReader` holds a client *and* does prefix arithmetic *and* parses responses; +`S3Store` is a factory function that decides credentials, provider quirks, bucket policy and +prefix handling in one 80-line body; `SupabaseS3BucketDol` subclasses the store to work +around a *client configuration* problem by hand-parsing HTTP framing out of object bodies. + +The code knows. There are five `TODO` comments saying so, including +`# TODO: Messy. Should use wrap_kvs.` and two `# TODO: Smelly. use trans tools`. + +We are not the first `*dol` blob adapter to face this. **`azuredol` completed exactly this +refactor**, and its `misc/docs/design_decisions.md` cites s3dol by name as the pattern it +rejected — §5 on `type(self)(**self.__dict__)` sub-stores, §12 on the cascading container +delete (*"This is convenient and dangerous. We refuse it."*). + +## Decision + +Adopt `azuredol`'s layering, in S3 vocabulary, with one addition. + +``` +D recipes factories + codec stacks, by composition only +C stores relative keys / prefix scoping, delegated to dol +B base close-to-metal, ABSOLUTE keys, bytes<->bytes, one error seam +A connection the credential + endpoint SSOT; the DI seam +``` + +The addition is the **A/B split being strict about key space**: Layer B addresses the +bucket's real keyspace and knows nothing about prefixes. This is not tidiness — it is what +makes `url_for`, `info` and the transfer strategies correct by construction, because they +operate on the key S3 actually sees rather than a user-facing alias. v0's `url_for` had to +re-apply `_id_of_key` by hand (`base.py:222-225`) precisely because that split didn't exist, +and any future method would have had to remember to do the same. + +**Layer D never subclasses.** If a recipe can't be built from Layer C + `dol` wrappers, that +is evidence the capability belongs in Layer B as a *parameter*. This is the rule that stops +per-vendor classes from reappearing. + +Class triads mirror `dol.filesys`: + +``` +BucketCollection -> BucketReader -> BucketStore (keys: object keys) +BucketsCollection -> BucketsReader -> Buckets (keys: bucket names) +ObjectHandle (not a Mapping) +``` + +Reader-only classes are **real classes**, not instances with methods deleted. `dol`'s +`mk_read_only` works by assigning `__delitem__`/`__setitem__` onto the object, which +`type(store).__setitem__(store, k, v)` bypasses and which static analysis cannot see. Real +classes let a type checker catch `reader[k] = v`, and let an anonymous or read-scoped +credential refuse to even attempt a write. + +## Consequences + +**Buys.** One place to look for credential behaviour. Sub-stores and codecs for free from +`dol`. A capability added at Layer B is automatically available through every Layer C/D +composition. A reader of `azuredol` can read `s3dol` — the family's main value. + +**Costs.** More modules (7 vs 3) for a package this size, and one genuinely awkward +consequence: because Layer B is absolute-keyed and Layer C is a `dol` wrapper, a method +added to Layer B is **not automatically key-correct** when reached through Layer C. That is +the `url_for` delegation bug in [ADR-0006](0006-key-scoping-and-dol-fixes.md), and it is the +price of delegating prefixing to `dol` rather than owning it. We pay it because owning it is +what produced v0's bugs, and because the fix is upstreamable. + +**What NOT to do.** + +1. Do not add a method to Layer B without deciding how it behaves through a Layer C wrap. + Every such method needs a key-mapping test. +2. Do not put provider knowledge anywhere but `presets.py`. +3. Do not let Layer D grow a class statement. diff --git a/misc/docs/decisions/0002-boto3-as-engine.md b/misc/docs/decisions/0002-boto3-as-engine.md new file mode 100644 index 0000000..49349d1 --- /dev/null +++ b/misc/docs/decisions/0002-boto3-as-engine.md @@ -0,0 +1,73 @@ +# ADR-0002: boto3 stays the engine; alternatives are optional, narrow, and later + +- **Status:** Accepted +- **Date:** 2026-08-10 + +## Context + +The brief asked whether s3dol should keep `boto3` or move to something lighter — `minio`, +`s3fs`, `smart_open`, `aioboto3`, or `obstore` (Rust `object_store` bindings). The measured +case against boto3 is real: `import s3dol` costs **172 ms**, of which **boto3 is 134 ms**. +One research pass recommended making `obstore` the default engine. + +## Decision + +**boto3/botocore remains the default and only required engine for v1.** Alternatives are +deferred behind a narrow protocol. + +### Why + +1. **The alternatives cannot back the whole surface.** `obstore` has no bucket + create/delete/list operations at all, so the bucket level (`Buckets`, keys = bucket + names) cannot be implemented on it. A "default engine" that can't serve a documented + layer isn't a default. +2. **Everything portability-related is botocore-shaped.** The checksum fix that makes + half the S3-compatible providers work is `botocore.Config(request_checksum_calculation=...)`. + The error taxonomy keys on `ClientError.response['Error']['Code']`. The transfer + strategies are `TransferConfig`/`upload_fileobj`. Choosing another engine discards the + research that makes the package correct. +3. **The blocker cited for boto3 was not load-bearing.** The claim was that s3dol's + `url_for(k, **params)` can't be honoured by obstore. The only external consumer calls it + with one positional argument and no params. The genuine blockers are (1) above. +4. **A `botodol` is planned.** The owner intends a unified collections interface over AWS + generally. Standardising s3dol on botocore keeps that path open; standardising on a + Rust binding closes it. + +### The import cost is addressed directly, not by switching engines + +boto3 is imported **lazily**: `from __future__ import annotations`, `if TYPE_CHECKING:` for +types, and the client as a `functools.cached_property` on `S3Connection`. Constructing a +store performs no import of boto3 and no I/O. Budget: **`import s3dol` < 30 ms**, enforced +by a test. + +This is strictly better than switching engines, because it also gives us the +lazy/picklable connection that [ADR-0003](0003-provider-presets-and-capabilities.md) +and the multiprocessing use case need anyway. + +### The seam for later + +Layer B talks to the backend through a small set of operations (get, get-range, put, +put-stream, delete, delete-many, list, head, presign). v1 defines that boundary as a +module-internal protocol, implemented once by botocore. It is deliberately **not** a public +extension point in v1 — publishing a protocol with one implementer is how you get an +abstraction shaped like its only implementation. + +`s3dol[fast]` (obstore for the object level only) and `s3dol.aio` are tracked in +[ADR-0009](0009-scope-and-deferrals.md). + +## Consequences + +**Buys.** All the provider-compatibility work stays applicable. No 0.x single-vendor +dependency in the required set. A fast import anyway. + +**Costs.** We carry boto3's weight for users who only ever touch one object, and we inherit +botocore's config surface — including the checksum default that broke half the ecosystem +(see [ADR-0003](0003-provider-presets-and-capabilities.md)). + +**Revisit when:** obstore grows bucket operations *and* an independent re-measurement in a +real environment confirms the import/throughput claims. The numbers behind the original +recommendation came from a throwaway venv and were not reproduced. + +**What NOT to do.** Do not add a second engine "just for benchmarking". Two engines means +two error tables, two checksum stories, and two sets of provider quirks — the cost is not +in the adapter, it's in the compatibility matrix. diff --git a/misc/docs/decisions/0003-provider-presets-and-capabilities.md b/misc/docs/decisions/0003-provider-presets-and-capabilities.md new file mode 100644 index 0000000..b2da1f3 --- /dev/null +++ b/misc/docs/decisions/0003-provider-presets-and-capabilities.md @@ -0,0 +1,160 @@ +# ADR-0003: Providers are config rows, not subclasses — presets + declared capabilities + +- **Status:** Accepted +- **Date:** 2026-08-10 + +## Context + +s3dol's reason to exist is *S3 and things that speak S3*. v0.1.x handles that with per-vendor +subclasses (`SupabaseS3BucketDol`, `S3BucketDolWithouBucketCheck` [sic]) and endpoint +string-sniffing scattered through the code (`".supabase." in endpoint` appears in three +places, in the hot path of every user, including those who have never heard of Supabase). + +A survey of 16 backends — AWS, MinIO, R2, Scaleway, Hetzner, Backblaze B2, Wasabi, +DigitalOcean Spaces, Ceph RadosGW, GCS XML interop, Supabase, Tigris, Oracle OCI, LocalStack, +moto, and Azure Blob — produced a clear result: + +> **Every divergence is expressible as (a) botocore `Config` + client kwargs, (b) one of a +> small set of named strategies, or (c) a declared capability. Nothing needs a subclass.** + +## Decision + +### 1. A preset registry, and a `Preset` is data + +```python +@dataclass(frozen=True) +class Preset: + name: str + endpoint_url: str | None = None # may contain {placeholders} + region_name: str | None = None + addressing_style: str = 'auto' # 'auto' | 'path' | 'virtual' + signature_version: str = 's3v4' + checksum: str = 'when_supported' # 'when_supported' | 'when_required' + payload_signing_enabled: bool | None = None + capabilities: Capabilities = DEFAULT_CAPABILITIES + client_kwargs: Mapping = MappingProxyType({}) +``` + +Adding a provider is adding a row. Open-closed. Users register their own: +`s3dol.presets.register(Preset(name='mycorp', ...))`. + +The registry is the SSOT for a set of facts nobody should have to rediscover: + +| Provider | endpoint | region | addressing | notes | +|---|---|---|---|---| +| aws | SDK-resolved | real region | auto | reference semantics | +| minio | `http://{host}:{port}` | `us-east-1` conventional | **path** unless wildcard DNS | | +| r2 | `https://{account_id}.r2.cloudflarestorage.com` | **`auto`** | virtual | presign only on the S3 API domain, never a custom domain | +| scaleway | `https://s3.{region}.scw.cloud` | same string | virtual | multipart capped at **1000 parts** | +| hetzner | `https://{loc}.your-objectstorage.com` | **must repeat `{loc}`** | virtual | needs `payload_signing_enabled=False` | +| backblaze | `https://s3.{region}.backblazeb2.com` | same string | virtual | checksum `when_required` **mandatory** | +| wasabi | `https://s3.{region}.wasabisys.com` | region string | path (vendor's own advice) | `GetBucketLocation` always says `us-east` | +| gcs | `https://storage.googleapis.com` | ignored | virtual (path for dotted buckets) | **no ListObjectsV2**, no batch delete | +| supabase | `https://{ref}.storage.supabase.co/storage/v1/s3` | project region | **path, forced** | see §3 | +| localstack / moto | `http://localhost:{port}` | `us-east-1` | path | test doubles | + +**Azure Blob is not in the registry.** It is not S3-compatible; reaching it requires a +translating proxy. It is a different backend (`azuredol`), not a preset. Recording this +explicitly because "S3-compatible" is often assumed to include it. + +### 2. Capabilities are *declared*, and unsupported means a loud, specific error + +```python +@dataclass(frozen=True) +class Capabilities: + list_objects_v2: bool = True + batch_delete: bool = True + presigned_post: bool = True + multipart: bool = True + max_multipart_parts: int = 10_000 + min_part_size: int = 5 * 2**20 + object_tagging: bool = True + versioning: bool = True + conditional_writes: bool = True + consistency: Literal['strong', 'read-after-write', 'eventual'] = 'strong' +``` + +Four options were considered for handling a missing capability — silent emulation, +fail-fast at construction, `NotSupportedError` at call time, and capability introspection. +We take a **combination**, chosen per capability: + +- **Emulate when the emulation is exact and cheap.** `batch_delete=False` (GCS) loops + `DeleteObject`. The observable result is identical; only cost differs. Emulate, and say so + in the docs. +- **Substitute when a documented equivalent exists.** `list_objects_v2=False` (GCS) uses the + V1 paginator. Same keys, same order. +- **Raise `NotSupportedError` when there is no honest fallback.** `presigned_post=False` + (Backblaze) cannot be emulated — raise, naming the provider and the operation. +- **Never silently degrade correctness.** Emulation is allowed only where the result is + indistinguishable. + +Capabilities are a *static table*, not a probe. A probe costs a round-trip per store, is +wrong under partial permissions, and cannot be trusted anyway (providers change). Users can +override per-connection when their deployment differs from the table — which matters most +for Ceph, whose behaviour varies more by release than by vendor. + +### 3. The checksum change is the single most important entry in this ADR + +Since **botocore ≥ 1.36**, integrity checksums are calculated by default +(`request_checksum_calculation="when_supported"`). Over HTTPS, `PutObject` takes the +*trailer* branch: the wire body becomes `aws-chunked` framed, `Content-Length` is deleted, +and `X-Amz-Decoded-Content-Length` carries the real size. + +Providers that don't implement it react in two ways: + +- **Loud** — Backblaze: `InvalidArgument: Unsupported header 'x-amz-sdk-checksum-algorithm'`. +- **Silent and destructive** — Supabase ignores `Content-Encoding: aws-chunked` and + **persists the framing verbatim**. Objects come back as + `\r\n\r\n0\r\nx-amz-checksum-crc32:\r\n\r\n`. + +That silent case is what v0's `SupabaseS3BucketDol.__getitem__` de-chunker exists to undo. It +is a **read-side workaround for a write-side misconfiguration**, and it is itself buggy — +it truncates at the first `\r\n` after the header, so it mangles any payload larger than one +chunk, and it misfires on any payload whose first bytes look like hex digits. + +**The fix is configuration:** + +```python +Config(request_checksum_calculation='when_required', + response_checksum_validation='when_required', + s3={'addressing_style': 'path'}) +``` + +with **`s3transfer >= 0.11.2`** pinned — 0.11.0 unconditionally re-enabled the default +checksum from inside `TransferManager`, defeating the setting for `upload_file`/`upload_fileobj` +(boto/s3transfer#327). + +Two caveats we must not forget: + +1. `when_required` does **not** silence everything. `DeleteObjects` *always* ships + `x-amz-checksum-crc32`. Providers that reject it need `batch_delete=False` — which is why + that capability exists. +2. **Fixing this does not un-corrupt already-written objects.** See Consequences. + +### 4. Per-vendor classes are deleted + +`SupabaseS3BucketDol` → `preset='supabase'`. `S3BucketDolWithouBucketCheck` → the default +(no probe-then-act, see [ADR-0001](0001-layered-architecture.md)). Provider detection from +an endpoint URL happens **once, at connection construction**, never in `__getitem__`. + +## Consequences + +**Buys.** A new provider is a row and a test. No vendor logic in the read/write path. The +matrix is documented where a user can find it. + +**Costs.** The table will drift; it needs a dated review and an escape hatch (it has one: +users override any field per-connection). And most rows are **doc-sourced, not verified +against a live endpoint** — they are marked as such in `presets.py`. Wrong values mostly fail +loudly, which is the acceptable failure mode, but the Supabase entry in particular is +inferred from behaviour rather than confirmed. + +**What NOT to do.** + +1. **Do not delete the Supabase de-chunker in the same change that adds the checksum fix.** + Sequence it: (1) ship `preset='supabase'`; (2) validate against a live project; (3) ship + a `detect_chunked_framing(store)` diagnostic and a documented repair path for objects + already written wrong; (4) *only then* remove the codec — and keep it available as an + explicitly-chosen value codec, never a default. Deleting it early converts silent + corruption into silently-unreadable data. +2. Do not probe capabilities at construction. +3. Do not let a preset carry credentials. Presets are public, shareable, committable config. diff --git a/misc/docs/decisions/0004-error-taxonomy.md b/misc/docs/decisions/0004-error-taxonomy.md new file mode 100644 index 0000000..46a0ea8 --- /dev/null +++ b/misc/docs/decisions/0004-error-taxonomy.md @@ -0,0 +1,131 @@ +# ADR-0004: One error-translation seam, and a taxonomy that never lies + +- **Status:** Accepted +- **Date:** 2026-08-10 +- **Addresses:** [discussion #6](https://github.com/i2mint/s3dol/discussions/6) + +## Context + +v0.1.x defines seven exception classes in `utility.py`. Five are never raised. One +(`KeyNotValidError`) is *caught* at `base.py:132` and raised nowhere, so that `except` clause +is dead code. Meanwhile the real error handling is string-sniffing spread across the +codebase: `e.response["Error"]["Code"] == "404"`, `"404" in str(e)`, and a bare +`except ClientError: return False` in `_bucket_exists` that reports *every* failure — +expired token, missing permission, wrong endpoint — as "the bucket doesn't exist". + +Discussion #6 proposes an `S3KeyError(KeyError)` carrying a class-level registry of backend +exception types plus a `register` classmethod. + +### The measured failure landscape + +Verified against botocore 1.40.53 and moto 5.2.2: + +| Operation | missing target | `Error.Code` | HTTP | caught by `client.exceptions.NoSuchKey`? | +|---|---|---|---|---| +| `GetObject` | key | `NoSuchKey` | 404 | **yes** | +| `HeadObject` | key | `404` | 404 | **no** | +| `HeadBucket` | bucket | `404` | 404 | **no** | +| `DeleteObject` | key | — | 204 | *no error at all* | +| `DeleteObjects` | keys | — | 200 | reported as `Deleted`, not an error | + +`HEAD` has no response body, so there is no XML error document for botocore to model — which +is why the modelled exception never fires and code that catches `NoSuchKey` around a +`head_object` silently never matches. This is the single most common S3 error-handling bug +and v0 has it. + +Provider divergence compounds it: **Supabase returns 400 where AWS returns 404** on +`HeadBucket`, and several providers return 403 to hide 404. + +## Decision + +### 1. One seam + +A single `@translate_s3_errors(...)` decorator in `errors.py` is the **only** place that +catches botocore exceptions in Layer B. Everything else propagates. This is `azuredol` §4, +and it is what makes the auth-vs-not-found distinction auditable from one file. + +**Auth, config and transport errors are never translated to `KeyError`.** A missing +permission must not look like a missing key — that is exactly the confusion that makes +`_bucket_exists` dangerous today. + +### 2. Classification is a table keyed on `(code, status)`, not on exception type + +Because the modelled exceptions are unreliable (above), classification reads +`ClientError.response['Error']['Code']` and the HTTP status, through two tiny predicates +(`code_of`, `status_of`). The table is per-provider-overridable, which is how Supabase's +400-means-404 is handled without any code branch. + +Testable without a network: the classifier takes a synthesized `ClientError`, so the bulk of +error tests are pure unit tests. + +### 3. The taxonomy + +| Condition | Raises | Rationale | +|---|---|---| +| object absent | `ObjectNotFound(KeyError)` | Mapping contract | +| bucket absent (object op) | `BucketNotFound(KeyError)` | still a key-space problem for the caller | +| bucket absent (bucket op) | `BucketNotFound(KeyError)` | key of the `Buckets` mapping | +| key syntactically invalid | `KeyNotValid(KeyError, ValueError)` | both, deliberately — see §5 | +| object archived (Glacier) | `ObjectArchived(KeyError)` | see §4 | +| permission denied | `AccessDenied(S3Error)` — **not** a `KeyError` | | +| credentials missing/expired | `CredentialsError(S3Error)` | | +| operation unsupported by provider | `NotSupported(S3Error)` | names provider + operation | +| transient / throttled | propagate (botocore retries) | | + +Everything derives from `S3Error(Exception)` so `except S3Error` catches the package. + +### 4. `ObjectArchived` is a `KeyError`, deliberately and arguably + +A Glacier object exists but `GetObject` returns `InvalidObjectState` / 403. `k in store` must +stay `True` — the key *does* exist, and any other answer breaks generic algorithms. But +`store[k]` must fail, and it must fail as a `KeyError` so that `store.get(k, default)` and +`dict(store)`-shaped code degrade to the not-available branch rather than exploding. + +So it is a `KeyError` that carries `.storage_class`, `.restore_status` and `.restore(days, +tier)`. This is the one place we knowingly let a `KeyError` mean something other than +"absent", and it is recorded here as a considered choice rather than an accident. + +### 5. Two collisions to avoid + +- **`dol` already has two different `KeyValidationError`s.** `dol.errors.KeyValidationError` + derives from `(NotValid, ValueError, TypeError)`; `dol.filesys.KeyValidationError` derives + from `(KeyError, LookupError)`. They are *not the same class*. s3dol therefore names its + own `KeyNotValid` rather than adding a third thing called `KeyValidationError`, and + inherits from both `KeyError` and `ValueError` so either `except` works. +- **`KeyError.__str__` uses `repr` of its args.** A multi-line, helpful message becomes a + single line full of `\n` escapes. Our `KeyError` subclasses therefore override `__str__`. + Without this, every "informative error" in the package is silently mangled — verified. + +### 6. On discussion #6's registry + +The proposed class-attribute registry with `register()` works, but it is **global mutable +state**: any import can change how every store in the process classifies errors, and the +effect is order-dependent and untestable in isolation. We take the same *intent* — +third-party extensibility — with a different mechanism: the translation table is a value on +the connection's preset, so extension is per-connection, explicit, and inspectable. + +**This belongs in `dol`, and that is a separate change.** Every `*dol` adapter has the same +"which backend exception means no-such-key" problem, and each has solved it differently (or +not at all). The proposed `dol` primitive is a `map_errors` decorator plus a `NotFound` +convention. It is *not* a blocker for s3dol: we implement it locally in a drop-in shape with +a linked issue, per [ADR-0006](0006-key-scoping-and-dol-fixes.md)'s policy on upstreams. + +### 7. Message quality, and the one thing they must not contain + +An error names the operation, the bucket, the key, the resolved endpoint, and the underlying +code. It **never** contains a credential, a token, or a signed URL — `url_for` returns URLs +carrying `X-Amz-Signature`, so redaction is a tested function, not a convention. + +## Consequences + +**Buys.** `except KeyError` keeps working for Mapping consumers — non-negotiable and +preserved. Auth failures stop masquerading as missing data. Provider quirks are table rows. +Most error tests need no network. + +**Costs.** The table needs maintenance as providers change, and `ObjectArchived` being a +`KeyError` will surprise someone eventually — which is why it is documented here and in the +class docstring. + +**What NOT to do.** Do not catch `ClientError` broadly anywhere outside `errors.py`. Do not +return `False`/`[]`/`None` on an exception path — the v0 `_bucket_exists` pattern is +banned by [ADR-0001](0001-layered-architecture.md) goal 4. diff --git a/misc/docs/decisions/0005-large-object-io.md b/misc/docs/decisions/0005-large-object-io.md new file mode 100644 index 0000000..bf7b70e --- /dev/null +++ b/misc/docs/decisions/0005-large-object-io.md @@ -0,0 +1,164 @@ +# ADR-0005: Large objects through a pure Mapping — value refs + injected transfer strategies + +- **Status:** Accepted +- **Date:** 2026-08-10 +- **Addresses:** [issue #5](https://github.com/i2mint/s3dol/issues/5) + +## Context + +Multipart upload is used *precisely when the data is too big to hold in memory*. That makes +`s[k] = v` awkward: if `v` is `bytes`, we have already lost. So what is `v`? A filepath? A +file object? An iterator of bytes? And is it acceptable that `s[k] = v` accepts something +different from what `s[k]` returns? + +A tempting escape is to split the store: a write-only multipart store that is Iterable + +Settable + Deletable but not Gettable, paired with a separate reader. + +The hard constraint from [ADR-0001](0001-layered-architecture.md): **base interfaces stay +pure.** No `s.upload_multipart(...)`. Infra capability must be reachable *through* `s[k] = v`. + +## Decision + +### 1. The write domain is a small, closed, explicitly-named union; the read codomain is `bytes` + +```python +BytesSource = bytes | bytearray | BinaryIO | Filepath | Chunks | Streamable +``` + +`Filepath`, `Chunks` and `Streamable` are tiny frozen dataclasses in `s3dol/values.py` — value +*refs*, not values. Dispatch is `functools.singledispatch`, so the union is open for +extension (users register their own ref types) and closed for modification. + +### 2. `str` is rejected, loudly + +`s['config'] = '{"a": 1}'` and `s['video'] = '/tmp/big.mp4'` are both overwhelmingly +plausible. There is no rule distinguishing them that isn't a latent data-corruption bug — +`os.path.exists` is a heuristic whose behaviour depends on the filesystem. + +Dispatch is not the crime; dispatching on `str` is. The test is **decidability**: a `Path` is +never content, an open file handle is never content, a `Chunks(...)` is never content. A +`str` is ambiguous, so it must not be guessed: + +``` +TypeError: A str value is ambiguous here: did you mean its utf-8 bytes, or a filepath? + content : s[k] = v.encode() (or wrap: dol.wrap_kvs(store, data_of_obj=str.encode, ...)) + filepath: s[k] = s3dol.Filepath(v) +``` + +Bare `os.PathLike` is rejected too — `Path` *is* unambiguous, but accepting it while +rejecting `str` is a confusing half-rule. One word, `Filepath(p)`, zero ambiguity. + +### 3. Asymmetric read/write types are correct, subject to a law + +The question "is it a real problem or just design ickiness?" has an answer: **it is normal, +and it is safe exactly when a canonical form exists.** + +Precedent, verified in the installed environment rather than recalled: + +| Library | write accepts | read returns | `MutableMapping`? | +|---|---|---|---| +| **h5py** | `list`, `ndarray`, scalars, `bytes`, **`SoftLink`/`ExternalLink` reference objects** | `Dataset`/`Group` (not an `ndarray`) | **yes** | +| **configparser** | a `Mapping` of anything | `SectionProxy`; values come back `str` | yes | +| **fsspec `FSMap`** | `bytes`, `bytearray`, `array`, anything with `__array__`; **`str` rejected** | `bytes` | yes | +| **`dol.Files`** | any buffer-protocol object | `bytes` | yes | +| shelve, numpy, zarr, pandas | broader than read | narrower | mostly | + +h5py is the decisive one: `SoftLink`/`ExternalLink` are purpose-built objects meaning *"the +value is a reference to content elsewhere"*, assigned through an ordinary +`MutableMapping.__setitem__`, in the most widely used scientific-data library in Python. +`Filepath`/`Chunks` are that design. + +The law that makes it safe — three conditions, all necessary: + +> **N1 Canonical form.** There is a total `normalize: WriteDomain → bytes`, identity on `bytes`. +> **N2 Stability.** Therefore `s[k] = s[k]` is a no-op and `dst.update(src)` terminates. +> **N3 The honest invariant.** Not `s[k] = v ⟹ s[k] == v`, but **`s[k] = v ⟹ s[k] == normalize(v)`**. + +Rejecting `str` is exactly what keeps `normalize` a *function* — with `str` admitted it would +have two candidate results, and N1 would fail. The rejection isn't fussiness; it's what makes +the rest sound. + +The residual cost is real and small: `setdefault` becomes type-unstable, `pop`/`popitem` +become expensive. Documented, not removed. + +### 4. Do NOT split into a write-only store + +Three reasons, in order of force: + +1. **The split solves a problem that doesn't exist.** After `complete_multipart_upload` the + object is an ordinary S3 object; `GetObject` reads it with no knowledge that it arrived in + parts. Multipart is a *transport* concern, not a storage-model one. There is no state in + which the key is writable but not readable. +2. **It conflates two different things.** "This blob is huge, upload it efficiently" is one + `__setitem__` and is fully solved by a strategy. "This is a live stream being appended to + over minutes" is a *session* with a lifetime, an `UploadId` and a completion event — not a + Mapping operation at all, and it should not be modelled as one. +3. **It costs the whole Mapping toolchain**: `dict(s)`, `s.items()`, `filt_iter`, `cache_vals`, + `kv_walk`, `Mapping.__eq__`, and every write-then-read doctest. + +For the record, on the typing question the issue raises: `collections.abc` offers **nothing** +for Iterable+Settable+Deletable-but-not-Gettable, and cannot cheaply — `MutableMapping` +inherits `__getitem__` as abstract from `Mapping`, and `pop`/`popitem`/`clear`/`setdefault` +are all defined in terms of it; only `update` survives. `dol` has `mk_read_only` / +`disable_setitem` / `disable_delitem` but **no `disable_getitem` and no `mk_write_only`** — +you can take writes away but not reads. We define the `Protocol`s anyway (~15 lines, they +document the shape and serve genuinely write-only sinks) and propose the missing dol +symmetry upstream. Note `@runtime_checkable` checks method *presence* only, so +`isinstance(d, WriteOnlyStore)` is `True` for a `dict`; "must not be gettable" needs an +explicit predicate. + +### 5. How the capability reaches through the pure interface: injected strategies + +```python +BucketStore( + bucket, connection=conn, + writes=transfer_writes(multipart_threshold=64<<20, max_concurrency=8), + reads=bytes_reads(), # or stream_reads() / ranged_reads() +) +``` + +A strategy is a callable, injected at construction. This is the answer to "how do we get +infra-specific optimization without polluting the interface": `__setitem__` stays +`__setitem__`; *how* it uploads is a constructor parameter. + +Note a **structural** reason this cannot be a `dol` value codec: a codec is a pure +`obj -> data` transformation applied by `Store.__setitem__` before the inner write. A +multipart upload needs the *key* and the *client*, and it is a side effect, not a +transformation. So the strategy must live in the leaf store, below `wrap_kvs`. + +Default: `transfer_writes` with boto3's threshold (8 MiB). Small writes take a single +`PutObject`; large ones transparently go multipart. The overhead on small objects is one +branch. + +### 6. The read side, symmetrically + +`s[k]` returns `bytes` — always, because N1 demands it. Streaming is reached three ways, +in increasing explicitness: a `reads=stream_reads()` strategy at construction (the store's +values become chunk iterators — a *different store*, honestly typed); `store.handle(k)` for +`.open()` / `.stream()` / `.read(byte_range=...)`; or a `Filepath` destination for +download-to-disk. + +`ObjectHandle` is not a Mapping and is the documented escape hatch — the same role +`BlobHandle` plays in `azuredol`. + +### 7. `s[k] += v` stays out + +`+=` on a Mapping is `__getitem__` then `__setitem__` — it reads the whole object back, +which is the opposite of the point. The general design belongs in `dol` (see +[dol discussion #29](https://github.com/i2mint/dol/discussions/29) and `dol/appendable.py`'s +`Extender`), and `azuredol` reached the same conclusion from the other side: it *removed* +append-blob-as-default because `MutableMapping.__setitem__` semantically replaces. + +## Consequences + +**Buys.** Arbitrarily large objects through `s[k] = v`. Store-to-store streaming copy where +neither side is ever fully in memory. No new public methods on the Mapping. Users extend the +write domain for their own types without touching s3dol. + +**Costs.** Users must learn one name (`Filepath`) for the filepath case. Refs are s3dol types +that a plain `dict` or `Files` doesn't understand — so they must not escape the leaf layer; +`as_fileobj` is a public extension point so other backends *can* register support, and +fan-out wrappers reject refs with a clear message rather than pickling them. + +**What NOT to do.** Do not accept `str` as a filepath, ever, under any flag. Do not add +`upload_multipart` to the store. Do not implement `+=` here. diff --git a/misc/docs/decisions/0006-key-scoping-and-dol-fixes.md b/misc/docs/decisions/0006-key-scoping-and-dol-fixes.md new file mode 100644 index 0000000..2a407aa --- /dev/null +++ b/misc/docs/decisions/0006-key-scoping-and-dol-fixes.md @@ -0,0 +1,146 @@ +# ADR-0006: Prefix scoping is delegated to `dol` — but only in its safe composition + +- **Status:** Accepted +- **Date:** 2026-08-10 +- **Severity:** This is the most important document in this set. Read it before writing any key-handling code. + +## Context + +v0.1.x does prefix scoping by hand: + +```python +def _key_of_id(self, id): + return id[len(self.prefix):] # base.py:201 +``` + +This is unguarded: if `id` doesn't start with `prefix`, it silently slices anyway. The +obvious fix — and the one every research pass recommended — is *"delete this and use `dol`'s +canonical mechanism, `mk_relative_path_store(prefix_attr='prefix')`"*. + +**That fix is wrong.** `dol`'s prefix machinery has the same bug. + +## The evidence + +Store `{'a/b': 1, 'a/c': 2, 'z': 3, 'ab/x': 4}`, prefix `'a/'`, run against dol 0.3.58: + +| Mechanism | keys produced | verdict | +|---|---|---| +| `KeyCodecs.prefixed('a/')` | `['', '/x', 'b', 'c']` | **CORRUPT** | +| `prefixless_view(store, prefix='a/')` | `['', '/x', 'b', 'c']` | **CORRUPT** | +| `mk_relative_path_store(cls, prefix_attr='prefix')` | `['', '/x', 'b', 'c']` | **CORRUPT** ← the recommended replacement | +| `handle_prefixes(store, prefix='a/')` | `['b', 'c']` | safe (filters first) | +| `Pipe(filt_iter.prefixes('a/'), KeyCodecs.prefixed('a/'))` | `['b', 'c']`, `len == 2`, `'' in p → False` | **safe** | + +The non-matching key `z` becomes `''` (and `w['']` then raises `KeyError: 'a/'`), and the +*sibling* key `ab/x` becomes `/x`. + +In S3 terms: a store scoped to `logs/` surfaces a neighbouring object `logs2/2026.txt` as a +plausible-looking, **writable** key `2/2026.txt`. Writing to it writes outside the store's +scope. For anyone using a prefix as a tenant or app boundary, that is a boundary violation +produced by the storage layer itself. + +## Decision + +### 1. `filt_iter.prefixes(p)` below every relativization is MANDATORY + +It is a **correctness requirement, not an optimization**. The only sanctioned composition: + +```python +relative = Pipe( + filt_iter.prefixes(prefix), # filter FIRST — not optional + KeyCodecs.prefixed(prefix), # then relativize +) +``` + +Bare `mk_relative_path_store` / `KeyCodecs.prefixed` / `prefixless_view` are **banned in +s3dol**, and the ban is enforced by a test in the conformance suite: a store containing +sibling and non-matching keys must expose exactly the in-scope ones, and round-trip them. + +Where the prefix is also pushed down to `ListObjectsV2(Prefix=...)`, the client-side filter is +usually redundant — but "usually" is doing dangerous work there (a pushdown that silently +fails, a provider that ignores `Prefix`, a wrapper composed in a different order), so the +filter stays unconditionally. + +### 2. `url_for` must reach the leaf with the fully-mapped key + +Verified, and worse than the above because it is silent: + +```python +w = KeyCodecs.prefixed('a/')(WithUrl)(...) +w['b'] # -> 1 correct, prefix applied +w.url_for('b') # -> https://x/b WRONG: should be https://x/a/b +isinstance(w, SupportsUrlFor) # -> True the Protocol cannot detect this +``` + +`dol` wrappers delegate unknown attributes to the inner store **with the outer, unmapped +key**. So the moment prefixing moves into a `dol` wrap, every presigned URL points at the +wrong object — and nothing fails. The existing `test_url_for.py` asserts only substring +presence (`"test-bucket" in url`, `"Signature" in url`), so a URL for the wrong key **passes +today**. + +Interim mechanism: route `url_for` through `dol.dig.inner_most_key`. Permanent mechanism: the +dol fix below. + +Test requirement: parse the URL and assert the path equals the fully-prefixed key, and +actually fetch it against moto. Substring assertions are banned for this method. + +### 3. Two fixes go upstream to `dol` first + +Per the owner's decision, these land in `dol` as its own reviewed change, and s3dol then +requires that version. Every other `*dol` adapter almost certainly has the same latent bugs, +so fixing them once in `dol` is worth more than fixing them once in s3dol. + +**dol fix 1 — strict prefix relativization.** A `strict=True` mode (proposed default in a +future major) on the prefix machinery: keys outside the prefix must **raise**, never be +silently sliced. Plus a property test: + +``` +∀ k in-scope: key_of_id(id_of_key(k)) == k +∀ i out-of-scope: key_of_id(i) raises # never returns a corrupted key +``` + +**dol fix 2 — key-mapped delegation.** A mechanism so that delegated methods (`url_for`, +`info`, and anything a backend adds) receive the fully-mapped inner key. Without it, every +capability s3dol adds at Layer B is silently wrong through a Layer C wrap, and the package's +own layering becomes a trap. + +Until both land, s3dol uses the safe local composition and `inner_most_key`, with `# TODO: +upstream to dol (dol#NN)` at each site and a linked issue. **Policy on upstreams:** never +block an s3dol release on a dol PR; never let a local copy diverge silently — raise the dol +floor the day each fix lands and delete the workaround in the same commit. + +### 4. Key validity is checked before the wire + +Probed behaviours that currently leak backend types through the Mapping: + +| key | today | +|---|---| +| `''` | `ParamValidationError` — a botocore type escaping through `__getitem__` | +| `'folder/'` | returns a **sub-store**, not bytes; absent from `list(s)`; `in` says `True` — the object is permanently unreadable through the interface | +| `'bad\ud800key'` | `UnicodeEncodeError` | +| 1025-char key | fine on moto, `KeyTooLongError` on AWS | + +Decisions: normalize all of these to `KeyNotValid` before the request; enforce the +1024-UTF-8-**byte** limit client-side so moto and AWS agree; and set `EncodingType='url'` by +default (per-preset opt-out — GCS rejects it) so keys containing control characters survive +the XML listing. + +The trailing-`/` overload is removed: **`store[k]` always returns bytes**. Sub-stores come +from `store.sub('folder/')`. `store['folder/']` survives only on an explicitly-constructed +navigable reader for notebook use. This is what makes empty-directory markers (which a +filesystem migration creates) addressable at all. + +## Consequences + +**Buys.** Prefix scoping that is actually correct, and correct for every `*dol` adapter once +upstreamed. Presigned URLs that point at the right object. Sub-stores with zero round-trips +from `dol` rather than `type(self)(**self.__dict__)`. + +**Costs.** A dependency on a `dol` release for the clean version, and an interim workaround +that must be deleted later — tracked, with the usual risk that it isn't. + +**What NOT to do.** + +1. **Never use `mk_relative_path_store`, `KeyCodecs.prefixed` or `prefixless_view` bare.** +2. Never assert on a presigned URL by substring. +3. Never add a Layer B method without a Layer C key-mapping test. diff --git a/misc/docs/decisions/0007-naming-and-compatibility.md b/misc/docs/decisions/0007-naming-and-compatibility.md new file mode 100644 index 0000000..9ca433e --- /dev/null +++ b/misc/docs/decisions/0007-naming-and-compatibility.md @@ -0,0 +1,121 @@ +# ADR-0007: Names, public API, and the deprecation path + +- **Status:** Accepted +- **Date:** 2026-08-10 + +## Context + +The v0.1.x names are the least consistent in the `*dol` family: + +- `S3BucketDolWithouBucketCheck` ships a typo. +- `S3Store` is a **function** annotated `-> Store` that never returns a `dol.Store` + (`isinstance(S3Store(...), dol.Store)` is `False` — verified). +- The `Dol` suffix (`S3Dol`, `S3ClientDol`, `BaseS3BucketDol`) is used by **no other** blob + adapter: `azuredol` has `ContainerCollection`/`ContainerReader`/`ContainerStore`; `cosmodol` + has `CosmosItems`/`CosmosAccount`. +- Six overlapping entry points exist, two are exported, and none is the one a first-time + user should type. +- `S3DolReadOnly` is a `functools.partial`, so it is a function — `isinstance` and + subclassing don't work on it. + +There are real dependents, which bounds how freely we rename: + +| Dependent | Usage | Ours to merge? | +|---|---|---| +| `lacing` | `from s3dol.store import S3Store`; `S3Store(bucket_name, path=prefix, **kw)` — bucket **positional** | yes | +| `http_cosmo_prep` | `from s3dol.store import S3Store`; `S3Store(path=..., bucket_name=..., endpoint_url=...)` — bucket **by keyword** | **no** (cosmograph-org, needs peer review) | +| `reelee` | declares `s3dol` in `pyproject.toml` | yes | +| `py2store` | `from s3dol.tests.util import extract_s3_access_info, get_s3_test_access_info_from_env_vars`, wrapped in `suppress(ImportError)` — so breakage is **silent** | yes | + +And merging auto-publishes to PyPI (`[tool.wads.ci.publish] enabled = true`); version numbers +burn permanently. + +## Decision + +### 1. New names, mirroring `dol.filesys` and `azuredol` + +| v0 | v1 | Why | +|---|---|---| +| `BaseS3BucketReader` | `BucketCollection` + `BucketReader` | splits two conflated responsibilities; mirrors `FileCollection`/`FileBytesReader` | +| `BaseS3BucketDol` | `BucketStore` | "Dol" carries no meaning | +| `S3BucketReader` / `S3BucketDol` | (Layer C) `BucketReader` / `BucketStore` with `prefix=` | one class, prefix is a parameter | +| `S3ClientReader` / `S3ClientDol` | `BucketsReader` / `Buckets` | the key *is* a bucket name; "Client" names the implementation, not the mapping | +| `S3Dol` | `S3Endpoints` | it maps endpoint/profile names → bucket stores | +| `S3DolReadOnly` | *deleted* | use the `*Reader` classes | +| `S3BucketDolWithouBucketCheck` | *deleted* | typo, and the behaviour is now the default | +| `SupabaseS3BucketDol` | *deleted* | → `preset='supabase'` ([ADR-0003](0003-provider-presets-and-capabilities.md)) | +| `s3dol.utility` | `s3dol.errors` | 5 of 7 exception classes and 5 of 9 `Resp` methods are dead | +| — | `ObjectHandle` | new; the per-object escape hatch | +| — | `s3_store(...)` | the lowercase factory, matching `azuredol.azure_store` | + +### 2. The one-liner + +`s3dol.s3_store(bucket)` is what line 1 of the README shows. Zero credential ceremony, no +`make_bucket`/`skip_bucket_check`/`profile_name` in sight. + +### 3. `s3dol.store.S3Store` becomes a deprecated shim, removed in v2 + +It keeps its current signature exactly — `bucket_name` accepted **both positionally and by +keyword**, `path=` (not renamed), plus `aws_access_key_id`, `aws_secret_access_key`, +`aws_session_token`, `endpoint_url`, `region_name`, `profile_name`, `make_bucket`, +`skip_bucket_check` — forwards to the new API, and emits `DeprecationWarning`. + +`s3dol/store.py` **must survive as a module**: both external dependents import the +fully-qualified path, not the package root. + +The shim is not merely compatible, it is a **fix delivery mechanism**: dependents get the +corrected endpoint/credential resolution ([ADR-0002](0002-boto3-as-engine.md), +[ADR-0003](0003-provider-presets-and-capabilities.md)) without changing a line. That matters +most for `http_cosmo_prep`, which passes an explicit `endpoint_url` for a non-AWS endpoint +and is therefore a live victim of the bug: whenever `AWS_ACCESS_KEY_ID` is exported, +`base.py:82` drops its endpoint and the store silently talks to AWS instead. + +`s3dol/tests/util.py` keeps `extract_s3_access_info` and +`get_s3_test_access_info_from_env_vars`, because `py2store`'s import of them fails silently. + +### 4. Behaviour changes that the shim deliberately does NOT preserve + +These are bug fixes, and preserving them would mean preserving data-misrouting: + +| v0 behaviour | v1 | +|---|---| +| explicit `endpoint_url` dropped when env credentials exist | honoured | +| explicit credentials overridden by env | explicit wins | +| write to a missing bucket **creates** it (even with `make_bucket=False`) | raises unless `on_missing_bucket='create'` | +| `list(store)` returns `[]` on any error | raises | +| `del buckets[name]` cascades, unpaginated | refuses non-empty; `force=True` is explicit | + +Each is called out in the release notes as behaviour-changing. The resolution ladder — +explicit kwargs > preset > `AWS_ENDPOINT_URL_S3` > `AWS_ENDPOINT_URL` > profile > chain — is +documented and tested, and `s3dol.diagnose()` prints what resolved and from where (never the +secret). + +`AWS_ENDPOINT_URL_S3` deserves its own note: it silently outranks everything, is +service-specific so it beats the generic `AWS_ENDPOINT_URL`, and it is **not** in botocore's +`BOTOCORE_DEFAUT_SESSION_VARIABLES`, so it is invisible to naive introspection. It is also +why CI is currently green on a data-misrouting bug — the test env re-supplies the endpoint +that the code throws away. + +### 5. Release mechanics + +`s3dol.__version__` is added (absent today). Deprecations name their removal version. Order +of operations, because publishing is automatic and irreversible: + +1. Land the `dol` fixes ([ADR-0006](0006-key-scoping-and-dol-fixes.md)). +2. Pin `s3dol<1` in `lacing` and `reelee`, and open the `http_cosmo_prep` PR. +3. Merge s3dol v1. Release notes lead with the behaviour changes. + +Step 2 before step 3 is not optional: a stalled cross-org PR must not be able to strand +that repo on a broken line. + +## Consequences + +**Buys.** Names that read like the rest of the family. Existing users keep working *and* get +the bug fixes. A documented migration. + +**Costs.** A compat module to carry until v2, and a legacy signature (`path=`, the +`make_bucket` tri-state) that must keep working while the new API uses better vocabulary +(`prefix=`, `on_missing_bucket=`). Two vocabularies coexist for one major version. + +**What NOT to do.** Do not remove `s3dol/store.py`. Do not rename `path=` on the shim. Do not +merge before the dependents are pinned. diff --git a/misc/docs/decisions/0008-testing-architecture.md b/misc/docs/decisions/0008-testing-architecture.md new file mode 100644 index 0000000..f9a9ea3 --- /dev/null +++ b/misc/docs/decisions/0008-testing-architecture.md @@ -0,0 +1,115 @@ +# ADR-0008: Four test tiers, a shipped in-memory fake, and an exported conformance suite + +- **Status:** Accepted +- **Date:** 2026-08-10 + +## Context + +Today: **7 tests**, of which **5 need a live S3 endpoint**. `pytest -q` on a developer +machine gives `3 passed, 5 deselected`. The three that always run are the presigned-URL +tests, and they assert only substring presence — `"test-bucket" in url`, `"Signature" in url` +— so a URL pointing at the **wrong key** passes ([ADR-0006](0006-key-scoping-and-dol-fixes.md) §2). + +`conftest.py` is genuinely thoughtful (it TCP-probes the endpoint and *deselects* rather than +skips), but the mechanism hides the problem: the suite is green while barely testing +anything, and `S3DOL_S3_PROBE_ENDPOINT` can only *enable* tests, never *redirect* them, +because the tests hardcode `localhost:4566`. + +s3dol also ships **nothing** for downstream users. A user whose service takes a store as a +dependency has no way to test their code without S3. + +## Decision + +### Four tiers, with tier 3 as the merge gate + +| Tier | What | Runs | +|---|---|---| +| 1 | **Pure unit, no I/O.** Error classification over *synthesized* `ClientError`s; key-codec property tests; preset merging; the prefix round-trip law. Should be the majority. | always | +| 2 | **In-process `@mock_aws` (moto).** The hermetic default. | always | +| 3 | **Container: moto-server + MinIO.** One conformance suite parameterized over endpoints. | merge gate | +| 4 | **Live providers** (R2, B2, Supabase), `@pytest.mark.live`, credentialed. | opt-in / nightly | + +Tier 1 matters more than it sounds: the error taxonomy, the key laws and the preset registry +are where the correctness lives, and none of them needs a network. Building tier 1 first is +what lets the rewrite be red/green rather than hopeful. + +### `s3dol.testing` is shipped, not just `tests/` + +```python +from s3dol.testing import mock_s3, conformance + +def test_my_service(): + assert MediaService(mock_s3()).url('a') # no network, no docker, no moto +``` + +`mock_s3()` is an in-process fake that passes **the same conformance suite** as the real +store — including `url_for`, `info`, ranged reads, `delete_many` and the error taxonomy. This +is the tier the ecosystem is missing: `azuredol` ships an Azurite context manager, but +nothing in the family ships an in-memory tier, and that is what downstream users actually +need. + +`conformance` is exported so sibling `*dol` packages and user code can run it against their +own stores. + +### What the conformance suite must assert + +Beyond the obvious Mapping laws, these are the ones that would have caught real bugs: + +1. **Prefix scoping round-trip.** With sibling and non-matching keys present + (`{'a/b','a/c','z','ab/x'}` scoped to `a/`), the store exposes exactly the in-scope keys. + This is the [ADR-0006](0006-key-scoping-and-dol-fixes.md) §1 ban, enforced. +2. **`url_for` correctness by parsing**, not substring: the URL path must equal the + fully-prefixed key, and fetching it against moto must return the object. +3. **`iter`/`contains` agreement**: `all(k in s for k in s)`. +4. **Never silently empty**: listing a missing/unlistable bucket raises. +5. **Value law**: `s[k] = v ⟹ s[k] == normalize(v)` for every member of the write domain + ([ADR-0005](0005-large-object-io.md) N3). +6. **Picklability**: `pickle.loads(pickle.dumps(store))` works, and a `ProcessPoolExecutor` + round-trip works. Today `pickle` raises `PicklingError` and `deepcopy` raises + `RecursionError` — a store that cannot cross a process boundary is unusable with Dask or + multiprocessing, and nothing currently notices. +7. **No secret in `repr`**: `assert SECRET not in repr(conn)`, plus a traceback-locals scan. + +### Cost model: `__len__` is not implemented + +`dol.base.Collection.__len__` counts by iterating, so `len(store)` is a full paginated +listing — and worse, `list(store)` currently costs **two** listings because `list()` takes a +length hint from `__len__`. Following `azuredol` §2, `BucketStore` does not implement +`__len__` at all; `len(store)` raises `TypeError` with guidance to `sum(1 for _ in store)`. +`Buckets.__len__` is fine — bucket counts are small. + +Listing caches are opt-in (`dol.cached_keys`), never default: the notebook explorer wants +them and the pipeline is actively harmed by them. + +### Fixing the harness + +- Delete the deselect-by-module-stem allowlist — it silently swallows any new hermetic test. +- Every test gets a unique bucket via fixture; `monkeypatch.setenv` only (tests currently + mutate `os.environ` process-wide). +- Make the endpoint env var able to **redirect**, not just enable. +- Doctests run in the default gate against the in-memory fake, so every documented example is + executable with no endpoint. `--doctest-modules` on; `py.typed` shipped. +- Add `lacing`'s `tests/test_artifact_store_s3.py` as a required cross-repo gate — it is the + tightest existing contract test for s3dol. + +### On moto's fidelity + +moto is good enough for tier 2 but diverges: it accepts >1024-byte keys, has open +`aws-chunked` and composite-checksum bugs, and returns bodies on HEAD. It is faithful on +`CreateBucket`/`LocationConstraint` (checked — a claim to the contrary in the research was +wrong). Tier 3 exists because tier 2's divergences are exactly in the areas +[ADR-0003](0003-provider-presets-and-capabilities.md) cares about. + +## Consequences + +**Buys.** A suite that is green because it passes, not because it deselected. Downstream +users can test. Provider compatibility is actually exercised. The rewrite gets a safety net +before it starts. + +**Costs.** An in-memory fake is code that must itself stay faithful — mitigated by running +the *same* conformance suite against it and the real thing, so drift fails a test. Tier 3 +needs containers in CI. + +**What NOT to do.** Do not assert on presigned URLs by substring. Do not let a test depend on +a bucket another test created. Do not add a tier-2-only test for behaviour that only tier 3 +can distinguish. diff --git a/misc/docs/decisions/0009-scope-and-deferrals.md b/misc/docs/decisions/0009-scope-and-deferrals.md new file mode 100644 index 0000000..2482cdd --- /dev/null +++ b/misc/docs/decisions/0009-scope-and-deferrals.md @@ -0,0 +1,141 @@ +# ADR-0009: What v1 contains, what waits, and where the `s3dol` / `botodol` line runs + +- **Status:** Accepted +- **Date:** 2026-08-10 + +## Context + +Two forces pull opposite ways. + +**Pull toward more:** S3 has ~108 operations, and a surprising number map beautifully onto +`collections.abc`. In-flight multipart uploads are the single best fit in the whole API — +`ListMultipartUploads` + `AbortMultipartUpload` is literally a `Mapping` whose `__delitem__` +aborts, and orphaned uploads are the classic silent S3 bill. Object versions, tags, user +metadata and bucket configuration all have clean Get/Put/Delete triples. + +**Pull toward less:** zarr is the cautionary tale. Its v3 rewrite replaced a +`MutableMapping` store interface with a 15-method async ABC plus capability flags, and in +doing so **killed five of its own backends** — `DBMStore`, `LMDBStore`, `SQLiteStore`, +`MongoDBStore`, `RedisStore` all "do not have an equivalent in Zarr-Python 3" — while +leaving vestigial flags like `supports_partial_writes -> Literal[False]`. + +The eight research passes collectively proposed ~6 Protocols, 4 strategy slots, 16 presets, +an async submodule, 8 new store families and 6 upstream `dol` changes. Shipping that at once +is how a package becomes unimplementable. + +## Decision + +### v1.0 — the scope + +``` +connection.py S3Connection: credential + endpoint SSOT; lazy, picklable, redacting +presets.py Preset + Capabilities registry +errors.py one translate_s3_errors seam + the taxonomy +values.py Filepath / Chunks / Streamable + as_fileobj +writes.py write strategies (simple / transfer / multipart) +reads.py read strategies (bytes / stream / ranged / to-file) +base.py BucketCollection/Reader/Store, BucketsCollection/Reader/Buckets, ObjectHandle +stores.py relative-key stores + codec facades +recipes.py s3_store(...) and friends +store.py deprecated shim +testing.py in-memory fake + exported conformance suite +``` + +Plus, from the "beyond blobs" analysis, the items that are **free or nearly so** because they +fall out of work already being done: + +- **`ObjectInfo` from LIST metadata.** Every `ListObjectsV2` response already carries size, + mtime, ETag and storage class, and v0 throws them away. A `store.info(k)` (one HEAD) and a + cheap listing-derived metadata view cost almost nothing and serve nearly every use case. +- **Prefix tree via `CommonPrefixes`.** `Resp.common_prefixes` has existed in `utility.py` + since 2023 and is **called from nowhere**. `store.prefixes()` is one LIST with a delimiter. +- **A presigned-URL view.** `url_for` already exists; a `Mapping` face over it is trivial. + +The multipart **parts** store (`MutableMapping[int, bytes]`, filled then committed) is built +as an *internal mechanism* of the write strategy in v1 — it is how `s[k] = v` stays pure — +but it is **not** exported as a public Mapping until v1.x. There is no `DeletePart` +operation, so its `__delitem__` would be a lie. + +### v1.x — deferred, tracked, in this order + +1. **In-flight multipart uploads as a `Mapping`** (`del uploads[(key, upload_id)]` aborts). + Highest portability of the deferred set, real cost savings, cleanest fit. +2. **Object versions** (+ delete-marker view). +3. **Object tags** and **user metadata** — natural, but portability 2/5 and 4/5; each needs + capability detection. Note user-metadata *writes* are a `CopyObject`, which is surprising + enough to document loudly. +4. **Bucket configuration as one `Mapping`** — `cfg['lifecycle'] = {...}` over the + Get/Put/Delete triples. +5. **Object annotations** — natively a per-object `MutableMapping[str, bytes]`, and the first + S3 feature that is a nested store rather than a flat blob store. Deferred not for design + reasons but for two hard facts: **no S3-compatible provider implements it** (portability + 0/5), and it needs `botocore>=1.43.31`, too fresh for a storage library's hard floor. + Feature-detect with `hasattr(client, 'put_object_annotation')`. +6. **fsspec adapter** (`to_fsspec` / `from_fsspec`) — one adapter buys pandas, dask, pyarrow + and zarr-v3-via-`FsspecStore`. +7. **`s3dol[fast]`** — obstore for the object level only, if re-measurement justifies it. +8. **`s3dol.aio`** — async, mirroring the sync surface. + +### Never + +- `GetObjectTorrent` — dead. +- `SelectObjectContent` — AWS closed it to new customers in July 2024 and points at Athena / + Object Lambda instead. Building a Mapping over a service new users cannot enable is + negative value. +- **Native async `Store` ABC / zarr-v3 store.** An s3dol store is already a working **zarr + v2** store for free. zarr v3 deliberately left `MutableMapping` behind for async and + byte-range coalescing; chasing it means becoming a different package. We adapt (via fsspec) + rather than compete. + +### The `s3dol` / `botodol` line + +Three mechanical tests, applied in order. + +**Test 1 — the endpoint test (decisive).** *Can it be reached through the same +`boto3.client('s3', endpoint_url=...)` a MinIO or R2 user already holds?* If yes → s3dol. + +This is a fact about botocore, not a taste judgement: `s3control` is a **separate service +model** with a different endpoint, every operation takes a required `AccountId`, and **no +S3-compatible provider implements any of it**. Same for `s3tables`, `s3vectors`, +`s3outposts`, `glacier`. Drawing the line here yields exactly "everything portable is +inside", and no case ever needs arguing. + +**Test 2 — the key-shape test.** If the natural key is an ARN or an +`(AccountId, Region, Name)` triple rather than a bucket name or object key → botodol. Storage +keys are strings a user typed; control-plane keys are identities AWS minted. + +**Test 3 — the value test.** If the value is a resource with a lifecycle you *poll* rather +than data or a config document → botodol. There is no `DeleteJob` for a Batch Operations job, +only `UpdateJobStatus(Cancelled)` — a `MutableMapping` whose `__delitem__` means "please try +to cancel" is a lie. + +**→ botodol, never s3dol:** all `s3control` operations (access points, batch jobs, Storage +Lens, Access Grants, MRAP), `s3tables`, `s3vectors`, `s3outposts`, `glacier`, Object Lambda. + +**Two apparent exceptions that the tests resolve correctly:** + +- **Access points** are *consumed* as bucket names — the API docs say to pass the ARN in + place of the bucket. So s3dol accepts an access-point ARN wherever it accepts a bucket + name (one line in the bucket-name normalizer). *Administering* them needs `s3control` → + botodol. The line runs between using and administering, and Test 1 puts it there. +- **S3 Express / directory buckets** need a zonal endpoint, `CreateSession`, `/`-only + delimiters and no `UploadIdMarker` pagination. That is one row in the preset registry — the + same mechanism that describes R2 and Supabase. No new abc, no new module. + +**The obligation this creates:** the spec-driven sub-resource store and the paginated-list +store are **boto-generic, not S3-specific** — the same code will serve DynamoDB tables, SQS +queues, Secrets Manager secrets. When they get built (v1.x, item 4), they go in a documented +public module and **botodol depends on s3dol** for them. Do not build a shared base package +speculatively; do not let botodol reimplement them. If they prove fully boto-agnostic, +promote to `dol` later — cheap in that direction, expensive in reverse. + +## Consequences + +**Buys.** A v1 that can actually ship, whose every element is 4/5 or 5/5 on portability. A +written, mechanical rule for future scope arguments. + +**Costs.** Several genuinely nice interfaces wait. The `Capabilities` table exists in v1 but +is barely exercised until the low-portability families arrive in v1.x — accepted, because +retrofitting capability declaration later is much worse. + +**Enforcement.** A line budget, and one rule: **no new `Protocol` without two implementers.** From f3c4b0702250fa7825dd263915306412036196da Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Mon, 10 Aug 2026 10:13:11 +0100 Subject: [PATCH 2/2] docs: revise ADRs after adversarial review; add ADR-0010 Three findings from the review were verified and are structural: - Prefix scoping cannot live in a dol wrapper above an absolute-keyed leaf: dol delegates methods with the OUTER key, so url_for/sub/handle/info/ prefixes/delete_many all address the wrong object (delete_many destroys it), prefix pushdown to ListObjectsV2 is unimplementable, and filt_iter resurrects the __len__ ADR-0008 deliberately omits. azuredol's CODE puts the prefix in the leaf (its architecture.md says otherwise). ADR-0001 and ADR-0006 rewritten accordingly. - botocore presigns with SigV2 unless signature_version is set explicitly (us-east-1 and every custom endpoint), while meta.config reports s3v4. This is a live bug in v0.1.x that the existing substring test cannot see. - Passing EncodingType='url' explicitly disables botocore's own decoder: 5 of 7 test keys stop round-tripping. Decision deleted. Also: ADR-0010 (bucket-existence policy, delete_many, cascading delete), plus fixes to the value law, singledispatch registration, typing claims, error classification key, 403-means-absent, setdefault-vs-archived, import budget, naming collision, and the conformance suite. Claude-Session: https://claude.ai/code/session_01LioNGNe7Yts1FC3247veKp --- misc/docs/README.md | 41 +++- misc/docs/architecture.md | 147 +++++++++---- .../decisions/0001-layered-architecture.md | 105 ++++++--- misc/docs/decisions/0002-boto3-as-engine.md | 16 +- .../0003-provider-presets-and-capabilities.md | 65 +++++- misc/docs/decisions/0004-error-taxonomy.md | 58 ++++- misc/docs/decisions/0005-large-object-io.md | 96 ++++++--- .../0006-key-scoping-and-dol-fixes.md | 201 ++++++++++-------- .../0007-naming-and-compatibility.md | 78 +++++-- .../decisions/0008-testing-architecture.md | 62 ++++-- .../decisions/0009-scope-and-deferrals.md | 13 +- .../0010-bucket-and-bulk-operations.md | 93 ++++++++ 12 files changed, 730 insertions(+), 245 deletions(-) create mode 100644 misc/docs/decisions/0010-bucket-and-bulk-operations.md diff --git a/misc/docs/README.md b/misc/docs/README.md index 13070eb..605f30d 100644 --- a/misc/docs/README.md +++ b/misc/docs/README.md @@ -17,24 +17,47 @@ output; don't put prose here that a build step will overwrite.) | [0003](decisions/0003-provider-presets-and-capabilities.md) | Providers are config rows, not subclasses | adding a backend, or hitting "works on AWS, breaks on X" | | [0004](decisions/0004-error-taxonomy.md) | One error seam; a taxonomy that never lies | touching exception handling | | [0005](decisions/0005-large-object-io.md) | Value refs + injected transfer strategies | anything about big objects, streaming, or `s[k] = v` types | -| [0006](decisions/0006-key-scoping-and-dol-fixes.md) | **Prefix scoping — read before writing key code** | always, if you touch keys | +| [0006](decisions/0006-key-scoping-and-dol-fixes.md) | **Prefix normalization, key validity, `dol` traps** | always, if you touch keys | | [0007](decisions/0007-naming-and-compatibility.md) | Names, public API, deprecation path | renaming anything, or planning the release | | [0008](decisions/0008-testing-architecture.md) | Four tiers, shipped fake, exported conformance | writing a test | | [0009](decisions/0009-scope-and-deferrals.md) | v1 scope, deferrals, the `s3dol`/`botodol` line | proposing a feature | +| [0010](decisions/0010-bucket-and-bulk-operations.md) | Bucket-existence policy, `delete_many`, cascading delete | touching bucket lifecycle or bulk ops | -## The three things most likely to bite you +## The five things most likely to bite you -1. **`dol`'s prefix machinery silently corrupts non-matching keys.** `mk_relative_path_store`, +1. **`url_for` presigns with SigV2 unless you set `signature_version` explicitly** — for + `us-east-1` and for *every custom endpoint* (MinIO, LocalStack, R2). AWS rejects SigV2 on + buckets created after June 2020. `client.meta.config.signature_version` reports `'s3v4'` + while doing it, and a `"Signature" in url` assertion cannot tell the two apart (SigV2 has + `Signature=`, SigV4 has `X-Amz-Signature=`). This is a live bug in v0.1.x. + [ADR-0003 §4](decisions/0003-provider-presets-and-capabilities.md). +2. **`dol`'s prefix machinery silently corrupts non-matching keys.** `mk_relative_path_store`, `KeyCodecs.prefixed` and `prefixless_view` turn a sibling key `ab/x` into `/x` and a - non-matching key `z` into `''`. Only `Pipe(filt_iter.prefixes(p), KeyCodecs.prefixed(p))` - is safe. [ADR-0006 §1](decisions/0006-key-scoping-and-dol-fixes.md). -2. **`url_for` through a `dol` key-wrap returns a URL for the wrong object, silently**, and - `isinstance(store, SupportsUrlFor)` still says `True`. - [ADR-0006 §2](decisions/0006-key-scoping-and-dol-fixes.md). -3. **botocore ≥1.36 sends checksums by default**, and several S3-compatible providers either + non-matching key `z` into `''`. This is why the prefix lives in the leaf; if you stack a + `dol` codec anyway, only `Pipe(filt_iter.prefixes(p), KeyCodecs.prefixed(p))` is safe — and + only after normalizing `p` to end in the delimiter. + [ADR-0006 §1](decisions/0006-key-scoping-and-dol-fixes.md). +3. **A `dol` wrapper delegates methods with the outer, unmapped key**, so `url_for`, `sub`, + `handle`, `info`, `prefixes` and `delete_many` silently address the wrong object — + `delete_many` *destroys* it. `isinstance(store, SupportsUrlFor)` still says `True`. Use + `inner_most_key(wrapped_self(self), k)`; `inner_most_key(self, k)` returns `None`. + [ADR-0001 §Why the prefix lives in the leaf](decisions/0001-layered-architecture.md). +4. **botocore ≥1.36 sends checksums by default**, and several S3-compatible providers either reject them loudly or persist the `aws-chunked` framing *into the object body*. The fix is client config plus `s3transfer>=0.11.2`. [ADR-0003 §3](decisions/0003-provider-presets-and-capabilities.md). +5. **Never pass `EncodingType` to a list call.** botocore sets it *and* decodes the response, + but only when it set it itself — passing it explicitly drops 5 of 7 test keys out of + round-trip. [ADR-0006 §4](decisions/0006-key-scoping-and-dol-fixes.md). + +## A note on how these docs were revised + +ADRs 0001 and 0006 were substantially rewritten after an adversarial review executed their +claims against the real `dol`, `botocore` and `moto`. The original design put prefix scoping in +a `dol` wrapper above an absolute-keyed leaf, citing `azuredol` as precedent — but `azuredol`'s +*code* does the opposite of its *documentation*, and the wrapper design produces six +silently-wrong methods plus full-bucket scans. The lesson is recorded in ADR-0001: **a sibling +package's design doc is a claim; its source is the evidence.** ## Convention diff --git a/misc/docs/architecture.md b/misc/docs/architecture.md index 003fbd7..877a397 100644 --- a/misc/docs/architecture.md +++ b/misc/docs/architecture.md @@ -39,16 +39,13 @@ see [decisions/0009](decisions/0009-scope-and-deferrals.md). ``` ┌────────────────────────────────────────────────────────────────────┐ -│ Layer D — recipes (s3dol.recipes) │ -│ s3_store(...) / s3(...) factories, codec stacks (S3Jsons, ...) │ +│ Layer C — recipes (s3dol.recipes) │ +│ s3_store(...) factory, codec stacks (S3Jsons, S3Texts, ...) │ │ Built ONLY by wrap_kvs / Pipe composition. Never by subclassing. │ ├────────────────────────────────────────────────────────────────────┤ -│ Layer C — relative-key stores (s3dol.stores) │ -│ Prefix scoping via dol. Sub-stores. This is what users hold. │ -├────────────────────────────────────────────────────────────────────┤ │ Layer B — close-to-metal (s3dol.base) │ -│ BucketCollection/Reader/Store, Buckets, ObjectHandle. │ -│ ABSOLUTE keys, bytes in / bytes out, one error seam, no codecs. │ +│ BucketCollection/Reader/Store, Endpoint*, ObjectHandle. │ +│ Owns a normalized `prefix`. bytes in / bytes out, one error seam. │ ├────────────────────────────────────────────────────────────────────┤ │ Layer A — connection (s3dol.connection) │ │ S3Connection: the credential + endpoint SSOT. Lazy, picklable, │ @@ -61,13 +58,23 @@ see [decisions/0009](decisions/0009-scope-and-deferrals.md). Every public class belongs to exactly one layer. No mixing. Two rules make the layering load-bearing rather than decorative: -- **Layer B keys are absolute.** All prefix arithmetic happens in Layer C, in `dol`. A - Layer B store addresses the bucket's real keyspace, which is what makes `url_for`, - `info`, and the transfer strategies correct by construction — they operate on the key S3 - actually sees. -- **Layer D never subclasses.** If a recipe cannot be expressed as a composition of Layer C +- **Layer B owns the prefix.** A bucket store carries a *normalized* `prefix` and addresses + keys relative to it: `_id_of_key`/`_key_of_id` live in the leaf, the prefix is pushed into + `ListObjectsV2(Prefix=…)`, it appears in `__repr__`, and sub-stores are built by + `self._with(prefix=…)`. `dol` is used for **codecs, filtering and caching — not for prefix + arithmetic.** + + > This reverses an earlier draft of this document, which put prefixing in a `dol` wrapper + > above an absolute-keyed leaf. That does not work, and the reasons are recorded in + > [ADR-0001](decisions/0001-layered-architecture.md) §"Why the prefix lives in the leaf". + > In short: `dol` wrappers delegate unknown attributes with the **outer, unmapped** key, so + > every capability method (`url_for`, `info`, `handle`, `sub`, `prefixes`, `delete_many`) + > silently addresses the wrong object; and there is no channel for a wrapper to push its + > prefix into the leaf's listing call, so every scoped listing becomes a full-bucket scan. + +- **Layer C never subclasses.** If a recipe cannot be expressed as a composition of Layer B plus `dol` wrappers, that is a signal the capability belongs in Layer B as a parameter, - not in Layer D as a subclass. This is the rule that keeps per-vendor classes + not in Layer C as a subclass. This is the rule that keeps per-vendor classes (`SupabaseS3BucketDol`) from reappearing. ### Layer A — `s3dol.connection` @@ -102,43 +109,72 @@ Follows `dol.filesys`' triangle, in S3 vocabulary: ``` BucketCollection (Collection — __iter__ over object keys) - └── BucketReader (+ __getitem__ -> bytes, url_for, info, handle) - └── BucketStore (+ __setitem__ / __delitem__) + └── BucketReader (+ __getitem__ -> bytes, url_for, info, handle, sub, prefixes) + └── BucketStore (+ __setitem__ / __delitem__ / delete_many) -BucketsCollection (Collection — __iter__ over bucket names) - └── BucketsReader (+ __getitem__ -> BucketReader) - └── Buckets (+ __setitem__ / __delitem__ for buckets) +EndpointCollection (Collection — __iter__ over bucket names) + └── EndpointReader (+ __getitem__ -> BucketReader) + └── EndpointStore (+ __setitem__ / __delitem__ for buckets) ``` plus `ObjectHandle` — the escape hatch for one object, which is **not** a Mapping and is where ranged reads, streaming, multipart and object metadata live. +`EndpointStore`, not `BucketsStore`: naming the *containing* resource (as `azuredol` does with +`ContainerStore`/`AccountStore`) avoids shipping `BucketStore` and `BucketsStore` — two of the +package's most-used classes, one silent `s` apart, with **opposite key spaces**, where a typo +yields a working, silently-wrong store. + | Operation | Contract | |---|---| | `__getitem__(k)` | Returns `bytes`. `KeyError` iff absent. Auth/config errors re-raised untouched. | | `__setitem__(k, v)` | `v` in a closed, documented union (see [0005](decisions/0005-large-object-io.md)). Replaces. | -| `__delitem__(k)` | `KeyError` iff absent. | -| `__contains__(k)` | One `HeadObject`. `False` iff absent; **raises** on auth/config failure. | -| `__iter__()` | Lazy paginated `ListObjectsV2`. **Raises** if the bucket is missing or unlistable — never yields empty. | +| `__delitem__(k)` | **Idempotent.** S3's `DeleteObject` returns 204 for an absent key; raising `KeyError` would require a `HeadObject` probe — banned below, doubles the cost of every delete, and is TOCTOU-racy. `strict_delete=True` opts into the probe and is documented as costing an extra request. | +| `__contains__(k)` | One `HeadObject`. `False` iff absent; **raises** on auth/config failure. See [0004](decisions/0004-error-taxonomy.md) §2 for the HEAD ambiguity. | +| `__iter__()` | Lazy paginated `ListObjectsV2(Prefix=self.prefix)`. **Raises** if the bucket is missing or unlistable — never yields empty. | | `__len__()` | **Not implemented.** Raises `TypeError` with guidance. See [0008](decisions/0008-testing-architecture.md) §cost model. | | `__repr__` | bucket, prefix, endpoint host, mode. No secrets, no addresses. | -| `url_for(k, ...)` | Presigned URL. Zero object requests. Prefix-aware. | +| `url_for(k, ...)` | Presigned URL. Zero object requests. **Always SigV4** — see [0003](decisions/0003-provider-presets-and-capabilities.md) §4. **The URL cannot outlive the signing credential**: with STS/SSO/instance-profile credentials it dies with the session (default 1 h) regardless of `expires_in`. Capped at 604800 s (SigV4's limit, which botocore does not enforce); clamps and warns when the resolved credentials expire sooner. | +| `sub(prefix)` | A `BucketReader`/`BucketStore` with an extended, normalized prefix. Zero round-trips. | +| `info(k)` | One `HeadObject` → size, mtime, etag, content-type, storage class, restore status. | +| `prefixes(p='')` | One `ListObjectsV2(Delimiter='/')` → `CommonPrefixes`, relative to the store's own prefix. | +| `delete_many(keys)` | See [0010](decisions/0010-bucket-and-bulk-operations.md). | -### Layer C — `s3dol.stores` +**The rule for what may join this table** (it is otherwise how a surface grows to twenty): a +method may be added iff it takes a key and is either a pure read of metadata or an address +(`info`, `url_for`), or returns a store or handle (`sub`, `handle`, `prefixes`). Anything that +mutates, batches, or takes non-key arguments belongs on `ObjectHandle` or a recipe. +`delete_many` is an explicit, named exception, admitted only because the cost difference +against a `__delitem__` loop is an order of magnitude. -Prefix scoping, delegated to `dol` — but **only in its safe composition**. This is a -correctness requirement, not an optimization; see -[decisions/0006](decisions/0006-key-scoping-and-dol-fixes.md), which is the most important -document here. - -### Layer D — `s3dol.recipes` +### Layer C — `s3dol.recipes` ```python -s3_store(bucket, *, prefix='', preset=None, connection=None, codec=None, ...) -> BucketStore -S3Jsons = wrap_kvs(BucketStore, value_codec=ValueCodecs.json()) -S3Texts = wrap_kvs(BucketStore, value_codec=ValueCodecs.str_to_bytes()) +s3_store( + bucket, *, prefix='', preset=None, connection=None, + value_codec=None, on_missing_bucket='assume', anon=False, readonly=False, +) -> MutableMapping[str, bytes] + +S3Texts = wrap_kvs(BucketStore, value_codec=ValueCodecs.str_to_bytes()) +S3Jsons = wrap_kvs( + BucketStore, + value_encoder=lambda o: json.dumps(o).encode(), + value_decoder=json.loads, +) ``` +Two footnotes that cost real debugging time if forgotten: + +- `s3_store` is annotated `-> MutableMapping[str, bytes]`, **not** `-> BucketStore`: a `dol` + wrap returns a new class that is not a `BucketStore` subclass, and the concrete type varies + with the arguments. (v0 annotated its factory `-> Store` and returned something that was + not one; inverting that lie is not an improvement.) +- `S3Jsons` uses `value_encoder`/`value_decoder`, not `value_codec=ValueCodecs.json()` — + `ValueCodecs.json()` encodes to **`str`**, which Layer B rejects ([0005](decisions/0005-large-object-io.md) §2), + and `value_codec=` does not compose via `Pipe` (`'Pipe' object has no attribute 'decoder'`). + A conformance assertion checks that every shipped recipe's encoder output is a member of + `BytesSource`. + --- ## Module layout @@ -152,15 +188,22 @@ s3dol/ values.py Filepath / Chunks / Streamable refs; as_fileobj singledispatch writes.py write strategies (simple / transfer / multipart) reads.py read strategies (bytes / stream / ranged / to-file) - base.py Layer B - stores.py Layer C + Layer D codec facades - recipes.py s3_store(...) and friends + base.py Layer B (owns prefix) + recipes.py Layer C — s3_store(...), codec facades + diagnose.py s3dol.diagnose() — prints resolved endpoint/region/credential SOURCE store.py COMPAT SHIM — legacy S3Store, DeprecationWarning, removed in v2 testing.py in-memory fake + the exported conformance suite tests/ util.py KEEP — py2store imports two functions from here ``` +`diagnose.py` is small but not optional: it is the step-0 safety mechanism for a change that +can silently move a live data target ([ADR-0007](decisions/0007-naming-and-compatibility.md) §5). + +**Public API** — `s3_store`, `BucketStore`, `BucketReader`, `EndpointStore`, `ObjectHandle`, +`S3Connection`, `Filepath`/`Chunks`/`Streamable`, the error classes, `diagnose`. Everything +else is implementation. + `store.py` must survive as an importable module: both external dependents do `from s3dol.store import S3Store`, not `from s3dol import S3Store`. @@ -172,18 +215,32 @@ s3dol/ `SupabaseS3BucketDol` is the anti-pattern; its behaviour becomes client configuration ([0003](decisions/0003-provider-presets-and-capabilities.md) §Supabase). - **`type(self)(**self.__dict__)` for sub-stores.** Fragile the moment any attribute isn't - an `__init__` arg. Sub-stores come from `dol`. -- **Probe-then-act.** No `head_bucket` before a write. Bucket policy is decided once, at - construction. -- **Cascading deletes as a side effect.** `del buckets[name]` refuses a non-empty bucket; - `buckets.delete(name, force=True)` is the explicit form. + an `__init__` arg. Sub-stores come from an explicit `self._with(prefix=…)` that names the + fields it carries (`azuredol/base.py:125` is the reference). +- **Probe-then-act.** No `head_bucket` before a write. Bucket policy is a construction-time + *parameter*; only `on_missing_bucket='raise'` performs I/O, and it says so + ([0010](decisions/0010-bucket-and-bulk-operations.md)). +- **Cascading deletes as a side effect.** `del endpoint[name]` refuses a non-empty bucket; + `endpoint.delete(name, force=True)` is the explicit form. - **`__len__` on a bucket store.** Unbounded pagination cost. - **Silent empties.** Anywhere. +- **Passing `EncodingType` to a list call.** botocore sets it itself *and* decodes the + response — but only when it set it. Passing it explicitly disables the decoder and returns + percent-encoded keys that no longer address their objects + ([0006](decisions/0006-key-scoping-and-dol-fixes.md) §4). ## Prior art -`azuredol` went through this refactor first; its -[design_decisions.md](https://github.com/i2mint/azuredol) is the direct ancestor of this -layout, and several of its sections cite s3dol v0 as the pattern being rejected. Where the -two packages face the same question, **we deliberately give the same answer** — the -`*dol` family's value is that one adapter reads like the next. +`azuredol` went through this refactor first, and where the two packages face the same +question **we deliberately give the same answer** — the `*dol` family's value is that one +adapter reads like the next. Concretely inherited: the layering, the Collection→Reader→Store +triads, the single error-translation decorator, no `__len__`, no global client cache, real +reader classes, and the refusal to cascade-delete a container (its `design_decisions.md` §12 +cites s3dol v0 by name: *"This is convenient and dangerous. We refuse it."*). + +**Read `azuredol`'s code, not only its docs.** Its `architecture.md` says container stores +are wrapped with `dol`'s `mk_relative_path_store`; its actual `base.py` does prefix +arithmetic in the leaf and uses `mk_relative_path_store` **zero times**. An earlier draft of +this document adopted the documented design rather than the shipped one, and that error is +the origin of the rewrite recorded in +[ADR-0001](decisions/0001-layered-architecture.md) §"Why the prefix lives in the leaf". diff --git a/misc/docs/decisions/0001-layered-architecture.md b/misc/docs/decisions/0001-layered-architecture.md index 577533d..78c29d4 100644 --- a/misc/docs/decisions/0001-layered-architecture.md +++ b/misc/docs/decisions/0001-layered-architecture.md @@ -22,56 +22,101 @@ delete (*"This is convenient and dangerous. We refuse it."*). ## Decision -Adopt `azuredol`'s layering, in S3 vocabulary, with one addition. +Adopt `azuredol`'s layering — as its **code** implements it — in S3 vocabulary. ``` -D recipes factories + codec stacks, by composition only -C stores relative keys / prefix scoping, delegated to dol -B base close-to-metal, ABSOLUTE keys, bytes<->bytes, one error seam +C recipes factories + codec stacks, by composition only +B base close-to-metal; owns a normalized `prefix`; one error seam A connection the credential + endpoint SSOT; the DI seam ``` -The addition is the **A/B split being strict about key space**: Layer B addresses the -bucket's real keyspace and knows nothing about prefixes. This is not tidiness — it is what -makes `url_for`, `info` and the transfer strategies correct by construction, because they -operate on the key S3 actually sees rather than a user-facing alias. v0's `url_for` had to -re-apply `_id_of_key` by hand (`base.py:222-225`) precisely because that split didn't exist, -and any future method would have had to remember to do the same. +**Layer B owns the prefix.** A bucket store carries a normalized `prefix`; +`_id_of_key`/`_key_of_id` live in the leaf; the prefix is pushed into +`ListObjectsV2(Prefix=…)`; it shows in `__repr__`; sub-stores come from an explicit +`self._with(prefix=…)`. `dol` is used for **codecs, filtering and caching — not prefix +arithmetic**. -**Layer D never subclasses.** If a recipe can't be built from Layer C + `dol` wrappers, that +**Layer C never subclasses.** If a recipe can't be built from Layer B + `dol` wrappers, that is evidence the capability belongs in Layer B as a *parameter*. This is the rule that stops per-vendor classes from reappearing. Class triads mirror `dol.filesys`: ``` -BucketCollection -> BucketReader -> BucketStore (keys: object keys) -BucketsCollection -> BucketsReader -> Buckets (keys: bucket names) -ObjectHandle (not a Mapping) +BucketCollection -> BucketReader -> BucketStore (keys: object keys) +EndpointCollection -> EndpointReader -> EndpointStore (keys: bucket names) +ObjectHandle (not a Mapping) ``` +## Why the prefix lives in the leaf + +An earlier draft of this ADR did the opposite: Layer B was absolute-keyed and knew nothing +about prefixes, with a separate Layer C applying prefix scoping through a `dol` wrapper. The +reasoning was that a leaf addressing the bucket's real keyspace makes `url_for` and friends +correct by construction. **That draft was wrong, and it was wrong in a way worth recording, +because the mistake is attractive.** + +Two mechanical facts kill it. + +**1. `dol` delegates unknown attributes with the outer, unmapped key.** +`dol.base.Store.__getattr__` returns the *bound leaf method*, so every non-dunder method the +package adds receives the user-facing key, not the mapped one. Verified against dol 0.3.58, +with a store scoped to `logs/` in a bucket that also holds `logs2/leak` and a root `a.txt`: + +| method | result through the wrap | consequence | +|---|---|---| +| `url_for('a.txt')` | `https://…/a.txt` | signs a URL for the **wrong object** | +| `sub('x/')` | store over the bucket **root** `x/` | scope escape | +| `handle('a.txt')` | handle on root `a.txt` | wrong object | +| `info('a.txt')` | `KeyError` for a key that is present | manufactured "absent" | +| `prefixes()` | `['logs/', 'logs2/']` | leaks the sibling prefix the scope exists to hide | +| `delete_many(['a.txt'])` | **root `a.txt` destroyed**, `logs/a.txt` untouched | silent destruction of the wrong object | + +`len()` also comes back from the dead: `dol.trans._filt_iter` assigns `store_cls.__len__` +**unconditionally, with no `hasattr` guard**, so the mandatory prefix filter undoes +[ADR-0008](0008-testing-architecture.md)'s deliberate omission of `__len__` *and* restores +the double-listing it diagnoses. + +**2. Prefix pushdown is unimplementable across that seam.** `Store.__iter__` calls +`self.store.__iter__()`. There is no channel by which a key-wrapper hands its prefix to the +leaf's `ListObjectsV2(Prefix=…)`. Every prefix-scoped listing becomes a full-bucket scan — +measured at 22 LIST requests where a leaf-owned prefix costs 1. + +**And the prior art cited for the draft was misread.** `azuredol`'s `architecture.md` says +container stores are wrapped with `mk_relative_path_store(prefix_attr='prefix')`. Its +**code** does no such thing: `base.py:88` normalizes the prefix in the leaf, `:100-103` +define `_id_of_key`/`_key_of_id` there, `:147`/`:171` push it into +`list_blobs(name_starts_with=…)`, `:109` puts it in `__repr__`, and `mk_relative_path_store` +appears **zero times** in the package. azuredol has none of the six bugs above precisely +because it does the thing this ADR now specifies. The lesson generalizes: **a sibling's +design doc is a claim; its source is the evidence.** + Reader-only classes are **real classes**, not instances with methods deleted. `dol`'s -`mk_read_only` works by assigning `__delitem__`/`__setitem__` onto the object, which -`type(store).__setitem__(store, k, v)` bypasses and which static analysis cannot see. Real -classes let a type checker catch `reader[k] = v`, and let an anonymous or read-scoped -credential refuse to even attempt a write. +`mk_read_only` is not merely bypassable — it is **non-functional on a `dol` store**: verified, +`ro['a'] = b'2'` on `mk_read_only(Store({...}))` succeeds silently, no exception. Real classes +let a type checker catch `reader[k] = v`, and let an anonymous or read-scoped credential +refuse to even attempt a write. ## Consequences -**Buys.** One place to look for credential behaviour. Sub-stores and codecs for free from -`dol`. A capability added at Layer B is automatically available through every Layer C/D -composition. A reader of `azuredol` can read `s3dol` — the family's main value. +**Buys.** One place to look for credential behaviour. Every capability method is key-correct +by construction, because there is no key-mapping seam between the method and the wire. Prefix +pushdown to `ListObjectsV2` is available. `__len__` stays absent. A reader of `azuredol` can +read `s3dol` — the family's main value. + +**Costs.** s3dol owns prefix arithmetic rather than inheriting it, which means it owns the +correctness of that arithmetic — including the normalization rule that +[ADR-0006](0006-key-scoping-and-dol-fixes.md) §1 specifies, and a round-trip property test to +keep it honest. That is a real cost and it is the one v0 paid badly. We accept it because the +alternative is not "`dol` does it correctly for us" — it is "`dol` does it incorrectly for us, +across six methods, silently". -**Costs.** More modules (7 vs 3) for a package this size, and one genuinely awkward -consequence: because Layer B is absolute-keyed and Layer C is a `dol` wrapper, a method -added to Layer B is **not automatically key-correct** when reached through Layer C. That is -the `url_for` delegation bug in [ADR-0006](0006-key-scoping-and-dol-fixes.md), and it is the -price of delegating prefixing to `dol` rather than owning it. We pay it because owning it is -what produced v0's bugs, and because the fix is upstreamable. +Users who *additionally* want a `dol` prefix codec on top are not prevented; they inherit the +delegation trap, which is why [ADR-0006](0006-key-scoping-and-dol-fixes.md) §2 documents it. **What NOT to do.** -1. Do not add a method to Layer B without deciding how it behaves through a Layer C wrap. - Every such method needs a key-mapping test. +1. Do not move prefix handling out of the leaf. Re-read §"Why the prefix lives in the leaf". 2. Do not put provider knowledge anywhere but `presets.py`. -3. Do not let Layer D grow a class statement. +3. Do not let Layer C grow a class statement. +4. Do not treat a sibling package's design doc as evidence about its behaviour. Read its code. diff --git a/misc/docs/decisions/0002-boto3-as-engine.md b/misc/docs/decisions/0002-boto3-as-engine.md index 49349d1..ac06dfd 100644 --- a/misc/docs/decisions/0002-boto3-as-engine.md +++ b/misc/docs/decisions/0002-boto3-as-engine.md @@ -7,7 +7,8 @@ The brief asked whether s3dol should keep `boto3` or move to something lighter — `minio`, `s3fs`, `smart_open`, `aioboto3`, or `obstore` (Rust `object_store` bindings). The measured -case against boto3 is real: `import s3dol` costs **172 ms**, of which **boto3 is 134 ms**. +case against boto3 is real: `import s3dol` costs **~167 ms** cumulative — boto3 ~141 ms and +`dol` ~61 ms (they overlap in shared stdlib), s3dol's own code ~2 ms. One research pass recommended making `obstore` the default engine. ## Decision @@ -18,7 +19,7 @@ deferred behind a narrow protocol. ### Why 1. **The alternatives cannot back the whole surface.** `obstore` has no bucket - create/delete/list operations at all, so the bucket level (`Buckets`, keys = bucket + create/delete/list operations at all, so the bucket level (`EndpointStore`, keys = bucket names) cannot be implemented on it. A "default engine" that can't serve a documented layer isn't a default. 2. **Everything portability-related is botocore-shaped.** The checksum fix that makes @@ -37,8 +38,15 @@ deferred behind a narrow protocol. boto3 is imported **lazily**: `from __future__ import annotations`, `if TYPE_CHECKING:` for types, and the client as a `functools.cached_property` on `S3Connection`. Constructing a -store performs no import of boto3 and no I/O. Budget: **`import s3dol` < 30 ms**, enforced -by a test. +store performs no import of boto3 and no I/O. + +Budget: **`import s3dol` adds < 10 ms on top of `import dol`**, enforced by a test that +measures the *delta*. An absolute budget is not achievable: measured cumulative import cost is +`dol` **61 ms**, boto3 141 ms, `s3dol` 167 ms — `dol` alone is twice any sub-30 ms target, and +it is a hard dependency of Layer B (which subclasses `dol.base.KvReader`) and of the +module-scope `wrap_kvs` recipes. Deferring `dol` too would mean making `S3Jsons`/`S3Texts` +`__getattr__`-lazy, which is not worth it. dol's own import cost is raised as an upstream +issue — it benefits every `*dol` package. This is strictly better than switching engines, because it also gives us the lazy/picklable connection that [ADR-0003](0003-provider-presets-and-capabilities.md) diff --git a/misc/docs/decisions/0003-provider-presets-and-capabilities.md b/misc/docs/decisions/0003-provider-presets-and-capabilities.md index b2da1f3..72c9f5c 100644 --- a/misc/docs/decisions/0003-provider-presets-and-capabilities.md +++ b/misc/docs/decisions/0003-provider-presets-and-capabilities.md @@ -32,12 +32,27 @@ class Preset: checksum: str = 'when_supported' # 'when_supported' | 'when_required' payload_signing_enabled: bool | None = None capabilities: Capabilities = DEFAULT_CAPABILITIES - client_kwargs: Mapping = MappingProxyType({}) + client_kwargs: tuple[tuple[str, Any], ...] = () # hashable: Preset must key a cache + # presign-specific overrides; default to the API values above + presign_endpoint_url: str | None = None + presign_addressing_style: str | None = None + verified: bool = False # against a live endpoint? with a date in the row ``` Adding a provider is adding a row. Open-closed. Users register their own: `s3dol.presets.register(Preset(name='mycorp', ...))`. +`client_kwargs` is a hashable tuple, not a `Mapping`: a frozen dataclass holding a +`MappingProxyType` is unhashable, so it could not key an `lru_cache` or be compared in +`diagnose()`. + +**Presigning needs its own config slots** because for two providers it genuinely differs from +the API config. Hetzner configures **path** style for normal use and documents *"uncomment +before you create presigned URLs"* for virtual — virtual-hosted style breaks ordinary +Get/Put there because the TLS certificate doesn't cover bucket subdomains. R2 requires +presigning against the S3 API domain even when objects are served from a custom domain. +`url_for` therefore builds and caches a **second client** from the merged presign config. + The registry is the SSOT for a set of facts nobody should have to rediscover: | Provider | endpoint | region | addressing | notes | @@ -46,7 +61,7 @@ The registry is the SSOT for a set of facts nobody should have to rediscover: | minio | `http://{host}:{port}` | `us-east-1` conventional | **path** unless wildcard DNS | | | r2 | `https://{account_id}.r2.cloudflarestorage.com` | **`auto`** | virtual | presign only on the S3 API domain, never a custom domain | | scaleway | `https://s3.{region}.scw.cloud` | same string | virtual | multipart capped at **1000 parts** | -| hetzner | `https://{loc}.your-objectstorage.com` | **must repeat `{loc}`** | virtual | needs `payload_signing_enabled=False` | +| hetzner | `https://{loc}.your-objectstorage.com` | **must repeat `{loc}`** | **path** (presign: virtual) | needs `payload_signing_enabled=False` | | backblaze | `https://s3.{region}.backblazeb2.com` | same string | virtual | checksum `when_required` **mandatory** | | wasabi | `https://s3.{region}.wasabisys.com` | region string | path (vendor's own advice) | `GetBucketLocation` always says `us-east` | | gcs | `https://storage.googleapis.com` | ignored | virtual (path for dotted buckets) | **no ListObjectsV2**, no batch delete | @@ -131,7 +146,51 @@ Two caveats we must not forget: that capability exists. 2. **Fixing this does not un-corrupt already-written objects.** See Consequences. -### 4. Per-vendor classes are deleted +### 4. `signature_version` is never left to botocore — this is the single most load-bearing default in the package + +botocore silently **downgrades presigning to SigV2** whenever the user has not *explicitly* +set `signature_version` (`botocore/client.py::_set_s3_presign_signature_version`). Measured: + +``` +no Config -> SigV2 (meta.config.signature_version reports 's3v4') +Config(s3={'addressing_style':'path'}) -> SigV2 (a Config that omits signature_version does NOT help) +Config(signature_version='s3v4') -> SigV4 +custom endpoint_url -> SigV2 <- every MinIO / LocalStack / moto user +us-east-1 -> SigV2 +eu-central-1 -> SigV4 +``` + +AWS rejects SigV2 on any bucket created after 2020-06-24; R2, Backblaze, Scaleway and modern +MinIO reject it outright. And `client.meta.config.signature_version` **reports `'s3v4'` while +producing SigV2**, so introspection cannot catch it. + +`S3Connection` therefore always constructs a `botocore.Config` with an explicit +`signature_version` (default `'s3v4'`), **including when `preset is None`**. + +This is a live bug in v0, not merely a design gap: `s3dol` today presigns with SigV2 for +`us-east-1` and for every custom endpoint. `test_url_for.py` cannot see it because it asserts +`"Signature" in url` — and a SigV2 URL contains `Signature=` while SigV4 contains +`X-Amz-Signature=`. The tier-1 assertion is therefore +`'X-Amz-Algorithm=AWS4-HMAC-SHA256' in url and 'AWSAccessKeyId' not in url`. + +### 5. Anonymous access + +`anon` is `bool | 'auto'` on the connection **and surfaced directly on `s3_store(...)`** — +reading a public bucket must not require learning Layer A. It is translated to +`botocore.UNSIGNED` *inside* the `cached_property` that builds the client: the singleton is +**unpicklable**, so it must never enter the dataclass or every anonymous store would fail +[ADR-0008](0008-testing-architecture.md)'s pickle conformance. + +`url_for` raises `NotSupported('url_for', reason='anonymous credentials cannot sign a URL')` +when the resolved signer is `UNSIGNED` — botocore otherwise returns a plain unsigned URL with +no error, which is a wrong answer under the never-silently-wrong goal. + +`'auto'` means *"try unsigned if no credentials resolve at all"*. It explicitly does **not** +mean "retry unsigned after `AccessDenied`": an expired token would then silently downgrade to +a different, public view of the data. It warns, naming what it did. Requester-pays buckets +forbid anonymous access outright, so a silent fallback would confuse there regardless. + +### 6. Per-vendor classes are deleted `SupabaseS3BucketDol` → `preset='supabase'`. `S3BucketDolWithouBucketCheck` → the default (no probe-then-act, see [ADR-0001](0001-layered-architecture.md)). Provider detection from diff --git a/misc/docs/decisions/0004-error-taxonomy.md b/misc/docs/decisions/0004-error-taxonomy.md index 46a0ea8..cf50613 100644 --- a/misc/docs/decisions/0004-error-taxonomy.md +++ b/misc/docs/decisions/0004-error-taxonomy.md @@ -48,15 +48,32 @@ and it is what makes the auth-vs-not-found distinction auditable from one file. permission must not look like a missing key — that is exactly the confusion that makes `_bucket_exists` dangerous today. -### 2. Classification is a table keyed on `(code, status)`, not on exception type +### 2. Classification is a table keyed on `(operation, code, status)`, not on exception type Because the modelled exceptions are unreliable (above), classification reads `ClientError.response['Error']['Code']` and the HTTP status, through two tiny predicates (`code_of`, `status_of`). The table is per-provider-overridable, which is how Supabase's 400-means-404 is handled without any code branch. +**The operation is part of the key because HEAD has no body.** botocore synthesizes the code +from the HTTP status, so on real AWS `HeadObject` against a missing *key* and against a +missing *bucket* are **both** `('404', 404)` — indistinguishable. AWS documents that the exact +error is not retrievable for HEAD. + +This ambiguity must not be resolved by guessing, and moto will not warn you: moto returns an +error **body** on HEAD where AWS returns none, so `head_object` against a missing bucket gives +`Code='NoSuchBucket'` under moto and `Code='404'` in production. `'k' in store` against a +typo'd bucket is therefore green in tiers 1–2 and silently **`False`** in production — the +exact silent-empty this package bans, produced by the taxonomy itself. + +Resolution: on `('HeadObject', '404', 404)`, `__contains__` returns `False` only if the +connection has already proven the bucket reachable this session; otherwise it performs one +`HeadBucket` disambiguation, cached on the connection. Add `405` (delete-marker HEAD on a +versioned bucket) and `301` (wrong region) to the table. + Testable without a network: the classifier takes a synthesized `ClientError`, so the bulk of -error tests are pure unit tests. +error tests are pure unit tests — including body-less HEAD errors, **which moto cannot +produce**. ### 3. The taxonomy @@ -64,26 +81,49 @@ error tests are pure unit tests. |---|---|---| | object absent | `ObjectNotFound(KeyError)` | Mapping contract | | bucket absent (object op) | `BucketNotFound(KeyError)` | still a key-space problem for the caller | -| bucket absent (bucket op) | `BucketNotFound(KeyError)` | key of the `Buckets` mapping | +| bucket absent (bucket op) | `BucketNotFound(KeyError)` | key of the `EndpointStore` mapping | | key syntactically invalid | `KeyNotValid(KeyError, ValueError)` | both, deliberately — see §5 | | object archived (Glacier) | `ObjectArchived(KeyError)` | see §4 | -| permission denied | `AccessDenied(S3Error)` — **not** a `KeyError` | | +| permission denied | `AccessDenied(S3Error)` — **not** a `KeyError` | but see below | | credentials missing/expired | `CredentialsError(S3Error)` | | | operation unsupported by provider | `NotSupported(S3Error)` | names provider + operation | | transient / throttled | propagate (botocore retries) | | Everything derives from `S3Error(Exception)` so `except S3Error` catches the package. +**403-means-absent is a real, common ambiguity and needs a stated policy.** AWS's +`HeadObject` docs: *"If you have `s3:ListBucket` … 404. If you don't have `s3:ListBucket`, +Amazon S3 returns 403 Forbidden."* The canonical least-privilege policy grants +`s3:GetObject`/`s3:PutObject` on `bucket/*` and omits `s3:ListBucket` on `bucket` — so under +the **most common production IAM policy, every miss is a 403**, which is not a `KeyError`, +which means `store.get(k, default)` and `k in store` *raise* and a cache-lookup service 500s +instead of taking the miss branch. + +Default: raise `AccessDenied`, with a message naming `s3:ListBucket` as the likely cause. +`S3Connection(deny_means_absent=True)` opts into classifying it as `ObjectNotFound` for +deployments that knowingly run that policy. This is a decision, not an omission. + ### 4. `ObjectArchived` is a `KeyError`, deliberately and arguably A Glacier object exists but `GetObject` returns `InvalidObjectState` / 403. `k in store` must stay `True` — the key *does* exist, and any other answer breaks generic algorithms. But `store[k]` must fail, and it must fail as a `KeyError` so that `store.get(k, default)` and -`dict(store)`-shaped code degrade to the not-available branch rather than exploding. - -So it is a `KeyError` that carries `.storage_class`, `.restore_status` and `.restore(days, -tier)`. This is the one place we knowingly let a `KeyError` mean something other than -"absent", and it is recorded here as a considered choice rather than an accident. +`store.pop(k, default)` degrade to the not-available branch rather than exploding. + +Be precise about how far that degradation goes, because the obvious claim is wrong: +`dict(store)`, `store.items()` and `Mapping.__eq__` still **raise** — only the defaulted +accessors degrade. Verified. + +And one genuinely dangerous interaction: `MutableMapping.setdefault` is +`try: self[k] except KeyError: self[k] = default`, so a `KeyError` here lets `setdefault` +**overwrite the archived object with the default** — verified, silently, no exception. +`BucketStore` therefore overrides `setdefault` and `pop(k, default)` to re-raise +`ObjectArchived` rather than swallow it, and the conformance suite asserts that `setdefault` +on an archived key does not mutate the object. + +So it is a `KeyError` carrying `.storage_class`, `.restore_status` and `.restore(days, tier)`. +This is the one place we knowingly let a `KeyError` mean something other than "absent", and it +is recorded here as a considered choice rather than an accident. ### 5. Two collisions to avoid diff --git a/misc/docs/decisions/0005-large-object-io.md b/misc/docs/decisions/0005-large-object-io.md index bf7b70e..c2f6a1f 100644 --- a/misc/docs/decisions/0005-large-object-io.md +++ b/misc/docs/decisions/0005-large-object-io.md @@ -14,8 +14,21 @@ different from what `s[k]` returns? A tempting escape is to split the store: a write-only multipart store that is Iterable + Settable + Deletable but not Gettable, paired with a separate reader. -The hard constraint from [ADR-0001](0001-layered-architecture.md): **base interfaces stay -pure.** No `s.upload_multipart(...)`. Infra capability must be reachable *through* `s[k] = v`. +The hard constraint: infra capability must be reachable *through* `s[k] = v`, not by bolting +`s.upload_multipart(...)` onto a Mapping. + +State that as a rule the next contributor can actually apply, because "keep the interface +pure" was being claimed while six methods were added: + +> **The Mapping protocol is closed.** A method may be added iff it takes a key and is either +> a pure read of metadata or an address (`info`, `url_for`), or returns a store or handle +> (`sub`, `handle`, `prefixes`). Anything that mutates, batches, or takes non-key arguments +> belongs on `ObjectHandle` or a recipe. `delete_many` is an explicit, named exception, +> admitted only because the cost difference against a `__delitem__` loop is an order of +> magnitude. + +For calibration: `azuredol` has exactly one such method (`walk`) and pushes everything +per-object onto `BlobHandle`. Six unexplained exceptions is how a surface grows to twenty. ## Decision @@ -29,6 +42,15 @@ BytesSource = bytes | bytearray | BinaryIO | Filepath | Chunks | Streamable *refs*, not values. Dispatch is `functools.singledispatch`, so the union is open for extension (users register their own ref types) and closed for modification. +**Register on `io.IOBase`, never `typing.BinaryIO`.** `@register(BinaryIO)` is accepted at +definition time and then **never fires** — `io.BytesIO` is not in `typing.BinaryIO`'s MRO, and +`isinstance(io.BytesIO(), typing.BinaryIO)` is `False`. So the second-most obvious thing a +user types, `s['big.mp4'] = open('big.mp4', 'rb')`, would raise `TypeError`. `io.IOBase` +covers `BytesIO`, `BufferedReader`, `botocore.response.StreamingBody` (which the store-to-store +streaming copy depends on), `SpooledTemporaryFile` and urllib3 responses. `BinaryIO` stays in +the *static* union for mypy only. `memoryview` and other buffer-protocol objects are also +accepted — `dol.Files` takes them, and the precedent table below credits it for that. + ### 2. `str` is rejected, loudly `s['config'] = '{"a": 1}'` and `s['video'] = '/tmp/big.mp4'` are both overwhelmingly @@ -70,16 +92,27 @@ value is a reference to content elsewhere"*, assigned through an ordinary The law that makes it safe — three conditions, all necessary: -> **N1 Canonical form.** There is a total `normalize: WriteDomain → bytes`, identity on `bytes`. -> **N2 Stability.** Therefore `s[k] = s[k]` is a no-op and `dst.update(src)` terminates. -> **N3 The honest invariant.** Not `s[k] = v ⟹ s[k] == v`, but **`s[k] = v ⟹ s[k] == normalize(v)`**. - -Rejecting `str` is exactly what keeps `normalize` a *function* — with `str` admitted it would -have two candidate results, and N1 would fail. The rejection isn't fussiness; it's what makes -the rest sound. - -The residual cost is real and small: `setdefault` becomes type-unstable, `pop`/`popitem` -become expensive. Documented, not removed. +> **N1 Canonical form.** `normalize: WriteDomain → bytes` is **total on +> `bytes | bytearray | Filepath`** and **one-shot on `BinaryIO | Chunks | Streamable`** — a +> stream ref is consumed by its first write; assigning the same ref twice is a documented +> error, not a second copy. +> **N2 Stability.** For the re-readable half, `s[k] = s[k]` is a no-op and `dst.update(src)` +> terminates. +> **N3 The honest invariant.** Not `s[k] = v ⟹ s[k] == v`, but +> **`s[k] = v ⟹ s[k] == normalize(v)`** — for the re-readable half. + +Note what this correction costs the argument: `str` was **not** the only thing breaking N1. +The single-consumption stream refs break it too (`s['a'] = f; s['b'] = f` yields `b'payload'` +then `b''`). So "rejecting `str` is what keeps `normalize` a function" is a non-sequitur, and +is struck. The `str` rejection stands on the **decidability** argument in §2 alone, which is +sufficient. + +The residual costs, stated honestly: `setdefault` becomes type-unstable, `pop`/`popitem` +become expensive, and — the larger loss — **refs only type-check against the concrete class**. +Through `MutableMapping[str, bytes]`, which is how dependents actually annotate the store +(`lacing/artifact_store.py:120`), `s[k] = Filepath(...)`, `update()` and `setdefault()` all +fail a type checker, because `MutableMapping`'s value type is invariant. `BucketStore` +declares an explicit `update(self, other: Mapping[str, BytesSource]) -> None` override. ### 4. Do NOT split into a write-only store @@ -101,11 +134,16 @@ for Iterable+Settable+Deletable-but-not-Gettable, and cannot cheaply — `Mutabl inherits `__getitem__` as abstract from `Mapping`, and `pop`/`popitem`/`clear`/`setdefault` are all defined in terms of it; only `update` survives. `dol` has `mk_read_only` / `disable_setitem` / `disable_delitem` but **no `disable_getitem` and no `mk_write_only`** — -you can take writes away but not reads. We define the `Protocol`s anyway (~15 lines, they -document the shape and serve genuinely write-only sinks) and propose the missing dol -symmetry upstream. Note `@runtime_checkable` checks method *presence* only, so -`isinstance(d, WriteOnlyStore)` is `True` for a `dict`; "must not be gettable" needs an -explicit predicate. +you can take writes away but not reads. + +We **do not ship** `WriteOnlyStore` Protocols in v1. They would have zero implementers, which +violates [ADR-0009](0009-scope-and-deferrals.md)'s own "no new `Protocol` without two +implementers" rule, and they cannot express the constraint anyway: `@runtime_checkable` checks +method *presence* only, so `isinstance({}, WriteOnlyStore)` is `True` and "must not be +gettable" needs a separate predicate. (Relatedly: since Python 3.12 `isinstance` uses +`getattr_static`, which detects a **class**-wrapped capability but not an **instance**-wrapped +one — capability detection must never rely on it.) The shape is recorded here; the Protocols +and the `disable_getitem`/`mk_write_only` symmetry go on the dol upstream list. ### 5. How the capability reaches through the pure interface: injected strategies @@ -121,22 +159,32 @@ A strategy is a callable, injected at construction. This is the answer to "how d infra-specific optimization without polluting the interface": `__setitem__` stays `__setitem__`; *how* it uploads is a constructor parameter. -Note a **structural** reason this cannot be a `dol` value codec: a codec is a pure -`obj -> data` transformation applied by `Store.__setitem__` before the inner write. A -multipart upload needs the *key* and the *client*, and it is a side effect, not a -transformation. So the strategy must live in the leaf store, below `wrap_kvs`. +Why this cannot be a `dol` value codec — the accurate version, since the obvious reason is +wrong. It is *not* that a codec can't see the key and the store: `wrap_kvs(preset=…)` with the +`(self, k, v)` convention **is** handed both. The real reasons are that (a) `preset`'s return +value is still passed to the inner `__setitem__`, so it can transform the value but cannot +*replace* the write, and (b) inside it, `self` is the unwrapped leaf. So the strategy must +live in the leaf store, below `wrap_kvs`. Default: `transfer_writes` with boto3's threshold (8 MiB). Small writes take a single `PutObject`; large ones transparently go multipart. The overhead on small objects is one branch. +Every non-`bytes` source routes through `upload_fileobj`/`TransferManager`, never +`PutObject` — so seekability is s3transfer's problem, and the size threshold is never +consulted for a non-seekable source (where it is undecidable anyway). `io.UnsupportedOperation` +and `botocore.exceptions.UnseekableStreamError` are part of the error seam: a non-seekable +stream raises from `botocore/httpchecksum.py` *before the request is built*, so a +`translate_s3_errors` that only catches `ClientError` would never see it. + ### 6. The read side, symmetrically `s[k]` returns `bytes` — always, because N1 demands it. Streaming is reached three ways, in increasing explicitness: a `reads=stream_reads()` strategy at construction (the store's -values become chunk iterators — a *different store*, honestly typed); `store.handle(k)` for -`.open()` / `.stream()` / `.read(byte_range=...)`; or a `Filepath` destination for -download-to-disk. +values become chunk iterators — a **runtime** variation, *not* a static one: the class's value +type does not change, and honest typing would need `BucketStore(Generic[VT])` plus overloaded +construction, which v1 does not do); `store.handle(k)` for `.open()` / `.stream()` / +`.read(byte_range=...)`; or a `Filepath` destination for download-to-disk. `ObjectHandle` is not a Mapping and is the documented escape hatch — the same role `BlobHandle` plays in `azuredol`. diff --git a/misc/docs/decisions/0006-key-scoping-and-dol-fixes.md b/misc/docs/decisions/0006-key-scoping-and-dol-fixes.md index 2a407aa..f440f36 100644 --- a/misc/docs/decisions/0006-key-scoping-and-dol-fixes.md +++ b/misc/docs/decisions/0006-key-scoping-and-dol-fixes.md @@ -1,113 +1,120 @@ -# ADR-0006: Prefix scoping is delegated to `dol` — but only in its safe composition +# ADR-0006: Prefix normalization, key validity, and the `dol` traps to avoid - **Status:** Accepted - **Date:** 2026-08-10 -- **Severity:** This is the most important document in this set. Read it before writing any key-handling code. +- **Severity:** Read before writing any key-handling code. +- **Note:** An earlier revision of this ADR made prefix scoping a `dol` wrapper above an + absolute-keyed leaf. [ADR-0001](0001-layered-architecture.md) §"Why the prefix lives in the + leaf" records why that was reversed. What remains here is the arithmetic the leaf must get + right, plus the traps for anyone stacking `dol` codecs on top. ## Context -v0.1.x does prefix scoping by hand: +v0.1.x does prefix scoping by hand and unsafely: ```python def _key_of_id(self, id): - return id[len(self.prefix):] # base.py:201 + return id[len(self.prefix):] # base.py:201 — slices even when it doesn't match ``` -This is unguarded: if `id` doesn't start with `prefix`, it silently slices anyway. The -obvious fix — and the one every research pass recommended — is *"delete this and use `dol`'s -canonical mechanism, `mk_relative_path_store(prefix_attr='prefix')`"*. +The natural fix is "use `dol`'s canonical mechanism". **`dol`'s mechanism has the same bug.** -**That fix is wrong.** `dol`'s prefix machinery has the same bug. - -## The evidence - -Store `{'a/b': 1, 'a/c': 2, 'z': 3, 'ab/x': 4}`, prefix `'a/'`, run against dol 0.3.58: +Store `{'a/b': 1, 'a/c': 2, 'z': 3, 'ab/x': 4}`, prefix `'a/'`, dol 0.3.58: | Mechanism | keys produced | verdict | |---|---|---| | `KeyCodecs.prefixed('a/')` | `['', '/x', 'b', 'c']` | **CORRUPT** | | `prefixless_view(store, prefix='a/')` | `['', '/x', 'b', 'c']` | **CORRUPT** | -| `mk_relative_path_store(cls, prefix_attr='prefix')` | `['', '/x', 'b', 'c']` | **CORRUPT** ← the recommended replacement | -| `handle_prefixes(store, prefix='a/')` | `['b', 'c']` | safe (filters first) | -| `Pipe(filt_iter.prefixes('a/'), KeyCodecs.prefixed('a/'))` | `['b', 'c']`, `len == 2`, `'' in p → False` | **safe** | - -The non-matching key `z` becomes `''` (and `w['']` then raises `KeyError: 'a/'`), and the -*sibling* key `ab/x` becomes `/x`. +| `mk_relative_path_store(cls, prefix_attr='prefix')` | `['', '/x', 'b', 'c']` | **CORRUPT** | +| `Pipe(filt_iter.prefixes('a/'), KeyCodecs.prefixed('a/'))` | `['b', 'c']` | safe | -In S3 terms: a store scoped to `logs/` surfaces a neighbouring object `logs2/2026.txt` as a -plausible-looking, **writable** key `2/2026.txt`. Writing to it writes outside the store's -scope. For anyone using a prefix as a tenant or app boundary, that is a boundary violation -produced by the storage layer itself. +The non-matching key `z` becomes `''`; the *sibling* key `ab/x` becomes `/x`. A store scoped +to `logs/` surfaces a neighbouring `logs2/2026.txt` as a plausible, **writable** key +`2/2026.txt`. For anyone using a prefix as a tenant or app boundary, that is a boundary +violation produced by the storage layer. ## Decision -### 1. `filt_iter.prefixes(p)` below every relativization is MANDATORY +### 1. Normalize the prefix, then filter, then relativize — in that order + +**Normalization is not optional and comes first:** + +```python +prefix = f"{prefix.strip(delimiter)}{delimiter}" if prefix else "" +``` + +Without it, `prefix='logs'` (no trailing slash) exposes `logs2/2026.txt` as a readable **and +writable** key `2/2026.txt` — the same boundary violation this section exists to prevent, one +character away. Verified: + +```python +safe = Pipe(filt_iter.prefixes('logs'), KeyCodecs.prefixed('logs'))(store) +safe['2/2026.txt'] # -> 2 OTHER TENANT, READ +safe['2/hacked.txt'] = 99 # -> writes 'logs2/hacked.txt' OTHER TENANT, WRITE +``` + +Both v0 (`base.py:100-105`) and `azuredol` (`base.py:88`) normalize. A prefix that does not +terminate in the delimiter is normalized, never accepted as-is. + +**In the leaf**, `_id_of_key`/`_key_of_id` operate on the normalized prefix, `__iter__` +passes `Prefix=self.prefix` to `ListObjectsV2`, and `_key_of_id` **raises** rather than +slicing a non-matching id. The server-side `Prefix` makes out-of-scope keys unreachable in +the common path; the raising `_key_of_id` is the belt to that suspenders, because a provider +that ignores `Prefix` must not silently produce corrupt keys. -It is a **correctness requirement, not an optimization**. The only sanctioned composition: +**If you additionally stack a `dol` prefix codec**, the only safe composition is: ```python -relative = Pipe( - filt_iter.prefixes(prefix), # filter FIRST — not optional - KeyCodecs.prefixed(prefix), # then relativize -) +Pipe(filt_iter.prefixes(prefix), KeyCodecs.prefixed(prefix)) # filter FIRST ``` Bare `mk_relative_path_store` / `KeyCodecs.prefixed` / `prefixless_view` are **banned in -s3dol**, and the ban is enforced by a test in the conformance suite: a store containing -sibling and non-matching keys must expose exactly the in-scope ones, and round-trip them. +s3dol**. The order is not a style preference — reversed, the store is silently empty. -Where the prefix is also pushed down to `ListObjectsV2(Prefix=...)`, the client-side filter is -usually redundant — but "usually" is doing dangerous work there (a pushdown that silently -fails, a provider that ignores `Prefix`, a wrapper composed in a different order), so the -filter stays unconditionally. +Related upstream bug: `dol.trans.filter_prefixes(['logs/', 'tmp/'])` compiles to +`^logs/|tmp/` = `(^logs/)|(tmp/)`, so `zzz/tmp/c` matches. Multi-prefix scoping leaks. -### 2. `url_for` must reach the leaf with the fully-mapped key +### 2. The delegation trap (for anyone stacking `dol` on top) -Verified, and worse than the above because it is silent: +`dol` wrappers delegate unknown attributes to the leaf **with the outer, unmapped key**: ```python w = KeyCodecs.prefixed('a/')(WithUrl)(...) -w['b'] # -> 1 correct, prefix applied -w.url_for('b') # -> https://x/b WRONG: should be https://x/a/b -isinstance(w, SupportsUrlFor) # -> True the Protocol cannot detect this +w['b'] # -> 1 correct +w.url_for('b') # -> https://x/b WRONG: should be https://x/a/b ``` -`dol` wrappers delegate unknown attributes to the inner store **with the outer, unmapped -key**. So the moment prefixing moves into a `dol` wrap, every presigned URL points at the -wrong object — and nothing fails. The existing `test_url_for.py` asserts only substring -presence (`"test-bucket" in url`, `"Signature" in url`), so a URL for the wrong key **passes -today**. - -Interim mechanism: route `url_for` through `dol.dig.inner_most_key`. Permanent mechanism: the -dol fix below. +`isinstance(w, SupportsUrlFor)` stays `True` — a `@runtime_checkable` Protocol checks method +*presence* only, so it cannot detect this. (It is also wrap-dependent: since Python 3.12 +`isinstance` uses `getattr_static`, which sees a **class**-wrapped capability but not an +**instance**-wrapped one. Capability detection must not rely on it.) -Test requirement: parse the URL and assert the path equals the fully-prefixed key, and -actually fetch it against moto. Substring assertions are banned for this method. +The correct escape is **`inner_most_key(wrapped_self(self), k)`**. -### 3. Two fixes go upstream to `dol` first +> `inner_most_key(self, k)` — which an earlier revision of this ADR prescribed — returns +> **`None`**, silently: inside a delegated method `self` *is* the unwrapped leaf (dol issue +> #18), so the URL becomes `https://…/None` with no exception. Do not copy that form. -Per the owner's decision, these land in `dol` as its own reviewed change, and s3dol then -requires that version. Every other `*dol` adapter almost certainly has the same latent bugs, -so fixing them once in `dol` is worth more than fixing them once in s3dol. +`dol.wrapped_self` **already ships in dol 0.3.58**, so this is a floor bump, not an upstream +project. `url_for` must additionally **raise** when the mapped key is not a `str`, so the +`None` failure mode is impossible rather than merely documented. -**dol fix 1 — strict prefix relativization.** A `strict=True` mode (proposed default in a -future major) on the prefix machinery: keys outside the prefix must **raise**, never be -silently sliced. Plus a property test: +### 3. What still goes upstream to `dol` -``` -∀ k in-scope: key_of_id(id_of_key(k)) == k -∀ i out-of-scope: key_of_id(i) raises # never returns a corrupted key -``` +Reduced, now that prefixing lives in the leaf and `wrapped_self` turns out to exist: -**dol fix 2 — key-mapped delegation.** A mechanism so that delegated methods (`url_for`, -`info`, and anything a backend adds) receive the fully-mapped inner key. Without it, every -capability s3dol adds at Layer B is silently wrong through a Layer C wrap, and the package's -own layering becomes a trap. +1. **Strict prefix relativization** — a `strict=True` mode where keys outside the prefix + raise rather than being silently sliced, plus the property test + (`∀ k in-scope: key_of_id(id_of_key(k)) == k`; `∀ i out-of-scope: key_of_id(i)` raises). + Every `*dol` adapter that uses this machinery has the latent bug. +2. **`filter_prefixes` regex grouping** — `^logs/|tmp/` must be `^(?:logs/|tmp/)`. +3. **`_filt_iter` assigning `__len__` unconditionally** — it should not resurrect a `__len__` + the wrapped class deliberately omits. +4. **Document `wrapped_self` as the delegation answer**, and make the family use it. -Until both land, s3dol uses the safe local composition and `inner_most_key`, with `# TODO: -upstream to dol (dol#NN)` at each site and a linked issue. **Policy on upstreams:** never -block an s3dol release on a dol PR; never let a local copy diverge silently — raise the dol -floor the day each fix lands and delete the workaround in the same commit. +**Policy on upstreams:** never block an s3dol release on a `dol` PR; never let a local copy +diverge silently — raise the `dol` floor the day each lands and delete the workaround in the +same commit. ### 4. Key validity is checked before the wire @@ -115,32 +122,56 @@ Probed behaviours that currently leak backend types through the Mapping: | key | today | |---|---| -| `''` | `ParamValidationError` — a botocore type escaping through `__getitem__` | -| `'folder/'` | returns a **sub-store**, not bytes; absent from `list(s)`; `in` says `True` — the object is permanently unreadable through the interface | +| `''` | `ParamValidationError` — a botocore type escaping `__getitem__` | +| `'folder/'` | returns a **sub-store**, not bytes; absent from `list(s)`; `in` says `True` — permanently unreadable through the interface | | `'bad\ud800key'` | `UnicodeEncodeError` | | 1025-char key | fine on moto, `KeyTooLongError` on AWS | -Decisions: normalize all of these to `KeyNotValid` before the request; enforce the -1024-UTF-8-**byte** limit client-side so moto and AWS agree; and set `EncodingType='url'` by -default (per-preset opt-out — GCS rejects it) so keys containing control characters survive -the XML listing. +Decisions: normalize all of these to `KeyNotValid` before the request, and enforce the +1024-UTF-8-**byte** limit client-side so moto and AWS agree. + +**Never pass `EncodingType`.** botocore sets it on every `ListObjects*` *and* URL-decodes the +response — but the decode is gated on a flag it sets only when the caller did **not** pass the +parameter (`botocore/handlers.py`). Passing it explicitly disables botocore's decoder and +returns percent-encoded keys that no longer address their objects. Verified on moto with the +keys `plain, 'a b', café, 'a+b', 'a\rb', 'p/x y', 'a%20b'`: -The trailing-`/` overload is removed: **`store[k]` always returns bytes**. Sub-stores come +``` +DEFAULT (not passed) -> 7/7 round-trip +EXPLICIT EncodingType='url' -> 2/7 round-trip + ['a%0Db','a%20b','a%2520b','a%2Bb','caf%C3%A9','p/x%20y','plain'] +``` + +An earlier revision of this ADR mandated `EncodingType='url'` "so keys with control characters +survive the XML listing". That decision **caused** the corruption it claimed to prevent, and +broke [ADR-0008](0008-testing-architecture.md)'s conformance law `all(k in s for k in s)`. Its +stated per-preset opt-out was also unimplementable: not passing the parameter doesn't remove +it, since botocore adds it. If a provider rejects botocore's auto-`EncodingType`, the preset +expresses that by unregistering botocore's handler pair — and s3dol then owns the decode. + +**The trailing-`/` overload is removed**: `store[k]` always returns bytes; sub-stores come from `store.sub('folder/')`. `store['folder/']` survives only on an explicitly-constructed -navigable reader for notebook use. This is what makes empty-directory markers (which a -filesystem migration creates) addressable at all. +navigable reader for notebook use (`azuredol` keeps both, and so do we). + +One consequence to handle explicitly: a view scoped to `a/` relativizes the **exact-prefix +marker object** `a/` to the key `''`, which this section forbids. Resolution: the scoped view +filters out the exact-prefix marker, and `store.sub(p)` documents that the parent's own marker +is not a key of the child. The marker stays addressable by its absolute key on an unscoped +store. (Directory markers matter: a filesystem migration creates them for empty directories.) ## Consequences -**Buys.** Prefix scoping that is actually correct, and correct for every `*dol` adapter once -upstreamed. Presigned URLs that point at the right object. Sub-stores with zero round-trips -from `dol` rather than `type(self)(**self.__dict__)`. +**Buys.** Prefix scoping that is correct, cheap (server-side `Prefix`), and visible in +`__repr__`. Keys that round-trip. A short upstream list that benefits every `*dol` adapter. -**Costs.** A dependency on a `dol` release for the clean version, and an interim workaround -that must be deleted later — tracked, with the usual risk that it isn't. +**Costs.** s3dol owns the arithmetic and therefore owns a property test for it. The +exact-prefix-marker rule is a genuine wart — an object *is* hidden from its own scoped view — +justified only because the alternative is a key the store iterates but refuses to read. **What NOT to do.** -1. **Never use `mk_relative_path_store`, `KeyCodecs.prefixed` or `prefixless_view` bare.** -2. Never assert on a presigned URL by substring. -3. Never add a Layer B method without a Layer C key-mapping test. +1. Never accept an un-normalized prefix. +2. Never use `mk_relative_path_store` / `KeyCodecs.prefixed` / `prefixless_view` bare. +3. Never pass `EncodingType`. +4. Never assert on a presigned URL by substring ([ADR-0008](0008-testing-architecture.md)). +5. Never write `inner_most_key(self, k)` inside a delegated method. diff --git a/misc/docs/decisions/0007-naming-and-compatibility.md b/misc/docs/decisions/0007-naming-and-compatibility.md index 9ca433e..1c786d2 100644 --- a/misc/docs/decisions/0007-naming-and-compatibility.md +++ b/misc/docs/decisions/0007-naming-and-compatibility.md @@ -39,8 +39,8 @@ burn permanently. | `BaseS3BucketReader` | `BucketCollection` + `BucketReader` | splits two conflated responsibilities; mirrors `FileCollection`/`FileBytesReader` | | `BaseS3BucketDol` | `BucketStore` | "Dol" carries no meaning | | `S3BucketReader` / `S3BucketDol` | (Layer C) `BucketReader` / `BucketStore` with `prefix=` | one class, prefix is a parameter | -| `S3ClientReader` / `S3ClientDol` | `BucketsReader` / `Buckets` | the key *is* a bucket name; "Client" names the implementation, not the mapping | -| `S3Dol` | `S3Endpoints` | it maps endpoint/profile names → bucket stores | +| `S3ClientReader` / `S3ClientDol` | `EndpointReader` / `EndpointStore` | the key *is* a bucket name; "Client" names the implementation, not the mapping | +| `S3Dol` | `S3Profiles` | it maps AWS **profile** names → bucket collections | | `S3DolReadOnly` | *deleted* | use the `*Reader` classes | | `S3BucketDolWithouBucketCheck` | *deleted* | typo, and the behaviour is now the default | | `SupabaseS3BucketDol` | *deleted* | → `preset='supabase'` ([ADR-0003](0003-provider-presets-and-capabilities.md)) | @@ -48,6 +48,22 @@ burn permanently. | — | `ObjectHandle` | new; the per-object escape hatch | | — | `s3_store(...)` | the lowercase factory, matching `azuredol.azure_store` | +**`BucketStore` and `BucketsStore` are rejected as a pair** — the package's two most-used +classes, one silent `s` apart, with *opposite* key spaces (object keys vs bucket names), where +a typo yields a working, silently-wrong store. `azuredol` avoids this by naming the +*containing* resource (`ContainerStore` inside one container, `AccountStore` of containers); +`BucketStore` / `EndpointStore` is the same move. `S3Profiles`, not `S3Endpoints`, because its +keys are verifiably AWS profile names (plus the literal `'environment variables'`), never +endpoints — and its values are bucket *collections*, not bucket stores. + +Vocabulary alignment with the family, since ADR-0001 claims "a reader of `azuredol` can read +`s3dol`": use `value_codec=` (not `codec=`), and keep `prefix=`. + +**Public API:** `s3_store`, `BucketStore`, `BucketReader`, `EndpointStore`, `ObjectHandle`, +`S3Connection`, `Filepath`/`Chunks`/`Streamable`, the error classes, `diagnose`. Everything +else is implementation. (A layered library legitimately has more constructors than a flat one; +what v0 lacked was a statement of which were *public*.) + ### 2. The one-liner `s3dol.s3_store(bucket)` is what line 1 of the README shows. Zero credential ceremony, no @@ -66,9 +82,17 @@ fully-qualified path, not the package root. The shim is not merely compatible, it is a **fix delivery mechanism**: dependents get the corrected endpoint/credential resolution ([ADR-0002](0002-boto3-as-engine.md), [ADR-0003](0003-provider-presets-and-capabilities.md)) without changing a line. That matters -most for `http_cosmo_prep`, which passes an explicit `endpoint_url` for a non-AWS endpoint -and is therefore a live victim of the bug: whenever `AWS_ACCESS_KEY_ID` is exported, -`base.py:82` drops its endpoint and the store silently talks to AWS instead. +most for `http_cosmo_prep`, which passes an explicit `endpoint_url` for a non-AWS endpoint and +is therefore **at risk**: whenever `AWS_ACCESS_KEY_ID` is exported, `base.py:82` drops its +endpoint and the store aims at AWS instead. + +Be careful how strongly that is stated. Bucket names are globally unique on AWS, so if they do +not own that name the misrouted call fails loudly and someone would have noticed. The genuinely +*silent* variant is `AWS_ENDPOINT_URL_S3` — and it has a consequence worth naming: v0's +**effective** precedence when env credentials exist is +`AWS_ENDPOINT_URL_S3 > AWS_ENDPOINT_URL > explicit kwarg`, and the corrected ladder **inverts +the top of it**. Any deployment currently relying on `AWS_ENDPOINT_URL_S3` to override a stale +hard-coded endpoint will silently redirect on upgrade. That is why §5 has a step 0. `s3dol/tests/util.py` keeps `extract_s3_access_info` and `get_s3_test_access_info_from_env_vars`, because `py2store`'s import of them fails silently. @@ -77,15 +101,23 @@ and is therefore a live victim of the bug: whenever `AWS_ACCESS_KEY_ID` is expor These are bug fixes, and preserving them would mean preserving data-misrouting: -| v0 behaviour | v1 | -|---|---| -| explicit `endpoint_url` dropped when env credentials exist | honoured | -| explicit credentials overridden by env | explicit wins | -| write to a missing bucket **creates** it (even with `make_bucket=False`) | raises unless `on_missing_bucket='create'` | -| `list(store)` returns `[]` on any error | raises | -| `del buckets[name]` cascades, unpaginated | refuses non-empty; `force=True` is explicit | - -Each is called out in the release notes as behaviour-changing. The resolution ladder — +| v0 behaviour | v1 | shim keeps v0? | +|---|---|---| +| explicit `endpoint_url` dropped when env credentials exist | honoured | no — this is the fix | +| explicit credentials overridden by env | explicit wins | no — this is the fix | +| write to a missing bucket **creates** it (even with `make_bucket=False`) | per `on_missing_bucket` ([ADR-0010](0010-bucket-and-bulk-operations.md)) | no | +| `list(store)` returns `[]` on any error | raises | no | +| `del endpoint[name]` cascades, unpaginated | refuses non-empty; `force=True` is explicit | no | +| `store[k] = 'a str'` accepted | `TypeError` ([ADR-0005](0005-large-object-io.md) §2) | **yes** — shim wraps with `str.encode` + `DeprecationWarning` | +| `store['folder/']` returns a sub-store | removed; use `store.sub()` | **yes** | +| `del store[absent]` silently succeeds | still idempotent ([ADR-0010](0010-bucket-and-bulk-operations.md)) | n/a — unchanged | + +The last three rows exist because `http_cosmo_prep`'s **currently-passing** tests do all +three: `s['current_time.txt'] = str(...)` (`tests/test_prod_resources.py:26-27`), +`s['scrap/test_s3_store/']` as a sub-store, and `del s[...]` relying on the no-op. A shim whose +justification is "existing users keep working" must actually keep them working. + +Each remaining row is called out in the release notes as behaviour-changing. The resolution ladder — explicit kwargs > preset > `AWS_ENDPOINT_URL_S3` > `AWS_ENDPOINT_URL` > profile > chain — is documented and tested, and `s3dol.diagnose()` prints what resolved and from where (never the secret). @@ -101,12 +133,22 @@ that the code throws away. `s3dol.__version__` is added (absent today). Deprecations name their removal version. Order of operations, because publishing is automatic and irreversible: -1. Land the `dol` fixes ([ADR-0006](0006-key-scoping-and-dol-fixes.md)). -2. Pin `s3dol<1` in `lacing` and `reelee`, and open the `http_cosmo_prep` PR. +0. **Ship `s3dol.diagnose()` in a non-breaking 0.1.x patch.** It prints the resolved endpoint, + region and credential *source* (never the secret). Have each dependent run it in their real + environment and record the answer. Only then flip the resolution order — and ship the + endpoint/credential fix and the naming/API change as **separate releases**, so a dependent + who sees their data target move can bisect it to one change. +1. Land the `dol` fixes ([ADR-0006](0006-key-scoping-and-dol-fixes.md) §3), and raise the dol + floor to ≥0.3.58 for `wrapped_self`. +2. Pin `s3dol<1` in `lacing` and `reelee`, and open the `http_cosmo_prep` PR. Also **re-enable + `lacing`'s `tests/test_artifact_store_s3.py`**, which is currently excluded in + `lacing/pyproject.toml:217-223` with the comment *"fail in CI's clean env (s3dol url_for → + None)"* — [ADR-0008](0008-testing-architecture.md) proposes it as a cross-repo gate, and a + disabled test cannot gate anything. 3. Merge s3dol v1. Release notes lead with the behaviour changes. -Step 2 before step 3 is not optional: a stalled cross-org PR must not be able to strand -that repo on a broken line. +Step 2 before step 3 is not optional: a stalled cross-org PR must not be able to strand that +repo on a broken line. ## Consequences diff --git a/misc/docs/decisions/0008-testing-architecture.md b/misc/docs/decisions/0008-testing-architecture.md index f9a9ea3..d72865f 100644 --- a/misc/docs/decisions/0008-testing-architecture.md +++ b/misc/docs/decisions/0008-testing-architecture.md @@ -55,20 +55,44 @@ own stores. Beyond the obvious Mapping laws, these are the ones that would have caught real bugs: -1. **Prefix scoping round-trip.** With sibling and non-matching keys present - (`{'a/b','a/c','z','ab/x'}` scoped to `a/`), the store exposes exactly the in-scope keys. - This is the [ADR-0006](0006-key-scoping-and-dol-fixes.md) §1 ban, enforced. -2. **`url_for` correctness by parsing**, not substring: the URL path must equal the - fully-prefixed key, and fetching it against moto must return the object. -3. **`iter`/`contains` agreement**: `all(k in s for k in s)`. +1. **Prefix scoping round-trip.** Fixture must include sibling, non-matching, marker and + un-normalized cases: `{'a/b','a/c','z','ab/x','a/'}` scoped to `a/`, **and** + `{'logs/2026.txt','logs2/2026.txt','logsX'}` scoped to the un-terminated `logs` — which + must normalize to `logs/` and expose exactly one key. This is + [ADR-0006](0006-key-scoping-and-dol-fixes.md) §1, enforced. +2. **`url_for` correctness**, split by tier: + - **2a (all tiers, structural):** parse the URL; the path must equal the fully-prefixed + key, and the query must carry `X-Amz-Algorithm=AWS4-HMAC-SHA256` and **not** + `AWSAccessKeyId` (the SigV2 tell — see [ADR-0003](0003-provider-presets-and-capabilities.md) §4). + - **2b (tier 2+):** fetch it and get the object back. Use `requests`, which moto patches — + `urllib` is **not** patched and would leave the machine. This proves path correctness + only: moto validates neither signature nor expiry. Add `requests` to the `test` extra + (today it is only transitive via `moto[s3]`). +3. **`iter`/`contains` agreement**: `all(k in s for k in s)`. Fixture includes + `plain, 'a b', café, 'a+b', 'a\rb', 'p/x y', 'a%20b'` — the keys that break if anyone + reintroduces `EncodingType`. 4. **Never silently empty**: listing a missing/unlistable bucket raises. -5. **Value law**: `s[k] = v ⟹ s[k] == normalize(v)` for every member of the write domain - ([ADR-0005](0005-large-object-io.md) N3). +5. **Value law**: `s[k] = v ⟹ s[k] == normalize(v)`, **Layer B only and re-readable sources + only** — stream refs are one-shot ([ADR-0005](0005-large-object-io.md) N1), and through a + Layer C json codec `normalize` is `json.loads ∘ json.dumps`, which is not identity + (`(1,2) → [1,2]`, `{1:'a'} → {'1':'a'}`). Per-layer `normalize` is a parameter of the suite. 6. **Picklability**: `pickle.loads(pickle.dumps(store))` works, and a `ProcessPoolExecutor` round-trip works. Today `pickle` raises `PicklingError` and `deepcopy` raises `RecursionError` — a store that cannot cross a process boundary is unusable with Dask or multiprocessing, and nothing currently notices. 7. **No secret in `repr`**: `assert SECRET not in repr(conn)`, plus a traceback-locals scan. +8. **`setdefault` on an archived key does not mutate the object** + ([ADR-0004](0004-error-taxonomy.md) §4). +9. **Every shipped recipe's encoder output is a member of `BytesSource`** — this is what + catches `ValueCodecs.json()` returning `str`. + +The suite is parameterized by capability flags, because not every tier can run every +assertion: `mock_s3()` runs 2a but not 2b (no HTTP server to fetch from). The claim is +therefore *"`mock_s3()` passes the same suite, parameterized by an `endpoint_is_fetchable` +capability"* — not *"the same suite"* unqualified. + +The `len()`-raises assertion belongs on the **user-facing** store, which is the Layer B store +itself now that prefixing lives there. ### Cost model: `__len__` is not implemented @@ -76,7 +100,7 @@ Beyond the obvious Mapping laws, these are the ones that would have caught real listing — and worse, `list(store)` currently costs **two** listings because `list()` takes a length hint from `__len__`. Following `azuredol` §2, `BucketStore` does not implement `__len__` at all; `len(store)` raises `TypeError` with guidance to `sum(1 for _ in store)`. -`Buckets.__len__` is fine — bucket counts are small. +`EndpointStore.__len__` is fine — bucket counts are small. Listing caches are opt-in (`dol.cached_keys`), never default: the notebook explorer wants them and the pipeline is actively harmed by them. @@ -94,11 +118,21 @@ them and the pipeline is actively harmed by them. ### On moto's fidelity -moto is good enough for tier 2 but diverges: it accepts >1024-byte keys, has open -`aws-chunked` and composite-checksum bugs, and returns bodies on HEAD. It is faithful on -`CreateBucket`/`LocationConstraint` (checked — a claim to the contrary in the research was -wrong). Tier 3 exists because tier 2's divergences are exactly in the areas -[ADR-0003](0003-provider-presets-and-capabilities.md) cares about. +moto is good enough for tier 2 but diverges, and two of the divergences are exactly where this +package's correctness lives: + +- **HEAD error responses.** moto returns an error **body** on HEAD where AWS returns none, so + `head_object` against a missing bucket yields `Code='NoSuchBucket'` under moto and a + body-less `Code='404'` in production. This is the single divergence most likely to ship a + bug — see [ADR-0004](0004-error-taxonomy.md) §2. Tier 1 must feed the classifier synthesized + body-less HEAD errors, because **moto cannot produce them**. +- **`DeleteObjects` limits.** moto accepts 1001 keys where AWS caps at 1000, so tier 2 will + never catch a missing chunker. + +Also: it accepts >1024-byte keys, and has open `aws-chunked` and composite-checksum bugs. It +*is* faithful on `CreateBucket`/`LocationConstraint` (checked — a claim to the contrary in the +research was wrong, and it was being used to justify a non-moto tier). And it validates no +presigned-URL signature at all, which is why assertion 2a is structural. ## Consequences diff --git a/misc/docs/decisions/0009-scope-and-deferrals.md b/misc/docs/decisions/0009-scope-and-deferrals.md index 2482cdd..06474a6 100644 --- a/misc/docs/decisions/0009-scope-and-deferrals.md +++ b/misc/docs/decisions/0009-scope-and-deferrals.md @@ -34,9 +34,10 @@ errors.py one translate_s3_errors seam + the taxonomy values.py Filepath / Chunks / Streamable + as_fileobj writes.py write strategies (simple / transfer / multipart) reads.py read strategies (bytes / stream / ranged / to-file) -base.py BucketCollection/Reader/Store, BucketsCollection/Reader/Buckets, ObjectHandle -stores.py relative-key stores + codec facades -recipes.py s3_store(...) and friends +base.py BucketCollection/Reader/Store (own the prefix), + EndpointCollection/Reader/Store, ObjectHandle +recipes.py s3_store(...) + codec facades +diagnose.py s3dol.diagnose() — the step-0 migration safety mechanism store.py deprecated shim testing.py in-memory fake + exported conformance suite ``` @@ -70,7 +71,8 @@ operation, so its `__delitem__` would be a lie. S3 feature that is a nested store rather than a flat blob store. Deferred not for design reasons but for two hard facts: **no S3-compatible provider implements it** (portability 0/5), and it needs `botocore>=1.43.31`, too fresh for a storage library's hard floor. - Feature-detect with `hasattr(client, 'put_object_annotation')`. + Feature-detect with `hasattr(client, 'put_object_annotation')`. (The exact botocore floor + is cited from a research pass and **unverified** — confirm against PyPI before pinning it.) 6. **fsspec adapter** (`to_fsspec` / `from_fsspec`) — one adapter buys pandas, dask, pyarrow and zarr-v3-via-`FsspecStore`. 7. **`s3dol[fast]`** — obstore for the object level only, if re-measurement justifies it. @@ -139,3 +141,6 @@ is barely exercised until the low-portability families arrive in v1.x — accept retrofitting capability declaration later is much worse. **Enforcement.** A line budget, and one rule: **no new `Protocol` without two implementers.** +That rule bites immediately and correctly: an earlier draft of +[ADR-0005](0005-large-object-io.md) §4 proposed `WriteOnlyStore` Protocols with zero +implementers, and they are now on the deferral list rather than in v1. diff --git a/misc/docs/decisions/0010-bucket-and-bulk-operations.md b/misc/docs/decisions/0010-bucket-and-bulk-operations.md new file mode 100644 index 0000000..44f24c9 --- /dev/null +++ b/misc/docs/decisions/0010-bucket-and-bulk-operations.md @@ -0,0 +1,93 @@ +# ADR-0010: Bucket-existence policy and bulk operations + +- **Status:** Accepted +- **Date:** 2026-08-10 + +## Context + +Three surfaces were load-bearing in earlier ADRs but appeared only as prose, with no default, +no value domain and no owner: `on_missing_bucket`, `delete_many`, and (until +[ADR-0003](0003-provider-presets-and-capabilities.md) §5) `anon`. Each is reachable from the +README's first ten lines, so "unspecified" is not a neutral state. + +`on_missing_bucket` was additionally **self-contradictory** across three documents: it replaces +v0's `make_bucket` tri-state and is said to be "decided once, at construction", but `'raise'` +requires *knowing* the bucket is missing, which costs a `HeadBucket` — I/O in a constructor +that [ADR-0002](0002-boto3-as-engine.md) promises is I/O-free, or else the probe-then-act that +the architecture bans. + +Compounding it: `HeadBucket` requires `s3:ListBucket`, which the canonical least-privilege +policy omits ([ADR-0004](0004-error-taxonomy.md) §3), so a probing default would 403 for users +who can read and write every object perfectly well. + +## Decision + +### 1. `on_missing_bucket='assume'` is the default, and it performs no I/O + +| value | behaviour | +|---|---| +| **`'assume'`** (default) | Never probes. A missing bucket surfaces as `BucketNotFound` on the first real operation. Costs nothing, races nothing. | +| `'create'` | **Recover, don't probe.** Attempt the write; on `NoSuchBucket`, `CreateBucket` then retry once. | +| `'raise'` | Explicit opt-in that performs **one `HeadBucket` at construction**, documented as such. Only `('404', 404)` means missing; 403 raises `AccessDenied` naming `s3:ListBucket`. | + +`'assume'` is right because the loud failure is already free: every real operation returns +`NoSuchBucket`, so a probe buys nothing but latency and a permission requirement. This also +resolves the constructor contradiction — only `'raise'` does I/O, and its docstring says so. + +`'create'` has two details that bite: + +- `CreateBucket` needs `CreateBucketConfiguration={'LocationConstraint': region}` **everywhere + except `us-east-1`**, where passing it is an error. +- It is idempotent only in `us-east-1`; elsewhere a re-create raises `BucketAlreadyOwnedByYou` + (409), which must be tolerated. + +The compat shim maps `make_bucket=True → 'create'`, `make_bucket=False → 'raise'`, +`make_bucket=None → 'assume'`. (`lacing` passes `make_bucket=True` today, so this mapping is +exercised by a real dependent.) + +**README consequence, stated so it isn't rediscovered:** with `'assume'`, the two-line +quickstart `s = s3_store('my-bucket'); s['k'] = b'v'` raises on a bucket that doesn't exist +yet. That is correct — silently creating a bucket from a typo is the v0 behaviour this whole +ADR set exists to remove — but the README must show `on_missing_bucket='create'` in the +"start from nothing" example rather than pretending the happy path needs no argument. + +### 2. `delete_many(keys)` + +Chunks at **1000** (AWS's cap; moto accepts 1001, so tier 2 cannot catch a missing chunker), +parses the `Errors` list out of what is an **HTTP 200** response, and on partial failure raises +a single `S3PartialFailure(S3Error)` carrying `.succeeded: list[str]` and +`.failures: dict[str, S3Error]`. + +**Not an `ExceptionGroup`** — that is 3.11+, and `requires-python` is `>=3.10`. + +`DeleteObjects` reports **absent keys as `Deleted`**, so `delete_many` does not distinguish +them. That differs from `__delitem__` only in that neither raises — both are idempotent +([architecture.md](../architecture.md) contract table). + +When `Capabilities.batch_delete=False` (GCS, and any provider that rejects the mandatory +`x-amz-checksum-crc32` on `DeleteObjects`), the emulation loops `DeleteObject` and produces the +**identical exception object**. [ADR-0003](0003-provider-presets-and-capabilities.md) §2's +"the observable result is identical; only cost differs" holds for present keys; neither form +distinguishes absent ones. + +### 3. Cascading bucket deletion stays explicit + +`del endpoint[name]` raises `BucketNotEmpty` if the bucket has objects. +`endpoint.delete(name, force=True)` is the documented cascading form, and it **paginates** — +v0's version listed one page and deleted at most 1000 objects before failing on +`delete_bucket`, i.e. a partial, non-idempotent destruction. + +## Consequences + +**Buys.** A default that is safe, free, and needs no permission the caller doesn't already +have. Bulk delete with honest partial-failure semantics on a 3.10 floor. No probe-then-act +anywhere. + +**Costs.** `'assume'` means a mistyped bucket name is not caught until the first operation — +a slightly later, but still loud, failure. The `S3PartialFailure` type is one more exception +for users to learn, justified because the alternative (an HTTP 200 whose failures are silently +in a list nobody reads) is exactly the class of bug this ADR set targets. + +**What NOT to do.** Do not make `'raise'` the default "to be safe" — it is the option that +costs a round-trip and a permission. Do not use `ExceptionGroup` while 3.10 is supported. Do +not let `delete_many` claim it reports absent keys.