fix(model): only carry scalar column defaults as server_default in migrations - #6770
fix(model): only carry scalar column defaults as server_default in migrations#6770anxkhn wants to merge 3 commits into
Conversation
Greptile SummaryThis PR fixes a
Confidence Score: 4/5Safe to merge for the common datetime/string callable-default case, but the callable evaluation approach can silently assign the same UUID to all existing rows when a unique-constrained callable default is added to a populated table. The rewriter evaluates callable defaults once at migration-script generation time. For timestamp and fixed-string callables this is correct. For callables designed to produce a unique value per row (uuid.uuid4), every pre-existing row receives the same evaluated value, failing visibly with a unique constraint or silently producing duplicate identifiers without one. Files Needing Attention: reflex/model.py — the render_add_column_with_server_default rewriter, specifically the one-shot callable evaluation on line 416 and the lack of a uniqueness check before setting server_default.
|
| Filename | Overview |
|---|---|
| reflex/model.py | Callable defaults are now evaluated at migration generation time instead of being skipped; works correctly for datetime but produces duplicate values for unique-constrained callables like uuid.uuid4. |
| tests/units/test_model.py | New test covers datetime and string callable defaults with non-unique values; no coverage for unique-constrained callable defaults (e.g. uuid.uuid4 with unique=True) that could surface the duplication bug. |
| news/6706.bugfix.md | Changelog entry correctly describes that callable defaults are evaluated before being carried as server_default, matching the actual implementation. |
Reviews (4): Last reviewed commit: "test(model): isolate callable default mi..." | Re-trigger Greptile
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c68b001a8c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…grations The AddColumnOp rewriter used during autogeneration copied every column default into a server_default by wrapping op.column.default.arg in sqlalchemy.literal(). For a callable default (e.g. Field(default_factory=datetime.now) or Field(default=uuid.uuid4)), op.column.default.arg is the Python function object rather than a value, so rendering the migration script raised sqlalchemy.exc.CompileError: "No literal value renderer is available for literal value <function ...>". This broke autogenerated migrations for the common case of adding a timestamp or UUID column to an existing table. Gate the server_default rewrite on op.column.default.is_scalar so only scalar defaults are carried as a SQL literal; callable defaults keep alembic's default behavior, which is correct since a per-row Python callable cannot be a single SQL literal. Add a regression test that adds callable-default columns to an existing table and fails with CompileError before the fix. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
|
fixed the full-matrix regression in 84ceadc. the new model migration test now clears its final metadata, so it no longer pollutes the following sqlalchemy automigration test. both automigration suites pass together (9 tests), and ruff plus pyright pass. @reflex-dev please take another look when ci completes. |
Type of change
What / why
reflex db makemigrationsandreflex db migrate --autogeneratecrash with asqlalchemy.exc.CompileErrorwhen the autogenerated migration adds a column thathas a callable default to an existing table. This is the single most common
real-world schema change: adding a
createdtimestamp or a UUID column, e.g.Root cause is in the
AddColumnOprewriter registered during autogeneration inreflex/model.py. To carry a sqlmodel default onto existing rows, it copies thecolumn default into a
server_defaultby wrappingop.column.default.arginsqlalchemy.literal(...):For a scalar default (
Field(default=7))op.column.default.argis the value(
7), which renders fine. For a callable default, SQLAlchemy stores aCallableColumnDefaultwhose.argis the Python function object itself, not avalue.
literal(<function>)becomes a bind parameter withNullType, and whenthe migration script is rendered with
literal_binds=TrueSQLAlchemy raises:so migration-script generation aborts.
Fix
Gate the
server_defaultrewrite onop.column.default.is_scalar, so only scalardefaults are carried as a SQL literal:
Callable defaults then keep alembic's default behavior (no
server_default), whichis correct: a per-row Python callable cannot be represented as a single SQL literal.
Scalar
server_defaultbehavior is unchanged.is_scalaris defined on the baseDefaultGenerator(class-levelFalse), so this is safe for every default typealembic can attach.
Tests
Added
tests/units/test_model.py::test_automigration_add_column_with_callable_default,which creates a table, migrates to head, then adds callable-default columns
(
default_factory=datetime.now, a nullabledefault_factory) and runs theautogenerate/migrate path. It fails with the
CompileErrorabove onmainandpasses with the fix. It also asserts that a scalar default alongside the callable
one still works and that rows read back correctly.
Local checks (all green):