From c528e1eb72a2ca1a9b3466ab1bf5e3f92445c41f Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Mon, 7 Sep 2026 01:11:38 +0200 Subject: [PATCH 1/2] 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 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 --- sqldol/base.py | 48 ++++++++- sqldol/stores.py | 2 +- sqldol/tests/test_base_mapping_contract.py | 111 +++++++++++++++++++++ 3 files changed, 155 insertions(+), 6 deletions(-) create mode 100644 sqldol/tests/test_base_mapping_contract.py diff --git a/sqldol/base.py b/sqldol/base.py index dae6cbf..434ad6c 100644 --- a/sqldol/base.py +++ b/sqldol/base.py @@ -1,6 +1,6 @@ """Base objects for sqldol""" -from typing import Union, List +from typing import Union, List, Literal from collections.abc import Iterable, Iterable, Mapping, Sized, MutableMapping from sqlalchemy import ( Table, @@ -11,6 +11,7 @@ exists, update, text, + func, Engine, Column, ) @@ -167,6 +168,21 @@ def _validate_key_columns(engine, table_name, key_columns): return key_columns +MissingKeyPolicy = Literal['empty', 'raise'] +_missing_key_policies = ('empty', 'raise') + + +def _validate_missing_key_policy(missing_key_policy) -> MissingKeyPolicy: + """Validate a ``missing_key_policy`` at construction time, not at lookup time.""" + if missing_key_policy not in _missing_key_policies: + msg = ( + f'missing_key_policy must be one of {_missing_key_policies}, ' + f'not {missing_key_policy!r}' + ) + raise ValueError(msg) + return missing_key_policy + + # TODO: Make SqlBaseKvReader into a context manager. See https://github.com/i2mint/dol/discussions/49#discussioncomment-8658626 # TODO: Implement filt. (Needs to filt iter, len, and getitem. # TODO: Refactor idea: Make a query class and a query executor class. Two layers below SqlBaseKvReader. @@ -178,6 +194,16 @@ class SqlBaseKvReader(Mapping): """A mapping view of a table, where keys are values from a key column and values are values from a value column. There's also a filter function that can be used to filter the rows. + + The ``missing_key_policy`` keyword decides what a lookup of an absent key does: + + - ``'empty'`` (the default) returns an empty result. This is the historical + behavior, kept as the default for backwards compatibility, but note that it + makes the inherited ``__contains__`` and ``get(key, default)`` lie: because + ``Mapping`` implements both in terms of ``__getitem__`` raising ``KeyError``, + every key looks present and ``get`` never returns its default. + - ``'raise'`` raises ``KeyError``, which is what ``collections.abc.Mapping`` + requires and what makes ``in`` and ``get`` truthful. """ def __init__( @@ -187,8 +213,11 @@ def __init__( key_columns: str = None, value_columns: str | list[str] = None, filt=None, + *, + missing_key_policy: MissingKeyPolicy = 'empty', ): self.engine = ensure_engine(engine) + self.missing_key_policy = _validate_missing_key_policy(missing_key_policy) self.table_name = table_name key_columns = _validate_key_columns(self.engine, self.table_name, key_columns) self.metadata = MetaData() @@ -222,17 +251,26 @@ def __iter__(self): yield self._extract_key(row) def __len__(self): + # Note: A SELECT's ``rowcount`` is -1 on drivers that don't pre-buffer + # results (SQLite, for one), so the row count has to be asked for directly. + query = select(func.count()).select_from(self.table) with self.engine.connect() as connection: - result = connection.execute(self._table_selection_query) - return result.rowcount + return connection.execute(query).scalar_one() def __getitem__(self, key): query = self._table_selection_query.where(self.table.c[self.key_columns] == key) with self.engine.connect() as connection: - try : + if self.missing_key_policy == 'raise': + rows = connection.execute(query).fetchall() + if not rows: + raise KeyError(key) + return map(self._extract_values, rows) + try: result = connection.execute(query) return map(self._extract_values, result.fetchall()) - except : + except Exception: + # Note: Narrowed from a bare ``except`` so that KeyboardInterrupt + # and SystemExit are no longer swallowed. return None # def __getitem__(self, key): diff --git a/sqldol/stores.py b/sqldol/stores.py index ef0b348..518838c 100644 --- a/sqldol/stores.py +++ b/sqldol/stores.py @@ -13,7 +13,7 @@ def _first_value(iterable): """ Get the first element of an iterable. - >>> _get_first([1, 2]) + >>> _first_value([1, 2]) 1 """ diff --git a/sqldol/tests/test_base_mapping_contract.py b/sqldol/tests/test_base_mapping_contract.py new file mode 100644 index 0000000..fa90729 --- /dev/null +++ b/sqldol/tests/test_base_mapping_contract.py @@ -0,0 +1,111 @@ +"""Tests for the ``Mapping`` contract of ``SqlBaseKvReader`` and its subclasses. + +Covers ``__len__`` (which must return a real, non-negative row count) and the +``missing_key_policy`` that decides what an absent key does: return an empty +result (the legacy default) or raise ``KeyError`` (which is what makes the +inherited ``__contains__`` and ``.get(key, default)`` truthful). + +Uses in-memory SQLite only -- no external service, no credentials. +""" + +import pytest +from sqlalchemy import create_engine, text + +from sqldol import SqlDictsReader, SqlDictReader, SqlRowReader, SqlRowsReader + +TABLE_NAME = 't' +KEY_COLUMN = 'k' + + +@pytest.fixture +def engine(): + """An in-memory SQLite engine holding ``t(k, v)`` with rows a/1 and b/2.""" + engine = create_engine('sqlite:///:memory:') + with engine.connect() as connection: + connection.execute(text(f'CREATE TABLE {TABLE_NAME} (k TEXT, v TEXT)')) + connection.execute(text(f"INSERT INTO {TABLE_NAME} VALUES ('a', '1'), ('b', '2')")) + connection.commit() + return engine + + +@pytest.mark.parametrize( + 'cls', [SqlRowsReader, SqlRowReader, SqlDictsReader, SqlDictReader] +) +def test_len_is_a_real_row_count(engine, cls): + """``__len__`` must count rows, not return a SELECT's ``rowcount``. + + Before the fix every one of these raised + ``ValueError: __len__() should return >= 0``, because a SELECT reports + ``rowcount == -1`` on SQLite (and any other driver that does not + pre-buffer results). + """ + store = cls(engine, TABLE_NAME, key_columns=KEY_COLUMN) + assert len(store) == 2 + + +@pytest.mark.parametrize( + 'cls', [SqlRowsReader, SqlRowReader, SqlDictsReader, SqlDictReader] +) +def test_list_of_store_yields_the_keys(engine, cls): + """``list(store)`` used to die on the ``__len__`` length hint, though + ``iter(store)`` worked fine.""" + store = cls(engine, TABLE_NAME, key_columns=KEY_COLUMN) + assert sorted(store) == ['a', 'b'] + assert sorted(list(store)) == ['a', 'b'] + + +def test_values_of_present_keys_are_unchanged(engine): + """Guard that nothing about the happy path moved.""" + assert SqlDictsReader(engine, TABLE_NAME, key_columns=KEY_COLUMN)['a'] == [ + {'k': 'a', 'v': '1'} + ] + assert SqlRowsReader(engine, TABLE_NAME, key_columns=KEY_COLUMN)['a'] == [ + ['a', '1'] + ] + assert SqlRowReader(engine, TABLE_NAME, key_columns=KEY_COLUMN)['a'] == ['a', '1'] + assert SqlDictReader(engine, TABLE_NAME, key_columns=KEY_COLUMN)['a'] == { + 'k': 'a', + 'v': '1', + } + + +@pytest.mark.parametrize( + 'cls', [SqlRowsReader, SqlRowReader, SqlDictsReader, SqlDictReader] +) +def test_missing_key_policy_raise_makes_the_mapping_contract_truthful(engine, cls): + """With ``missing_key_policy='raise'``, ``in`` and ``.get`` stop lying. + + Before the fix ``'zzz' in store`` was ``True`` for every store (because + ``Mapping.__contains__`` is getitem-based and getitem never raised), and + ``.get('zzz', default)`` returned an empty result instead of ``default``. + """ + store = cls(engine, TABLE_NAME, key_columns=KEY_COLUMN, missing_key_policy='raise') + + with pytest.raises(KeyError): + store['zzz'] + + assert 'zzz' not in store + assert 'a' in store + assert store.get('zzz', 'DEFAULT') == 'DEFAULT' + assert store.get('a') is not None + + +def test_missing_key_policy_defaults_to_the_legacy_empty_behavior(engine): + """Regression guard: the default must keep 2024-era callers working. + + Existing consumers branch on ``SqlDictReader[missing] is None``, so the + ``KeyError`` behaviour has to stay opt-in. + """ + assert SqlDictReader(engine, TABLE_NAME, key_columns=KEY_COLUMN)['zzz'] is None + assert SqlDictsReader(engine, TABLE_NAME, key_columns=KEY_COLUMN)['zzz'] == [] + assert SqlRowsReader(engine, TABLE_NAME, key_columns=KEY_COLUMN)['zzz'] == [] + with pytest.raises(StopIteration): + SqlRowReader(engine, TABLE_NAME, key_columns=KEY_COLUMN)['zzz'] + + +def test_unknown_missing_key_policy_is_rejected_at_construction(engine): + """A typo must fail loudly, and at construction time, not at lookup time.""" + with pytest.raises(ValueError): + SqlDictsReader( + engine, TABLE_NAME, key_columns=KEY_COLUMN, missing_key_policy='nope' + ) From 39d9bb55bbb24b99158ea2599a53e5d1a9c538ed Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Mon, 7 Sep 2026 01:13:21 +0200 Subject: [PATCH 2/2] fix(sql_base): make `k in store` agree with `store[k]` 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 --- sqldol/sql_base.py | 18 ++++ .../tests/test_sqlalchemy_store_contract.py | 83 +++++++++++++++++++ 2 files changed, 101 insertions(+) create mode 100644 sqldol/tests/test_sqlalchemy_store_contract.py diff --git a/sqldol/sql_base.py b/sqldol/sql_base.py index 9915731..735d37a 100644 --- a/sqldol/sql_base.py +++ b/sqldol/sql_base.py @@ -347,6 +347,24 @@ def __getitem__(self, k): return doc + def __contains__(self, k) -> bool: + """True if and only if a row matches the key query ``k``. + + Getitem-based, so it agrees with ``__getitem__``, and it costs one + query instead of the brute-force scan inherited from + ``dol.base.Collection``. That inherited scan compared ``k`` against + whatever ``__iter__`` yields -- ORM row objects, never keys -- and so + answered False for every key, present or absent. + + A key that cannot name a row at all (not a mapping of field to value, + or naming a field the table doesn't have) is simply not contained, + which is what the inherited scan answered too. + """ + try: + return self.query.filter_by(**k).first() is not None + except (TypeError, sqlalchemy.exc.InvalidRequestError): + return False + def __setitem__(self, k, v): try: doc = self[k] diff --git a/sqldol/tests/test_sqlalchemy_store_contract.py b/sqldol/tests/test_sqlalchemy_store_contract.py new file mode 100644 index 0000000..1ab97e3 --- /dev/null +++ b/sqldol/tests/test_sqlalchemy_store_contract.py @@ -0,0 +1,83 @@ +"""Tests that ``k in store`` agrees with ``store[k]`` for the SQLAlchemy stores. + +``SQLAlchemyPersister.__iter__`` yields ORM row objects rather than keys, so the +brute-force ``__contains__`` inherited from ``dol.base.Collection`` compared a key +against a row and answered ``False`` for every key. ``SQLAlchemyStore`` and +``SQLAlchemyTupleStore`` inherited that answer through ``dol.base.Store``. + +Uses in-memory SQLite only -- no external service, no credentials. +""" + +import pytest + +from sqldol import SQLAlchemyPersister, SQLAlchemyStore, SQLAlchemyTupleStore + +SQLITE_DB_URI = 'sqlite:///:memory:' +KEY_FIELDS = {'id': SQLAlchemyPersister.TYPE_STRING} +DATA_FIELDS = {'doc': SQLAlchemyPersister.TYPE_TEXT} + + +def _mk(cls, collection_name): + return cls( + uri=SQLITE_DB_URI, + collection_name=collection_name, + key_fields=KEY_FIELDS, + data_fields=DATA_FIELDS, + ) + + +def test_persister_contains_agrees_with_getitem(): + persister = _mk(SQLAlchemyPersister, 'persister_contains') + persister[{'id': 'a'}] = {'doc': 'x'} + + assert {'id': 'a'} in persister + assert {'id': 'zzz'} not in persister + assert persister[{'id': 'a'}] is not None + assert len(persister) == 1 + + +def test_store_contains_agrees_with_getitem(): + store = _mk(SQLAlchemyStore, 'store_contains') + store[{'id': 'a'}] = {'doc': 'x'} + + assert {'id': 'a'} in store + assert {'id': 'zzz'} not in store + assert store[{'id': 'a'}] is not None + assert len(store) == 1 + + +def test_tuple_store_contains_agrees_with_getitem(): + store = _mk(SQLAlchemyTupleStore, 'tuple_store_contains') + store[('a',)] = ('x',) + + assert ('a',) in store + assert ('zzz',) not in store + assert store[('a',)] == ('x',) + assert len(store) == 1 + + +def test_tuple_store_iteration_still_yields_keys(): + """Pins that ``__iter__`` was deliberately left alone. + + Making the persister's ``__iter__`` yield keys instead of ORM rows is a + separate, not-backwards-compatible change: ``SQLAlchemyTupleStore._key_of_id`` + does ``getattr(obj, field)`` on whatever is yielded, and at least one known + consumer wraps the row-yielding behaviour. + """ + store = _mk(SQLAlchemyTupleStore, 'tuple_store_iter') + store[('a',)] = ('x',) + + assert list(store) == [('a',)] + + +def test_contains_does_not_raise_on_a_key_it_cannot_query(): + """``in`` must answer, not explode, for keys that can't name a row. + + ``Container.__contains__`` returned False for these before, so keeping them + False (rather than propagating a SQLAlchemy/TypeError) preserves behaviour. + """ + persister = _mk(SQLAlchemyPersister, 'persister_bad_key') + persister[{'id': 'a'}] = {'doc': 'x'} + + assert 'not-a-dict' not in persister + assert {'no_such_column': 'a'} not in persister