Merge "Remove MappedCollection converter; deprecate @converter"

doc/build/changelog/unreleased_13/3604.rst

@@ -0,0 +1,17 @@
+.. change::
+   :tags: orm, bug
+   :tickets: 3604
+
+   Removed the collection converter used by the :class:`.MappedCollection`
+   class. This converter was used only to assert that the incoming dictionary
+   keys matched that of their corresponding objects, and only during a bulk set
+   operation.  The converter can interfere with a custom validator or
+   :meth:`.AttributeEvents.bulk_replace` listener that wants to convert
+   incoming values further.  The ``TypeError`` which would be raised by this
+   converter when an incoming key didn't match the value is removed; incoming
+   values during a bulk assignment will be keyed to their value-generated key,
+   and not the key that's explicitly present in the dictionary.
+
+   Overall, @converter is superseded by the
+   :meth:`.AttributeEvents.bulk_replace` event handler added as part of
+   :ticket:`3896`.

lib/sqlalchemy/orm/collections.py

@@ -442,6 +442,7 @@ def linker(fn):
     """deprecated; synonym for :meth:`.collection.linker`."""
 
     @staticmethod
+    @util.deprecated("1.3", "Use the bulk_replace event handler")
     def converter(fn):
         """Tag the method as the collection converter.
 
@@ -1517,30 +1518,6 @@ def remove(self, value, _sa_initiator=None):
                 (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
 # subclasses these and uses @internally_instrumented,

test/orm/test_collection.py

@@ -1,5 +1,4 @@
 from sqlalchemy.testing import eq_, ne_
-import sys
 from operator import and_
 
 import sqlalchemy.orm.collections as collections
@@ -13,6 +12,7 @@
     attributes, instrumentation
 from sqlalchemy.testing import fixtures
 from sqlalchemy.testing import assert_raises, assert_raises_message
+from sqlalchemy import testing
 
 
 class Canary(sa.orm.interfaces.AttributeExtension):
@@ -1093,27 +1093,14 @@ class Foo(object):
         # MappedCollection but is not present in basic, @converter-less
         # dict collections.
         e3 = creator()
-        if isinstance(obj.attr, collections.MappedCollection):
-            real_dict = dict(badkey=e3)
-            try:
-                obj.attr = real_dict
-                self.assert_(False)
-            except TypeError:
-                pass
-            self.assert_(obj.attr is not real_dict)
-            self.assert_('badkey' not in obj.attr)
-            eq_(set(collections.collection_adapter(obj.attr)),
-                set([e2]))
-            self.assert_(e3 not in canary.added)
-        else:
-            real_dict = dict(keyignored1=e3)
-            obj.attr = real_dict
-            self.assert_(obj.attr is not real_dict)
-            self.assert_('keyignored1' not in obj.attr)
-            eq_(set(collections.collection_adapter(obj.attr)),
-                set([e3]))
-            self.assert_(e2 in canary.removed)
-            self.assert_(e3 in canary.added)
+        real_dict = dict(keyignored1=e3)
+        obj.attr = real_dict
+        self.assert_(obj.attr is not real_dict)
+        self.assert_('keyignored1' not in obj.attr)
+        eq_(set(collections.collection_adapter(obj.attr)),
+            set([e3]))
+        self.assert_(e2 in canary.removed)
+        self.assert_(e3 in canary.added)
 
         obj.attr = typecallable()
         eq_(list(collections.collection_adapter(obj.attr)), [])
@@ -1182,6 +1169,7 @@ def __init__(self):
         self._test_dict_bulk(MyOrdered)
         self.assert_(getattr(MyOrdered, '_sa_instrumented') == id(MyOrdered))
 
+    @testing.uses_deprecated(r".*Use the bulk_replace event handler")
     def test_dict_subclass4(self):
         # tests #2654
         class MyDict(collections.MappedCollection):
@@ -2248,6 +2236,7 @@ class Touchy(list):
 
         collections._instrument_class(Touchy)
 
+    @testing.uses_deprecated(r".*Use the bulk_replace event handler")
     def test_name_setup(self):
 
         class Base(object):

test/orm/test_validators.py

@@ -2,6 +2,7 @@
 from sqlalchemy.testing import fixtures, assert_raises, eq_, ne_, \
     assert_raises_message
 from sqlalchemy.orm import mapper, Session, validates, relationship
+from sqlalchemy.orm import collections
 from sqlalchemy.testing.mock import Mock, call
 from sqlalchemy import exc
 
@@ -184,6 +185,50 @@ def validate_address(self, key, item, remove):
             [Address(email_address="e3"), Address(email_address="e4")]
         )
 
+    def test_validator_bulk_dict_set(self):
+        users, addresses, Address = (self.tables.users,
+                                     self.tables.addresses,
+                                     self.classes.Address)
+
+        class User(fixtures.ComparableEntity):
+
+            @validates('addresses', include_removes=True)
+            def validate_address(self, key, item, remove):
+                if not remove:
+                    assert isinstance(item, str)
+                else:
+                    assert isinstance(item, Address)
+                item = Address(email_address=item)
+                return item
+
+        mapper(User, users, properties={
+            'addresses': relationship(
+                Address,
+                collection_class=collections.attribute_mapped_collection(
+                    "email_address")
+            )
+        })
+        mapper(Address, addresses)
+
+        u1 = User()
+        u1.addresses["e1"] = "e1"
+        u1.addresses["e2"] = "e2"
+        eq_(
+            u1.addresses,
+            {
+                "e1": Address(email_address="e1"),
+                "e2": Address(email_address="e2")
+            }
+        )
+        u1.addresses = {"e3": "e3", "e4": "e4"}
+        eq_(
+            u1.addresses,
+            {
+                "e3": Address(email_address="e3"),
+                "e4": Address(email_address="e4")
+            }
+        )
+
     def test_validator_multi_warning(self):
         users = self.tables.users