Merge lp:~leonardr/lazr.restful/enforce-version-order into lp:lazr.restful
- enforce-version-order
- Merge into trunk
Proposed by
Leonard Richardson
Status: | Merged |
---|---|
Merged at revision: | not available |
Proposed branch: | lp:~leonardr/lazr.restful/enforce-version-order |
Merge into: | lp:lazr.restful |
Diff against target: |
256 lines (+159/-24) 3 files modified
src/lazr/restful/docs/webservice-declarations.txt (+85/-22) src/lazr/restful/metazcml.py (+72/-0) src/lazr/restful/simple.py (+2/-2) |
To merge this branch: | bzr merge lp:~leonardr/lazr.restful/enforce-version-order |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Paul Hummer (community) | Approve | ||
Review via email: mp+18223@code.launchpad.net |
Commit message
Description of the change
To post a comment you must log in.
Revision history for this message
Leonard Richardson (leonardr) wrote : | # |
Revision history for this message
Leonard Richardson (leonardr) wrote : | # |
I should mention that I refactored some code in webservice-
- 108. By Leonard Richardson
-
Clarified explanation in doctest.:
- 109. By Leonard Richardson
-
Changed 'after' to 'on top' to fit better with the stack metaphor.
Revision history for this message
Paul Hummer (rockstar) wrote : | # |
Hi Leonard-
Sorry this took so long. I ended up having to look at a lot more code than what is in the patch. This lazr.restful stuff is a bit more involved than I thought. These changes all look good though. Please land it.
Cheers,
Paul
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'src/lazr/restful/docs/webservice-declarations.txt' |
2 | --- src/lazr/restful/docs/webservice-declarations.txt 2010-01-28 14:21:19 +0000 |
3 | +++ src/lazr/restful/docs/webservice-declarations.txt 2010-01-28 18:11:15 +0000 |
4 | @@ -888,7 +888,7 @@ |
5 | >>> class MyWebServiceConfiguration: |
6 | ... implements(IWebServiceConfiguration) |
7 | ... view_permission = "lazr.View" |
8 | - ... active_versions = ["beta"] |
9 | + ... active_versions = ["beta", "1.0", "2.0", "3.0"] |
10 | ... code_revision = "1.0b" |
11 | ... default_batch_size = 50 |
12 | ... latest_version_uri_prefix = 'beta' |
13 | @@ -1855,11 +1855,7 @@ |
14 | <lazr.restful.fields.CollectionField object...> |
15 | |
16 | [XXX leonardr mutator_for modifies the field, not the method, so it |
17 | -won't work until I add multiversion support for fields. Also, it's |
18 | -possible to remove a named operation from a certain version and then |
19 | -uselessly annotate it some more. The best place to catch this error |
20 | -turns out to be when generating the adapter classes, not when |
21 | -collecting annotations.] |
22 | +won't work until I add multiversion support for fields.] |
23 | |
24 | Security |
25 | ======== |
26 | @@ -1895,7 +1891,6 @@ |
27 | zope.security._proxy._Proxy (using zope.security.checker.Checker) |
28 | public: __call__, send_modification_event |
29 | |
30 | -================= |
31 | ZCML Registration |
32 | ================= |
33 | |
34 | @@ -1908,23 +1903,34 @@ |
35 | |
36 | >>> import sys |
37 | >>> from types import ModuleType |
38 | - >>> bookexample = ModuleType('bookexample') |
39 | - >>> sys.modules['lazr.restful.bookexample'] = bookexample |
40 | - >>> bookexample.IBook = IBook |
41 | - >>> bookexample.IBookSet = IBookSet |
42 | - >>> bookexample.IBookOnSteroids = IBookOnSteroids |
43 | - >>> bookexample.IBookSetOnSteroids = IBookSetOnSteroids |
44 | - >>> bookexample.ISimpleComment = ISimpleComment |
45 | - >>> bookexample.InvalidEmail = InvalidEmail |
46 | + >>> def create_test_module(name, *contents): |
47 | + ... """Defines a new module and adds it to sys.modules.""" |
48 | + ... new_module = ModuleType(name) |
49 | + ... sys.modules['lazr.restful.' + name] = new_module |
50 | + ... for object in contents: |
51 | + ... setattr(new_module, object.__name__, object) |
52 | + ... return new_module |
53 | |
54 | >>> from zope.configuration import xmlconfig |
55 | - >>> zcmlcontext = xmlconfig.string(""" |
56 | - ... <configure |
57 | - ... xmlns:webservice="http://namespaces.canonical.com/webservice"> |
58 | - ... <include package="lazr.restful" file="meta.zcml" /> |
59 | - ... <webservice:register module="lazr.restful.bookexample" /> |
60 | - ... </configure> |
61 | - ... """) |
62 | + >>> def register_test_module(name, *contents): |
63 | + ... new_module = create_test_module(name, *contents) |
64 | + ... try: |
65 | + ... zcmlcontext = xmlconfig.string(""" |
66 | + ... <configure |
67 | + ... xmlns:webservice= |
68 | + ... "http://namespaces.canonical.com/webservice"> |
69 | + ... <include package="lazr.restful" file="meta.zcml" /> |
70 | + ... <webservice:register module="lazr.restful.%s" /> |
71 | + ... </configure> |
72 | + ... """ % name) |
73 | + ... except Exception, e: |
74 | + ... del sys.modules['lazr.restful.' + name] |
75 | + ... raise e |
76 | + ... return new_module |
77 | + |
78 | + >>> bookexample = register_test_module( |
79 | + ... 'bookexample', IBook, IBookSet, IBookOnSteroids, |
80 | + ... IBookSetOnSteroids, ISimpleComment, InvalidEmail) |
81 | |
82 | After the registration, adapters from IBook to IEntry, and IBookSet to |
83 | ICollection are available: |
84 | @@ -1980,3 +1986,60 @@ |
85 | |
86 | >>> del bookexample |
87 | >>> del sys.modules['lazr.restful.bookexample'] |
88 | + |
89 | +Error handling |
90 | +-------------- |
91 | + |
92 | +Some error handling happens in the ZCML registration phase. At this |
93 | +point, all the annotations have been processed, and the |
94 | +IWebServiceConfiguration utility (with its canonical list of versions) |
95 | +has become available. This lets us run checks on the versioning |
96 | +annotations that couldn't be run before. |
97 | + |
98 | +Here's a class annotated by someone who believes that version 1.0 of |
99 | +the web service is a later version than version 2.0. (Or who believes |
100 | +that named operation annotations proceed from the top down rather than |
101 | +the bottom up.) |
102 | + |
103 | + >>> class WrongOrderVersions(Interface): |
104 | + ... export_as_webservice_entry() |
105 | + ... @export_operation_as('10_name') |
106 | + ... @operation_for_version("1.0") |
107 | + ... @operation_parameters(arg=Float()) |
108 | + ... @export_read_operation() |
109 | + ... @operation_for_version("2.0") |
110 | + ... def method(arg): |
111 | + ... """A method.""" |
112 | + |
113 | +An attempt to register this module with ZCML results in an error |
114 | +explaining the problem. |
115 | + |
116 | + >>> register_test_module('wrongorder', WrongOrderVersions) |
117 | + Traceback (most recent call last): |
118 | + ... |
119 | + ConfigurationExecutionError: <type 'exceptions.AssertionError'>: |
120 | + Annotations on "WrongOrderVersions.method" put an earlier version |
121 | + on top of a later version: "beta", "2.0", "1.0". The correct order |
122 | + is: "beta", "1.0", "2.0". ... |
123 | + |
124 | +Here's a class in which a named operation is removed in version 1.0 |
125 | +and then annotated without being reinstated. |
126 | + |
127 | + >>> class AnnotatingARemovedMethod(Interface): |
128 | + ... export_as_webservice_entry() |
129 | + ... @operation_parameters(arg=TextLine()) |
130 | + ... @export_operation_as('already_been_removed') |
131 | + ... @operation_removed_in_version("1.0") |
132 | + ... @operation_parameters(arg=Float()) |
133 | + ... @export_read_operation() |
134 | + ... @operation_for_version("2.0") |
135 | + ... def method(arg): |
136 | + ... """A method.""" |
137 | + |
138 | + >>> register_test_module('annotatingremoved', AnnotatingARemovedMethod) |
139 | + Traceback (most recent call last): |
140 | + ... |
141 | + ZopeXMLConfigurationError: ... |
142 | + AssertionError: Method "method" contains annotations for version |
143 | + "1.0", even though it's not published in that version. The bad |
144 | + annotations are: "params", "as". |
145 | |
146 | === modified file 'src/lazr/restful/metazcml.py' |
147 | --- src/lazr/restful/metazcml.py 2010-01-27 17:33:32 +0000 |
148 | +++ src/lazr/restful/metazcml.py 2010-01-28 18:11:15 +0000 |
149 | @@ -34,6 +34,50 @@ |
150 | title=u'Module which will be inspected for webservice declarations') |
151 | |
152 | |
153 | +def ensure_correct_version_ordering(name, version_list): |
154 | + """Make sure that a list mentions versions from earliest to latest. |
155 | + |
156 | + If an earlier version shows up after a later version, this is a |
157 | + sign that a developer was confused about version names when |
158 | + annotating the web service. |
159 | + |
160 | + :param name: The name of the object annotated with the |
161 | + possibly mis-ordered versions. |
162 | + |
163 | + :param version_list: The list of versions found in the interface. |
164 | + |
165 | + :raise AssertionError: If the given version list is not a |
166 | + earlier-to-later ordering of a subset of the web service's |
167 | + versions. |
168 | + """ |
169 | + configuration = getUtility(IWebServiceConfiguration) |
170 | + actual_versions = configuration.active_versions + [ |
171 | + configuration.latest_version_uri_prefix] |
172 | + # Replace None with the actual version number of the earliest |
173 | + # version. |
174 | + try: |
175 | + earliest_version_pos = version_list.index(None) |
176 | + version_list = list(version_list) |
177 | + version_list[earliest_version_pos] = actual_versions[0] |
178 | + except ValueError: |
179 | + # The earliest version was not mentioned in the version list. |
180 | + # Do nothing. |
181 | + pass |
182 | + |
183 | + # Sort version_list according to the list of actual versions. |
184 | + # If the sorted list is different from the unsorted list, at |
185 | + # least one version is out of place. |
186 | + def compare(x, y): |
187 | + return cmp (actual_versions.index(x), actual_versions.index(y)) |
188 | + sorted_version_list = sorted(version_list, cmp=compare) |
189 | + if sorted_version_list != version_list: |
190 | + bad_versions = '", "'.join(version_list) |
191 | + good_versions = '", "'.join(sorted_version_list) |
192 | + msg = ('Annotations on "%s" put an earlier version on top of a ' |
193 | + 'later version: "%s". The correct order is: "%s".') |
194 | + raise AssertionError, msg % (name, bad_versions, good_versions) |
195 | + |
196 | + |
197 | def register_adapter_for_version(factory, interface, version_name, |
198 | provides, name, info): |
199 | """A version-aware wrapper for the registerAdapter operation. |
200 | @@ -147,6 +191,14 @@ |
201 | # This method is not published as a named operation. |
202 | continue |
203 | |
204 | + # First, make sure that this method was annotated with the |
205 | + # versions in the right order. |
206 | + context.action( |
207 | + discriminator=('webservice version ordering', method), |
208 | + callable=ensure_correct_version_ordering, |
209 | + args=(interface.__name__ + '.' + method.__name__, tag.dict_names) |
210 | + ) |
211 | + |
212 | operation_name = None |
213 | # If an operation's name does not change between version n and |
214 | # version n+1, we want lookups for requests that come in for |
215 | @@ -175,6 +227,26 @@ |
216 | operation_name = None |
217 | operation_provides = None |
218 | factory = None |
219 | + |
220 | + # If there are any other tags besides 'type', it means |
221 | + # that the developer tried to annotate a method for a |
222 | + # version where the method isn't published. Let's warn |
223 | + # them about it. |
224 | + # |
225 | + # (We can't make this check when the annotation |
226 | + # happens, because it will reject a method that uses |
227 | + # an annotation like @export_operation_as before using |
228 | + # an annotation like @export_read_operation. That's a |
229 | + # little sloppy, but it's not bad enough to warrant an |
230 | + # exception.) |
231 | + tag_names = list(tag.keys()) |
232 | + if tag_names != ['type']: |
233 | + tag_names.remove('type') |
234 | + raise AssertionError( |
235 | + 'Method "%s" contains annotations for version "%s", ' |
236 | + 'even though it\'s not published in that version. ' |
237 | + 'The bad annotations are: "%s".' % ( |
238 | + method.__name__, version, '", "'.join(tag_names))) |
239 | else: |
240 | if tag['type'] == 'read_operation': |
241 | operation_provides = IResourceGETOperation |
242 | |
243 | === modified file 'src/lazr/restful/simple.py' |
244 | --- src/lazr/restful/simple.py 2010-01-12 15:06:15 +0000 |
245 | +++ src/lazr/restful/simple.py 2010-01-28 18:11:15 +0000 |
246 | @@ -32,8 +32,8 @@ |
247 | |
248 | from lazr.restful import EntryAdapterUtility, ServiceRootResource |
249 | from lazr.restful.interfaces import ( |
250 | - IServiceRootResource, ITraverseWithGet, IWebServiceConfiguration, |
251 | - IWebServiceLayer) |
252 | + IServiceRootResource, ITopLevelEntryLink, ITraverseWithGet, |
253 | + IWebServiceConfiguration, IWebServiceLayer) |
254 | from lazr.restful.publisher import ( |
255 | WebServicePublicationMixin, WebServiceRequestTraversal) |
256 | from lazr.restful.utils import implement_from_dict |
This branch adds two bits of multiversion- related error checking.
1. Named operations are annotated with a stack of version information. The most recent version is supposed to be at the top of the stack.
This is right:
@operation_ for_version( "2.0") for_version( "1.0") for_version( "beta")
...
@operation_
...
@operation_
This is wrong:
@operation_ for_version( "2.0") for_version( "beta") for_version( "1.0")
...
@operation_
...
@operation_
The ensure_ correct_ version_ ordering function looks up the canonical version ordering in the IWebServiceConf iguration implementation, and makes sure every named operation is annotated with a consistent ordering. If the developer makes a mistake, ensure_ correct_ version_ ordering raises an AssertionError explaining the problem and giving the correct ordering.
This code has to run in the second stage of ZCML processing, rather than when the annotations are initially processed, because until the IWebServiceConf iguration utility becomes available, there's no way to know whether version "foo" comes before or after version "bar".
2. If a named operation is not published in a certain version, it makes no sense to annotate it for that version.
@export_ operation_ as("its_ gone_in_ 1_0") <--This annotation is stupid! removed_ in_version( "1.0") as_read_ operation( )
@operation_
@export_
The register_ webservice_ operations method will now make sure there are no extra annotations for a version in which a named operation is not published. If there are any extra annotations, it will raise an AssertionError and complain about them.
This code has to run in the second stage of ZCML processing because by then all the annotations have been processed. If I make each annotations in charge of complaining when it's applied to an unpublished method, it'll reject cases in which the method seems unpublished at the time but gets published later.
@export_ as_read_ operation( ) operation_ as("its_ redefined_ in_1_0" ) <--This looks stupid, but turns out to be valid! removed_ in_version( "1.0") as_read_ operation( )
@export_
@operation_
@export_
This set of annotations is sloppier than this one:
@export_ operation_ as("its_ redefined_ in_1_0" ) <--This is clearly valid! as_read_ operation( ) removed_ in_version( "1.0") as_read_ operation( )
@export_
@operation_
@export_
But that's not worth raising an error about.