From: Mike Bayer Date: Fri, 26 May 2023 14:51:02 +0000 (-0400) Subject: tighten up overwritten attribute detection and use a deprecation X-Git-Tag: rel_2_0_16~24^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=58400352607c315fda15903a9909b2e9a61114a5;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git tighten up overwritten attribute detection and use a deprecation A deprecation warning is emitted whenever a property is added to a :class:`_orm.Mapper` where an ORM mapped property were already configured, or an attribute is already present on the class. Previously, there was a non-deprecation warning for this case that did not emit consistently. The logic for this warning has been improved so that it detects end-user replacement of attribute while not having false positives for internal Declarative and other cases where replacement of descriptors with new ones is expected. Fixes: #9841 Change-Id: I21ef25571466eeca3fedbec71612f7553fdbf2bf --- diff --git a/doc/build/changelog/unreleased_20/9841.rst b/doc/build/changelog/unreleased_20/9841.rst new file mode 100644 index 0000000000..116f67e76c --- /dev/null +++ b/doc/build/changelog/unreleased_20/9841.rst @@ -0,0 +1,12 @@ +.. change:: + :tags: bug, orm + :tickets: 9841 + + A deprecation warning is emitted whenever a property is added to a + :class:`_orm.Mapper` where an ORM mapped property were already configured, + or an attribute is already present on the class. Previously, there was a + non-deprecation warning for this case that did not emit consistently. The + logic for this warning has been improved so that it detects end-user + replacement of attribute while not having false positives for internal + Declarative and other cases where replacement of descriptors with new ones + is expected. diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py index 0533c82a69..88cbe8f4ef 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -1746,7 +1746,9 @@ class Mapper( if _map_as_property_now: self._configure_property( - key, possible_col_prop, init=False + key, + possible_col_prop, + init=False, ) # step 2: pull properties from the inherited mapper. reconcile @@ -1810,7 +1812,10 @@ class Mapper( column_key = mapper._columntoproperty[column].key self._configure_property( - column_key, column, init=False, setparent=True + column_key, + column, + init=False, + setparent=True, ) def _configure_polymorphic_setter(self, init=False): @@ -2165,37 +2170,50 @@ class Mapper( "%r for column %r" % (syn, key, key, syn) ) + # replacement cases + + # case one: prop is replacing a prop that we have mapped. this is + # independent of whatever might be in the actual class dictionary if ( key in self._props - and not isinstance(prop, properties.ColumnProperty) and not isinstance( - self._props[key], - ( - properties.ColumnProperty, - descriptor_props.ConcreteInheritedProperty, - ), + self._props[key], descriptor_props.ConcreteInheritedProperty ) + and not isinstance(prop, descriptor_props.SynonymProperty) ): - util.warn( - "Property %s on %s being replaced with new " - "property %s; the old property will be discarded" - % (self._props[key], self, prop) - ) + if warn_for_existing: + util.warn_deprecated( + f"User-placed attribute {self.class_.__name__}.{key} on " + f"{self} is replacing an existing ORM-mapped attribute. " + "Behavior is not fully defined in this case. This " + "use is deprecated and will raise an error in a future " + "release", + "2.0", + ) oldprop = self._props[key] self._path_registry.pop(oldprop, None) - if ( + # case two: prop is replacing an attribute on the class of some kind. + # we have to be more careful here since it's normal when using + # Declarative that all the "declared attributes" on the class + # get replaced. + elif ( warn_for_existing and self.class_.__dict__.get(key, None) is not None + and not isinstance(prop, descriptor_props.SynonymProperty) and not isinstance( self._props.get(key, None), - (descriptor_props.ConcreteInheritedProperty,), + 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) + util.warn_deprecated( + f"User-placed attribute {self.class_.__name__}.{key} on " + f"{self} is replacing an existing class-bound " + "attribute of the same name. " + "Behavior is not fully defined in this case. This " + "use is deprecated and will raise an error in a future " + "release", + "2.0", ) self._props[key] = prop @@ -2403,7 +2421,9 @@ class Mapper( the given MapperProperty is configured immediately. """ - prop = self._configure_property(key, prop, init=self.configured) + prop = self._configure_property( + key, prop, init=self.configured, warn_for_existing=True + ) assert isinstance(prop, MapperProperty) self._init_properties[key] = prop diff --git a/test/aaa_profiling/test_memusage.py b/test/aaa_profiling/test_memusage.py index dc5a39910e..047853e675 100644 --- a/test/aaa_profiling/test_memusage.py +++ b/test/aaa_profiling/test_memusage.py @@ -1064,9 +1064,8 @@ class MemUsageWBackendTest(fixtures.MappedTest, EnsureZeroed): t1_mapper = self.mapper_registry.map_imperatively(T1, t1) - @testing.emits_warning( - r"This declarative base", r"Property .* being replaced" - ) + @testing.emits_warning(r"This declarative base") + @testing.expect_deprecated(r"User-placed attribute .* is replacing") @profile_memory() def go(): class T2: diff --git a/test/orm/test_deprecations.py b/test/orm/test_deprecations.py index db64f91f18..2ab046cdab 100644 --- a/test/orm/test_deprecations.py +++ b/test/orm/test_deprecations.py @@ -844,6 +844,37 @@ class DeprecatedMapperTest( self.sql_count_(1, go) + @testing.variation("prop_type", ["relationship", "col_prop"]) + def test_prop_replacement_warns(self, prop_type: testing.Variation): + users, User = self.tables.users, self.classes.User + addresses, Address = self.tables.addresses, self.classes.Address + + m = self.mapper( + User, + users, + properties={ + "foo": column_property(users.c.name), + "addresses": relationship(Address), + }, + ) + self.mapper(Address, addresses) + + if prop_type.relationship: + key = "addresses" + new_prop = relationship(Address) + elif prop_type.col_prop: + key = "foo" + new_prop = column_property(users.c.name) + else: + prop_type.fail() + + with expect_deprecated( + f"Property User.{key} on Mapper|User|users being replaced " + f"with new property User.{key}; the old property will " + "be discarded", + ): + m.add_property(key, new_prop) + class DeprecatedOptionAllTest(OptionsPathTest, _fixtures.FixtureTest): run_inserts = "once" diff --git a/test/orm/test_mapper.py b/test/orm/test_mapper.py index e89e26ec9e..1cc624344f 100644 --- a/test/orm/test_mapper.py +++ b/test/orm/test_mapper.py @@ -6,6 +6,7 @@ import sqlalchemy as sa from sqlalchemy import column from sqlalchemy import ForeignKey from sqlalchemy import func +from sqlalchemy import inspect from sqlalchemy import Integer from sqlalchemy import literal from sqlalchemy import MetaData @@ -43,8 +44,8 @@ from sqlalchemy.testing import assert_raises_message from sqlalchemy.testing import assert_warns_message from sqlalchemy.testing import AssertsCompiledSQL from sqlalchemy.testing import eq_ +from sqlalchemy.testing import expect_deprecated 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 @@ -722,11 +723,15 @@ class MapperTest(_fixtures.FixtureTest, AssertsCompiledSQL): 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' + with expect_deprecated( + re.escape( + f"User-placed attribute MyClass.addresses on " + f"{str(inspect(MyClass))} is replacing an existing " + "class-bound attribute of the same name. " + "Behavior is not fully defined in this case. This " + "use is deprecated and will raise an error in a future " + "release", + ), ): configure_mappers() @@ -789,7 +794,10 @@ class MapperTest(_fixtures.FixtureTest, AssertsCompiledSQL): ): configure_mappers() elif attr_type.assocprox: - with expect_warnings("User-placed attribute"): + with expect_deprecated( + "User-placed attribute .* replacing an " + "existing class-bound attribute" + ): configure_mappers() else: attr_type.fail() @@ -981,6 +989,7 @@ class MapperTest(_fixtures.FixtureTest, AssertsCompiledSQL): self.mapper(Address, addresses) m.add_property("_name", deferred(users.c.name)) + m.add_property("name", synonym("_name")) m.add_property("addresses", relationship(Address)) @@ -1049,25 +1058,6 @@ class MapperTest(_fixtures.FixtureTest, AssertsCompiledSQL): u.name = "jacko" assert m._columntoproperty[users.c.name] is m.get_property("_name") - def test_replace_rel_prop_with_rel_warns(self): - users, User = self.tables.users, self.classes.User - addresses, Address = self.tables.addresses, self.classes.Address - - m = self.mapper( - User, users, properties={"addresses": relationship(Address)} - ) - self.mapper(Address, addresses) - - assert_warns_message( - sa.exc.SAWarning, - "Property User.addresses on Mapper|User|users being replaced " - "with new property User.addresses; the old property will " - "be discarded", - m.add_property, - "addresses", - relationship(Address), - ) - @testing.combinations((True,), (False,)) def test_add_column_prop_adaption(self, autoalias): """test ultimately from #2316 revised for #8064""" diff --git a/test/orm/test_query.py b/test/orm/test_query.py index 9ca74cb841..05f51a27bd 100644 --- a/test/orm/test_query.py +++ b/test/orm/test_query.py @@ -7694,6 +7694,8 @@ class BooleanEvalTest(fixtures.TestBase, testing.AssertsCompiledSQL): class SessionBindTest(QueryTest): + run_setup_mappers = "each" + @contextlib.contextmanager def _assert_bind_args(self, session, expect_mapped_bind=True): get_bind = mock.Mock(side_effect=session.get_bind)