From a4f30de1662c9bc2c79e66b80565aaf8ba18f8c4 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Fri, 9 Apr 2021 10:46:21 -0400 Subject: [PATCH] Apply recursive check to immediateloader and generalize Fixed critical regression caused by the new feature added as part of :ticket:`1763`, eager loaders are invoked on unexpire operations. The new feature makes use of the "immediateload" eager loader strategy as a substitute for a collection loading strategy, which unlike the other "post-load" strategies was not accommodating for recursive invocations between mutually-dependent relationships, leading to recursion overflow errors. Fixes: #6232 Change-Id: Ifb2286281f40d1a04c24741261d4438659b6e3dd --- doc/build/changelog/unreleased_14/6232.rst | 12 ++++++ lib/sqlalchemy/orm/strategies.py | 45 ++++++++++++++++------ test/orm/test_eager_relations.py | 38 ++++++++++++++++++ 3 files changed, 83 insertions(+), 12 deletions(-) create mode 100644 doc/build/changelog/unreleased_14/6232.rst diff --git a/doc/build/changelog/unreleased_14/6232.rst b/doc/build/changelog/unreleased_14/6232.rst new file mode 100644 index 0000000000..5aee050e12 --- /dev/null +++ b/doc/build/changelog/unreleased_14/6232.rst @@ -0,0 +1,12 @@ +.. change:: + :tags: bug, orm, regression + :tickets: 6232 + + Fixed critical regression caused by the new feature added as part of + :ticket:`1763`, eager loaders are invoked on unexpire operations. The new + feature makes use of the "immediateload" eager loader strategy as a + substitute for a collection loading strategy, which unlike the other + "post-load" strategies was not accommodating for recursive invocations + between mutually-dependent relationships, leading to recursion overflow + errors. + diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index 824df040af..da8e0439fe 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -1166,6 +1166,28 @@ class LoadLazyAttribute(object): class PostLoader(AbstractRelationshipLoader): """A relationship loader that emits a second SELECT statement.""" + def _check_recursive_postload(self, context, path, join_depth=None): + effective_path = ( + context.compile_state.current_path or orm_util.PathRegistry.root + ) + path + + if loading.PostLoad.path_exists( + context, effective_path, self.parent_property + ): + return True + + path_w_prop = path[self.parent_property] + effective_path_w_prop = effective_path[self.parent_property] + + if not path_w_prop.contains(context.attributes, "loader"): + if join_depth: + if effective_path_w_prop.length / 2 > join_depth: + return True + elif effective_path_w_prop.contains_mapper(self.mapper): + return True + + return False + def _immediateload_create_row_processor( self, context, @@ -1214,6 +1236,9 @@ class ImmediateLoader(PostLoader): def load_immediate(state, dict_, row): state.get_impl(self.key).get(state, dict_) + if self._check_recursive_postload(context, path): + return + populators["delayed"].append((self.key, load_immediate)) @@ -1727,6 +1752,7 @@ class SubqueryLoader(PostLoader): adapter, populators, ): + if context.refresh_state: return self._immediateload_create_row_processor( context, @@ -1738,6 +1764,10 @@ class SubqueryLoader(PostLoader): adapter, populators, ) + # the subqueryloader does a similar check in setup_query() unlike + # the other post loaders, however we have this here for consistency + elif self._check_recursive_postload(context, path, self.join_depth): + return if not self.parent.class_manager[self.key].impl.supports_population: raise sa_exc.InvalidRequestError( @@ -2689,6 +2719,7 @@ class SelectInLoader(PostLoader, util.MemoizedSlots): adapter, populators, ): + if context.refresh_state: return self._immediateload_create_row_processor( context, @@ -2700,6 +2731,8 @@ class SelectInLoader(PostLoader, util.MemoizedSlots): adapter, populators, ) + elif self._check_recursive_postload(context, path, self.join_depth): + return if not self.parent.class_manager[self.key].impl.supports_population: raise sa_exc.InvalidRequestError( @@ -2721,13 +2754,7 @@ class SelectInLoader(PostLoader, util.MemoizedSlots): context.compile_state.current_path or orm_util.PathRegistry.root ) + path - if loading.PostLoad.path_exists( - context, selectin_path, self.parent_property - ): - return - path_w_prop = path[self.parent_property] - selectin_path_w_prop = selectin_path[self.parent_property] # build up a path indicating the path from the leftmost # entity to the thing we're subquery loading. @@ -2739,12 +2766,6 @@ class SelectInLoader(PostLoader, util.MemoizedSlots): else: effective_entity = self.entity - if not path_w_prop.contains(context.attributes, "loader"): - if self.join_depth: - if selectin_path_w_prop.length / 2 > self.join_depth: - return - elif selectin_path_w_prop.contains_mapper(self.mapper): - return loading.PostLoad.callable_for_path( context, selectin_path, diff --git a/test/orm/test_eager_relations.py b/test/orm/test_eager_relations.py index c0213cf227..421568b65a 100644 --- a/test/orm/test_eager_relations.py +++ b/test/orm/test_eager_relations.py @@ -3703,6 +3703,44 @@ class LoadOnExistingTest(_fixtures.FixtureTest): assert "addresses" in u1.__dict__ + @testing.combinations( + ("selectin",), + ("subquery",), + ("immediate",), + ) + def test_refresh_no_recursion(self, strat): + User, Address = self.classes.User, self.classes.Address + mapper( + User, + self.tables.users, + properties={ + "addresses": relationship( + Address, lazy="joined", back_populates="user" + ) + }, + ) + mapper( + Address, + self.tables.addresses, + properties={ + "user": relationship( + User, lazy=strat, back_populates="addresses" + ) + }, + ) + sess = fixture_session(autoflush=False) + + u1 = sess.query(User).get(8) + assert "addresses" in u1.__dict__ + sess.expire(u1) + + def go(): + eq_(u1.id, 8) + + self.assert_sql_count(testing.db, go, 1) + + assert "addresses" 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 -- 2.47.2