Merge lp:~cjwatson/lazr.restful/exported-interfaces-honour-all into lp:lazr.restful

[Colin Watson]

Restrict find_exported_interfaces to names that would ordinarily be considered to be exported from a module.

find_exported_interfaces considers all names in a module (subject to _is_exported_interface, which only checks webservice-specific properties), and doesn't honour __all__. This produces some weird effects. For instance, in Launchpad, I tried to move IncompatibleArguments to lp.app.errors and therefore needed to add a webservice:register declaration for lp.app.errors; but this failed because lp.registry.errors imports NameLookupFailed from lp.app.errors in order to subclass it, and this is considered to be exported for webservice purposes even though it's not listed in __all__.

I think this is clearly a bug, although upgrading lazr.restful to a fixed version may require some minor adjustments in application code. We should probably bump the next version to 0.20.0 to indicate that.

As for the particular policy implemented here, I used the rules for public names given in https://docs.python.org/2/reference/simple_stmts.html#import.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'src/lazr/restful/metazcml.py'
--- src/lazr/restful/metazcml.py	2015-04-08 20:11:29 +0000
+++ src/lazr/restful/metazcml.py	2016-11-26 01:04:38 +0000
@@ -245,8 +245,14 @@
 
     :return: iterator of interfaces.
     """
+    try:
+        module_all = set(module.__all__)
+        is_exported_name = lambda name: name in module_all
+    except AttributeError:
+        is_exported_name = lambda name: not name.startswith('_')
     return (interface for name, interface
-            in inspect.getmembers(module, _is_exported_interface))
+            in inspect.getmembers(module, _is_exported_interface)
+            if is_exported_name(name))
 
 
 class ConflictInContributingInterfaces(Exception):
 
=== modified file 'src/lazr/restful/tests/test_declarations.py'
--- src/lazr/restful/tests/test_declarations.py	2016-02-17 01:07:21 +0000
+++ src/lazr/restful/tests/test_declarations.py	2016-11-26 01:04:38 +0000
@@ -2,6 +2,7 @@
 
 """Unit tests for the conversion of interfaces into a web service."""
 
+import testtools
 from zope.component import (
     adapter,
     getMultiAdapter,
@@ -31,6 +32,7 @@
 from lazr.restful.declarations import (
     accessor_for,
     call_with,
+    error_status,
     export_as_webservice_entry,
     exported,
     export_read_operation,
@@ -58,6 +60,7 @@
     AttemptToContributeToNonExportedInterface,
     ConflictInContributingInterfaces,
     contribute_to_interface,
+    find_exported_interfaces,
     generate_and_register_webservice_operations,
     )
 from lazr.restful._resource import (
@@ -328,6 +331,67 @@
             'product', EntryAdapterUtility(adapter.__class__).singular_type)
 
 
+class NotExportedException(Exception):
+    pass
+
+
+@error_status(400)
+class ExportedExceptionOne(Exception):
+    pass
+
+
+@error_status(400)
+class ExportedExceptionTwo(Exception):
+    pass
+
+
+class INotExported(Interface):
+    pass
+
+
+class IExported(Interface):
+    export_as_webservice_entry()
+
+
+class TestFindExportedInterfaces(testtools.TestCase):
+    """Tests for find_exported_interfaces."""
+
+    def assertExportsNames(self, module, names):
+        self.assertEqual(
+            sorted(names),
+            sorted(
+                iface.__name__ for iface in find_exported_interfaces(module)))
+
+    def test_finds_exported_exceptions(self):
+        class Module(object):
+            NotExportedException_copy = NotExportedException
+            ExportedExceptionOne_copy = ExportedExceptionOne
+
+        self.assertExportsNames(Module, ['ExportedExceptionOne'])
+
+    def test_finds_exported_interfaces(self):
+        class Module(object):
+            INotExported_copy = INotExported
+            IExported_copy = IExported
+
+        self.assertExportsNames(Module, ['IExported'])
+
+    def test_honours_all(self):
+        class Module(object):
+            __all__ = ['ExportedExceptionOne_copy']
+            ExportedExceptionOne_copy = ExportedExceptionOne
+            ExportedExceptionTwo_copy = ExportedExceptionTwo
+
+        self.assertExportsNames(Module, ['ExportedExceptionOne'])
+
+    def test_skips_leading_underscore_without_all(self):
+        class Module(object):
+            ExportedExceptionOne_copy = ExportedExceptionOne
+            _ExportedExceptionTwo_copy = ExportedExceptionTwo
+
+        self.assertExportsNames(Module, ['ExportedExceptionOne'])
+
+
 class IProduct(Interface):
     export_as_webservice_entry()
     title = exported(TextLine(title=u'The product title'))