From: Mike Bayer Date: Mon, 30 Sep 2019 13:55:40 +0000 (-0400) Subject: Ensure states with null m2o FK value are still populated by selectinloader X-Git-Tag: rel_1_4_0b1~714 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9f3539b1745cbb287a1338812872d27cde4ebf24;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Ensure states with null m2o FK value are still populated by selectinloader Fixed regression in selectinload loader strategy caused by #4775 (released in version 1.3.6) where a many-to-one attribute of None would no longer be populated by the loader. While this was usually not noticeable due to the lazyloader populating None upon get, it would lead to a detached instance error if the object were detached. Fixes: #4872 Change-Id: I18466841683111643810ebc4331376df38c4767b --- diff --git a/doc/build/changelog/unreleased_13/4872.rst b/doc/build/changelog/unreleased_13/4872.rst new file mode 100644 index 0000000000..4766d445d0 --- /dev/null +++ b/doc/build/changelog/unreleased_13/4872.rst @@ -0,0 +1,9 @@ +.. change:: + :tags: bug, orm + :tickets: 4872 + + Fixed regression in selectinload loader strategy caused by #4775 (released + in version 1.3.6) where a many-to-one attribute of None would no longer be + populated by the loader. While this was usually not noticeable due to the + lazyloader populating None upon get, it would lead to a detached instance + error if the object were detached. diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index cdc5c10c98..1f2f657285 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -546,6 +546,11 @@ class LazyLoader(AbstractRelationshipLoader, util.MemoizedSlots): # determine if our "lazywhere" clause is the same as the mapper's # get() clause. then we can just use mapper.get() + # + # TODO: the "not self.uselist" can be taken out entirely; a m2o + # load that populates for a list (very unusual, but is possible with + # the API) can still set for "None" and the attribute system will + # populate as an empty list. self.use_get = ( not self.is_aliased_class and not self.uselist @@ -2269,6 +2274,7 @@ class SelectInLoader(PostLoader, util.MemoizedSlots): if query_info.load_only_child: our_states = collections.defaultdict(list) + none_states = [] mapper = self.parent @@ -2289,11 +2295,19 @@ class SelectInLoader(PostLoader, util.MemoizedSlots): if attributes.PASSIVE_NO_RESULT in related_ident: query_info = self._fallback_query_info break + + # organize states into lists keyed to particular foreign + # key values. if None not in related_ident: our_states[related_ident].append( (state, state_dict, overwrite) ) + else: + # For FK values that have None, add them to a + # separate collection that will be populated separately + none_states.append((state, state_dict, overwrite)) + # note the above conditional may have changed query_info if not query_info.load_only_child: our_states = [ (state.key[1], state, state.dict, overwrite) @@ -2389,11 +2403,13 @@ class SelectInLoader(PostLoader, util.MemoizedSlots): q.add_criteria(_setup_outermost_orderby) if query_info.load_only_child: - self._load_via_child(our_states, query_info, q, context) + self._load_via_child( + our_states, none_states, query_info, q, context + ) else: self._load_via_parent(our_states, query_info, q, context) - def _load_via_child(self, our_states, query_info, q, context): + def _load_via_child(self, our_states, none_states, query_info, q, context): uselist = self.uselist # this sort is really for the benefit of the unit tests @@ -2401,7 +2417,6 @@ class SelectInLoader(PostLoader, util.MemoizedSlots): while our_keys: chunk = our_keys[0 : self._chunksize] our_keys = our_keys[self._chunksize :] - data = { k: v for k, v in q(context.session).params( @@ -2427,6 +2442,14 @@ class SelectInLoader(PostLoader, util.MemoizedSlots): dict_, related_obj if not uselist else [related_obj], ) + # populate none states with empty value / collection + for state, dict_, overwrite in none_states: + if not overwrite and self.key in dict_: + continue + + # note it's OK if this is a uselist=True attribute, the empty + # collection will be populated + state.get_impl(self.key).set_committed_value(state, dict_, None) def _load_via_parent(self, our_states, query_info, q, context): uselist = self.uselist diff --git a/test/orm/test_attributes.py b/test/orm/test_attributes.py index cd9d63c678..9f7979116a 100644 --- a/test/orm/test_attributes.py +++ b/test/orm/test_attributes.py @@ -1089,6 +1089,32 @@ class UtilTest(fixtures.ORMTest): attributes.del_attribute(f1, "coll") assert "coll" not in f1.__dict__ + def test_set_commited_value_none_uselist(self): + """test that set_committed_value->None to a uselist generates an + empty list """ + + class Foo(object): + pass + + class Bar(object): + pass + + instrumentation.register_class(Foo) + instrumentation.register_class(Bar) + attributes.register_attribute( + Foo, "col_list", uselist=True, useobject=True + ) + attributes.register_attribute( + Foo, "col_set", uselist=True, useobject=True, typecallable=set + ) + + f1 = Foo() + attributes.set_committed_value(f1, "col_list", None) + eq_(f1.col_list, []) + + attributes.set_committed_value(f1, "col_set", None) + eq_(f1.col_set, set()) + def test_initiator_arg(self): class Foo(object): pass diff --git a/test/orm/test_selectin_relations.py b/test/orm/test_selectin_relations.py index 9cdf997cd2..c38659a16c 100644 --- a/test/orm/test_selectin_relations.py +++ b/test/orm/test_selectin_relations.py @@ -1171,6 +1171,87 @@ class EagerTest(_fixtures.FixtureTest, testing.AssertsCompiledSQL): self.assert_sql_count(testing.db, go, 2) + def test_m2o_none_value_present(self): + orders, Order, addresses, Address = ( + self.tables.orders, + self.classes.Order, + self.tables.addresses, + self.classes.Address, + ) + + mapper( + Order, + orders, + properties={"address": relationship(Address, lazy="selectin")}, + ) + mapper(Address, addresses) + + sess = create_session() + q = sess.query(Order).filter(Order.id.in_([4, 5])).order_by(Order.id) + + o4, o5 = q.all() + assert o4.__dict__["address"] is not None + assert o5.__dict__["address"] is None + + # test overwrite + + o5.address = Address() + sess.query(Order).filter(Order.id.in_([4, 5])).order_by(Order.id).all() + assert o5.__dict__["address"] is not None + + o5.address = Address() + sess.query(Order).populate_existing().filter( + Order.id.in_([4, 5]) + ).order_by(Order.id).all() + assert o5.__dict__["address"] is None + + def test_m2o_uselist_none_value_present(self): + orders, Order, addresses, Address = ( + self.tables.orders, + self.classes.Order, + self.tables.addresses, + self.classes.Address, + ) + + mapper( + Order, + orders, + properties={ + "address": relationship(Address, lazy="selectin", uselist=True) + }, + ) + mapper(Address, addresses) + + sess = create_session() + q = sess.query(Order).filter(Order.id.in_([4, 5])).order_by(Order.id) + + o4, o5 = q.all() + assert len(o4.__dict__["address"]) + eq_(o5.__dict__["address"], []) + + def test_o2m_empty_list_present(self): + Address, addresses, users, User = ( + self.classes.Address, + self.tables.addresses, + self.tables.users, + self.classes.User, + ) + + mapper( + User, + users, + properties=dict( + addresses=relationship( + mapper(Address, addresses), lazy="selectin" + ) + ), + ) + q = create_session().query(User) + result = q.filter(users.c.id == 10).all() + u1 = result[0] + + eq_(u1.__dict__["addresses"], []) + def test_double_with_aggregate(self): User, users, orders, Order = ( self.classes.User, @@ -1336,7 +1417,7 @@ class LoadOnExistingTest(_fixtures.FixtureTest): eq_(u1.id, 8) self.assert_sql_count(testing.db, go, 2) - assert 'addresses' in u1.__dict__ + assert "addresses" in u1.__dict__ def test_no_query_on_deferred(self): User, Address, sess = self._deferred_config_fixture()