From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Wed, 26 Apr 2017 22:50:05 +0000 (-0400)
Subject: Run eager loaders on unexpire
X-Git-Tag: rel_1_4_0b1~748
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2fc7078a08db057ea7e43991205aaee5562d7fd3;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Run eager loaders on unexpire

Eager loaders, such as joined loading, SELECT IN loading, etc., when
configured on a mapper or via query options will now be invoked during
the refresh on an expired object; in the case of selectinload and
subqueryload, since the additional load is for a single object only,
the "immediateload" scheme is used in these cases which resembles the
single-parent query emitted by lazy loading.

Change-Id: I7ca2c77bff58dc21015d60093a88c387937376b2
Fixes: #1763
---

diff --git a/doc/build/changelog/migration_14.rst b/doc/build/changelog/migration_14.rst
index 1fa89c2aec..5160a881ba 100644
--- a/doc/build/changelog/migration_14.rst
+++ b/doc/build/changelog/migration_14.rst
@@ -188,6 +188,67 @@ refined so that it is more compatible with Core.
 Behavioral Changes - ORM
 ========================
 
+.. _change_1763:
+
+Eager loaders emit during unexpire operations
+---------------------------------------------
+
+A long sought behavior was that when an expired object is accessed, configured
+eager loaders will run in order to eagerly load relationships on the expired
+object when the object is refreshed or otherwise unexpired.   This behavior has
+now been added, so that joinedloaders will add inline JOINs as usual, and
+selectin/subquery loaders will run an "immediateload" operation for a given
+relationship, when an expired object is unexpired or an object is refreshed::
+
+    >>> a1 = session.query(A).options(joinedload(A.bs)).first()
+    >>> a1.data = 'new data'
+    >>> session.commit()
+
+Above, the ``A`` object was loaded with a ``joinedload()`` option associated
+with it in order to eagerly load the ``bs`` collection.    After the
+``session.commit()``, the state of the object is expired.  Upon accessing
+the ``.data`` column attribute, the object is refreshed and this will now
+include the joinedload operation as well::
+
+    >>> a1.data
+    SELECT a.id AS a_id, a.data AS a_data, b_1.id AS b_1_id, b_1.a_id AS b_1_a_id
+    FROM a LEFT OUTER JOIN b AS b_1 ON a.id = b_1.a_id
+    WHERE a.id = ?
+
+The behavior applies both to loader strategies applied to the
+:func:`.relationship` directly, as well as with options used with
+:meth:`.Query.options`, provided that the object was originally loaded by that
+query.
+
+For the "secondary" eager loaders "selectinload" and "subqueryload", the SQL
+strategy for these loaders is not necessary in order to eagerly load attributes
+on a single object; so they will instead invoke the "immediateload" strategy in
+a refresh scenario, which resembles the query emitted by "lazyload", emitted as
+an additional query::
+
+    >>> a1 = session.query(A).options(selectinload(A.bs)).first()
+    >>> a1.data = 'new data'
+    >>> session.commit()
+    >>> a1.data
+    SELECT a.id AS a_id, a.data AS a_data
+    FROM a
+    WHERE a.id = ?
+    (1,)
+    SELECT b.id AS b_id, b.a_id AS b_a_id
+    FROM b
+    WHERE ? = b.a_id
+    (1,)
+
+Note that a loader option does not apply to an object that was introduced
+into the :class:`.Session` in a different way.  That is, if the ``a1`` object
+were just persisted in this :class:`.Session`, or was loaded with a different
+query before the eager option had been applied, then the object doesn't have
+an eager load option associated with it.  This is not a new concept, however
+users who are looking for the eagerload on refresh behavior may find this
+to be more noticeable.
+
+:ticket:`1763`
+
 .. _change_4519:
 
 Accessing an uninitialized collection attribute on a transient object no longer mutates __dict__
diff --git a/doc/build/changelog/unreleased_14/1763.rst b/doc/build/changelog/unreleased_14/1763.rst
new file mode 100644
index 0000000000..18ec01584c
--- /dev/null
+++ b/doc/build/changelog/unreleased_14/1763.rst
@@ -0,0 +1,14 @@
+.. change::
+    :tags: feature, orm
+    :tickets: 1763
+
+    Eager loaders, such as joined loading, SELECT IN loading, etc., when
+    configured on a mapper or via query options will now be invoked during
+    the refresh on an expired object; in the case of selectinload and
+    subqueryload, since the additional load is for a single object only,
+    the "immediateload" scheme is used in these cases which resembles the
+    single-parent query emitted by lazy loading.
+
+    .. seealso::
+
+        :ref:`change_1763`
diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py
index 2f54fcd329..d50eab03ec 100644
--- a/lib/sqlalchemy/orm/attributes.py
+++ b/lib/sqlalchemy/orm/attributes.py
@@ -702,7 +702,8 @@ class AttributeImpl(object):
                 if not passive & CALLABLES_OK:
                     return PASSIVE_NO_RESULT
 
-                if key in state.expired_attributes:
+                if self.accepts_scalar_loader and \
+                        key in state.expired_attributes:
                     value = state._load_expired(state, passive)
                 elif key in state.callables:
                     callable_ = state.callables[key]
diff --git a/lib/sqlalchemy/orm/instrumentation.py b/lib/sqlalchemy/orm/instrumentation.py
index ee0cc06006..61184ee0a0 100644
--- a/lib/sqlalchemy/orm/instrumentation.py
+++ b/lib/sqlalchemy/orm/instrumentation.py
@@ -123,6 +123,10 @@ class ClassManager(dict):
             ]
         )
 
+    @_memoized_key_collection
+    def _loader_impls(self):
+        return frozenset([attr.impl for attr in self.values()])
+
     @util.memoized_property
     def mapper(self):
         # raises unless self.mapper has been assigned
diff --git a/lib/sqlalchemy/orm/loading.py b/lib/sqlalchemy/orm/loading.py
index fd283f4327..f07067d17d 100644
--- a/lib/sqlalchemy/orm/loading.py
+++ b/lib/sqlalchemy/orm/loading.py
@@ -269,6 +269,10 @@ def load_on_pk_identity(
     else:
         version_check = False
 
+    if refresh_state and refresh_state.load_options:
+        q = q._with_current_path(refresh_state.load_path.parent)
+        q = q._conditional_options(refresh_state.load_options)
+
     q._get_options(
         populate_existing=bool(refresh_state),
         version_check=version_check,
diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index 5e8d25647b..0c4e1543bb 100644
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -2812,11 +2812,14 @@ class Mapper(InspectionAttr):
         """
         props = self._props
 
+        col_attribute_names = set(attribute_names).intersection(
+            state.mapper.column_attrs.keys()
+        )
         tables = set(
             chain(
                 *[
                     sql_util.find_tables(c, check_columns=True)
-                    for key in attribute_names
+                    for key in col_attribute_names
                     for c in props[key].columns
                 ]
             )
@@ -2884,7 +2887,7 @@ class Mapper(InspectionAttr):
         cond = sql.and_(*allconds)
 
         cols = []
-        for key in attribute_names:
+        for key in col_attribute_names:
             cols.extend(props[key].columns)
         return sql.select(cols, cond, use_labels=True)
 
diff --git a/lib/sqlalchemy/orm/state.py b/lib/sqlalchemy/orm/state.py
index f6c06acc82..ead9bf2bbe 100644
--- a/lib/sqlalchemy/orm/state.py
+++ b/lib/sqlalchemy/orm/state.py
@@ -595,12 +595,23 @@ class InstanceState(interfaces.InspectionAttrInfo):
         self.expired_attributes.update(
             [
                 impl.key
-                for impl in self.manager._scalar_loader_impls
+                for impl in self.manager._loader_impls
                 if impl.expire_missing or impl.key in dict_
             ]
         )
 
         if self.callables:
+            # the per state loader callables we can remove here are
+            # LoadDeferredColumns, which undefers a column at the instance
+            # level that is mapped with deferred, and LoadLazyAttribute,
+            # which lazy loads a relationship at the instance level that
+            # is mapped with "noload" or perhaps "immediateload".
+            # Before 1.4, only column-based
+            # attributes could be considered to be "expired", so here they
+            # were the only ones "unexpired", which means to make them deferred
+            # again.   For the moment, as of 1.4 we also apply the same
+            # treatment relationships now, that is, an instance level lazy
+            # loader is reset in the same way as a column loader.
             for k in self.expired_attributes.intersection(self.callables):
                 del self.callables[k]
 
diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py
index fc86076b1f..49b5b4f64a 100644
--- a/lib/sqlalchemy/orm/strategies.py
+++ b/lib/sqlalchemy/orm/strategies.py
@@ -702,6 +702,9 @@ class LazyLoader(AbstractRelationshipLoader, util.MemoizedSlots):
             if _none_set.issuperset(primary_key_identity):
                 return None
 
+            if self.key in state.dict:
+                return attributes.ATTR_WAS_SET
+
             # look for this identity in the identity map.  Delegate to the
             # Query class in use, as it may have special rules for how it
             # does this, including how it decides what the correct
@@ -841,6 +844,8 @@ class LazyLoader(AbstractRelationshipLoader, util.MemoizedSlots):
                 )
 
         lazy_clause, params = self._generate_lazy_clause(state, passive)
+        if self.key in state.dict:
+            return attributes.ATTR_WAS_SET
 
         if pending:
             if util.has_intersection(orm_util._none_set, params.values()):
@@ -934,8 +939,21 @@ class LoadLazyAttribute(object):
         return strategy._load_for_state(state, passive)
 
 
+class PostLoader(AbstractRelationshipLoader):
+    """A relationship loader that emits a second SELECT statement."""
+
+    def _immediateload_create_row_processor(
+        self, context, path, loadopt, mapper, result, adapter, populators
+    ):
+        return self.parent_property._get_strategy(
+            (("lazy", "immediate"),)
+        ).create_row_processor(
+            context, path, loadopt, mapper, result, adapter, populators
+        )
+
+
 @properties.RelationshipProperty.strategy_for(lazy="immediate")
-class ImmediateLoader(AbstractRelationshipLoader):
+class ImmediateLoader(PostLoader):
     __slots__ = ()
 
     def init_class_attribute(self, mapper):
@@ -967,7 +985,7 @@ class ImmediateLoader(AbstractRelationshipLoader):
 
 @log.class_logger
 @properties.RelationshipProperty.strategy_for(lazy="subquery")
-class SubqueryLoader(AbstractRelationshipLoader):
+class SubqueryLoader(PostLoader):
     __slots__ = ("join_depth",)
 
     def __init__(self, parent, strategy_key):
@@ -991,7 +1009,7 @@ class SubqueryLoader(AbstractRelationshipLoader):
         **kwargs
     ):
 
-        if not context.query._enable_eagerloads:
+        if not context.query._enable_eagerloads or context.refresh_state:
             return
         elif context.query._yield_per:
             context.query._no_yield_per("subquery")
@@ -1320,6 +1338,11 @@ class SubqueryLoader(AbstractRelationshipLoader):
     def create_row_processor(
         self, context, path, loadopt, mapper, result, adapter, populators
     ):
+        if context.refresh_state:
+            return self._immediateload_create_row_processor(
+                context, path, loadopt, mapper, result, adapter, populators
+            )
+
         if not self.parent.class_manager[self.key].impl.supports_population:
             raise sa_exc.InvalidRequestError(
                 "'%s' does not support object "
@@ -2066,7 +2089,7 @@ class JoinedLoader(AbstractRelationshipLoader):
 
 @log.class_logger
 @properties.RelationshipProperty.strategy_for(lazy="selectin")
-class SelectInLoader(AbstractRelationshipLoader, util.MemoizedSlots):
+class SelectInLoader(PostLoader, util.MemoizedSlots):
     __slots__ = (
         "join_depth",
         "omit_join",
@@ -2182,6 +2205,11 @@ class SelectInLoader(AbstractRelationshipLoader, util.MemoizedSlots):
     def create_row_processor(
         self, context, path, loadopt, mapper, result, adapter, populators
     ):
+        if context.refresh_state:
+            return self._immediateload_create_row_processor(
+                context, path, loadopt, mapper, result, adapter, populators
+            )
+
         if not self.parent.class_manager[self.key].impl.supports_population:
             raise sa_exc.InvalidRequestError(
                 "'%s' does not support object "
diff --git a/test/orm/test_eager_relations.py b/test/orm/test_eager_relations.py
index 70ca993b79..7b51f1d823 100644
--- a/test/orm/test_eager_relations.py
+++ b/test/orm/test_eager_relations.py
@@ -3466,7 +3466,7 @@ class LoadOnExistingTest(_fixtures.FixtureTest):
         sess = Session(autoflush=False)
         return User, Address, sess
 
-    def test_no_query_on_refresh(self):
+    def test_runs_query_on_refresh(self):
         User, Address, sess = self._eager_config_fixture()
 
         u1 = sess.query(User).get(8)
@@ -3477,7 +3477,8 @@ class LoadOnExistingTest(_fixtures.FixtureTest):
             eq_(u1.id, 8)
 
         self.assert_sql_count(testing.db, go, 1)
-        assert "addresses" not in u1.__dict__
+
+        assert 'addresses' in u1.__dict__
 
     def test_populate_existing_propagate(self):
         # both SelectInLoader and SubqueryLoader receive the loaded collection
diff --git a/test/orm/test_expire.py b/test/orm/test_expire.py
index ed4950a06c..6c61e9c5d1 100644
--- a/test/orm/test_expire.py
+++ b/test/orm/test_expire.py
@@ -12,6 +12,7 @@ from sqlalchemy.orm import create_session
 from sqlalchemy.orm import defer
 from sqlalchemy.orm import deferred
 from sqlalchemy.orm import exc as orm_exc
+from sqlalchemy.orm import joinedload
 from sqlalchemy.orm import lazyload
 from sqlalchemy.orm import make_transient_to_detached
 from sqlalchemy.orm import mapper
@@ -616,9 +617,9 @@ class ExpireTest(_fixtures.FixtureTest):
             assert u.addresses[0].email_address == "jack@bean.com"
             assert u.name == "jack"
 
-        # two loads, since relationship() + scalar are
-        # separate right now on per-attribute load
-        self.assert_sql_count(testing.db, go, 2)
+        # one load, due to #1763 allows joinedload to
+        # take over
+        self.assert_sql_count(testing.db, go, 1)
         assert "name" in u.__dict__
         assert "addresses" in u.__dict__
 
@@ -663,7 +664,7 @@ class ExpireTest(_fixtures.FixtureTest):
         assert "name" in u.__dict__
         assert len(u.addresses) == 2
 
-    def test_joinedload_props_dontload(self):
+    def test_mapper_joinedload_props_load(self):
         users, Address, addresses, User = (
             self.tables.users,
             self.classes.Address,
@@ -671,15 +672,7 @@ class ExpireTest(_fixtures.FixtureTest):
             self.classes.User,
         )
 
-        # relationships currently have to load separately from scalar instances
-        # the use case is: expire "addresses".  then access it.  lazy load
-        # fires off to load "addresses", but needs foreign key or primary key
-        # attributes in order to lazy load; hits those attributes, such as
-        # below it hits "u.id".  "u.id" triggers full unexpire operation,
-        # joinedloads addresses since lazy='joined'. this is all within lazy
-        # load which fires unconditionally; so an unnecessary joinedload (or
-        # lazyload) was issued.  would prefer not to complicate lazyloading to
-        # "figure out" that the operation should be aborted right now.
+        # changed in #1763, eager loaders are run when we unexpire
 
         mapper(
             User,
@@ -695,10 +688,66 @@ class ExpireTest(_fixtures.FixtureTest):
         u = sess.query(User).get(8)
         sess.expire(u)
         u.id
-        assert "addresses" not in u.__dict__
+
+        assert "addresses" in u.__dict__
+        u.addresses
+        assert "addresses" in u.__dict__
+
+    def test_options_joinedload_props_load(self):
+        users, Address, addresses, User = (
+            self.tables.users,
+            self.classes.Address,
+            self.tables.addresses,
+            self.classes.User,
+        )
+
+        # changed in #1763, eager loaders are run when we unexpire
+
+        mapper(
+            User,
+            users,
+            properties={"addresses": relationship(Address, backref="user")},
+        )
+        mapper(Address, addresses)
+        sess = create_session()
+        u = sess.query(User).options(joinedload(User.addresses)).get(8)
+        sess.expire(u)
+        u.id
+        assert "addresses" in u.__dict__
         u.addresses
         assert "addresses" in u.__dict__
 
+    def test_joinedload_props_load_two(self):
+        users, Address, addresses, User = (
+            self.tables.users,
+            self.classes.Address,
+            self.tables.addresses,
+            self.classes.User,
+        )
+        mapper(
+            User,
+            users,
+            properties={
+                "addresses": relationship(
+                    Address, backref="user", lazy="joined"
+                )
+            },
+        )
+        mapper(Address, addresses)
+        sess = create_session()
+        u = sess.query(User).get(8)
+        sess.expire(u)
+
+        # here, the lazy loader will encounter the attribute already
+        # loaded when it goes to get the PK, so the loader itself
+        # needs to no longer fire off.
+        def go():
+            u.addresses
+            assert "addresses" in u.__dict__
+            assert "id" in u.__dict__
+
+        self.assert_sql_count(testing.db, go, 1)
+
     def test_expire_synonym(self):
         User, users = self.classes.User, self.tables.users
 
@@ -1180,17 +1229,14 @@ class ExpireTest(_fixtures.FixtureTest):
             attributes.instance_state(u1).callables["addresses"],
             strategies.LoadLazyAttribute,
         )
-        # expire, it stays
+        # expire, it goes away from callables as of 1.4 and is considered
+        # to be expired
         sess.expire(u1)
-        assert (
-            "addresses" not in attributes.instance_state(u1).expired_attributes
-        )
-        assert isinstance(
-            attributes.instance_state(u1).callables["addresses"],
-            strategies.LoadLazyAttribute,
-        )
 
-        # load over it.  callable goes away.
+        assert "addresses" in attributes.instance_state(u1).expired_attributes
+        assert "addresses" not in attributes.instance_state(u1).callables
+
+        # load it
         sess.query(User).first()
         assert (
             "addresses" not in attributes.instance_state(u1).expired_attributes
diff --git a/test/orm/test_selectin_relations.py b/test/orm/test_selectin_relations.py
index d15cfe531e..c9b54c49c3 100644
--- a/test/orm/test_selectin_relations.py
+++ b/test/orm/test_selectin_relations.py
@@ -1325,7 +1325,7 @@ class LoadOnExistingTest(_fixtures.FixtureTest):
         sess = Session(autoflush=False)
         return User, Address, sess
 
-    def test_no_query_on_refresh(self):
+    def test_runs_query_on_refresh(self):
         User, Address, sess = self._eager_config_fixture()
 
         u1 = sess.query(User).get(8)
@@ -1335,8 +1335,8 @@ class LoadOnExistingTest(_fixtures.FixtureTest):
         def go():
             eq_(u1.id, 8)
 
-        self.assert_sql_count(testing.db, go, 1)
-        assert "addresses" not in u1.__dict__
+        self.assert_sql_count(testing.db, go, 2)
+        assert 'addresses' in u1.__dict__
 
     def test_no_query_on_deferred(self):
         User, Address, sess = self._deferred_config_fixture()
diff --git a/test/orm/test_subquery_relations.py b/test/orm/test_subquery_relations.py
index 505debd76f..b32b6547fa 100644
--- a/test/orm/test_subquery_relations.py
+++ b/test/orm/test_subquery_relations.py
@@ -1317,7 +1317,7 @@ class LoadOnExistingTest(_fixtures.FixtureTest):
         sess = Session(autoflush=False)
         return User, Address, sess
 
-    def test_no_query_on_refresh(self):
+    def test_runs_query_on_refresh(self):
         User, Address, sess = self._eager_config_fixture()
 
         u1 = sess.query(User).get(8)
@@ -1327,8 +1327,8 @@ class LoadOnExistingTest(_fixtures.FixtureTest):
         def go():
             eq_(u1.id, 8)
 
-        self.assert_sql_count(testing.db, go, 1)
-        assert "addresses" not in u1.__dict__
+        self.assert_sql_count(testing.db, go, 2)
+        assert "addresses" in u1.__dict__
 
     def test_no_query_on_deferred(self):
         User, Address, sess = self._deferred_config_fixture()