fix(dig): resolution failures raise instead of returning None; export inner_most_key

dol/__init__.py

@@ -186,7 +186,11 @@ def ihead(store, n=1):
     # PathMappedData,  # A mapping that extracts data from a mapping according to paths
 )
 
-from dol.dig import trace_getitem  # trace getitem calls, stepping through the layers
+from dol.dig import (
+    trace_getitem,  # trace getitem calls, stepping through the layers
+    inner_most_key,  # resolve a key through every layer of a wrapped store
+    unravel_key,  # ... same, but yielding the key at each layer
+)
 
 from dol.explicit import ExplicitKeyMap, invertible_maps, KeysReader
 

dol/dig.py

@@ -35,12 +35,30 @@ def recursive_get_attr(store, attr, default=None):
 
 
 def store_trans_path(store, arg, method):
+    """Yield ``arg`` transformed by ``method`` at each layer, outermost first.
+
+    Walks the ``.store`` chain, applying ``store.<method>`` at every layer that defines it.
+
+    >>> from dol import wrap_kvs
+    >>> s = wrap_kvs({'a.txt': 1}, id_of_key=lambda k: k + '.txt')
+    >>> list(store_trans_path(s, 'a', '_id_of_key'))
+    ['a.txt', 'a.txt']
+
+    Yields nothing when no layer defines ``method`` -- which is why :func:`inner_most`
+    raises rather than returning the ``None`` that an empty walk would otherwise produce.
+
+    >>> list(store_trans_path({}, 'a', '_id_of_key'))
+    []
+    """
     f = getattr(store, method, None)
     if f is not None:
         trans_arg = f(arg)
         yield trans_arg
         if hasattr(store, "store"):
-            yield from unravel_key(store.store, trans_arg)
+            # NOTE: recurse with the SAME ``method``. This used to hardcode ``unravel_key``
+            # (i.e. ``_id_of_key``), so ``unravel_val``/``inner_most_val`` applied
+            # ``_data_of_obj`` at the top layer and ``_id_of_key`` at every deeper one.
+            yield from store_trans_path(store.store, trans_arg, method)
 
 
 def print_trans_path(store, arg, method, with_type=False):
@@ -59,8 +77,42 @@ def last_element(gen):
     return x
 
 
-def inner_most(store, arg, method):
-    return last_element(store_trans_path(store, arg, method))
+def inner_most(store, arg, method, default=no_default):
+    """The value of ``arg`` after every layer's ``method`` has been applied.
+
+    >>> from dol import wrap_kvs
+    >>> s = wrap_kvs({'a.txt': 1}, id_of_key=lambda k: k + '.txt')
+    >>> inner_most(s, 'a', '_id_of_key')
+    'a.txt'
+
+    **Raises when no layer defines ``method``**, instead of silently returning ``None``:
+
+    >>> inner_most({}, 'a', '_id_of_key')
+    Traceback (most recent call last):
+      ...
+    AttributeError: No layer of dict defines '_id_of_key', so 'a' cannot be resolved. ...
+
+    A ``None`` here is the worst possible answer: callers use the result as a key or a
+    path, so it surfaces far from its cause -- as ``https://.../None``, or as
+    ``TypeError: expected str, bytes or os.PathLike object, not NoneType``.
+
+    Pass ``default`` to opt out of raising:
+
+    >>> inner_most({}, 'a', '_id_of_key', default=None) is None
+    True
+    """
+    x = last_element(store_trans_path(store, arg, method))
+    if x is None and getattr(store, method, None) is None:
+        # Distinguish "no layer supplied the method" (a resolution failure) from "a layer
+        # legitimately returned None" (only reachable when the outermost layer HAS the method).
+        if default is no_default:
+            raise AttributeError(
+                f"No layer of {type(store).__name__} defines {method!r}, so {arg!r} "
+                f"cannot be resolved. Pass default= if you want a fallback instead of "
+                f"this error. (Previously this returned None silently.)"
+            )
+        return default
+    return x
 
 
 # TODO: Better change the signature to reflect context (k (key) or v (val) instead of arg)

dol/filesys.py

@@ -821,13 +821,24 @@ def __setitem__(self, k, v):
             # TODO: ... But perhaps a more precise (but sufficient) exception list better?
             from dol.dig import inner_most_key
 
-            # get the inner most key, which should be a full path
-            _id = inner_most_key(self, k)
+            # Get the inner most key, which should be a full path.
+            # ``default=k``: this mixin is normally mixed into a persister whose keys ARE
+            # full paths and which therefore defines no ``_id_of_key``. Without the default
+            # the resolution fails, and it used to fail as a silent ``None`` -- turning the
+            # original write error into ``TypeError: expected str ... not NoneType``.
+            _id = inner_most_key(self, k, default=k)
             # get the full path of directory needed for this file
             dirname = os.path.dirname(_id)
+            # Only create directories for an ABSOLUTE path. A relative one means the key
+            # was never resolved to a filepath (a persister with relative keys and no
+            # ``_id_of_key``), and creating it would silently mkdir under the process CWD --
+            # outside the store -- after which the write would still fail. Re-raise instead.
+            if not os.path.isabs(dirname):
+                raise
             # make all the directories needed
-            ensure_dir(dirname, self._verbose)
-            os.makedirs(dirname, exist_ok=True)
+            # ``verbose`` is keyword-only; this was passing it positionally, so the
+            # recovery path raised TypeError instead of creating the directory.
+            ensure_dir(dirname, verbose=self._verbose)
             # try writing again
             super().__setitem__(k, v)
             # TODO: Undesirable here: If the setitem still fails, we created dirs

dol/tests/test_dig.py

@@ -231,3 +231,114 @@ def test_inner_most_key():
     result = inner_most_key(store, "test")
     # Should return final key transformation or None
     assert result is None or isinstance(result, str)
+
+
+# -------------------------------------------------------------------------------------
+# Resolution failures must be loud, not None
+#
+# ``inner_most_key`` used to return ``None`` when no layer of the chain defined
+# ``_id_of_key``. Callers use the result as a key or a path, so the ``None`` surfaced far
+# from its cause -- as a URL ending in ``/None``, or as
+# ``TypeError: expected str, bytes or os.PathLike object, not NoneType`` inside
+# ``MakeMissingDirsStoreMixin``.
+
+
+class _NoKeyMethods:
+    """A leaf with no ``_id_of_key`` -- the shape that used to yield a silent ``None``."""
+
+    def __getitem__(self, k):
+        return k
+
+
+def test_inner_most_raises_when_no_layer_defines_the_method():
+    with pytest.raises(AttributeError) as excinfo:
+        inner_most_key(_NoKeyMethods(), "some_key")
+    msg = str(excinfo.value)
+    assert "_id_of_key" in msg
+    assert "some_key" in msg
+
+    with pytest.raises(AttributeError):
+        inner_most_key({}, "some_key")
+
+
+def test_inner_most_default_opts_out_of_raising():
+    assert inner_most_key(_NoKeyMethods(), "k", default=None) is None
+    assert inner_most_key({}, "k", default="fallback") == "fallback"
+    # default is ignored when resolution succeeds
+    assert inner_most_key(SimpleStore({}), "k", default="fallback") == "id_k"
+
+
+def test_inner_most_still_resolves_through_a_real_wrap():
+    from dol import KeyCodecs
+
+    store = KeyCodecs.prefixed("a/")({"a/b": 1})
+    assert inner_most_key(store, "b") == "a/b"
+    assert list(unravel_key(store, "b")) == ["a/b", "a/b"]
+
+
+def test_store_trans_path_recurses_with_the_given_method():
+    """``inner_most_val`` used to apply ``_data_of_obj`` at the top layer and
+    ``_id_of_key`` at every deeper one, because the recursion hardcoded ``unravel_key``."""
+    from dol import wrap_kvs
+    from dol.dig import inner_most_val
+
+    store = wrap_kvs(
+        wrap_kvs({"k": 1}, data_of_obj=lambda v: v * 10), data_of_obj=lambda v: v + 1
+    )
+    assert inner_most_val(store, 5) == 60  # (5 + 1) * 10, both layers applied
+
+
+def test_inner_most_key_is_exported_from_dol():
+    """s3dol and other adapters need this as public API, not a submodule import."""
+    import dol
+
+    assert dol.inner_most_key is inner_most_key
+    assert hasattr(dol, "unravel_key")
+
+
+def test_make_missing_dirs_store_mixin_creates_dirs(tmpdir):
+    """Regression: this recovery path raised ``TypeError`` twice over -- once from the
+    ``None`` key, once from passing keyword-only ``verbose`` positionally."""
+    import os
+    from dol.filesys import MakeMissingDirsStoreMixin, FileBytesPersister
+
+    class S(MakeMissingDirsStoreMixin, FileBytesPersister):
+        pass
+
+    rootdir = str(tmpdir)
+    filepath = os.path.join(rootdir, "deep", "deeper", "f.bin")
+    S(rootdir)[filepath] = b"hello"
+    with open(filepath, "rb") as fp:
+        assert fp.read() == b"hello"
+
+
+def test_make_missing_dirs_mixin_does_not_mkdir_under_the_cwd(tmpdir):
+    """A relative dirname means the key was never resolved to a filepath. Creating it
+    would mkdir under the process CWD -- outside the store -- and the write would still
+    fail, so the original error is re-raised instead."""
+    import os
+    import tempfile
+
+    from dol.filesys import MakeMissingDirsStoreMixin
+
+    class RelPersister:
+        def __init__(self, rootdir):
+            self.rootdir = rootdir
+
+        def __setitem__(self, k, v):
+            with open(os.path.join(self.rootdir, k), "wb") as fp:
+                fp.write(v)
+
+    class S(MakeMissingDirsStoreMixin, RelPersister):
+        pass
+
+    cwd = str(tmpdir)
+    store_root = tempfile.mkdtemp()
+    previous = os.getcwd()
+    os.chdir(cwd)
+    try:
+        with pytest.raises(Exception):
+            S(store_root)["sub/dir/f.bin"] = b"hi"
+        assert not os.path.exists(os.path.join(cwd, "sub")), "mkdir'd outside the store"
+    finally:
+        os.chdir(previous)