From ce5a5820482fe9cca7205b0dbec7613038b6fc6d Mon Sep 17 00:00:00 2001 From: Ryan Malloy Date: Thu, 3 Sep 2026 15:51:46 -0600 Subject: [PATCH] Testing Row found the same mistake I keep making I hand-listed the names a column could shadow: count and index, the two tuple methods. Then I wrote keys(), _asdict() and _fields on the same class and did not go back. A column called "keys" returned a bound method instead of a value, silently, which is precisely the failure shape this driver spent a fortnight removing from its decoders. The fix is not a longer list. The reserved set is now computed from dir(Row), so adding a method later cannot reopen the hole, and the test is parametrized over that computed set so it grows with the class. _fields and _map moved to name-mangled attributes. They are read by repr() and _asdict(), so shadowing the public _fields with a column would have made the machinery report the column value instead of the column names. Mangling keeps the two apart, and there is a test that shadows _fields and checks repr still works. Also exercised, and all clean: value-identical output against the plain tuple path across twenty columns covering every awkward type plus a fully NULL row, which is the strongest available statement that Row is a presentation layer and not a bug; zero-column and 500-column rows; unicode, spaced, digit-leading, empty-string and dunder column names; rows outliving eviction of their class from the bounded cache; the four async fetch routes and the async pool; and twelve threads racing on the class cache, which correctly share one class per shape. 472 tests on 15 and 14.10, 471 on 12.10. --- src/informix_db/rows.py | 93 +++++++++++------- tests/test_async_threads.py | 49 ++++++++++ tests/test_rows.py | 189 ++++++++++++++++++++++++++++++++++-- 3 files changed, 287 insertions(+), 44 deletions(-) diff --git a/src/informix_db/rows.py b/src/informix_db/rows.py index e427da6..6b71a22 100644 --- a/src/informix_db/rows.py +++ b/src/informix_db/rows.py @@ -33,19 +33,28 @@ a choice. ``Row`` subclasses ``tuple``, so ``row == (1, "x")`` is still true and existing code keeps working unchanged. -**Two names are shadowed.** ``tuple`` already has ``count`` and -``index`` methods. A column called either one wins, because on a -database row that is obviously what the caller meant, and the tuple -methods remain reachable as ``tuple.count(row, x)``. Every other column -name resolves through ``__getattr__``, which only runs after normal -lookup fails and so costs nothing for the names that do not collide. +**A column always beats a method of the same name.** ``tuple`` defines +``count`` and ``index``; this class adds ``keys``, ``_asdict`` and +``_fields``. A column named any of those would otherwise resolve to the +method and hand back a bound method instead of a value, which is exactly +the shape of the framing bugs this driver spent a fortnight removing: a +plausible-looking wrong answer, in silence. So every such name gets a +descriptor and the column wins, and the reserved set is *computed* from +the class rather than hand-listed, because hand-listing it is what +missed ``keys``, ``_asdict`` and ``_fields`` the first time round. The +methods stay reachable through the class: ``tuple.count(row, x)``, +``Row.keys(row)``. + +The machinery itself is name-mangled (``__fields`` / ``__map``) so that +shadowing ``_fields`` cannot break ``repr`` or ``_asdict``. Column names come from ``cursor.description``. Informix folds unquoted identifiers to lower case, so ``SELECT Config_Key`` is reachable as ``row.config_key``. Expressions get server-generated names that are not Python identifiers, like ``(count(*))``; those are reachable by subscript but not as attributes. Duplicate names resolve to the first -occurrence, matching ``pyodbc``. +occurrence, matching ``pyodbc``. Dunder names cannot be shadowed and +stay subscript-only, which no real schema should notice. """ from __future__ import annotations @@ -56,11 +65,6 @@ from typing import ClassVar __all__ = ["Row", "make_row_class"] -# Attributes ``tuple`` already defines that a column could plausibly be -# named. A column with one of these names gets a descriptor so the column -# wins; see the module docstring. -_TUPLE_ATTRS = frozenset({"count", "index"}) - class Row(tuple): """A result row addressable by position, name, or attribute. @@ -72,61 +76,78 @@ class Row(tuple): __slots__ = () - # Overridden on the per-result-set subclass. - _fields: ClassVar[tuple[str, ...]] = () - _map: ClassVar[dict[str, int]] = {} + # Name-mangled to ``_Row__fields`` / ``_Row__map`` so that a column + # called "_fields" can be shadowed without breaking the machinery + # that reads it. Set on the per-result-set subclass. + __fields: ClassVar[tuple[str, ...]] = () + __map: ClassVar[dict[str, int]] = {} def __getitem__(self, key): - # ``key.__class__ is str`` rather than isinstance: this is the - # hot path, and it runs on every subscript. + # ``key.__class__ is str`` rather than isinstance: this runs on + # every subscript. if key.__class__ is str: try: - return tuple.__getitem__(self, self._map[key]) + return tuple.__getitem__(self, self.__map[key]) except KeyError: raise KeyError( f"no column named {key!r}; this row has " - f"{list(self._fields)}" + f"{list(self.__fields)}" ) from None return tuple.__getitem__(self, key) def __getattr__(self, name): # Only reached when normal attribute lookup has already failed, - # so this costs nothing for real tuple attributes. + # so this costs nothing for names that do not collide. try: - return tuple.__getitem__(self, self._map[name]) + return tuple.__getitem__(self, self.__map[name]) except KeyError: raise AttributeError( f"no column named {name!r}; this row has " - f"{list(self._fields)}" + f"{list(self.__fields)}" ) from None + @property + def _fields(self) -> tuple[str, ...]: + """Column names, in select order. Mirrors ``namedtuple._fields``.""" + return self.__fields + def keys(self) -> tuple[str, ...]: """Column names, in select order.""" - return self._fields + return self.__fields def _asdict(self) -> dict: """A plain ``dict`` of the row. - On duplicate column names the last occurrence wins here, which - differs from subscript access where the first does. A dict cannot - represent both, and losing a duplicate silently is better than - raising on a query that is otherwise fine. + On duplicate column names the last occurrence wins here, while + subscript access gives the first. A dict cannot represent both, + and quietly dropping a duplicate is better than raising on a + query that is otherwise fine. """ - return dict(zip(self._fields, self, strict=True)) + return dict(zip(self.__fields, self, strict=True)) def __repr__(self) -> str: - if not self._fields: + fields = self.__fields + if len(fields) != len(self): return tuple.__repr__(self) body = ", ".join( f"{name}={value!r}" - for name, value in zip(self._fields, self, strict=True) + for name, value in zip(fields, self, strict=True) ) return f"Row({body})" def __reduce__(self): # The per-result-set class is created at runtime and cannot be # pickled by reference, so rebuild it from the field names. - return (_rebuild_row, (self._fields, tuple(self))) + return (_rebuild_row, (self.__fields, tuple(self))) + + +# Every non-dunder attribute a Row already answers to. A column with one +# of these names gets a descriptor so the column wins. Computed, not +# hand-listed: the hand-listed version covered ``count`` and ``index`` +# and silently missed ``keys``, ``_asdict`` and ``_fields``. +_RESERVED = frozenset( + name for name in dir(Row) if not name.startswith("__") +) - {"_Row__fields", "_Row__map"} def _rebuild_row(fields: tuple[str, ...], values: tuple): @@ -144,14 +165,16 @@ def make_row_class(fields: tuple[str, ...]) -> type[Row]: """ namespace: dict = { "__slots__": (), - "_fields": fields, + "_Row__fields": fields, # First occurrence wins on duplicates, matching pyodbc. Building # the map in reverse and letting earlier entries overwrite later # ones is the shortest way to say that. - "_map": {name: i for i, name in reversed(list(enumerate(fields)))}, + "_Row__map": { + name: i for i, name in reversed(list(enumerate(fields))) + }, } for i, name in enumerate(fields): - if name in _TUPLE_ATTRS: - # Shadow the tuple method so the column wins. + if name in _RESERVED: + # A column of this name would otherwise resolve to a method. namespace[name] = property(operator.itemgetter(i)) return type("Row", (Row,), namespace) diff --git a/tests/test_async_threads.py b/tests/test_async_threads.py index 1819997..54e46ca 100644 --- a/tests/test_async_threads.py +++ b/tests/test_async_threads.py @@ -214,3 +214,52 @@ async def test_closing_a_connection_stops_its_thread( f"connection threads outlived their connections " f"({before} -> {threading.active_count()})" ) + + +@pytest.mark.asyncio +async def test_row_factory_reaches_the_async_paths( + conn_params: ConnParams, +) -> None: + """The async layer wraps the sync cursor, so row_factory should flow + through untouched. Each of the four async fetch routes is a separate + call site, and async iteration goes through __anext__ rather than + __next__.""" + import informix_db + + conn = await aio.connect(row_factory=informix_db.Row, **_kw(conn_params)) + try: + cur = await conn.cursor() + sql = "SELECT FIRST 2 tabid, tabname FROM systables ORDER BY tabid" + + await cur.execute(sql) + one = await cur.fetchone() + assert one[0] == one["tabid"] == one.tabid + + await cur.execute(sql) + assert [r.tabid for r in await cur.fetchall()] == [1, 2] + + await cur.execute(sql) + assert all(r.tabname for r in await cur.fetchmany(2)) + + await cur.execute(sql) + assert [r.tabid async for r in cur] == [1, 2] + finally: + await conn.close() + + +@pytest.mark.asyncio +async def test_async_pool_forwards_the_row_factory( + conn_params: ConnParams, +) -> None: + import informix_db + + pool = await aio.create_pool( + row_factory=informix_db.Row, min_size=1, max_size=2, **_kw(conn_params) + ) + try: + async with pool.connection() as conn: + cur = await conn.cursor() + await cur.execute("SELECT FIRST 1 tabid FROM systables") + assert (await cur.fetchone()).tabid == 1 + finally: + await pool.close() diff --git a/tests/test_rows.py b/tests/test_rows.py index c4c0af3..f19d4e0 100644 --- a/tests/test_rows.py +++ b/tests/test_rows.py @@ -20,12 +20,13 @@ places, and so does everybody's code. from __future__ import annotations +import contextlib import pickle import pytest import informix_db -from informix_db.rows import Row, make_row_class +from informix_db.rows import _RESERVED, Row, make_row_class from tests.conftest import ConnParams # --------------------------------------------------------------------------- @@ -114,14 +115,37 @@ def test_duplicate_names_resolve_to_the_first() -> None: assert row[1] == 2, "positional access must still reach the second" -def test_column_named_count_beats_the_tuple_method() -> None: - """``tuple`` already has ``count`` and ``index``. On a database row a - column with that name is what the caller meant, so the column wins - and the tuple method stays reachable through the class.""" - row = _row(("count", "index"), (7, 9)) - assert row.count == 7 - assert row.index == 9 - assert tuple.count(row, 7) == 1 +@pytest.mark.parametrize("name", sorted(_RESERVED)) +def test_a_column_always_beats_a_method_of_the_same_name(name: str) -> None: + """Parametrized over the *computed* reserved set rather than a + hand-written list, because the hand-written list is what missed + ``keys``, ``_asdict`` and ``_fields`` on the first attempt. Adding a + method to Row later cannot silently reopen the hole: this test grows + with it. + + Without the guard these return a bound method, which is the exact + failure shape this driver spent a fortnight removing from its + decoders: a plausible-looking wrong answer, in silence.""" + row = _row((name, "other"), (7, 9)) + assert getattr(row, name) == 7 + assert row[name] == 7 + + +def test_shadowed_methods_stay_reachable_through_the_base_class() -> None: + row = _row(("count", "keys", "_asdict"), (1, 2, 3)) + assert tuple.count(row, 1) == 1 + assert Row.keys(row) == ("count", "keys", "_asdict") + assert Row._asdict(row) == {"count": 1, "keys": 2, "_asdict": 3} + + +def test_shadowing_fields_does_not_break_repr_or_asdict() -> None: + """The machinery reads its own field list, so if a column named + ``_fields`` shadowed it, repr and _asdict would report the column + value instead of the names. Mangled attributes keep them separate.""" + row = _row(("_fields", "x"), ("not the names", 2)) + assert row._fields == "not the names" + assert Row.keys(row) == ("_fields", "x") + assert repr(row) == "Row(_fields='not the names', x=2)" def test_non_identifier_names_are_subscript_only() -> None: @@ -262,3 +286,150 @@ def test_pool_forwards_the_factory(conn_params: ConnParams) -> None: assert cur.fetchone().tabid is not None finally: pool.close() + + +def test_zero_column_row() -> None: + row = make_row_class(())(()) + assert row == () + assert row.keys() == () + assert repr(row) == "Row()" + + +def test_wide_row() -> None: + """500 columns: the name map is a dict, so this should be flat, but + it is the shape most likely to expose an off-by-one.""" + fields = tuple(f"c{i}" for i in range(500)) + row = make_row_class(fields)(tuple(range(500))) + assert row["c499"] == row.c499 == row[499] == 499 + assert row["c0"] == row[0] == 0 + + +def test_names_that_cannot_be_attributes_are_subscript_only() -> None: + row = _row(("col with space", "1leading_digit", ""), (1, 2, 3)) + assert row["col with space"] == 1 + assert row["1leading_digit"] == 2 + assert row[""] == 3 + + +def test_unicode_column_name() -> None: + row = _row(("café", "x"), (1, 2)) + assert row["café"] == 1 + assert row.café == 1 + + +def test_dunder_named_column_is_subscript_only() -> None: + """A dunder cannot be shadowed without breaking the object protocol, + so the column is reachable by subscript and the attribute keeps its + ordinary meaning. No real schema should notice.""" + row = _row(("__class__", "ok"), (1, 2)) + assert row["__class__"] == 1 + assert row.__class__.__name__ == "Row" + + +def test_row_outlives_eviction_of_its_class_from_the_cache() -> None: + """The class cache is bounded. A row already handed to the caller + holds its class alive, so eviction must not affect it.""" + row = make_row_class(("z1", "z2"))((9, 8)) + for i in range(300): + make_row_class((f"evict{i}", "b")) + assert (row.z1, row["z2"], row[0]) == (9, 8, 9) + + +def test_class_cache_is_bounded() -> None: + for i in range(400): + make_row_class((f"bounded{i}", "b")) + assert make_row_class.cache_info().currsize <= 256 + + +# --------------------------------------------------------------------------- +# Row must not change a single value +# --------------------------------------------------------------------------- + +_WIDE_DDL = """CREATE TABLE t_rowdiff ( + c_int INT, c_small SMALLINT, c_big BIGINT, c_int8 INT8, c_serial SERIAL, + c_float FLOAT, c_smallfloat SMALLFLOAT, c_dec DECIMAL(16,4), + c_decu DECIMAL(16), c_money MONEY(12,2), c_char CHAR(10), + c_vchar VARCHAR(40), c_nchar NCHAR(8), c_lvar LVARCHAR(200), c_date DATE, + c_dt DATETIME YEAR TO FRACTION(5), c_ivl INTERVAL YEAR TO MONTH, + c_bool BOOLEAN, c_set SET(INT NOT NULL), c_tail INT)""" + +_WIDE_COLS = ( + "c_int,c_small,c_big,c_int8,c_serial,c_float,c_smallfloat,c_dec,c_decu," + "c_money,c_char,c_vchar,c_nchar,c_lvar,c_date,c_dt,c_ivl,c_bool,c_set," + "c_tail" +) + + +@pytest.mark.integration +def test_rows_are_value_identical_to_tuples(conn_params: ConnParams) -> None: + """The strongest statement available: across every awkward type, with + a fully populated row and a fully NULL one, wrapping must change + nothing. If it does, Row is not a presentation layer, it is a bug.""" + sql = f"SELECT {_WIDE_COLS} FROM t_rowdiff ORDER BY c_tail" + with _connect(conn_params) as conn: + cur = conn.cursor() + with contextlib.suppress(Exception): + cur.execute("DROP TABLE t_rowdiff") + cur.execute(_WIDE_DDL) + try: + cur.execute( + "INSERT INTO t_rowdiff VALUES (1,2,3,4,0,1.5,2.5,12.34,99," + "9.99,'ch','vc','nc','lv',TODAY,CURRENT," + "INTERVAL(1-2) YEAR TO MONTH,'t',SET{1,2},77)" + ) + cur.execute( + "INSERT INTO t_rowdiff VALUES (NULL,NULL,NULL,NULL,0,NULL," + "NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL," + "NULL,NULL,88)" + ) + cur.execute(sql) + plain = cur.fetchall() + + with _connect(conn_params, row_factory=informix_db.Row) as named_conn: + named = named_conn.cursor() + named.execute(sql) + rows = named.fetchall() + + assert rows == plain, "wrapping changed a value" + names = _WIDE_COLS.split(",") + first = rows[0] + for i, name in enumerate(names): + assert first[i] == first[name] == getattr(first, name), name + assert rows[1]["c_int"] is None + assert rows[1].c_tail == 88 + finally: + with contextlib.suppress(Exception): + cur.execute("DROP TABLE t_rowdiff") + + +@pytest.mark.integration +def test_threads_share_one_class_per_shape(conn_params: ConnParams) -> None: + """The cache is process-wide and the pool hands connections to many + threads, so two threads running the same query must land on the same + class rather than racing to build competing ones.""" + import threading + + seen: list[type] = [] + errors: list[Exception] = [] + + def worker() -> None: + try: + with _connect(conn_params, row_factory=informix_db.Row) as conn: + cur = conn.cursor() + for _ in range(5): + cur.execute( + "SELECT FIRST 1 tabid, tabname FROM systables" + ) + row = cur.fetchone() + assert row.tabname == row["tabname"] == row[1] + seen.append(type(row)) + except Exception as exc: + errors.append(exc) + + threads = [threading.Thread(target=worker) for _ in range(6)] + for t in threads: + t.start() + for t in threads: + t.join(60) + assert not errors, errors[:2] + assert len({id(c) for c in seen}) == 1, "same shape built more than once"