]> git.ipfire.org Git - thirdparty/sqlalchemy/sqlalchemy.git/commitdiff
- Made some fixes to the system of producing custom instrumented
authorMike Bayer <mike_mp@zzzcomputing.com>
Mon, 21 Jan 2013 22:57:24 +0000 (17:57 -0500)
committerMike Bayer <mike_mp@zzzcomputing.com>
Mon, 21 Jan 2013 22:57:24 +0000 (17:57 -0500)
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.

doc/build/changelog/changelog_08.rst
lib/sqlalchemy/orm/collections.py
test/orm/test_collection.py

index c9b66be7ec909974df8427cf98dc3f8c9734ae9e..bd175b5300b411454c86907e97a3391eac000339 100644 (file)
@@ -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
index 620c20910b4b2f400bb46ceb61f96bc24c463523..c37f9b135ad983f0b274bd4011693086b3e1005b 100644 (file)
@@ -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
index 7571d59ff3bd3c78da3e35c8d735996bb2d3c611..7859af37c9dbf1c15d65d5c286264698f206ad2d 100644 (file)
@@ -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")
+
+
+