Skip to content

fix(sql_base,base): make the Mapping contract truthful (contains, len, missing-key policy) - #4

Merged
thorwhalen merged 2 commits into
masterfrom
fix/mapping-contract
Sep 22, 2026
Merged

thorwhalen merged 2 commits into
masterfrom
fix/mapping-contract

Conversation

@thorwhalen

Copy link
Copy Markdown
Member

Two commits fixing sqldol's Mapping-contract violations, both scoped to leave
default behavior unchanged:

  1. fix(base): make SqlBaseKvReader.__len__ return a real count — __len__
    returned a SELECT's rowcount, which is -1 on drivers that don't pre-buffer
    (SQLite among them), so len(store)/list(store) raised ValueError on those
    backends. Now issues SELECT count(*). Also narrows __getitem__'s bare
    except: to except Exception: (no longer swallows KeyboardInterrupt/
    SystemExit) and adds an opt-in keyword-only missing_key_policy ('empty'
    default = today's behavior, 'raise' makes in/.get() truthful per
    collections.abc.Mapping).
  2. fix(sql_base): make \k in store` agree with `store[k]`—SQLAlchemyPersisterhad nocontains, so it inherited the brute-force scan from dol.base.Collection, which compares against iter's yield — but this persister's iteryields ORM row objects, never keys, sok in persisterwasFalsefor every key, including keyspersister[k]resolves happily. Adds a getitem-basedcontains`.

Refs #2 (fixes the __contains__ half only; making __iter__ yield keys instead
of ORM rows is deliberately out of scope — not backward compatible, a known
consumer is written against the row-yielding behavior — and is pinned by a new
regression test). Does not close #2.

Dependents check (fleet_dependents.json lists raglab-app as the sole
dependent of sqldol): raglab_app imports only SqlDictReader/SqlDictStore
(both SqlBaseKvReader/SqlBaseKvStore subclasses, commit 1's class) — never
SQLAlchemyPersister/SQLAlchemyStore/SQLAlchemyTupleStore (commit 2's
classes), so the __contains__ change cannot reach it. Commit 1 keeps
missing_key_policy defaulted to 'empty' (byte-for-byte prior behavior,
pinned by test_missing_key_policy_defaults_to_the_legacy_empty_behavior), and
the __len__/except Exception changes are strictly corrective. raglab_app's
own test suite is intentionally disabled in its CI ([tool.wads.ci.testing] enabled = false — "private Streamlit app with no test suite; its modules use
script-style imports and run code at import time") and its full install needs a
pg_config/postgres toolchain not present on this box, so I could not execute it
directly; the source-level compatibility read above is the substitute. Recorded
in DECISIONS.md.

Branch sat pushed with green CI and no PR for two weeks (thorwhalen/fleet_stuff
cleanup). Verified: master has not moved since the branch was cut (no rebase
needed). sqldol has no pyproject.toml/wads CI config (setup.py/setup.cfg
only), so gated with a plain venv + pytest instead of wads ci-local: 20/20
tests pass (sqldol/tests/test_base_mapping_contract.py,
sqldol/tests/test_sqlalchemy_store_contract.py). Hosted CI on the branch was
already green from the original 2026-09-08 push.

Refs #2

🤖 Generated with Claude Code

`__len__` returned a SELECT's `rowcount`, which is -1 on drivers that don't
pre-buffer results (SQLite among them). CPython's `__len__` guard then raised
`ValueError: __len__() should return >= 0`, so `len(store)` and `list(store)`
were both unusable even though `iter(store)` worked. It now issues a
`SELECT count(*)`.

Two smaller changes in the same area:

- `__getitem__`'s bare `except:` is narrowed to `except Exception:`, so
  KeyboardInterrupt and SystemExit are no longer swallowed. Behaviour is
  otherwise unchanged.
- A new keyword-only `missing_key_policy` makes the Mapping contract opt-in
  fixable. It defaults to `'empty'`, i.e. exactly today's behaviour (an absent
  key yields an empty result, so the inherited `__contains__` reports True for
  every key and `.get(key, default)` never returns its default). Passing
  `'raise'` raises KeyError instead, which is what `collections.abc.Mapping`
  requires and what makes `in` and `.get` truthful. The default is left alone
  because existing consumers branch on the empty/None result.

Also fixes the `_first_value` doctest, which called a name (`_get_first`) that
does not exist.

Adds sqldol/tests/test_base_mapping_contract.py (in-memory SQLite, no external
service), covering both the count fix and both policies -- including regression
guards pinning the default's legacy per-class behaviour.

Claude-Session: https://claude.ai/code/session_01L1aQPB34n7PU7jmbztSjBe
Addresses the first half of issue #2 (deliberately does not close it -- see
below).

`SQLAlchemyPersister` defined no `__contains__`, so it inherited the
brute-force one from `dol.base.Collection`, which scans `iter(self)` looking
for a key equal to `k`. But the persister's `__iter__` yields ORM row objects,
never keys, so the comparison never matched and `k in persister` was False for
every key -- including keys `persister[k]` resolves happily.
`SQLAlchemyStore` and `SQLAlchemyTupleStore` inherited the same wrong answer,
since `Store.__contains__` delegates through `_id_of_key`.

Adds a getitem-based `__contains__` on the persister, which fixes all three
classes at once and costs one query instead of a full scan. A key that cannot
name a row at all still answers False rather than raising, matching what the
inherited scan did.

Deliberately out of scope: the other half of #2, making `__iter__` yield keys
instead of ORM rows. That one is not backwards compatible --
`SQLAlchemyTupleStore._key_of_id` does `getattr(obj, field)` on whatever is
yielded, and at least one known consumer is written against the row-yielding
behaviour. A test pins the current iteration behaviour so the split is explicit,
and the issue stays open for that half.

Adds sqldol/tests/test_sqlalchemy_store_contract.py (in-memory SQLite, no
external service).

Claude-Session: https://claude.ai/code/session_01L1aQPB34n7PU7jmbztSjBe
@thorwhalen

Copy link
Copy Markdown
Member Author

Post-merge review notes (design, not fixed here):

  1. With the default missing_key_policy='empty', k in store is still True for every key, and that is the default anyone gets. __contains__ does not have to depend on the policy. An explicit __contains__ that runs SELECT EXISTS(... WHERE key = :k) would make in truthful under both policies and would not touch the store[missing] is None callers the default protects. I would add that and keep missing_key_policy for __getitem__/get only.
  2. SqlBaseKvStore._mk_column_filter builds its WHERE clause by f-string interpolation into text(). A key containing a quote breaks the query, and it is an injection vector. Use self.table.c[col] == value, the way __getitem__ already does.
  3. The same rowcount-on-SELECT trap this PR fixed in __len__ was still in __setitem__. On SQLite, writing to an existing key inserted a duplicate row. Fixed in Fix: SqlBaseKvStore.__setitem__ duplicated rows instead of updating (rowcount trap left by #4) #5.

thorwhalen added a commit that referenced this pull request Sep 22, 2026
…uplicated (#5)

Update-vs-insert was decided from a SELECT's rowcount, which is -1 on SQLite,
so writing an existing key always inserted a duplicate row and reads kept
returning the old value. Same rowcount trap #4 fixed in __len__.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
thorwhalen added a commit that referenced this pull request Sep 22, 2026
)

The same rowcount trap #4 fixed in SqlBaseKvReader.__len__ and #5 fixed in
SqlBaseKvStore.__setitem__ was still in TableRows.__len__, so len() of a
TableRows over SQLite raised "ValueError: __len__() should return >= 0".
Ask the database for COUNT(*), honouring the row filter.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.

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

1 participant