fix: two delegation bugs in dol's own code (content_url, filesys key validation) - #85
Merged
thorwhalen merged 3 commits intoAug 10, 2026
Merged
Conversation
…validation) Both are instances of the same root cause (#83): a wrapper delegates a non-dunder method to an inner layer bound to that layer, so it receives the OUTER, unmapped key. 1. `content_url(store, ref_or_key)` did a flat `getattr(store, 'url_for')(key)`. On a wrapped store that returns the method bound to an inner layer, so the backend got the outer key and returned a URL for a different object than `store[key]` reads: wrapped = KeyCodecs.prefixed('a/')(Served)({'a/k': b'v'}) wrapped['k'] # b'v' -> reads a/k content_url(wrapped, 'k') # 'https://cdn/k' -> WRONG, addresses k It now walks the .store chain to the layer that actually defines `url_for`, applying each OUTER layer's `_id_of_key` on the way and stopping there -- because that layer applies its own. Resolving all the way would double-apply the provider's transform, which matters for a backend that owns a prefix (an s3dol store, the case `dol/content.py`'s docstring names). Uses `inspect.getattr_static` to tell a real `url_for` from a class-wrap's forwarding `DelegatedAttribute`. None of the existing tests caught this: they are all identity-keyed. 2. `Files(d).is_valid_key(k)` returned **False** for a key that demonstrably exists, breaking the invariant `all(s.is_valid_key(k) for k in s)`. `Files` is `mk_relative_path_store(FileBytesPersister)`, so `is_valid_key`/`validate_key` were reached through `Store.__getattr__` with the RELATIVE key while the leaf matched them against a pattern built from the absolute path. `mk_relative_path_store` now maps the key first -- the same thing its `with_key_validation=True` branch already did by hand. Tests: 4 new cases (class-wrap, instance-wrap, Pipe, no-double-apply, keyless wraps unchanged) plus a filesys case asserting the `all(is_valid_key(k) for k in s)` invariant. Verified: full suite 513 passed / 3 skipped. Dependents gate byte-for-byte identical to HEAD (xdol 4, pdfdol 23+1, chromadol 2, focal 1, dol_cookbook 1, ftpdol 1); the rest fail on absent backend SDKs both before and after. (2) is also what `focal` inherits via `LocalBinaryStore(Files)`.
This was referenced Aug 10, 2026
9 tasks
Three defects found by an independent refutation pass, all in the new `_url_for_provider_and_key`, plus its cost. None were caught by the tests. 1. UNBOUNDED WALK. `while layer is not None` had no cycle detection and no depth bound, so `content_url(MagicMock(), k)` hung forever -- every `.store` on a mock mints a fresh child -- as did a store whose `.store` returns itself. Master was bounded by Python's recursion limit; this was not. Any downstream test passing a mock store to `content_url` would have wedged CI. Now bounded by an id-based `seen` set (cycles) AND a depth cap (chains that never repeat and never end). 2. STATIC LOOKUP BROKE DUCK TYPING. A store providing `url_for` via `__getattr__` (proxies, lazy-client wrappers) is invisible to a static lookup, so the walk found no provider and `content_url` returned None -- which callers read as "no URL exists". That is exactly the silent-wrong-answer failure mode this PR set is against. Now falls back to plain `getattr(store, 'url_for')` with the original key when the walk finds nothing, making the behaviour a strict superset of master's. 3. COLLISION WITH `with_key_validation=True`. That option redefines `_id_of_key` to RAISE `KeyError` on an invalid key, and the new `is_valid_key`/`validate_key` wrappers called it -- so the predicate raised for precisely the input it exists to answer "no" for, and `validate_key`'s exception type changed from the leaf's to `KeyError`. The wrappers now use `PrefixRelativizationMixin._id_of_key`, the unvalidated mapping. Also: `content_url` was 14.5x slower (3.4us -> 49.5us). Two causes, both fixed -- `import inspect` + `from dol.base import ...` ran on every call (hoisted to module level), and `getattr_static` was called even for unwrapped stores. Checking `.store` before the static lookup restores the unwrapped fast path (3.2us, master parity), and replacing `getattr_static` with a hand-rolled MRO scan makes the wrapped path ~7x cheaper. The scan agrees with `getattr_static` on every shape that matters (instance attribute, plain method, __slots__, class-wrap, instance-wrap, __getattr__-provided), and anything it misses falls through to the plain-getattr fallback, so a miss costs correctness nothing. Documented the `DelegatedAttribute` duplication (`dol.base` vs the unused `dol.util` copy) that the isinstance check depends on. Tests: 3 new cases -- pathological chains terminate, a dynamically-provided `url_for` is still found, and `with_key_validation=True` coexists with the wrappers. 516 passed / 3 skipped.
thorwhalen
added a commit
that referenced
this pull request
Aug 10, 2026
Discussion #86 captured this, but a discussion is easy to lose and hard to cite from code review. misc/docs/ is where dol's durable design records live, so this lands next to dol_issue18_design.md (its inverse) and dol_issue10_design.md. Records, with the running-code evidence for each: - TWO delegation routes, not one -- Store.__getattr__ AND DelegatedAttribute.__get__. A fix covering one is a silent no-op on the other, which is why several past fixes had to be applied twice. - The 13-package census, stated accurately: the defect is overwhelmingly LATENT (it bites only when a user applies a key codec), and 12 survey claims were refuted outright. An earlier downstream draft claimed the family was actively destroying data; it is not, and the record says so. - Options A-F with what each actually costs: A wrapped_self -- has its own silent hole: degrades to the raw leaf when nothing holds a strong reference to the wrapper, and where the leaf owns a prefix the wrong answer is a plausible str. CPython 3.10-3.14. Detectable via the KeysView probe, so it can be made loud. B declarative -- dol already ships this (ingoing_key_methods), untested and broken for leaf-defined methods. The reflective test is the part that holds, not the registry. C free funcs -- break on a non-Store layer, where the method form is right. dol's own instance (content_url) had the bug until #85. D rebind -- rejected in dol_issue18_design.md with evidence; binds to the INNERMOST wrap, so it misses the Pipe case it exists to fix. E parallel maps -- broken as an ATTRIBUTE (a wrapper does not re-wrap one), correct by construction as a sibling STORE keyed through __getitem__. The only option needing no resolution primitive. F is-a -- the terminal fix; dissolves #83, #18 and #6 together. Section 5 is the explicit carry-forward list for a future redesign: the has-a/is-a choice is one decision not three; a wrapper must be able to express "this method takes a key"; the two routes must be unified; key mapping needs an inverse (inward->outward has no supported helper); weakref backrefs are the wrong substrate for correctness; non-Store layers exist in the wild; and "how many keyed methods does this force adapters to write?" is a design metric worth tracking (azuredol has ~0 and is clean). Indexed from CLAUDE.md and dol_misc_docs_guide.md. All file:line citations verified against the current source.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two live bugs in dol itself, both instances of #83's root cause: a wrapper delegates a
non-dunder method to an inner layer bound to that layer, so it receives the outer, unmapped
key.
1.
content_urlreturns a URL for the wrong objectcontent_urldid a flatgetattr(store, 'url_for')(key). It now walks the.storechain tothe layer that actually defines
url_for, applying each outer layer's_id_of_keyon theway and stopping there — because that layer applies its own.
Stopping matters. Resolving all the way would double-apply the provider's own transform, which
breaks exactly the backend
dol/content.py's docstring names — ans3dolstore, which owns aprefix:
Uses
inspect.getattr_staticto distinguish a realurl_forfrom a class-wrap's forwardingDelegatedAttribute— without it the walk stops at the wrapper and never maps the key.None of the existing tests caught this: they are all identity-keyed.
2.
Files(d).is_valid_key(k)is False for a key that existsFilesismk_relative_path_store(FileBytesPersister), sois_valid_key/validate_keywerereached through
Store.__getattr__with the relative key, while the leaf matched themagainst a pattern built from the absolute path. This breaks the invariant
all(s.is_valid_key(k) for k in s).mk_relative_path_storenow maps the key before delegating — the same thing itswith_key_validation=Truebranch (dol/paths.py:1199-1206) already did by hand for_id_of_key.focalinherits this viaLocalBinaryStore(Files).Verification
Pipe, the no-double-apply case, keyless wrapsunchanged, and the
all(is_valid_key(k) for k in s)invariant.2, focal 1, dol_cookbook 1, ftpdol 1. mongodol/sqldol/hfdol/dropboxdol/sshdol fail on absent
backend SDKs both before and after.
Part of #83. Independent of #84 (different modules); either can merge first.