Merge "omit_join=True is not supported"

4 files changed, 74 insertions, 1 deletions

diff --git a/doc/build/changelog/unreleased_13/4954.rst b/doc/build/changelog/unreleased_13/4954.rst
new file mode 100644
index 000000000..1c147d1dc
--- /dev/null
+++ b/doc/build/changelog/unreleased_13/4954.rst
@@ -0,0 +1,14 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 4954
+
+    The :paramref:`.relationship.omit_join` flag was not intended to be
+    manually set to True, and will now emit a warning when this occurs.  The
+    omit_join optimization is detected automatically, and the ``omit_join``
+    flag was only intended to disable the optimization in the hypothetical case
+    that the optimization may have interfered with correct results, which has
+    not been observed with the modern version of this feature.   Setting the
+    flag to True when it is not automatically detected may cause the selectin
+    load feature to not work correctly when a non-default primary join
+    condition is in use.
+
diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py
index a8013f36d..aa49b8c7c 100644
--- a/lib/sqlalchemy/orm/relationships.py
+++ b/lib/sqlalchemy/orm/relationships.py
@@ -817,7 +817,16 @@ class RelationshipProperty(StrategizedProperty):
         :param omit_join:
           Allows manual control over the "selectin" automatic join
           optimization.  Set to ``False`` to disable the "omit join" feature
-          added in SQLAlchemy 1.3.
+          added in SQLAlchemy 1.3; or leave as ``None`` to leave automatic
+          optimization in place.
+
+          .. note:: This flag may only be set to ``False``.   It is not
+             necessary to set it to ``True`` as the "omit_join" optimization is
+             automatically detected; if it is not detected, then the
+             optimization is not supported.
+
+             .. versionchanged:: 1.3.11  setting ``omit_join`` to True will now
+                emit a warning as this was not the intended use of this flag.
 
           .. versionadded:: 1.3
 
@@ -848,6 +857,15 @@ class RelationshipProperty(StrategizedProperty):
         self.doc = doc
         self.active_history = active_history
         self.join_depth = join_depth
+        if omit_join:
+            util.warn(
+                "setting omit_join to True is not supported; selectin "
+                "loading of this relationship may not work correctly if this "
+                "flag is set explicitly.  omit_join optimization is "
+                "automatically detected for conditions under which it is "
+                "supported."
+            )
+
         self.omit_join = omit_join
         self.local_remote_pairs = _local_remote_pairs
         self.bake_queries = bake_queries
diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py
index 59877a521..44ef43418 100644
--- a/lib/sqlalchemy/orm/strategies.py
+++ b/lib/sqlalchemy/orm/strategies.py
@@ -2167,6 +2167,7 @@ class SelectInLoader(PostLoader, util.MemoizedSlots):
                     use_proxies=True,
                     equivalents=self.parent._equivalent_columns,
                 )
+
         if self.omit_join:
             if is_m2o:
                 self._query_info = self._init_for_omit_join_m2o()
diff --git a/test/orm/test_selectin_relations.py b/test/orm/test_selectin_relations.py
index c38659a16..2004923b6 100644
--- a/test/orm/test_selectin_relations.py
+++ b/test/orm/test_selectin_relations.py
@@ -3108,6 +3108,7 @@ class M2OWDegradeTest(
             id = Column(Integer, primary_key=True)
             b_id = Column(ForeignKey("b.id"))
             b = relationship("B")
+            b_no_omit_join = relationship("B", omit_join=False)
             q = Column(Integer)
 
         class B(fixtures.ComparableEntity, Base):
@@ -3133,6 +3134,13 @@ class M2OWDegradeTest(
         )
         s.commit()
 
+    def test_omit_join_warn_on_true(self):
+        with testing.expect_warnings(
+            "setting omit_join to True is not supported; selectin "
+            "loading of this relationship"
+        ):
+            relationship("B", omit_join=True)
+
     def test_use_join_parent_criteria(self):
         A, B = self.classes("A", "B")
         s = Session()
@@ -3228,6 +3236,38 @@ class M2OWDegradeTest(
             ],
         )
 
+    def test_use_join_omit_join_false(self):
+        A, B = self.classes("A", "B")
+        s = Session()
+        q = s.query(A).options(selectinload(A.b_no_omit_join)).order_by(A.id)
+        results = self.assert_sql_execution(
+            testing.db,
+            q.all,
+            CompiledSQL(
+                "SELECT a.id AS a_id, a.b_id AS a_b_id, a.q AS a_q "
+                "FROM a ORDER BY a.id",
+                [{}],
+            ),
+            CompiledSQL(
+                "SELECT a_1.id AS a_1_id, b.id AS b_id, b.x AS b_x, "
+                "b.y AS b_y FROM a AS a_1 JOIN b ON b.id = a_1.b_id "
+                "WHERE a_1.id IN ([POSTCOMPILE_primary_keys]) ORDER BY a_1.id",
+                [{"primary_keys": [1, 2, 3, 4, 5]}],
+            ),
+        )
+
+        b1, b2 = B(id=1, x=5, y=9), B(id=2, x=10, y=8)
+        eq_(
+            results,
+            [
+                A(id=1, b_no_omit_join=b1),
+                A(id=2, b_no_omit_join=b2),
+                A(id=3, b_no_omit_join=b2),
+                A(id=4, b_no_omit_join=None),
+                A(id=5, b_no_omit_join=b1),
+            ],
+        )
+
     def test_use_join_parent_degrade_on_defer(self):
         A, B = self.classes("A", "B")
         s = Session()