Gave the "state" internals a good solid

cleanup with less complexity, datamembers,
method calls, blank dictionary creates.

5 files changed, 156 insertions, 137 deletions

diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py
index 54ecd73c2..57b4ee8bf 100644
--- a/lib/sqlalchemy/orm/attributes.py
+++ b/lib/sqlalchemy/orm/attributes.py
@@ -494,9 +494,6 @@ class MutableScalarAttributeImpl(ScalarAttributeImpl):
         return History.from_attribute(
             self, state, v)
 
-    def commit_to_state(self, state, dict_, dest):
-        dest[self.key] = self.copy(dict_[self.key])
-
     def check_mutable_modified(self, state, dict_):
         (added, unchanged, deleted) = self.get_history(state, dict_, passive=PASSIVE_NO_INITIALIZE)
         return bool(added or deleted)
diff --git a/lib/sqlalchemy/orm/dynamic.py b/lib/sqlalchemy/orm/dynamic.py
index 308d69fe8..2157bafc8 100644
--- a/lib/sqlalchemy/orm/dynamic.py
+++ b/lib/sqlalchemy/orm/dynamic.py
@@ -94,7 +94,11 @@ class DynamicAttributeImpl(attributes.AttributeImpl):
         if self.key not in state.committed_state:
             state.committed_state[self.key] = CollectionHistory(self, state)
 
-        state.modified_event(dict_, self, False, attributes.NEVER_SET, passive=attributes.PASSIVE_NO_INITIALIZE)
+        state.modified_event(dict_, 
+                                self, 
+                                False, 
+                                attributes.NEVER_SET, 
+                                passive=attributes.PASSIVE_NO_INITIALIZE)
 
         # this is a hack to allow the _base.ComparableEntity fixture
         # to work
diff --git a/lib/sqlalchemy/orm/interfaces.py b/lib/sqlalchemy/orm/interfaces.py
index 75f9d4438..579101f0d 100644
--- a/lib/sqlalchemy/orm/interfaces.py
+++ b/lib/sqlalchemy/orm/interfaces.py
@@ -634,9 +634,10 @@ class StrategizedProperty(MapperProperty):
     """A MapperProperty which uses selectable strategies to affect
     loading behavior.
 
-    There is a single default strategy selected by default.  Alternate
+    There is a single strategy selected by default.  Alternate
     strategies can be selected at Query time through the usage of
     ``StrategizedOption`` objects via the Query.options() method.
+    
     """
 
     def __get_context_strategy(self, context, path):
@@ -661,10 +662,12 @@ class StrategizedProperty(MapperProperty):
         return strategy
 
     def setup(self, context, entity, path, adapter, **kwargs):
-        self.__get_context_strategy(context, path + (self.key,)).setup_query(context, entity, path, adapter, **kwargs)
+        self.__get_context_strategy(context, path + (self.key,)).\
+                    setup_query(context, entity, path, adapter, **kwargs)
 
     def create_row_processor(self, context, path, mapper, row, adapter):
-        return self.__get_context_strategy(context, path + (self.key,)).create_row_processor(context, path, mapper, row, adapter)
+        return self.__get_context_strategy(context, path + (self.key,)).\
+                    create_row_processor(context, path, mapper, row, adapter)
 
     def do_init(self):
         self.__all_strategies = {}
@@ -835,7 +838,8 @@ class PropertyOption(MapperOption):
                     mappers.append(prop.parent)
                     key = prop.key
                 else:
-                    raise sa_exc.ArgumentError("mapper option expects string key or list of attributes")
+                    raise sa_exc.ArgumentError("mapper option expects string key "
+                                                "or list of attributes")
 
                 if current_path and key == current_path[1]:
                     current_path = current_path[2:]
diff --git a/lib/sqlalchemy/orm/state.py b/lib/sqlalchemy/orm/state.py
index a9494a50e..a579801f1 100644
--- a/lib/sqlalchemy/orm/state.py
+++ b/lib/sqlalchemy/orm/state.py
@@ -15,7 +15,6 @@ class InstanceState(object):
     session_id = None
     key = None
     runid = None
-    expired_attributes = EMPTY_SET
     load_options = EMPTY_SET
     load_path = ()
     insert_order = None
@@ -67,6 +66,8 @@ class InstanceState(object):
                 instance_dict.remove(self)
             except AssertionError:
                 pass
+        # remove possible cycles
+        self.__dict__.pop('callables', None)
         self.dispose()
     
     def obj(self):
@@ -140,20 +141,12 @@ class InstanceState(object):
         self.manager.events.run('on_load', instance)
     
     def __getstate__(self):
-        d = {
-            'instance':self.obj(),
-        }
+        d = {'instance':self.obj()}
 
         d.update(
             (k, self.__dict__[k]) for k in (
                 'committed_state', 'pending', 'parents', 'modified', 'expired', 
-                'callables'
-            ) if self.__dict__.get(k, False)
-        )
-        
-        d.update(
-            (k, self.__dict__[k]) for k in (
-                'key', 'load_options', 'expired_attributes', 'mutable_dict'
+                'callables', 'key', 'load_options', 'mutable_dict'
             ) if k in self.__dict__ 
         )
         if self.load_path:
@@ -179,13 +172,13 @@ class InstanceState(object):
         self.modified = state.get('modified', False)
         self.expired = state.get('expired', False)
         self.callables = state.get('callables', {})
-        
+            
         if self.modified:
             self._strong_obj = state['instance']
             
         self.__dict__.update([
             (k, state[k]) for k in (
-                'key', 'load_options', 'expired_attributes', 'mutable_dict'
+                'key', 'load_options', 'mutable_dict'
             ) if k in state 
         ])
 
@@ -193,77 +186,43 @@ class InstanceState(object):
             self.load_path = interfaces.deserialize_path(state['load_path'])
 
     def initialize(self, key):
-        self.manager.get_impl(key).initialize(self, self.dict)
-
-    def set_callable(self, key, callable_):
-        self.dict.pop(key, None)
-        self.callables[key] = callable_
-
-    def __call__(self, **kw):
-        """__call__ allows the InstanceState to act as a deferred
-        callable for loading expired attributes, which is also
-        serializable (picklable).
-
-        """
-
-        if kw.get('passive') is attributes.PASSIVE_NO_FETCH:
-            return attributes.PASSIVE_NO_RESULT
+        """Set this attribute to an empty value or collection, 
+           based on the AttributeImpl in use."""
         
-        unmodified = self.unmodified
-        class_manager = self.manager
-        class_manager.deferred_scalar_loader(self, [
-            attr.impl.key for attr in class_manager.attributes if
-                attr.impl.accepts_scalar_loader and
-                attr.impl.key in self.expired_attributes and
-                attr.impl.key in unmodified
-            ])
-        for k in self.expired_attributes:
-            self.callables.pop(k, None)
-        del self.expired_attributes
-        return ATTR_WAS_SET
-
-    @property
-    def unmodified(self):
-        """a set of keys which have no uncommitted changes"""
-        
-        return set(self.manager).difference(self.committed_state)
+        self.manager.get_impl(key).initialize(self, self.dict)
 
-    @property
-    def unloaded(self):
-        """a set of keys which do not have a loaded value.
+    def reset(self, dict_, key):
+        """Remove the given attribute and any 
+           callables associated with it."""
 
-        This includes expired attributes and any other attribute that
-        was never populated or modified.
+        dict_.pop(key, None)
+        self.callables.pop(key, None)
 
-        """
-        return set(
-            key for key in self.manager.iterkeys()
-            if key not in self.committed_state and key not in self.dict)
-    
     def expire_attribute_pre_commit(self, dict_, key):
         """a fast expire that can be called by column loaders during a load.
-        
+
         The additional bookkeeping is finished up in commit_all().
-        
+
         This method is actually called a lot with joined-table
         loading, when the second table isn't present in the result.
-        
+
         """
-        # TODO: yes, this is still a little too busy.
-        # need to more cleanly separate out handling 
-        # for the various AttributeImpls and the contracts 
-        # they wish to maintain with their strategies
-        if not self.expired_attributes:
-            self.expired_attributes = set(self.expired_attributes)
-            
         dict_.pop(key, None)
         self.callables[key] = self
-        self.expired_attributes.add(key)
-        
-    def expire_attributes(self, dict_, attribute_names, instance_dict=None):
-        if not self.expired_attributes:
-            self.expired_attributes = set(self.expired_attributes)
 
+    def set_callable(self, dict_, key, callable_):
+        """Remove the given attribute and set the given callable
+           as a loader."""
+           
+        dict_.pop(key, None)
+        self.callables[key] = callable_
+    
+    def expire_attributes(self, dict_, attribute_names, instance_dict=None):
+        """Expire all or a group of attributes.
+        
+        If all attributes are expired, the "expired" flag is set to True.
+        
+        """
         if attribute_names is None:
             attribute_names = self.manager.keys()
             self.expired = True
@@ -274,31 +233,82 @@ class InstanceState(object):
                         instance_dict._modified.discard(self)
                 else:
                     instance_dict._modified.discard(self)
-                    
+
             self.modified = False
             filter_deferred = True
         else:
             filter_deferred = False
+
+        to_clear = (
+            self.__dict__.get('pending', None),
+            self.__dict__.get('committed_state', None),
+            self.mutable_dict
+        )
         
         for key in attribute_names:
             impl = self.manager[key].impl
-            if not filter_deferred or \
-                impl.expire_missing or \
-                key in dict_:
-                self.expired_attributes.add(key)
-                if impl.accepts_scalar_loader:
-                    self.callables[key] = self
+            if impl.accepts_scalar_loader and \
+                (not filter_deferred or impl.expire_missing or key in dict_):
+                self.callables[key] = self
             dict_.pop(key, None)
-            self.pending.pop(key, None)
-            self.committed_state.pop(key, None)
-            if self.mutable_dict:
-                self.mutable_dict.pop(key, None)
-                
-    def reset(self, key, dict_):
-        """remove the given attribute and any callables associated with it."""
+            
+            for d in to_clear:
+                if d is not None:
+                    d.pop(key, None)
 
-        dict_.pop(key, None)
-        self.callables.pop(key, None)
+    def __call__(self, **kw):
+        """__call__ allows the InstanceState to act as a deferred
+        callable for loading expired attributes, which is also
+        serializable (picklable).
+
+        """
+
+        if kw.get('passive') is attributes.PASSIVE_NO_FETCH:
+            return attributes.PASSIVE_NO_RESULT
+        
+        toload = self.expired_attributes.\
+                        intersection(self.unmodified)
+        
+        self.manager.deferred_scalar_loader(self, toload)
+
+        # if the loader failed, or this 
+        # instance state didn't have an identity,
+        # the attributes still might be in the callables
+        # dict.  ensure they are removed.
+        for k in toload.intersection(self.callables):
+            del self.callables[k]
+            
+        return ATTR_WAS_SET
+
+    @property
+    def unmodified(self):
+        """Return the set of keys which have no uncommitted changes"""
+        
+        return set(self.manager).difference(self.committed_state)
+
+    @property
+    def unloaded(self):
+        """Return the set of keys which do not have a loaded value.
+
+        This includes expired attributes and any other attribute that
+        was never populated or modified.
+
+        """
+        return set(self.manager).\
+                    difference(self.committed_state).\
+                    difference(self.dict)
+
+    @property
+    def expired_attributes(self):
+        """Return the set of keys which are 'expired' to be loaded by
+           the manager's deferred scalar loader, assuming no pending 
+           changes.  
+           
+           see also the ``unmodified`` collection which is intersected
+           against this set when a refresh operation occurs.
+           
+        """
+        return set([k for k, v in self.callables.items() if v is self])
 
     def _instance_dict(self):
         return None
@@ -345,17 +355,17 @@ class InstanceState(object):
         class_manager = self.manager
         for key in keys:
             if key in dict_ and key in class_manager.mutable_attributes:
-                class_manager[key].impl.commit_to_state(self, dict_, self.committed_state)
+                self.committed_state[key] = self.manager[key].impl.copy(dict_[key])
             else:
                 self.committed_state.pop(key, None)
-
+        
         self.expired = False
-        # unexpire attributes which have loaded
-        for key in self.expired_attributes.intersection(keys):
-            if key in dict_:
-                self.expired_attributes.remove(key)
-                self.callables.pop(key, None)
-
+        
+        for key in set(self.callables).\
+                            intersection(keys).\
+                            intersection(dict_):
+            del self.callables[key]
+    
     def commit_all(self, dict_, instance_dict=None):
         """commit all attributes unconditionally.
 
@@ -365,28 +375,29 @@ class InstanceState(object):
          - all attributes are marked as "committed"
          - the "strong dirty reference" is removed
          - the "modified" flag is set to False
-         - any "expired" markers/callables are removed.
+         - any "expired" markers/callables for attributes loaded are removed.
 
         Attributes marked as "expired" can potentially remain "expired" after this step
         if a value was not populated in state.dict.
 
         """
         
-        self.committed_state = {}
-        self.pending = {}
-            
-        if self.expired_attributes:
-            for key in self.expired_attributes.intersection(dict_):
-                self.callables.pop(key, None)
-            self.expired_attributes.difference_update(dict_)
+        self.__dict__.pop('committed_state', None)
+        self.__dict__.pop('pending', None)
+
+        if 'callables' in self.__dict__:
+            callables = self.callables
+            for key in list(callables):
+                if key in dict_ and callables[key] is self:
+                    del callables[key]
 
         for key in self.manager.mutable_attributes:
             if key in dict_:
-                self.manager[key].impl.commit_to_state(self, dict_, self.committed_state)
-
+                self.committed_state[key] = self.manager[key].impl.copy(dict_[key])
+                
         if instance_dict and self.modified:
             instance_dict._modified.discard(self)
-
+        
         self.modified = self.expired = False
         self._strong_obj = None
 
@@ -398,9 +409,10 @@ class MutableAttrInstanceState(InstanceState):
     for changes upon dereference, resurrecting if needed.
     
     """
-    def __init__(self, obj, manager):
-        self.mutable_dict = {}
-        InstanceState.__init__(self, obj, manager)
+    
+    @util.memoized_property
+    def mutable_dict(self):
+        return {}
         
     def _get_modified(self, dict_=None):
         if self.__dict__.get('modified', False):
@@ -424,11 +436,12 @@ class MutableAttrInstanceState(InstanceState):
         """a set of keys which have no uncommitted changes"""
 
         dict_ = self.dict
-        return set(
-            key for key in self.manager.iterkeys()
+        
+        return set([
+            key for key in self.manager
             if (key not in self.committed_state or
                 (key in self.manager.mutable_attributes and
-                 not self.manager[key].impl.check_mutable_modified(self, dict_))))
+                 not self.manager[key].impl.check_mutable_modified(self, dict_)))])
 
     def _is_really_none(self):
         """do a check modified/resurrect.
@@ -445,9 +458,9 @@ class MutableAttrInstanceState(InstanceState):
         else:
             return None
 
-    def reset(self, key, dict_):
+    def reset(self, dict_, key):
         self.mutable_dict.pop(key, None)
-        InstanceState.reset(self, key, dict_)
+        InstanceState.reset(self, dict_, key)
     
     def _cleanup(self, ref):
         """weakref callback.
diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py
index 4d5ec3da4..17b1fd8e8 100644
--- a/lib/sqlalchemy/orm/strategies.py
+++ b/lib/sqlalchemy/orm/strategies.py
@@ -185,6 +185,7 @@ class DeferredColumnLoader(LoaderStrategy):
         col = self.columns[0]
         if adapter:
             col = adapter.columns[col]
+            
         if col in row:
             return self.parent_property._get_strategy(ColumnLoader).\
                         create_row_processor(
@@ -192,12 +193,12 @@ class DeferredColumnLoader(LoaderStrategy):
 
         elif not self.is_class_level:
             def new_execute(state, dict_, row, isnew):
-                state.set_callable(self.key, LoadDeferredColumns(state, self.key))
+                state.set_callable(dict_, self.key, LoadDeferredColumns(state, self.key))
         else:
             def new_execute(state, dict_, row, isnew):
                 # reset state on the key so that deferred callables
                 # fire off on next access.
-                state.reset(self.key, dict_)
+                state.reset(dict_, self.key)
 
         return new_execute, None
 
@@ -223,7 +224,8 @@ class DeferredColumnLoader(LoaderStrategy):
             (self.group is not None and context.attributes.get(('undefer', self.group), False)) or \
             (only_load_props and self.key in only_load_props):
             
-            self.parent_property._get_strategy(ColumnLoader).setup_query(context, entity, path, adapter, **kwargs)
+            self.parent_property._get_strategy(ColumnLoader).\
+                            setup_query(context, entity, path, adapter, **kwargs)
     
     def _class_level_loader(self, state):
         if not mapperutil._state_has_identity(state):
@@ -303,6 +305,7 @@ class UndeferGroupOption(MapperOption):
 
     def __init__(self, group):
         self.group = group
+        
     def process_query(self, query):
         query._attributes[('undefer', self.group)] = True
 
@@ -310,14 +313,10 @@ class AbstractRelationLoader(LoaderStrategy):
     """LoaderStratgies which deal with related objects as opposed to scalars."""
 
     def init(self):
-        for attr in ['mapper', 'target', 'table', 'uselist']:
-            setattr(self, attr, getattr(self.parent_property, attr))
-        
-    def _init_instance_attribute(self, state, callable_=None):
-        if callable_:
-            state.set_callable(self.key, callable_)
-        else:
-            state.initialize(self.key)
+        self.mapper = self.parent_property.mapper
+        self.target = self.parent_property.target
+        self.table = self.parent_property.table
+        self.uselist = self.parent_property.uselist
 
 class NoLoader(AbstractRelationLoader):
     """Strategize a relation() that doesn't load data automatically."""
@@ -333,7 +332,7 @@ class NoLoader(AbstractRelationLoader):
 
     def create_row_processor(self, selectcontext, path, mapper, row, adapter):
         def new_execute(state, dict_, row, isnew):
-            self._init_instance_attribute(state)
+            state.initialize(self.key)
         return new_execute, None
 
 log.class_logger(NoLoader)
@@ -343,7 +342,9 @@ class LazyLoader(AbstractRelationLoader):
 
     def init(self):
         super(LazyLoader, self).init()
-        (self.__lazywhere, self.__bind_to_col, self._equated_columns) = self._create_lazy_clause(self.parent_property)
+        self.__lazywhere, \
+        self.__bind_to_col, \
+        self._equated_columns = self._create_lazy_clause(self.parent_property)
         
         self.logger.info("%s lazy loading clause %s", self, self.__lazywhere)
 
@@ -436,14 +437,14 @@ class LazyLoader(AbstractRelationLoader):
                 # this currently only happens when using a "lazyload" option on a "no load"
                 # attribute - "eager" attributes always have a class-level lazyloader
                 # installed.
-                self._init_instance_attribute(state, callable_=LoadLazyAttribute(state, self.key))
+                state.set_callable(dict_, self.key, LoadLazyAttribute(state, self.key))
         else:
             def new_execute(state, dict_, row, isnew):
                 # we are the primary manager for this attribute on this class - reset its
                 # per-instance attribute state, so that the class-level lazy loader is
                 # executed when next referenced on this instance.  this is needed in
                 # populate_existing() types of scenarios to reset any existing state.
-                state.reset(self.key, dict_)
+                state.reset(dict_, self.key)
 
         return new_execute, None