From: Mike Bayer Date: Mon, 9 Sep 2024 13:21:20 +0000 (-0400) Subject: deprecate joinedload, subqueryload with DML; use correct statement X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9ea449bf41006e94273186a974d3a1b091a0552a;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git deprecate joinedload, subqueryload with DML; use correct statement An ORM exception is raised if :func:`_orm.joinedload` or :func:`_orm.subqueryload` are used as a top level option against a statement that is not a SELECT statement, such as with an ``insert().returning()``. There are no JOINs in INSERT statements nor is there a "subquery" that can be repurposed for subquery eager loading, and for UPDATE/DELETE joinedload does not support these either, so it is never appropriate for this use to pass silently. Fixed issue where using eager loaders such as :func:`_orm.selectinload` with additional criteria in combination with ORM DML such as :func:`_sql.insert` with RETURNING would not correctly set up internal contexts required for caching to work correctly, leading to incorrect results. Fixes: #11853 Fixes: #11855 Change-Id: Ibbf46ba4f83e472441074c3257e23388e0fcec37 --- diff --git a/doc/build/changelog/unreleased_20/11853.rst b/doc/build/changelog/unreleased_20/11853.rst new file mode 100644 index 0000000000..92e6abdb68 --- /dev/null +++ b/doc/build/changelog/unreleased_20/11853.rst @@ -0,0 +1,11 @@ +.. change:: + :tags: bug, orm + :tickets: 11853 + + An warning is emitted if :func:`_orm.joinedload` or + :func:`_orm.subqueryload` are used as a top level option against a + statement that is not a SELECT statement, such as with an + ``insert().returning()``. There are no JOINs in INSERT statements nor is + there a "subquery" that can be repurposed for subquery eager loading, and + for UPDATE/DELETE joinedload does not support these either, so it is never + appropriate for this use to pass silently. diff --git a/doc/build/changelog/unreleased_20/11855.rst b/doc/build/changelog/unreleased_20/11855.rst new file mode 100644 index 0000000000..cee30cf8b3 --- /dev/null +++ b/doc/build/changelog/unreleased_20/11855.rst @@ -0,0 +1,9 @@ +.. change:: + :tags: bug, orm + :tickets: 11855 + + Fixed issue where using loader options such as :func:`_orm.selectinload` + with additional criteria in combination with ORM DML such as + :func:`_sql.insert` with RETURNING would not correctly set up internal + contexts required for caching to work correctly, leading to incorrect + results. diff --git a/lib/sqlalchemy/orm/bulk_persistence.py b/lib/sqlalchemy/orm/bulk_persistence.py index b5134034d6..9a14a7ecfc 100644 --- a/lib/sqlalchemy/orm/bulk_persistence.py +++ b/lib/sqlalchemy/orm/bulk_persistence.py @@ -621,6 +621,7 @@ class ORMDMLState(AbstractORMCompileState): querycontext = QueryContext( compile_state.from_statement_ctx, compile_state.select_statement, + statement, params, session, load_options, diff --git a/lib/sqlalchemy/orm/context.py b/lib/sqlalchemy/orm/context.py index 9ed154d067..4d11398bc7 100644 --- a/lib/sqlalchemy/orm/context.py +++ b/lib/sqlalchemy/orm/context.py @@ -108,6 +108,7 @@ class QueryContext: "top_level_context", "compile_state", "query", + "user_passed_query", "params", "load_options", "bind_arguments", @@ -155,6 +156,10 @@ class QueryContext: Select[Unpack[TupleAny]], FromStatement[Unpack[TupleAny]], ], + user_passed_query: Union[ + Select[Unpack[TupleAny]], + FromStatement[Unpack[TupleAny]], + ], params: _CoreSingleExecuteParams, session: Session, load_options: Union[ @@ -169,6 +174,13 @@ class QueryContext: self.bind_arguments = bind_arguments or _EMPTY_DICT self.compile_state = compile_state self.query = statement + + # the query that the end user passed to Session.execute() or similar. + # this is usually the same as .query, except in the bulk_persistence + # routines where a separate FromStatement is manufactured in the + # compile stage; this allows differentiation in that case. + self.user_passed_query = user_passed_query + self.session = session self.loaders_require_buffering = False self.loaders_require_uniquing = False @@ -176,7 +188,7 @@ class QueryContext: self.top_level_context = load_options._sa_top_level_orm_context cached_options = compile_state.select_statement._with_options - uncached_options = statement._with_options + uncached_options = user_passed_query._with_options # see issue #7447 , #8399 for some background # propagated loader options will be present on loaded InstanceState @@ -587,6 +599,7 @@ class ORMCompileState(AbstractORMCompileState): querycontext = QueryContext( compile_state, statement, + statement, params, session, load_options, diff --git a/lib/sqlalchemy/orm/query.py b/lib/sqlalchemy/orm/query.py index 88b4862e47..11936bbce8 100644 --- a/lib/sqlalchemy/orm/query.py +++ b/lib/sqlalchemy/orm/query.py @@ -2961,6 +2961,7 @@ class Query( context = QueryContext( compile_state, compile_state.statement, + compile_state.statement, self._params, self.session, self.load_options, @@ -3320,6 +3321,7 @@ class Query( context = QueryContext( compile_state, compile_state.statement, + compile_state.statement, self._params, self.session, self.load_options, diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index 5adbc5f125..996bdbc1d9 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -1965,6 +1965,18 @@ class SubqueryLoader(PostLoader): adapter, populators, ): + if ( + loadopt + and context.compile_state.statement is not None + and context.compile_state.statement.is_dml + ): + util.warn_deprecated( + "The subqueryload loader option is not compatible with DML " + "statements such as INSERT, UPDATE. Only SELECT may be used." + "This warning will become an exception in a future release.", + "2.0", + ) + if context.refresh_state: return self._immediateload_create_row_processor( context, @@ -2130,6 +2142,17 @@ class JoinedLoader(AbstractRelationshipLoader): if not compile_state.compile_options._enable_eagerloads: return + elif ( + loadopt + and compile_state.statement is not None + and compile_state.statement.is_dml + ): + util.warn_deprecated( + "The joinedload loader option is not compatible with DML " + "statements such as INSERT, UPDATE. Only SELECT may be used." + "This warning will become an exception in a future release.", + "2.0", + ) elif self.uselist: compile_state.multi_row_eager_loaders = True @@ -3215,7 +3238,7 @@ class SelectInLoader(PostLoader, util.MemoizedSlots): orig_query = context.compile_state.select_statement # the actual statement that was requested is this one: - # context_query = context.query + # context_query = context.user_passed_query # # that's not the cached one, however. So while it is of the identical # structure, if it has entities like AliasedInsp, which we get from @@ -3239,11 +3262,11 @@ class SelectInLoader(PostLoader, util.MemoizedSlots): effective_path = path[self.parent_property] - if orig_query is context.query: + if orig_query is context.user_passed_query: new_options = orig_query._with_options else: cached_options = orig_query._with_options - uncached_options = context.query._with_options + uncached_options = context.user_passed_query._with_options # propagate compile state options from the original query, # updating their "extra_criteria" as necessary. diff --git a/lib/sqlalchemy/orm/strategy_options.py b/lib/sqlalchemy/orm/strategy_options.py index f4b0bb9a96..d62fba9890 100644 --- a/lib/sqlalchemy/orm/strategy_options.py +++ b/lib/sqlalchemy/orm/strategy_options.py @@ -1081,7 +1081,7 @@ class Load(_AbstractLoad): else: return self - replacement_cache_key = context.query._generate_cache_key() + replacement_cache_key = context.user_passed_query._generate_cache_key() if replacement_cache_key is None: return self diff --git a/test/orm/dml/test_bulk_statements.py b/test/orm/dml/test_bulk_statements.py index 1e5c17c9de..8c6acf4dec 100644 --- a/test/orm/dml/test_bulk_statements.py +++ b/test/orm/dml/test_bulk_statements.py @@ -25,13 +25,21 @@ from sqlalchemy import update from sqlalchemy.orm import aliased from sqlalchemy.orm import Bundle from sqlalchemy.orm import column_property +from sqlalchemy.orm import DeclarativeBase +from sqlalchemy.orm import immediateload +from sqlalchemy.orm import joinedload +from sqlalchemy.orm import lazyload from sqlalchemy.orm import load_only from sqlalchemy.orm import Mapped from sqlalchemy.orm import mapped_column from sqlalchemy.orm import orm_insert_sentinel +from sqlalchemy.orm import relationship +from sqlalchemy.orm import selectinload from sqlalchemy.orm import Session +from sqlalchemy.orm import subqueryload from sqlalchemy.testing import config from sqlalchemy.testing import eq_ +from sqlalchemy.testing import expect_deprecated from sqlalchemy.testing import expect_raises_message from sqlalchemy.testing import expect_warnings from sqlalchemy.testing import fixtures @@ -2298,3 +2306,206 @@ class CTETest(fixtures.DeclarativeMappedTest): asserter.assert_( CompiledSQL(expected, [{"param_1": id_, "param_2": "some user"}]) ) + + +class EagerLoadTest( + fixtures.DeclarativeMappedTest, testing.AssertsExecutionResults +): + run_inserts = "each" + + @classmethod + def setup_classes(cls): + Base = cls.DeclarativeBasic + + class A(Base): + __tablename__ = "a" + id: Mapped[int] = mapped_column(Integer, primary_key=True) + cs = relationship("C") + + class B(Base): + __tablename__ = "b" + id: Mapped[int] = mapped_column(Integer, primary_key=True) + a_id: Mapped[int] = mapped_column(ForeignKey("a.id")) + a = relationship("A") + + class C(Base): + __tablename__ = "c" + id: Mapped[int] = mapped_column(Integer, primary_key=True) + a_id: Mapped[int] = mapped_column(ForeignKey("a.id")) + + @classmethod + def insert_data(cls, connection): + A = cls.classes.A + C = cls.classes.C + with Session(connection) as sess: + sess.add_all( + [ + A(id=1, cs=[C(id=1), C(id=2)]), + A(id=2), + A(id=3, cs=[C(id=3), C(id=4)]), + ] + ) + sess.commit() + + @testing.fixture + def fixture_with_loader_opt(self): + def go(lazy): + class Base(DeclarativeBase): + pass + + class A(Base): + __tablename__ = "a" + id: Mapped[int] = mapped_column(Integer, primary_key=True) + + class B(Base): + __tablename__ = "b" + id: Mapped[int] = mapped_column(Integer, primary_key=True) + a_id: Mapped[int] = mapped_column(ForeignKey("a.id")) + a = relationship("A", lazy=lazy) + + return A, B + + return go + + @testing.combinations( + (selectinload,), + (immediateload,), + ) + def test_insert_supported(self, loader): + A, B = self.classes("A", "B") + + sess = fixture_session() + + result = sess.execute( + insert(B).returning(B).options(loader(B.a)), + [ + {"id": 1, "a_id": 1}, + {"id": 2, "a_id": 1}, + {"id": 3, "a_id": 2}, + {"id": 4, "a_id": 3}, + {"id": 5, "a_id": 3}, + ], + ).scalars() + + for b in result: + assert "a" in b.__dict__ + + @testing.combinations( + (joinedload,), + (subqueryload,), + ) + def test_insert_not_supported(self, loader): + """test #11853""" + + A, B = self.classes("A", "B") + + sess = fixture_session() + + stmt = insert(B).returning(B).options(loader(B.a)) + + with expect_deprecated( + f"The {loader.__name__} loader option is not compatible " + "with DML statements", + ): + sess.execute(stmt, [{"id": 1, "a_id": 1}]) + + @testing.combinations( + (joinedload,), + (subqueryload,), + (selectinload,), + (immediateload,), + ) + def test_secondary_opt_ok(self, loader): + A, B = self.classes("A", "B") + + sess = fixture_session() + + opt = selectinload(B.a) + opt = getattr(opt, loader.__name__)(A.cs) + + result = sess.execute( + insert(B).returning(B).options(opt), + [ + {"id": 1, "a_id": 1}, + {"id": 2, "a_id": 1}, + {"id": 3, "a_id": 2}, + {"id": 4, "a_id": 3}, + {"id": 5, "a_id": 3}, + ], + ).scalars() + + for b in result: + assert "a" in b.__dict__ + assert "cs" in b.a.__dict__ + + @testing.combinations( + ("joined",), + ("select",), + ("subquery",), + ("selectin",), + ("immediate",), + argnames="lazy_opt", + ) + def test_insert_handles_implicit(self, fixture_with_loader_opt, lazy_opt): + """test #11853""" + + A, B = fixture_with_loader_opt(lazy_opt) + + sess = fixture_session() + + for b_obj in sess.execute( + insert(B).returning(B), + [ + {"id": 1, "a_id": 1}, + {"id": 2, "a_id": 1}, + {"id": 3, "a_id": 2}, + {"id": 4, "a_id": 3}, + {"id": 5, "a_id": 3}, + ], + ).scalars(): + + if lazy_opt in ("select", "joined", "subquery"): + # these aren't supported by DML + assert "a" not in b_obj.__dict__ + else: + # the other three are + assert "a" in b_obj.__dict__ + + @testing.combinations( + (lazyload,), (selectinload,), (immediateload,), argnames="loader_opt" + ) + @testing.combinations( + (joinedload,), + (subqueryload,), + (selectinload,), + (immediateload,), + (lazyload,), + argnames="secondary_opt", + ) + def test_secondary_w_criteria_caching(self, loader_opt, secondary_opt): + """test #11855""" + A, B, C = self.classes("A", "B", "C") + + for i in range(3): + with fixture_session() as sess: + + opt = loader_opt(B.a) + opt = getattr(opt, secondary_opt.__name__)( + A.cs.and_(C.a_id == 1) + ) + stmt = insert(B).returning(B).options(opt) + + b1 = sess.scalar(stmt, [{"a_id": 1}]) + + eq_({c.id for c in b1.a.cs}, {1, 2}) + + opt = loader_opt(B.a) + opt = getattr(opt, secondary_opt.__name__)( + A.cs.and_(C.a_id == 3) + ) + + stmt = insert(B).returning(B).options(opt) + + b3 = sess.scalar(stmt, [{"a_id": 3}]) + + eq_({c.id for c in b3.a.cs}, {3, 4})