diff --git a/dol/__init__.py b/dol/__init__.py index 34f96d3b..6ea59f74 100644 --- a/dol/__init__.py +++ b/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 diff --git a/dol/dig.py b/dol/dig.py index 31916e54..e43448b1 100644 --- a/dol/dig.py +++ b/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.`` 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) diff --git a/dol/filesys.py b/dol/filesys.py index 841c78ab..3341f173 100644 --- a/dol/filesys.py +++ b/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 diff --git a/dol/tests/test_dig.py b/dol/tests/test_dig.py index fafba09a..c1c73cc3 100644 --- a/dol/tests/test_dig.py +++ b/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)