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

[Leonard Richardson]

[Leonard Richardson (leonardr)]

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.

[Leonard Richardson (leonardr)]

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.

[lp:~leonardr/lazr.restful/enforce-version-order]

108. By Leonard Richardson on 2010-01-28

Clarified explanation in doctest.:

109. By Leonard Richardson on 2010-01-28

Changed 'after' to 'on top' to fit better with the stack metaphor.

[Paul Hummer (rockstar)]

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

=== modified file 'src/lazr/restful/docs/webservice-declarations.txt'
--- src/lazr/restful/docs/webservice-declarations.txt	2010-01-28 14:21:19 +0000
+++ src/lazr/restful/docs/webservice-declarations.txt	2010-01-28 18:11:15 +0000
@@ -888,7 +888,7 @@
     >>> class MyWebServiceConfiguration:
     ...     implements(IWebServiceConfiguration)
     ...     view_permission = "lazr.View"
-    ...     active_versions = ["beta"]
+    ...     active_versions = ["beta", "1.0", "2.0", "3.0"]
     ...     code_revision = "1.0b"
     ...     default_batch_size = 50
     ...     latest_version_uri_prefix = 'beta'
@@ -1855,11 +1855,7 @@
     <lazr.restful.fields.CollectionField object...>
 
 [XXX leonardr mutator_for modifies the field, not the method, so it
-won't work until I add multiversion support for fields. Also, it's
-possible to remove a named operation from a certain version and then
-uselessly annotate it some more. The best place to catch this error
-turns out to be when generating the adapter classes, not when
-collecting annotations.]
+won't work until I add multiversion support for fields.]
 
 Security
 ========
@@ -1895,7 +1891,6 @@
     zope.security._proxy._Proxy (using zope.security.checker.Checker)
         public: __call__, send_modification_event
 
-=================
 ZCML Registration
 =================
 
@@ -1908,23 +1903,34 @@
 
     >>> import sys
     >>> from types import ModuleType
-    >>> bookexample = ModuleType('bookexample')
-    >>> sys.modules['lazr.restful.bookexample'] = bookexample
-    >>> bookexample.IBook = IBook
-    >>> bookexample.IBookSet = IBookSet
-    >>> bookexample.IBookOnSteroids = IBookOnSteroids
-    >>> bookexample.IBookSetOnSteroids = IBookSetOnSteroids
-    >>> bookexample.ISimpleComment = ISimpleComment
-    >>> bookexample.InvalidEmail = InvalidEmail
+    >>> def create_test_module(name, *contents):
+    ...     """Defines a new module and adds it to sys.modules."""
+    ...     new_module = ModuleType(name)
+    ...     sys.modules['lazr.restful.' + name] = new_module
+    ...     for object in contents:
+    ...         setattr(new_module, object.__name__, object)
+    ...     return new_module
 
     >>> from zope.configuration import xmlconfig
-    >>> zcmlcontext = xmlconfig.string("""
-    ... <configure
-    ...     xmlns:webservice="http://namespaces.canonical.com/webservice">
-    ...   <include package="lazr.restful" file="meta.zcml" />
-    ...   <webservice:register module="lazr.restful.bookexample" />
-    ... </configure>
-    ... """)
+    >>> def register_test_module(name, *contents):
+    ...     new_module = create_test_module(name, *contents)
+    ...     try:
+    ...         zcmlcontext = xmlconfig.string("""
+    ...         <configure
+    ...             xmlns:webservice=
+    ...                 "http://namespaces.canonical.com/webservice">
+    ...           <include package="lazr.restful" file="meta.zcml" />
+    ...           <webservice:register module="lazr.restful.%s" />
+    ...         </configure>
+    ...         """ % name)
+    ...     except Exception, e:
+    ...         del sys.modules['lazr.restful.' + name]
+    ...         raise e
+    ...     return new_module
+
+    >>> bookexample = register_test_module(
+    ...     'bookexample', IBook, IBookSet, IBookOnSteroids,
+    ...     IBookSetOnSteroids, ISimpleComment, InvalidEmail)
 
 After the registration, adapters from IBook to IEntry, and IBookSet to
 ICollection are available:
@@ -1980,3 +1986,60 @@
 
     >>> del bookexample
     >>> del sys.modules['lazr.restful.bookexample']
+
+Error handling
+--------------
+
+Some error handling happens in the ZCML registration phase. At this
+point, all the annotations have been processed, and the
+IWebServiceConfiguration utility (with its canonical list of versions)
+has become available. This lets us run checks on the versioning
+annotations that couldn't be run before.
+
+Here's a class annotated by someone who believes that version 1.0 of
+the web service is a later version than version 2.0. (Or who believes
+that named operation annotations proceed from the top down rather than
+the bottom up.)
+
+    >>> class WrongOrderVersions(Interface):
+    ...     export_as_webservice_entry()
+    ...     @export_operation_as('10_name')
+    ...     @operation_for_version("1.0")
+    ...     @operation_parameters(arg=Float())
+    ...     @export_read_operation()
+    ...     @operation_for_version("2.0")
+    ...     def method(arg):
+    ...         """A method."""
+
+An attempt to register this module with ZCML results in an error
+explaining the problem.
+
+    >>> register_test_module('wrongorder', WrongOrderVersions)
+    Traceback (most recent call last):
+    ...
+    ConfigurationExecutionError: <type 'exceptions.AssertionError'>:
+    Annotations on "WrongOrderVersions.method" put an earlier version
+    on top of a later version: "beta", "2.0", "1.0". The correct order
+    is: "beta", "1.0", "2.0".  ...
+
+Here's a class in which a named operation is removed in version 1.0
+and then annotated without being reinstated.
+
+    >>> class AnnotatingARemovedMethod(Interface):
+    ...     export_as_webservice_entry()
+    ...     @operation_parameters(arg=TextLine())
+    ...     @export_operation_as('already_been_removed')
+    ...     @operation_removed_in_version("1.0")
+    ...     @operation_parameters(arg=Float())
+    ...     @export_read_operation()
+    ...     @operation_for_version("2.0")
+    ...     def method(arg):
+    ...         """A method."""
+
+    >>> register_test_module('annotatingremoved', AnnotatingARemovedMethod)
+    Traceback (most recent call last):
+    ...
+    ZopeXMLConfigurationError: ...
+    AssertionError: Method "method" contains annotations for version
+    "1.0", even though it's not published in that version. The bad
+    annotations are: "params", "as".

=== modified file 'src/lazr/restful/metazcml.py'
--- src/lazr/restful/metazcml.py	2010-01-27 17:33:32 +0000
+++ src/lazr/restful/metazcml.py	2010-01-28 18:11:15 +0000
@@ -34,6 +34,50 @@
         title=u'Module which will be inspected for webservice declarations')
 
 
+def ensure_correct_version_ordering(name, version_list):
+    """Make sure that a list mentions versions from earliest to latest.
+
+    If an earlier version shows up after a later version, this is a
+    sign that a developer was confused about version names when
+    annotating the web service.
+
+    :param name: The name of the object annotated with the
+        possibly mis-ordered versions.
+
+    :param version_list: The list of versions found in the interface.
+
+    :raise AssertionError: If the given version list is not a
+        earlier-to-later ordering of a subset of the web service's
+        versions.
+    """
+    configuration = getUtility(IWebServiceConfiguration)
+    actual_versions = configuration.active_versions + [
+        configuration.latest_version_uri_prefix]
+    # Replace None with the actual version number of the earliest
+    # version.
+    try:
+        earliest_version_pos = version_list.index(None)
+        version_list = list(version_list)
+        version_list[earliest_version_pos] = actual_versions[0]
+    except ValueError:
+        # The earliest version was not mentioned in the version list.
+        # Do nothing.
+        pass
+
+    # Sort version_list according to the list of actual versions.
+    # If the sorted list is different from the unsorted list, at
+    # least one version is out of place.
+    def compare(x, y):
+        return cmp (actual_versions.index(x), actual_versions.index(y))
+    sorted_version_list = sorted(version_list, cmp=compare)
+    if sorted_version_list != version_list:
+        bad_versions = '", "'.join(version_list)
+        good_versions = '", "'.join(sorted_version_list)
+        msg = ('Annotations on "%s" put an earlier version on top of a '
+               'later version: "%s". The correct order is: "%s".')
+        raise AssertionError, msg % (name, bad_versions, good_versions)
+
+
 def register_adapter_for_version(factory, interface, version_name,
                                  provides, name, info):
     """A version-aware wrapper for the registerAdapter operation.
@@ -147,6 +191,14 @@
             # This method is not published as a named operation.
             continue
 
+        # First, make sure that this method was annotated with the
+        # versions in the right order.
+        context.action(
+            discriminator=('webservice version ordering', method),
+            callable=ensure_correct_version_ordering,
+            args=(interface.__name__ + '.' + method.__name__, tag.dict_names)
+        )
+
         operation_name = None
         # If an operation's name does not change between version n and
         # version n+1, we want lookups for requests that come in for
@@ -175,6 +227,26 @@
                 operation_name = None
                 operation_provides = None
                 factory = None
+
+                # If there are any other tags besides 'type', it means
+                # that the developer tried to annotate a method for a
+                # version where the method isn't published. Let's warn
+                # them about it.
+                #
+                # (We can't make this check when the annotation
+                # happens, because it will reject a method that uses
+                # an annotation like @export_operation_as before using
+                # an annotation like @export_read_operation. That's a
+                # little sloppy, but it's not bad enough to warrant an
+                # exception.)
+                tag_names = list(tag.keys())
+                if tag_names != ['type']:
+                    tag_names.remove('type')
+                    raise AssertionError(
+                        'Method "%s" contains annotations for version "%s", '
+                        'even though it\'s not published in that version. '
+                        'The bad annotations are: "%s".' % (
+                            method.__name__, version, '", "'.join(tag_names)))
             else:
                 if tag['type'] == 'read_operation':
                     operation_provides = IResourceGETOperation

=== modified file 'src/lazr/restful/simple.py'
--- src/lazr/restful/simple.py	2010-01-12 15:06:15 +0000
+++ src/lazr/restful/simple.py	2010-01-28 18:11:15 +0000
@@ -32,8 +32,8 @@
 
 from lazr.restful import EntryAdapterUtility, ServiceRootResource
 from lazr.restful.interfaces import (
-    IServiceRootResource, ITraverseWithGet, IWebServiceConfiguration,
-    IWebServiceLayer)
+    IServiceRootResource, ITopLevelEntryLink, ITraverseWithGet,
+    IWebServiceConfiguration, IWebServiceLayer)
 from lazr.restful.publisher import (
     WebServicePublicationMixin, WebServiceRequestTraversal)
 from lazr.restful.utils import implement_from_dict