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.
This commit is contained in:
parent
805fa58eb2
commit
ce5a582048
@ -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)
|
||||
|
||||
@ -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()
|
||||
|
||||
@ -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"
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user