Key-transform delegation: `is_valid_key` / `validate_key` reject every valid key on `LocalBinaryStore` and `MultiFileStore` (dol #83)

[thorwhalen]

Summary

focal's two stores — LocalBinaryStore (focal/extension_store.py:55) and MultiFileStore (focal/extension_store.py:156) — expose is_valid_key and validate_key, and both answer for the wrong key. is_valid_key returns False for every key the store actually holds, and validate_key raises KeyValidationError on keys that read and write fine.

This is confirmed live — no user-applied key codec is required. It reproduces on a plain MultiFileStore(some_dir). Verified against focal 0.1.11 and dol 0.3.58.

CRUD (__getitem__, __setitem__, __delitem__, __contains__, __iter__) is correct. Only the non-dunder capability methods are wrong.

This is one instance of the family tracked by the umbrella issue i2mint/dol#83, whose root cause is i2mint/dol#18.

Mechanism

dol wraps stores by delegation (has-a), not inheritance. A wrapper that applies a key transform maps keys correctly for the dunder protocol methods, but every other (non-dunder) method is handed the outer, unmapped key. There are two delegation routes:

• Route A — Store.__getattr__ (dol/base.py:742) returns getattr(self.store, attr), the leaf-bound method.
• Route B — delegate_to (dol/base.py:416) installs a DelegatedAttribute descriptor whose __get__ (dol/base.py:279) also returns the leaf-bound method.

focal is hit via Route A, twice. dol.filesys.Files is not a subclass of the file persister — it is mk_relative_path_store(prefix_attr='rootdir')(...) (dol/paths.py:1102), which builds type(name, (PrefixRelativizationMixin, Store), {}), an instance wrapper. So:

MultiFileStore instance          <- wrap_kvs Store layer (postget/preset; no key transform)
  .store -> LocalBinaryStore     <- PrefixRelativizationMixin + Store  (THE key transform: rootdir prefix)
    .store -> Files leaf         <- FileSysCollection; owns is_valid_key / validate_key / _key_pattern

is_valid_key lives only on the leaf FileSysCollection (dol/filesys.py:422) and matches against _key_pattern, which is built from the absolute rootdir. Two __getattr__ hops later, the leaf method receives the relative outer key. Confirmed empirically: an audit over dir(store) checking getattr(store, attr).__self__ is leaf reports exactly three leaf-bound methods — is_valid_key, validate_key, with_relative_paths — and confirms no DelegatedAttribute exists for them (the class only carries _prefix, _prefix_attr_name, _prefix_length), so Route A is the live path.

How focal wraps (census)

construct	count in focal/	what it wraps
mk_relative_path_store	0 direct, 1 inherited	via from dol.filesys import Files (focal/extension_store.py:51), subclassed at :55 — this is the key transform that causes the bug
PrefixRelativizationMixin	0 direct	present in the MRO of the inherited Files
wrap_kvs	1 (focal/extension_store.py:155)	multi_extension_wrap — value-side only (postget/preset), no key transform
KeyCodecs	0	—
prefixless_view	0	—
filt_iter	0	—

Note the second row of that table matters for triage: the wrap_kvs layer does not contribute to the key offset. It adds a second delegation hop and it strips codecs from with_relative_paths() (below), but the entire key mismatch originates in the Files relativization underneath. Verified separately that postget/preset receive the outer, relative key, so focal's extension-dispatch logic (get_extension, focal/extension_store.py:134) is correct and is not implicated.

Also worth flagging for users: focal/__init__.py does from dol import *, so focal.Files, focal.FilesReader etc. are re-exported and carry the identical defect. Those are dol's to fix; the two symbols owned by focal are the ones in the table below.

Affected symbols

symbol	defined	verdict	severity	key arg
LocalBinaryStore.is_valid_key	inherited (focal/extension_store.py:55)	confirmed-live	silent-wrong-result	k (pos 1)
LocalBinaryStore.validate_key	inherited (focal/extension_store.py:55)	confirmed-live	wrong-scope	k (pos 1)
MultiFileStore.is_valid_key	inherited (focal/extension_store.py:156)	confirmed-live	silent-wrong-result	k (pos 1)
MultiFileStore.validate_key	inherited (focal/extension_store.py:156)	confirmed-live	wrong-scope	k (pos 1)
MultiFileStore.with_relative_paths	inherited (focal/extension_store.py:156)	confirmed-live	value-side	none (scope-wide)

Repro (REAL — runs today, no mocks, no synthetic stand-in)

import os, tempfile
from focal.extension_store import LocalBinaryStore, MultiFileStore

tmp = tempfile.mkdtemp()

s = LocalBinaryStore(tmp)
s["a.bin"] = b"hello"
assert list(s) == ["a.bin"] and s["a.bin"] == b"hello"   # CRUD is fine

print(s.is_valid_key("a.bin"))                            # False  <-- WRONG (key exists)
print(s.is_valid_key(os.path.join(tmp, "a.bin")))         # True   <-- not a key of s

d = MultiFileStore(tmp)
d["x.json"] = {"a": 1}
assert d["x.json"] == {"a": 1}                            # codecs work

print(d.is_valid_key("x.json"))                           # False  <-- WRONG
try:
    d.validate_key("x.json")
except Exception as e:
    print(type(e).__name__, e)
    # KeyValidationError 'Key not valid (usually because does not exist
    #  or access not permitted): x.json'   -- on a key that EXISTS

r = d.with_relative_paths()
print(repr(r["x.json"]))                                  # b'{"a": 1}'  <-- codecs silently dropped

Actual output:

False
True
False
KeyValidationError 'Key not valid (usually because does not exist or access not permitted): x.json'
b'{"a": 1}'

User-visible consequence

Nothing is destroyed. No data loss, no writes to the wrong location, no deletions. The failure is one of reporting, and it is silent in the direction that matters most:

1. is_valid_key returns False for 100% of the store's real keys. Any caller using it as a guard — if store.is_valid_key(k): ..., a filter, a pre-flight validation in a pipeline, a UI enable/disable check — rejects every legitimate key with no error raised. The store looks empty of valid keys while list(store) happily enumerates them.
2. validate_key raises KeyValidationError on keys that exist. Loud, but pointed at the wrong culprit: the message names a key that is demonstrably fine, sending debugging in the wrong direction.
3. with_relative_paths() silently downgrades MultiFileStore to raw bytes. The returned store has the right keys, so it looks correct — but r["x.json"] yields b'{"a": 1}' where d["x.json"] yields {"a": 1}. Code that stores the result and keeps reading gets bytes where it expects decoded objects. This is the value-side mirror of the same delegation defect.

Two things deliberately not claimed here:

• The false positive (is_valid_key(<absolute path>) -> True) is not caused by delegation. _key_pattern for a subpath-less store is a bare rootdir prefix match, so rootdir + abspath still matches after any correct key mapping. That is a separate upstream looseness in dol/filesys.py, not a focal bug — mentioned only so it isn't mistaken for part of this fix.
• Internal CRUD is untouched. dol's __getitem__/__setitem__/__delitem__ are decorated with validate_key_and_raise_key_error_on_exception (dol/filesys.py:339) on the leaf class, where they correctly receive the already-mapped absolute key.

Remediation

The real fix is upstream, and focal needs no code change once it lands. focal defines neither is_valid_key nor validate_key; it only subclasses dol.filesys.Files. Precisely what would repair this:

mk_relative_path_store (dol/paths.py:1102) builds type(store_cls.__name__, (PrefixRelativizationMixin, Store), {}) with no is_valid_key / validate_key override. Adding key-aware overrides on that generated class — so they map the outer key inward before consulting the leaf — fixes dol.filesys.Files, and LocalBinaryStore inherits the fix, and MultiFileStore's wrap_kvs layer forwards it unchanged (that layer adds no key transform). One upstream change closes all four key-taking rows in the table above.

with_relative_paths needs the #18 treatment separately: FileSysCollection.with_relative_paths (dol/filesys.py:440) does return with_relative_paths(self) where self is the leaf, so it must use wrapped_self(self) to see the outer wrapper.

If focal wants a local shim before the upstream fix lands, the escape hatch is inner_most_key(wrapped_self(self), k):

from dol import wrap_kvs, wrapped_self          # wrapped_self IS exported from dol
from dol.dig import inner_most_key              # inner_most_key is NOT exported from dol
from dol.filesys import Files, KeyValidationError, _dflt_not_valid_error_msg


class LocalBinaryStore(Files):
    # ... existing __init__ unchanged ...

    def is_valid_key(self, k):
        _id = inner_most_key(wrapped_self(self), k)
        if not isinstance(_id, str):   # see trap #2 below
            return False
        return self.store.is_valid_key(_id)

    def validate_key(self, k, err_msg_format=_dflt_not_valid_error_msg,
                     err_type=KeyValidationError):
        if not self.is_valid_key(k):
            raise err_type(err_msg_format.format(k))

Verified: with this shim, is_valid_key("a.bin") -> True on both LocalBinaryStore and MultiFileStore, validate_key stops raising on valid keys, and the JSON/txt round-trip is unaffected.

Two traps with inner_most_key, both of which will bite you silently:

1. It walks the whole chain, including the leaf's own _id_of_key. It therefore replaces self._id_of_key(k) — never compose the two, or the key gets transformed twice.
2. It returns None, silently, when no layer in the chain defines _id_of_key. The isinstance(_id, str) check above is mandatory, not defensive padding — without it you pass None into a regex match.

Whichever route is taken, a regression test belongs in focal/tests/test_extension_store.py (currently it only covers the JSON/txt round-trip, which is exactly why this went unnoticed):

def test_is_valid_key_agrees_with_iteration():
    import tempfile
    from focal.extension_store import MultiFileStore
    d = MultiFileStore(tempfile.mkdtemp())
    d["x.json"] = {"a": 1}
    for k in d:
        assert d.is_valid_key(k), f"{k} is in the store but is_valid_key says no"
        d.validate_key(k)  # must not raise

References

• Umbrella: i2mint/dol#83 — key-transform wrappers delegate capability methods with the unmapped key
• Root cause: i2mint/dol#18 — wrap_kvs wraps the instance, but self inside the instance's methods is not wrapped
• Existing precedent for the escape hatch inside dol itself: dol/filesys.py:766 and dol/filesys.py:825 already use inner_most_key for exactly this reason

---

Note on in-flight upstream fixes (added when filing)

Two dol PRs are open and change details referenced above:

• i2mint/dol#84 — inner_most_key and unravel_key
  become importable from dol directly (no more from dol.dig import ...), and
  inner_most_key now raises instead of returning None when no layer of the chain
  defines _id_of_key. If you write a local shim, the isinstance(_id, str) guard becomes
  unnecessary once that lands — but the "it replaces _id_of_key, never composes with it" trap
  still applies.
• i2mint/dol#85 — fixes dol.content_url to
  resolve the key through wrapping layers, and makes mk_relative_path_store install
  key-mapping is_valid_key/validate_key. Any finding above that is inherited from
  dol.filesys.Files is repaired by #85 with no change needed in this repo — this issue will
  be closed with verification once it merges.

Design context for why the ecosystem-wide answer is not "sprinkle wrapped_self everywhere":
i2mint/s3dol#14 and
s3dol ADR-0011.
Short version: wrapped_self is a best-effort guardrail with its own silent failure mode (it
degrades to the raw leaf when nothing holds a reference to the wrapper), so the durable fix is
to have no key-taking methods rather than to harden each one.

[thorwhalen]

Verified: 4 of the 5 findings are fixed upstream — no change needed in this repo

i2mint/dol#85 merged (dol master). As predicted in the Remediation section,
mk_relative_path_store now maps the key inward before consulting the leaf, so
LocalBinaryStore and MultiFileStore inherit the fix.

Verified against dol master just now:

LocalBinaryStore:
  list           -> ['a.bin']
  is_valid_key   -> True          (was False)
  validate_key   -> no raise      (was KeyValidationError)
MultiFileStore:
  read codec     -> {'a': 1}
  is_valid_key   -> True          (was False)
  validate_key   -> no raise      (was KeyValidationError)
  all(is_valid_key(k) for k in d) -> True

Fixed: LocalBinaryStore.is_valid_key, LocalBinaryStore.validate_key,
MultiFileStore.is_valid_key, MultiFileStore.validate_key.

Still open: with_relative_paths drops the codecs

d.with_relative_paths()['x.json']  ->  b'{"a": 1}'     (want {'a': 1})

This one is the other direction of the same root cause — not "a method receives an unmapped
key" (#83) but "a method's self is the unwrapped leaf" (i2mint/dol#18).
FileSysCollection.with_relative_paths does return with_relative_paths(self) where self is
the leaf, so it re-wraps the leaf and discards MultiFileStore's wrap_kvs codec layer
entirely.

It is dol's to fix (the method is defined there), and the fix is the #18 treatment —
wrapped_self(self). Worth knowing before relying on that: wrapped_self has its own silent
failure mode when nothing holds a reference to the wrapper (i2mint/dol#18), so this is not a
one-liner anyone should apply without reading that thread.

Leaving this issue open, scoped to with_relative_paths only. Nothing to do in focal for
either half.

A regression test is still worth adding here, since it is what would have caught the original
bug:

def test_is_valid_key_agrees_with_iteration():
    import tempfile
    from focal.extension_store import MultiFileStore
    d = MultiFileStore(tempfile.mkdtemp())
    d["x.json"] = {"a": 1}
    for k in d:
        assert d.is_valid_key(k), f"{k} is in the store but is_valid_key says no"
        d.validate_key(k)

Requires dol >= 0.3.59.