Merge lp:~leonardr/lazr.restful/generate-multiversion-collections into lp:lazr.restful

Proposed by Leonard Richardson
Status: Merged
Approved by: Edwin Grubbs
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~leonardr/lazr.restful/generate-multiversion-collections
Merge into: lp:lazr.restful
Diff against target: 450 lines (+210/-63)
6 files modified
src/lazr/restful/declarations.py (+41/-15)
src/lazr/restful/docs/webservice-declarations.txt (+99/-12)
src/lazr/restful/example/multiversion/resources.py (+10/-1)
src/lazr/restful/example/multiversion/root.py (+2/-1)
src/lazr/restful/example/multiversion/tests/introduction.txt (+27/-7)
src/lazr/restful/metazcml.py (+31/-27)
To merge this branch: bzr merge lp:~leonardr/lazr.restful/generate-multiversion-collections
Reviewer Review Type Date Requested Status
Edwin Grubbs (community) code Approve
Review via email: mp+18153@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Leonard Richardson (leonardr) wrote :

This branch makes the @collection_default_content annotation
version-aware. For different versions of the web service you can
specify different methods to be used as the collection's find()
implementation, and different values for that method's keyword
arguments.

I was able to refactor some code because this is the second branch
that makes our code generation version-aware. I also simplified the
implementation of register_adapter_for_version. Previously if the
version in question was None (indicating the earliest version), I
looked up which version was the earliest and then looked up its
version-specific marker interface. Now I simply use the generic web
service request interface, IWebServiceClientRequest. This prevents
test failures where all that changed is that a lookup that failed with
IWebServiceClientRequest needed to be changed to
IWebServiceBetaClientRequest.

Here are some questions I asked myself during development. They all had to do
with whether version differences were better solved with annotations
or left to navigation code.

Q: Do I need an analogue to @operation_removed_in_version?
A: No. If a collection disappears in a certain version, it needs to
   disappear from the navigation. Hiding the
   collection_default_content for that version won't solve anything.

   The reason we need @operation_removed_in_version is that lookup of
   named operation happens after the navigation is done. The
   navigation takes you to an entry or collection, and then the named
   operation lookup needs to succeed or fail depending on the version.

   If it turns out to be too much trouble to change the navigation, we
   could create a @collection_removed_in_version and make the
   lazr.restful behavior to raise a 404 instead of calling find(), but
   for now I say YAGNI.

Q: What happens if a collection changes its name between versions?
A: That, too is a navigation issue. And again, we had to deal with it
   specially for named operations because operation lookup is not
   navigation.

Q: What happens if a collection doesn't have a
   @collection_default_content for the earliest version? Don't I
   need to prevent that?
A: That corresponds to a case where the collection didn't exist in the
   earliest version and was added later. This is a legitimate case,
   and it will work fine if accompanied by appropriate navigation
   code.

Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :
Download full text (5.4 KiB)

Hi Leonard,

This is a nice refactoring and improvement. I only have minor comments below.

merge-conditional

-Edwin

>=== modified file 'src/lazr/restful/declarations.py'
>--- src/lazr/restful/declarations.py 2010-01-26 20:05:25 +0000
>+++ src/lazr/restful/declarations.py 2010-01-27 22:49:27 +0000
>@@ -202,7 +202,7 @@
> collection, or if used more than once in the same interface.
> """
>
>- def __init__(self, **params):
>+ def __init__(self, version=None, **params):
> """Create the decorator marking the default collection method.
>
> :param params: Optional parameter values to use when calling the

The docstring should explain how the version parameter should be used.

>@@ -891,20 +898,26 @@
> return method(**params)
>
>
>-def generate_collection_adapter(interface):
>+def generate_collection_adapter(interface, version=None):
> """Create a class adapting from interface to ICollection."""
> _check_tagged_interface(interface, 'collection')
>
> tag = interface.getTaggedValue(LAZR_WEBSERVICE_EXPORTED)
>- method_name = tag['collection_default_content']
>+ default_content_by_version = tag['collection_default_content']
>+ assert (version in default_content_by_version), (
>+ "'%s' isn't tagged for export to web service "
>+ "version '%s'." % (interface.__name__, version))
>+ method_name, params = default_content_by_version[version]
> entry_schema = tag['collection_entry_schema']
> class_dict = {
> 'entry_schema' : CollectionEntrySchema(entry_schema),
>- 'method_name': tag['collection_default_content'],
>- 'params': tag['collection_default_content_params'],
>+ 'method_name': method_name,
>+ 'params': params,
> '__doc__': interface.__doc__,
> }
>- classname = "%sCollectionAdapter" % interface.__name__[1:]
>+ classname =_versioned_class_name(

s/=_/= _/

>+ "%sCollectionAdapter" % interface.__name__[1:],
>+ version)
> factory = type(classname, (BaseCollectionAdapter,), class_dict)
>
> protect_schema(factory, ICollection)
>@@ -1016,8 +1029,9 @@
> if return_type is None:
> return_type = None
>
>- name = '%s_%s_%s_%s' % (prefix, method.interface.__name__, tag['as'],
>- version)
>+ name = _versioned_class_name(
>+ '%s_%s_%s' % (prefix, method.interface.__name__, tag['as']),
>+ version)
> class_dict = {'params' : tuple(tag['params'].values()),
> 'return_type' : return_type,
> '_export_info': tag,
>@@ -1026,8 +1040,20 @@
>
> if tag['type'] == 'write_operation':
> class_dict['send_modification_event'] = True
>- factory = type(make_identifier_safe(name), bases, class_dict)
>+ factory = type(name, bases, class_dict)
> classImplements(factory, provides)
> protect_schema(factory, provides)
>
> return factory
>+
>+
>+def _versioned_class_name(base_name, version):
>+ """Create a class name incorporating the given version string."""
>+ if version is None:
>+ # We need to incorporate the version into a Python class name,
>+ # but we won't find out t...

Read more...

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/declarations.py'
2--- src/lazr/restful/declarations.py 2010-01-26 20:05:25 +0000
3+++ src/lazr/restful/declarations.py 2010-01-27 18:14:15 +0000
4@@ -202,7 +202,7 @@
5 collection, or if used more than once in the same interface.
6 """
7
8- def __init__(self, **params):
9+ def __init__(self, version=None, **params):
10 """Create the decorator marking the default collection method.
11
12 :param params: Optional parameter values to use when calling the
13@@ -218,19 +218,26 @@
14 "@collection_default_content can only be used from within an "
15 "interface exported as a collection.")
16
17- if 'collection_default_content' in tag:
18+ default_content_methods = tag.setdefault(
19+ 'collection_default_content', {})
20+
21+ if version in default_content_methods:
22+ if version is None:
23+ version = "(earliest)"
24 raise TypeError(
25- "only one method should be marked with "
26- "@collection_default_content.")
27-
28- tag['collection_default_content_params'] = params
29+ "Only one method can be marked with "
30+ "@collection_default_content for version %s." % version)
31+ self.version = version
32+ self.params = params
33
34 def __call__(self, f):
35 """Annotates the collection with the name of the method to call."""
36 tag = _get_interface_tags()[LAZR_WEBSERVICE_EXPORTED]
37- tag['collection_default_content'] = f.__name__
38+ tag['collection_default_content'][self.version] = (
39+ f.__name__, self.params)
40 return f
41
42+
43 WEBSERVICE_ERROR = '__lazr_webservice_error__'
44
45 def webservice_error(status):
46@@ -891,20 +898,26 @@
47 return method(**params)
48
49
50-def generate_collection_adapter(interface):
51+def generate_collection_adapter(interface, version=None):
52 """Create a class adapting from interface to ICollection."""
53 _check_tagged_interface(interface, 'collection')
54
55 tag = interface.getTaggedValue(LAZR_WEBSERVICE_EXPORTED)
56- method_name = tag['collection_default_content']
57+ default_content_by_version = tag['collection_default_content']
58+ assert (version in default_content_by_version), (
59+ "'%s' isn't tagged for export to web service "
60+ "version '%s'." % (interface.__name__, version))
61+ method_name, params = default_content_by_version[version]
62 entry_schema = tag['collection_entry_schema']
63 class_dict = {
64 'entry_schema' : CollectionEntrySchema(entry_schema),
65- 'method_name': tag['collection_default_content'],
66- 'params': tag['collection_default_content_params'],
67+ 'method_name': method_name,
68+ 'params': params,
69 '__doc__': interface.__doc__,
70 }
71- classname = "%sCollectionAdapter" % interface.__name__[1:]
72+ classname =_versioned_class_name(
73+ "%sCollectionAdapter" % interface.__name__[1:],
74+ version)
75 factory = type(classname, (BaseCollectionAdapter,), class_dict)
76
77 protect_schema(factory, ICollection)
78@@ -1016,8 +1029,9 @@
79 if return_type is None:
80 return_type = None
81
82- name = '%s_%s_%s_%s' % (prefix, method.interface.__name__, tag['as'],
83- version)
84+ name = _versioned_class_name(
85+ '%s_%s_%s' % (prefix, method.interface.__name__, tag['as']),
86+ version)
87 class_dict = {'params' : tuple(tag['params'].values()),
88 'return_type' : return_type,
89 '_export_info': tag,
90@@ -1026,8 +1040,20 @@
91
92 if tag['type'] == 'write_operation':
93 class_dict['send_modification_event'] = True
94- factory = type(make_identifier_safe(name), bases, class_dict)
95+ factory = type(name, bases, class_dict)
96 classImplements(factory, provides)
97 protect_schema(factory, provides)
98
99 return factory
100+
101+
102+def _versioned_class_name(base_name, version):
103+ """Create a class name incorporating the given version string."""
104+ if version is None:
105+ # We need to incorporate the version into a Python class name,
106+ # but we won't find out the name of the earliest version until
107+ # runtime. Use a generic string that won't conflict with a
108+ # real version string.
109+ version = "__Earliest"
110+ name = "%s_%s" % (base_name, version)
111+ return make_identifier_safe(name)
112
113=== modified file 'src/lazr/restful/docs/webservice-declarations.txt'
114--- src/lazr/restful/docs/webservice-declarations.txt 2010-01-25 19:26:30 +0000
115+++ src/lazr/restful/docs/webservice-declarations.txt 2010-01-27 18:14:15 +0000
116@@ -170,15 +170,14 @@
117 tagged value.
118
119 >>> print_export_tag(IBookSet)
120- collection_default_content: 'getAllBooks'
121- collection_default_content_params: {}
122+ collection_default_content: {None: ('getAllBooks', {})}
123 collection_entry_schema: <InterfaceClass __builtin__.IBook>
124 type: 'collection'
125
126 >>> print_export_tag(ICheckedOutBookSet)
127- collection_default_content: 'getByTitle'
128- collection_default_content_params: {'title': '',
129- 'user': <object...>}
130+ collection_default_content: {None: ('getByTitle',
131+ {'title': '',
132+ 'user': <object object ...>})}
133 collection_entry_schema: <InterfaceClass __builtin__.IBook>
134 type: 'collection'
135
136@@ -223,8 +222,8 @@
137 ... """Another getAll()."""
138 Traceback (most recent call last):
139 ...
140- TypeError: only one method should be marked with
141- @collection_default_content.
142+ TypeError: Only one method can be marked with
143+ @collection_default_content for version (earliest).
144
145 export_as_webservice_collection() can only be used on Interface.
146
147@@ -1452,7 +1451,6 @@
148 It is possible to cache a server response in the browser cache using
149 the @cache_for decorator:
150
151- >>> from lazr.restful.testing.webservice import FakeRequest
152 >>> from lazr.restful.declarations import cache_for
153 >>>
154 >>> class ICachedBookSet(IBookSet):
155@@ -1472,14 +1470,13 @@
156 ...
157 ... def getAllBooks(self):
158 ... return self.books
159- >>>
160- >>> request = FakeRequest()
161+
162 >>> read_method_adapter_factory = generate_operation_adapter(
163 ... ICachedBookSet['getAllBooks'])
164 >>> read_method_adapter = read_method_adapter_factory(
165 ... CachedBookSet(['Cool book']), request)
166 >>> print read_method_adapter.call()
167- ["Cool book"]
168+ ['Cool book']
169 >>> print request.response.headers
170 {'Content-Type': 'application/json', 'Cache-control': 'max-age=60'}
171
172@@ -1509,6 +1506,96 @@
173 Different versions of the webservice can publish the same data model
174 object in totally different ways.
175
176+Collections
177+-----------
178+
179+A collection's contents are determined by calling one of its
180+methods. Which method is called, and with which arguments, can vary
181+across versions.
182+
183+ >>> from lazr.restful.declarations import generate_operation_adapter
184+
185+ >>> class IMultiVersionCollection(Interface):
186+ ... export_as_webservice_collection(Interface)
187+ ...
188+ ... @collection_default_content('2.0')
189+ ... def content_20():
190+ ... """The content method for version 2.0."""
191+ ...
192+ ... @collection_default_content('1.0', argument='1.0 value')
193+ ... @collection_default_content(argument='pre-1.0 value')
194+ ... def content_pre_20(argument):
195+ ... """The content method for versions before 2.0"""
196+
197+Here's a simple implementation of IMultiVersionCollection. It'll
198+illustrate how the different versions of the web service invoke
199+different methods to find the collection contents.
200+
201+ >>> class MultiVersionCollection():
202+ ... """Simple IMultiVersionCollection implementation."""
203+ ... implements(IMultiVersionCollection)
204+ ...
205+ ... def content_20(self):
206+ ... return ["contents", "for", "version", "2.0"]
207+ ...
208+ ... def content_pre_20(self, argument):
209+ ... return ["you", "passed", "in", argument]
210+
211+By passing a version string into generate_collection_adapter(), we can
212+get different adapter classes for different versions of the web
213+service. We'll be invoking each version against the same data model
214+object. Here it is:
215+
216+ >>> data_object = MultiVersionCollection()
217+
218+Passing in None to generate_collection_adapter gets us the collection
219+as it appears in the earliest version of the web service. The
220+content_pre_20() method is invoked with the 'argument' parameter equal
221+to "pre-1.0 value".
222+
223+ >>> interface = IMultiVersionCollection
224+ >>> adapter_earliest_factory = generate_collection_adapter(
225+ ... interface, None)
226+ >>> print adapter_earliest_factory.__name__
227+ MultiVersionCollectionCollectionAdapter___Earliest
228+
229+ >>> collection_earliest = adapter_earliest_factory(data_object, request)
230+ >>> print collection_earliest.find()
231+ ['you', 'passed', 'in', 'pre-1.0 value']
232+
233+Passing in '1.0' gets us the collection as it appears in the 1.0
234+version of the web service. Note that the argument passed in to
235+content_pre_20() is different, and so the returned contents are
236+slightly different.
237+
238+ >>> adapter_10_factory = generate_collection_adapter(interface, '1.0')
239+ >>> print adapter_10_factory.__name__
240+ MultiVersionCollectionCollectionAdapter_1_0
241+
242+ >>> collection_10 = adapter_10_factory(data_object, request)
243+ >>> print collection_10.find()
244+ ['you', 'passed', 'in', '1.0 value']
245+
246+Passing in '2.0' gets us a collection with totally different contents,
247+because a totally different method is being called.
248+
249+ >>> adapter_20_factory = generate_collection_adapter(interface, '2.0')
250+ >>> print adapter_20_factory.__name__
251+ MultiVersionCollectionCollectionAdapter_2_0
252+
253+ >>> collection_20 = adapter_20_factory(data_object, request)
254+ >>> print collection_20.find()
255+ ['contents', 'for', 'version', '2.0']
256+
257+An error occurs when we try to generate an adapter for a version
258+that's not mentioned in the annotations.
259+
260+ >>> generate_collection_adapter(interface, 'NoSuchVersion')
261+ Traceback (most recent call last):
262+ ...
263+ AssertionError: 'IMultiVersionCollection' isn't tagged for export
264+ to web service version 'NoSuchVersion'.
265+
266 Named operations
267 ----------------
268
269@@ -1865,8 +1952,8 @@
270 >>> from zope.component import getGlobalSiteManager, getUtility
271 >>> adapter_registry = getGlobalSiteManager().adapters
272
273- >>> request_interface = getUtility(IWebServiceVersion, name='beta')
274 >>> from lazr.restful.interfaces import IWebServiceClientRequest
275+ >>> request_interface = IWebServiceClientRequest
276 >>> adapter_registry.lookup(
277 ... (IBookSetOnSteroids, request_interface),
278 ... IResourceGETOperation, 'searchBooks')
279
280=== modified file 'src/lazr/restful/example/multiversion/resources.py'
281--- src/lazr/restful/example/multiversion/resources.py 2010-01-21 18:26:58 +0000
282+++ src/lazr/restful/example/multiversion/resources.py 2010-01-27 18:14:15 +0000
283@@ -31,10 +31,17 @@
284 class IPairSet(ILocation):
285 export_as_webservice_collection(IKeyValuePair)
286
287- @collection_default_content()
288+ # In versions 2.0 and 3.0, the collection of key-value pairs
289+ # includes all pairs.
290+ @collection_default_content("2.0")
291 def getPairs():
292 """Return the key-value pairs."""
293
294+ # Before 2.0, it only includes pairs whose values are not None.
295+ @collection_default_content('beta')
296+ def getNonEmptyPairs():
297+ """Return the key-value pairs that don't map to None."""
298+
299 def get(request, name):
300 """Retrieve a key-value pair by its key."""
301
302@@ -59,6 +66,8 @@
303 def find_for_value(self, value):
304 return [pair for pair in self.pairs if value == pair.value]
305
306+ def getNonEmptyPairs(self):
307+ return [pair for pair in self.pairs if pair.value is not None]
308
309 class KeyValuePair(BasicKeyValuePair):
310 implements(IKeyValuePair)
311
312=== modified file 'src/lazr/restful/example/multiversion/root.py'
313--- src/lazr/restful/example/multiversion/root.py 2010-01-21 17:57:02 +0000
314+++ src/lazr/restful/example/multiversion/root.py 2010-01-27 18:14:15 +0000
315@@ -46,7 +46,8 @@
316 pairset = PairSet()
317 pairset.pairs = [
318 KeyValuePair(pairset, "foo", "bar"),
319- KeyValuePair(pairset, "1", "2")
320+ KeyValuePair(pairset, "1", "2"),
321+ KeyValuePair(pairset, "Some", None)
322 ]
323 collections = dict(pairs=(IKeyValuePair, pairset))
324 return collections, {}
325
326=== modified file 'src/lazr/restful/example/multiversion/tests/introduction.txt'
327--- src/lazr/restful/example/multiversion/tests/introduction.txt 2010-01-21 20:07:54 +0000
328+++ src/lazr/restful/example/multiversion/tests/introduction.txt 2010-01-27 18:14:15 +0000
329@@ -71,13 +71,33 @@
330 Collections and entries
331 =======================
332
333-The web service presents a single collection of key-value pairs.
334-
335- >>> body = webservice.get('/pairs').jsonBody()
336- >>> for entry in body['entries']:
337- ... print entry['self_link'], entry['key'], entry['value']
338- http://multiversion.dev/3.0/pairs/foo foo bar
339- http://multiversion.dev/3.0/pairs/1 1 2
340+The web service presents a single collection of key-value pairs. In
341+versions previous to 2.0, the collection omits key-value pairs where
342+the value is None. In 2.0 and 3.0, all key-value pairs are published.
343+
344+ >>> from operator import itemgetter
345+ >>> def show_pairs(version):
346+ ... body = webservice.get('/pairs', api_version=version).jsonBody()
347+ ... for entry in sorted(body['entries'], key=itemgetter('key')):
348+ ... print "%s: %s" % (entry['key'], entry['value'])
349+
350+ >>> show_pairs('beta')
351+ 1: 2
352+ foo: bar
353+
354+ >>> show_pairs('1.0')
355+ 1: 2
356+ foo: bar
357+
358+ >>> show_pairs('2.0')
359+ 1: 2
360+ Some: None
361+ foo: bar
362+
363+ >>> show_pairs('3.0')
364+ 1: 2
365+ Some: None
366+ foo: bar
367
368 >>> body = webservice.get('/pairs/foo').jsonBody()
369 >>> print body['key'], body['value']
370
371=== modified file 'src/lazr/restful/metazcml.py'
372--- src/lazr/restful/metazcml.py 2010-01-25 19:51:11 +0000
373+++ src/lazr/restful/metazcml.py 2010-01-27 18:14:15 +0000
374@@ -52,23 +52,21 @@
375 calls Zope's handler('registerAdapter').
376 """
377 if version_name is None:
378- # When we were processing annotations we didn't know the name
379- # of the earliest supported version. We know this now.
380- utility = getUtility(IWebServiceConfiguration)
381- if len(utility.active_versions) > 0:
382- version_name = utility.active_versions[0]
383- else:
384- # This service only publishes a 'development' version.
385- version_name = utility.latest_version_uri_prefix
386- # Make sure the given version string has an
387- # IWebServiceVersion utility registered for it, and is not
388- # just a random string.
389- marker = getUtility(IWebServiceVersion, name=version_name)
390+ # This adapter is for the earliest supported version. Register
391+ # it against the generic IWebServiceClientRequest interface,
392+ # which is the superclass of the marker interfaces for every
393+ # specific version.
394+ marker = IWebServiceClientRequest
395+ else:
396+ # Look up the marker interface for the given version. This
397+ # will also ensure the given version string has an
398+ # IWebServiceVersion utility registered for it, and is not
399+ # just a random string.
400+ marker = getUtility(IWebServiceVersion, name=version_name)
401
402 handler('registerAdapter', factory, (interface, marker),
403 provides, name, info)
404
405-
406 def find_exported_interfaces(module):
407 """Find all the interfaces in a module marked for export.
408
409@@ -111,23 +109,29 @@
410 web_interface = generate_entry_interface(interface)
411 factory = generate_entry_adapter(interface, web_interface)
412 provides = IEntry
413+ context.action(
414+ discriminator=('adapter', interface, provides, ''),
415+ callable=handler,
416+ # XXX leonardr bug=503948 Refactor this code once both
417+ # entries and collections are multiversion.
418+ args=('registerAdapter',
419+ factory, (interface, IWebServiceClientRequest),
420+ provides, '', context.info),
421+ )
422 elif tag['type'] == 'collection':
423- factory = generate_collection_adapter(interface)
424- provides = ICollection
425+ for version in tag['collection_default_content'].keys():
426+ factory = generate_collection_adapter(interface, version)
427+ provides = ICollection
428+ context.action(
429+ discriminator=(
430+ 'webservice versioned adapter', interface, provides,
431+ '', version),
432+ callable=register_adapter_for_version,
433+ args=(factory, interface, version, provides, '',
434+ context.info),
435+ )
436 else:
437 raise AssertionError('Unknown export type: %s' % tag['type'])
438- context.action(
439- discriminator=('adapter', interface, provides, ''),
440- callable=handler,
441- # XXX leonardr bug=503948 Register the adapter against a
442- # generic IWebServiceClientRequest. It will be picked up
443- # for all versions of the web service. Later on, this will
444- # be changed to register different adapters for different
445- # versions.
446- args=('registerAdapter',
447- factory, (interface, IWebServiceClientRequest),
448- provides, '', context.info),
449- )
450 register_webservice_operations(context, interface)
451
452

Subscribers

People subscribed via source and target branches