deprecate rx Model#6442
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
Greptile SummaryThis PR deprecates
Confidence Score: 3/5Not safe to merge without fixing the two P1 regressions in model.py Two P1 issues: a missing fallback stub for create_all (NameError when sqlalchemy absent) and unconditional sqlmodel import injection in migration scripts for pure-SQLAlchemy users (broken migrations without sqlmodel). Both affect existing code paths exposed by this refactor. reflex/model.py — specifically the sqlalchemy else-block (missing create_all stub) and _alembic_render_item (unconditional sqlmodel injection) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[find_spec sqlalchemy] -->|True| B[get_engine / create_all / ModelRegistry / sqla_session]
A -->|False| C[_print_db_not_available stubs]
B --> D[find_spec sqlalchemy + alembic]
D -->|True| E[alembic_init / get_migration_history\nalembic_autogenerate / migrate\n_alembic_render_item adds 'import sqlmodel']
D -->|False| F[_print_db_not_available stubs]
B --> G[find_spec sqlmodel + sqlalchemy + pydantic]
G -->|True| H[Model class deprecated wrapper\n_warn_about_model_deprecation\nsession / asession / serialize_sqlmodel]
G -->|False| I[Model / session / asession not available]
H -->|delegates to| B
H -->|delegates to| E
|
| def _alembic_render_item( | ||
| type_: str, | ||
| obj: Any, | ||
| autogen_context: alembic.autogenerate.api.AutogenContext, | ||
| ): | ||
| """Alembic render_item hook call. | ||
|
|
||
| This method is called to provide python code for the given obj, | ||
| but currently it is only used to add `sqlmodel` to the import list | ||
| when generating migration scripts. | ||
|
|
||
| See https://alembic.sqlalchemy.org/en/latest/api/runtime.html | ||
|
|
||
| Args: | ||
| type_: One of "schema", "table", "column", "index", | ||
| "unique_constraint", or "foreign_key_constraint". | ||
| obj: The object being rendered. | ||
| autogen_context: Shared AutogenContext passed to each render_item call. | ||
|
|
||
| Returns: | ||
| False - Indicating that the default rendering should be used. | ||
| """ | ||
| autogen_context.imports.add("import sqlmodel") | ||
| return False |
There was a problem hiding this comment.
_alembic_render_item injects import sqlmodel unconditionally
This function now lives in the find_spec("sqlalchemy") and find_spec("alembic") block, which does not require sqlmodel. It unconditionally adds "import sqlmodel" to every generated migration script.
Users using the new pure-SQLAlchemy path (ModelBase/sqla_session) without sqlmodel installed will find that every autogenerated migration file contains import sqlmodel, causing those migration scripts to fail at runtime with ModuleNotFoundError: No module named 'sqlmodel'.
The injection should be conditional:
if find_spec("sqlmodel"):
autogen_context.imports.add("import sqlmodel")
return False| def _warn_about_model_deprecation(): | ||
| console.deprecate( | ||
| feature_name="reflex.Model", | ||
| reason="", | ||
| deprecation_version="0.9.2", | ||
| removal_version="1.0.0", | ||
| ) |
There was a problem hiding this comment.
| def migrate(autogenerate: bool = False) -> bool | None: | ||
| """Execute alembic migrations for all sqlmodel Model classes. | ||
|
|
||
| If alembic is not installed or has not been initialized for the project, | ||
| then no action is performed. |
There was a problem hiding this comment.
Stale docstring still references sqlmodel
The standalone migrate() function says "Execute alembic migrations for all sqlmodel Model classes," but this function now operates independently of sqlmodel. The docstring should be updated to reflect that it works with any registered SQLAlchemy/SQLModel models.
| # assert migrate(autogenerate=True) #noqa: ERA001 | ||
| # assert len(list(versions.glob("*.py"))) == 4 #noqa: ERA001 |
There was a problem hiding this comment.
Commented-out test assertions should be removed
Per project convention, commented-out code should be removed before merging. These two lines (the no-op autogenerate check) have been commented out and updated in this PR — if they're intentionally skipped, they should either be deleted or replaced with a pytest.mark.skip/xfail with a comment explaining why.
Rule Used: Remove commented-out code before merging PRs. (source)
Learned From
reflex-dev/reflex-web#1619
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
No description provided.