fix: two delegation bugs in dol's own code (content_url, filesys key validation)

dol/content.py

@@ -63,6 +63,7 @@
 )
 
 from dol.base import KvPersister
+from dol.base import DelegatedAttribute as _DelegatedAttribute
 
 #: The ``_tag`` discriminator value carried on the JSON wire form (cross-language parity).
 CONTENT_REF_TAG = "ContentRef"
@@ -192,6 +193,71 @@ def _url_of(ref_or_key: Any) -> Optional[str]:
     return None
 
 
+_MAX_STORE_CHAIN_DEPTH = 100  # no real wrap stack is remotely this deep
+
+
+def _statically_defines_url_for(obj) -> bool:
+    """True if ``obj`` itself provides ``url_for`` (as opposed to forwarding it inward).
+
+    Looks the attribute up **without invoking descriptors**, so a class-wrap's
+    ``DelegatedAttribute`` is seen as itself rather than as the inner bound method -- a layer
+    that only *forwards* ``url_for`` is not the provider.
+
+    This is a hand-rolled ``inspect.getattr_static``. It agrees with it on every shape that
+    matters here (instance attribute, plain method, ``__slots__``, class-wrap, instance-wrap,
+    ``__getattr__``-provided) and is ~7x faster, which matters because ``content_url`` is
+    called per object. Anything this misses falls through to the plain-``getattr`` fallback
+    in :func:`content_url`, so a miss costs correctness nothing.
+
+    NOTE: ``DelegatedAttribute`` is defined twice in dol -- ``dol.base`` (the one the wrap
+    machinery constructs) and an unused duplicate in ``dol.util``. If a wrap path ever
+    switches to the other copy this check silently stops working, so they must not diverge.
+    """
+    d = getattr(obj, "__dict__", None)
+    if d is not None and "url_for" in d:
+        return not isinstance(d["url_for"], _DelegatedAttribute)
+    for klass in type(obj).__mro__:
+        attr = klass.__dict__.get("url_for")
+        if attr is not None:
+            return not isinstance(attr, _DelegatedAttribute)
+    return False
+
+
+def _url_for_provider_and_key(store: Any, key: str):
+    """The layer that owns ``url_for``, and ``key`` expressed in *that layer's* key space.
+
+    ``getattr(store, 'url_for')`` on a wrapped store returns the method bound to an inner
+    layer, so handing it the outer key addresses the wrong object. Walk the ``.store`` chain
+    to find the layer that actually defines ``url_for``, applying every *outer* layer's
+    ``_id_of_key`` on the way -- and stopping there, because that layer applies its own.
+
+    Returns ``(None, key)`` -- with ``key`` unchanged -- when the walk finds no provider, so
+    the caller can fall back to plain ``getattr``.
+
+    The walk is bounded two ways. ``seen`` catches a chain that points back at itself; the
+    depth cap catches one that never repeats and never ends, which is what a ``MagicMock``
+    does (every ``.store`` mints a fresh child).
+    """
+    layer, k, seen = store, key, set()
+    for _ in range(_MAX_STORE_CHAIN_DEPTH):
+        if layer is None or id(layer) in seen:
+            break
+        seen.add(id(layer))
+        inner = getattr(layer, "store", None)
+        if inner is None:
+            # Innermost layer: nothing left to forward to, so it is the provider if it has
+            # ``url_for`` at all. Checking ``.store`` first keeps the unwrapped case --
+            # by far the common one -- on a single cheap ``getattr``.
+            return layer, k
+        if _statically_defines_url_for(layer):
+            return layer, k
+        id_of_key = getattr(layer, "_id_of_key", None)
+        if callable(id_of_key):
+            k = id_of_key(k)
+        layer = inner
+    return None, key
+
+
 def content_url(store: Any, ref_or_key: Any) -> Optional[str]:
     """A fetchable URL for content, resolved **on demand**.
 
@@ -206,12 +272,33 @@ def content_url(store: Any, ref_or_key: Any) -> Optional[str]:
     True
     >>> content_url({}, ContentRef('k1', url='https://carried/k1'))  # ref carries its own
     'https://carried/k1'
+
+    The key is resolved **through any wrapping layers**, so a URL addresses the same object
+    ``store[key]`` reads. Without this, a key-transforming wrap would hand the backend the
+    outer key and silently return a URL for a different object:
+
+    >>> from dol import KeyCodecs
+    >>> wrapped = KeyCodecs.prefixed('a/')(Served)({'a/k1': b'v'})
+    >>> wrapped['k1']
+    b'v'
+    >>> content_url(wrapped, 'k1')
+    'https://cdn.example/a/k1'
     """
     carried = _url_of(ref_or_key)
     if carried:
         return carried
+    outer_key = _key_of(ref_or_key)
+    provider, key = _url_for_provider_and_key(store, outer_key)
+    if provider is not None:
+        url_for = getattr(provider, "url_for", None)
+        if callable(url_for):
+            return url_for(key)
+    # Fallback to plain duck typing, with the key as given. The walk above looks attributes
+    # up statically and so cannot see a ``url_for`` supplied by ``__getattr__`` (a proxy, a
+    # lazy-client wrapper). Returning None there would be a silent wrong answer; this keeps
+    # such stores working exactly as they did before.
     url_for = getattr(store, "url_for", None)
-    return url_for(_key_of(ref_or_key)) if callable(url_for) else None
+    return url_for(outer_key) if callable(url_for) else None
 
 
 def _ref(

dol/paths.py

@@ -1207,6 +1207,32 @@ def _id_of_key(self, k):
 
         cls._id_of_key = _id_of_key
 
+    # Key-validation methods must see the INNER (absolute) key.
+    #
+    # ``is_valid_key``/``validate_key`` are defined on the wrapped class and reached through
+    # ``Store.__getattr__``, which hands the leaf the OUTER (relativized) key. The leaf then
+    # matches it against a pattern built from the absolute path, so
+    # ``Files(d).is_valid_key('a.txt')`` was False for a key that demonstrably exists. Map the
+    # key first, exactly as the ``with_key_validation`` branch above already does by hand.
+    for _method_name in ("is_valid_key", "validate_key"):
+        if hasattr(store_cls, _method_name) and _method_name not in cls.__dict__:
+
+            def _key_mapped(self, k, *args, __name=_method_name, **kwargs):
+                # Use the mixin's *unvalidated* mapping, not ``self._id_of_key``. Under
+                # ``with_key_validation=True`` the latter is redefined above to RAISE
+                # ``KeyError`` on an invalid key -- which would make ``is_valid_key`` raise
+                # for exactly the input it exists to answer "no" for, and would change
+                # ``validate_key``'s exception type.
+                _id = PrefixRelativizationMixin._id_of_key(self, k)
+                return getattr(self.store, __name)(_id, *args, **kwargs)
+
+            _key_mapped.__name__ = _method_name
+            _key_mapped.__qualname__ = f"{cls.__name__}.{_method_name}"
+            _key_mapped.__doc__ = (
+                f"``{_method_name}`` on the inner key -- see ``mk_relative_path_store``."
+            )
+            setattr(cls, _method_name, _key_mapped)
+
     # if __module__ is not None:
     #     cls.__module__ = __module__
 

dol/tests/test_content.py

@@ -169,3 +169,113 @@ def test_backend_injection_dict_vs_class():
     cas = with_content_addressing(backing)
     ref = cas.add(b"shared-bytes")
     assert backing[ref.item_id] == b"shared-bytes"  # writes land in the injected backend
+
+
+# -------------------------------------------------------------------------------------
+# content_url must resolve the key through wrapping layers
+#
+# It used to do a flat ``getattr(store, 'url_for')(key)``. On a wrapped store that returns
+# the method bound to an inner layer, so the backend received the OUTER key and returned a
+# URL for a different object than ``store[key]`` reads.
+
+
+class _Served(dict):
+    def url_for(self, key):
+        return f"https://cdn/{key}"
+
+
+def test_content_url_resolves_through_a_key_wrap():
+    from dol import KeyCodecs, Pipe, content_url
+
+    # class-wrap: url_for reaches the leaf via a DelegatedAttribute
+    wrapped = KeyCodecs.prefixed("a/")(_Served)({"a/k": b"v"})
+    assert wrapped["k"] == b"v"
+    assert content_url(wrapped, "k") == "https://cdn/a/k"
+
+    # instance-wrap: url_for reaches the leaf via Store.__getattr__
+    wrapped = KeyCodecs.prefixed("a/")(_Served({"a/k": b"v"}))
+    assert content_url(wrapped, "k") == "https://cdn/a/k"
+
+    # stacked
+    stacked = Pipe(KeyCodecs.prefixed("a/"), KeyCodecs.prefixed("b/"))(
+        _Served({"a/b/k": b"v"})
+    )
+    assert content_url(stacked, "k") == "https://cdn/a/b/k"
+
+
+def test_content_url_unchanged_for_unwrapped_and_keyless_wraps():
+    from dol import KeyCodecs, content_url, filt_iter, wrap_kvs
+
+    assert content_url(_Served({"k": b"v"}), "k") == "https://cdn/k"
+    assert content_url({}, "k") is None
+    # wraps that do not change keys must not change the URL
+    assert (
+        content_url(wrap_kvs(_Served({"k": b"v"}), obj_of_data=lambda v: v), "k")
+        == "https://cdn/k"
+    )
+    assert (
+        content_url(filt_iter(_Served({"k": b"v"}), filt=lambda k: True), "k")
+        == "https://cdn/k"
+    )
+
+
+def test_content_url_does_not_double_apply_the_providers_own_transform():
+    """A backend whose own ``url_for`` applies ``self._id_of_key`` (e.g. a store that owns
+    a prefix) must receive the key in ITS key space, not the fully-resolved one."""
+    from dol import KeyCodecs, content_url
+    from dol.base import KvReader
+
+    class Prefixed(KvReader):
+        def __init__(self, prefix=""):
+            self.prefix = prefix
+
+        def _id_of_key(self, k):
+            return f"{self.prefix}{k}"
+
+        def _key_of_id(self, i):
+            return i[len(self.prefix) :]
+
+        def __iter__(self):
+            yield from ()
+
+        def __getitem__(self, k):
+            return b"v"
+
+        def url_for(self, k):
+            return f"https://s3/{self._id_of_key(k)}"
+
+    assert content_url(Prefixed("logs/"), "f") == "https://s3/logs/f"
+    assert content_url(KeyCodecs.prefixed("x/")(Prefixed("logs/")), "f") == (
+        "https://s3/logs/x/f"
+    )
+
+
+def test_content_url_terminates_on_pathological_chains():
+    """The chain walk must be bounded. A ``MagicMock`` mints a fresh child for every
+    ``.store``, and a self-referential store cycles -- both used to hang forever."""
+    from unittest.mock import MagicMock
+
+    from dol import content_url
+
+    content_url(MagicMock(), "k")  # must simply return
+
+    class SelfStore:
+        @property
+        def store(self):
+            return self
+
+    assert content_url(SelfStore(), "k") is None
+
+
+def test_content_url_still_finds_a_dynamically_provided_url_for():
+    """``url_for`` supplied via ``__getattr__`` is invisible to a static lookup. Returning
+    None there would be a silent wrong answer, so we fall back to plain duck typing."""
+    from dol import content_url
+
+    class Dyn(dict):
+        def __getattr__(self, name):
+            if name == "url_for":
+                return lambda key: f"https://dyn/{key}"
+            raise AttributeError(name)
+
+    assert content_url(Dyn({"k": 1}), "k") == "https://dyn/k"

dol/tests/test_filesys.py

@@ -247,3 +247,30 @@ def test_subfolder_stores():
         assert set(folder2_store.keys()) == {"this.txt", "over.json"}
         assert folder2_store["this.txt"] == b"that"
         assert folder2_store["over.json"] == b"there"
+
+
+def test_is_valid_key_sees_the_inner_key(tmpdir):
+    """``Files`` is ``mk_relative_path_store(FileBytesPersister)``, so ``is_valid_key`` was
+    reached through ``Store.__getattr__`` with the RELATIVE key while the leaf matched it
+    against a pattern built from the absolute path -- returning False for keys that exist."""
+    import os
+
+    from dol import Files, TextFiles
+
+    rootdir = str(tmpdir)
+    os.makedirs(os.path.join(rootdir, "sub"), exist_ok=True)
+    with open(os.path.join(rootdir, "a.txt"), "w") as fp:
+        fp.write("x")
+    with open(os.path.join(rootdir, "sub", "b.txt"), "w") as fp:
+        fp.write("y")
+
+    s = Files(rootdir)
+    nested_key = os.path.join("sub", "b.txt")  # separator differs on Windows
+    assert sorted(s) == ["a.txt", nested_key]
+    # the invariant that was broken: every key the store yields is a valid key
+    assert all(s.is_valid_key(k) for k in s)
+    assert s.is_valid_key("a.txt")
+    assert s.is_valid_key(nested_key)
+    s.validate_key("a.txt")  # must not raise
+    assert TextFiles(rootdir).is_valid_key("a.txt")
+    assert s["a.txt"] == b"x"  # reads unaffected

dol/tests/test_paths.py

@@ -91,3 +91,31 @@ def test_string_template_simple():
     VersionedFile = st.dict_to_namedtuple({"i01_": "life", "version": 42})
     assert VersionedFile == namedtuple("VersionedFile", ["i01_", "version"])("life", 42)
     assert st.namedtuple_to_dict(VersionedFile) == {"i01_": "life", "version": 42}
+
+
+def test_key_validation_wrappers_coexist_with_with_key_validation():
+    """``with_key_validation=True`` redefines ``_id_of_key`` to RAISE on an invalid key, so
+    the key-mapping wrappers must use the unvalidated mapping -- otherwise ``is_valid_key``
+    raises for exactly the input it exists to answer 'no' for."""
+    from dol.paths import mk_relative_path_store
+
+    class Leaf(dict):
+        _prefix = "/ROOT/"
+
+        def is_valid_key(self, k):
+            return k.startswith("/ROOT/") and k.endswith(".txt")
+
+        def validate_key(self, k):
+            if not self.is_valid_key(k):
+                raise ValueError(k)
+
+    s = mk_relative_path_store(Leaf, with_key_validation=True)()
+    assert s.is_valid_key("b.txt") is True
+    assert s.is_valid_key("b.json") is False  # a bool, not a raise
+    s.validate_key("b.txt")
+    try:
+        s.validate_key("b.json")
+    except ValueError:
+        pass  # the LEAF's exception type, not KeyError from the validating _id_of_key
+    else:
+        raise AssertionError("validate_key should have raised ValueError")