diff --git a/news/6706.bugfix.md b/news/6706.bugfix.md new file mode 100644 index 00000000000..415e5bd6509 --- /dev/null +++ b/news/6706.bugfix.md @@ -0,0 +1 @@ +Fixed `reflex db makemigrations`/`migrate` crashing with `CompileError` when autogenerating a migration that adds a column with a callable default (e.g. `default_factory=datetime.now` or `default=uuid.uuid4`) to an existing table; callable defaults are now evaluated before being carried as a SQL `server_default`. diff --git a/reflex/model.py b/reflex/model.py index a57078a2fdc..0007cc3cd8f 100644 --- a/reflex/model.py +++ b/reflex/model.py @@ -408,10 +408,14 @@ def render_add_column_with_server_default( op: Any, ): # Carry the sqlmodel default as server_default so that newly added - # columns get the desired default value in existing rows. + # columns get the desired default value in existing rows. Only scalar + # defaults can be rendered as a SQL literal. Evaluate callable defaults + # once so non-nullable columns can be added to tables with existing rows. if op.column.default is not None and op.column.server_default is None: + default = op.column.default + value = default.arg(None) if default.is_callable else default.arg op.column.server_default = sqlalchemy.DefaultClause( - sqlalchemy.sql.expression.literal(op.column.default.arg), + sqlalchemy.sql.expression.literal(value), ) return op diff --git a/tests/units/test_model.py b/tests/units/test_model.py index c3d62d94a17..ea71e3cbc7b 100644 --- a/tests/units/test_model.py +++ b/tests/units/test_model.py @@ -210,6 +210,110 @@ class AlembicThing(Model, table=True): assert len(list(versions.glob("*.py"))) == 6 +@pytest.mark.filterwarnings( + "ignore:This declarative base already contains a class with the same class name", +) +def test_automigration_add_column_with_callable_default( + tmp_working_dir: Path, + monkeypatch: pytest.MonkeyPatch, + model_registry: type[ModelRegistry], +): + """Test adding a column with a callable default to an existing table. + + A callable default (e.g. ``default_factory=datetime.now``) must be evaluated + before it can be rendered as a SQL literal server_default for existing rows. + + Args: + tmp_working_dir: directory where database and migrations are stored + monkeypatch: pytest fixture to overwrite attributes + model_registry: clean reflex ModelRegistry + """ + import datetime + + import sqlmodel + + alembic_ini = tmp_working_dir / "alembic.ini" + versions = tmp_working_dir / "alembic" / "versions" + monkeypatch.setattr(reflex.constants, "ALEMBIC_CONFIG", str(alembic_ini)) + + config_mock = mock.Mock() + config_mock.db_url = f"sqlite:///{tmp_working_dir}/reflex.db" + monkeypatch.setattr(reflex.model, "get_config", mock.Mock(return_value=config_mock)) + + alembic_init() + + class AlembicCallable(Model, table=True): # pyright: ignore [reportRedeclaration] + t1: str + + with get_engine().connect() as connection: + assert alembic_autogenerate(connection=connection, message="Initial Revision") + assert migrate() + + with reflex.model.session() as session: + session.add(AlembicCallable(t1="existing")) + session.commit() + + model_registry.get_metadata().clear() + + # Add a non-nullable column with a callable default alongside a scalar + # default. Rendering the migration script previously raised CompileError + # because the callable was fed into sqlalchemy.literal(); both defaults must + # be carried as server defaults so the existing row can be migrated. + class AlembicCallable(Model, table=True): # pyright: ignore [reportRedeclaration] + t1: str + created: datetime.datetime = sqlmodel.Field( + default_factory=datetime.datetime.now + ) + count: int = 5 + + assert migrate(autogenerate=True) + assert len(list(versions.glob("*.py"))) == 2 + + now = datetime.datetime.now() + with reflex.model.session() as session: + session.add(AlembicCallable(t1="foo")) + session.commit() + result = session.exec(sqlmodel.select(AlembicCallable)).all() + assert len(result) == 2 + assert result[0].t1 == "existing" + assert result[0].count == 5 + assert result[0].created < now + assert result[1].t1 == "foo" + assert result[1].count == 5 + assert result[1].created >= now + + model_registry.get_metadata().clear() + + # A nullable callable default is evaluated for existing rows and remains a + # Python-side default for new rows. + class AlembicCallable(Model, table=True): # pyright: ignore [reportRedeclaration] + t1: str + created: datetime.datetime = sqlmodel.Field( + default_factory=datetime.datetime.now + ) + count: int = 5 + note: str | None = sqlmodel.Field(default_factory=lambda: "generated") + + assert migrate(autogenerate=True) + assert len(list(versions.glob("*.py"))) == 3 + + with reflex.model.session() as session: + session.add(AlembicCallable(t1="bar")) + session.commit() + result = session.exec(sqlmodel.select(AlembicCallable)).all() + assert len(result) == 3 + # Pre-existing rows receive the evaluated server default. + assert result[0].t1 == "existing" + assert result[0].note == "generated" + assert result[1].t1 == "foo" + assert result[1].note == "generated" + # Newly inserted row gets the callable default from sqlmodel. + assert result[2].t1 == "bar" + assert result[2].note == "generated" + + model_registry.get_metadata().clear() + + class ReflexModel(Model): """A model for testing."""