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

[Leonard Richardson]

[Leonard Richardson (leonardr)]

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.

[Edwin Grubbs (edwin-grubbs)]

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...

review: Approve (code)

=== 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 18:14:15 +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
@@ -218,19 +218,26 @@
                 "@collection_default_content can only be used from within an "
                 "interface exported as a collection.")
 
-        if 'collection_default_content' in tag:
+        default_content_methods = tag.setdefault(
+            'collection_default_content', {})
+
+        if version in default_content_methods:
+            if version is None:
+                version = "(earliest)"
             raise TypeError(
-                "only one method should be marked with "
-                "@collection_default_content.")
-
-        tag['collection_default_content_params'] = params
+                "Only one method can be marked with "
+                "@collection_default_content for version %s." % version)
+        self.version = version
+        self.params = params
 
     def __call__(self, f):
         """Annotates the collection with the name of the method to call."""
         tag = _get_interface_tags()[LAZR_WEBSERVICE_EXPORTED]
-        tag['collection_default_content'] = f.__name__
+        tag['collection_default_content'][self.version] = (
+            f.__name__, self.params)
         return f
 
+
 WEBSERVICE_ERROR = '__lazr_webservice_error__'
 
 def webservice_error(status):
@@ -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(
+        "%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 the name of the earliest version until
+        # runtime. Use a generic string that won't conflict with a
+        # real version string.
+        version = "__Earliest"
+    name = "%s_%s" % (base_name, version)
+    return make_identifier_safe(name)

=== modified file 'src/lazr/restful/docs/webservice-declarations.txt'
--- src/lazr/restful/docs/webservice-declarations.txt	2010-01-25 19:26:30 +0000
+++ src/lazr/restful/docs/webservice-declarations.txt	2010-01-27 18:14:15 +0000
@@ -170,15 +170,14 @@
 tagged value.
 
     >>> print_export_tag(IBookSet)
-    collection_default_content: 'getAllBooks'
-    collection_default_content_params: {}
+    collection_default_content: {None: ('getAllBooks', {})}
     collection_entry_schema: <InterfaceClass __builtin__.IBook>
     type: 'collection'
 
     >>> print_export_tag(ICheckedOutBookSet)
-    collection_default_content: 'getByTitle'
-    collection_default_content_params: {'title': '',
-                                        'user': <object...>}
+    collection_default_content: {None: ('getByTitle',
+            {'title': '',
+             'user': <object object ...>})}
     collection_entry_schema: <InterfaceClass __builtin__.IBook>
     type: 'collection'
 
@@ -223,8 +222,8 @@
     ...         """Another getAll()."""
     Traceback (most recent call last):
       ...
-    TypeError: only one method should be marked with
-    @collection_default_content.
+    TypeError: Only one method can be marked with
+    @collection_default_content for version (earliest).
 
 export_as_webservice_collection() can only be used on Interface.
 
@@ -1452,7 +1451,6 @@
 It is possible to cache a server response in the browser cache using
 the @cache_for decorator:
 
-    >>> from lazr.restful.testing.webservice import FakeRequest
     >>> from lazr.restful.declarations import cache_for
     >>>
     >>> class ICachedBookSet(IBookSet):
@@ -1472,14 +1470,13 @@
     ...
     ...     def getAllBooks(self):
     ...         return self.books
-    >>>
-    >>> request = FakeRequest()
+
     >>> read_method_adapter_factory = generate_operation_adapter(
     ...     ICachedBookSet['getAllBooks'])
     >>> read_method_adapter = read_method_adapter_factory(
     ...     CachedBookSet(['Cool book']), request)
     >>> print read_method_adapter.call()
-    ["Cool book"]
+    ['Cool book']
     >>> print request.response.headers
     {'Content-Type': 'application/json', 'Cache-control': 'max-age=60'}
 
@@ -1509,6 +1506,96 @@
 Different versions of the webservice can publish the same data model
 object in totally different ways.
 
+Collections
+-----------
+
+A collection's contents are determined by calling one of its
+methods. Which method is called, and with which arguments, can vary
+across versions.
+
+    >>> from lazr.restful.declarations import generate_operation_adapter
+
+    >>> class IMultiVersionCollection(Interface):
+    ...     export_as_webservice_collection(Interface)
+    ...
+    ...     @collection_default_content('2.0')
+    ...     def content_20():
+    ...         """The content method for version 2.0."""
+    ...
+    ...     @collection_default_content('1.0', argument='1.0 value')
+    ...     @collection_default_content(argument='pre-1.0 value')
+    ...     def content_pre_20(argument):
+    ...         """The content method for versions before 2.0"""
+
+Here's a simple implementation of IMultiVersionCollection. It'll
+illustrate how the different versions of the web service invoke
+different methods to find the collection contents.
+
+    >>> class MultiVersionCollection():
+    ...     """Simple IMultiVersionCollection implementation."""
+    ...     implements(IMultiVersionCollection)
+    ...
+    ...     def content_20(self):
+    ...         return ["contents", "for", "version", "2.0"]
+    ...
+    ...     def content_pre_20(self, argument):
+    ...         return ["you", "passed", "in", argument]
+
+By passing a version string into generate_collection_adapter(), we can
+get different adapter classes for different versions of the web
+service. We'll be invoking each version against the same data model
+object. Here it is:
+
+    >>> data_object = MultiVersionCollection()
+
+Passing in None to generate_collection_adapter gets us the collection
+as it appears in the earliest version of the web service. The
+content_pre_20() method is invoked with the 'argument' parameter equal
+to "pre-1.0 value".
+
+    >>> interface = IMultiVersionCollection
+    >>> adapter_earliest_factory = generate_collection_adapter(
+    ...     interface, None)
+    >>> print adapter_earliest_factory.__name__
+    MultiVersionCollectionCollectionAdapter___Earliest
+
+    >>> collection_earliest = adapter_earliest_factory(data_object, request)
+    >>> print collection_earliest.find()
+    ['you', 'passed', 'in', 'pre-1.0 value']
+
+Passing in '1.0' gets us the collection as it appears in the 1.0
+version of the web service. Note that the argument passed in to
+content_pre_20() is different, and so the returned contents are
+slightly different.
+
+    >>> adapter_10_factory = generate_collection_adapter(interface, '1.0')
+    >>> print adapter_10_factory.__name__
+    MultiVersionCollectionCollectionAdapter_1_0
+
+    >>> collection_10 = adapter_10_factory(data_object, request)
+    >>> print collection_10.find()
+    ['you', 'passed', 'in', '1.0 value']
+
+Passing in '2.0' gets us a collection with totally different contents,
+because a totally different method is being called.
+
+    >>> adapter_20_factory = generate_collection_adapter(interface, '2.0')
+    >>> print adapter_20_factory.__name__
+    MultiVersionCollectionCollectionAdapter_2_0
+
+    >>> collection_20 = adapter_20_factory(data_object, request)
+    >>> print collection_20.find()
+    ['contents', 'for', 'version', '2.0']
+
+An error occurs when we try to generate an adapter for a version
+that's not mentioned in the annotations.
+
+    >>> generate_collection_adapter(interface, 'NoSuchVersion')
+    Traceback (most recent call last):
+    ...
+    AssertionError: 'IMultiVersionCollection' isn't tagged for export
+    to web service version 'NoSuchVersion'.
+
 Named operations
 ----------------
 
@@ -1865,8 +1952,8 @@
     >>> from zope.component import getGlobalSiteManager, getUtility
     >>> adapter_registry = getGlobalSiteManager().adapters
 
-    >>> request_interface = getUtility(IWebServiceVersion, name='beta')
     >>> from lazr.restful.interfaces import IWebServiceClientRequest
+    >>> request_interface = IWebServiceClientRequest
     >>> adapter_registry.lookup(
     ...     (IBookSetOnSteroids, request_interface),
     ...     IResourceGETOperation, 'searchBooks')

=== modified file 'src/lazr/restful/example/multiversion/resources.py'
--- src/lazr/restful/example/multiversion/resources.py	2010-01-21 18:26:58 +0000
+++ src/lazr/restful/example/multiversion/resources.py	2010-01-27 18:14:15 +0000
@@ -31,10 +31,17 @@
 class IPairSet(ILocation):
     export_as_webservice_collection(IKeyValuePair)
 
-    @collection_default_content()
+    # In versions 2.0 and 3.0, the collection of key-value pairs
+    # includes all pairs.
+    @collection_default_content("2.0")
     def getPairs():
         """Return the key-value pairs."""
 
+    # Before 2.0, it only includes pairs whose values are not None.
+    @collection_default_content('beta')
+    def getNonEmptyPairs():
+        """Return the key-value pairs that don't map to None."""
+
     def get(request, name):
         """Retrieve a key-value pair by its key."""
 
@@ -59,6 +66,8 @@
     def find_for_value(self, value):
         return [pair for pair in self.pairs if value == pair.value]
 
+    def getNonEmptyPairs(self):
+        return [pair for pair in self.pairs if pair.value is not None]
 
 class KeyValuePair(BasicKeyValuePair):
     implements(IKeyValuePair)

=== modified file 'src/lazr/restful/example/multiversion/root.py'
--- src/lazr/restful/example/multiversion/root.py	2010-01-21 17:57:02 +0000
+++ src/lazr/restful/example/multiversion/root.py	2010-01-27 18:14:15 +0000
@@ -46,7 +46,8 @@
         pairset = PairSet()
         pairset.pairs = [
             KeyValuePair(pairset, "foo", "bar"),
-            KeyValuePair(pairset, "1", "2")
+            KeyValuePair(pairset, "1", "2"),
+            KeyValuePair(pairset, "Some", None)
             ]
         collections = dict(pairs=(IKeyValuePair, pairset))
         return collections, {}

=== modified file 'src/lazr/restful/example/multiversion/tests/introduction.txt'
--- src/lazr/restful/example/multiversion/tests/introduction.txt	2010-01-21 20:07:54 +0000
+++ src/lazr/restful/example/multiversion/tests/introduction.txt	2010-01-27 18:14:15 +0000
@@ -71,13 +71,33 @@
 Collections and entries
 =======================
 
-The web service presents a single collection of key-value pairs.
-
-    >>> body = webservice.get('/pairs').jsonBody()
-    >>> for entry in body['entries']:
-    ...     print entry['self_link'], entry['key'], entry['value']
-    http://multiversion.dev/3.0/pairs/foo foo bar
-    http://multiversion.dev/3.0/pairs/1 1 2
+The web service presents a single collection of key-value pairs. In
+versions previous to 2.0, the collection omits key-value pairs where
+the value is None. In 2.0 and 3.0, all key-value pairs are published.
+
+    >>> from operator import itemgetter
+    >>> def show_pairs(version):
+    ...     body = webservice.get('/pairs', api_version=version).jsonBody()
+    ...     for entry in sorted(body['entries'], key=itemgetter('key')):
+    ...         print "%s: %s" % (entry['key'], entry['value'])
+
+    >>> show_pairs('beta')
+    1: 2
+    foo: bar
+
+    >>> show_pairs('1.0')
+    1: 2
+    foo: bar
+
+    >>> show_pairs('2.0')
+    1: 2
+    Some: None
+    foo: bar
+
+    >>> show_pairs('3.0')
+    1: 2
+    Some: None
+    foo: bar
 
     >>> body = webservice.get('/pairs/foo').jsonBody()
     >>> print body['key'], body['value']

=== modified file 'src/lazr/restful/metazcml.py'
--- src/lazr/restful/metazcml.py	2010-01-25 19:51:11 +0000
+++ src/lazr/restful/metazcml.py	2010-01-27 18:14:15 +0000
@@ -52,23 +52,21 @@
     calls Zope's handler('registerAdapter').
     """
     if version_name is None:
-        # When we were processing annotations we didn't know the name
-        # of the earliest supported version. We know this now.
-        utility = getUtility(IWebServiceConfiguration)
-        if len(utility.active_versions) > 0:
-            version_name = utility.active_versions[0]
-        else:
-            # This service only publishes a 'development' version.
-            version_name = utility.latest_version_uri_prefix
-    # Make sure the given version string has an
-    # IWebServiceVersion utility registered for it, and is not
-    # just a random string.
-    marker = getUtility(IWebServiceVersion, name=version_name)
+        # This adapter is for the earliest supported version. Register
+        # it against the generic IWebServiceClientRequest interface,
+        # which is the superclass of the marker interfaces for every
+        # specific version.
+        marker = IWebServiceClientRequest
+    else:
+        # Look up the marker interface for the given version. This
+        # will also ensure the given version string has an
+        # IWebServiceVersion utility registered for it, and is not
+        # just a random string.
+        marker = getUtility(IWebServiceVersion, name=version_name)
 
     handler('registerAdapter', factory, (interface, marker),
             provides, name, info)
 
-
 def find_exported_interfaces(module):
     """Find all the interfaces in a module marked for export.
 
@@ -111,23 +109,29 @@
             web_interface = generate_entry_interface(interface)
             factory = generate_entry_adapter(interface, web_interface)
             provides = IEntry
+            context.action(
+                discriminator=('adapter', interface, provides, ''),
+                callable=handler,
+                # XXX leonardr bug=503948 Refactor this code once both
+                # entries and collections are multiversion.
+                args=('registerAdapter',
+                      factory, (interface, IWebServiceClientRequest),
+                      provides, '', context.info),
+                )
         elif tag['type'] == 'collection':
-            factory = generate_collection_adapter(interface)
-            provides = ICollection
+            for version in tag['collection_default_content'].keys():
+                factory = generate_collection_adapter(interface, version)
+                provides = ICollection
+                context.action(
+                    discriminator=(
+                        'webservice versioned adapter', interface, provides,
+                        '', version),
+                    callable=register_adapter_for_version,
+                    args=(factory, interface, version, provides, '',
+                          context.info),
+                    )
         else:
             raise AssertionError('Unknown export type: %s' % tag['type'])
-        context.action(
-            discriminator=('adapter', interface, provides, ''),
-            callable=handler,
-            # XXX leonardr bug=503948 Register the adapter against a
-            # generic IWebServiceClientRequest.  It will be picked up
-            # for all versions of the web service. Later on, this will
-            # be changed to register different adapters for different
-            # versions.
-            args=('registerAdapter',
-                  factory, (interface, IWebServiceClientRequest),
-                            provides, '', context.info),
-            )
         register_webservice_operations(context, interface)