From: Mike Bayer Date: Mon, 21 Jan 2013 22:57:24 +0000 (-0500) Subject: - Made some fixes to the system of producing custom instrumented X-Git-Tag: rel_0_8_0~28^2~2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=2c425c9680b813952254292c5d1ee8e234b6f53a;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git - Made some fixes to the system of producing custom instrumented collections, mainly that the usage of the @collection decorators will now honor the __mro__ of the given class, applying the logic of the sub-most classes' version of a particular collection method. Previously, it wasn't predictable when subclassing an existing instrumented class such as :class:`.MappedCollection` whether or not custom methods would resolve correctly. [ticket:2654] - The undocumented (and hopefully unused) system of producing custom collections using an ``__instrumentation__`` datastructure associated with the collection has been removed, as this was a complex and untested feature which was also essentially redundant versus the decorator approach. Other internal simplifcations to the orm.collections module have been made as well. --- diff --git a/doc/build/changelog/changelog_08.rst b/doc/build/changelog/changelog_08.rst index c9b66be7ec..bd175b5300 100644 --- a/doc/build/changelog/changelog_08.rst +++ b/doc/build/changelog/changelog_08.rst @@ -6,6 +6,28 @@ .. changelog:: :version: 0.8.0 + .. change:: + :tags: orm, bug + :tickets: 2654 + + Made some fixes to the system of producing custom instrumented + collections, mainly that the usage of the @collection decorators + will now honor the __mro__ of the given class, applying the + logic of the sub-most classes' version of a particular collection + method. Previously, it wasn't predictable when subclassing + an existing instrumented class such as :class:`.MappedCollection` + whether or not custom methods would resolve correctly. + + .. change:: + :tags: orm, removed + + The undocumented (and hopefully unused) system of producing + custom collections using an ``__instrumentation__`` datastructure + associated with the collection has been removed, as this was a complex + and untested feature which was also essentially redundant versus the + decorator approach. Other internal simplifcations to the + orm.collections module have been made as well. + .. change:: :tags: mssql, feature :pullreq: 35 diff --git a/lib/sqlalchemy/orm/collections.py b/lib/sqlalchemy/orm/collections.py index 620c20910b..c37f9b135a 100644 --- a/lib/sqlalchemy/orm/collections.py +++ b/lib/sqlalchemy/orm/collections.py @@ -12,7 +12,7 @@ used, allowing arbitrary types (including built-ins) to be used as entity collections without requiring inheritance from a base class. Instrumentation decoration relays membership change events to the -``InstrumentedCollectionAttribute`` that is currently managing the collection. +:class:`.CollectionAttributeImpl` that is currently managing the collection. The decorators observe function call arguments and return values, tracking entities entering or leaving the collection. Two decorator approaches are provided. One is a bundle of generic decorators that map function arguments @@ -97,12 +97,11 @@ instrumentation may be the answer. Within your method, ``collection_adapter(self)`` will retrieve an object that you can use for explicit control over triggering append and remove events. -The owning object and InstrumentedCollectionAttribute are also reachable +The owning object and :class:`.CollectionAttributeImpl` are also reachable through the adapter, allowing for some very sophisticated behavior. """ -import copy import inspect import operator import weakref @@ -832,7 +831,7 @@ def prepare_instrumentation(factory): # Did factory callable return a builtin? if cls in __canned_instrumentation: # Wrap it so that it returns our 'Instrumented*' - factory = __converting_factory(factory) + factory = __converting_factory(cls, factory) cls = factory() # Instrument the class if needed. @@ -846,52 +845,27 @@ def prepare_instrumentation(factory): return factory -def __converting_factory(original_factory): - """Convert the type returned by collection factories on the fly. - - Given a collection factory that returns a builtin type (e.g. a list), - return a wrapped function that converts that type to one of our - instrumented types. +def __converting_factory(specimen_cls, original_factory): + """Return a wrapper that converts a "canned" collection like + set, dict, list into the Instrumented* version. """ + + instrumented_cls = __canned_instrumentation[specimen_cls] + def wrapper(): collection = original_factory() - type_ = type(collection) - if type_ in __canned_instrumentation: - # return an instrumented type initialized from the factory's - # collection - return __canned_instrumentation[type_](collection) - else: - raise sa_exc.InvalidRequestError( - "Collection class factories must produce instances of a " - "single class.") - try: - # often flawed but better than nothing - wrapper.__name__ = "%sWrapper" % original_factory.__name__ - wrapper.__doc__ = original_factory.__doc__ - except: - pass - return wrapper + return instrumented_cls(collection) + + # often flawed but better than nothing + wrapper.__name__ = "%sWrapper" % original_factory.__name__ + wrapper.__doc__ = original_factory.__doc__ + return wrapper def _instrument_class(cls): """Modify methods in a class and install instrumentation.""" - # this can be documented as a decoratorless - # option for specifying instrumentation. (likely doc'd here in code only, - # not in online docs.) Useful for C types too. - # - # __instrumentation__ = { - # 'rolename': 'methodname', # ... - # 'methods': { - # 'methodname': ('fire_{append,remove}_event', argspec, - # 'fire_{append,remove}_event'), - # 'append': ('fire_append_event', 1, None), - # '__setitem__': ('fire_append_event', 1, 'fire_remove_event'), - # 'pop': (None, None, 'fire_remove_event'), - # } - # } - # In the normal call flow, a request for any of the 3 basic collection # types is transformed into one of our trivial subclasses # (e.g. InstrumentedList). Catch anything else that sneaks in here... @@ -900,53 +874,55 @@ def _instrument_class(cls): "Can not instrument a built-in type. Use a " "subclass, even a trivial one.") + roles = {} + methods = {} + + # search for _sa_instrument_role-decorated methods in + # method resolution order, assign to roles + for supercls in cls.__mro__: + for name, method in vars(supercls).items(): + if not util.callable(method): + continue + + # note role declarations + if hasattr(method, '_sa_instrument_role'): + role = method._sa_instrument_role + assert role in ('appender', 'remover', 'iterator', + 'link', 'converter') + roles.setdefault(role, name) + + # transfer instrumentation requests from decorated function + # to the combined queue + before, after = None, None + if hasattr(method, '_sa_instrument_before'): + op, argument = method._sa_instrument_before + assert op in ('fire_append_event', 'fire_remove_event') + before = op, argument + if hasattr(method, '_sa_instrument_after'): + op = method._sa_instrument_after + assert op in ('fire_append_event', 'fire_remove_event') + after = op + if before: + methods[name] = before[0], before[1], after + elif after: + methods[name] = None, None, after + + # see if this class has "canned" roles based on a known + # collection type (dict, set, list). Apply those roles + # as needed to the "roles" dictionary, and also + # prepare "decorator" methods collection_type = util.duck_type_collection(cls) if collection_type in __interfaces: - roles = __interfaces[collection_type].copy() - decorators = roles.pop('_decorators', {}) - else: - roles, decorators = {}, {} - - if hasattr(cls, '__instrumentation__'): - roles.update(copy.deepcopy(getattr(cls, '__instrumentation__'))) - - methods = roles.pop('methods', {}) - - for name in dir(cls): - method = getattr(cls, name, None) - if not util.callable(method): - continue - - # note role declarations - if hasattr(method, '_sa_instrument_role'): - role = method._sa_instrument_role - assert role in ('appender', 'remover', 'iterator', - 'link', 'converter') - roles[role] = name - - # transfer instrumentation requests from decorated function - # to the combined queue - before, after = None, None - if hasattr(method, '_sa_instrument_before'): - op, argument = method._sa_instrument_before - assert op in ('fire_append_event', 'fire_remove_event') - before = op, argument - if hasattr(method, '_sa_instrument_after'): - op = method._sa_instrument_after - assert op in ('fire_append_event', 'fire_remove_event') - after = op - if before: - methods[name] = before[0], before[1], after - elif after: - methods[name] = None, None, after + canned_roles, decorators = __interfaces[collection_type] + for role, name in canned_roles.items(): + roles.setdefault(role, name) - # apply ABC auto-decoration to methods that need it - - for method, decorator in decorators.items(): - fn = getattr(cls, method, None) - if (fn and method not in methods and - not hasattr(fn, '_sa_instrumented')): - setattr(cls, method, decorator(fn)) + # apply ABC auto-decoration to methods that need it + for method, decorator in decorators.items(): + fn = getattr(cls, method, None) + if (fn and method not in methods and + not hasattr(fn, '_sa_instrumented')): + setattr(cls, method, decorator(fn)) # ensure all roles are present, and apply implicit instrumentation if # needed @@ -973,13 +949,13 @@ def _instrument_class(cls): # apply ad-hoc instrumentation from decorators, class-level defaults # and implicit role declarations - for method, (before, argument, after) in methods.items(): - setattr(cls, method, - _instrument_membership_mutator(getattr(cls, method), + for method_name, (before, argument, after) in methods.items(): + setattr(cls, method_name, + _instrument_membership_mutator(getattr(cls, method_name), before, argument, after)) # intern the role map - for role, method in roles.items(): - setattr(cls, '_sa_%s' % role, getattr(cls, method)) + for role, method_name in roles.items(): + setattr(cls, '_sa_%s' % role, getattr(cls, method_name)) setattr(cls, '_sa_instrumented', id(cls)) @@ -1032,12 +1008,12 @@ def _instrument_membership_mutator(method, before, argument, after): if res is not None: getattr(executor, after)(res, initiator) return res - try: - wrapper._sa_instrumented = True - wrapper.__name__ = method.__name__ - wrapper.__doc__ = method.__doc__ - except: - pass + + wrapper._sa_instrumented = True + if hasattr(method, "_sa_instrument_role"): + wrapper._sa_instrument_role = method._sa_instrument_role + wrapper.__name__ = method.__name__ + wrapper.__doc__ = method.__doc__ return wrapper @@ -1461,31 +1437,14 @@ def _set_decorators(): class InstrumentedList(list): """An instrumented version of the built-in list.""" - __instrumentation__ = { - 'appender': 'append', - 'remover': 'remove', - 'iterator': '__iter__', } - class InstrumentedSet(set): """An instrumented version of the built-in set.""" - __instrumentation__ = { - 'appender': 'add', - 'remover': 'remove', - 'iterator': '__iter__', } - class InstrumentedDict(dict): """An instrumented version of the built-in dict.""" - # Py3K - #__instrumentation__ = { - # 'iterator': 'values', } - # Py2K - __instrumentation__ = { - 'iterator': 'itervalues', } - # end Py2K __canned_instrumentation = { list: InstrumentedList, @@ -1494,24 +1453,22 @@ __canned_instrumentation = { } __interfaces = { - list: {'appender': 'append', - 'remover': 'remove', - 'iterator': '__iter__', - '_decorators': _list_decorators(), }, - set: {'appender': 'add', + list: ( + {'appender': 'append', 'remover': 'remove', + 'iterator': '__iter__'}, _list_decorators() + ), + + set: ({'appender': 'add', 'remover': 'remove', - 'iterator': '__iter__', - '_decorators': _set_decorators(), }, + 'iterator': '__iter__'}, _set_decorators() + ), + # decorators are required for dicts and object collections. # Py3K - #dict: {'iterator': 'values', - # '_decorators': _dict_decorators(), }, + #dict: ({'iterator': 'values'}, _dict_decorators()), # Py2K - dict: {'iterator': 'itervalues', - '_decorators': _dict_decorators(), }, + dict: ({'iterator': 'itervalues'}, _dict_decorators()), # end Py2K - # < 0.4 compatible naming, deprecated- use decorators instead. - None: {} } @@ -1541,14 +1498,16 @@ class MappedCollection(dict): """ self.keyfunc = keyfunc + @collection.appender + @collection.internally_instrumented def set(self, value, _sa_initiator=None): """Add an item by value, consulting the keyfunc for the key.""" key = self.keyfunc(value) self.__setitem__(key, value, _sa_initiator) - set = collection.internally_instrumented(set) - set = collection.appender(set) + @collection.remover + @collection.internally_instrumented def remove(self, value, _sa_initiator=None): """Remove an item by value, consulting the keyfunc for the key.""" @@ -1563,9 +1522,8 @@ class MappedCollection(dict): "values after flush?" % (value, self[key], key)) self.__delitem__(key, _sa_initiator) - remove = collection.internally_instrumented(remove) - remove = collection.remover(remove) + @collection.converter def _convert(self, dictlike): """Validate and convert a dict-like object into values for set()ing. @@ -1588,7 +1546,6 @@ class MappedCollection(dict): "keying function requires a key of %r for this value." % ( incoming_key, value, new_key)) yield value - _convert = collection.converter(_convert) # 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 7571d59ff3..7859af37c9 100644 --- a/test/orm/test_collection.py +++ b/test/orm/test_collection.py @@ -1098,6 +1098,32 @@ class CollectionsTest(fixtures.ORMTest): self._test_dict_bulk(MyOrdered) self.assert_(getattr(MyOrdered, '_sa_instrumented') == id(MyOrdered)) + def test_dict_subclass4(self): + # tests #2654 + class MyDict(collections.MappedCollection): + def __init__(self): + super(MyDict, self).__init__(lambda value: "k%d" % value) + + @collection.converter + def _convert(self, dictlike): + for key, value in dictlike.iteritems(): + yield value + 5 + + class Foo(object): + pass + + canary = Canary() + + instrumentation.register_class(Foo) + attributes.register_attribute(Foo, 'attr', uselist=True, + extension=canary, + typecallable=MyDict, useobject=True) + + f = Foo() + f.attr = {"k1": 1, "k2": 2} + + eq_(f.attr, {'k7': 7, 'k6': 6}) + def test_dict_duck(self): class DictLike(object): def __init__(self): @@ -2075,3 +2101,48 @@ class InstrumentationTest(fixtures.ORMTest): collections._instrument_class(Touchy) + def test_name_setup(self): + + class Base(object): + @collection.iterator + def base_iterate(self, x): + return "base_iterate" + + @collection.appender + def base_append(self, x): + return "base_append" + + @collection.converter + def base_convert(self, x): + return "base_convert" + + @collection.remover + def base_remove(self, x): + return "base_remove" + + + from sqlalchemy.orm.collections import _instrument_class + _instrument_class(Base) + + eq_(Base._sa_remover(Base(), 5), "base_remove") + eq_(Base._sa_appender(Base(), 5), "base_append") + eq_(Base._sa_iterator(Base(), 5), "base_iterate") + eq_(Base._sa_converter(Base(), 5), "base_convert") + + class Sub(Base): + @collection.converter + def base_convert(self, x): + return "sub_convert" + + @collection.remover + def sub_remove(self, x): + return "sub_remove" + _instrument_class(Sub) + + eq_(Sub._sa_appender(Sub(), 5), "base_append") + eq_(Sub._sa_remover(Sub(), 5), "sub_remove") + eq_(Sub._sa_iterator(Sub(), 5), "base_iterate") + eq_(Sub._sa_converter(Sub(), 5), "sub_convert") + + +