From: Mike Bayer Date: Thu, 18 Apr 2024 22:17:21 +0000 (-0400) Subject: consider propagate_to_loaders at application time X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=80399cefa1b16a8548ba0c997a1eda94b8e9db01;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git consider propagate_to_loaders at application time Fixed regression from 1.4 where using :func:`_orm.defaultload` in conjunction with a non-propagating loader like :func:`_orm.contains_eager` would nonetheless propagate the :func:`_orm.contains_eager` to a lazy load operation, causing incorrect queries as this option is only intended to come from an original load. Fixes: #11292 Change-Id: I79928afa108970b523f2166c3190f7952eca73ed --- diff --git a/doc/build/changelog/unreleased_20/11292.rst b/doc/build/changelog/unreleased_20/11292.rst new file mode 100644 index 0000000000..65fbdf719a --- /dev/null +++ b/doc/build/changelog/unreleased_20/11292.rst @@ -0,0 +1,11 @@ +.. change:: + :tags: bug, orm, regression + :tickets: 11292 + + Fixed regression from 1.4 where using :func:`_orm.defaultload` in + conjunction with a non-propagating loader like :func:`_orm.contains_eager` + would nonetheless propagate the :func:`_orm.contains_eager` to a lazy load + operation, causing incorrect queries as this option is only intended to + come from an original load. + + diff --git a/lib/sqlalchemy/orm/strategy_options.py b/lib/sqlalchemy/orm/strategy_options.py index 4bfdd78ff5..8d691aa20c 100644 --- a/lib/sqlalchemy/orm/strategy_options.py +++ b/lib/sqlalchemy/orm/strategy_options.py @@ -98,6 +98,7 @@ class _AbstractLoad(traversals.GenerativeOnTraversal, LoaderOption): attr: _AttrType, alias: Optional[_FromClauseArgument] = None, _is_chain: bool = False, + _propagate_to_loaders: bool = False, ) -> Self: r"""Indicate that the given attribute should be eagerly loaded from columns stated manually in the query. @@ -158,7 +159,7 @@ class _AbstractLoad(traversals.GenerativeOnTraversal, LoaderOption): cloned = self._set_relationship_strategy( attr, {"lazy": "joined"}, - propagate_to_loaders=False, + propagate_to_loaders=_propagate_to_loaders, opts={"eager_from_alias": coerced_alias}, _reconcile_to_other=True if _is_chain else None, ) @@ -1146,7 +1147,20 @@ class Load(_AbstractLoad): mapper_entities, raiseerr ) + # if the context has a current path, this is a lazy load + has_current_path = bool(compile_state.compile_options._current_path) + for loader in self.context: + # issue #11292 + # historically, propagate_to_loaders was only considered at + # object loading time, whether or not to carry along options + # onto an object's loaded state where it would be used by lazyload. + # however, the defaultload() option needs to propagate in case + # its sub-options propagate_to_loaders, but its sub-options + # that dont propagate should not be applied for lazy loaders. + # so we check again + if has_current_path and not loader.propagate_to_loaders: + continue loader.process_compile_state( self, compile_state, diff --git a/test/orm/test_default_strategies.py b/test/orm/test_default_strategies.py index 657875aa9d..178b03fe6f 100644 --- a/test/orm/test_default_strategies.py +++ b/test/orm/test_default_strategies.py @@ -1,11 +1,18 @@ import sqlalchemy as sa +from sqlalchemy import Column +from sqlalchemy import ForeignKey +from sqlalchemy import Integer +from sqlalchemy import select from sqlalchemy import testing from sqlalchemy import util +from sqlalchemy.orm import contains_eager from sqlalchemy.orm import defaultload from sqlalchemy.orm import joinedload from sqlalchemy.orm import relationship +from sqlalchemy.orm import Session from sqlalchemy.orm import subqueryload from sqlalchemy.testing import eq_ +from sqlalchemy.testing import fixtures from sqlalchemy.testing.assertions import expect_raises_message from sqlalchemy.testing.fixtures import fixture_session from test.orm import _fixtures @@ -738,3 +745,122 @@ class NoLoadTest(_fixtures.FixtureTest): eq_(a1.user, None) self.sql_count_(0, go) + + +class Issue11292Test(fixtures.DeclarativeMappedTest): + @classmethod + def setup_classes(cls): + Base = cls.DeclarativeBasic + + class Parent(Base): + __tablename__ = "parent" + + id = Column(Integer, primary_key=True) + + extension = relationship( + "Extension", back_populates="parent", uselist=False + ) + + class Child(Base): + __tablename__ = "child" + + id = Column(Integer, primary_key=True) + + extensions = relationship("Extension", back_populates="child") + + class Extension(Base): + __tablename__ = "extension" + + id = Column(Integer, primary_key=True) + parent_id = Column(Integer, ForeignKey(Parent.id)) + child_id = Column(Integer, ForeignKey(Child.id)) + + parent = relationship("Parent", back_populates="extension") + child = relationship("Child", back_populates="extensions") + + @classmethod + def insert_data(cls, connection): + Parent, Child, Extension = cls.classes("Parent", "Child", "Extension") + with Session(connection) as session: + for id_ in (1, 2, 3): + session.add(Parent(id=id_)) + session.add(Child(id=id_)) + session.add(Extension(id=id_, parent_id=id_, child_id=id_)) + session.commit() + + @testing.variation("load_as_option", [True, False]) + def test_defaultload_dont_propagate(self, load_as_option): + Parent, Child, Extension = self.classes("Parent", "Child", "Extension") + + session = fixture_session() + + # here, we want the defaultload() to go away on subsequent loads, + # becuase Parent.extension is propagate_to_loaders=False + query = ( + select(Parent) + .join(Extension) + .join(Child) + .options( + contains_eager(Parent.extension), + ( + defaultload(Parent.extension).options( + contains_eager(Extension.child) + ) + if load_as_option + else defaultload(Parent.extension).contains_eager( + Extension.child + ) + ), + ) + ) + + parents = session.scalars(query).all() + + eq_( + [(p.id, p.extension.id, p.extension.child.id) for p in parents], + [(1, 1, 1), (2, 2, 2), (3, 3, 3)], + ) + + session.expire_all() + + eq_( + [(p.id, p.extension.id, p.extension.child.id) for p in parents], + [(1, 1, 1), (2, 2, 2), (3, 3, 3)], + ) + + @testing.variation("load_as_option", [True, False]) + def test_defaultload_yes_propagate(self, load_as_option): + Parent, Child, Extension = self.classes("Parent", "Child", "Extension") + + session = fixture_session() + + # here, we want the defaultload() to go away on subsequent loads, + # becuase Parent.extension is propagate_to_loaders=False + query = select(Parent).options( + ( + defaultload(Parent.extension).options( + joinedload(Extension.child) + ) + if load_as_option + else defaultload(Parent.extension).joinedload(Extension.child) + ), + ) + + parents = session.scalars(query).all() + + eq_( + [(p.id, p.extension.id, p.extension.child.id) for p in parents], + [(1, 1, 1), (2, 2, 2), (3, 3, 3)], + ) + + session.expire_all() + + # this would be 9 without the joinedload + with self.assert_statement_count(testing.db, 6): + eq_( + [ + (p.id, p.extension.id, p.extension.child.id) + for p in parents + ], + [(1, 1, 1), (2, 2, 2), (3, 3, 3)], + ) diff --git a/test/orm/test_options.py b/test/orm/test_options.py index db9b51607c..c6058a80b3 100644 --- a/test/orm/test_options.py +++ b/test/orm/test_options.py @@ -419,7 +419,10 @@ class OptionsTest(PathTest, QueryTest): # loader option works this way right now; the rest all use # defaultload() for the "chain" elements return strategy_options._generate_from_keys( - strategy_options.Load.contains_eager, arg, True, {} + strategy_options.Load.contains_eager, + arg, + True, + dict(_propagate_to_loaders=True), ) @testing.combinations( diff --git a/test/profiles.txt b/test/profiles.txt index 7c8b174dc1..b585ad64ab 100644 --- a/test/profiles.txt +++ b/test/profiles.txt @@ -1,15 +1,15 @@ # /home/classic/dev/sqlalchemy/test/profiles.txt # This file is written out on a per-environment basis. -# For each test in aaa_profiling, the corresponding function and +# For each test in aaa_profiling, the corresponding function and # environment is located within this file. If it doesn't exist, # the test is skipped. -# If a callcount does exist, it is compared to what we received. +# If a callcount does exist, it is compared to what we received. # assertions are raised if the counts do not match. -# -# To add a new callcount test, apply the function_call_count -# decorator and re-run the tests using the --write-profiles +# +# To add a new callcount test, apply the function_call_count +# decorator and re-run the tests using the --write-profiles # option - this file will be rewritten including the new count. -# +# # TEST: test.aaa_profiling.test_compiler.CompileTest.test_insert @@ -296,17 +296,17 @@ test.aaa_profiling.test_orm.AttributeOverheadTest.test_collection_append_remove # TEST: test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_key_bound_branching -test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_key_bound_branching x86_64_linux_cpython_3.11_sqlite_pysqlite_dbapiunicode_cextensions 128 -test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_key_bound_branching x86_64_linux_cpython_3.11_sqlite_pysqlite_dbapiunicode_nocextensions 128 -test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_key_bound_branching x86_64_linux_cpython_3.12_sqlite_pysqlite_dbapiunicode_cextensions 124 -test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_key_bound_branching x86_64_linux_cpython_3.12_sqlite_pysqlite_dbapiunicode_nocextensions 124 +test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_key_bound_branching x86_64_linux_cpython_3.11_sqlite_pysqlite_dbapiunicode_cextensions 136 +test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_key_bound_branching x86_64_linux_cpython_3.11_sqlite_pysqlite_dbapiunicode_nocextensions 136 +test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_key_bound_branching x86_64_linux_cpython_3.12_sqlite_pysqlite_dbapiunicode_cextensions 132 +test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_key_bound_branching x86_64_linux_cpython_3.12_sqlite_pysqlite_dbapiunicode_nocextensions 132 # TEST: test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_unbound_branching -test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_unbound_branching x86_64_linux_cpython_3.11_sqlite_pysqlite_dbapiunicode_cextensions 128 -test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_unbound_branching x86_64_linux_cpython_3.11_sqlite_pysqlite_dbapiunicode_nocextensions 128 -test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_unbound_branching x86_64_linux_cpython_3.12_sqlite_pysqlite_dbapiunicode_cextensions 124 -test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_unbound_branching x86_64_linux_cpython_3.12_sqlite_pysqlite_dbapiunicode_nocextensions 124 +test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_unbound_branching x86_64_linux_cpython_3.11_sqlite_pysqlite_dbapiunicode_cextensions 136 +test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_unbound_branching x86_64_linux_cpython_3.11_sqlite_pysqlite_dbapiunicode_nocextensions 136 +test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_unbound_branching x86_64_linux_cpython_3.12_sqlite_pysqlite_dbapiunicode_cextensions 132 +test.aaa_profiling.test_orm.BranchedOptionTest.test_query_opts_unbound_branching x86_64_linux_cpython_3.12_sqlite_pysqlite_dbapiunicode_nocextensions 132 # TEST: test.aaa_profiling.test_orm.DeferOptionsTest.test_baseline