- Added a new option to :paramref:`.relationship.innerjoin` which is

to specify the string ``"nested"``.  When set to ``"nested"`` as opposed
to ``True``, the "chaining" of joins will parenthesize the inner join on the
right side of an existing outer join, instead of chaining as a string
of outer joins.   This possibly should have been the default behavior
when 0.9 was released, as we introduced the feature of right-nested
joins in the ORM, however we are keeping it as a non-default for now
to avoid further surprises.
fixes #2976

3 files changed, 72 insertions, 24 deletions

diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py
index 43db14e13..f0e3076ad 100644
--- a/lib/sqlalchemy/orm/relationships.py
+++ b/lib/sqlalchemy/orm/relationships.py
@@ -488,11 +488,22 @@ class RelationshipProperty(StrategizedProperty):
           or when the reference is one-to-one or a collection that is guaranteed
           to have one or at least one entry.
 
+          If the joined-eager load is chained onto an existing LEFT OUTER JOIN,
+          ``innerjoin=True`` will be bypassed and the join will continue to
+          chain as LEFT OUTER JOIN so that the results don't change.  As an alternative,
+          specify the value ``"nested"``.  This will instead nest the join
+          on the right side, e.g. using the form "a LEFT OUTER JOIN (b JOIN c)".
+
+          .. versionadded:: 0.9.4 Added ``innerjoin="nested"`` option to support
+             nesting of eager "inner" joins.
+
           .. seealso::
 
             :ref:`what_kind_of_loading` - Discussion of some details of
             various loader options.
 
+            :parmref:`.joinedload.innerjoin` - loader option version
+
         :param join_depth:
           when non-``None``, an integer value indicating how many levels
           deep "eager" loaders should join on a self-referring or cyclical
@@ -522,8 +533,8 @@ class RelationshipProperty(StrategizedProperty):
 
           * ``joined`` - items should be loaded "eagerly" in the same query as
             that of the parent, using a JOIN or LEFT OUTER JOIN.  Whether
-            the join is "outer" or not is determined by the ``innerjoin``
-            parameter.
+            the join is "outer" or not is determined by the
+            :paramref:`~.relationship.innerjoin` parameter.
 
           * ``subquery`` - items should be loaded "eagerly" as the parents are
             loaded, using one additional SQL statement, which issues a JOIN to a
diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py
index 2c18e8129..473b665c8 100644
--- a/lib/sqlalchemy/orm/strategies.py
+++ b/lib/sqlalchemy/orm/strategies.py
@@ -10,6 +10,7 @@
 from .. import exc as sa_exc, inspect
 from .. import util, log, event
 from ..sql import util as sql_util, visitors
+from .. import sql
 from . import (
         attributes, interfaces, exc as orm_exc, loading,
         unitofwork, util as orm_util
@@ -1032,7 +1033,6 @@ class JoinedLoader(AbstractRelationshipLoader):
 
     def setup_query(self, context, entity, path, loadopt, adapter, \
                                 column_collection=None, parentmapper=None,
-                                allow_innerjoin=True,
                                 **kwargs):
         """Add a left outer join to the statement thats being constructed."""
 
@@ -1062,10 +1062,9 @@ class JoinedLoader(AbstractRelationshipLoader):
                 elif path.contains_mapper(self.mapper):
                     return
 
-            clauses, adapter, add_to_collection, \
-                allow_innerjoin = self._generate_row_adapter(
+            clauses, adapter, add_to_collection = self._generate_row_adapter(
                     context, entity, path, loadopt, adapter,
-                    column_collection, parentmapper, allow_innerjoin
+                    column_collection, parentmapper
                 )
 
         with_poly_info = path.get(
@@ -1088,8 +1087,7 @@ class JoinedLoader(AbstractRelationshipLoader):
                 path,
                 clauses,
                 parentmapper=self.mapper,
-                column_collection=add_to_collection,
-                allow_innerjoin=allow_innerjoin)
+                column_collection=add_to_collection)
 
         if with_poly_info is not None and \
             None in set(context.secondary_columns):
@@ -1167,7 +1165,7 @@ class JoinedLoader(AbstractRelationshipLoader):
 
     def _generate_row_adapter(self,
         context, entity, path, loadopt, adapter,
-        column_collection, parentmapper, allow_innerjoin
+        column_collection, parentmapper
     ):
         with_poly_info = path.get(
             context.attributes,
@@ -1189,16 +1187,12 @@ class JoinedLoader(AbstractRelationshipLoader):
         if self.parent_property.direction != interfaces.MANYTOONE:
             context.multi_row_eager_loaders = True
 
-        innerjoin = allow_innerjoin and (
+        innerjoin = (
                             loadopt.local_opts.get(
                                 'innerjoin', self.parent_property.innerjoin)
                             if loadopt is not None
                             else self.parent_property.innerjoin
                         )
-        if not innerjoin:
-            # if this is an outer join, all eager joins from
-            # here must also be outer joins
-            allow_innerjoin = False
 
         context.create_eager_joins.append(
             (self._create_eager_join, context,
@@ -1209,7 +1203,7 @@ class JoinedLoader(AbstractRelationshipLoader):
         add_to_collection = context.secondary_columns
         path.set(context.attributes, "eager_row_processor", clauses)
 
-        return clauses, adapter, add_to_collection, allow_innerjoin
+        return clauses, adapter, add_to_collection
 
     def _create_eager_join(self, context, entity,
                             path, adapter, parentmapper,
@@ -1265,13 +1259,34 @@ class JoinedLoader(AbstractRelationshipLoader):
             onclause = self.parent_property
 
         assert clauses.aliased_class is not None
-        context.eager_joins[entity_key] = eagerjoin = \
-                                orm_util.join(
-                                            towrap,
-                                            clauses.aliased_class,
-                                            onclause,
-                                            isouter=not innerjoin
-                                            )
+
+        join_to_outer = innerjoin and isinstance(towrap, sql.Join) and towrap.isouter
+
+        if join_to_outer and innerjoin == 'nested':
+            inner = orm_util.join(
+                                    towrap.right,
+                                    clauses.aliased_class,
+                                    onclause,
+                                    isouter=False
+                                )
+
+            eagerjoin = orm_util.join(
+                                towrap.left,
+                                inner,
+                                towrap.onclause,
+                                isouter=True
+                        )
+            eagerjoin._target_adapter = inner._target_adapter
+        else:
+            if join_to_outer:
+                innerjoin = False
+            eagerjoin = orm_util.join(
+                                        towrap,
+                                        clauses.aliased_class,
+                                        onclause,
+                                        isouter=not innerjoin
+                                        )
+        context.eager_joins[entity_key] = eagerjoin
 
         # send a hint to the Query as to where it may "splice" this join
         eagerjoin.stop_on = entity.selectable
diff --git a/lib/sqlalchemy/orm/strategy_options.py b/lib/sqlalchemy/orm/strategy_options.py
index e290e6597..bcd7b76c6 100644
--- a/lib/sqlalchemy/orm/strategy_options.py
+++ b/lib/sqlalchemy/orm/strategy_options.py
@@ -609,11 +609,20 @@ def joinedload(loadopt, attr, innerjoin=None):
         # joined-load the keywords collection
         query(Order).options(lazyload(Order.items).joinedload(Item.keywords))
 
-    :func:`.orm.joinedload` also accepts a keyword argument `innerjoin=True` which
-    indicates using an inner join instead of an outer::
+    :param innerjoin: if ``True``, indicates that the joined eager load should
+     use an inner join instead of the default of left outer join::
 
         query(Order).options(joinedload(Order.user, innerjoin=True))
 
+     If the joined-eager load is chained onto an existing LEFT OUTER JOIN,
+     ``innerjoin=True`` will be bypassed and the join will continue to
+     chain as LEFT OUTER JOIN so that the results don't change.  As an alternative,
+     specify the value ``"nested"``.  This will instead nest the join
+     on the right side, e.g. using the form "a LEFT OUTER JOIN (b JOIN c)".
+
+     .. versionadded:: 0.9.4 Added ``innerjoin="nested"`` option to support
+        nesting of eager "inner" joins.
+
     .. note::
 
         The joins produced by :func:`.orm.joinedload` are **anonymously aliased**.
@@ -634,6 +643,11 @@ def joinedload(loadopt, attr, innerjoin=None):
 
         :func:`.orm.lazyload`
 
+        :paramref:`.relationship.lazy`
+
+        :paramref:`.relationship.innerjoin` - :func:`.relationship`-level version
+         of the :paramref:`.joinedload.innerjoin` option.
+
     """
     loader = loadopt.set_relationship_strategy(attr, {"lazy": "joined"})
     if innerjoin is not None:
@@ -680,6 +694,8 @@ def subqueryload(loadopt, attr):
 
         :func:`.orm.lazyload`
 
+        :paramref:`.relationship.lazy`
+
     """
     return loadopt.set_relationship_strategy(attr, {"lazy": "subquery"})
 
@@ -699,6 +715,10 @@ def lazyload(loadopt, attr):
     This function is part of the :class:`.Load` interface and supports
     both method-chained and standalone operation.
 
+    .. seealso::
+
+        :paramref:`.relationship.lazy`
+
     """
     return loadopt.set_relationship_strategy(attr, {"lazy": "select"})
 
@@ -726,6 +746,8 @@ def immediateload(loadopt, attr):
 
         :func:`.orm.lazyload`
 
+        :paramref:`.relationship.lazy`
+
     """
     loader = loadopt.set_relationship_strategy(attr, {"lazy": "immediate"})
     return loader