From: Mike Bayer Date: Fri, 11 Dec 2020 15:34:44 +0000 (-0500) Subject: Revise attribute refresh for with_loader_criteria, related X-Git-Tag: rel_1_4_0b2~107^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c46279d1c215b7af956e40cb29afafba29d9143f;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Revise attribute refresh for with_loader_criteria, related Added new attribute :attr:`_orm.ORMExecuteState.is_column_load` to indicate that a :meth:`_orm.SessionEvents.do_orm_execute` handler that a particular operation is a primary-key-directed column attribute load, such as from an expiration or a deferred attribute, and that WHERE criteria or additional loader options should not be added to the query. This has been added to the examples which illustrate the :func:`_orm.with_loader_criteria` option. The :func:`_orm.with_loader_criteria` option has been modified so that it will never apply its criteria to the SELECT statement for an ORM refresh operation, such as that invoked by :meth:`_orm.Session.refresh` or whenever an expired attribute is loaded. These queries are only against the primary key row of the object that is already present in memory so there should not be additional criteria added. Added doc caveats for using lambdas. Added test coverage for most ORMExecuteState flags and fixed a few basic access issues. Change-Id: I6707e4cf0dc95cdfb8ce93e5ca22ead86074baa7 References: #5760 Fixes: #5761 Fixes: #5762 --- diff --git a/doc/build/changelog/unreleased_14/5761.rst b/doc/build/changelog/unreleased_14/5761.rst new file mode 100644 index 0000000000..9e36f2a898 --- /dev/null +++ b/doc/build/changelog/unreleased_14/5761.rst @@ -0,0 +1,10 @@ +.. change:: + :tags: bug, orm + :tickets: 5761 + + Added new attribute :attr:`_orm.ORMExecuteState.is_column_load` to indicate + that a :meth:`_orm.SessionEvents.do_orm_execute` handler that a particular + operation is a primary-key-directed column attribute load, such as from an + expiration or a deferred attribute, and that WHERE criteria or additional + loader options should not be added to the query. This has been added to + the examples which illustrate the :func:`_orm.with_loader_criteria` option. \ No newline at end of file diff --git a/doc/build/changelog/unreleased_14/5762.rst b/doc/build/changelog/unreleased_14/5762.rst new file mode 100644 index 0000000000..7b5a90cdf1 --- /dev/null +++ b/doc/build/changelog/unreleased_14/5762.rst @@ -0,0 +1,10 @@ +.. change:: + :tags: bug, orm + :tickets: 5762 + + The :func:`_orm.with_loader_criteria` option has been modified so that it + will never apply its criteria to the SELECT statement for an ORM refresh + operation, such as that invoked by :meth:`_orm.Session.refresh` or whenever + an expired attribute is loaded. These queries are only against the + primary key row of the object that is already present in memory so there + should not be additional criteria added. \ No newline at end of file diff --git a/doc/build/orm/session_events.rst b/doc/build/orm/session_events.rst index 0040f6f47f..544a6c5773 100644 --- a/doc/build/orm/session_events.rst +++ b/doc/build/orm/session_events.rst @@ -87,7 +87,12 @@ may be used on its own, or is ideally suited to be used within the @event.listens_for(Session, "do_orm_execute") def _do_orm_execute(orm_execute_state): - if orm_execute_state.is_select: + + if ( + orm_execute_state.is_select and + not orm_execute_state.is_column_load and + not orm_execute_state.is_relationship_load + ): orm_execute_state.statement = orm_execute_state.statement.options( with_loader_criteria(MyEntity.public == True) ) @@ -95,7 +100,9 @@ may be used on its own, or is ideally suited to be used within the Above, an option is added to all SELECT statements that will limit all queries against ``MyEntity`` to filter on ``public == True``. The criteria will be applied to **all** loads of that class within the scope of the -immediate query as well as subsequent relationship loads, which includes +immediate query. The :func:`_orm.with_loader_criteria` option by default +will automatically propagate to relationship loaders as well, which will +apply to subsequent relationship loads, which includes lazy loads, selectinloads, etc. For a series of classes that all feature some common column structure, @@ -127,7 +134,11 @@ to intercept all objects that extend from ``HasTimestamp`` and filter their @event.listens_for(Session, "do_orm_execute") def _do_orm_execute(orm_execute_state): - if orm_execute_state.is_select: + if ( + orm_execute_state.is_select + and not orm_execute_state.is_column_load + and not orm_execute_state.is_relationship_load + ): one_month_ago = datetime.datetime.today() - datetime.timedelta(months=1) orm_execute_state.statement = orm_execute_state.statement.options( @@ -138,6 +149,12 @@ to intercept all objects that extend from ``HasTimestamp`` and filter their ) ) +.. warning:: The use of a lambda inside of the call to + :func:`_orm.with_loader_criteria` is only invoked **once per unique class**. + Custom functions should not be invoked within this lambda. See + :ref:`engine_lambda_caching` for an overview of the "lambda SQL" feature, + which is for advanced use only. + .. seealso:: :ref:`examples_session_orm_events` - includes working examples of the diff --git a/examples/extending_query/filter_public.py b/examples/extending_query/filter_public.py index b6d4ec0db3..53472e95a3 100644 --- a/examples/extending_query/filter_public.py +++ b/examples/extending_query/filter_public.py @@ -37,7 +37,8 @@ def _add_filtering_criteria(execute_state): # query. if ( - not execute_state.is_relationship_load + not execute_state.is_column_load + and not execute_state.is_relationship_load and not execute_state.execution_options.get("include_private", False) ): execute_state.statement = execute_state.statement.options( diff --git a/lib/sqlalchemy/orm/interfaces.py b/lib/sqlalchemy/orm/interfaces.py index 64f561cbd1..bacec422c8 100644 --- a/lib/sqlalchemy/orm/interfaces.py +++ b/lib/sqlalchemy/orm/interfaces.py @@ -724,8 +724,8 @@ class ORMOption(ExecutableOption): propagate_to_loaders = False """if True, indicate this option should be carried along - to "secondary" Query objects produced during lazy loads - or refresh operations. + to "secondary" SELECT statements that occur for relationship + lazy loaders as well as attribute load / refresh operations. """ diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index 2a531b1183..f6943cc5f1 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -272,6 +272,8 @@ class ORMExecuteState(util.MemoizedSlots): self.local_execution_options = self.local_execution_options.union(opts) def _orm_compile_options(self): + if not self.is_select: + return None opts = self.statement._compile_options if isinstance(opts, context.ORMCompileState.default_compile_options): return opts @@ -307,6 +309,33 @@ class ORMExecuteState(util.MemoizedSlots): else: return None + @property + def is_column_load(self): + """Return True if the operation is refreshing column-oriented + attributes on an existing ORM object. + + This occurs during operations such as :meth:`_orm.Session.refresh`, + as well as when an attribute deferred by :func:`_orm.defer` is + being loaded, or an attribute that was expired either directly + by :meth:`_orm.Session.expire` or via a commit operation is being + loaded. + + Handlers will very likely not want to add any options to queries + when such an operation is occurring as the query should be a straight + primary key fetch which should not have any additional WHERE criteria, + and loader options travelling with the instance + will have already been added to the query. + + .. versionadded:: 1.4.0b2 + + .. seealso:: + + :attr:`_orm.ORMExecuteState.is_relationship_load` + + """ + opts = self._orm_compile_options() + return opts is not None and opts._for_refresh_state + @property def is_relationship_load(self): """Return True if this load is loading objects on behalf of a @@ -317,7 +346,19 @@ class ORMExecuteState(util.MemoizedSlots): SELECT statement being emitted is on behalf of a relationship load. + Handlers will very likely not want to add any options to queries + when such an operation is occurring, as loader options are already + capable of being propigated to relationship loaders and should + be already present. + + .. seealso:: + + :attr:`_orm.ORMExecuteState.is_column_load` + """ + opts = self._orm_compile_options() + if opts is None: + return False path = self.loader_strategy_path return path is not None and not path.is_root diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py index 886ae9a116..c9437d1b2e 100644 --- a/lib/sqlalchemy/orm/util.py +++ b/lib/sqlalchemy/orm/util.py @@ -935,17 +935,38 @@ class LoaderCriteriaOption(CriteriaOption): @event.listens_for("do_orm_execute", session) def _add_filtering_criteria(execute_state): - execute_state.statement = execute_state.statement.options( - with_loader_criteria( - SecurityRole, - lambda cls: cls.role.in_(['some_role']), - include_aliases=True - ) - ) - The given class will expand to include all mapped subclass and - need not itself be a mapped class. + if ( + execute_state.is_select + and not execute_state.is_column_load + and not execute_state.is_relationship_load + ): + execute_state.statement = execute_state.statement.options( + with_loader_criteria( + SecurityRole, + lambda cls: cls.role.in_(['some_role']), + include_aliases=True + ) + ) + In the above example, the :meth:`_orm.SessionEvents.do_orm_execute` + event will intercept all queries emitted using the + :class:`_orm.Session`. For those queries which are SELECT statements + and are not attribute or relationship loads a custom + :func:`_orm.with_loader_criteria` option is added to the query. The + :func:`_orm.with_loader_criteria` option will be used in the given + statement and will also be automatically propagated to all relationship + loads that descend from this query. + + The criteria argument given is a ``lambda`` that accepts a ``cls`` + argument. The given class will expand to include all mapped subclass + and need not itself be a mapped class. + + .. warning:: The use of a lambda inside of the call to + :func:`_orm.with_loader_criteria` is only invoked **once per unique + class**. Custom functions should not be invoked within this lambda. + See :ref:`engine_lambda_caching` for an overview of the "lambda SQL" + feature, which is for advanced use only. :param entity_or_base: a mapped class, or a class that is a super class of a particular set of mapped classes, to which the rule @@ -1028,7 +1049,8 @@ class LoaderCriteriaOption(CriteriaOption): # if options to limit the criteria to immediate query only, # use compile_state.attributes instead - self.get_global_criteria(compile_state.global_attributes) + if not compile_state.compile_options._for_refresh_state: + self.get_global_criteria(compile_state.global_attributes) def get_global_criteria(self, attributes): for mp in self._all_mappers(): diff --git a/test/orm/test_events.py b/test/orm/test_events.py index f8600894fd..bc72d2f213 100644 --- a/test/orm/test_events.py +++ b/test/orm/test_events.py @@ -1,4 +1,5 @@ import sqlalchemy as sa +from sqlalchemy import delete from sqlalchemy import event from sqlalchemy import ForeignKey from sqlalchemy import Integer @@ -6,6 +7,7 @@ from sqlalchemy import literal_column from sqlalchemy import select from sqlalchemy import String from sqlalchemy import testing +from sqlalchemy import update from sqlalchemy.ext.declarative import declarative_base from sqlalchemy.orm import attributes from sqlalchemy.orm import class_mapper @@ -166,6 +168,97 @@ class ORMExecuteTest(_RemoveListeners, _fixtures.FixtureTest): }, ) + def test_flags(self): + User, Address = self.classes("User", "Address") + + sess = Session(testing.db, future=True) + + canary = Mock() + + @event.listens_for(sess, "do_orm_execute") + def do_orm_execute(ctx): + + if not ctx.is_select: + assert_raises_message( + sa.exc.InvalidRequestError, + "This ORM execution is not against a SELECT statement", + lambda: ctx.lazy_loaded_from, + ) + + canary.options( + is_select=ctx.is_select, + is_update=ctx.is_update, + is_delete=ctx.is_delete, + is_orm_statement=ctx.is_orm_statement, + is_relationship_load=ctx.is_relationship_load, + is_column_load=ctx.is_column_load, + lazy_loaded_from=ctx.lazy_loaded_from + if ctx.is_select + else None, + ) + + u1 = sess.execute(select(User).filter_by(id=7)).scalar_one() + + u1.addresses + + sess.expire(u1) + + eq_(u1.name, "jack") + + sess.execute(delete(User).filter_by(id=18)) + sess.execute(update(User).filter_by(id=18).values(name="eighteen")) + + eq_( + canary.mock_calls, + [ + call.options( + is_select=True, + is_update=False, + is_delete=False, + is_orm_statement=True, + is_relationship_load=False, + is_column_load=False, + lazy_loaded_from=None, + ), + call.options( + is_select=True, + is_update=False, + is_delete=False, + is_orm_statement=True, + is_relationship_load=False, + is_column_load=False, + lazy_loaded_from=u1._sa_instance_state, + ), + call.options( + is_select=True, + is_update=False, + is_delete=False, + is_orm_statement=True, + is_relationship_load=False, + is_column_load=True, + lazy_loaded_from=None, + ), + call.options( + is_select=False, + is_update=False, + is_delete=True, + is_orm_statement=True, + is_relationship_load=False, + is_column_load=False, + lazy_loaded_from=None, + ), + call.options( + is_select=False, + is_update=True, + is_delete=False, + is_orm_statement=True, + is_relationship_load=False, + is_column_load=False, + lazy_loaded_from=None, + ), + ], + ) + def test_chained_events_two(self): sess = Session(testing.db, future=True) diff --git a/test/orm/test_relationship_criteria.py b/test/orm/test_relationship_criteria.py index 7237dd264f..87589d3be1 100644 --- a/test/orm/test_relationship_criteria.py +++ b/test/orm/test_relationship_criteria.py @@ -12,6 +12,7 @@ from sqlalchemy import sql from sqlalchemy import String from sqlalchemy import testing from sqlalchemy.orm import aliased +from sqlalchemy.orm import defer from sqlalchemy.orm import joinedload from sqlalchemy.orm import lazyload from sqlalchemy.orm import mapper @@ -597,6 +598,53 @@ class LoaderCriteriaTest(_Fixtures, testing.AssertsCompiledSQL): eq_(s.execute(stmt).scalars().all(), [UserWFoob(name=name)]) + def test_never_for_refresh(self, user_address_fixture): + User, Address = user_address_fixture + + s = Session(testing.db) + u1 = s.get(User, 8) + + @event.listens_for(s, "do_orm_execute") + def add_criteria(orm_context): + orm_context.statement = orm_context.statement.options( + with_loader_criteria(User, User.id != 8) + ) + + s.refresh(u1) + eq_(u1.name, "ed") + + def test_never_for_unexpire(self, user_address_fixture): + User, Address = user_address_fixture + + s = Session(testing.db) + u1 = s.get(User, 8) + + s.expire(u1) + + @event.listens_for(s, "do_orm_execute") + def add_criteria(orm_context): + orm_context.statement = orm_context.statement.options( + with_loader_criteria(User, User.id != 8) + ) + + eq_(u1.name, "ed") + + def test_never_for_undefer(self, user_address_fixture): + User, Address = user_address_fixture + + s = Session(testing.db) + u1 = s.execute( + select(User).options(defer(User.name)).filter(User.id == 8) + ).scalar_one() + + @event.listens_for(s, "do_orm_execute") + def add_criteria(orm_context): + orm_context.statement = orm_context.statement.options( + with_loader_criteria(User, User.id != 8) + ) + + eq_(u1.name, "ed") + class TemporalFixtureTest(testing.fixtures.DeclarativeMappedTest): @classmethod