From 0ed7dd9fc8404d4a13cc880181d1355d3bcda2dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Bene=C5=A1?= Date: Fri, 25 Sep 2026 22:59:21 +0000 Subject: [PATCH] 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 `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 --- CHANGELOG.md | 17 +++++ src/lembas/__init__.py | 2 +- src/lembas/db/migrations.py | 93 ++++++++++++++++++++++++++- tests/test_migrations.py | 121 ++++++++++++++++++++++++++++++++++++ 4 files changed, 230 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b6a4e9b..9d936c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,23 @@ for 1.0.0 have something to be assembled from. ## Unreleased +## 1.3.1 + +- Fixed: **updating to 1.2.0 or later broke every page that lists models**, with + a 500 and nothing but the error page to show for it. The per-model reasoning + effort list added in 1.2.0 was the first list-shaped setting this application + had ever added to a table that already had rows in it, and the code that fills + in such a column on existing rows could not tell a list from a dictionary — so + it wrote the wrong kind of empty value into every model, and reading one back + raised rather than returning nothing. + + A fresh install was never affected, which is exactly why it was not caught: + the column is only filled in that way on a database that already existed. + + This release both stops it happening and **puts right the rows already + written**, on start, with nothing to run by hand. If your instance is showing + the error page, updating is the whole fix. + ## 1.3.0 - **A model's reasoning efforts can now be detected rather than known.** There diff --git a/src/lembas/__init__.py b/src/lembas/__init__.py index 005e1a0..508d50f 100644 --- a/src/lembas/__init__.py +++ b/src/lembas/__init__.py @@ -1,3 +1,3 @@ """LLeMbas - a Middle-earth themed web UI for OpenAI-compatible LLM endpoints.""" -__version__ = "1.3.0" +__version__ = "1.3.1" diff --git a/src/lembas/db/migrations.py b/src/lembas/db/migrations.py index 431808b..f4990d1 100644 --- a/src/lembas/db/migrations.py +++ b/src/lembas/db/migrations.py @@ -39,6 +39,29 @@ log = logging.getLogger(__name__) MANUAL_STEPS: list[str] = [] +def _default_shape(column: Column) -> type | None: + """`list` or `dict`, from the column's own Python-side default. + + `default=list` and `default=dict` are how the two JSON flavours are + declared, and SQLAlchemy keeps the callable. Calling it is cheap and is the + only way to tell a MutableList column from a MutableDict one -- see the note + in `_literal_default`. + """ + default = column.default + if default is None or not getattr(default, "is_callable", False): + return None + try: + # SQLAlchemy wraps a zero-argument callable to take a context. + produced = default.arg(None) + except Exception: # noqa: BLE001 - a default we cannot call tells us nothing + return None + if isinstance(produced, list): + return list + if isinstance(produced, dict): + return dict + return None + + def _literal_default(column: Column) -> str | None: """A SQL literal to backfill an existing row's new column with. @@ -63,8 +86,22 @@ def _literal_default(column: Column) -> str | None: if "JSON" in affinity: # MutableList columns must start as [] and MutableDict as {}; guessing # wrong makes the first read blow up rather than return empty. - python_type = getattr(column.type, "python_type", None) - return "'[]'" if python_type is list else "'{}'" + # + # 🚨 NOT `column.type.python_type`. `MutableList.as_mutable(JSON)` + # returns the *same* JSON type object with an event listener attached -- + # it does not subclass or wrap it -- so the type cannot tell you which + # of the two it is, and `JSON.python_type` is `dict` for both. That read + # as "this is a dict column" for every list column, and the first one + # ever added by a migration (`Model.reasoning_efforts`, 1.2.0) arrived + # as `'{}'` on every existing row. `MutableList` refuses a dict, so the + # failure was not an empty list but a ValueError on *load* -- every page + # that lists models, 500, on an instance that had simply been updated. + # + # The Python-side default is the only honest signal: a JSONList column + # is declared `default=list` and a JSONDict one `default=dict`, and + # calling it says which. Anything that cannot be called or produces + # neither falls back to `{}`, which is what this always assumed. + return "'[]'" if _default_shape(column) is list else "'{}'" if "BOOL" in affinity: return "0" if any(token in affinity for token in ("INT", "FLOAT", "NUMERIC", "DECIMAL")): @@ -190,6 +227,50 @@ def ensure_fts(engine: Engine) -> list[str]: return created +def repair_json_shapes(engine: Engine) -> list[str]: + """Put right any JSON column backfilled with the wrong empty value. + + `_literal_default` used to read the shape off `column.type.python_type`, + which is `dict` for a MutableList column as well as a MutableDict one -- so + the first list-shaped JSON column ever added by a migration arrived as + `'{}'` on every row that already existed. `MutableList` refuses a dict, and + refuses it while *loading*, so the symptom was not an empty list but a + `ValueError` and a 500 on every page that touched the table. + + Converges, like `ensure_fts` beside it: it runs on every start, it is + idempotent, and on a database that was never damaged it does nothing. Only + the exact wrong value is rewritten -- `'{}'` in a column whose default + produces a list -- because `{}` cannot be a legitimate value there, while + anything else in that column might be somebody's data. + """ + fixed: list[str] = [] + inspector = inspect(engine) + known = set(inspector.get_table_names()) + + with engine.begin() as connection: + for table in Base.metadata.sorted_tables: + if table.name not in known: + continue + for column in table.columns: + if "JSON" not in column.type.__class__.__name__.upper(): + continue + if _default_shape(column) is not list: + continue + result = connection.execute( + text( + f'UPDATE "{table.name}" SET "{column.name}" = \'[]\' ' + f'WHERE "{column.name}" = \'{{}}\'' + ) + ) + if result.rowcount: + fixed.append(f"{table.name}.{column.name} ({result.rowcount} row(s))") + log.warning( + "repaired %s.%s on %d row(s): was '{}' in a list column", + table.name, column.name, result.rowcount, + ) + return fixed + + def sync_schema(engine: Engine) -> list[str]: """Bring the database up to the declared schema. Returns what it changed.""" import lembas.db.models # noqa: F401 (registers every table on the metadata) @@ -219,6 +300,14 @@ def sync_schema(engine: Engine) -> list[str]: changes.append(f"add column {table.name}.{column.name}") log.info("schema: %s", statement) + # Before the search indexes, and before anything can try to load a row: + # a column left holding the wrong empty value makes the ORM raise on read. + try: + for repair in repair_json_shapes(engine): + changes.append(f"repair {repair}") + except Exception: # noqa: BLE001 - a repair that fails must not stop a start + log.exception("could not repair JSON column shapes") + try: for index in ensure_fts(engine): changes.append(f"create search index {index}") diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 70bbe58..cd9070a 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -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 +# +# `_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 == "{}"