From: Mike Bayer Date: Wed, 23 Nov 2022 15:58:28 +0000 (-0500) Subject: add "merge" to viewonly cascades; propagate NO_RAISE when merging X-Git-Tag: rel_1_4_45~24^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=36914ae33a9d3357c7ad2d219e5af1031891be3c;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git add "merge" to viewonly cascades; propagate NO_RAISE when merging Fixed bug where :meth:`_orm.Session.merge` would fail to preserve the current loaded contents of relationship attributes that were indicated with the :paramref:`_orm.relationship.viewonly` parameter, thus defeating strategies that use :meth:`_orm.Session.merge` to pull fully loaded objects from caches and other similar techniques. In a related change, fixed issue where an object that contains a loaded relationship that was nonetheless configured as ``lazy='raise'`` on the mapping would fail when passed to :meth:`_orm.Session.merge`; checks for "raise" are now suspended within the merge process assuming the :paramref:`_orm.Session.merge.load` parameter remains at its default of ``True``. Overall, this is a behavioral adjustment to a change introduced in the 1.4 series as of :ticket:`4994`, which took "merge" out of the set of cascades applied by default to "viewonly" relationships. As "viewonly" relationships aren't persisted under any circumstances, allowing their contents to transfer during "merge" does not impact the persistence behavior of the target object. This allows :meth:`_orm.Session.merge` to correctly suit one of its use cases, that of adding objects to a :class:`.Session` that were loaded elsewhere, often for the purposes of restoring from a cache. Fixes: #8862 Change-Id: I8731c7810460e6a71f8bf5e8ded59142b9b02956 (cherry picked from commit 1e009bf086a42134190030f07068bc463e9a9794) --- diff --git a/doc/build/changelog/unreleased_14/8862.rst b/doc/build/changelog/unreleased_14/8862.rst new file mode 100644 index 0000000000..3be0078909 --- /dev/null +++ b/doc/build/changelog/unreleased_14/8862.rst @@ -0,0 +1,24 @@ +.. change:: + :tags: bug, orm + :tickets: 8862 + + Fixed bug where :meth:`_orm.Session.merge` would fail to preserve the + current loaded contents of relationship attributes that were indicated with + the :paramref:`_orm.relationship.viewonly` parameter, thus defeating + strategies that use :meth:`_orm.Session.merge` to pull fully loaded objects + from caches and other similar techniques. In a related change, fixed issue + where an object that contains a loaded relationship that was nonetheless + configured as ``lazy='raise'`` on the mapping would fail when passed to + :meth:`_orm.Session.merge`; checks for "raise" are now suspended within + the merge process assuming the :paramref:`_orm.Session.merge.load` + parameter remains at its default of ``True``. + + Overall, this is a behavioral adjustment to a change introduced in the 1.4 + series as of :ticket:`4994`, which took "merge" out of the set of cascades + applied by default to "viewonly" relationships. As "viewonly" relationships + aren't persisted under any circumstances, allowing their contents to + transfer during "merge" does not impact the persistence behavior of the + target object. This allows :meth:`_orm.Session.merge` to correctly suit one + of its use cases, that of adding objects to a :class:`.Session` that were + loaded elsewhere, often for the purposes of restoring from a cache. + diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py index efa20fb1cd..37c7d70235 100644 --- a/lib/sqlalchemy/orm/attributes.py +++ b/lib/sqlalchemy/orm/attributes.py @@ -1585,7 +1585,13 @@ class CollectionAttributeImpl(AttributeImpl): self.dispatch.bulk_replace(state, new_values, evt) - old = self.get(state, dict_, passive=PASSIVE_ONLY_PERSISTENT) + # propagate NO_RAISE in passive through to the get() for the + # existing object (ticket #8862) + old = self.get( + state, + dict_, + passive=PASSIVE_ONLY_PERSISTENT ^ (passive & NO_RAISE), + ) if old is PASSIVE_NO_RESULT: old = self._default_value(state, dict_) elif old is orig_iterable: diff --git a/lib/sqlalchemy/orm/base.py b/lib/sqlalchemy/orm/base.py index 8e94d7b384..c2f87b54a1 100644 --- a/lib/sqlalchemy/orm/base.py +++ b/lib/sqlalchemy/orm/base.py @@ -162,6 +162,12 @@ PASSIVE_ONLY_PERSISTENT = util.symbol( canonical=PASSIVE_OFF ^ NON_PERSISTENT_OK, ) +PASSIVE_MERGE = util.symbol( + "PASSIVE_OFF | NO_RAISE", + "Symbol used specifically for session.merge() and similar cases", + canonical=PASSIVE_OFF | NO_RAISE, +) + DEFAULT_MANAGER_ATTR = "_sa_class_manager" DEFAULT_STATE_ATTR = "_sa_instance_state" diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py index 9a6cfb68cc..4e3664a0cb 100644 --- a/lib/sqlalchemy/orm/relationships.py +++ b/lib/sqlalchemy/orm/relationships.py @@ -21,6 +21,7 @@ import weakref from . import attributes from .base import _is_mapped_class +from .base import PASSIVE_MERGE from .base import state_str from .interfaces import MANYTOMANY from .interfaces import MANYTOONE @@ -1048,7 +1049,7 @@ class RelationshipProperty(StrategizedProperty): if cascade is not False: self.cascade = cascade elif self.viewonly: - self.cascade = "none" + self.cascade = "merge" else: self.cascade = "save-update, merge" @@ -1904,7 +1905,9 @@ class RelationshipProperty(StrategizedProperty): # map for those already present. # also assumes CollectionAttributeImpl behavior of loading # "old" list in any case - dest_state.get_impl(self.key).get(dest_state, dest_dict) + dest_state.get_impl(self.key).get( + dest_state, dest_dict, passive=PASSIVE_MERGE + ) dest_list = [] for current in instances_iterable: @@ -1929,7 +1932,11 @@ class RelationshipProperty(StrategizedProperty): coll.append_without_event(c) else: dest_state.get_impl(self.key).set( - dest_state, dest_dict, dest_list, _adapt=False + dest_state, + dest_dict, + dest_list, + _adapt=False, + passive=PASSIVE_MERGE, ) else: current = source_dict[self.key] diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py index 265f62660f..1a9699a0e0 100644 --- a/lib/sqlalchemy/orm/util.py +++ b/lib/sqlalchemy/orm/util.py @@ -70,7 +70,7 @@ class CascadeOptions(frozenset): ) _allowed_cascades = all_cascades - _viewonly_cascades = ["expunge", "all", "none", "refresh-expire"] + _viewonly_cascades = ["expunge", "all", "none", "refresh-expire", "merge"] __slots__ = ( "save_update", diff --git a/test/orm/test_cascade.py b/test/orm/test_cascade.py index c5dd946e75..dd23f84377 100644 --- a/test/orm/test_cascade.py +++ b/test/orm/test_cascade.py @@ -4296,7 +4296,7 @@ class ViewonlyFlagWarningTest(fixtures.MappedTest): ({"delete"}, {"delete"}), ( {"all, delete-orphan"}, - {"delete", "delete-orphan", "merge", "save-update"}, + {"delete", "delete-orphan", "save-update"}, ), ({"save-update, expunge"}, {"save-update"}), ) @@ -4403,7 +4403,10 @@ class ViewonlyFlagWarningTest(fixtures.MappedTest): not_in(o1, sess) not_in(o2, sess) - def test_default_merge_cascade(self): + @testing.combinations( + "persistent", "pending", argnames="collection_status" + ) + def test_default_merge_cascade(self, collection_status): User, Order, orders, users = ( self.classes.User, self.classes.Order, @@ -4435,12 +4438,31 @@ class ViewonlyFlagWarningTest(fixtures.MappedTest): Order(id=2, user_id=1, description="someotherorder"), ) - u1.orders.append(o1) - u1.orders.append(o2) + if collection_status == "pending": + # technically this is pointless, one should not be appending + # to this collection + u1.orders.append(o1) + u1.orders.append(o2) + elif collection_status == "persistent": + sess.add(u1) + sess.flush() + sess.add_all([o1, o2]) + sess.flush() + u1.orders + else: + assert False u1 = sess.merge(u1) - assert not u1.orders + # in 1.4, as of #4993 this was asserting that u1.orders would + # not be present in the new object. However, as observed during + # #8862, this defeats schemes that seek to restore fully loaded + # objects from caches which may even have lazy="raise", but + # in any case would want to not emit new SQL on those collections. + # so we assert here that u1.orders is in fact present + assert "orders" in u1.__dict__ + assert u1.__dict__["orders"] + assert u1.orders def test_default_cascade(self): User, Order, orders, users = ( @@ -4466,7 +4488,7 @@ class ViewonlyFlagWarningTest(fixtures.MappedTest): }, ) - eq_(umapper.attrs["orders"].cascade, set()) + eq_(umapper.attrs["orders"].cascade, {"merge"}) def test_write_cascade_disallowed_w_viewonly(self): @@ -4474,7 +4496,7 @@ class ViewonlyFlagWarningTest(fixtures.MappedTest): assert_raises_message( sa_exc.ArgumentError, - 'Cascade settings "delete, delete-orphan, merge, save-update" ' + 'Cascade settings "delete, delete-orphan, save-update" ' "apply to persistence operations", relationship, Order, diff --git a/test/orm/test_merge.py b/test/orm/test_merge.py index 3e29d5cd79..8627755008 100644 --- a/test/orm/test_merge.py +++ b/test/orm/test_merge.py @@ -7,6 +7,7 @@ from sqlalchemy import event from sqlalchemy import ForeignKey from sqlalchemy import Integer from sqlalchemy import PickleType +from sqlalchemy import select from sqlalchemy import String from sqlalchemy import testing from sqlalchemy import Text @@ -16,6 +17,7 @@ from sqlalchemy.orm import configure_mappers from sqlalchemy.orm import defer from sqlalchemy.orm import deferred from sqlalchemy.orm import foreign +from sqlalchemy.orm import joinedload from sqlalchemy.orm import relationship from sqlalchemy.orm import selectinload from sqlalchemy.orm import Session @@ -28,6 +30,7 @@ from sqlalchemy.testing import expect_warnings from sqlalchemy.testing import fixtures from sqlalchemy.testing import in_ from sqlalchemy.testing import not_in +from sqlalchemy.testing.assertsql import CountStatements from sqlalchemy.testing.fixtures import fixture_session from sqlalchemy.testing.schema import Column from sqlalchemy.testing.schema import Table @@ -1396,6 +1399,126 @@ class MergeTest(_fixtures.FixtureTest): except sa.exc.InvalidRequestError as e: assert "load=False option does not support" in str(e) + @testing.combinations("viewonly", "normal", argnames="viewonly") + @testing.combinations("load", "noload", argnames="load") + @testing.combinations("select", "raise", "raise_on_sql", argnames="lazy") + @testing.combinations( + "merge_persistent", "merge_detached", argnames="merge_persistent" + ) + @testing.combinations("detached", "persistent", argnames="detach_original") + @testing.combinations("o2m", "m2o", argnames="direction") + def test_relationship_population_maintained( + self, + viewonly, + load, + lazy, + merge_persistent, + direction, + detach_original, + ): + """test #8862""" + + User, Address = self.classes("User", "Address") + users, addresses = self.tables("users", "addresses") + + self.mapper_registry.map_imperatively( + User, + users, + properties={ + "addresses": relationship( + Address, + viewonly=viewonly == "viewonly", + lazy=lazy, + back_populates="user", + ) + }, + ) + + self.mapper_registry.map_imperatively( + Address, + addresses, + properties={ + "user": relationship( + User, + viewonly=viewonly == "viewonly", + lazy=lazy, + back_populates="addresses", + ) + }, + ) + + s = fixture_session() + + u1 = User(id=1, name="u1") + s.add(u1) + s.flush() + s.add_all( + [Address(user_id=1, email_address="e%d" % i) for i in range(1, 4)] + ) + s.commit() + + if direction == "o2m": + cls_to_merge = User + obj_to_merge = ( + s.scalars(select(User).options(joinedload(User.addresses))) + .unique() + .one() + ) + attrname = "addresses" + + elif direction == "m2o": + cls_to_merge = Address + obj_to_merge = ( + s.scalars( + select(Address) + .filter_by(email_address="e1") + .options(joinedload(Address.user)) + ) + .unique() + .one() + ) + attrname = "user" + else: + assert False + + assert attrname in obj_to_merge.__dict__ + + s2 = Session(testing.db) + + if merge_persistent == "merge_persistent": + target_persistent = s2.get(cls_to_merge, obj_to_merge.id) # noqa + + if detach_original == "detach": + s.expunge(obj_to_merge) + + with self.sql_execution_asserter(testing.db) as assert_: + merged_object = s2.merge(obj_to_merge, load=load == "load") + + assert_.assert_( + CountStatements( + 0 + if load == "noload" + else 1 + if merge_persistent == "merge_persistent" + else 2 + ) + ) + + assert attrname in merged_object.__dict__ + + with self.sql_execution_asserter(testing.db) as assert_: + if direction == "o2m": + eq_( + merged_object.addresses, + [ + Address(user_id=1, email_address="e%d" % i) + for i in range(1, 4) + ], + ) + elif direction == "m2o": + eq_(merged_object.user, User(id=1, name="u1")) + assert_.assert_(CountStatements(0)) + def test_synonym(self): users = self.tables.users