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) <noreply@anthropic.com>

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)
 
 

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
 

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

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