Wrapper-bypass audit (dol #83): `.query` and `.setup()` escape `SQLAlchemyTupleStore`'s codecs — plus 4 survey claims refuted

[thorwhalen]

Summary

Audit of sqldol against the wrapper-bypass defect tracked in i2mint/dol#83 (umbrella) / i2mint/dol#18 (root cause).

Result: 2 confirmed-live, 3 latent, 4 refuted. Everything below was run for real against sqlite with SQLAlchemy 2.0.51 and dol master — no synthetic stand-ins were needed.

The headline is a narrow one, and deliberately so: sqldol applies zero key transforms of its own, so the classic "wrapper maps the key, the delegated method doesn't" failure has essentially no surface here. What it does have is the sibling form of the same defect — scope-wide leaf state and scope-wide leaf mutators handed straight through the wrapper.

Two of the six symbols named in the originating survey are wrong and are refuted below with evidence. In particular, teardown() does not drop any table.

Mechanism (short)

dol wraps stores by delegation (has-a). A wrapper maps keys and values correctly for __getitem__ / __setitem__ / __delitem__ / __contains__ / __iter__. Every other non-dunder attribute is served leaf-bound, in the leaf's coordinate system, by one of two routes — both in dol/base.py:

• Route A — instance wraps. Store.__getattr__ (dol/base.py:742) returns getattr(self.store, attr).
• Route B — class wraps. delegate_to (dol/base.py:416-480) installs a DelegatedAttribute descriptor for every attribute in dir(wrapped); DelegatedAttribute.__get__ (dol/base.py:279) also returns getattr(instance.store, attr).

Both routes are live in sqldol, and I confirmed each at runtime.

How sqldol actually wraps (census)

wrapper	count	what it wraps
mk_relative_path_store	0	—
KeyCodecs	0	—
prefixless_view	0	—
filt_iter	0	—
PrefixRelativizationMixin	0	—
wrap_kvs	5 calls (sqldol/stores.py:9, 23, 50, 86, 87), applied as 5 class decorators (:90, 95, 100, 105, 109)	SqlBaseKvReader / SqlBaseKvStore — all obj_of_data= only, no id_of_key, no key_of_id, no data_of_obj
dol.base.Store subclass	2 (sqldol/sql_base.py:409, :416)	SQLAlchemyStore = identity codec; SQLAlchemyTupleStore = the package's only key transform (_id_of_key :421, _key_of_id :424, _data_of_obj :431, _obj_of_data :435)

Delegated-attribute sets, enumerated at runtime:

• Route A on SQLAlchemyTupleStore: _collection_name, _create_table, _uri, autocommit, connection, query, session, setup, table, table_columns, teardown, TYPE_*. None of these takes a key.
• Route B on SqlDictStore: DelegatedAttribute descriptors for _extract_key, _extract_values, _mk_column_filter, _prepare. Of these, only _mk_column_filter(key) takes a key.

Findings

symbol	location	verdict	severity
SQLAlchemyPersister.query	sqldol/sql_base.py:339	confirmed-live	value-side
SQLAlchemyPersister.setup	sqldol/sql_base.py:292	confirmed-live	wrong-scope
SqlBaseKvStore._mk_column_filter	sqldol/base.py:255	latent	wrong-scope
SqlTableRowsCollection.count_rows	sqldol/sql_base.py:59	latent	wrong-scope
SqlTableRowsCollection.refresh_row_count	sqldol/sql_base.py:71	latent	wrong-scope
SQLAlchemyPersister.teardown	sqldol/sql_base.py:306	refuted	—
SQLAlchemyPersister.table_columns	sqldol/sql_base.py:303	refuted (as a scoping bug)	—
TableRows.table_name	sqldol/base.py:105	refuted	—
TableRows.column_names	sqldol/base.py:109	refuted	—

1. SQLAlchemyPersister.query — confirmed-live, value-side

# sqldol/sql_base.py:338-340
@property
def query(self):
    return self.session.query(self.table)

SQLAlchemyTupleStore promises tuple keys and tuple values. .query — reached through Route A — hands back a leaf sqlalchemy.orm.Query yielding raw DeclarativeTable ORM instances, bypassing _key_of_id and _obj_of_data entirely. The same applies to the delegated .table, .session and .connection.

User-visible consequence: .query is the only querying API the class offers beyond __getitem__, so it is exactly what a user reaches for to do anything non-trivial. Iterating the store gives ('a',); iterating store.query gives an ORM object. Anyone who mixes the two — filter with .query, then feed the results back through the store — is silently working in two different coordinate systems. Nothing raises. No data is destroyed.

2. SQLAlchemyPersister.setup — confirmed-live, wrong-scope

# sqldol/sql_base.py:292-301
def setup(self, db_uri, collection_name, **db_kwargs):
    engine = create_engine(db_uri, **db_kwargs)
    self.connection = engine.connect()
    self.table = self._create_table(collection_name, engine)
    self.session = sessionmaker(bind=engine)()

setup is public, delegated unchanged to every wrapper above it, and repoints the entire store at a different database and table mid-life. self._uri and self._collection_name are set only in __init__ (:288-289) and are not updated, so afterwards the object misreports which database it is on.

Qualifier, stated plainly: setup takes no key. This is not the key-mis-mapping form of dol#83 — it is the scope-wide-mutator form. Calling it is not an accident a user stumbles into; the defect is that a store's public surface includes a whole-store rebind that no wrapper can intercept, invalidate, or veto.

User-visible consequence: a wrapped store silently starts serving a different table. Writes that the caller believes are landing in the original table land in the new one. No data already written is destroyed — the original table is untouched, just orphaned.

3. SqlBaseKvStore._mk_column_filter — latent, wrong-scope (not in the originating survey)

# sqldol/base.py:255-271
def _mk_column_filter(self, key):
    if isinstance(key, str):
        return text(f"{self.key_columns} = '{key}'")
    ...

The only key-taking non-dunder method in the package, and it is installed as a Route-B DelegatedAttribute on all five stores.py classes. Because sqldol's own wraps are value-only, it receives the correct key today. Add a key codec and it receives the outer key. Impact is bounded: it is private, and the leaf's own __setitem__ / __delitem__ call it with an already-mapped key, so only a direct external call is wrong.

4-5. SqlTableRowsCollection.count_rows / refresh_row_count — latent, not live

The survey's framing ("they disagree with len(wrapped)") does not hold: SqlTableRowsCollection has no filt, is constructed bare by SqlDbReader.__getitem__ (sqldol/sql_base.py:197-198), and is wrapped by nothing in the package. There is no wrapper for them to disagree with. They become a genuine hazard only if a user stacks filt_iter on top, at which point len() narrows and count_rows() does not.

Caveat that matters more than the finding: setup.cfg pins sqlalchemy with no upper bound, so a fresh install gets 2.x, where this entire legacy layer is dead code. Observed on 2.0.51:

count_rows   -> ObjectNotExecutableError: Not an executable object: 'SELECT COUNT(*) FROM t'
len          -> ObjectNotExecutableError: Not an executable object: 'SELECT COUNT(*) FROM t'
column_names -> ObjectNotExecutableError: Not an executable object: 'DESCRIBE t'
SqlDbReader  -> ObjectNotExecutableError: Not an executable object: 'show tables'
TableRows    -> ValueError: __len__() should return >= 0   (rowcount is -1 for SELECT)

That is a bigger problem than the scoping question and deserves its own issue.

Refuted claims

teardown() does NOT drop the table. The survey alleged it "DROPS the leaf table regardless of outer scoping". The actual body is:

# sqldol/sql_base.py:306-308
def teardown(self):
    self.session.close()
    self.connection.close()

rg -n 'drop|DROP' across the package returns exactly one hit, sqldol/util.py:183, inside create_table_from_dict — unrelated and not delegated. Verified destructively: after teardown() the tables are still in sqlite_master. No data is destroyed.

table_columns() is not a scoping bug. It takes no key, and column names are schema metadata invariant under key codecs. It is broken, for unrelated reasons: self.table is the ORM class from _create_table (:317-336), so the emitted SQL is literally DESCRIBE <class '...DeclarativeTable'>, and raw-string execute is rejected by SQLAlchemy 2.x.

TableRows.table_name / .column_names are not affected. TableRows (sqldol/base.py:85) subclasses Sized, Iterable — no __getitem__, therefore not a Mapping, therefore not meaningfully wrappable by a dol key codec. Its filt filters rows; table identity and column names are invariant under both filtering and key transforms. Neither property can return the wrong thing in any scenario, live or latent.

Repro — REAL (not synthetic)

Run against sqldol @ e9e69de, dol master, SQLAlchemy 2.0.51, sqlite.

import tempfile, sqlite3, sqlalchemy
from sqldol import SQLAlchemyTupleStore

d = tempfile.mkdtemp()
uri_a, uri_b = f'sqlite:///{d}/a.db', f'sqlite:///{d}/b.db'
s = SQLAlchemyTupleStore(
    uri_a, 'tbl',
    key_fields={'id': sqlalchemy.String}, data_fields={'data': sqlalchemy.String},
)
s[('a',)] = ('one',)

# --- 1. query: the store's codec is bypassed -------------------------------
print('list(s)       ->', list(s))        # [('a',)]                 store coordinates
print('list(s.query) ->', list(s.query))  # [<DeclarativeTable ...>] leaf coordinates

# --- 2. setup: scope-wide rebind through the wrapper -----------------------
print('table ->', s.store.table.__table__.name, '| _uri ->', s.store._uri)
s.setup(uri_b, 'other_tbl')               # delegated straight to the leaf
print('list(s) ->', list(s))              # []   <- different DB *and* table
print('table ->', s.store.table.__table__.name, '| _uri ->', s.store._uri)  # _uri is stale

# --- 3. teardown does NOT drop anything (claim refuted) --------------------
s.teardown()
for name in ('a', 'b'):
    con = sqlite3.connect(f'{d}/{name}.db')
    print(f'{name}.db ->', con.execute(
        "select name from sqlite_master where type='table'").fetchall())

# --- 4. table_columns() is simply broken (unrelated to key scoping) --------
try:
    s.table_columns()
except Exception as e:
    print('table_columns ->', type(e).__name__, str(e)[:90])

Observed output:

list(s)       -> [('a',)]
list(s.query) -> [<sqldol.sql_base.SQLAlchemyPersister._create_table.<locals>.DeclarativeTable object at 0x...>]
table -> tbl       | _uri -> sqlite:///.../a.db
list(s) -> []
table -> other_tbl | _uri -> sqlite:///.../a.db
a.db -> [('tbl',)]
b.db -> [('other_tbl',)]
table_columns -> ObjectNotExecutableError Not an executable object: "DESCRIBE <class 'sqldol.sql_base...

Repro for the latent Route-B case:

import tempfile
from dol import KeyCodecs
from sqldol import SqlDictStore
from sqldol.util import create_table_from_dict

d = tempfile.mkdtemp(); uri = f'sqlite:///{d}/a.db'
create_table_from_dict({'k': ['u/a', 'u/b'], 'v': [1, 2]}, engine=uri, table_name='t')

w = KeyCodecs.prefixed('u/')(SqlDictStore)(uri, 't', key_columns='k')
print(w['a'])                             # {'k': 'u/a', 'v': 1}   correct
print(w._mk_column_filter('a'))           # k = 'a'                WRONG
print(w.store._mk_column_filter('u/a'))   # k = 'u/a'              expected

Suggested remediation

Shrinking the surface is the right fix here, not patching each method. Because sqldol has almost no key-taking delegated methods, per-method key translation buys very little. What actually causes trouble is that the leaf's whole lifecycle and connection surface is reachable from every wrapper.

1. setup should not be public. Rename to _setup (or fold it into __init__ and make the persister immutable in its table binding). If in-place rebinding must stay supported, it should mint a new persister rather than mutate the live one, and it must update _uri / _collection_name so the object stops misreporting itself. This is the same move azuredol made with BlobHandle: put per-target state on a handle keyed at construction instead of exposing a mutator on the shared store.

2. query / table / session / connection should be marked as leaf escape hatches, either by renaming them with a leading underscore or by documenting in the class docstring that they operate in the persister's coordinate system, not the store's. Alternatively, SQLAlchemyTupleStore can override query to map results through _key_of_id / _obj_of_data.

3. _mk_column_filter is the one place where the standard dol escape hatch applies, if it is ever promoted to public:

   from dol import wrapped_self
   from dol.dig import inner_most_key   # NOTE: not exported from dol itself
   
   def _mk_column_filter(self, key):
       leaf_key = inner_most_key(wrapped_self(self), key)
       if not isinstance(leaf_key, str):
           leaf_key = self._id_of_key(key)   # nothing in the chain defines _id_of_key
       ...

   Two traps, both mandatory to respect:

   • inner_most_key walks the whole chain including the leaf's own _id_of_key. It replaces self._id_of_key(k) — never compose the two, or the key is transformed twice.
   • It returns None silently when no layer in the chain defines _id_of_key, so the isinstance check is not optional.

4. Out of scope for this issue but more urgent: the legacy sql_base.py layer (SqlTableRowsCollection, SqlDbCollection, SqlDbReader, iter_rows, table_columns) and TableRows.__len__ are non-functional under SQLAlchemy 2.x, which is what the unpinned dependency installs today. Either cap sqlalchemy<2, port the raw-string executes to text(...), or delete the layer.

References

• Key-transform wrappers delegate capability methods with the unmapped key (url_for etc. silently address the wrong object) dol#83 — umbrella issue for the wrapper-bypass defect class
• wrap_kvs will wrap the instance but self of instance is not wrapped dol#18 — root cause (delegation hands leaf-bound methods the outer key)

---

Note on in-flight upstream fixes (added when filing)

Two dol PRs are open and change details referenced above:

• i2mint/dol#84 — inner_most_key and unravel_key
  become importable from dol directly (no more from dol.dig import ...), and
  inner_most_key now raises instead of returning None when no layer of the chain
  defines _id_of_key. If you write a local shim, the isinstance(_id, str) guard becomes
  unnecessary once that lands — but the "it replaces _id_of_key, never composes with it" trap
  still applies.
• i2mint/dol#85 — fixes dol.content_url to
  resolve the key through wrapping layers, and makes mk_relative_path_store install
  key-mapping is_valid_key/validate_key. Any finding above that is inherited from
  dol.filesys.Files is repaired by #85 with no change needed in this repo — this issue will
  be closed with verification once it merges.

Design context for why the ecosystem-wide answer is not "sprinkle wrapped_self everywhere":
i2mint/s3dol#14 and
s3dol ADR-0011.
Short version: wrapped_self is a best-effort guardrail with its own silent failure mode (it
degrades to the raw leaf when nothing holds a reference to the wrapper), so the durable fix is
to have no key-taking methods rather than to harden each one.