Skip to content

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

Merged
thorwhalen merged 2 commits into
masterfrom
claude/dig-export-and-harden
Aug 10, 2026
Merged

fix(dig): resolution failures raise instead of returning None; export inner_most_key#84
thorwhalen merged 2 commits into
masterfrom
claude/dig-export-and-harden

Conversation

@thorwhalen

Copy link
Copy Markdown
Member

Three related defects in dol.dig, found while building an S3 adapter that needs to resolve a
key through a wrapped store.

1. inner_most_key returned None on a resolution failure

When no layer of the chain defines _id_of_key, last_element over an empty generator returned
None. Callers use the result as a key or a path, so it surfaced far from its cause:

>>> from dol.dig import inner_most_key
>>> inner_most_key({}, 'some_key')     # before: None

That is what turns a presigned URL into https://.../None, and what makes
MakeMissingDirsStoreMixin die with TypeError: expected str, bytes or os.PathLike object, not NoneType. It now raises AttributeError naming the method and the argument. default= opts
out.

2. store_trans_path ignored its own method argument when recursing

Line 43 hardcoded unravel_key, so _id_of_key was applied at every layer below the first
regardless of what was asked for. This is the acknowledged TODO on inner_most_val — it applied
_data_of_obj at the top layer and _id_of_key below it. Now recurses with the given method:

>>> store = wrap_kvs(wrap_kvs({'k': 1}, data_of_obj=lambda v: v * 10), data_of_obj=lambda v: v + 1)
>>> inner_most_val(store, 5)
60                                      # (5 + 1) * 10 -- before, both layers were not applied

3. inner_most_key / unravel_key are now exported from dol

They were importable only from dol.dig, while being the documented answer to "resolve a key
through a wrap" — which pushed every *dol adapter into a private-module import.

Also: two bugs in MakeMissingDirsStoreMixin.__setitem__

Its recovery path could never have worked. It fed the None from (1) into os.path.dirname,
and then passed keyword-only verbose positionally to ensure_dir. Both raised TypeError, so
the mixin turned a recoverable write error into an unrelated crash. Now resolves with
default=k — correct, because the mixin targets persisters whose keys are full paths and
which therefore define no _id_of_key.

Compatibility

The only behaviour change is (1), and it converts a silent wrong answer into a loud one, in line
with dol's No Silent Failures convention. Ecosystem callers of inner_most_key (xdol,
tapyoca, graze's vendored copy, py2store's star re-export) all pass stores that define
_id_of_key, so none of them reach the new raise. inner_most_val/unravel_val have no
callers outside dol.

Verification

  • Full suite: 511 passed, 3 skipped.
  • 6 new tests in test_dig.py: the raise, the default= escape, the recursion fix, the export,
    resolution through a real wrap, and a MakeMissingDirsStoreMixin round-trip.
  • Dependents gate byte-for-byte identical to HEAD for every dependent whose backend SDK is
    installed here — xdol 4, pdfdol 23+1 skipped, chromadol 2, focal 1, dol_cookbook 1, ftpdol 1.
    mongodol/sqldol/hfdol/dropboxdol/sshdol fail on absent SDKs (pymongo, sqlalchemy, datasets,
    dropbox, paramiko) both before and after.

Needed by the s3dol v1 redesign (i2mint/s3dol#17), which resolves keys through possibly-wrapped
stores and cannot depend on a private module or a silent None.

… inner_most_key

Three related defects in dol.dig, all found while building an S3 adapter that needs
to resolve a key through a wrapped store.

1. `inner_most_key(store, k)` returned **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`. It now raises
   AttributeError naming the method and the argument. Pass `default=` to opt out.

2. `store_trans_path` hardcoded `unravel_key` for its recursive step, so it applied
   `_id_of_key` at every layer below the first regardless of the `method` argument.
   That is the acknowledged TODO on `inner_most_val`: it applied `_data_of_obj` at the
   top layer and `_id_of_key` below. Now recurses with the method it was given, so
   `inner_most_val` does what its name says.

3. `inner_most_key` / `unravel_key` are now exported from `dol`. They were importable
   only from `dol.dig` while being the documented answer to resolving a key through a
   wrap, which pushed every adapter into a private-module import.

Also fixes the two bugs (1) exposed in `MakeMissingDirsStoreMixin.__setitem__`, whose
recovery path could never have worked: it fed the None to `os.path.dirname`, and then
passed keyword-only `verbose` positionally to `ensure_dir`. Both raised TypeError. It
now resolves with `default=k` -- correct, since the mixin targets persisters whose keys
ARE full paths and which therefore define no `_id_of_key`.

Tests: 6 new cases in test_dig.py covering the raise, the `default=` escape, the
recursion fix, the export, and a MakeMissingDirsStoreMixin round-trip.

Verified: full suite 511 passed / 3 skipped. Dependents gate byte-for-byte identical to
HEAD for every dependent whose backend SDK is installed (xdol 4, pdfdol 23+1, chromadol
2, focal 1, dol_cookbook 1, ftpdol 1); the rest fail on absent SDKs both before and
after.
This was referenced Aug 10, 2026
Adversarial-review finding on the `default=k` fallback added to
`MakeMissingDirsStoreMixin`. When the mixin targets a persister with relative keys
and no `_id_of_key`, `default=k` yields a relative dirname, and `ensure_dir` then
creates it under the process CWD -- outside the store -- after which the write still
fails. Now guarded by `os.path.isabs`: a relative dirname means the key was never
resolved to a filepath, so the original write error is re-raised untouched.

Severity was capped by the fact that this recovery path was 100% dead before this PR
(it raised TypeError unconditionally on the keyword-only `verbose`), so nothing could
have depended on it -- but "fails and leaves a stray directory in your CWD" is worse
than "fails", and this is a mixin dol still exports.

Test added. 517 passed / 3 skipped.
@thorwhalen
thorwhalen merged commit fa37de7 into master Aug 10, 2026
12 checks passed
@thorwhalen
thorwhalen deleted the claude/dig-export-and-harden branch August 10, 2026 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant