Leaf-bound delegation: persist_data bypasses data_of_obj/id_of_key and destroys documents; shipped stores.py classes miss the MongoBaseStore remedy #11

Summary

mongodol is one of the better-defended packages against the dol leaf-bound delegation bug — it already ships a purpose-built remedy, MongoBaseStore (mongodol/base.py:540), and that remedy works. But there are two real gaps:

1. persist_data is the one method the remedy forgot. It is delegated leaf-bound, bypasses both _data_of_obj and _id_of_key, and issues a replace_one(..., upsert=True) — so it silently destroys fields of the stored document and rewrites it in the wrong shape. Live on stores built by mongodol's own set_key_and_data_fields.
2. mongodol/stores.py never opts into the remedy. Line 8 imports wrap_kvs from dol, not from mongodol.trans, so all six publicly exported store classes get a plain Store wrapper. Their postget value transform is skipped by values(), items(), contains_value() and contains_item().

Two claims from the survey that prompted this investigation did not hold up and are explicitly refuted below — MongoBaseStore is the fix, not the bug, and distinct/aggregate are not an instance of the key-mapping bug.

Umbrella: i2mint/dol#83 · Root cause: i2mint/dol#18

Mechanism (brief)

dol wraps stores by delegation (has-a), and there are two routes, both in dol/base.py:

• Route A — instance-wraps: Store.__getattr__ (dol/base.py:742) returns getattr(self.store, attr), i.e. the leaf-bound method.
• Route B — class-wraps: delegate_to (dol/base.py:416) installs a DelegatedAttribute for every attr of the wrapped class; DelegatedAttribute.__get__ (dol/base.py:279) returns getattr(instance.store, attr) — also leaf-bound.

__getitem__/__setitem__/__delitem__/__contains__/__iter__ are transform-aware. Every other non-dunder method is handed the outer, untransformed key/value.

The escape valve that mongodol correctly discovered: delegate_to computes

attrs = attributes_of_wrapped - set(dir(wrapper_cls))   # dol/base.py:459

so any method the wrapper class defines itself is excluded from delegation and survives. That is exactly how MongoBaseStore works.

Affected symbols

1. persist_data — destructive (confirmed live)

# mongodol/base.py:475
def persist_data(self, data):
    return self.__setitem__({ID: data[ID]}, data)

This is the only one of the seven value-side methods that MongoBaseStore does not override. It is therefore delegated leaf-bound, and three things go wrong at once:

• data is the outer object, written raw — _data_of_obj never runs.
• the key {ID: data[ID]} is built from the outer object — _id_of_key never runs, so the store's key codec is bypassed entirely.
• the leaf __setitem__ (mongodol/base.py:415) is mgc.replace_one(..., upsert=True) — a whole-document replace.

What gets destroyed

Every field of the target document that is not present in the outer object is permanently deleted, and the document is rewritten in the outer (application) shape rather than the mongo shape. Subsequent reads through obj_of_data then raise KeyError on the missing mongo fields — the collection is left unreadable through its own store, and no mongo index or query on the original field names matches any more.

Repro — real mongodol classes

(run against a real mongo; verified here with the real mongodol/dol source over an in-memory pymongo stand-in, since pymongo was not installed in the verification environment)

from mongodol.trans import wrap_kvs            # this is wrapper=MongoBaseStore, the "fixed" path
from mongodol.base import MongoCollectionPersister

def obj_of_data(d): return {'_id': d['_id'], 'payload': {'n': d['n'], 'tag': d['tag']}}
def data_of_obj(o): return {'_id': o['_id'], 'n': o['payload']['n'], 'tag': o['payload']['tag']}

S = wrap_kvs(MongoCollectionPersister, obj_of_data=obj_of_data, data_of_obj=data_of_obj)

mgc.insert_many([{'_id': 0, 'n': 1, 'tag': 'x', 'audit': 'KEEP-ME'}])
s = S(mgc)

s.persist_data({'_id': 0, 'payload': {'n': 99, 'tag': 'y'}})

BEFORE : [{'_id': 0, 'n': 1, 'tag': 'x', 'audit': 'KEEP-ME'}]
AFTER  : [{'_id': 0, 'payload': {'n': 99, 'tag': 'y'}}]

data_of_obj(outer) would have been: {'_id': 0, 'n': 99, 'tag': 'y'}

audit is gone; n and tag have been buried inside a payload sub-document that no mongo-side index or query knows about.

This is live, not latent: mongodol itself ships set_key_and_data_fields (mongodol/trans.py:346), which builds precisely such a transform-applying store, and persist_data is public on it with no further user wrapping.

2. mongodol/stores.py does not use mongodol's own wrap_kvs (confirmed live)

# mongodol/stores.py:8
from dol import Store, wrap_kvs        # <-- dol's, not mongodol.trans's

mongodol/trans.py:316 defines the protected variant:

wrap_kvs = partial(dol_wrap_kvs, wrapper=MongoBaseStore)

…but stores.py never imports it. dol defaults to wrapper or Store (dol/trans.py:702), so the six classes built at mongodol/stores.py:19,22,69,99 — all re-exported from mongodol/__init__.py:9-16 — are plain Store wraps with none of the protections:

MongoCollectionUniqueDocReader  MRO: [..., 'Store', 'KvPersister', ...]   # no MongoBaseStore
  contains_value / iter_values / contains_item / iter_items   DELEGATED (leaf-bound)
  distinct / unique / aggregate                                DELEGATED (leaf-bound)
MongoCollectionUniqueDocPersister additionally:
  append / extend / persist_data                               DELEGATED (leaf-bound)

Consequence

These classes carry their value transform as postget, so __getitem__ enforces a contract that the iteration path silently does not:

from mongodol import MongoCollectionUniqueDocReader

mgc.insert_many([{'_id':0,'s':'a','n':1}, {'_id':1,'s':'b','n':2}, {'_id':2,'s':'b','n':3}])
s = MongoCollectionUniqueDocReader(
    mgc, iter_projection={'s': True, '_id': False},
         getitem_projection={'n': True, '_id': False})

s[{'s':'a'}]    = {'n': 1}
s[{'s':'b'}]   -> KeyNotUniqueError: Key was not unique (i.e. cursor has more than one match)
s[{'s':'zzz'}] -> KeyError: No document found for query

list(s.items())  = [({'s':'a'},{'n':1}), ({'s':'b'},{'n':2}), ({'s':'b'},{'n':3})]
list(s.values()) = [{'n':1}, {'n':2}, {'n':3}]

values()/items() happily serve the duplicate 'b' rows that s[k] refuses to serve. The unicity guarantee that is this class's entire reason to exist is unenforced on the iteration path, and dict(s.items()) silently keeps only the last duplicate.

Remediation trap (verified): just switching the stores.py import to mongodol.trans.wrap_kvs would not fix this. MongoBaseStore.iter_values applies _obj_of_data, and postget is not _obj_of_data — on these classes s._obj_of_data is identity_func. MongoBaseStore needs a postget-aware path too, or these classes need to express their transform as obj_of_data.

What is refuted

MongoBaseStore re-exports these methods broken — false

mongodol/base.py:540-561 is the opposite of the allegation; it is a correct, working remedy:

class MongoBaseStore(Store):
    def contains_value(self, v):
        return self.store.contains_value(self._data_of_obj(v))
    def iter_values(self):
        return map(self._obj_of_data, self.store.iter_values())
    def contains_item(self, item):
        k, v = item
        return self.store.contains_item((self._id_of_key(k), self._data_of_obj(v)))
    def iter_items(self):
        yield from ((self._key_of_id(key), self._obj_of_data(doc))
                    for key, doc in self.store.iter_items())
    def append(self, v):
        return self.store.append(self._data_of_obj(v))
    def extend(self, values):
        return self.store.extend(list(map(self._data_of_obj, values)))

Side-by-side on the same leaf and the same codec:

dol.wrap_kvs (plain Store)            MongoBaseStore in MRO: False
   s.append(3)          -> AssertionError: v (value) must be a mapping ... v=3
   s.extend([4, 5])     -> AssertionError: values must be mappings
   s.contains_value(1)  -> TypeError: filter must be a dict. Got int: 1

mongodol.trans.wrap_kvs               MongoBaseStore in MRO: True
   s.append(3)          -> OK
   s.extend([4, 5])     -> OK
   s.contains_value(1)  -> OK
   docs now: [..., {'n': 3, ...}, {'n': 4, ...}, {'n': 5, ...}]   # data_of_obj applied

There is already an integration test asserting this contract — mongodol/tests/int_tests/base_int_test.py:171-180 passes wrapper=MongoBaseStore and checks that append/extend/values()/items() all round-trip through the mappers.

distinct/aggregate take field paths instead of store keys, so it's the same bug — false

A mongo field path was never a store key, not even on the unwrapped leaf: keys there are query dicts ({'user': 'alice'}) while distinct takes the string 'user'. Different namespaces by design — see the package's own mongodol/tests/base_test.py:146-147 (s.distinct('color'), s.distinct('dims.x')). No key codec is being skipped because none was ever in that path.

Nor do they leak the package's own scope. Because delegation binds them to the leaf, self.filter and self._merge_with_filt are the leaf's — which is the store's mongo-native scope:

scoped = MongoCollectionReader(mgc, filter={'user': {'$in': ['alice','bob']}},
                               iter_projection={'user': True, '_id': False})
list(scoped)              # [{'user': 'alice'}, {'user': 'bob'}]
scoped.distinct('user')   # ['alice', 'bob']   <- native filter IS honoured

What is real (latent): an outer, dol-level filter is invisible to them.

from dol import filt_iter
Only = filt_iter(MongoCollectionReader, filt=lambda k: k == {'user': 'alice'})
o = Only(mgc, iter_projection={'user': True, '_id': False})

list(o)               # [{'user': 'alice'}]
o.distinct('user')    # ['alice', 'bob', 'carol']            <- leaked
list(o.aggregate([])) # all three documents                  <- leaked

A store handed out as a restricted view still discloses every distinct value and every document in the collection. This is latent — mongodol scopes with the mongo-native filter argument, which works; it only triggers if a caller layers filt_iter or a similar key-restricting dol wrapper on top.

Suggested remediation

A. Fix persist_data (the destructive one) — highest priority.

The cleanest fix costs nothing and needs no dol helper: persist_data is a thin convenience over __setitem__, so move it onto MongoBaseStore alongside its six siblings and route it through the wrapper's own transform-aware __setitem__. Because delegate_to excludes anything in dir(wrapper_cls) (dol/base.py:459), defining it on MongoBaseStore is sufficient — no dol change required.

Prefer taking the key explicitly rather than inferring it from data[ID], since the _id inference is itself what bypasses the key codec:

# mongodol/base.py, inside class MongoBaseStore
def persist_data(self, data, key=None):
    if key is None:                       # backward-compatible inference path
        key = self._key_of_id({ID: self._data_of_obj(data)[ID]})
    self[key] = data                      # goes through _id_of_key + _data_of_obj

Also consider making persist_data non-destructive (update_one with $set rather than replace_one). Silently dropping every unmentioned field is surprising regardless of wrapping, and is what turns this from "wrong value written" into "data lost".

B. Make the shipped classes use the remedy. Change mongodol/stores.py:8 to import wrap_kvs from mongodol.trans, and teach MongoBaseStore about postget (or re-express those classes' transforms as obj_of_data) — as noted above, the import change alone is not sufficient.

C. For any leaf method that genuinely needs the fully-mapped key, the escape hatch is:

from dol import wrapped_self          # exported from dol
from dol.dig import inner_most_key    # NOT exported from dol — import from dol.dig

def some_method(self, k):
    _id = inner_most_key(wrapped_self(self), k)
    if not isinstance(_id, str):      # mandatory: see trap 2
        _id = self._id_of_key(k)
    ...

Two traps, both important:

1. inner_most_key walks the whole chain including the leaf's own _id_of_key. It therefore replaces self._id_of_key(k) and must never be composed with it, or the key is transformed twice.
2. It returns None silently when no layer in the chain defines _id_of_key. A type check is mandatory before use.

Verified that the backref machinery works on mongodol's actual shape:

leaf  = s.store
outer = wrapped_self(leaf)
assert outer is s                 # True
outer[{'_id': 0}] = outer_obj     # correctly applies data_of_obj

D. Consider shrinking the surface. The long-term fix for distinct/unique/aggregate is not to map keys through them — they are mongo-native escape hatches, not Mapping operations. Moving them onto an explicit handle obtained from the store (the way azuredol's BlobHandle scopes per-object methods at construction) would make the scope they operate in unambiguous, instead of leaving it to depend on which wrapper layer the caller happens to hold.

Incidental findings

Found while verifying the above; unrelated to delegation, but all three reproduce on the current tree:

• MongoCollectionMultipleDocsReader / MongoCollectionMultipleDocsPersister raise on every __getitem__. mongodol/stores.py:69-71 and 99-101 pass postget=partial(ObjOfData.all_docs_fetch, doc_collector=list), but dol calls postget(k, v) (dol/trans.py:2240), binding cursor=k and doc_collector=v — which collides with the partial's keyword:
  TypeError: ObjOfData.all_docs_fetch() got multiple values for argument 'doc_collector'. Both classes are exported from mongodol/__init__.py.
• MongoCollectionMultipleDocsPersister.__setitem__ always raises. mongodol/stores.py:116-118 uses self._mgc, but the leaf sets self.mgc (mongodol/base.py:32) — an attribute rename that was never propagated. Result: AttributeError: ... object has no attribute '_mgc'.
• _items_projection crashes when getitem_projection is a list. mongodol/base.py:245 calls projection_union(...) without normalizing getitem_projection, so a list-valued projection reaches flatten_dict_items and raises AttributeError: 'list' object has no attribute 'items' from mongodol/util.py:84. Reached through items() on any store constructed with e.g. getitem_projection=['n'].

Verification notes

pymongo was not installed in the verification environment, so the real mongodol and dol source were exercised against a minimal in-memory pymongo stand-in (find/insert_one/insert_many/replace_one/delete_one/delete_many/count_documents/distinct/aggregate, with $and/$in/$lte/$gte matching and projections). All class-structure findings — MRO, which methods are DelegatedAttribute vs. defined, and delegate_to's exclusion rule — are backend-independent. The behavioural traces above should be re-confirmed against a live mongo before the fixes land.

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.