From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Tue, 18 Jan 2022 16:02:57 +0000 (-0500)
Subject: detect map_imperatively() called twice
X-Git-Tag: rel_2_0_0b1~537
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e6ded82eef63235d7cbfe3ab3382a48f32913640;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

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
index 0000000000..3cf6c5ba78
--- /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 00c5574fa8..29a9c9edf7 100644
--- 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 87829d6433..e9a89d102b 100644
--- 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 5496c6d761..fc7406b816 100644
--- 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."""