warn when backref will replace existing userland descriptor

A warning is emitted if a backref name used in :func:`_orm.relationship`
names an attribute on the target class which already has a method or
attribute assigned to that name, as the backref declaration will replace
that attribute.

Fixes: #4629
Change-Id: I0059b35ce60f43b0f3d8be008f12411154484ea1

diff --git a/doc/build/changelog/unreleased_20/4629.rst b/doc/build/changelog/unreleased_20/4629.rst
new file mode 100644 (file)
index 0000000..dd2f22e
--- /dev/null
+++ b/doc/build/changelog/unreleased_20/4629.rst
@@ -0,0 +1,8 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 4629
+
+    A warning is emitted if a backref name used in :func:`_orm.relationship`
+    names an attribute on the target class which already has a method or
+    attribute assigned to that name, as the backref declaration will replace
+    that attribute.

diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index 7a752462120316cd91b706713bf44a7bb2d62932..93b6c4ecb94fab57cf36cd289b6083f2b85b954f 100644 (file)
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -1658,7 +1658,7 @@ class Mapper(
                         )
                         continue
 
-                self._configure_property(key, possible_col_prop, False)
+                self._configure_property(key, possible_col_prop, init=False)
 
         # step 2: pull properties from the inherited mapper.  reconcile
         # columns with those which are explicit above.  for properties that
@@ -1698,7 +1698,7 @@ class Mapper(
                 # it now in the order in which it corresponds to the
                 # Table / selectable
                 key, prop = explicit_col_props_by_column[column]
-                self._configure_property(key, prop, False)
+                self._configure_property(key, prop, init=False)
                 continue
 
             elif column in self._columntoproperty:
@@ -1962,8 +1962,10 @@ class Mapper(
         self,
         key: str,
         prop_arg: Union[KeyedColumnElement[Any], MapperProperty[Any]],
+        *,
         init: bool = True,
         setparent: bool = True,
+        warn_for_existing: bool = False,
     ) -> MapperProperty[Any]:
         descriptor_props = util.preloaded.orm_descriptor_props
         self._log(
@@ -2073,6 +2075,20 @@ class Mapper(
             oldprop = self._props[key]
             self._path_registry.pop(oldprop, None)
 
+        if (
+            warn_for_existing
+            and self.class_.__dict__.get(key, None) is not None
+            and not isinstance(
+                self._props.get(key, None),
+                (descriptor_props.ConcreteInheritedProperty,),
+            )
+        ):
+            util.warn(
+                "User-placed attribute %r on %s being replaced with "
+                'new property "%s"; the old attribute will be discarded'
+                % (self.class_.__dict__[key], self, prop)
+            )
+
         self._props[key] = prop
 
         if not self.non_primary:

diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py
index 2a659bdef3b3988cd1b0e505b65ae3bb00543001..9852fa9a9c5c8c57e04e2fd32290fe52ef7c003e 100644 (file)
--- a/lib/sqlalchemy/orm/relationships.py
+++ b/lib/sqlalchemy/orm/relationships.py
@@ -2108,7 +2108,9 @@ class RelationshipProperty(
                 back_populates=self.key,
                 **kwargs,
             )
-            mapper._configure_property(backref_key, relationship)
+            mapper._configure_property(
+                backref_key, relationship, warn_for_existing=True
+            )
 
         if self.back_populates:
             self._add_reverse_property(self.back_populates)

diff --git a/test/orm/inheritance/test_concrete.py b/test/orm/inheritance/test_concrete.py
index 6b5f4f6fa1d5a6500fe80012128aa3b22a3b38f1..3cbccfcd3d44d506c78b69a8e9cddd87ea86e78b 100644 (file)
--- a/test/orm/inheritance/test_concrete.py
+++ b/test/orm/inheritance/test_concrete.py
@@ -1013,6 +1013,12 @@ class PropertyInheritanceTest(fixtures.MappedTest):
         assert sess.query(B).filter(B.bname == "b1").one() is b1
 
     def test_overlapping_backref_relationship(self):
+        """test #3630.
+
+        was revisited in #4629 (not fixed until 2.0.0b5 despite the old
+        issue number)
+
+        """
         A, B, b_table, a_table, Dest, dest_table = (
             self.classes.A,
             self.classes.B,

diff --git a/test/orm/test_mapper.py b/test/orm/test_mapper.py
index 5e869f6b348109135f92fc677725ee64bcbde9f2..b1d701d03975847f12ed7780082f6e89a71644fd 100644 (file)
--- a/test/orm/test_mapper.py
+++ b/test/orm/test_mapper.py
@@ -1,5 +1,6 @@
 import logging
 import logging.handlers
+import re
 
 import sqlalchemy as sa
 from sqlalchemy import column
@@ -14,6 +15,7 @@ from sqlalchemy import table
 from sqlalchemy import testing
 from sqlalchemy import util
 from sqlalchemy.engine import default
+from sqlalchemy.ext.associationproxy import association_proxy
 from sqlalchemy.orm import aliased
 from sqlalchemy.orm import attributes
 from sqlalchemy.orm import backref
@@ -22,6 +24,7 @@ from sqlalchemy.orm import clear_mappers
 from sqlalchemy.orm import column_property
 from sqlalchemy.orm import composite
 from sqlalchemy.orm import configure_mappers
+from sqlalchemy.orm import declared_attr
 from sqlalchemy.orm import deferred
 from sqlalchemy.orm import dynamic_loader
 from sqlalchemy.orm import Load
@@ -39,6 +42,7 @@ from sqlalchemy.testing import assert_warns_message
 from sqlalchemy.testing import AssertsCompiledSQL
 from sqlalchemy.testing import eq_
 from sqlalchemy.testing import expect_raises_message
+from sqlalchemy.testing import expect_warnings
 from sqlalchemy.testing import fixtures
 from sqlalchemy.testing import is_
 from sqlalchemy.testing import is_false
@@ -666,6 +670,116 @@ class MapperTest(_fixtures.FixtureTest, AssertsCompiledSQL):
 
         assert m._is_userland_descriptor("foo", MyClass.foo)
 
+    @testing.variation(
+        "attr_type",
+        ["method", "descriptor", "assocprox", "plain", "relationship"],
+    )
+    def test_backref_replacing_descriptor_warning(self, attr_type):
+        """test #4629"""
+        User, users = self.classes.User, self.tables.users
+        Address, addresses = self.classes.Address, self.tables.addresses
+
+        class MyClass(User):
+            if attr_type.method:
+
+                def addresses(self):
+                    pass
+
+            elif attr_type.descriptor:
+
+                @property
+                def addresses(self):
+                    pass
+
+            elif attr_type.assocprox:
+                addresses = association_proxy("addresses", "email")
+            elif attr_type.plain:
+                addresses = "addresses"
+            elif attr_type.relationship:
+                addresses = relationship(Address)
+            else:
+                attr_type.fail()
+
+        self.mapper(MyClass, users)
+
+        self.mapper(
+            Address,
+            addresses,
+            properties={"user": relationship(MyClass, backref="addresses")},
+        )
+
+        attr_repr = re.sub(r"[\(\)]", ".", repr(MyClass.__dict__["addresses"]))
+        with expect_warnings(
+            rf"User-placed attribute {attr_repr} on "
+            r"Mapper\[MyClass\(users\)\] being replaced with new property "
+            '"MyClass.addresses"; the old attribute will be discarded'
+        ):
+            configure_mappers()
+
+    @testing.variation(
+        "attr_type",
+        ["assocprox", "relationship"],
+    )
+    @testing.variation("as_mixin", [True, False])
+    def test_backref_replacing_descriptor_warning_declarative(
+        self, attr_type, as_mixin
+    ):
+        """test #4629"""
+        users = self.tables.users
+        Address, addresses = self.classes.Address, self.tables.addresses
+
+        Base = self.mapper_registry.generate_base()
+
+        if as_mixin:
+
+            class MyMixin:
+                if attr_type.assocprox:
+
+                    @declared_attr
+                    def addresses(cls):
+                        return association_proxy("addresses", "email")
+
+                elif attr_type.relationship:
+
+                    @declared_attr
+                    def addresses(cls):
+                        return relationship(Address)
+
+                else:
+                    attr_type.fail()
+
+            class MyClass(MyMixin, Base):
+                __table__ = users
+
+        else:
+
+            class MyClass(Base):
+                __table__ = users
+
+                if attr_type.assocprox:
+                    addresses = association_proxy("addresses", "email")
+                elif attr_type.relationship:
+                    addresses = relationship(Address)
+                else:
+                    attr_type.fail()
+
+        self.mapper(
+            Address,
+            addresses,
+            properties={"user": relationship(MyClass, backref="addresses")},
+        )
+
+        if attr_type.relationship:
+            with expect_raises_message(
+                sa.exc.ArgumentError, "Error creating backref"
+            ):
+                configure_mappers()
+        elif attr_type.assocprox:
+            with expect_warnings("User-placed attribute"):
+                configure_mappers()
+        else:
+            attr_type.fail()
+
     def test_configure_on_get_props_1(self):
         User, users = self.classes.User, self.tables.users