Merge lp:~leonardr/lazr.restful/enforce-version-order into lp:lazr.restful

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
Reviewer Review Type Date Requested Status
Paul Hummer (community) Approve
Review via email: mp+18223@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Leonard Richardson (leonardr) wrote :

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")
...
@operation_for_version("1.0")
...
@operation_for_version("beta")

This is wrong:

@operation_for_version("2.0")
...
@operation_for_version("beta")
...
@operation_for_version("1.0")

The ensure_correct_version_ordering function looks up the canonical version ordering in the IWebServiceConfiguration 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 IWebServiceConfiguration 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!
@operation_removed_in_version("1.0")
@export_as_read_operation()

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()
@export_operation_as("its_redefined_in_1_0") <--This looks stupid, but turns out to be valid!
@operation_removed_in_version("1.0")
@export_as_read_operation()

This set of annotations is sloppier than this one:

@export_operation_as("its_redefined_in_1_0") <--This is clearly valid!
@export_as_read_operation()
@operation_removed_in_version("1.0")
@export_as_read_operation()

But that's not worth raising an error about.

Revision history for this message
Leonard Richardson (leonardr) wrote :

I should mention that I refactored some code in webservice-declarations.txt that creates a module out of web service classes and then runs lazr.restful's ZCML processing on that module, so that I could test these two cases.

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

Subscribers

People subscribed via source and target branches