diff --git a/misc/docs/README.md b/misc/docs/README.md index 605f30d..24627f5 100644 --- a/misc/docs/README.md +++ b/misc/docs/README.md @@ -22,6 +22,7 @@ output; don't put prose here that a build step will overwrite.) | [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 | +| [0011](decisions/0011-keyed-capability-surface.md) | **No key-taking methods; capabilities are sibling stores** | adding any capability, or wondering where `url_for` went | ## The five things most likely to bite you @@ -37,11 +38,13 @@ output; don't put prose here that a build step will overwrite.) `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). +3. **A `dol` wrapper delegates methods with the outer, unmapped key**, so a keyed method like + `url_for` silently addresses the wrong object. Capability detection can't see it, and + **the obvious escape is also broken**: `inner_most_key(wrapped_self(self), k)` is silently + wrong when nothing holds a reference to the wrapper (`inner_most_key(self, k)` returns + `None`, which at least fails loudly). So s3dol has **no key-taking methods**: capabilities + are sibling stores keyed through `__getitem__` — the one thing `dol` maps correctly at every + depth. [ADR-0011](decisions/0011-keyed-capability-surface.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`. diff --git a/misc/docs/architecture.md b/misc/docs/architecture.md index 639d47d..d84338c 100644 --- a/misc/docs/architecture.md +++ b/misc/docs/architecture.md @@ -68,9 +68,14 @@ load-bearing rather than decorative: > 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. + > every capability method 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. + > + > **Necessary, not sufficient.** Prefix-in-leaf removes the seam *we* introduce; a user who + > stacks a `dol` key codec on top reintroduces it. The second half of the answer is to have + > almost no keyed methods to begin with — + > [ADR-0011](decisions/0011-keyed-capability-surface.md). - **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, @@ -109,16 +114,39 @@ Follows `dol.filesys`' triangle, in S3 vocabulary: ``` BucketCollection (Collection — __iter__ over object keys) - └── BucketReader (+ __getitem__ -> bytes, url_for, info, handle, sub, prefixes) - └── BucketStore (+ __setitem__ / __delitem__ / delete_many) + └── BucketReader (+ __getitem__ -> bytes) + └── BucketStore (+ __setitem__ / __delitem__) + +BucketHandles (KvReader — __getitem__ -> ObjectHandle) ┐ sibling capability +BucketUrls (KvReader — __getitem__ -> presigned URL) │ stores over the SAME +BucketInfo (KvReader — __getitem__ -> ObjectInfo) ┘ key space EndpointCollection (Collection — __iter__ over bucket names) └── EndpointReader (+ __getitem__ -> BucketReader) └── EndpointStore (+ __setitem__ / __delitem__ for buckets) ``` +The four bucket-level readers share one private base holding `prefix`, connection and +`_id_of_key`/`_key_of_id`, so the key arithmetic exists exactly once. + plus `ObjectHandle` — the escape hatch for one object, which is **not** a Mapping and is -where ranged reads, streaming, multipart and object metadata live. +where ranged reads, streaming, multipart, presigned URLs and object metadata live. +`ObjectHandle` binds its key **at construction**, which is what makes it immune to the +delegation trap ([ADR-0011](decisions/0011-keyed-capability-surface.md) §D1, following +`azuredol`'s `BlobHandle`). + +> **Layer B has no key-taking public methods** (one guarded exception, `url_for` — below). The +> Mapping dunders are the whole keyed surface, because `dol` maps *those* correctly at any +> wrapper depth. A keyed capability is a **sibling store** you index; anything non-keyed is a +> **free function taking the store first**. +> +> This is not stylistic. A `dol` key wrapper hands any non-dunder method the *outer, unmapped* +> key — and the obvious hardening, `inner_most_key(wrapped_self(self), k)`, is **itself silently +> wrong** when nothing holds a reference to the wrapper, returning a plausible but incorrect key. +> Sibling stores route through `__getitem__` instead, so they are correct *by construction* with +> no key-resolution primitive at all. See +> [ADR-0011](decisions/0011-keyed-capability-surface.md) §D1a/§D2. A reflective conformance test +> enforces the rule (§D5). `EndpointStore`, not `BucketsStore`: naming the *containing* resource (as `azuredol` does with `ContainerStore`/`AccountStore`) avoids shipping `BucketStore` and `BucketsStore` — two of the @@ -134,18 +162,48 @@ yields a working, silently-wrong store. | `__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. **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). | - -**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. + +**Keyed capabilities — sibling stores you index** ([ADR-0011](decisions/0011-keyed-capability-surface.md) §D2): + +| Store | `__getitem__(k)` returns | +|---|---| +| `BucketHandles` — `s3dol.handles(store)` | An `ObjectHandle` bound to `k`. Zero round-trips. | +| `BucketUrls` — `s3dol.urls(store)` | A presigned URL. Zero object requests. | +| `BucketInfo` — `s3dol.info(store)` | One `HeadObject` → `ObjectInfo`. | + +The `s3dol.(store)` accessors derive a sibling from an **unwrapped** store and **raise** on +a wrapped one, naming the remedy: wrap the sibling in parallel with the same codec. s3dol will +not guess a user's codec chain (that is dol#10). + +**Non-keyed operations — free functions** (§D3): + +| Function | Contract | +|---|---| +| `s3dol.sub(store, prefix)` | A store with an extended, normalized prefix, **in the caller's key space**. Zero round-trips on an unwrapped store, where it pushes down; on a wrapped store it composes `Pipe(filt_iter.prefixes(p), KeyCodecs.prefixed(p))` over the outer store, loses pushdown (§D8), and returns a different type. | +| `s3dol.prefixes(store)` | One `ListObjectsV2(Delimiter='/')` → `CommonPrefixes`, mapped back out to the caller's key space. | +| `s3dol.delete_many(store, keys)` | See [0010](decisions/0010-bucket-and-bulk-operations.md) §2. | +| `s3dol.delete_bucket(endpoint, name, force=True)` | The explicit cascading form. See [0010](decisions/0010-bucket-and-bulk-operations.md) §3. | + +**The one keyed method**, `BucketReader.url_for(k)`: exists only because `dol.SupportsUrlFor` +requires a *method* and `dol.content_url` reaches it by `getattr`. Guarded so it is correct when +unwrapped or when the wrapper is referenced, and **raises** otherwise — never silently wrong. +`s3dol.urls(store)[k]` is the canonical form (§D3b). + +On `ObjectHandle` (key bound at construction, so no delegation seam — these are methods): + +| Operation | Contract | +|---|---| +| `url(...)` | 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. | +| `info()` | One `HeadObject` → size, mtime, etag, content-type, storage class, restore status. | + +**The rule for what may join the Layer B table** (it is otherwise how a surface grows to +twenty): **nothing that takes a key.** Anything addressing one object becomes a sibling +capability store or lives on `ObjectHandle`; anything else is a free function or a recipe. + +The reason is mechanical, not aesthetic: `dol` hands any non-dunder method the outer, unmapped +key when the store is key-wrapped, and the standard workaround is itself silently wrong when the +wrapper is unreferenced. `azuredol` — the reference implementation — has ~zero keyed methods for +the same reason. See [ADR-0011](decisions/0011-keyed-capability-surface.md). ### Layer C — `s3dol.recipes` @@ -188,7 +246,11 @@ 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 (owns prefix) + base.py Layer B (owns prefix). NO key-taking public methods except the + guarded url_for shim — ADR-0011 §D1/§D3b + capabilities.py sibling stores BucketHandles/BucketUrls/BucketInfo + their + accessors; free functions sub / prefixes / delete_many / + delete_bucket (ADR-0011 §D2/§D3) 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 @@ -200,9 +262,11 @@ s3dol/ `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. +**Public API** — `s3_store`, `BucketStore`, `BucketReader`, `BucketHandles`, `BucketUrls`, +`BucketInfo`, `EndpointStore`, `ObjectHandle`, `S3Connection`, +`Filepath`/`Chunks`/`Streamable`, the error classes, `diagnose`, the capability accessors +`handles` / `urls` / `info`, and the free functions `sub` / `prefixes` / `delete_many` / +`delete_bucket`. 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`. @@ -221,7 +285,8 @@ else is implementation. *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. + `s3dol.delete_bucket(endpoint, name, force=True)` is the explicit form — a free function, not + a method ([ADR-0011](decisions/0011-keyed-capability-surface.md) §D4). - **`__len__` on a bucket store.** Unbounded pagination cost. - **Silent empties.** Anywhere. - **Passing `EncodingType` to a list call.** botocore sets it itself *and* decodes the @@ -235,8 +300,13 @@ else is implementation. 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."*). +reader classes, 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."*), and — the one that +took longest to see — **a container store with essentially no key-taking public methods**, with +the rich per-object surface on a handle that binds its key at construction +(`azuredol/base.py:233 BlobHandle`). That last property, not the prefix location, is what makes +it robust under user-applied key codecs +([ADR-0011](decisions/0011-keyed-capability-surface.md) §D1). **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 diff --git a/misc/docs/decisions/0001-layered-architecture.md b/misc/docs/decisions/0001-layered-architecture.md index 78c29d4..517431b 100644 --- a/misc/docs/decisions/0001-layered-architecture.md +++ b/misc/docs/decisions/0001-layered-architecture.md @@ -3,6 +3,10 @@ - **Status:** Accepted - **Date:** 2026-08-10 - **Supersedes:** the v0.1.x `base.py` / `store.py` / `utility.py` split +- **Amended by:** [ADR-0011](0011-keyed-capability-surface.md) — Layer B has **no** key-taking + public methods. `url_for`/`info` move onto `ObjectHandle` (key bound at construction); + `handle`, `sub`, `prefixes` and `delete_many` become free functions taking the store first. + See §"Prefix-in-leaf is necessary, not sufficient" below. ## Context @@ -91,6 +95,41 @@ appears **zero times** in the package. azuredol has none of the six bugs above p 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.** +## Prefix-in-leaf is necessary, not sufficient + +*(Added by [ADR-0011](0011-keyed-capability-surface.md). The section above is correct about why +the prefix must live in the leaf; it drew an incomplete lesson from `azuredol`.)* + +Putting the prefix in the leaf removes the key-mapping seam **for the prefix s3dol owns**. It +does nothing for a user who wraps an s3dol store with a `dol` key codec — every capability +method is then delegated with the outer, unmapped key again, by one of *two* routes +(`Store.__getattr__` at `dol/base.py:742` for instance-wraps and `mk_relative_path_store` +subclasses; `DelegatedAttribute.__get__` at `dol/base.py:279` for class-wraps). + +Re-reading `azuredol`'s code with that in mind gives the sharper finding: **`ContainerStore` has +essentially no key-taking public methods at all.** The rich per-object surface lives on +`BlobHandle` (`azuredol/base.py:233`), which binds its blob **at construction**, so a key codec +over the store cannot corrupt it. `azuredol`'s only residual exposures are +`ContainerCollection.walk` (`base.py:164`, leaf-keyed return) and `AccountStore.delete` +(`base.py:460`, keyed and destructive). + +So `azuredol` is not safe because of where its prefix lives. It is safe because **it has almost +no seam to get wrong.** The table above lists six methods this ADR originally proposed for +Layer B; [ADR-0011](0011-keyed-capability-surface.md) reduces that to **zero** (plus one guarded +`url_for` shim for `dol.SupportsUrlFor`), turning keyed capabilities into **sibling stores** you +index — `s3dol.handles(store)[k]` — and the rest into free functions. + +Before proposing that one keyed method be kept: the obvious hardening, +`inner_most_key(wrapped_self(self), k)`, is **itself silently wrong** when nothing holds a +reference to the wrapper, because the delegated bound method holds none and the weakref registry +entry is removed when the wrapper dies. Sibling stores avoid the question entirely by routing +through `__getitem__`, which `dol` maps correctly at every depth. Verified; see ADR-0011 +§D1a/§D2. + +The "Consequences" claim below that *"every capability method is key-correct by construction"* +holds only for an unwrapped store. Read it as: *there is no seam **we** introduce* — a user can +still add one. + Reader-only classes are **real classes**, not instances with methods deleted. `dol`'s `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 diff --git a/misc/docs/decisions/0005-large-object-io.md b/misc/docs/decisions/0005-large-object-io.md index 4aa9052..c398ada 100644 --- a/misc/docs/decisions/0005-large-object-io.md +++ b/misc/docs/decisions/0005-large-object-io.md @@ -27,6 +27,14 @@ pure" was being claimed while six methods were added: > admitted only because the cost difference against a `__delitem__` loop is an order of > magnitude. +> **Superseded by [ADR-0011](0011-keyed-capability-surface.md).** The rule above is now +> stricter and simpler: **a method may be added iff it takes no key at all.** A keyed method is +> handed the *outer, unmapped* key by any `dol` key wrapper, and the standard escape is itself +> silently wrong when nothing references the wrapper — so `info`, `url_for`, `sub`, `handle`, +> `prefixes` and `delete_many` are *not* admissible as methods. Capabilities become sibling +> stores keyed through `__getitem__` (`s3dol.handles(store)[k]`) or free functions. `url_for` +> survives as a single guarded exception, purely to satisfy `dol.SupportsUrlFor`. + 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. @@ -184,7 +192,7 @@ stream raises from `botocore/httpchecksum.py` *before the request is built*, so in increasing explicitness: a `reads=stream_reads()` strategy at construction (the store's 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()` / +construction, which v1 does not do); `s3dol.handle(store, 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 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 dcb719c..5600b58 100644 --- a/misc/docs/decisions/0006-key-scoping-and-dol-fixes.md +++ b/misc/docs/decisions/0006-key-scoping-and-dol-fixes.md @@ -76,7 +76,21 @@ Related upstream bug: `dol.trans.filter_prefixes(['logs/', 'tmp/'])` compiles to ### 2. The delegation trap (for anyone stacking `dol` on top) -`dol` wrappers delegate unknown attributes to the leaf **with the outer, unmapped key**: +> **Amended by [ADR-0011](0011-keyed-capability-surface.md), and the headline below is now +> wrong.** `inner_most_key(wrapped_self(self), k)` is **not** a correct escape: it is silently +> wrong whenever the wrapper is a temporary (§D1a), and the failure is undetectable. s3dol's +> answer is instead to have **no key-taking methods at all** and to resolve keys in free +> functions that receive the store as an argument. Read this section for the *mechanism* — it is +> accurate about how the trap works — but take the remedy from ADR-0011, not from here. + +`dol` wrappers delegate unknown attributes to the leaf **with the outer, unmapped key**. There +are **two** routes, and a fix that covers one is a silent no-op on the other: + +| route | when | site | +|---|---|---| +| `Store.__getattr__` | instance-wraps; `mk_relative_path_store` subclasses | `dol/base.py:742` | +| `DelegatedAttribute.__get__` | class-wraps (one descriptor per attr of `dir(wrapped)`) | `dol/base.py:279` | + ```python w = KeyCodecs.prefixed("a/")(WithUrl)(...) @@ -89,7 +103,9 @@ w.url_for("b") # -> https://x/b WRONG: should be https://x/a/b `isinstance` uses `getattr_static`, which sees a **class**-wrapped capability but not an **instance**-wrapped one. Capability detection must not rely on it.) -The correct escape is **`inner_most_key(wrapped_self(self), k)`**. +The escape this ADR originally prescribed was **`inner_most_key(wrapped_self(self), k)`**. +**Do not use it** — trap 4 below shows it is silently wrong on temporary wrappers. It is kept +here because traps 1–3 apply to the *replacement* too, and because the reasoning matters. > `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 @@ -99,6 +115,61 @@ The correct escape is **`inner_most_key(wrapped_self(self), k)`**. 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. +**Four things about that form, all verified, none of them obvious. The fourth retires it.** + +1. **`inner_most_key` is not importable from `dol`.** It lives in `dol.dig` + (`dol/dig.py:69`); `dol/__init__.py` exports only `trace_getitem` from that module. The + import is `from dol.dig import inner_most_key`. (`wrapped_self` *is* exported from `dol`.) + Making it public is on the upstream list in §3. + +2. **It REPLACES `self._id_of_key(k)`; it never composes with it.** Because §1 of this ADR puts + the prefix in the leaf, `inner_most_key` walks the whole chain *including the leaf's own + `_id_of_key`*, so it already returns the fully-prefixed absolute key. Composing them + double-prefixes, silently — and `self._id_of_key(k)` is exactly what a capability method's + author will reach for: + + ```python + # store scoped to 'logs/', outer key 'a.txt' + inner_most_key(wrapped_self(self), k) # 'logs/a.txt' correct + self._id_of_key(inner_most_key(wrapped_self(self), k)) # 'logs/logs/a.txt' WRONG + ``` + + Verified correct in all five wrap shapes — unwrapped, `KeyCodecs.prefixed`, `filt_iter` + (key-identity layer), a `Pipe` stack, and a value-only `wrap_kvs`. + +3. **The `str` check is mandatory, not defensive.** `inner_most_key` returns `None` — silently, + via `last_element` over an empty generator — when no layer in the chain supplies + `_id_of_key`. That is what turns a wrong key into `https://…/None`, and in `dol`'s own + `filesys.MakeMissingDirsStoreMixin` it turns a write error into + `TypeError: expected str … not NoneType`. + +4. **It is silently wrong when the wrapper is a temporary — which retires the form.** + `wrapped_self` resolves through a weakref registry keyed by `id(inner)`. A delegated bound + method holds **no reference to the wrapper**, so in a chained expression the wrapper is freed + before the body runs, and `_register_wrapper_backref`'s cleanup callback *removes the + registry entry* — making it indistinguishable from "never wrapped": + + ```python + s = KeyCodecs.prefixed("x/")(BucketReader(data, prefix="logs/")) + s.m_abs("b.txt") # 'logs/x/b.txt' correct (wrapper is named, so alive) + KeyCodecs.prefixed("x/")(BucketReader(data, "logs/")).m_abs("b.txt") + # 'logs/b.txt' WRONG, silently (wrapper was a temporary) + ``` + + And because §1 puts the prefix in the leaf, the wrong answer is a **plausible `str`** — the + leaf's own `_id_of_key` still fires — so trap 3's type check does not catch it. Measured: + free-function form 6/6 correct across wrap × lifetime shapes; method form 2/4. + + Consequence: `wrapped_self` is a **best-effort guardrail, not a correctness mechanism** + (dol#18's own doc: *"a guardrail, not a cure"*). s3dol therefore resolves keys in **free + functions that receive the store as an argument**, where the caller keeps it alive and the + registry is never consulted — [ADR-0011](0011-keyed-capability-surface.md) §D2/§D3. The + temporary-wrapper hole itself goes upstream to dol#18 (ADR-0011 §D9). + +`wrapped_self` does survive `pickle` and `deepcopy` (dol#18's `__setstate__` re-registration +landed) — that part of the story is fine; it is object lifetime, not serialization, that breaks +it. + ### 3. What still goes upstream to `dol` Reduced, now that prefixing lives in the leaf and `wrapped_self` turns out to exist: @@ -112,6 +183,33 @@ Reduced, now that prefixing lives in the leaf and `wrapped_self` turns out to ex the wrapped class deliberately omits. 4. **Document `wrapped_self` as the delegation answer**, and make the family use it. +Added by [ADR-0011](0011-keyed-capability-surface.md), both **blocking for s3dol**: + +5. **Export `inner_most_key`** from `dol/__init__.py`, and **harden `dig.store_trans_path`** — + raise instead of returning `None` when no layer supplies `_id_of_key`, and fix `dol/dig.py:41` + hardcoding `unravel_key` for the recursive step (which is why `inner_most_val` does not do + what its name says — an acknowledged TODO at `dol/dig.py:71`). s3dol's `_abs_key` depends on + this function being both importable and non-silent. + +6. **Fix `dol.content_url`** (`dol/content.py:210-214`). It resolves with a flat + `getattr(store, 'url_for')(key)` — no chain walk — so through any key wrap it returns a URL + for the unmapped key. `dol/content.py`'s module docstring names an `s3dol` store as the + intended S3 backend and frames `url_for` as the presigned-URL seam, so this **blocks that + integration being correct at all**. Its existing tests are all identity-keyed, which is why + it was never caught. + +7. **Report the `wrapped_self` temporary-wrapper hole** on dol#18 (§2 trap 4). Not blocking — + s3dol routes around it via free functions — but dol ships `wrapped_self` as the *blessed* + pattern, and `xdol` and `unbox` have already adopted it, so they inherit a silent failure + mode. Not s3dol's to fix; s3dol's to report with the repro. + +Adjacent, non-blocking, and worth fixing because it is the best regression sentinel for any +future delegation fix: **`dol.filesys.FileSysCollection.is_valid_key`/`validate_key`** +(`dol/filesys.py:422,425`) are confirmed-live broken — `Files(d).is_valid_key(k)` is `False` +for a key that exists, because `Files` is a `mk_relative_path_store` and the leaf's regex matches +absolute paths. `dol/paths.py:1199-1206` already carries the hand-rolled fix for exactly this +shape. + **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. @@ -150,12 +248,13 @@ it, since botocore adds it. If a provider rejects botocore's auto-`EncodingType` 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 +from `s3dol.sub(store, 'folder/')` ([ADR-0011](0011-keyed-capability-surface.md) — a free +function, not a method). `store['folder/']` survives only on an explicitly-constructed 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 +filters out the exact-prefix marker, and `s3dol.sub(store, 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.) @@ -175,3 +274,9 @@ justified only because the alternative is a key the store iterates but refuses t 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. +6. Never resolve a key inside a delegated method **at all** — not even with + `wrapped_self`. Resolve it in a free function that receives the store (§2 trap 4, + [ADR-0011](0011-keyed-capability-surface.md) §D1a/§D3). +7. Never compose `inner_most_key(store, k)` with `store._id_of_key(...)`. It already includes + the leaf's prefix; composing double-prefixes silently (§2). +8. Never use the result without checking it is a `str` — `None` is a legal return (§2). diff --git a/misc/docs/decisions/0007-naming-and-compatibility.md b/misc/docs/decisions/0007-naming-and-compatibility.md index 1c786d2..b462b5c 100644 --- a/misc/docs/decisions/0007-naming-and-compatibility.md +++ b/misc/docs/decisions/0007-naming-and-compatibility.md @@ -109,7 +109,7 @@ These are bug fixes, and preserving them would mean preserving data-misrouting: | `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** | +| `store['folder/']` returns a sub-store | removed; use `s3dol.sub(store, …)` ([ADR-0011](0011-keyed-capability-surface.md)) | **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 diff --git a/misc/docs/decisions/0009-scope-and-deferrals.md b/misc/docs/decisions/0009-scope-and-deferrals.md index 06474a6..ec41af8 100644 --- a/misc/docs/decisions/0009-scope-and-deferrals.md +++ b/misc/docs/decisions/0009-scope-and-deferrals.md @@ -46,11 +46,19 @@ Plus, from the "beyond blobs" analysis, the items that are **free or nearly so** 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. + mtime, ETag and storage class, and v0 throws them away. One HEAD, plus a cheap + listing-derived metadata view, costs almost nothing and serves 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. + since 2023 and is **called from nowhere**. One LIST with a delimiter. +- **A presigned-URL view.** The presigning logic already exists; a `Mapping` face over it is + trivial. + +> **Shapes updated by [ADR-0011](0011-keyed-capability-surface.md).** These three are no longer +> `store.info(k)` / `store.prefixes()` / a view *attribute*. They are **sibling stores** — +> `s3dol.info(store)[k]`, `s3dol.urls(store)[k]` — plus the free function +> `s3dol.prefixes(store)`. That is not extra work: ADR-0011 §D2 makes all three instances of one +> mechanism, which is why they move from "nearly free" to actually shared. The ADR-0011 note +> that they were "deferred" is about the *attribute* form only. 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 — diff --git a/misc/docs/decisions/0010-bucket-and-bulk-operations.md b/misc/docs/decisions/0010-bucket-and-bulk-operations.md index 44f24c9..5a71e70 100644 --- a/misc/docs/decisions/0010-bucket-and-bulk-operations.md +++ b/misc/docs/decisions/0010-bucket-and-bulk-operations.md @@ -53,6 +53,17 @@ ADR set exists to remove — but the README must show `on_missing_bucket='create ### 2. `delete_many(keys)` +> **Amended by [ADR-0011](0011-keyed-capability-surface.md) §D4: `delete_many` is a free +> function, `s3dol.delete_many(store, keys)`, not a store method.** The semantics below are +> unchanged; only the surface moves. +> +> Reason: a keyed, destructive, *delegated* method is the exact combination that the `*dol` +> family census found already destroying the wrong data in three sibling packages +> (`cosmodol.CosmosItems.batch`, `cosmodol.CosmosDatabase.delete`, `azuredol.AccountStore.delete`). +> As a method it would be handed the outer, unmapped key by any key wrap and would delete +> objects the caller cannot see; as a free function it resolves the key through the whole +> wrapper chain first. See [dol#83](https://github.com/i2mint/dol/issues/83). + 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 @@ -72,8 +83,15 @@ distinguishes absent ones. ### 3. Cascading bucket deletion stays explicit +> **Amended by [ADR-0011](0011-keyed-capability-surface.md) §D4: the cascading form is +> `s3dol.delete_bucket(endpoint, name, force=True)`, a free function — not +> `endpoint.delete(...)`.** A public, key-taking, destructive, *delegated* method is +> structurally identical to `azuredol.AccountStore.delete`, which ADR-0011 cites as a census +> exhibit. `del endpoint[name]` is unaffected: it is a Mapping dunder, so `dol` maps it +> correctly. + `del endpoint[name]` raises `BucketNotEmpty` if the bucket has objects. -`endpoint.delete(name, force=True)` is the documented cascading form, and it **paginates** — +The cascading form **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. diff --git a/misc/docs/decisions/0011-keyed-capability-surface.md b/misc/docs/decisions/0011-keyed-capability-surface.md new file mode 100644 index 0000000..56f90c9 --- /dev/null +++ b/misc/docs/decisions/0011-keyed-capability-surface.md @@ -0,0 +1,372 @@ +# ADR-0011: The keyed capability surface and the unmapped-key problem + +- **Status:** Accepted +- **Date:** 2026-08-10 +- **Discussion:** [#14](https://github.com/i2mint/s3dol/discussions/14) +- **Amends:** [ADR-0001](0001-layered-architecture.md) (Layer B method table), + [ADR-0005](0005-large-object-io.md) §2 (the surface-growth rule), + [ADR-0006](0006-key-scoping-and-dol-fixes.md) §2 (the escape form), + [ADR-0007](0007-naming-and-compatibility.md) (sub-store migration row), + [ADR-0009](0009-scope-and-deferrals.md) (v1.0 scope: `info`/`prefixes` shapes), + [ADR-0010](0010-bucket-and-bulk-operations.md) §2–§3 (`delete_many`, cascading delete) +- **Upstream:** [dol#83](https://github.com/i2mint/dol/issues/83) (this problem), + [dol#18](https://github.com/i2mint/dol/issues/18) (its root) + +## Context + +[ADR-0001](0001-layered-architecture.md) puts the prefix in the leaf, which removes the +key-mapping seam between a capability method and the wire **for the prefix s3dol owns**. It does +not remove the seam in general: a user who wraps an s3dol store with a `dol` **key** codec still +gets a silently wrong `url_for`. Discussion #14 asked how to fix that properly. + +### The mechanism, precisely + +`dol` wraps by **delegation (has-a)**. Key transforms are applied by the wrapper's +`__getitem__` / `__setitem__` / `__delitem__` / `__contains__` / `__iter__`. Every other method +is handed the **outer, unmapped** key. There are **two** delegation routes, not one: + +| route | when | site | +|---|---|---| +| `Store.__getattr__` | instance-wraps, and `mk_relative_path_store` subclasses | `dol/base.py:742` | +| `DelegatedAttribute.__get__` | class-wraps (`delegate_to` installs one descriptor per attr of `dir(wrapped)`) | `dol/base.py:279`, installed at `dol/base.py:416-480` | + +Both return the method **bound to the leaf**. #14's framing named only the first; a fix covering +one and not the other is a silent no-op on half the cases. + +Capability detection cannot see it: a `@runtime_checkable` Protocol checks method *presence* +only. It is also wrap-dependent — since 3.12 `isinstance` uses `getattr_static`, so +`isinstance(w, SupportsUrlFor)` is `True` for a **class**-wrapped capability and `False` for an +**instance**-wrapped one. Either way it says nothing about whether the key is right. + +### What the family census showed + +13 sibling packages were surveyed and every claim re-verified by source read plus a runnable +repro. **Stated accurately, because an earlier draft of this ADR overstated it:** the defect is +overwhelmingly **latent** — it bites only when a user applies a key codec, and most of these +packages never wrap their own stores. Verified breakdown: `focal` confirmed-live throughout; +`azuredol`, `aiofiledol`, `chromadol`, `sqldol`, `mongodol`, `redisdol` mixed; `cosmodol`, +`pydrivedol`, `sshdol`, `dynamodol`, `hfdol` latent-only; `couchdol` essentially refuted. **12 +survey claims were refuted outright.** + +Latent is not harmless — the latent cases include `cosmodol.CosmosItems.replace`, which under a +key codec silently overwrites a *different, real* document in full, and +`pydrivedol.GDReader.get_url`, which returns a URL for the wrong file *and grants +anyone/reader permission on it*. But "already destroying data in production" is not what the +evidence supports, and this ADR does not claim it. Per-package detail and repros go to the +responsible repos, indexed from [dol#83](https://github.com/i2mint/dol/issues/83). + +One case *is* confirmed-live in `dol` itself: `dol.filesys.Files(d).is_valid_key(k)` returns +`False` for a key that exists. + +## Decision + +### D1 — Layer B carries no key-taking public methods (one documented exception) + +[ADR-0001](0001-layered-architecture.md)'s Layer B table proposed **six** keyed methods +(`url_for`, `info`, `handle`, `sub`, `prefixes`, `delete_many`). `azuredol` — the reference +implementation for our layering — has **~zero**: `ContainerStore` is deliberately method-free, +and the rich per-object surface lives on `BlobHandle` (`azuredol/base.py:233`), which takes its +blob **at construction**, so wrapping the store with a key codec cannot corrupt it. Its only +residual exposures are `ContainerCollection.walk` (`base.py:164`) and `AccountStore.delete` +(`base.py:460`). + +**`azuredol` is not safe because the prefix lives in the leaf. It is safe because it has almost +no seam to get wrong.** Prefix-in-leaf is necessary, not sufficient. That is the finding, and +ADR-0001 previously drew the wrong lesson from the same source. + +The single exception is **`url_for(k)`, kept solely to satisfy `dol.SupportsUrlFor`** — see D3b. +It is the only entry on D5's allowlist and it is guarded so it is correct-or-loud, never +silently wrong. + +### D1a — Why a hardened *method* is not the answer + +The first draft kept `handle(k)`/`sub(prefix)` as methods hardened with +`inner_most_key(wrapped_self(self), k)` — the form ADR-0006 §2 prescribed. Testing that plan +refuted it. + +`wrapped_self` resolves the outer store through a weakref registry keyed by `id(inner)`. A +`DelegatedAttribute` returns a method bound to the **leaf**, holding no reference to the +wrapper. So when nothing else holds the wrapper, it is freed before the method body runs and the +registry entry is removed by the cleanup callback: + +```python +s = KeyCodecs.prefixed('x/')(BucketReader(data, prefix='logs/')) +s.m_abs('b.txt') # 'logs/x/b.txt' correct +KeyCodecs.prefixed('x/')(BucketReader(data, 'logs/')).m_abs('b.txt') + # 'logs/b.txt' WRONG, silently +``` + +The wrong answer is a **plausible `str`** — precisely because ADR-0001 puts the prefix in the +leaf, the leaf's own `_id_of_key` still fires — so a type check cannot catch it. + +**Four corrections to that draft, all from an adversarial review, all re-verified:** + +1. **The predicate is "no live strong reference", not "temporary".** A temporary caught in a + reference cycle silently starts working, and `operator.methodcaller('m', k)(obj)` is correct + where `obj.m(k)` is not. The failure is **intermittent**. +2. **The trigger is narrower than claimed.** `s3_store('bucket', prefix='p').handle(k)` is + **not** an instance of the bug: `s3_store` returns a bare leaf or a **value**-codec wrap, and + both are correct. Nor is `filt_iter` alone. Only a user-applied **key** codec breaks it. +3. **It is detectable.** `Store.__init__` probes the leaf with `hasattr(self.store, "KeysView")`, + so a leaf can record that it was ever wrapped and refuse when `wrapped_self(self) is self` + but that flag is set. Verified against instance-wrap, class-wrap and `Pipe`. So the honest + claim is *"a method can be made loud rather than silently wrong, at the cost of a hack"* — + not *"a method is impossible"*. +4. **Reproduced on CPython 3.10–3.14**; not gc-, version- or bytecode-specific. + +So the method form is rejected on **cost/benefit**, not impossibility: it needs a guard riding +on an incidental `hasattr` probe, which misses the unpickle path and false-positives on a +once-wrapped leaf used bare. D2 gets the same correctness with none of that. + +This still demotes `wrapped_self` generally: it is a **best-effort guardrail with a silent +failure mode**, not "the correct escape". The hole goes upstream (D9). + +### D2 — Keyed capabilities are sibling *stores*, keyed through `__getitem__` + +A capability becomes a Layer B `KvReader` over the same key space whose `__getitem__` returns +the capability: + +``` +BucketCollection → BucketReader → BucketStore k -> bytes +BucketHandles k -> ObjectHandle +BucketUrls k -> presigned URL (str) +BucketInfo k -> ObjectInfo +``` + +All four share one private base holding `prefix`, connection and `_id_of_key`/`_key_of_id`, so +the key arithmetic exists once. + +**`__getitem__` is the one thing `dol` maps correctly at every depth**, so this is correct *by +construction*: no `inner_most_key`, no `wrapped_self`, no private `dol.dig` import, no upstream +PR, no guard. Verified correct with no live reference, under `Pipe`, under `cached_keys`, and +under a hand-rolled non-`Store` passthrough layer — the last being a case where the +free-function form is **silently wrong** (D3a). + +It also restores `[k]` ergonomics, and subsumes three capability features +[ADR-0009](0009-scope-and-deferrals.md) currently defers (presigned-URL store, +ObjectInfo-from-LIST, handles) into one mechanism rather than three. + +**The cost, stated plainly:** a user who wraps the data store must wrap the sibling in parallel. + +```python +s = s3_store('bucket', prefix='p/') +h = s3_handles('bucket', prefix='p/') +h['f'].url() # correct + +c = KeyCodecs.prefixed('x/') # user-applied key codec +s2, h2 = c(s), c(h) # wrap BOTH +h2['f'] # -> p/x/f correct at any depth, any lifetime +``` + +`s3dol.handles(store)` / `.urls(store)` / `.info(store)` derive a sibling from an **unwrapped** +store and **raise** on a wrapped one, naming the parallel-wrap remedy. That refusal is reliable: +these are free functions holding the store, so `isinstance(store, dol.base.Store)` is a real +test — no weakref, none of D1a's fragility. Re-deriving the user's codec chain onto a sibling is +`dol`'s recursive-wrap problem (dol#10) and is explicitly **not** attempted here. + +### D3 — Free functions for everything not keyed through a store + +Bulk and endpoint-level operations have no Mapping to ride on, so they are free functions: + +```python +s3dol.delete_many(store, keys) # D4; see ADR-0010 §2 +s3dol.prefixes(store) # relative to the caller's key space +s3dol.sub(store, prefix) # a store in the CALLER's key space +s3dol.delete_bucket(endpoint, name, force=True) # D4; see ADR-0010 §3 +``` + +This matches `dol`'s own idiom (`content_url`, `get_content`, `put_content`, `add_content` all +take the store first). + +**`sub` has an obligation:** resolving the absolute prefix is not sufficient, because a leaf +sub-store would speak the leaf's key space and silently drop the user's outer codec. On an +unwrapped store use `leaf._with(prefix=…)` (cheap, pushes down); on a wrapped store compose over +the **outer** store with `Pipe(filt_iter.prefixes(p), KeyCodecs.prefixed(p))` +([ADR-0006](0006-key-scoping-and-dol-fixes.md) §1) — correct, but it loses pushdown (D8) and +returns a different type than the unwrapped branch. Document both. + +**`prefixes` has the mirror obligation:** its results must be mapped **outward** through the +chain (`_key_of_id` per layer, outermost last) — the inverse of `inner_most_key`, which +`dol.dig` does not provide. Implement locally with a round-trip property test. + +#### D3a — Free functions are *more* reliable than methods, not reliable + +`inner_most_key` walks `.store` applying each layer's `_id_of_key`, which breaks when a layer is +**not** a `dol` `Store`. Verified against a ground-truth oracle: + +| chain | wire key | free function | method form | +|---|---|---|---| +| `KeyCodecs` / `Pipe` / `cached_keys` / `filt_iter` / `wrap_kvs` | — | correct | correct (if referenced) | +| hand-rolled `__getattr__`-passthrough over the leaf | `logs/b.txt` | **`logs/logs/b.txt`** | correct | +| same passthrough over a key codec | `logs/x/b.txt` | **`logs/x/x/b.txt`** | correct | +| key codec over a plain `.store`-holding middle layer | `logs/x/b.txt` | **`x/b.txt`** | also wrong | + +A passthrough `__getattr__` resolves `_id_of_key` to the *leaf's* bound method, so the walk +applies it at the delegator and again at the leaf; a middle layer without `_id_of_key` truncates +the walk instead. Every `dol`-shipped wrapper is safe (every `Store` inherits an identity +`_id_of_key`) — but `dol/base.py` ships a documented hand-rolled `Delegator` recipe of exactly +the breaking shape. + +**No docstring may claim "correct at any wrapper depth" for a free function.** D2's stores may: +they never call `inner_most_key`. + +#### D3b — `url_for` survives as a method, for the protocol only + +`dol.SupportsUrlFor` requires a `url_for` **method**, and `dol.content_url` reaches it with +`getattr(store, 'url_for', None)`. With no method anywhere, `content_url` would return `None` +for every s3dol store forever — and `dol/content.py`'s module docstring names an `s3dol` store +as its intended S3 backend. + +So `BucketReader.url_for(k)` ships, with the D1a §3 guard: correct when unwrapped or when the +wrapper is referenced, and **raising** (naming `s3dol.urls(store)[k]`) when the wrapper is +unreachable. Verified correct-or-loud in all three cases, and `dol.content_url` returns the +right URL for both an unwrapped and a referenced-wrapped store. + +`s3dol.urls(store)[k]` (D2) remains the canonical form. `url_for` is a compatibility shim with a +documented limitation, and the real fix is upstream: `content_url` must resolve through the +chain **and** call the innermost `url_for` with the resolved key — resolving without that second +half would double-transform. + +### D4 — Destructive operations are free functions + +`delete_many(keys)` and `EndpointStore.delete(name, force=True)` both leave Layer B. + +Destructive **+** key-taking **+** delegated is the shape the census found in `cosmodol` +(`CosmosItems.replace`/`batch`, `CosmosDatabase.delete`, `CosmosAccount.delete`), `azuredol` +(`AccountStore.delete`) and `pydrivedol` (`GDStore.upload`). Latent or not, s3dol does not add +another. [ADR-0010](0010-bucket-and-bulk-operations.md)'s semantics are unchanged — 1000-key +chunking, the HTTP-200 `Errors` parse, `S3PartialFailure`, the refusal to cascade implicitly — +only the surface moves. + +`del endpoint[name]` stays, and still refuses a non-empty bucket: it is a Mapping dunder, so +`dol` maps it correctly. + +One honest caveat: a module-level `delete_many(store, keys)` accepts any object as its first +argument, where a method could not be called on a store lacking it. Free-function form fixes the +*unmapped key* problem, not the *wrong target* problem. Validate the first argument. + +### D5 — A conformance test, not a registry + +No `_key_methods` registry. A **reflective conformance test** enumerates the public methods of +every Layer B class and fails on any that takes a key — allowlist: `url_for` (D3b), and nothing +else. + +The predicate cannot be "first argument is named like a key": that misses `delete(name, ...)`, +`delete_many(keys)`, `cosmodol`'s `batch(operations)` and `sshdol`'s `sync_to(target)` — several +of the shapes this ADR most cares about. It is "does this method accept anything that reaches +the wire as a key, at any argument position or nested in a structure", which is a judgement. +The test therefore asserts against an **explicit inventory of every public method** and fails on +any *new* one, forcing the judgement at review time rather than pretending a signature check +suffices. + +#14 observed that a declarative registry fails the same silent way when an author forgets to +declare a method, and that a reflective test is "the kind of guard that actually holds". Note +also that `dol` already ships a hook of Option B's shape — +`wrap_kvs(ingoing_key_methods=…, outcoming_key_methods=…)` — which is untested and verified +broken for leaf-defined methods on both wrap paths (it fails loudly, at least). Option B would +mean replacing it, not building on it. + +### D6 — Option E: rejected as an *attribute*, adopted as a *store* + +#14's Option E argued that capabilities-as-Mappings work because "key transformation happens +through the Mapping protocol the wrapper already handles correctly." + +- **As a Mapping-valued *attribute* (`store.urls`) that is false.** Verified: a wrapper does not + re-wrap such an attribute; `store.urls` returns an inner-keyed mapping under both class- and + instance-wrap. That form needs the same key resolution as everything else. +- **As a sibling *store* it is exactly true**, and that is D2. + +The first draft of this ADR conflated the two and deferred Option E wholesale. That was wrong: +the provisional lean in #14 (C + E, with B) was right in substance — the correction is that E's +value comes from being a *store*, not an attribute, and that C's role shrinks to the +non-keyed operations. + +Making `store.urls` work as an attribute would need `dol` to propagate and re-wrap +Mapping-valued attributes — dol#10, and the `.meta` sidecar design in +`dol/misc/docs/dol_content_metadata_bifurcation.md` §2.2, whose stated blocker is precisely +key-transform propagation. Deferred to v1.x; D2 does not depend on it. + +### D7 — Option D (rebind delegated methods to the wrapper) is rejected + +Not primarily for blast radius. `dol/misc/docs/dol_issue18_design.md` surveyed 26 ecosystem +sites and rejected the rebind family on three verified defects, the fatal one being that +rebinding binds `self` to the **innermost** `Wrap` — so under a `Pipe` stack it *does not fix the +case it exists to fix*, and stacked-codec writes gain a partial-transform corruption surface. +Plus statically-undetectable crashes: a leaf method calling `super().__getitem__(k)` compiles +`super(SomeClass, self)` with `self` now a `Wrap` → `TypeError`. + +That document's Phase-1 instructions say: *"**Do not touch** `DelegatedAttribute.__get__`, the +`delegate_to` copy loop, or the `base.py:451` signature graft."* We comply. + +**The terminal fix is `dol`'s is-a wrapping** (dol#18 Approach C), which would resolve dol#18 and +dol#6 together and make D1a moot. Note it is the doc's *recommended* terminal direction but +still an open question for the maintainer (§9 of that doc), and Phases 2–3 have not shipped. So +the selection criterion here was *correct on dol 0.3.x **and** harmlessly redundant under is-a* — +which D2 satisfies: sibling stores stay correct either way. + +### D8 — Prefix pushdown is closed, not deferred + +ADR-0001 already solves pushdown for the prefix s3dol owns: the leaf passes +`ListObjectsV2(Prefix=self.prefix)`. The residual case is a user stacking `filt_iter.prefixes(…)` +on top, which is **general predicate pushdown** — `dol` has no framework for it, and an +`__iter__(*, prefix_hint=…)` protocol would have exactly one implementer, violating +[ADR-0009](0009-scope-and-deferrals.md)'s own "no new `Protocol` without two implementers" rule. + +A user-stacked filter **accepts a full scan**; `s3dol.sub(store, prefix)` is the documented cheap +path and pushes down on an unwrapped store. This is an answer, not a deferral; do not reopen it +without a second implementer. + +### D9 — Upstream findings + +Blocking for s3dol ([ADR-0006](0006-key-scoping-and-dol-fixes.md) §3): + +1. **Export `dol.dig.inner_most_key`** and harden `store_trans_path` (raise instead of returning + `None`; fix `dol/dig.py:41` hardcoding `unravel_key`). Needed by D3's free functions. +2. **Fix `dol.content_url`** (`dol/content.py:210-214`) — see D3b for the two-part fix. + +Non-blocking, reported not fixed by us: + +3. **`wrapped_self` has a lost-reference hole** (D1a). dol ships it as the *blessed* pattern and + `xdol`, `unbox` and `lexis` have adopted it, so they inherit a silent failure mode. dol's own + test suite does not cover it — all of `dol/tests/base_test.py` binds the wrapper to a name. +4. **`dol.filesys.is_valid_key`/`validate_key`** (`dol/filesys.py:422,425`) — confirmed-live, and + the best regression sentinel for any future delegation fix. `dol/paths.py:1199-1206` already + carries the hand-rolled fix for the same shape. +5. **`dol#83`'s own "Ask"** requests the two things this ADR rejects (bless `wrapped_self`; ship + a declarative key-method helper). It needs correcting with D1a and D7. + +## Consequences + +**Buys.** The keyed surface is correct *by construction* rather than by a resolution primitive: +D2's stores never call `inner_most_key`, so they are immune to both D1a's lost-reference hole and +D3a's non-`Store`-layer hole — the only form in this ADR that is. `[k]` ergonomics survive. Three +ADR-0009 deferrals collapse into one mechanism. No destructive delegated method. Nothing to +unwind when `dol` lands is-a wrapping. + +**Costs.** + +- **Parallel wrapping.** Wrapping the data store does not wrap its siblings, and s3dol will not + guess the chain. `s3dol.handles(store)` raises rather than silently returning an unwrapped + sibling, so the cost is visible — but it is a real ergonomic tax on the one case (user-applied + key codec) this whole ADR is about. +- **More classes.** Four Layer B readers where there was one, plus factories. +- `sub` and `prefixes` become non-trivial (D3's two obligations) where as leaf methods they were + three lines, and `sub` returns different types on the wrapped and unwrapped branches. +- `url_for` survives as a guarded shim, which is one exception to an otherwise clean rule, and + the guard rides on an incidental `dol` implementation detail. +- s3dol depends on `dol.dig.inner_most_key`, not currently public API. + +**What NOT to do.** + +1. **Do not add a key-taking method to a Layer B class** — not even hardened with `wrapped_self` + (D1a). Add a sibling capability store (D2), put it on `ObjectHandle` (key bound at + construction), or ship a free function (D3). The conformance test allows `url_for` and + nothing else. +2. Do not compose `inner_most_key(store, k)` with `_id_of_key` — it already includes the leaf's + prefix, and composing double-prefixes silently. +3. Do not claim a free function is correct at any wrapper depth (D3a). +4. Do not treat `wrapped_self` as a correctness mechanism (D1a, D9). +5. Do not rely on `isinstance(store, SupportsUrlFor)` to detect a capability. +6. Do not re-propose rebinding delegated methods (D7). +7. Do not invent a pushdown hint protocol for one implementer (D8). +8. Do not attempt to re-derive a user's codec chain onto a sibling store — that is dol#10. diff --git a/misc/docs/state-of-play.md b/misc/docs/state-of-play.md index eb48da9..bf0cbd6 100644 --- a/misc/docs/state-of-play.md +++ b/misc/docs/state-of-play.md @@ -63,12 +63,13 @@ consumer's requirements. | Deferred scope | **#12** — v1.x interfaces + the `s3dol`/`botodol` line | | Live bug found | **#10** — `url_for` emits SigV2 | | Answered | **#5** (multipart / value types), **discussion #6** (extendable KeyError) | -| Upstream blockers | **i2mint/dol#82** (prefix corruption), **i2mint/dol#83** (delegation with unmapped key) | -| Open design questions | **§7** and **§8** of this document | +| Upstream blockers | **i2mint/dol#82** (prefix corruption), **i2mint/dol#83** (delegation with unmapped key), plus `inner_most_key` export + `content_url` resolution (ADR-0006 §3) | +| Resolved design question | **§7** / discussion **#14** → [ADR-0011](decisions/0011-keyed-capability-surface.md) | +| Open design question | **§8** / discussion **#15** — credential and endpoint resolution | **Nothing has been implemented.** The repo still ships v0.1.9 unchanged. -## 3. The ten ADRs, in one paragraph each +## 3. The eleven ADRs, in one paragraph each **[0001 — Four-layer architecture](decisions/0001-layered-architecture.md).** Three layers (`connection` → `base` → `recipes`), mirroring `azuredol` so one adapter in the family reads @@ -108,8 +109,10 @@ join the Mapping surface at all. **[0006 — Prefix normalization, key validity, dol traps](decisions/0006-key-scoping-and-dol-fixes.md).** Prefix normalization is mandatory and comes first. The dol corruption table. The delegation -trap and the `inner_most_key(wrapped_self(self), k)` form. The `EncodingType` prohibition. The -reduced dol upstream list. +trap — accurate on the *mechanism*, but its prescribed remedy +(`inner_most_key(wrapped_self(self), k)`) is **retired** by ADR-0011 §D1a; take the remedy from +there. The `EncodingType` prohibition. The dol upstream list, now including the +`inner_most_key` export and the `content_url` fix. **[0007 — Naming and compatibility](decisions/0007-naming-and-compatibility.md).** New names; `s3dol.store.S3Store` as a deprecated shim removed in v2, doubling as the fix-delivery @@ -128,7 +131,19 @@ restraint. Rule: no new `Protocol` without two implementers. **[0010 — Bucket and bulk operations](decisions/0010-bucket-and-bulk-operations.md).** `on_missing_bucket='assume'` default (no probe, ever); `delete_many` chunking at 1000 with `S3PartialFailure` (not `ExceptionGroup` — 3.10 floor); cascading delete stays explicit and -paginates. +paginates. *Amended by 0011: `delete_many` is a free function, not a store method.* + +**[0011 — Keyed capability surface](decisions/0011-keyed-capability-surface.md).** Resolves §7 / +discussion #14. **Layer B gets no key-taking public methods.** A keyed capability becomes a +**sibling store** keyed through `__getitem__` (`s3dol.handles(store)[k]`, `.urls`, `.info`) — +correct *by construction*, since `__getitem__` is the one thing `dol` maps correctly at every +depth, so no key-resolution primitive is involved at all. Non-keyed operations are free +functions; `url_for` survives as one guarded method for `dol.SupportsUrlFor`. Three findings +drive it: **`azuredol` is robust because it has almost no keyed methods, not because its prefix +lives in the leaf** (§D1); **the `wrapped_self` escape is silently wrong when the wrapper is +unreferenced** (§D1a); and **free functions are not categorically safe either** — they break on +non-`Store` layers (§D3a). Also carries the corrected census: the family defect is +overwhelmingly *latent*, and 12 survey claims were refuted. ## 4. Verified findings — what is actually wrong with v0.1.9 @@ -193,15 +208,20 @@ own rule. Moved to the deferral list. **(g) An absolute `import s3dol < 30 ms` budget.** Arithmetically impossible: `dol` alone is ~61 ms and is a hard dependency of Layer B. Replaced by a delta budget. -## 6. The two open design questions +## 6. The design questions -These are the ones worth discussing before any code is written. Each has a GitHub discussion -with concrete options; this section is the short framing. +- **§7 — Layered transformation and unmapped keys.** Discussion **#14** — **RESOLVED**, see + [ADR-0011](decisions/0011-keyed-capability-surface.md). §7 below is kept as the problem + statement; the answer is in the ADR and summarised at the end of the section. +- **§8 — Credential and endpoint resolution.** Discussion **#15** — **still open**. This is the + one to pick up next. -- **§7 — Layered transformation and unmapped keys.** Discussion: **#14** -- **§8 — Credential and endpoint resolution.** Discussion: **#15** +## 7. ~~Open question 1~~ RESOLVED — layered transformation and the unmapped key -## 7. Open question 1 — layered transformation and the unmapped key +> **Resolved 2026-08-10 in [ADR-0011](decisions/0011-keyed-capability-surface.md)** (discussion +> #14). The framing below stands, with two corrections found while resolving it — see +> §7-resolution at the end. Do not re-litigate the options without reading the ADR: three of the +> five were eliminated by running code, not by argument. ### The problem, precisely @@ -300,7 +320,75 @@ The pushdown half (failure mode 2) is separate and probably needs its own answer protocol (`__iter__(self, *, prefix_hint=…)`) or an explicit `iter_prefix` on the leaf that the wrapper knows to route to. -## 8. Open question 2 — credential and endpoint resolution +### §7-resolution — what was decided, and the two corrections + +Full record: [ADR-0011](decisions/0011-keyed-capability-surface.md). Short form: + +**Decided.** Layer B gets **no key-taking public methods**. A keyed capability becomes a +**sibling store** over the same key space — `s3dol.handles(store)[k]`, `s3dol.urls(store)[k]`, +`s3dol.info(store)[k]` — keyed through `__getitem__`, which is the one thing `dol` maps +correctly at every wrapper depth. Non-keyed operations (`sub`, `prefixes`, `delete_many`, +`delete_bucket`) are free functions taking the store first. `url_for` survives as a single +guarded method purely to satisfy `dol.SupportsUrlFor`. Option D rejected; pushdown closed; +Option E rejected as an *attribute* and adopted as a *store*. + +This is close to the provisional lean in #14 (C + E, with B) — refined by evidence: E's value +comes from being a **store**, not an attribute; C shrinks to the non-keyed operations; B +contributes only its guard. + +**Correction 0 — two design drafts were refuted by testing rather than argument.** + +*Draft 1* kept `handle(k)`/`sub(prefix)` as methods hardened with +`inner_most_key(wrapped_self(self), k)` — the form ADR-0006 §2 prescribed. That form is silently +wrong when **nothing holds a reference to the wrapper**, because a delegated bound method holds +none and `wrapped_self`'s weakref cleanup then removes the registry entry: + +```python +s = KeyCodecs.prefixed('x/')(s3_store(...)); s.handle(k) # correct +KeyCodecs.prefixed('x/')(s3_store(...)).handle(k) # WRONG, silently +``` + +Because the prefix lives in the leaf, the wrong answer is a plausible `str`, so a type check +cannot catch it. + +*Draft 2* concluded from that that **free functions** were the only reliable form. An adversarial +review refuted three of its supporting claims, all re-verified: + +- **"Undetectable" was false** — a guard is writable (`Store.__init__` probes the leaf with + `hasattr(store, 'KeysView')`, so a leaf can record that it was ever wrapped). +- **Free functions are not categorically safe** — with a hand-rolled non-`Store` layer in the + chain they return a plausible wrong key in three configurations, two of which the *method* + form gets right. All `dol`-shipped wrappers are fine. +- **`s3_store(...).handle(k)` is not an instance of the bug at all** — `s3_store` returns a bare + leaf or a *value*-codec wrap, both correct. Only a user-applied **key** codec triggers it. + +Sibling stores need no key-resolution primitive, so they are immune to both holes. That is what +settled it. + +**Correction 1 — the provisional lean above was wrong about Option E.** It claims E works +because key transformation "happens through the Mapping protocol the wrapper already handles +correctly". Verified false at dol 0.3.58: a wrapper does **not** re-wrap a Mapping-valued +attribute — `store.urls` returns an inner-keyed mapping under both class- and instance-wrap. E +needs the same `inner_most_key` call as A and C, so it is a *surface* over one mechanism, not a +third mechanism. That removes its main claimed advantage. + +**Correction 2 — Option C's cited prior art has the bug.** `dol.content_url` resolves with a +flat `getattr(store, 'url_for')(key)` (`dol/content.py:210-214`), so through a key wrap it +returns a URL for the unmapped key. The idiom is prior art; the resolution is not. Fixing it is +now a blocking upstream item ([ADR-0006](decisions/0006-key-scoping-and-dol-fixes.md) §3). + +**Also, two things the framing above got structurally incomplete:** + +- There are **two** delegation routes, not one — `Store.__getattr__` (`dol/base.py:742`) for + instance-wraps and `mk_relative_path_store` subclasses, and `DelegatedAttribute.__get__` + (`dol/base.py:279`) for class-wraps. A fix covering one is a silent no-op on the other. +- **Option D was already rejected upstream** in `dol/misc/docs/dol_issue18_design.md`, with + better reasons than blast radius: rebinding binds to the *innermost* `Wrap`, so under a `Pipe` + stack it does not fix the case it exists to fix. The terminal fix is dol's **is-a wrapping** + (dol#18 Approach C, committed for 0.4/1.0) — which #14's option list did not contain, and + which is why the selection criterion became *correct on 0.3.x AND redundant under is-a*. + +## 8. Open question — credential and endpoint resolution ### The problem @@ -460,6 +548,76 @@ run(True) # 2 of 7 — ['a%0Db','a%20b','a%2520b','a%2Bb','caf%C3%A9','p/x%20y' # moto head_object missing bucket -> Code='NoSuchBucket'; real AWS -> Code='404' ``` +```python +# The temporary-wrapper hole in `wrapped_self` (ADR-0011 §D1a) — the finding that forced +# "zero keyed methods". Needs only dol. +from dol import KeyCodecs, wrapped_self +from dol.base import KvReader +from dol.dig import inner_most_key # NOTE: not exported from `dol` + + +class BR(KvReader): # an s3dol-shaped leaf: it OWNS its prefix, so it has _id_of_key + def __init__(self, d, prefix=""): + self._d = d + self.prefix = f"{prefix.strip('/')}/" if prefix else "" + + def _id_of_key(self, k): + return f"{self.prefix}{k}" + + def _key_of_id(self, i): + return i[len(self.prefix) :] if self.prefix else i + + def __iter__(self): + yield from (self._key_of_id(i) for i in self._d if i.startswith(self.prefix)) + + def __getitem__(self, k): + return self._d[self._id_of_key(k)] + + def m_abs(self, k): # METHOD form — ADR-0006 §2's original prescription + return inner_most_key(wrapped_self(self), k) + + +def f_abs(store, k): # FREE-FUNCTION form — ADR-0011 §D2 + return inner_most_key(store, k) + + +DATA = {"logs/x/b.txt": 2} +mk = lambda: KeyCodecs.prefixed("x/")(BR(DATA, "logs/")) # want 'logs/x/b.txt' + +s = mk() +s.m_abs("b.txt") # 'logs/x/b.txt' correct — wrapper is NAMED, so alive +mk().m_abs("b.txt") # 'logs/b.txt' WRONG — wrapper was a TEMPORARY +# ^ a plausible str, so an isinstance check cannot catch it; +# and `id(leaf) in dol.base._wrapper_backrefs` is False in BOTH the +# collected-temporary and never-wrapped cases, so it is undetectable. + +f_abs(s, "b.txt") # 'logs/x/b.txt' correct +f_abs(mk(), "b.txt") # 'logs/x/b.txt' correct — store is an ARGUMENT, so it stays alive +``` + +```python +# dol.content_url resolves with the OUTER key (ADR-0006 §3 item 6) +from dol import KeyCodecs, content_url + + +class Leaf(dict): + def url_for(self, k): + return f"s3://B/{k}" + + +content_url(KeyCodecs.prefixed("a/")(Leaf)({"a/b": b"v"}), "b") +# -> 's3://B/b' the bytes are at 'a/b' + +# dol.filesys.Files.is_valid_key — confirmed live +import os, tempfile +from dol import Files + +d = tempfile.mkdtemp() +open(os.path.join(d, "a.txt"), "w").write("x") +list(Files(d)) # ['a.txt'] +Files(d).is_valid_key("a.txt") # False <- for a key that exists +``` + `azuredol` is the reference implementation for the layering — read its `base.py` (not its `architecture.md`): normalized prefix in the leaf, `_id_of_key`/`_key_of_id` in the leaf, pushdown via `list_blobs(name_starts_with=…)`, prefix in `__repr__`, sub-stores via @@ -482,10 +640,15 @@ before it can serve as the cross-repo gate ADR-0008 proposes. ## 11. Suggested next steps -1. **Resolve §7 and §8** (discussions #14, #15) — both shape the module boundaries, so they - are worth settling before code. +1. ~~Resolve §7~~ **done** — [ADR-0011](decisions/0011-keyed-capability-surface.md). + **Resolve §8** (discussion #15) — it still shapes the module boundaries, so settle it + before code. 2. **#10 (SigV2)** as a standalone 0.1.x patch. Independent, strictly a fix, immediate value. 3. **`s3dol.diagnose()`** in the same 0.1.x line, so dependents can record what their environment resolves to *before* the resolution order changes. -4. **dol#82 / dol#83** — both block the implementation and benefit every `*dol` adapter. -5. **P0 tier-1 test scaffolding**, then module-by-module per #11. +4. **dol upstreams** — dol#82 / dol#83 benefit every `*dol` adapter; the two ADR-0011 + dependencies (`inner_most_key` export + hardening, `content_url` chain resolution) block the + implementation directly. See [ADR-0006](decisions/0006-key-scoping-and-dol-fixes.md) §3. +5. **P0 tier-1 test scaffolding**, then module-by-module per #11. The reflective + keyed-surface conformance test (ADR-0011 §D5) belongs in P0 — it is cheap and it is what keeps + the §7 decision from decaying.