fix: make model metadata merging deterministic

news/+7830.bugfix.md

@@ -0,0 +1 @@
+Fail database metadata merging with a clear error when multiple model bases define the same table.

reflex/model.py

@@ -204,11 +204,14 @@ class ModelRegistry:
         registered too.
 
         Attributes:
+            _registered_models: Track explicitly registered models in insertion order.
             _metadata: Cache the metadata to avoid re-creating it.
         """
 
         models: ClassVar[set[SQLModelOrSqlAlchemy]] = set()
 
+        _registered_models: ClassVar[dict[SQLModelOrSqlAlchemy, None]] = {}
+
         _metadata: ClassVar[sqlalchemy.MetaData | None] = None
 
         @classmethod
@@ -222,6 +225,7 @@ def register(cls, model: SQLModelOrSqlAlchemyT) -> SQLModelOrSqlAlchemyT:
                 The model passed in as an argument (Allows decorator usage)
             """
             cls.models.add(model)
+            cls._registered_models.setdefault(model, None)
             return model
 
         @classmethod
@@ -234,33 +238,60 @@ def get_models(cls, include_empty: bool = False) -> set[SQLModelOrSqlAlchemy]:
             Returns:
                 The registered models.
             """
+            return set(cls._get_ordered_models(include_empty=include_empty))
+
+        @classmethod
+        def _get_ordered_models(
+            cls, include_empty: bool = False
+        ) -> tuple[SQLModelOrSqlAlchemy, ...]:
+            """Get models in registration order, followed by detected bases.
+
+            Args:
+                include_empty: If True, include models with empty metadata.
+
+            Returns:
+                The models in deterministic order.
+            """
+            for model in tuple(cls._registered_models):
+                if model not in cls.models:
+                    del cls._registered_models[model]
+            registered_models = tuple(cls._registered_models)
+            registered_models += tuple(
+                sorted(
+                    cls.models.difference(registered_models),
+                    key=lambda model: (model.__module__, model.__qualname__),
+                )
+            )
             sqlmodel = sys.modules.get("sqlmodel")
             # A registered base opts out of detection; SQLModel ones, such as the
             # always registered rx.Model, do not.
             models = (
-                cls.models
+                registered_models
                 if any(
                     sqlmodel is None or not issubclass(model, sqlmodel.SQLModel)
-                    for model in cls.models
+                    for model in registered_models
                 )
-                else cls.models | cls._find_bases()
+                else (*registered_models, *cls._find_bases())
             )
+            ordered_models = tuple(dict.fromkeys(models))
             if include_empty:
-                return models
-            return {
-                model for model in models if not cls._model_metadata_is_empty(model)
-            }
+                return ordered_models
+            return tuple(
+                model
+                for model in ordered_models
+                if not cls._model_metadata_is_empty(model)
+            )
 
         @staticmethod
-        def _find_bases() -> set[type[sqlalchemy.orm.DeclarativeBase]]:
+        def _find_bases() -> tuple[type[sqlalchemy.orm.DeclarativeBase], ...]:
             """Find the model bases: the DeclarativeBase subclasses with their own metadata.
 
             Only a base that its module holds under its qualified name counts, such
             as ``Base`` or ``Namespace.Base``, so one that a reload of its module
             replaced, or one made inside a function, does not.
 
             Returns:
-                The model bases.
+                The model bases in deterministic order.
             """
             bases = set()
             pending = sqlalchemy.orm.DeclarativeBase.__subclasses__()
@@ -277,8 +308,11 @@ def _find_bases() -> set[type[sqlalchemy.orm.DeclarativeBase]]:
                     is cls
                 ):
                     bases.add(cls)
-            logger.debug(f"Detected model bases: {sorted(map(str, bases))}")
-            return bases
+            ordered_bases = tuple(
+                sorted(bases, key=lambda model: (model.__module__, model.__qualname__))
+            )
+            logger.debug(f"Detected model bases: {list(map(str, ordered_bases))}")
+            return ordered_bases
 
         @staticmethod
         def _model_metadata_is_empty(model: SQLModelOrSqlAlchemy) -> bool:
@@ -302,17 +336,33 @@ def get_metadata(cls) -> sqlalchemy.MetaData:
             if cls._metadata is not None:
                 return cls._metadata
 
-            models = cls.get_models(include_empty=False)
+            models = cls._get_ordered_models(include_empty=False)
 
             if len(models) == 1:
-                metadata = next(iter(models)).metadata
+                metadata = models[0].metadata
             else:
                 # Merge the metadata from all the models.
                 # This allows mixing bare sqlalchemy models with sqlmodel models in one database.
                 metadata = sqlalchemy.MetaData()
+                table_owners: dict[
+                    str, tuple[sqlalchemy.MetaData, SQLModelOrSqlAlchemy]
+                ] = {}
                 for model in models:
                     for table in model.metadata.tables.values():
+                        if table.key in table_owners:
+                            original_metadata, original_model = table_owners[table.key]
+                            if original_metadata is table.metadata:
+                                continue
+                            table_key = table.key
+                            first_model = f"{original_model.__module__}.{original_model.__qualname__}"
+                            second_model = f"{model.__module__}.{model.__qualname__}"
+                            msg = (
+                                f"Table '{table_key}' is defined by both "
+                                f"{first_model} and {second_model}."
+                            )
+                            raise sqlalchemy.exc.InvalidRequestError(msg)
                         table.to_metadata(metadata)
+                        table_owners[table.key] = (table.metadata, model)
 
             # Cache the metadata
             cls._metadata = metadata

tests/units/test_model.py

@@ -343,7 +343,9 @@ def registry(
         ModelRegistry,
         "_find_bases",
         staticmethod(
-            lambda: {base for base in find_bases() if base.__module__ == BASES_MODULE}
+            lambda: tuple(
+                base for base in find_bases() if base.__module__ == BASES_MODULE
+            )
         ),
     )
     return model_registry
@@ -359,11 +361,16 @@ def test_unregistered_declarative_bases_are_detected(
     """
     module = _import_bases_module(monkeypatch)
 
-    assert registry._find_bases() == {
+    assert set(registry._find_bases()) == {
         module.Base,
         module.EmptyBase,
         module.Namespace.Base,
     }
+    assert registry._find_bases() == (
+        module.Base,
+        module.EmptyBase,
+        module.Namespace.Base,
+    )
     assert module.Base in registry.get_models()
     assert module.EmptyBase not in registry.get_models()
     assert module.EmptyBase in registry.get_models(include_empty=True)
@@ -415,3 +422,135 @@ def test_metadata_detects_bases_once(
     assert len(calls) == 1
     assert "user" in metadata.tables
     assert module.Base in registry.get_models()
+
+
+def test_metadata_rejects_duplicate_tables_from_different_bases(
+    monkeypatch: pytest.MonkeyPatch, registry: type[ModelRegistry]
+):
+    """Reject two bases that define the same table instead of choosing one."""
+    module = types.ModuleType("_model_duplicate_bases_app")
+    monkeypatch.setitem(sys.modules, module.__name__, module)
+    exec(
+        """
+from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column
+from reflex.model import Model
+
+class First(Model, table=True):
+    __tablename__ = "duplicate_issue_7830"
+    name: str
+
+class SecondBase(DeclarativeBase):
+    pass
+
+class Second(SecondBase):
+    __tablename__ = "duplicate_issue_7830"
+    id: Mapped[int] = mapped_column(primary_key=True)
+""",
+        module.__dict__,
+    )
+    monkeypatch.setattr(
+        registry,
+        "_find_bases",
+        staticmethod(lambda: (module.SecondBase,)),
+    )
+
+    try:
+        with pytest.raises(sa.exc.InvalidRequestError) as exc_info:
+            registry.get_metadata()
+
+        assert str(exc_info.value) == (
+            "Table 'duplicate_issue_7830' is defined by both "
+            "reflex.model.Model and "
+            "_model_duplicate_bases_app.SecondBase."
+        )
+    finally:
+        Model.metadata.remove(module.First.__table__)
+
+
+def test_metadata_merges_registered_bases_in_registration_order(
+    monkeypatch: pytest.MonkeyPatch, registry: type[ModelRegistry]
+):
+    """Merge registered model metadata in the order bases were registered."""
+    module = types.ModuleType("_model_ordered_bases_app")
+    monkeypatch.setitem(sys.modules, module.__name__, module)
+    exec(
+        """
+from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column
+
+class FirstBase(DeclarativeBase):
+    pass
+
+class First(FirstBase):
+    __tablename__ = "first_issue_7830"
+    id: Mapped[int] = mapped_column(primary_key=True)
+
+class SecondBase(DeclarativeBase):
+    pass
+
+class Second(SecondBase):
+    __tablename__ = "second_issue_7830"
+    id: Mapped[int] = mapped_column(primary_key=True)
+""",
+        module.__dict__,
+    )
+    monkeypatch.setattr(
+        registry,
+        "models",
+        {module.SecondBase, module.FirstBase},
+    )
+    monkeypatch.setattr(
+        registry,
+        "_registered_models",
+        {module.SecondBase: None, module.FirstBase: None},
+    )
+    monkeypatch.setattr(registry, "_find_bases", staticmethod(lambda: ()))
+
+    metadata = registry.get_metadata()
+
+    assert tuple(metadata.tables) == ("second_issue_7830", "first_issue_7830")
+
+
+def test_metadata_merge_preserves_convention_generated_constraint_names(
+    monkeypatch: pytest.MonkeyPatch, registry: type[ModelRegistry]
+):
+    """Keep names generated by a source metadata naming convention when merging."""
+    module = types.ModuleType("_model_naming_convention_app")
+    monkeypatch.setitem(sys.modules, module.__name__, module)
+    exec(
+        """
+from sqlalchemy import MetaData, UniqueConstraint
+from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column
+from reflex.model import Model
+
+class NamedBase(DeclarativeBase):
+    metadata = MetaData(
+        naming_convention={"uq": "uq_%(table_name)s_%(column_0_name)s"}
+    )
+
+class Comment(NamedBase):
+    __tablename__ = "comment_issue_7830"
+    id: Mapped[int] = mapped_column(primary_key=True)
+    text: Mapped[str]
+    __table_args__ = (UniqueConstraint("text"),)
+
+class Legacy(Model, table=True):
+    __tablename__ = "legacy_issue_7830"
+    name: str
+""",
+        module.__dict__,
+    )
+    monkeypatch.setattr(
+        registry, "_find_bases", staticmethod(lambda: (module.NamedBase,))
+    )
+
+    try:
+        metadata = registry.get_metadata()
+
+        unique_constraint = next(
+            constraint
+            for constraint in metadata.tables["comment_issue_7830"].constraints
+            if isinstance(constraint, sa.UniqueConstraint)
+        )
+        assert unique_constraint.name == "uq_comment_issue_7830_text"
+    finally:
+        Model.metadata.remove(module.Legacy.__table__)