diff options
| -rw-r--r-- | doc/build/changelog/unreleased_20/4629.rst | 8 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/mapper.py | 20 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/relationships.py | 4 | ||||
| -rw-r--r-- | test/orm/inheritance/test_concrete.py | 6 | ||||
| -rw-r--r-- | test/orm/test_mapper.py | 114 |
5 files changed, 149 insertions, 3 deletions
diff --git a/doc/build/changelog/unreleased_20/4629.rst b/doc/build/changelog/unreleased_20/4629.rst new file mode 100644 index 000000000..dd2f22e47 --- /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 7a7524621..93b6c4ecb 100644 --- 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 2a659bdef..9852fa9a9 100644 --- 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 6b5f4f6fa..3cbccfcd3 100644 --- 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 5e869f6b3..b1d701d03 100644 --- 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 |