From: Mike Bayer Date: Mon, 3 May 2021 13:55:12 +0000 (-0400) Subject: loader strategy regression fixes X-Git-Tag: rel_1_4_13~2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=216de6e7ceb893200fcde6b158c51180866f4004;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git loader strategy regression fixes Fixed regression where using :func:`_orm.selectinload` and :func:`_orm.subqueryload` to load a two-level-deep path would lead to an attribute error. Fixed regression where using the :func:`_orm.noload` loader strategy in conjunction with a "dynamic" relationship would lead to an attribute error as the noload strategy would attempt to apply itself to the dynamic loader. Fixes: #6419 Fixes: #6420 Change-Id: I933b208f16a9723f6ebeab7addbe118903a1f8f5 --- diff --git a/doc/build/changelog/unreleased_14/6419_6420.rst b/doc/build/changelog/unreleased_14/6419_6420.rst new file mode 100644 index 0000000000..af1d4d732c --- /dev/null +++ b/doc/build/changelog/unreleased_14/6419_6420.rst @@ -0,0 +1,15 @@ +.. change:: + :tags: bug, orm, regression + :tickets: 6419 + + Fixed regression where using :func:`_orm.selectinload` and + :func:`_orm.subqueryload` to load a two-level-deep path would lead to an + attribute error. + +.. change:: + :tags: bug, orm, regression + :tickets: #6420 + + Fixed regression where using the :func:`_orm.noload` loader strategy in + conjunction with a "dynamic" relationship would lead to an attribute error + as the noload strategy would attempt to apply itself to the dynamic loader. diff --git a/lib/sqlalchemy/orm/dynamic.py b/lib/sqlalchemy/orm/dynamic.py index bf8fc0e337..a4b5f58c72 100644 --- a/lib/sqlalchemy/orm/dynamic.py +++ b/lib/sqlalchemy/orm/dynamic.py @@ -105,10 +105,11 @@ class DynamicAttributeImpl(attributes.AttributeImpl): passive=attributes.PASSIVE_NO_INITIALIZE, ): if not passive & attributes.SQL_OK: - return self._get_collection_history(state, passive).added_items + data = self._get_collection_history(state, passive).added_items else: history = self._get_collection_history(state, passive) - return history.added_plus_unchanged + data = history.added_plus_unchanged + return DynamicCollectionAdapter(data) @util.memoized_property def _append_token(self): @@ -259,6 +260,27 @@ class DynamicAttributeImpl(attributes.AttributeImpl): self.remove(state, dict_, value, initiator, passive=passive) +class DynamicCollectionAdapter(object): + """simplified CollectionAdapter for internal API consistency""" + + def __init__(self, data): + self.data = data + + def __iter__(self): + return iter(self.data) + + def _reset_empty(self): + pass + + def __len__(self): + return len(self.data) + + def __bool__(self): + return True + + __nonzero__ = __bool__ + + class AppenderMixin(object): query_class = None diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index e43fa09a0b..2c37ecaa0f 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -1265,7 +1265,13 @@ class SubqueryLoader(PostLoader): (("lazy", "select"),) ).init_class_attribute(mapper) - def _get_leftmost(self, subq_path, current_compile_state, is_root): + def _get_leftmost( + self, + orig_query_entity_index, + subq_path, + current_compile_state, + is_root, + ): given_subq_path = subq_path subq_path = subq_path.path subq_mapper = orm_util._class_to_mapper(subq_path[0]) @@ -1285,9 +1291,8 @@ class SubqueryLoader(PostLoader): # of the current state. this is for the specific case of the entity # is an AliasedClass against a subquery that's not otherwise going # to adapt - new_subq_path = current_compile_state._entities[ - 0 + orig_query_entity_index ].entity_zero._path_registry[leftmost_prop] additional = len(subq_path) - len(new_subq_path) if additional: @@ -1609,6 +1614,7 @@ class SubqueryLoader(PostLoader): def _setup_query_from_rowproc( self, context, + query_entity, path, entity, loadopt, @@ -1621,6 +1627,7 @@ class SubqueryLoader(PostLoader): ): return + orig_query_entity_index = compile_state._entities.index(query_entity) context.loaders_require_buffering = True path = path[self.parent_property] @@ -1645,6 +1652,8 @@ class SubqueryLoader(PostLoader): # if not via query option, check for # a cycle + # TODO: why is this here??? this is now handled + # by the _check_recursive_postload call if not path.contains(compile_state.attributes, "loader"): if self.join_depth: if ( @@ -1699,7 +1708,12 @@ class SubqueryLoader(PostLoader): leftmost_attr, leftmost_relationship, rewritten_path, - ) = self._get_leftmost(rewritten_path, orig_compile_state, is_root) + ) = self._get_leftmost( + orig_query_entity_index, + rewritten_path, + orig_compile_state, + is_root, + ) # generate a new Query from the original, then # produce a subquery from it. @@ -1796,6 +1810,7 @@ class SubqueryLoader(PostLoader): subq = self._setup_query_from_rowproc( context, + query_entity, path, path[-1], loadopt, diff --git a/test/orm/test_dynamic.py b/test/orm/test_dynamic.py index fc47b91dfb..8ea04c2689 100644 --- a/test/orm/test_dynamic.py +++ b/test/orm/test_dynamic.py @@ -11,6 +11,7 @@ from sqlalchemy.orm import backref from sqlalchemy.orm import configure_mappers from sqlalchemy.orm import exc as orm_exc from sqlalchemy.orm import mapper +from sqlalchemy.orm import noload from sqlalchemy.orm import Query from sqlalchemy.orm import relationship from sqlalchemy.testing import assert_raises @@ -421,6 +422,36 @@ class DynamicTest(_DynamicFixture, _fixtures.FixtureTest, AssertsCompiledSQL): [], ) + @testing.combinations( + ("star",), + ("attronly",), + ) + def test_noload_issue(self, type_): + """test #6420. a noload that hits the dynamic loader + should have no effect. + + """ + + User, Address = self._user_address_fixture() + + s = fixture_session() + + if type_ == "star": + u1 = s.query(User).filter_by(id=7).options(noload("*")).first() + assert "name" not in u1.__dict__["name"] + elif type_ == "attronly": + u1 = ( + s.query(User) + .filter_by(id=7) + .options(noload(User.addresses)) + .first() + ) + + eq_(u1.__dict__["name"], "jack") + + # noload doesn't affect a dynamic loader, because it has no state + eq_(list(u1.addresses), [Address(id=1)]) + def test_m2m(self): Order, Item = self._order_item_fixture( items_args={"backref": backref("orders", lazy="dynamic")} diff --git a/test/orm/test_subquery_relations.py b/test/orm/test_subquery_relations.py index 5fa9254c02..d5f5f98b19 100644 --- a/test/orm/test_subquery_relations.py +++ b/test/orm/test_subquery_relations.py @@ -15,6 +15,7 @@ from sqlalchemy.orm import deferred from sqlalchemy.orm import joinedload from sqlalchemy.orm import mapper from sqlalchemy.orm import relationship +from sqlalchemy.orm import selectinload from sqlalchemy.orm import Session from sqlalchemy.orm import subqueryload from sqlalchemy.orm import undefer @@ -26,6 +27,7 @@ from sqlalchemy.testing import fixtures from sqlalchemy.testing import is_ from sqlalchemy.testing import is_not from sqlalchemy.testing import is_true +from sqlalchemy.testing.assertions import expect_warnings from sqlalchemy.testing.assertsql import CompiledSQL from sqlalchemy.testing.entities import ComparableEntity from sqlalchemy.testing.fixtures import fixture_session @@ -3562,3 +3564,115 @@ class FromSubqTest(fixtures.DeclarativeMappedTest): ), ) s.close() + + +class Issue6149Test(fixtures.DeclarativeMappedTest): + @classmethod + def setup_classes(cls): + Base = cls.DeclarativeBasic + + class Exam(ComparableEntity, Base): + __tablename__ = "exam" + id = Column(Integer, primary_key=True, autoincrement=True) + name = Column(String(50), nullable=False) + submissions = relationship( + "Submission", backref="exam", cascade="all", lazy=True + ) + + class Submission(ComparableEntity, Base): + __tablename__ = "submission" + id = Column(Integer, primary_key=True, autoincrement=True) + exam_id = Column( + Integer, ForeignKey("exam.id"), nullable=False + ) # backref exam + solutions = relationship( + "Solution", + backref="submission", + cascade="all", + order_by="Solution.id", + lazy=True, + ) + + class Solution(ComparableEntity, Base): + __tablename__ = "solution" + id = Column(Integer, primary_key=True, autoincrement=True) + name = Column(String(50), nullable=False) + submission_id = Column( + Integer, ForeignKey("submission.id"), nullable=False + ) # backref submission + + @classmethod + def insert_data(cls, connection): + Exam, Submission, Solution = cls.classes( + "Exam", "Submission", "Solution" + ) + + s = Session(connection) + + e1 = Exam( + id=1, + name="e1", + submissions=[ + Submission( + solutions=[Solution(name="s1"), Solution(name="s2")] + ), + Submission( + solutions=[Solution(name="s3"), Solution(name="s4")] + ), + ], + ) + + s.add(e1) + s.commit() + + def test_issue_6419(self): + Exam, Submission, Solution = self.classes( + "Exam", "Submission", "Solution" + ) + + s = fixture_session() + + for i in range(3): + # this warns because subqueryload is from the + # selectinload, which means we have to unwrap the + # selectinload query to see what its entities are. + with expect_warnings(r".*must invoke lambda callable"): + + # the bug is that subqueryload looks at the query that + # selectinload created and assumed the "entity" was + # compile_state._entities[0], which in this case is a + # Bundle, it needs to look at compile_state._entities[1]. + # so subqueryloader passes through orig_query_entity_index + # so it knows where to look. + ex1 = ( + s.query(Exam) + .options( + selectinload(Exam.submissions).subqueryload( + Submission.solutions + ) + ) + .filter_by(id=1) + .first() + ) + + eq_( + ex1, + Exam( + name="e1", + submissions=[ + Submission( + solutions=[ + Solution(name="s1"), + Solution(name="s2"), + ] + ), + Submission( + solutions=[ + Solution(name="s3"), + Solution(name="s4"), + ] + ), + ], + ), + ) + s.close()