detect map_imperatively() called twice

Fixed issue where calling upon :meth:`_orm.regsitry.map_imperatively` more
than once for the same class would produce an unexpected error, rather than
an informative error that the target class is already mapped. This behavior
differed from that of the :func:`_orm.mapper` function which does report an
informative message already.

For 2.0, this change also cleans up the logic that detects against
`Mapper()` or `_mapper()` being invoked directly.  1.4's backport will
take on a different format as `mapper()` is still public API in that
release.

Fixes: #7579
Change-Id: Ie74a1a2e97f8b6a81ac1942040edd8cae82f4bd8

diff --git a/doc/build/changelog/unreleased_14/7579.rst b/doc/build/changelog/unreleased_14/7579.rst
new file mode 100644 (file)
index 0000000..3cf6c5b
--- /dev/null
+++ b/doc/build/changelog/unreleased_14/7579.rst
@@ -0,0 +1,9 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 7579
+
+    Fixed issue where calling upon :meth:`_orm.regsitry.map_imperatively` more
+    than once for the same class would produce an unexpected error, rather than
+    an informative error that the target class is already mapped. This behavior
+    differed from that of the :func:`_orm.mapper` function which does report an
+    informative message already.

diff --git a/lib/sqlalchemy/orm/decl_api.py b/lib/sqlalchemy/orm/decl_api.py
index 00c5574fa8e82a41cecaa7688f52d2182ccfb6f4..29a9c9edf737b4f11ffb3f4083e07a00c36b0f4e 100644 (file)
--- a/lib/sqlalchemy/orm/decl_api.py
+++ b/lib/sqlalchemy/orm/decl_api.py
@@ -772,6 +772,11 @@ class registry:
 
     def _add_manager(self, manager):
         self._managers[manager] = True
+        if manager.is_mapped:
+            raise exc.ArgumentError(
+                "Class '%s' already has a primary mapper defined. "
+                % manager.class_
+            )
         assert manager.registry is None
         manager.registry = self
 

diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index 87829d6433b3d96fd0c6f7c5f53781664d99f69b..e9a89d102ba77704af2e452f461b775079415e9a 100644 (file)
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -1221,17 +1221,13 @@ class Mapper(
             manager.registry._add_non_primary_mapper(self)
             return
 
-        if manager is not None:
-            assert manager.class_ is self.class_
-            if manager.is_mapped:
-                raise sa_exc.ArgumentError(
-                    "Class '%s' already has a primary mapper defined. "
-                    % self.class_
-                )
-            # else:
-            # a ClassManager may already exist as
-            # ClassManager.instrument_attribute() creates
-            # new managers for each subclass if they don't yet exist.
+        if manager is None or not manager.registry:
+            raise sa_exc.InvalidRequestError(
+                "The _mapper() function and Mapper() constructor may not be "
+                "invoked directly outside of a declarative registry."
+                " Please use the sqlalchemy.orm.registry.map_imperatively() "
+                "function for a classical mapping."
+            )
 
         self.dispatch.instrument_class(self, self.class_)
 
@@ -1252,14 +1248,6 @@ class Mapper(
             finalize=True,
         )
 
-        if not manager.registry:
-            raise sa_exc.InvalidRequestError(
-                "The _mapper() function may not be invoked directly outside "
-                "of a declarative registry."
-                " Please use the sqlalchemy.orm.registry.map_imperatively() "
-                "function for a classical mapping."
-            )
-
         self.class_manager = manager
         self.registry = manager.registry
 

diff --git a/test/orm/test_mapper.py b/test/orm/test_mapper.py
index 5496c6d761e055fed2d53bcaa4cc8af5bfa9d678..fc7406b8164a331267c21f0d2ef732591dcb3349 100644 (file)
--- a/test/orm/test_mapper.py
+++ b/test/orm/test_mapper.py
@@ -114,6 +114,35 @@ class MapperTest(_fixtures.FixtureTest, AssertsCompiledSQL):
             foobar="x",
         )
 
+    def test_class_already_mapped(self):
+        users, User = (
+            self.tables.users,
+            self.classes.User,
+        )
+
+        self.mapper(User, users)
+
+        with expect_raises_message(
+            sa.exc.ArgumentError,
+            "Class .*User.* already has a primary mapper defined",
+        ):
+            self.mapper(User, users)
+
+    def test_cant_call_legacy_constructor_directly(self):
+        users, User = (
+            self.tables.users,
+            self.classes.User,
+        )
+
+        from sqlalchemy.orm import Mapper
+
+        with expect_raises_message(
+            sa.exc.InvalidRequestError,
+            r"The _mapper\(\) function and Mapper\(\) constructor may not be "
+            "invoked directly",
+        ):
+            Mapper(User, users)
+
     def test_prop_shadow(self):
         """A backref name may not shadow an existing property name."""