From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Thu, 23 Jun 2022 20:43:49 +0000 (-0400)
Subject: add an extra load for non-new but unloaded
X-Git-Tag: rel_2_0_0b1~164^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=00584a78486b6f80f6308976e3f846961d19deb4;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

add an extra load for non-new but unloaded

Made an improvement to the "deferred" / "load_only" set of strategy options
where if a certain object is loaded from two different logical paths within
one query, attributes that have been configured by at least one of the
options to be populated will be populated in all cases, even if other load
paths for that same object did not set this option. previously, it was
based on randomness as to which "path" addressed the object first.

Fixes: #8166
Change-Id: I923a1484721d3a04d490ef882bc9fa609c9cd077
---

diff --git a/doc/build/changelog/unreleased_20/8166.rst b/doc/build/changelog/unreleased_20/8166.rst
new file mode 100644
index 0000000000..92e6d40aff
--- /dev/null
+++ b/doc/build/changelog/unreleased_20/8166.rst
@@ -0,0 +1,10 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 8166
+
+    Made an improvement to the "deferred" / "load_only" set of strategy options
+    where if a certain object is loaded from two different logical paths within
+    one query, attributes that have been configured by at least one of the
+    options to be populated will be populated in all cases, even if other load
+    paths for that same object did not set this option. previously, it was
+    based on randomness as to which "path" addressed the object first.
diff --git a/lib/sqlalchemy/orm/loading.py b/lib/sqlalchemy/orm/loading.py
index 5d78a55804..7317d48be9 100644
--- a/lib/sqlalchemy/orm/loading.py
+++ b/lib/sqlalchemy/orm/loading.py
@@ -1238,6 +1238,12 @@ def _populate_partial(
 ):
 
     if not isnew:
+        if unloaded:
+            # extra pass, see #8166
+            for key, getter in populators["quick"]:
+                if key in unloaded:
+                    dict_[key] = getter(row)
+
         to_load = context.partials[state]
         for key, populator in populators["existing"]:
             if key in to_load:
diff --git a/test/orm/test_deferred.py b/test/orm/test_deferred.py
index ee8731c707..277ebfd882 100644
--- a/test/orm/test_deferred.py
+++ b/test/orm/test_deferred.py
@@ -37,6 +37,7 @@ from sqlalchemy.testing import AssertsCompiledSQL
 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.fixtures import fixture_session
 from sqlalchemy.testing.schema import Column
 from sqlalchemy.testing.schema import Table
@@ -1480,6 +1481,99 @@ class DeferredOptionsTest(AssertsCompiledSQL, _fixtures.FixtureTest):
         )
 
 
+@testing.combinations(
+    (
+        "order_one",
+        True,
+    ),
+    (
+        "order_two",
+        False,
+    ),
+    argnames="name, rel_ordering",
+    id_="sa",
+)
+class MultiPathTest(fixtures.DeclarativeMappedTest):
+    """test #8166"""
+
+    @classmethod
+    def setup_classes(cls):
+        Base = cls.DeclarativeBasic
+
+        class User(Base):
+            __tablename__ = "users"
+            id = Column(Integer, primary_key=True)
+            name = Column(String(10))
+            phone = Column(String(10))
+
+        class Task(Base):
+            __tablename__ = "tasks"
+            id = Column(Integer, primary_key=True)
+            name = Column(String(10))
+            created_by_id = Column(Integer, ForeignKey("users.id"))
+            managed_by_id = Column(Integer, ForeignKey("users.id"))
+
+            # reverse the order of these two in order to see it change
+            if cls.rel_ordering:
+                managed_by = relationship("User", foreign_keys=[managed_by_id])
+                created_by = relationship("User", foreign_keys=[created_by_id])
+            else:
+                created_by = relationship("User", foreign_keys=[created_by_id])
+                managed_by = relationship("User", foreign_keys=[managed_by_id])
+
+    @classmethod
+    def insert_data(cls, connection):
+        User, Task = cls.classes("User", "Task")
+
+        u1 = User(name="u1", phone="p1")
+        u2 = User(name="u2", phone="p2")
+        u3 = User(name="u3", phone="p3")
+
+        with Session(connection) as session:
+            session.add(Task(name="t1", created_by=u2, managed_by=u3))
+            session.add(Task(name="t2", created_by=u1, managed_by=u1))
+            session.commit()
+
+    def test_data_loaded(self):
+
+        User, Task = self.classes("User", "Task")
+        session = fixture_session()
+
+        all_tasks = session.query(Task).all()  # noqa: F841
+        all_users = session.query(User).all()  # noqa: F841
+
+        # expire all objects
+        session.expire_all()
+
+        # now load w/ the special paths.   User.phone needs to be
+        # undeferred
+        tasks = (
+            session.query(Task)
+            .options(
+                joinedload(Task.managed_by).load_only(User.name),
+                joinedload(Task.created_by).load_only(User.name, User.phone),
+            )
+            .all()
+        )
+
+        session.close()
+        for task in tasks:
+            if task.name == "t1":
+                # for User u2, created_by path includes User.phone
+                eq_(task.created_by.phone, "p2")
+
+                # for User u3, managed_by path does not
+                assert "phone" not in task.managed_by.__dict__
+            elif task.name == "t2":
+                # User u1 was loaded by both created_by and managed_by
+                # path, so 'phone' should be unconditionally populated
+                is_(task.created_by, task.managed_by)
+                eq_(task.created_by.phone, "p1")
+                eq_(task.managed_by.phone, "p1")
+            else:
+                assert False
+
+
 class SelfReferentialMultiPathTest(testing.fixtures.DeclarativeMappedTest):
     """test for [ticket:3822]"""