SQLAlchemyStore violates the Mapping contract: __iter__ yields ORM rows (not keys) and __contains__ always False

[thorwhalen]

Summary

SQLAlchemyStore (and its underlying SQLAlchemyPersister) does not satisfy the Mapping/MutableMapping contract for two methods. Found while wiring sqldol as a durable catalog backend for lacing.ArtifactStore (a content-addressed artifact catalog). Both are surmountable from the consumer side, but they are genuine contract bugs that surprise anyone wrapping the store with dol.wrap_kvs or treating it as a plain mapping.

Repro

from sqldol import SQLAlchemyStore
from sqldol.sql_base import SQLAlchemyPersister as P

store = SQLAlchemyStore(
    uri='sqlite:///:memory:',
    collection_name='cat',
    key_fields={'id': P.TYPE_STRING},
    data_fields={'doc': P.TYPE_TEXT},
)
store[{'id': 'a'}] = {'doc': 'x'}

# 1) __iter__ yields ORM row objects, not keys:
print(list(store))          # -> [<DeclarativeTable ...>], expected ['a'] (or [{'id': 'a'}])

# 2) __contains__ is always False, even for present keys:
print({'id': 'a'} in store) # -> False, expected True

Root cause

• SQLAlchemyPersister.__iter__ does yield from self.query, i.e. it yields ORM row objects rather than keys. A Mapping must iterate keys.
• __contains__ is inherited from dol.base.Collection, which loops for existing_x in iter(self): if existing_x == x. Since iter(self) yields ORM rows, comparing a row to the query key ({'id': 'a'}) is never equal, so membership is always False.

Note SQLAlchemyTupleStore relies on __iter__ yielding row objects (its _key_of_id extracts fields off the ORM object), so a fix for #1 needs to preserve that path — likely by overriding _key_of_id/iteration so the base store iterates keys while the tuple store keeps its current extraction.

Suggested fixes

• SQLAlchemyStore.__iter__ should yield keys (e.g. {kf: getattr(row, kf) for kf in key_fields} — or the single key value when there's one key field).
• __contains__ should be getitem-based (try self[k], catch KeyError) — cheap, correct, and dialect-agnostic.

Consumer workaround (what lacing does)

Wrapping with dol.wrap_kvs(..., key_of_id=...) that handles both the key-dict (write path) and the ORM row (iter path) recovers correct iteration; and exposing the store through a collections.abc.Mapping facade (whose __contains__ is getitem-based) sidesteps the broken __contains__. Works, but every consumer has to know this.