From: Mike Bayer Date: Fri, 1 Oct 2021 22:04:46 +0000 (-0400) Subject: disallow adding to identity map that's been discarded X-Git-Tag: rel_1_4_26~38^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=e7e0757efe042b8343ef44d4f61a33cdbc072ff3;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git disallow adding to identity map that's been discarded Fixed bug where iterating a :class:`.Result` from a :class:`_orm.Session` after that :class:`_orm.Session` were closed would partially attach objects to that session in an essentially invalid state. It now raises an exception with a link to new documentation if an **un-buffered** result is iterated from a :class:`_orm.Session` that was closed or otherwise had the :meth:`_orm.Session.expunge_all` method called after that :class:`.Result` was generated. The "prebuffer_rows" execution option, as is used by the asyncio extension, may be used to produce a :class:`.Result` where the ORM objects are prebuffered, and in this case iterating the result will produce a series of detached objects. Fixes: #7128 Change-Id: I59f0ae32a83a64587937741b80f31ff825bbb574 --- diff --git a/doc/build/changelog/unreleased_14/7128.rst b/doc/build/changelog/unreleased_14/7128.rst new file mode 100644 index 0000000000..85991d21c3 --- /dev/null +++ b/doc/build/changelog/unreleased_14/7128.rst @@ -0,0 +1,18 @@ +.. change:: + :tags: bug, orm + :tickets: 7128 + + Fixed bug where iterating a :class:`.Result` from a :class:`_orm.Session` + after that :class:`_orm.Session` were closed would partially attach objects + to that session in an essentially invalid state. It now raises an exception + with a link to new documentation if an **un-buffered** result is iterated + from a :class:`_orm.Session` that was closed or otherwise had the + :meth:`_orm.Session.expunge_all` method called after that :class:`.Result` + was generated. The "prebuffer_rows" execution option, as is used by the + asyncio extension, may be used to produce a :class:`.Result` where the ORM + objects are prebuffered, and in this case iterating the result will produce + a series of detached objects. + + .. seealso:: + + :ref:`error_lkrp` \ No newline at end of file diff --git a/doc/build/errors.rst b/doc/build/errors.rst index da52623817..2b163ec269 100644 --- a/doc/build/errors.rst +++ b/doc/build/errors.rst @@ -1409,6 +1409,85 @@ items in each case:: Above, the ORM will know that the overlap between ``Parent.c1``, ``Parent.c2`` and ``Child.parent`` is intentional. +.. _error_lkrp: + +Object cannot be converted to 'persistent' state, as this identity map is no longer valid. +------------------------------------------------------------------------------------------- + +.. versionadded:: 1.4.26 + +This message was added to accommodate for the case where a +:class:`_result.Result` object that would yield ORM objects is iterated after +the originating :class:`_orm.Session` has been closed, or otherwise had its +:meth:`_orm.Session.expunge_all` method called. When a :class:`_orm.Session` +expunges all objects at once, the internal :term:`identity map` used by that +:class:`_orm.Session` is replaced with a new one, and the original one +discarded. An unconsumed and unbuffered :class:`_result.Result` object will +internally maintain a reference to that now-discarded identity map. Therefore, +when the :class:`_result.Result` is consumed, the objects that would be yielded +cannot be associated with that :class:`_orm.Session`. This arrangement is by +design as it is generally not recommended to iterate an unbuffered +:class:`_result.Result` object outside of the transactional context in which it +was created:: + + # context manager creates new Session + with Session(engine) as session_obj: + result = sess.execute(select(User).where(User.id == 7)) + + # context manager is closed, so session_obj above is closed, identity + # map is replaced + + # iterating the result object can't associate the object with the + # Session, raises this error. + user = result.first() + +The above situation typically will **not** occur when using the ``asyncio`` +ORM extension, as when :class:`.AsyncSession` returns a sync-style +:class:`_result.Result`, the results have been pre-buffered when the statement +was executed. This is to allow secondary eager loaders to invoke without needing +an additional ``await`` call. + +To pre-buffer results in the above situation using the regular +:class:`_orm.Session` in the same way that the ``asyncio`` extension does it, +the ``prebuffer_rows`` execution option may be used as follows:: + + # context manager creates new Session + with Session(engine) as session_obj: + + # result internally pre-fetches all objects + result = sess.execute( + select(User).where(User.id == 7), + execution_options={"prebuffer_rows": True} + ) + + # context manager is closed, so session_obj above is closed, identity + # map is replaced + + # pre-buffered objects are returned + user = result.first() + + # however they are detached from the session, which has been closed + assert inspect(user).detached + assert inspect(user).session is None + +Above, the selected ORM objects are fully generated within the ``session_obj`` +block, associated with ``session_obj`` and buffered within the +:class:`_result.Result` object for iteration. Outside the block, +``session_obj`` is closed and expunges these ORM objects. Iterating the +:class:`_result.Result` object will yield those ORM objects, however as their +originating :class:`_orm.Session` has expunged them, they will be delivered in +the :term:`detached` state. + +.. note:: The above reference to a "pre-buffered" vs. "un-buffered" + :class:`_result.Result` object refers to the process by which the ORM + converts incoming raw database rows from the :term:`DBAPI` into ORM + objects. It does not imply whether or not the underyling ``cursor`` + object itself, which represents pending results from the DBAPI, is itself + buffered or unbuffered, as this is essentially a lower layer of buffering. + For background on buffering of the ``cursor`` results itself, see the + section :ref:`engine_stream_results`. + + AsyncIO Exceptions ================== diff --git a/lib/sqlalchemy/orm/identity.py b/lib/sqlalchemy/orm/identity.py index 7eec4fd8d8..6aea0d1854 100644 --- a/lib/sqlalchemy/orm/identity.py +++ b/lib/sqlalchemy/orm/identity.py @@ -18,6 +18,9 @@ class IdentityMap(object): self._modified = set() self._wr = weakref.ref(self) + def _kill(self): + self._add_unpresent = _killed + def keys(self): return self._dict.keys() @@ -238,3 +241,14 @@ class WeakInstanceDict(IdentityMap): if st is state: self._dict.pop(state.key, None) self._manage_removed_state(state) + + +def _killed(state, key): + # external function to avoid creating cycles when assigned to + # the IdentityMap + raise sa_exc.InvalidRequestError( + "Object %s cannot be converted to 'persistent' state, as this " + "identity map is no longer valid. Has the owning Session " + "been closed?" % orm_util.state_str(state), + code="lkrp", + ) diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index f051d8df2c..d5fb8a8e1b 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -1836,6 +1836,7 @@ class Session(_SessionClassMethods): """ all_states = self.identity_map.all_states() + list(self._new) + self.identity_map._kill() self.identity_map = identity.WeakInstanceDict() self._new = {} self._deleted = {} diff --git a/test/orm/test_session.py b/test/orm/test_session.py index efe10f4ccf..afd9ca8159 100644 --- a/test/orm/test_session.py +++ b/test/orm/test_session.py @@ -28,8 +28,10 @@ from sqlalchemy.testing import assertions from sqlalchemy.testing import config from sqlalchemy.testing import engines from sqlalchemy.testing import eq_ +from sqlalchemy.testing import expect_raises_message from sqlalchemy.testing import fixtures from sqlalchemy.testing import is_ +from sqlalchemy.testing import is_false from sqlalchemy.testing import is_not from sqlalchemy.testing import is_true from sqlalchemy.testing import mock @@ -2176,6 +2178,81 @@ class SessionInterface(fixtures.MappedTest): ) +class NewStyleExecutionTest(_fixtures.FixtureTest): + run_setup_mappers = "once" + run_inserts = "once" + run_deletes = None + + @classmethod + def setup_mappers(cls): + cls._setup_stock_mapping() + + @testing.combinations(("close",), ("expunge_all",)) + def test_unbuffered_result_session_is_closed(self, meth): + """test #7128""" + User = self.classes.User + + sess = fixture_session() + + result = sess.execute(select(User)) + + # close or expunge_all + getattr(sess, meth)() + + with expect_raises_message( + sa.exc.InvalidRequestError, + "Object .*User.* cannot be converted to 'persistent' state, " + "as this identity map is no longer valid.", + ): + result.all() + + @testing.combinations((True,), (False,), argnames="prebuffered") + @testing.combinations(("close",), ("expunge_all",), argnames="meth") + def test_unbuffered_result_before_session_is_closed( + self, prebuffered, meth + ): + """test #7128""" + User = self.classes.User + + sess = fixture_session() + + if prebuffered: + result = sess.execute( + select(User), execution_options={"prebuffer_rows": True} + ) + else: + result = sess.execute(select(User)) + u1 = result.scalars().all()[0] + + is_true(inspect(u1).persistent) + is_false(inspect(u1).detached) + is_(inspect(u1).session, sess) + + # close or expunge_all + getattr(sess, meth)() + + is_true(inspect(u1).detached) + is_(inspect(u1).session, None) + + @testing.combinations(("close",), ("expunge_all",)) + def test_prebuffered_result_session_is_closed(self, meth): + """test #7128""" + User = self.classes.User + + sess = fixture_session() + + result = sess.execute( + select(User), execution_options={"prebuffer_rows": True} + ) + # close or expunge_all + getattr(sess, meth)() + + u1 = result.scalars().all()[0] + + is_true(inspect(u1).detached) + is_(inspect(u1).session, None) + + class FlushWarningsTest(fixtures.MappedTest): run_setup_mappers = "each"