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_1_4_30~6
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e542448ac9a9cb315a010a8973e58ee1157b91ef;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
(cherry picked from commit e6ded82eef63235d7cbfe3ab3382a48f32913640)
---

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 452f2eaf53..42419e48cd 100644
--- a/lib/sqlalchemy/orm/decl_api.py
+++ b/lib/sqlalchemy/orm/decl_api.py
@@ -644,7 +644,11 @@ class registry(object):
 
     def _add_manager(self, manager):
         self._managers[manager] = True
-        assert manager.registry is None
+        if manager.registry is not None and manager.is_mapped:
+            raise exc.ArgumentError(
+                "Class '%s' already has a primary mapper defined. "
+                % manager.class_
+            )
         manager.registry = self
 
     def configure(self, cascade=False):
diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index 984e4de979..b12ade59c3 100644
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -1230,6 +1230,11 @@ class Mapper(
         if manager is not None:
             assert manager.class_ is self.class_
             if manager.is_mapped:
+                # changed in #7579:
+                # this message is defined in two places as of this change,
+                # also in decl_api -> _add_manager().  in 2.0, this codepath
+                # is removed as any calls to mapper() / Mapper without
+                # the registry setting up first will be rejected.
                 raise sa_exc.ArgumentError(
                     "Class '%s' already has a primary mapper defined. "
                     % self.class_
diff --git a/test/orm/test_mapper.py b/test/orm/test_mapper.py
index 0f84923ac8..11a762e60b 100644
--- a/test/orm/test_mapper.py
+++ b/test/orm/test_mapper.py
@@ -23,6 +23,8 @@ from sqlalchemy.orm import deferred
 from sqlalchemy.orm import dynamic_loader
 from sqlalchemy.orm import Load
 from sqlalchemy.orm import load_only
+from sqlalchemy.orm import Mapper
+from sqlalchemy.orm import mapper
 from sqlalchemy.orm import reconstructor
 from sqlalchemy.orm import registry
 from sqlalchemy.orm import relationship
@@ -33,6 +35,7 @@ from sqlalchemy.testing import assert_raises
 from sqlalchemy.testing import assert_raises_message
 from sqlalchemy.testing import AssertsCompiledSQL
 from sqlalchemy.testing import eq_
+from sqlalchemy.testing import expect_deprecated_20
 from sqlalchemy.testing import expect_raises_message
 from sqlalchemy.testing import fixtures
 from sqlalchemy.testing import is_
@@ -114,6 +117,38 @@ 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)
+
+    @testing.combinations(mapper, Mapper)
+    def test_class_already_mapped_legacy(self, fn):
+        users, User = (
+            self.tables.users,
+            self.classes.User,
+        )
+
+        with expect_deprecated_20(
+            r"Calling the mapper\(\) function directly outside"
+        ):
+            fn(User, users)
+
+        with expect_raises_message(
+            sa.exc.ArgumentError,
+            "Class .*User.* already has a primary mapper defined",
+        ):
+            fn(User, users)
+
     def test_prop_shadow(self):
         """A backref name may not shadow an existing property name."""