Merge lp:~wgrant/lazr.restful/cross-module-contribution into lp:lazr.restful

[William Grant]

Rework contributes_to handling to not require that an interface and its extensions all live in one webservice:register module.

The implementation of extension interfaces with contributes_to explodes with zope.configuration conflicts when an interface and its contributors span multiple webservice:register directives. find_interfaces_and_contributors is run once per webservice:register, and does all of the aggregation internally, not considering that there might be other modules involved:

zope.configuration.config.ConfigurationConflictError: Conflicting configuration actions
  For: ('webservice entry interface', <InterfaceClass lp.registry.interfaces.product.IProduct>)
    File "/home/wgrant/src/launchpad/branches/sandbox/lib/lp/blueprints/configure.zcml", line 248.2-248.70
        <webservice:register module="lp.blueprints.interfaces.webservice" />
    File "/home/wgrant/src/launchpad/branches/sandbox/lib/lp/registry/configure.zcml", line 2078.4-2078.70
          <webservice:register module="lp.registry.interfaces.webservice" />
  For: ('webservice versioned operations', <InterfaceClass lp.registry.interfaces.product.IProduct>)
    File "/home/wgrant/src/launchpad/branches/sandbox/lib/lp/blueprints/configure.zcml", line 248.2-248.70
        <webservice:register module="lp.blueprints.interfaces.webservice" />
    File "/home/wgrant/src/launchpad/branches/sandbox/lib/lp/registry/configure.zcml", line 2078.4-2078.70
          <webservice:register module="lp.registry.interfaces.webservice" />

This is a fatal flaw for the situation that contributes_to was meant to solve: it's not possible to have entry extensions pluggable by optional bits of ZCML.

This branch fixes extension interfaces to be calculated globally. I use a global dict in metazcml.py, cleared in zope.testing.cleanup like the other global Zopey state (see zope.component, zope.security, storm for examples). I had to use a custom configuration action order to ensure that all contributors are registered before we start generating entry and operation adapters.

[Celso Providelo (cprov)]

Thanks for landing this change, it will help a lot the tasks we have in mind for LP and hopefully other large products using lazr.restful.

review: Approve

[lp:~wgrant/lazr.restful/cross-module-contribution]

210. By William Grant on 2014-03-20

Update NEWS.

=== modified file 'src/lazr/restful/NEWS.txt'
--- src/lazr/restful/NEWS.txt	2012-12-06 18:52:21 +0000
+++ src/lazr/restful/NEWS.txt	2014-03-20 01:50:54 +0000
@@ -2,12 +2,20 @@
 NEWS for lazr.restful
 =====================
 
+0.9.11 (UNRELEASED)
+===================
+
+Fixed bug 1294543: contributes_to can now reference interfaces in other
+modules and webservice:register directives.
+
 0.19.10 (2012-12-06)
+====================
 
 Fixed bug 809863: WebServicePublicationMixin.getResouce() converts
 ComponentLookupErrors to NotFound.
 
 0.19.9 (2012-10-23)
+===================
 
 Fixed bug 924291: The FixedVocabularyFieldMarshaller will now correctly return
 the entire vocabulary if the value passed in is None.

=== modified file 'src/lazr/restful/metazcml.py'
--- src/lazr/restful/metazcml.py	2011-03-24 21:29:49 +0000
+++ src/lazr/restful/metazcml.py	2014-03-20 01:50:54 +0000
@@ -6,6 +6,7 @@
 __all__ = []
 
 
+import collections
 import inspect
 import itertools
 
@@ -54,6 +55,23 @@
 REGISTERED_ENTRIES = []
 REGISTERED_OPERATIONS = []
 
+# Keep a global mapping of each interface's contributors, registered by
+# contribute_to_interface and inspected by the entry generators.
+REGISTERED_CONTRIBUTORS = collections.defaultdict(list)
+
+
+def reset_globals():
+    del REGISTERED_ENTRIES[:]
+    del REGISTERED_OPERATIONS[:]
+    REGISTERED_CONTRIBUTORS.clear()
+
+try:
+    from zope.testing.cleanup import addCleanUp
+except ImportError:
+    pass
+else:
+    addCleanUp(reset_globals)
+
 
 class IRegisterDirective(Interface):
     """Directive to hook up webservice based on the declarations in a module.
@@ -62,8 +80,30 @@
         title=u'Module which will be inspected for webservice declarations')
 
 
-def generate_and_register_entry_adapters(interface, info, contributors,
-                                         repository=None):
+def contribute_to_interface(interface, info):
+    tag = interface.getTaggedValue(LAZR_WEBSERVICE_EXPORTED)
+    for iface in tag['contributes_to']:
+        if iface.queryTaggedValue(LAZR_WEBSERVICE_EXPORTED) is None:
+            raise AttemptToContributeToNonExportedInterface(
+                "Interface %s contributes to %s, which is not "
+                "exported." % (interface.__name__, iface.__name__))
+
+        # Check that no names conflict with the extended interface or
+        # its contributors.
+        contributors = REGISTERED_CONTRIBUTORS[iface]
+        names = {}
+        for iface in itertools.chain([iface, interface], contributors):
+            for name, f in iface.namesAndDescriptions(all=True):
+                if f.queryTaggedValue(LAZR_WEBSERVICE_EXPORTED) is not None:
+                    L = names.setdefault(name, [])
+                    L.append(iface)
+        for name, interfaces in names.items():
+            if len(interfaces) > 1:
+                raise ConflictInContributingInterfaces(name, interfaces)
+        contributors.append(interface)
+
+
+def generate_and_register_entry_adapters(interface, info, repository=None):
     """Generate an entry adapter for every version of the web service.
 
     This code generates an IEntry subinterface for every version, each
@@ -75,6 +115,7 @@
     # Get the list of versions.
     config = getUtility(IWebServiceConfiguration)
     versions = list(config.active_versions)
+    contributors = REGISTERED_CONTRIBUTORS[interface]
 
     # Generate an interface and an adapter for every version.
     web_interfaces = generate_entry_interfaces(
@@ -208,57 +249,6 @@
             in inspect.getmembers(module, _is_exported_interface))
 
 
-def find_interfaces_and_contributors(module):
-    """Find the interfaces and its contributors marked for export.
-
-    Return a dictionary with interfaces as keys and their contributors as
-    values.
-    """
-    interfaces_with_contributors = {}
-    for interface in find_exported_interfaces(module):
-        if issubclass(interface, Exception):
-            # Exceptions can't have interfaces, so just store it in
-            # interfaces_with_contributors and move on.
-            interfaces_with_contributors.setdefault(interface, [])
-            continue
-
-        tag = interface.getTaggedValue(LAZR_WEBSERVICE_EXPORTED)
-        if tag.get('contributes_to'):
-            # Append this interface (which is a contributing interface) to the
-            # list of contributors of every interface it contributes to.
-            for iface in tag['contributes_to']:
-                if iface.queryTaggedValue(LAZR_WEBSERVICE_EXPORTED) is None:
-                    raise AttemptToContributeToNonExportedInterface(
-                        "Interface %s contributes to %s, which is not "
-                        "exported." % (interface.__name__, iface.__name__))
-                    raise AssertionError('foo')
-                contributors = interfaces_with_contributors.setdefault(
-                    iface, [])
-                contributors.append(interface)
-        else:
-            # This is a regular interface, but one of its contributing
-            # interfaces may have been processed previously and in that case a
-            # key for it would already exist in interfaces_with_contributors;
-            # that's why we use setdefault.
-            interfaces_with_contributors.setdefault(interface, [])
-
-    # For every exported interface, check that none of its names are exported
-    # in more than one contributing interface.
-    for interface, contributors in interfaces_with_contributors.items():
-        if len(contributors) == 0:
-            continue
-        names = {}
-        for iface in itertools.chain([interface], contributors):
-            for name, f in iface.namesAndDescriptions(all=True):
-                if f.queryTaggedValue(LAZR_WEBSERVICE_EXPORTED) is not None:
-                    L = names.setdefault(name, [])
-                    L.append(iface)
-        for name, interfaces in names.items():
-            if len(interfaces) > 1:
-                raise ConflictInContributingInterfaces(name, interfaces)
-    return interfaces_with_contributors
-
-
 class ConflictInContributingInterfaces(Exception):
     """More than one interface tried to contribute a given attribute/method to
     another interface.
@@ -287,21 +277,32 @@
     if not inspect.ismodule(module):
         raise TypeError("module attribute must be a module: %s, %s" %
                         module, type(module))
-    interfaces_with_contributors = find_interfaces_and_contributors(module)
 
-    for interface, contributors in interfaces_with_contributors.items():
+    for interface in find_exported_interfaces(module):
         if issubclass(interface, Exception):
             register_exception_view(context, interface)
             continue
 
         tag = interface.getTaggedValue(LAZR_WEBSERVICE_EXPORTED)
         if tag['type'] == ENTRY_TYPE:
-            context.action(
-                discriminator=('webservice entry interface', interface),
-                    callable=generate_and_register_entry_adapters,
-                    args=(interface, context.info, contributors,
-                          REGISTERED_ENTRIES),
-                    )
+            if tag.get('contributes_to'):
+                # This isn't a standalone entry, so just register its
+                # contributions in the global map. This has to be done
+                # before entry and operation adapter generation, so we
+                # customise the order.
+                context.action(
+                    discriminator=('webservice entry contributor', interface),
+                        callable=contribute_to_interface,
+                        args=(interface, context.info),
+                        order=1,
+                        )
+            else:
+                context.action(
+                    discriminator=('webservice entry interface', interface),
+                        callable=generate_and_register_entry_adapters,
+                        args=(interface, context.info, REGISTERED_ENTRIES),
+                        order=2,
+                        )
         elif tag['type'] == COLLECTION_TYPE:
             for version in tag['collection_default_content'].keys():
                 factory = generate_collection_adapter(interface, version)
@@ -318,8 +319,9 @@
             raise AssertionError('Unknown export type: %s' % tag['type'])
         context.action(
             discriminator=('webservice versioned operations', interface),
-            args=(context, interface, contributors, REGISTERED_OPERATIONS),
-            callable=generate_and_register_webservice_operations)
+            args=(context, interface, REGISTERED_OPERATIONS),
+            callable=generate_and_register_webservice_operations,
+            order=2)
 
 @adapter(IProcessStarting)
 def webservice_sanity_checks(registration):
@@ -448,7 +450,7 @@
 
 
 def generate_and_register_webservice_operations(
-        context, interface, contributors, repository=None):
+        context, interface, repository=None):
     """Create and register adapters for all exported methods.
 
     Different versions of the web service may publish the same
@@ -470,7 +472,7 @@
             block_mutator_operations_as_of_version = None
 
     methods = interface.namesAndDescriptions(True)
-    for iface in contributors:
+    for iface in REGISTERED_CONTRIBUTORS[interface]:
         methods.extend(iface.namesAndDescriptions(True))
 
     for name, method in methods:

=== modified file 'src/lazr/restful/testing/helpers.py'
--- src/lazr/restful/testing/helpers.py	2011-03-24 21:18:07 +0000
+++ src/lazr/restful/testing/helpers.py	2014-03-20 01:50:54 +0000
@@ -26,11 +26,6 @@
 
 def register_test_module(name, *contents):
     new_module = create_test_module(name, *contents)
-    # Clear out any registrations from other imports that weren't
-    # sanity-checked.
-    metazcml.REGISTERED_OPERATIONS = []
-    metazcml.REGISTERED_ENTRIES = []
-
     try:
         xmlconfig.string("""
             <configure

=== modified file 'src/lazr/restful/tests/test_declarations.py'
--- src/lazr/restful/tests/test_declarations.py	2011-07-27 13:27:33 +0000
+++ src/lazr/restful/tests/test_declarations.py	2014-03-20 01:50:54 +0000
@@ -58,7 +58,7 @@
 from lazr.restful.metazcml import (
     AttemptToContributeToNonExportedInterface,
     ConflictInContributingInterfaces,
-    find_interfaces_and_contributors,
+    contribute_to_interface,
     generate_and_register_webservice_operations,
     )
 from lazr.restful._resource import (
@@ -277,11 +277,10 @@
     def test_duplicate_contributed_attributes(self):
         # We do not allow a given attribute to be contributed to a given
         # interface by more than one contributing interface.
-        testmod = create_test_module(
-            'testmod', IBranch, IProduct, IHasBugs, IHasBugs2)
+        contribute_to_interface(IHasBugs, {})
         self.assertRaises(
             ConflictInContributingInterfaces,
-            find_interfaces_and_contributors, testmod)
+            contribute_to_interface, IHasBugs2, {})
 
     def test_contributing_interface_not_exported(self):
         # Contributing interfaces are not exported by themselves -- they only
@@ -301,18 +300,17 @@
         class IContributor(Interface):
             export_as_webservice_entry(contributes_to=[INotExported])
             title = exported(TextLine(title=u'The project title'))
-        testmod = create_test_module('testmod', IContributor, INotExported)
         self.assertRaises(
             AttemptToContributeToNonExportedInterface,
-            find_interfaces_and_contributors, testmod)
+            contribute_to_interface, IContributor, {})
 
     def test_duplicate_contributed_methods(self):
         # We do not allow a given method to be contributed to a given
         # interface by more than one contributing interface.
-        testmod = create_test_module('testmod', IProduct, IHasBugs, IHasBugs3)
+        contribute_to_interface(IHasBugs, {})
         self.assertRaises(
             ConflictInContributingInterfaces,
-            find_interfaces_and_contributors, testmod)
+            contribute_to_interface, IHasBugs3, {})
 
     def test_ConflictInContributingInterfaces(self):
         # The ConflictInContributingInterfaces exception states what are the