A list column backfilled with a dictionary
Reported as a 500 on a live instance, immediately after it updated, and read
off its journal rather than guessed at:
ValueError: Attribute 'reasoning_efforts' does not accept objects of
type <class 'dict'>
`Mapped[list[str]]` is not Optional, so the column is NOT NULL, so SQLite
demands a default for the rows that already exist. `_literal_default` chose one
by asking `column.type.python_type` -- and `MutableList.as_mutable(JSON)`
returns the *same* JSON type object with a listener attached rather than
subclassing it, so `python_type` is `dict` for both flavours. Every existing row
got '{}' in a list column, and MutableList refuses a dict while *loading*: not a
wrong value sitting quietly, an exception on every read of the table.
Model.reasoning_efforts was the first list-shaped JSON column this project had
ever added to a table that already had rows, so the flaw had been harmless since
the runner was written. 1.2.0 stepped on it.
The shape now comes from the column's Python-side default -- `default=list`
against `default=dict` -- which is the only thing that can tell the two apart.
And `repair_json_shapes` puts right what was already written, on start,
converging like ensure_fts beside it, narrow enough that a legitimate {} in a
dict column survives.
Why 1981 tests missed it: conftest builds a fresh database, where the column is
created from the model with its real default. The backfill only runs on a
database that already exists, so the suite had never once exercised the path
that broke. The new tests corrupt a row exactly as the migration did and assert
it loads again.
Verified against a backup of the reporting instance's own database: the load
raises before, eleven rows are repaired, all eleven models load after.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -230,3 +230,124 @@ def test_each_new_table_is_usable_after_the_upgrade(db, table):
|
||||
|
||||
declared = {column.name for column in Base.metadata.tables[table].c}
|
||||
assert _columns(engine, table) == declared
|
||||
|
||||
|
||||
# --- A list-shaped JSON column added to a database that already had rows -----
|
||||
#
|
||||
# Reported as a 500 on a live instance the moment it updated:
|
||||
#
|
||||
# ValueError: Attribute 'reasoning_efforts' does not accept objects
|
||||
# of type <class 'dict'>
|
||||
#
|
||||
# `_literal_default` read the shape off `column.type.python_type`, and
|
||||
# `MutableList.as_mutable(JSON)` returns the *same* JSON type object with a
|
||||
# listener attached -- it does not subclass it -- so `python_type` is `dict` for
|
||||
# both flavours. Every existing row got `'{}'` in a list column, and MutableList
|
||||
# refuses a dict while *loading*, so every page that listed models raised.
|
||||
#
|
||||
# The suite never caught it because `conftest.py` builds a fresh database, where
|
||||
# the column is created from the model rather than backfilled by a migration.
|
||||
# These tests exercise the path that actually ran.
|
||||
def test_a_list_column_is_backfilled_with_a_list():
|
||||
from lembas.db.migrations import _default_shape, _literal_default
|
||||
from lembas.db.models import Model
|
||||
|
||||
columns = {c.name: c for c in Model.__table__.columns}
|
||||
assert _default_shape(columns["reasoning_efforts"]) is list
|
||||
assert _literal_default(columns["reasoning_efforts"]) == "'[]'"
|
||||
|
||||
|
||||
def test_a_dict_column_still_gets_a_dict():
|
||||
from lembas.db.migrations import _literal_default
|
||||
from lembas.db.models import Model
|
||||
|
||||
columns = {c.name: c for c in Model.__table__.columns}
|
||||
assert _literal_default(columns["capabilities_json"]) == "'{}'"
|
||||
assert _literal_default(columns["params_json"]) == "'{}'"
|
||||
|
||||
|
||||
def _seed_model(engine, **overrides):
|
||||
"""A real row, made the way the application makes one.
|
||||
|
||||
Built through the ORM rather than a hand-written INSERT: the table has
|
||||
several NOT NULL columns and a test that enumerates them is a test that
|
||||
breaks every time one is added, for reasons having nothing to do with what
|
||||
it is checking.
|
||||
"""
|
||||
from sqlalchemy.orm import Session
|
||||
|
||||
from lembas.db.models import Connection, Model
|
||||
|
||||
with Session(engine) as session:
|
||||
connection = Connection(
|
||||
name="local", base_url="http://127.0.0.1:1", api_key_encrypted=""
|
||||
)
|
||||
session.add(connection)
|
||||
session.flush()
|
||||
model = Model(connection_id=connection.id, model_id="bonsai", **overrides)
|
||||
session.add(model)
|
||||
session.commit()
|
||||
return model.id
|
||||
|
||||
|
||||
def test_the_damage_already_written_is_repaired_on_start(tmp_path):
|
||||
"""The fix to `_literal_default` helps the next instance. This is the one
|
||||
that helps the instance that has already updated."""
|
||||
from sqlalchemy import create_engine, text
|
||||
|
||||
from lembas.db.migrations import repair_json_shapes, sync_schema
|
||||
|
||||
engine = create_engine(f"sqlite:///{tmp_path}/repair.db")
|
||||
sync_schema(engine)
|
||||
model_id = _seed_model(engine)
|
||||
|
||||
# Exactly what the broken backfill left behind on a row that predated the
|
||||
# column: the wrong empty value, in a column that refuses it on load.
|
||||
with engine.begin() as connection:
|
||||
connection.execute(
|
||||
text("UPDATE models SET reasoning_efforts = '{}' WHERE id = :id"),
|
||||
{"id": model_id},
|
||||
)
|
||||
|
||||
assert repair_json_shapes(engine)
|
||||
|
||||
with engine.begin() as connection:
|
||||
stored = connection.execute(
|
||||
text("SELECT reasoning_efforts FROM models WHERE id = :id"), {"id": model_id}
|
||||
).scalar()
|
||||
assert stored == "[]"
|
||||
|
||||
# And the row loads again, which is the whole point -- the failure was a
|
||||
# ValueError while reading, not a wrong value sitting harmlessly.
|
||||
from sqlalchemy.orm import Session
|
||||
|
||||
from lembas.db.models import Model
|
||||
|
||||
with Session(engine) as session:
|
||||
assert session.get(Model, model_id).reasoning_efforts == []
|
||||
|
||||
# Converges: a second run finds nothing left to do.
|
||||
assert repair_json_shapes(engine) == []
|
||||
|
||||
|
||||
def test_the_repair_leaves_a_dict_column_alone(tmp_path):
|
||||
"""`{}` is a legitimate value in a MutableDict column and must survive."""
|
||||
from sqlalchemy import create_engine, text
|
||||
|
||||
from lembas.db.migrations import repair_json_shapes, sync_schema
|
||||
|
||||
engine = create_engine(f"sqlite:///{tmp_path}/keep.db")
|
||||
sync_schema(engine)
|
||||
model_id = _seed_model(engine)
|
||||
|
||||
with engine.begin() as connection:
|
||||
connection.execute(
|
||||
text("UPDATE models SET capabilities_json = '{}' WHERE id = :id"),
|
||||
{"id": model_id},
|
||||
)
|
||||
repair_json_shapes(engine)
|
||||
with engine.begin() as connection:
|
||||
stored = connection.execute(
|
||||
text("SELECT capabilities_json FROM models WHERE id = :id"), {"id": model_id}
|
||||
).scalar()
|
||||
assert stored == "{}"
|
||||
|
||||
Reference in New Issue
Block a user