- simplified _mapper_registry further. its now just a weakkeydict of mapper->True, stores

all mappers including non primaries, and is strictly used for the list of "to compile/dispose".
- all global references are now weak referencing.  if you del a mapped class and any dependent classes,
its mapper and all dependencies fall out of scope.
- attributes.py still had issues which were barely covered by tests.  added way more tests (coverage.py still says 71%, doh)
fixed things, took out unnecessary commit to states.  attribute history is also asserted for ordering.

6 files changed, 71 insertions, 107 deletions

diff --git a/lib/sqlalchemy/orm/__init__.py b/lib/sqlalchemy/orm/__init__.py
index ac784ec08..7e8d8b8bf 100644
--- a/lib/sqlalchemy/orm/__init__.py
+++ b/lib/sqlalchemy/orm/__init__.py
@@ -592,9 +592,8 @@ def compile_mappers():
     This is equivalent to calling ``compile()`` on any individual mapper.
     """
 
-    if not _mapper_registry:
-        return
-    _mapper_registry.values()[0][0].compile()
+    for m in list(_mapper_registry):
+        m.compile()
 
 def clear_mappers():
     """Remove all mappers that have been created thus far.
@@ -604,11 +603,8 @@ def clear_mappers():
     """
     mapperlib._COMPILE_MUTEX.acquire()
     try:
-        for mapper in chain(*_mapper_registry.values()):
+        for mapper in _mapper_registry:
             mapper.dispose()
-        _mapper_registry.clear()
-        from sqlalchemy.orm import dependency
-        dependency.MapperStub.dispose(dependency.MapperStub)
     finally:
         mapperlib._COMPILE_MUTEX.release()
         
diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py
index af589f340..01be8813f 100644
--- a/lib/sqlalchemy/orm/attributes.py
+++ b/lib/sqlalchemy/orm/attributes.py
@@ -429,11 +429,9 @@ class CollectionAttributeImpl(AttributeImpl):
                 return _create_history(self, state, current)
 
     def fire_append_event(self, state, value, initiator):
-        if self.key not in state.committed_state:
-            if self.key in state.dict:
-                state.committed_state[self.key] = self.copy(state.dict[self.key])
-            else:
-                state.committed_state[self.key] = NO_VALUE
+        if self.key not in state.committed_state and self.key in state.dict:
+            state.committed_state[self.key] = self.copy(state.dict[self.key])
+
         state.modified = True
 
         if self.trackparent and value is not None:
@@ -443,18 +441,13 @@ class CollectionAttributeImpl(AttributeImpl):
             ext.append(instance, value, initiator or self)
 
     def fire_pre_remove_event(self, state, initiator):
-        if self.key not in state.committed_state:
-            if self.key in state.dict:
-                state.committed_state[self.key] = self.copy(state.dict[self.key])
-            else:
-                state.committed_state[self.key] = NO_VALUE
+        if self.key not in state.committed_state and self.key in state.dict:
+            state.committed_state[self.key] = self.copy(state.dict[self.key])
 
     def fire_remove_event(self, state, value, initiator):
-        if self.key not in state.committed_state:
-            if self.key in state.dict:
-                state.committed_state[self.key] = self.copy(state.dict[self.key])
-            else:
-                state.committed_state[self.key] = NO_VALUE
+        if self.key not in state.committed_state and self.key in state.dict:
+            state.committed_state[self.key] = self.copy(state.dict[self.key])
+                
         state.modified = True
 
         if self.trackparent and value is not None:
@@ -492,12 +485,6 @@ class CollectionAttributeImpl(AttributeImpl):
         if initiator is self:
             return
 
-        if self.key not in state.committed_state:
-            if self.key in state.dict:
-                state.committed_state[self.key] = self.copy(state.dict[self.key])
-            else:
-                state.committed_state[self.key] = NO_VALUE
-                
         collection = self.get_collection(state, passive=passive)
         if collection is PASSIVE_NORESULT:
             state.get_pending(self.key).append(value)
@@ -509,12 +496,6 @@ class CollectionAttributeImpl(AttributeImpl):
         if initiator is self:
             return
 
-        if self.key not in state.committed_state:
-            if self.key in state.dict:
-                state.committed_state[self.key] = self.copy(state.dict[self.key])
-            else:
-                state.committed_state[self.key] = NO_VALUE
-
         collection = self.get_collection(state, passive=passive)
         if collection is PASSIVE_NORESULT:
             state.get_pending(self.key).remove(value)
@@ -533,12 +514,6 @@ class CollectionAttributeImpl(AttributeImpl):
         if initiator is self:
             return
 
-        if self.key not in state.committed_state:
-            if self.key in state.dict:
-                state.committed_state[self.key] = self.copy(state.dict[self.key])
-            else:
-                state.committed_state[self.key] = NO_VALUE
-
         # we need a CollectionAdapter to adapt the incoming value to an
         # assignable iterable.  pulling a new collection first so that
         # an adaptation exception does not trigger a lazy load of the
@@ -547,6 +522,8 @@ class CollectionAttributeImpl(AttributeImpl):
         new_values = list(new_collection.adapt_like_to_iterable(value))
 
         old = self.get(state)
+        state.committed_state[self.key] = self.copy(old)
+        
         old_collection = self.get_collection(state, old)
 
         idset = util.IdentitySet
@@ -576,16 +553,30 @@ class CollectionAttributeImpl(AttributeImpl):
         """
 
         collection, user_data = self._build_collection(state)
-        self._load_collection(state, value or [], emit_events=False,
-                              collection=collection)
-        value = user_data
 
-        if state.committed_state is not None:
-            state.commit_attr(self, value)
-        # remove per-instance callable, if any
+        if value:
+            for item in value:
+                collection.append_without_event(item)
+
         state.callables.pop(self.key, None)
-        state.dict[self.key] = value
-        return value
+        state.dict[self.key] = user_data
+
+        if self.key in state.pending:
+            # pending items.  commit loaded data, add/remove new data
+            state.committed_state[self.key] = list(value or [])
+            added = state.pending[self.key].added_items
+            removed = state.pending[self.key].deleted_items
+            for item in added:
+                collection.append_without_event(item)
+            for item in removed:
+                collection.remove_without_event(item)
+            del state.pending[self.key]
+        elif self.key in state.committed_state:
+            # no pending items.  remove committed state if any.
+            # (this can occur with an expired attribute)
+            del state.committed_state[self.key]
+
+        return user_data
 
     def _build_collection(self, state):
         """build a new, blank collection and return it wrapped in a CollectionAdapter."""
@@ -594,34 +585,12 @@ class CollectionAttributeImpl(AttributeImpl):
         collection = collections.CollectionAdapter(self, state, user_data)
         return collection, user_data
 
-    def _load_collection(self, state, values, emit_events=True, collection=None):
-        """given an empty CollectionAdapter, load the collection with current values.
-        
-        Loads the collection from lazy callables in all cases.
-        """
+    def get_collection(self, state, user_data=None, passive=False):
+        """retrieve the CollectionAdapter associated with the given state.
         
-        collection = collection or self.get_collection(state)
-        if values is None:
-            return
-
-        appender = emit_events and collection.append_with_event or collection.append_without_event
+        Creates a new CollectionAdapter if one does not exist.
         
-        if self.key in state.pending:
-            # move 'pending' items into the newly loaded collection
-            added = state.pending[self.key].added_items
-            removed = state.pending[self.key].deleted_items
-            for item in values:
-                if item not in removed:
-                    appender(item)
-            for item in added:
-                appender(item)
-            del state.pending[self.key]
-        else:
-            for item in values:
-                appender(item)
-
-    def get_collection(self, state, user_data=None, passive=False):
-        """retrieve the CollectionAdapter associated with the given state."""
+        """
         
         if user_data is None:
             user_data = self.get(state, passive=passive)
diff --git a/lib/sqlalchemy/orm/dependency.py b/lib/sqlalchemy/orm/dependency.py
index 0a097fd24..8340ccdcc 100644
--- a/lib/sqlalchemy/orm/dependency.py
+++ b/lib/sqlalchemy/orm/dependency.py
@@ -481,7 +481,7 @@ class MapperStub(object):
     """
 
     __metaclass__ = util.ArgSingleton
-
+    
     def __init__(self, parent, mapper, key):
         self.mapper = mapper
         self.base_mapper = self
diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index 6ea45a598..d8c6a4023 100644
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -183,7 +183,14 @@ class Mapper(object):
                 return False
 
     def get_property(self, key, resolve_synonyms=False, raiseerr=True):
-        """return MapperProperty with the given key."""
+        """return a MapperProperty associated with the given key."""
+        
+        self.compile()
+        return self._get_property(key, resolve_synonyms=resolve_synonyms, raiseerr=raiseerr)
+
+    def _get_property(self, key, resolve_synonyms=False, raiseerr=True):
+        """private in-compilation version of get_property()."""
+        
         prop = self.__props.get(key, None)
         if resolve_synonyms:
             while isinstance(prop, SynonymProperty):
@@ -193,6 +200,7 @@ class Mapper(object):
         return prop
     
     def iterate_properties(self):
+        self.compile()
         return self.__props.itervalues()
     iterate_properties = property(iterate_properties, doc="returns an iterator of all MapperProperty objects.")
     
@@ -200,11 +208,15 @@ class Mapper(object):
         raise NotImplementedError("Public collection of MapperProperty objects is provided by the get_property() and iterate_properties accessors.")
     properties = property(properties)
     
+    compiled = property(lambda self:self.__props_init, doc="return True if this mapper is compiled")
+    
     def dispose(self):
         # disaable any attribute-based compilation
         self.__props_init = True
-        if hasattr(self.class_, 'c'):
+        try:
             del self.class_.c
+        except AttributeError:
+            pass
         if not self.non_primary and self.entity_name in self._class_state.mappers:
             del self._class_state.mappers[self.entity_name]
         if not self._class_state.mappers:
@@ -221,24 +233,16 @@ class Mapper(object):
             # double-check inside mutex
             if self.__props_init:
                 return self
+                
             # initialize properties on all mappers
-            for mapper in chain(*_mapper_registry.values()):
+            for mapper in list(_mapper_registry):
                 if not mapper.__props_init:
                     mapper.__initialize_properties()
 
-            # if we're not primary, compile us
-            if self.non_primary:
-                self.__initialize_properties()
-
             return self
         finally:
             _COMPILE_MUTEX.release()
 
-    def _check_compile(self):
-        if self.non_primary and not self.__props_init:
-            self.__initialize_properties()
-        return self
-        
     def __initialize_properties(self):
         """Call the ``init()`` method on all ``MapperProperties``
         attached to this mapper.
@@ -727,6 +731,7 @@ class Mapper(object):
 
         if self.non_primary:
             self._class_state = self.class_._class_state
+            _mapper_registry[self] = True
             return
 
         if not self.non_primary and '_class_state' in self.class_.__dict__ and (self.entity_name in self.class_._class_state.mappers):
@@ -743,15 +748,9 @@ class Mapper(object):
         attributes.register_class(self.class_, extra_init=extra_init, on_exception=on_exception)
         
         self._class_state = self.class_._class_state
-        if self._class_state not in _mapper_registry:
-            _mapper_registry[self._class_state] = []
+        _mapper_registry[self] = True
 
-        _COMPILE_MUTEX.acquire()
-        try:
-            _mapper_registry[self._class_state].append(self)
-            self.class_._class_state.mappers[self.entity_name] = self
-        finally:
-            _COMPILE_MUTEX.release()
+        self.class_._class_state.mappers[self.entity_name] = self
 
         for ext in util.to_list(self.extension, []):
             ext.instrument_class(self, self.class_)
diff --git a/lib/sqlalchemy/orm/properties.py b/lib/sqlalchemy/orm/properties.py
index b6d6cef63..027cefd69 100644
--- a/lib/sqlalchemy/orm/properties.py
+++ b/lib/sqlalchemy/orm/properties.py
@@ -150,7 +150,7 @@ class SynonymProperty(MapperProperty):
 
     def do_init(self):
         class_ = self.parent.class_
-        aliased_property = self.parent.get_property(self.key, resolve_synonyms=True)
+        aliased_property = self.parent._get_property(self.key, resolve_synonyms=True)
         self.logger.info("register managed attribute %s on class %s" % (self.key, class_.__name__))
         if self.instrument is None:
             class SynonymProp(object):
@@ -396,23 +396,23 @@ class PropertyLoader(StrategizedProperty):
 
     def _determine_targets(self):
         if isinstance(self.argument, type):
-            self.mapper = mapper.class_mapper(self.argument, entity_name=self.entity_name, compile=False)._check_compile()
+            self.mapper = mapper.class_mapper(self.argument, entity_name=self.entity_name, compile=False)
         elif isinstance(self.argument, mapper.Mapper):
-            self.mapper = self.argument._check_compile()
+            self.mapper = self.argument
         else:
             raise exceptions.ArgumentError("relation '%s' expects a class or a mapper argument (received: %s)" % (self.key, type(self.argument)))
 
         # ensure the "select_mapper", if different from the regular target mapper, is compiled.
-        self.mapper.get_select_mapper()._check_compile()
+        self.mapper.get_select_mapper()
 
         if not self.parent.concrete:
             for inheriting in self.parent.iterate_to_root():
-                if inheriting is not self.parent and inheriting.get_property(self.key, raiseerr=False):
+                if inheriting is not self.parent and inheriting._get_property(self.key, raiseerr=False):
                     warnings.warn(RuntimeWarning("Warning: relation '%s' on mapper '%s' supercedes the same relation on inherited mapper '%s'; this can cause dependency issues during flush" % (self.key, self.parent, inheriting)))
 
         if self.association is not None:
             if isinstance(self.association, type):
-                self.association = mapper.class_mapper(self.association, entity_name=self.entity_name, compile=False)._check_compile()
+                self.association = mapper.class_mapper(self.association, entity_name=self.entity_name, compile=False)
 
         self.target = self.mapper.mapped_table
         self.select_mapper = self.mapper.get_select_mapper()
@@ -650,7 +650,7 @@ class PropertyLoader(StrategizedProperty):
 
             if self.backref is not None:
                 self.backref.compile(self)
-        elif not mapper.class_mapper(self.parent.class_).get_property(self.key, raiseerr=False):
+        elif not mapper.class_mapper(self.parent.class_, compile=False)._get_property(self.key, raiseerr=False):
             raise exceptions.ArgumentError("Attempting to assign a new relation '%s' to a non-primary mapper on class '%s'.  New relations can only be added to the primary mapper, i.e. the very first mapper created for class '%s' " % (self.key, self.parent.class_.__name__, self.parent.class_.__name__))
 
         super(PropertyLoader, self).do_init()
@@ -727,7 +727,7 @@ class BackRef(object):
         self.prop = prop
         
         mapper = prop.mapper.primary_mapper()
-        if mapper.get_property(self.key, raiseerr=False) is None:
+        if mapper._get_property(self.key, raiseerr=False) is None:
             pj = self.kwargs.pop('primaryjoin', None)
             sj = self.kwargs.pop('secondaryjoin', None)
 
@@ -742,8 +742,8 @@ class BackRef(object):
                                       
             mapper._compile_property(self.key, relation);
 
-            prop.reverse_property = mapper.get_property(self.key)
-            mapper.get_property(self.key).reverse_property = prop
+            prop.reverse_property = mapper._get_property(self.key)
+            mapper._get_property(self.key).reverse_property = prop
 
         else:
             raise exceptions.ArgumentError("Error creating backref '%s' on relation '%s': property of that name exists on mapper '%s'" % (self.key, prop, mapper))
diff --git a/lib/sqlalchemy/util.py b/lib/sqlalchemy/util.py
index 3e26217c9..463d4b8af 100644
--- a/lib/sqlalchemy/util.py
+++ b/lib/sqlalchemy/util.py
@@ -4,7 +4,7 @@
 # This module is part of SQLAlchemy and is released under
 # the MIT License: http://www.opensource.org/licenses/mit-license.php
 
-import itertools, sys, warnings, sets
+import itertools, sys, warnings, sets, weakref
 import __builtin__
 
 from sqlalchemy import exceptions, logging
@@ -113,7 +113,7 @@ def flatten_iterator(x):
             yield elem
 
 class ArgSingleton(type):
-    instances = {}
+    instances = weakref.WeakValueDictionary()
 
     def dispose(cls):
         for key in list(ArgSingleton.instances):