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 (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 452f2eaf533934ebe7f7e19630e3d410908fcc58..42419e48cde78dcf35b92abd61415724dec683ed 100644 (file)
--- 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 984e4de979a66a0367de15254a976294acbd348c..b12ade59c33cf2a7e707d0877c60910941da87c9 100644 (file)
--- 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 0f84923ac8575100c903df4ce314bda5d1846fde..11a762e60b1be69803b4c9f27e377b27fdc104f0 100644 (file)
--- 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."""