From fe8ddb71d98f9f8b5e8e5bcf54b4208a1dfad2fd Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Wed, 26 Sep 2018 18:52:44 -0400 Subject: [PATCH] Remove MappedCollection converter; deprecate @converter 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`. Fixes: #3604 Change-Id: Id0f7bd2cec938f5975eb2ab94df9ba5754dd43c3 --- doc/build/changelog/unreleased_13/3604.rst | 17 ++++++++ lib/sqlalchemy/orm/collections.py | 25 +----------- test/orm/test_collection.py | 33 ++++++---------- test/orm/test_validators.py | 45 ++++++++++++++++++++++ 4 files changed, 74 insertions(+), 46 deletions(-) create mode 100644 doc/build/changelog/unreleased_13/3604.rst diff --git a/doc/build/changelog/unreleased_13/3604.rst b/doc/build/changelog/unreleased_13/3604.rst new file mode 100644 index 0000000000..41b09f55f1 --- /dev/null +++ b/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`. diff --git a/lib/sqlalchemy/orm/collections.py b/lib/sqlalchemy/orm/collections.py index 5faff83a97..d6c23f5d29 100644 --- a/lib/sqlalchemy/orm/collections.py +++ b/lib/sqlalchemy/orm/collections.py @@ -442,6 +442,7 @@ class collection(object): """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 @@ 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 # subclasses these and uses @internally_instrumented, diff --git a/test/orm/test_collection.py b/test/orm/test_collection.py index f0f4de8a99..58c8706457 100644 --- a/test/orm/test_collection.py +++ b/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 @@ from sqlalchemy.orm import create_session, mapper, relationship, \ 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 CollectionsTest(fixtures.ORMTest): # 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 @@ class CollectionsTest(fixtures.ORMTest): 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 InstrumentationTest(fixtures.ORMTest): collections._instrument_class(Touchy) + @testing.uses_deprecated(r".*Use the bulk_replace event handler") def test_name_setup(self): class Base(object): diff --git a/test/orm/test_validators.py b/test/orm/test_validators.py index 7ceab44dcf..cbbf9f7a8e 100644 --- a/test/orm/test_validators.py +++ b/test/orm/test_validators.py @@ -2,6 +2,7 @@ from test.orm import _fixtures 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 @@ class ValidatorTest(_fixtures.FixtureTest): [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 -- 2.47.2