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

[Leonard Richardson]

[Leonard Richardson (leonardr)]

This branch adds multi-version error checking for destructor methods. A destructor method cannot have any arguments that are not fixed to specific values. The previous code was not version-aware, so it was only checking for the most recent version of the web service. In this branch I check every version in which the destructor method is published.

I also defined a tiny helper method, _version_name, which takes care of this logic which I was putting all over the place when printing out error messages:

    if version is None:
        version = "(earliest version)"

[Paul Hummer (rockstar)]

Based on previous knowledge from reviewing the dependent branch, this branch is a bit easier to grok. Once again, your doctests make this patch easier to understand.

review: Approve (code)

=== modified file 'src/lazr/restful/declarations.py'
--- src/lazr/restful/declarations.py	2010-02-04 21:50:06 +0000
+++ src/lazr/restful/declarations.py	2010-02-04 21:53:10 +0000
@@ -285,11 +285,10 @@
             'collection_default_content', {})
 
         if version in default_content_methods:
-            if version is None:
-                version = "(earliest version)"
             raise TypeError(
                 "Only one method can be marked with "
-                "@collection_default_content for version '%s'." % version)
+                "@collection_default_content for version '%s'." % (
+                    _version_name(version)))
         self.version = version
         self.params = params
 
@@ -424,11 +423,6 @@
         # Make sure that each version of the web service defines
         # a self-consistent view of this method.
         for version, annotations in annotation_stack.stack:
-            if version is None:
-                # Create a human-readable name for the earliest version
-                # without trying to look it up.
-                version = "(earliest version)"
-
             if annotations['type'] == REMOVED_OPERATION_TYPE:
                 # The method is published in other versions of the web
                 # service, but not in this one. Don't try to validate this
@@ -456,15 +450,15 @@
                 raise TypeError(
                     'method "%s" doesn\'t have the following exported '
                     'parameters in version "%s": %s.' % (
-                        method.__name__, version,
+                        method.__name__, _version_name(version),
                         ", ".join(sorted(undefined_params))))
             missing_params = set(
                 info['required']).difference(exported_params)
             if missing_params:
                 raise TypeError(
-                    'method "%s" is missing exported parameter definitions '
-                    'in version "%s": %s' % (
-                        method.__name__, version,
+                    'method "%s" is missing definitions for parameter(s) '
+                    'exported in version "%s": %s' % (
+                        method.__name__, _version_name(version),
                         ", ".join(sorted(missing_params))))
 
             _update_default_and_required_params(annotations['params'], info)
@@ -784,21 +778,29 @@
     """
     type = "destructor"
 
-    def annotate_method(self, method, annotations):
+    def annotate_method(self, method, annotation_stack):
         """See `_method_annotator`.
 
         Store information about the mutator method with the method.
+
+        Every version must have a self-consistent set of annotations.
         """
+        super(export_destructor_operation, self).annotate_method(
+              method, annotation_stack)
         # The mutator method must take no arguments, not counting
         # arguments with values fixed by call_with().
         signature = fromFunction(method).getSignatureInfo()
-        free_params = _free_parameters(method, annotations)
-        if len(free_params) != 0:
-            raise TypeError("A destructor method must take no "
-                            "non-fixed arguments; %s takes %d." %
-                            (method.__name__, len(free_params)))
-        super(export_destructor_operation, self).annotate_method(
-            method, annotations)
+        for version, annotations in annotation_stack.stack:
+            if annotations['type'] == REMOVED_OPERATION_TYPE:
+                continue
+            free_params = _free_parameters(method, annotations)
+            if len(free_params) != 0:
+                raise TypeError(
+                    "A destructor method must take no non-fixed arguments. "
+                    'In version %s, the "%s" method takes %d: "%s".' % (
+                        _version_name(version), method.__name__,
+                        len(free_params), '", "'.join(free_params))
+                        )
 
 
 def _check_tagged_interface(interface, type):
@@ -845,12 +847,11 @@
             if annotations.get('type') == export_destructor_operation.type:
                 destructor = destructor_for_version.get(version)
                 if destructor is not None:
-                    if version is None:
-                        version = "(earliest)"
                     raise TypeError(
                         'An entry can only have one destructor method for '
                         'version %s; %s and %s make two.' % (
-                            version, method.__name__, destructor.__name__))
+                            _version_name(version), method.__name__,
+                            destructor.__name__))
                 destructor_for_version[version] = method
 
     # Next, we'll normalize each published field. A normalized field
@@ -1267,6 +1268,19 @@
     versioned_dict.stack = new_stack
     return field
 
+
+def _version_name(version):
+    """Return a human-readable version name.
+
+    If `version` is None (indicating the as-yet-unknown earliest
+    version), returns "(earliest version)". Otherwise returns the version
+    name.
+    """
+    if version is None:
+        return "(earliest version)"
+    return version
+
+
 def _versioned_class_name(base_name, version):
     """Create a class name incorporating the given version string."""
     if version is None:

=== modified file 'src/lazr/restful/docs/webservice-declarations.txt'
--- src/lazr/restful/docs/webservice-declarations.txt	2010-02-03 20:59:22 +0000
+++ src/lazr/restful/docs/webservice-declarations.txt	2010-02-04 21:53:10 +0000
@@ -534,8 +534,8 @@
     ...     def a_method(param1, param2, param3, param4): pass
     Traceback (most recent call last):
       ...
-    TypeError: method "a_method" is missing exported parameter definitions
-    in version "(earliest version)": param3, param4
+    TypeError: method "a_method" is missing definitions for parameter(s)
+    exported in version "(earliest version)": param3, param4
 
 Defining a parameter not available on the method also results in an
 error:
@@ -1281,12 +1281,14 @@
     ...     text = exported(TextLine(readonly=True))
     ...
     ...     @export_destructor_operation()
+    ...     @operation_parameters(argument=TextLine())
     ...     def destroy(argument):
     ...         pass
     Traceback (most recent call last):
     ...
-    TypeError: A destructor method must take no non-fixed arguments;
-    destroy takes 1.
+    TypeError: A destructor method must take no non-fixed arguments.
+    In version (earliest version), the "destroy" method takes 1:
+    "argument".
 
     >>> class IHasText(Interface):
     ...     export_as_webservice_entry()
@@ -1298,7 +1300,6 @@
     ...         pass
     >>> ignored = generate_entry_interfaces(IHasText, 'beta')
 
-
 An entry cannot have more than one destructor.
 
     >>> from lazr.restful.declarations import export_destructor_operation
@@ -1317,7 +1318,7 @@
     Traceback (most recent call last):
     ...
     TypeError: An entry can only have one destructor method for
-    version (earliest); destroy and destroy2 make two.
+    version (earliest version); destroy and destroy2 make two.
 
 
 Mutators
@@ -2247,6 +2248,56 @@
 [XXX leonardr mutator_for modifies the field, not the method, so it
 won't work until I add multiversion support for fields.]
 
+Destructor operations
+*********************
+
+A destructor can be published in different ways in different versions,
+but the restrictions on destructor arguments are enforced separately
+for each version.
+
+Here, the destructor fixes a value for the 'fixed2' argument in the
+earliest version, but not in '1.0'. This is fine: the 1.0 value for
+'fixed2' will be inherited from the previous version.
+
+    >>> class IGoodDestructorEntry(Interface):
+    ...     export_as_webservice_entry()
+    ...
+    ...     @call_with(fixed1="value3")
+    ...     @operation_for_version('1.0')
+    ...     @export_destructor_operation()
+    ...     @call_with(fixed1="value1", fixed2="value")
+    ...     @operation_parameters(fixed1=TextLine(), fixed2=TextLine())
+    ...     def destructor(fixed1, fixed2):
+    ...         """Another destructor method."""
+
+    >>> ignore = generate_entry_interfaces(
+    ...     IGoodDestructorEntry, 'beta', '1.0')
+
+This is not fine. In this case, the destructor is removed in 1.0 and
+added back in 2.0. The 2.0 version does not inherit any values from
+its prior incarnation, so the fact that it does not fix any value for
+'fixed2' is a problem.
+
+    >>> class IBadDestructorEntry(Interface):
+    ...     export_as_webservice_entry()
+    ...
+    ...     @export_destructor_operation()
+    ...     @operation_parameters(fixed1=TextLine(), fixed2=TextLine())
+    ...     @call_with(fixed1="value3")
+    ...     @operation_for_version('2.0')
+    ...     @operation_removed_in_version('1.0')
+    ...     @export_destructor_operation()
+    ...     @call_with(fixed1="value1", fixed2="value")
+    ...     @operation_parameters(fixed1=TextLine(), fixed2=TextLine())
+    ...     def destructor(fixed1, fixed2):
+    ...         """Another destructor method."""
+    Traceback (most recent call last):
+    ...
+    TypeError: A destructor method must take no non-fixed
+    arguments. In version 2.0, the "destructor" method takes 1:
+    "fixed2".
+
+
 Security
 ========