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

Proposed by Colin Watson on 2016-11-26
Status: Merged
Merged at revision: 214
Proposed branch: lp:~cjwatson/lazr.restful/exported-interfaces-honour-all
Merge into: lp:lazr.restful
Diff against target: 115 lines (+71/-1)
2 files modified
src/lazr/restful/metazcml.py (+7/-1)
src/lazr/restful/tests/test_declarations.py (+64/-0)
To merge this branch: bzr merge lp:~cjwatson/lazr.restful/exported-interfaces-honour-all
Reviewer Review Type Date Requested Status
William Grant code 2016-11-26 Approve on 2016-11-29
Review via email: mp+311867@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
William Grant (wgrant) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/lazr/restful/metazcml.py'
2--- src/lazr/restful/metazcml.py 2015-04-08 20:11:29 +0000
3+++ src/lazr/restful/metazcml.py 2016-11-26 01:04:38 +0000
4@@ -245,8 +245,14 @@
5
6 :return: iterator of interfaces.
7 """
8+ try:
9+ module_all = set(module.__all__)
10+ is_exported_name = lambda name: name in module_all
11+ except AttributeError:
12+ is_exported_name = lambda name: not name.startswith('_')
13 return (interface for name, interface
14- in inspect.getmembers(module, _is_exported_interface))
15+ in inspect.getmembers(module, _is_exported_interface)
16+ if is_exported_name(name))
17
18
19 class ConflictInContributingInterfaces(Exception):
20
21=== modified file 'src/lazr/restful/tests/test_declarations.py'
22--- src/lazr/restful/tests/test_declarations.py 2016-02-17 01:07:21 +0000
23+++ src/lazr/restful/tests/test_declarations.py 2016-11-26 01:04:38 +0000
24@@ -2,6 +2,7 @@
25
26 """Unit tests for the conversion of interfaces into a web service."""
27
28+import testtools
29 from zope.component import (
30 adapter,
31 getMultiAdapter,
32@@ -31,6 +32,7 @@
33 from lazr.restful.declarations import (
34 accessor_for,
35 call_with,
36+ error_status,
37 export_as_webservice_entry,
38 exported,
39 export_read_operation,
40@@ -58,6 +60,7 @@
41 AttemptToContributeToNonExportedInterface,
42 ConflictInContributingInterfaces,
43 contribute_to_interface,
44+ find_exported_interfaces,
45 generate_and_register_webservice_operations,
46 )
47 from lazr.restful._resource import (
48@@ -328,6 +331,67 @@
49 'product', EntryAdapterUtility(adapter.__class__).singular_type)
50
51
52+class NotExportedException(Exception):
53+ pass
54+
55+
56+@error_status(400)
57+class ExportedExceptionOne(Exception):
58+ pass
59+
60+
61+@error_status(400)
62+class ExportedExceptionTwo(Exception):
63+ pass
64+
65+
66+class INotExported(Interface):
67+ pass
68+
69+
70+class IExported(Interface):
71+ export_as_webservice_entry()
72+
73+
74+class TestFindExportedInterfaces(testtools.TestCase):
75+ """Tests for find_exported_interfaces."""
76+
77+ def assertExportsNames(self, module, names):
78+ self.assertEqual(
79+ sorted(names),
80+ sorted(
81+ iface.__name__ for iface in find_exported_interfaces(module)))
82+
83+ def test_finds_exported_exceptions(self):
84+ class Module(object):
85+ NotExportedException_copy = NotExportedException
86+ ExportedExceptionOne_copy = ExportedExceptionOne
87+
88+ self.assertExportsNames(Module, ['ExportedExceptionOne'])
89+
90+ def test_finds_exported_interfaces(self):
91+ class Module(object):
92+ INotExported_copy = INotExported
93+ IExported_copy = IExported
94+
95+ self.assertExportsNames(Module, ['IExported'])
96+
97+ def test_honours_all(self):
98+ class Module(object):
99+ __all__ = ['ExportedExceptionOne_copy']
100+ ExportedExceptionOne_copy = ExportedExceptionOne
101+ ExportedExceptionTwo_copy = ExportedExceptionTwo
102+
103+ self.assertExportsNames(Module, ['ExportedExceptionOne'])
104+
105+ def test_skips_leading_underscore_without_all(self):
106+ class Module(object):
107+ ExportedExceptionOne_copy = ExportedExceptionOne
108+ _ExportedExceptionTwo_copy = ExportedExceptionTwo
109+
110+ self.assertExportsNames(Module, ['ExportedExceptionOne'])
111+
112+
113 class IProduct(Interface):
114 export_as_webservice_entry()
115 title = exported(TextLine(title=u'The product title'))

Subscribers

People subscribed via source and target branches