- refactor a bit the loader options system to make it a bit more

intelligible, given the fixes for ref #3623.  unfortunately the system
is still quite weird even though it was rewritten to be... less weird

1 files changed, 38 insertions, 31 deletions

diff --git a/lib/sqlalchemy/orm/strategy_options.py b/lib/sqlalchemy/orm/strategy_options.py
index f08367941..aa818258a 100644
--- a/lib/sqlalchemy/orm/strategy_options.py
+++ b/lib/sqlalchemy/orm/strategy_options.py
@@ -80,6 +80,8 @@ class Load(Generative, MapperOption):
     def __init__(self, entity):
         insp = inspect(entity)
         self.path = insp._path_registry
+        # note that this .context is shared among all descendant
+        # Load objects
         self.context = {}
         self.local_opts = {}
 
@@ -88,7 +90,7 @@ class Load(Generative, MapperOption):
         cloned.local_opts = {}
         return cloned
 
-    _merge_into_path = False
+    is_opts_only = False
     strategy = None
     propagate_to_loaders = False
 
@@ -201,7 +203,7 @@ class Load(Generative, MapperOption):
             self._set_path_strategy()
 
     @_generative
-    def set_column_strategy(self, attrs, strategy, opts=None):
+    def set_column_strategy(self, attrs, strategy, opts=None, opts_only=False):
         strategy = self._coerce_strat(strategy)
 
         for attr in attrs:
@@ -212,21 +214,34 @@ class Load(Generative, MapperOption):
             cloned.propagate_to_loaders = True
             if opts:
                 cloned.local_opts.update(opts)
+            if opts_only:
+                cloned.is_opts_only = True
             cloned._set_path_strategy()
 
-    def _set_path_strategy(self):
-        if self._merge_into_path:
-            # special helper for undefer_group
-            existing = self.path.get(self.context, "loader")
+    def _set_for_path(self, context, path, replace=True, merge_opts=False):
+        if merge_opts or not replace:
+            existing = path.get(self.context, "loader")
+
             if existing:
-                existing.local_opts.update(self.local_opts)
+                if merge_opts:
+                    existing.local_opts.update(self.local_opts)
             else:
-                self.path.set(self.context, "loader", self)
+                path.set(context, "loader", self)
+        else:
+            existing = path.get(self.context, "loader")
+            path.set(context, "loader", self)
+            if existing and existing.is_opts_only:
+                self.local_opts.update(existing.local_opts)
 
-        elif self.path.has_entity:
-            self.path.parent.set(self.context, "loader", self)
+    def _set_path_strategy(self):
+        if self.path.has_entity:
+            effective_path = self.path.parent
         else:
-            self.path.set(self.context, "loader", self)
+            effective_path = self.path
+
+        self._set_for_path(
+            self.context, effective_path, replace=True,
+            merge_opts=self.is_opts_only)
 
     def __getstate__(self):
         d = self.__dict__.copy()
@@ -314,7 +329,7 @@ class _UnboundLoad(Load):
             val._bind_loader(query, query._attributes, raiseerr)
 
     @classmethod
-    def _from_keys(self, meth, keys, chained, kw):
+    def _from_keys(cls, meth, keys, chained, kw):
         opt = _UnboundLoad()
 
         def _split_key(key):
@@ -399,6 +414,7 @@ class _UnboundLoad(Load):
         loader = Load(path_element)
         loader.context = context
         loader.strategy = self.strategy
+        loader.is_opts_only = self.is_opts_only
 
         path = loader.path
         for token in start_path:
@@ -420,24 +436,15 @@ class _UnboundLoad(Load):
 
         if effective_path.is_token:
             for path in effective_path.generate_for_superclasses():
-                if self._merge_into_path:
-                    # special helper for undefer_group
-                    existing = path.get(context, "loader")
-                    if existing:
-                        existing.local_opts.update(self.local_opts)
-                    else:
-                        path.set(context, "loader", loader)
-                elif self._is_chain_link:
-                    path.setdefault(context, "loader", loader)
-                else:
-                    path.set(context, "loader", loader)
+                loader._set_for_path(
+                    context, path,
+                    replace=not self._is_chain_link,
+                    merge_opts=self.is_opts_only)
         else:
-            # only supported for the undefer_group() wildcard opt
-            assert not self._merge_into_path
-            if self._is_chain_link:
-                effective_path.setdefault(context, "loader", loader)
-            else:
-                effective_path.set(context, "loader", loader)
+            loader._set_for_path(
+                context, effective_path,
+                replace=not self._is_chain_link,
+                merge_opts=self.is_opts_only)
 
     def _find_entity_prop_comparator(self, query, token, mapper, raiseerr):
         if _is_aliased_class(mapper):
@@ -1043,11 +1050,11 @@ def undefer_group(loadopt, name):
         :func:`.orm.undefer`
 
     """
-    loadopt._merge_into_path = True
     return loadopt.set_column_strategy(
         "*",
         None,
-        {"undefer_group_%s" % name: True}
+        {"undefer_group_%s" % name: True},
+        opts_only=True
     )