From: Mike Bayer Date: Thu, 22 Aug 2019 00:19:43 +0000 (-0400) Subject: Ensure discarded collection removed from empty collections X-Git-Tag: rel_1_4_0b1~751^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9405089dfce141196157c6d89323c3f9aec2c0c0;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Ensure discarded collection removed from empty collections A bulk replace operation was not attending to the previous list still present in the "_empty_collections" dictionary which was added as part of #4519. Fixes: #4519 Change-Id: I3f99f8647c0fb8140b3dfb03686a5d3b90da633f --- diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py index d47740e3db..2f54fcd329 100644 --- a/lib/sqlalchemy/orm/attributes.py +++ b/lib/sqlalchemy/orm/attributes.py @@ -671,6 +671,11 @@ class AttributeImpl(object): def _default_value(self, state, dict_): """Produce an empty value for an uninitialized scalar attribute.""" + assert self.key not in dict_, ( + "_default_value should only be invoked for an " + "uninitialized or expired attribute" + ) + value = None for fn in self.dispatch.init_scalar: ret = fn(state, value, dict_) @@ -1201,6 +1206,11 @@ class CollectionAttributeImpl(AttributeImpl): def _default_value(self, state, dict_): """Produce an empty collection for an un-initialized attribute""" + assert self.key not in dict_, ( + "_default_value should only be invoked for an " + "uninitialized or expired attribute" + ) + if self.key in state._empty_collections: return state._empty_collections[self.key] @@ -1321,8 +1331,18 @@ class CollectionAttributeImpl(AttributeImpl): new_values, old_collection, new_collection, initiator=evt ) - del old._sa_adapter - self.dispatch.dispose_collection(state, old, old_collection) + self._dispose_previous_collection(state, old, old_collection, True) + + def _dispose_previous_collection( + self, state, collection, adapter, fire_event + ): + del collection._sa_adapter + + # discarding old collection make sure it is not referenced in empty + # collections. + state._empty_collections.pop(self.key, None) + if fire_event: + self.dispatch.dispose_collection(state, collection, adapter) def _invalidate_collection(self, collection): adapter = getattr(collection, "_sa_adapter") @@ -1360,7 +1380,9 @@ class CollectionAttributeImpl(AttributeImpl): ): """Retrieve the CollectionAdapter associated with the given state. - Creates a new CollectionAdapter if one does not exist. + if user_data is None, retrieves it from the state using normal + "get()" rules, which will fire lazy callables or return the "empty" + collection value. """ if user_data is None: @@ -1368,7 +1390,7 @@ class CollectionAttributeImpl(AttributeImpl): if user_data is PASSIVE_NO_RESULT: return user_data - return getattr(user_data, "_sa_adapter") + return user_data._sa_adapter def backref_listeners(attribute, key, uselist): @@ -1905,12 +1927,22 @@ def init_collection(obj, key): def init_state_collection(state, dict_, key): - """Initialize a collection attribute and return the collection adapter.""" + """Initialize a collection attribute and return the collection adapter. + + Discards any existing collection which may be there. + """ attr = state.manager[key].impl + + old = dict_.pop(key, None) # discard old collection + if old is not None: + old_collection = old._sa_adapter + attr._dispose_previous_collection(state, old, old_collection, False) + user_data = attr._default_value(state, dict_) adapter = attr.get_collection(state, dict_, user_data) adapter._reset_empty() + return adapter diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index 8e8242c669..fc86076b1f 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -1981,6 +1981,9 @@ class JoinedLoader(AbstractRelationshipLoader): def _create_collection_loader(self, context, key, _instance, populators): def load_collection_from_joined_new_row(state, dict_, row): + # note this must unconditionally clear out any existing collection. + # an existing collection would be present only in the case of + # populate_existing(). collection = attributes.init_state_collection(state, dict_, key) result_list = util.UniqueAppender( collection, "append_without_event" diff --git a/test/orm/test_attributes.py b/test/orm/test_attributes.py index 4498a6ab02..cd9d63c678 100644 --- a/test/orm/test_attributes.py +++ b/test/orm/test_attributes.py @@ -14,7 +14,9 @@ from sqlalchemy.testing import eq_ from sqlalchemy.testing import fixtures from sqlalchemy.testing import is_ from sqlalchemy.testing import is_false +from sqlalchemy.testing import is_not_ from sqlalchemy.testing import is_true +from sqlalchemy.testing import not_in_ from sqlalchemy.testing.mock import call from sqlalchemy.testing.mock import Mock from sqlalchemy.testing.util import all_partial_orderings @@ -3659,6 +3661,65 @@ class EventPropagateTest(fixtures.TestBase): canary[:] = [] +class CollectionInitTest(fixtures.TestBase): + def setUp(self): + class A(object): + pass + + class B(object): + pass + + self.A = A + self.B = B + instrumentation.register_class(A) + instrumentation.register_class(B) + attributes.register_attribute(A, "bs", uselist=True, useobject=True) + + def test_bulk_replace_resets_empty(self): + A = self.A + a1 = A() + state = attributes.instance_state(a1) + + existing = a1.bs + + is_(state._empty_collections["bs"], existing) + is_not_(existing._sa_adapter, None) + + a1.bs = [] # replaces previous "empty" collection + not_in_("bs", state._empty_collections) # empty is replaced + is_(existing._sa_adapter, None) + + def test_assert_false_on_default_value(self): + A = self.A + a1 = A() + state = attributes.instance_state(a1) + + attributes.init_state_collection(state, state.dict, "bs") + + assert_raises( + AssertionError, A.bs.impl._default_value, state, state.dict + ) + + def test_loader_inits_collection_already_exists(self): + A, B = self.A, self.B + a1 = A() + b1, b2 = B(), B() + a1.bs = [b1, b2] + eq_(a1.__dict__["bs"], [b1, b2]) + + old = a1.__dict__["bs"] + is_not_(old._sa_adapter, None) + state = attributes.instance_state(a1) + + # this occurs during a load with populate_existing + adapter = attributes.init_state_collection(state, state.dict, "bs") + + new = a1.__dict__["bs"] + eq_(new, []) + is_(new._sa_adapter, adapter) + is_(old._sa_adapter, None) + + class TestUnlink(fixtures.TestBase): def setUp(self): class A(object): diff --git a/test/orm/test_eager_relations.py b/test/orm/test_eager_relations.py index cd20342c1a..70ca993b79 100644 --- a/test/orm/test_eager_relations.py +++ b/test/orm/test_eager_relations.py @@ -3479,6 +3479,34 @@ class LoadOnExistingTest(_fixtures.FixtureTest): self.assert_sql_count(testing.db, go, 1) assert "addresses" not in u1.__dict__ + def test_populate_existing_propagate(self): + # both SelectInLoader and SubqueryLoader receive the loaded collection + # at once and use attributes.set_committed_value(). However + # joinedloader receives the collection per-row, so has an initial + # step where it invokes init_state_collection(). This has to clear + # out an existing collection to function correctly with + # populate_existing. + User, Address, sess = self._eager_config_fixture() + u1 = sess.query(User).get(8) + u1.addresses[2].email_address = "foofoo" + del u1.addresses[1] + u1 = sess.query(User).populate_existing().filter_by(id=8).one() + # collection is reverted + eq_(len(u1.addresses), 3) + + # attributes on related items reverted + eq_(u1.addresses[2].email_address, "ed@lala.com") + + def test_no_crash_on_existing(self): + User, Address, sess = self._eager_config_fixture() + u1 = User(id=12, name="u", addresses=[]) + sess.add(u1) + sess.commit() + + sess.query(User).filter(User.id == 12).options( + joinedload(User.addresses) + ).first() + def test_loads_second_level_collection_to_scalar(self): User, Address, Dingaling, sess = self._collection_to_scalar_fixture()