- dictlike wipticket_3604

3 files changed, 103 insertions, 55 deletions

diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py
index 8605df785..03c48151e 100644
--- a/lib/sqlalchemy/orm/attributes.py
+++ b/lib/sqlalchemy/orm/attributes.py
@@ -855,7 +855,7 @@ class CollectionAttributeImpl(AttributeImpl):
 
     __slots__ = (
         'copy', 'collection_factory', '_append_token', '_remove_token',
-        '_duck_typed_as'
+        '_duck_typed_as', '_dictlike'
     )
 
     def __init__(self, class_, key, callable_, dispatch,
@@ -878,6 +878,7 @@ class CollectionAttributeImpl(AttributeImpl):
         self._remove_token = None
         self._duck_typed_as = util.duck_type_collection(
             self.collection_factory())
+        self._dictlike = self._duck_typed_as is dict
 
         if getattr(self.collection_factory, "_sa_linker", None):
 
@@ -1025,12 +1026,15 @@ class CollectionAttributeImpl(AttributeImpl):
             passive=PASSIVE_OFF, pop=False, _adapt=True):
         iterable = orig_iterable = value
 
+        dictlike = self._dictlike
+
         # pulling a new collection first so that an adaptation exception does
         # not trigger a lazy load of the old collection.
         new_collection, user_data = self._initialize_collection(state)
         if _adapt:
             if new_collection._converter is not None:
                 iterable = new_collection._converter(iterable)
+                dictlike = False
             else:
                 setting_type = util.duck_type_collection(iterable)
                 receiving_type = self._duck_typed_as
@@ -1047,15 +1051,20 @@ class CollectionAttributeImpl(AttributeImpl):
                 # adapter.
                 if hasattr(iterable, '_sa_iterator'):
                     iterable = iterable._sa_iterator()
+                    dictlike = False
                 elif setting_type is dict:
-                    if util.py3k:
-                        iterable = iterable.values()
+                    if dictlike:
+                        iterable = iterable.items()
                     else:
-                        iterable = getattr(
-                            iterable, 'itervalues', iterable.values)()
+                        iterable = iterable.values()
                 else:
+                    assert not dictlike
                     iterable = iter(iterable)
-        new_values = list(iterable)
+
+        if dictlike:
+            new_values = dict(iterable)
+        else:
+            new_values = list(iterable)
 
         old = self.get(state, dict_, passive=PASSIVE_ONLY_PERSISTENT)
         if old is PASSIVE_NO_RESULT:
@@ -1072,8 +1081,12 @@ class CollectionAttributeImpl(AttributeImpl):
 
         dict_[self.key] = user_data
 
-        collections.bulk_replace(
-            new_values, old_collection, new_collection)
+        if dictlike:
+            collections.bulk_replace_dictlike(
+                new_values, old_collection, new_collection)
+        else:
+            collections.bulk_replace(
+                new_values, old_collection, new_collection)
 
         del old._sa_adapter
         self.dispatch.dispose_collection(state, old, old_collection)
diff --git a/lib/sqlalchemy/orm/collections.py b/lib/sqlalchemy/orm/collections.py
index 58a69227c..c63c480b3 100644
--- a/lib/sqlalchemy/orm/collections.py
+++ b/lib/sqlalchemy/orm/collections.py
@@ -451,24 +451,18 @@ class collection(object):
             myobj.acollection = [newvalue1, newvalue2]
 
         The converter method will receive the object being assigned and should
-        return an iterable of values suitable for use by the ``appender``
-        method.  A converter must not assign values or mutate the collection,
+        return an iterable of values suitable for use by the
+        :meth:`.collection.appender` -decorated method.
+        A converter must not assign values or mutate the collection,
         its sole job is to adapt the value the user provides into an iterable
         of values for the ORM's use.
 
-        The default converter implementation will use duck-typing to do the
-        conversion.  A dict-like collection will be convert into an iterable
-        of dictionary values, and other types will simply be iterated::
-
-            @collection.converter
-            def convert(self, other): ...
-
-        If the duck-typing of the object does not match the type of this
-        collection, a TypeError is raised.
+        When a converter is not specified, the set() implementation handles
+        incoming collections automatcally using a duck-typing approach.
 
         Supply an implementation of this method if you want to expand the
-        range of possible types that can be assigned in bulk or perform
-        validation on the values about to be assigned.
+        range of possible types that can be assigned in bulk, or perform
+        validation on values which are to be assigned in bulk.
 
         """
         fn._sa_instrument_role = 'converter'
@@ -557,6 +551,19 @@ class collection(object):
             return fn
         return decorator
 
+    @staticmethod
+    def items_iterator(fn):
+        """Tag a method as the "items iterator" for a dictionary-oriented
+        collection.
+
+        Only used during bulk set operations.
+
+        .. versionadded:: 1.1
+
+        """
+        fn._sa_instrument_role = 'items_iterator'
+        return fn
+
 
 collection_adapter = operator.attrgetter('_sa_adapter')
 """Fetch the :class:`.CollectionAdapter` for a collection."""
@@ -605,6 +612,16 @@ class CollectionAdapter(object):
         """
         return self.owner_state.dict[self._key] is self._data()
 
+    # TODO: provide "bulk" versions of these
+    def setitem_with_event(self, key, value, initiator=None):
+        self._data().__setitem__(key, value, _sa_initiator=initiator)
+
+    def setitem_without_event(self, key, value, initiator=None):
+        self._data().__setitem__(key, value, _sa_initiator=False)
+
+    def delitem_with_event(self, key, initiator=None):
+        self._data().__delitem__(key, _sa_initiator=initiator)
+
     def bulk_appender(self):
         return self._data()._sa_appender
 
@@ -722,6 +739,25 @@ class CollectionAdapter(object):
         self._data = weakref.ref(d['data'])
 
 
+def bulk_replace_dictlike(dict_, existing_adapter, new_adapter):
+
+    existing_dict = dict(existing_adapter._data()._sa_items_iterator())
+    existing_keyset = set(existing_dict)
+    constants = existing_keyset.intersection(dict_.keys())
+    additions = set(dict_.keys()).difference(constants)
+    removals = existing_keyset.difference(constants)
+
+    for key in dict_:
+        if key in additions:
+            new_adapter.setitem_with_event(key, dict_[key])
+        elif key in constants:
+            new_adapter.setitem_without_event(key, dict_[key])
+
+    if existing_adapter:
+        for key in removals:
+            existing_adapter.delitem_with_event(key)
+
+
 def bulk_replace(values, existing_adapter, new_adapter):
     """Load a new collection, firing events based on prior like membership.
 
@@ -830,9 +866,9 @@ def _instrument_class(cls):
 
     roles, methods = _locate_roles_and_methods(cls)
 
-    _setup_canned_roles(cls, roles, methods)
+    isdict = _setup_canned_roles(cls, roles, methods)
 
-    _assert_required_roles(cls, roles, methods)
+    _assert_required_roles(cls, roles, methods, isdict)
 
     _set_collection_attributes(cls, roles, methods)
 
@@ -855,7 +891,7 @@ def _locate_roles_and_methods(cls):
             if hasattr(method, '_sa_instrument_role'):
                 role = method._sa_instrument_role
                 assert role in ('appender', 'remover', 'iterator',
-                                'linker', 'converter')
+                                'linker', 'converter', 'items_iterator')
                 roles.setdefault(role, name)
 
             # transfer instrumentation requests from decorated function
@@ -896,8 +932,10 @@ def _setup_canned_roles(cls, roles, methods):
                     not hasattr(fn, '_sa_instrumented')):
                 setattr(cls, method, decorator(fn))
 
+    return collection_type is dict
 
-def _assert_required_roles(cls, roles, methods):
+
+def _assert_required_roles(cls, roles, methods, isdict):
     """ensure all roles are present, and apply implicit instrumentation if
     needed
 
@@ -923,6 +961,14 @@ def _assert_required_roles(cls, roles, methods):
             "Type %s must elect an iterator method to be "
             "a collection class" % cls.__name__)
 
+    if isdict and (
+            'items_iterator' not in roles
+            or not hasattr(cls, roles['items_iterator'])
+    ):
+        raise sa_exc.ArgumentError(
+            "Type %s must elect a items_iterator method to be "
+            "a collection class" % cls.__name__)
+
 
 def _set_collection_attributes(cls, roles, methods):
     """apply ad-hoc instrumentation from decorators, class-level defaults
@@ -1456,8 +1502,10 @@ __interfaces = {
           ),
 
     # decorators are required for dicts and object collections.
-    dict: ({'iterator': 'values'}, _dict_decorators()) if util.py3k
-    else ({'iterator': 'itervalues'}, _dict_decorators()),
+    dict: ({'iterator': 'values', 'items_iterator': 'items'},
+           _dict_decorators()) if util.py3k
+    else ({'iterator': 'itervalues', 'items_iterator': 'iteritems'},
+          _dict_decorators()),
 }
 
 
@@ -1512,29 +1560,6 @@ class MappedCollection(dict):
                 (value, self[key], key))
         self.__delitem__(key, _sa_initiator)
 
-    @collection.converter
-    def _convert(self, dictlike):
-        """Validate and convert a dict-like object into values for set()ing.
-
-        This is called behind the scenes when a MappedCollection is replaced
-        entirely by another collection, as in::
-
-          myobj.mappedcollection = {'a':obj1, 'b': obj2} # ...
-
-        Raises a TypeError if the key in any (key, value) pair in the dictlike
-        object does not match the key that this collection's keyfunc would
-        have assigned for that value.
-
-        """
-        for incoming_key, value in util.dictlike_iteritems(dictlike):
-            new_key = self.keyfunc(value)
-            if incoming_key != new_key:
-                raise TypeError(
-                    "Found incompatible key %r for value %r; this "
-                    "collection's "
-                    "keying function requires a key of %r for this value." % (
-                        incoming_key, value, new_key))
-            yield value
 
 # ensure instrumentation is associated with
 # these built-in classes; if a user-defined class
diff --git a/test/orm/test_collection.py b/test/orm/test_collection.py
index 82331b9af..f51cb3a26 100644
--- a/test/orm/test_collection.py
+++ b/test/orm/test_collection.py
@@ -1028,10 +1028,7 @@ class CollectionsTest(fixtures.ORMTest):
         self.assert_(e1 in canary.removed)
         self.assert_(e2 in canary.added)
 
-
-        # key validity on bulk assignment is a basic feature of
-        # MappedCollection but is not present in basic, @converter-less
-        # dict collections.
+        # auto key-validity from bulk operations removed in 1.1
         e3 = creator()
         if isinstance(obj.attr, collections.MappedCollection):
             real_dict = dict(badkey=e3)
@@ -1049,7 +1046,7 @@ class CollectionsTest(fixtures.ORMTest):
             real_dict = dict(keyignored1=e3)
             obj.attr = real_dict
             self.assert_(obj.attr is not real_dict)
-            self.assert_('keyignored1' not in obj.attr)
+            self.assert_('keyignored1' in obj.attr)
             eq_(set(collections.collection_adapter(obj.attr)),
                               set([e3]))
             self.assert_(e2 in canary.removed)
@@ -1152,31 +1149,44 @@ class CollectionsTest(fixtures.ORMTest):
             def __init__(self):
                 self.data = dict()
 
+            @collection.items_iterator
+            def _item_iter(self):
+                return self.data.items()
+
             @collection.appender
             @collection.replaces(1)
             def set(self, item):
                 current = self.data.get(item.a, None)
                 self.data[item.a] = item
                 return current
+
             @collection.remover
             def _remove(self, item):
                 del self.data[item.a]
+
             def __setitem__(self, key, value):
                 self.data[key] = value
+
             def __getitem__(self, key):
                 return self.data[key]
+
             def __delitem__(self, key):
                 del self.data[key]
+
             def values(self):
                 return list(self.data.values())
+
             def __contains__(self, key):
                 return key in self.data
+
             @collection.iterator
             def itervalues(self):
                 return iter(self.data.values())
             __hash__ = object.__hash__
+
             def __eq__(self, other):
                 return self.data == other
+
             def __repr__(self):
                 return 'DictLike(%s)' % repr(self.data)