From 2f978d84d13e1e0234ccd25c1a14e19685478f8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Bene=C5=A1?= Date: Tue, 21 Jul 2026 20:03:01 +0200 Subject: [PATCH] Add nullable columns without a default Deploying knowledge bases showed the migration runner doing the wrong thing: ALTER TABLE "documents" ADD COLUMN "base_id" VARCHAR(32) DEFAULT '' `base_id` is nullable and its absent value is NULL, but the runner derived a default from the column type and backfilled every existing row with the empty string. Nothing then matched `base_id IS NULL`, so the startup sweep that files pre-bases documents into a default base would have skipped all of them and the documents would have stayed invisible. Nobody lost anything -- the live instance had no documents yet -- but the fault is general: any nullable column added from here would arrive as "" rather than NULL, and every "is this set?" check would be wrong about the rows that predate it. So a default is now emitted only for NOT NULL columns, where SQLite requires one. The sweep also accepts "" as meaning unfiled, since a deployment that upgraded through the previous release has rows holding it. Co-Authored-By: Claude Opus 4.8 (1M context) --- CLAUDE.md | 8 +++++- src/lembas/db/migrations.py | 8 +++++- src/lembas/services/library/documents.py | 7 +++++- tests/test_settings.py | 31 ++++++++++++++++++++++++ tests/test_sharing.py | 22 +++++++++++++++-- 5 files changed, 71 insertions(+), 5 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ea69ca5..490982c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -19,7 +19,7 @@ lembas info # paths + counts, useful when confused lembas secret-key # generate LEMBAS_SECRET_KEY lembas create-admin # create or promote an admin -pytest # 437 tests, ~27s +pytest # 440 tests, ~28s # PLAN.md tracks what is and is not built ruff check . # lint (line length 100) python scripts/build_artwork.py # regenerate artwork (SVG + PWA icons; @@ -402,6 +402,12 @@ models against the live database and issues `ALTER TABLE ... ADD COLUMN` for anything missing, so adding a column to a model is free: restart and it appears, with existing rows backfilled from a type-derived default. +**A nullable column is added with no default**, so existing rows get NULL -- +the value the model treats as absent. Only a NOT NULL column gets one, because +SQLite refuses to add one without. An earlier version defaulted every column by +type, which meant an added foreign key arrived as `""` on old rows and every +"is this set?" check downstream was wrong about them. + It cannot rename, drop or retype a column, or add a UNIQUE/PRIMARY KEY to an existing table — SQLite mostly cannot do those with ALTER TABLE either. Those need the create-copy-swap dance by hand; record them in `MANUAL_STEPS` so a diff --git a/src/lembas/db/migrations.py b/src/lembas/db/migrations.py index 83d99a6..4ebac50 100644 --- a/src/lembas/db/migrations.py +++ b/src/lembas/db/migrations.py @@ -90,9 +90,15 @@ def _add_column_sql(table: Table, column: Column, dialect) -> str | None: parts = [f'ALTER TABLE "{table.name}" ADD COLUMN "{column.name}" {type_sql}'] if not column.nullable: + # SQLite refuses a NOT NULL column with no default, so existing rows + # have to be given something. That is the only reason a default is + # emitted at all. parts.append("NOT NULL") - if default is not None: parts.append(f"DEFAULT {default}") + # A nullable column gets no default on purpose. Backfilling one would give + # existing rows a value the model does not consider absent -- an added + # foreign key would arrive as "" rather than NULL, and every "is this set?" + # check downstream would be wrong about rows that predate it. return " ".join(parts) diff --git a/src/lembas/services/library/documents.py b/src/lembas/services/library/documents.py index df2f947..8b2f49c 100644 --- a/src/lembas/services/library/documents.py +++ b/src/lembas/services/library/documents.py @@ -133,7 +133,12 @@ def sweep_unfiled(db: DBSession) -> int: table that already had rows. This is what makes "always set" true in practice, and it runs at startup beside the orphaned-upload sweep. """ - unfiled = list(db.scalars(select(Document).where(Document.base_id.is_(None)))) + # Empty string as well as NULL: an earlier release added the column with a + # type-derived default, so a deployment that upgraded through it has rows + # holding "" rather than NULL. Both mean the same thing here. + unfiled = list( + db.scalars(select(Document).where((Document.base_id.is_(None)) | (Document.base_id == ""))) + ) if not unfiled: return 0 diff --git a/tests/test_settings.py b/tests/test_settings.py index 5214228..40f5f2f 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -200,3 +200,34 @@ def test_other_sessions_are_revoked_but_this_one_survives(client: TestClient, db assert other.get("/chat", follow_redirects=False).status_code == 303 # ...and the tab that made the change is still in. assert client.get("/chat", follow_redirects=False).status_code == 200 + + +# --- Adding a column to a table that already has rows ------------------------- +def test_a_nullable_column_is_added_without_a_default(db): + """Backfilling one would give existing rows a value the model does not treat + as absent: an added foreign key arrives as "" rather than NULL, and every + "is this set?" check downstream is then wrong about the old rows.""" + from sqlalchemy import Column, String + + from lembas.db.migrations import _add_column_sql + from lembas.db.models import Document + + sql = _add_column_sql( + Document.__table__, Column("later", String(32), nullable=True), db.bind.dialect + ) + assert "DEFAULT" not in sql + + +def test_a_not_null_column_still_gets_one(db): + """SQLite refuses NOT NULL with no default, so there it is unavoidable.""" + from sqlalchemy import Column, String + + from lembas.db.migrations import _add_column_sql + from lembas.db.models import Document + + sql = _add_column_sql( + Document.__table__, + Column("later", String(32), nullable=False, default=""), + db.bind.dialect, + ) + assert "NOT NULL" in sql and "DEFAULT" in sql diff --git a/tests/test_sharing.py b/tests/test_sharing.py index 2142814..d8a3bb9 100644 --- a/tests/test_sharing.py +++ b/tests/test_sharing.py @@ -272,12 +272,30 @@ def test_an_uploaded_document_always_lands_in_a_base(db, people): assert document.base_id is not None -def test_documents_predating_bases_are_filed_at_startup(db, people): +@pytest.mark.parametrize("absent", [None, ""]) +def test_documents_predating_bases_are_filed_at_startup(db, people, absent): + """Both spellings of "no base": NULL, and the empty string a release that + added the column with a type-derived default left behind. + + The empty string has to be written with foreign keys off, because that is + the only way it could ever have got there -- ALTER TABLE ADD COLUMN does not + check existing rows, but an UPDATE would. + """ + from sqlalchemy import text + document = documents_service.store_upload( db, owner=people["frodo"], payload=b"x", filename="a.txt" ) - document.base_id = None db.commit() + + db.execute(text("PRAGMA foreign_keys=OFF")) + db.execute( + text("UPDATE documents SET base_id = :value WHERE id = :id"), + {"value": absent, "id": document.id}, + ) + db.commit() + db.execute(text("PRAGMA foreign_keys=ON")) + db.expire(document) assert document not in db.scalars(documents_service.visible(db, people["frodo"])) assert documents_service.sweep_unfiled(db) == 1