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

[Leonard Richardson]

[Leonard Richardson (leonardr)]

This diff makes mutators version-aware. It lets you define different mutators for different versions, and it prevents you from defining more than one mutator for any particular version.

The complication here is that the @mutator_for annotation is attached to the mutator method, but it modifies a field. Before multi-version, @mutator_for just grabbed the field's annotation dictionary and modified it. But now, "modifying the field's annotation dictionary" means modifying the way the field is published *in the most recent version*. We need to modify the way the field is published in a *specific* version. But we have no guarantee that the field already has an existing set of annotations for that version, and if it's not in the stack already we have no idea whether that version comes before or after other versions.

Here's an example. (I don't have a test like this because it doesn't add any code coverage, and I don't think it's necessary to write a test to explain the internal implementation of something, but it's the best way to explain the problem.) Let's say I publish an entry like this:

    field = exported(TextLine(), ('foo', dict(exported_as='foofield'),
                                 ('bar', dict(exported_as='barfield'))

    @mutator_for(field)
    @export_write_operation()
    @operation_for_version('baz')
    def mutator(value):
        ...

When I process the 'mutator' method I need to modify the annotations on the 'field' field. That field's LAZR_WEBSERVICE_EXPORTED annotation stack looks the same as it did in the exported() call:

[('foo', dict(exported_as='foofield'),
 ('bar', dict(exported_as='barfield')]

Where does 'baz' go? There is *no way* to know. The decision has to be deferred until generate_entry_interfaces, when we have an ordered list of the versions. So this 'baz' thing can't go into LAZR_WEBSERVICE_EXPORTED at all. That's why I created a separate annotation dictionary just for mutators, LAZR_WEBSERVICE_MUTATORS. This is a regular dictionary (not a VersionedDict) that maps versions to (mutator, mutator_annotations) 2-tuples. When we encounter @mutator_for, the stuff that was going to go into LAZR_WEBSERVICE_EXPORTED gets put into here instead. Later on, in generate_entry_interfaces and generate_entry_adapters, when we need to do per-version error checking or generate a per-version Property, we pull the version-indexed value out of LAZR_WEBSERVICE_MUTATORS.

[Paul Hummer (rockstar)]

Leonard, thanks for making the pagetests so detailed. It makes reviewing easier, since I hop back and forth between test and code when I have questions. I'm also glad to see the tests that were XXX missing from a previous branch are in this one.

review: Approve (code)

=== modified file 'src/lazr/restful/declarations.py'
--- src/lazr/restful/declarations.py	2010-02-04 22:23:54 +0000
+++ src/lazr/restful/declarations.py	2010-02-05 15:08:15 +0000
@@ -8,6 +8,7 @@
     'ENTRY_TYPE',
     'FIELD_TYPE',
     'LAZR_WEBSERVICE_EXPORTED',
+    'LAZR_WEBSERVICE_MUTATORS',
     'OPERATION_TYPES',
     'REQUEST_USER',
     'cache_for',
@@ -63,6 +64,7 @@
     make_identifier_safe, VersionedDict)
 
 LAZR_WEBSERVICE_EXPORTED = '%s.exported' % LAZR_WEBSERVICE_NS
+LAZR_WEBSERVICE_MUTATORS = '%s.exported.mutators' % LAZR_WEBSERVICE_NS
 COLLECTION_TYPE = 'collection'
 ENTRY_TYPE = 'entry'
 FIELD_TYPE = 'field'
@@ -220,6 +222,13 @@
 
     # Now we can annotate the field object with the VersionedDict.
     field.setTaggedValue(LAZR_WEBSERVICE_EXPORTED, annotation_stack)
+
+    # We track the field's mutator information separately because it's
+    # defined in the named operations, not in the fields. The last
+    # thing we want to do is try to insert a foreign value into an
+    # already create annotation stack.
+    field.setTaggedValue(LAZR_WEBSERVICE_MUTATORS, {})
+
     return field
 
 
@@ -531,13 +540,19 @@
                             "non-fixed argument. %s takes %d." %
                             (method.__name__, len(free_params)))
 
-        field_annotations = self.field.queryTaggedValue(
-            LAZR_WEBSERVICE_EXPORTED)
-        if 'mutated_by' in field_annotations:
-            raise TypeError("A field can only have one mutator method; "
-                            "%s makes two." % method.__name__ )
-        field_annotations['mutated_by'] = method
-        field_annotations['mutated_by_annotations'] = annotations
+        # We need to keep mutator annotations in a separate dictionary
+        # from the field's main annotations because we're not
+        # processing the field.  We're processing a named operation,
+        # and we have no idea where the named operation's current
+        # version fits into the field's annotations.
+        version, method_annotations = annotations.stack[-1]
+        mutator_annotations = self.field.queryTaggedValue(
+            LAZR_WEBSERVICE_MUTATORS)
+        if version in mutator_annotations:
+            raise TypeError(
+                "A field can only have one mutator method for version %s; "
+                "%s makes two." % (_version_name(version), method.__name__ ))
+        mutator_annotations[version] = (method, dict(method_annotations))
 
 
 def _free_parameters(method, annotations):
@@ -879,8 +894,15 @@
             tags = tags[0]
             if tags.get('exported') is False:
                 continue
-            readonly = (field.readonly
-                        and tags.get('mutated_by', None) is None)
+            mutator_annotations = field.queryTaggedValue(
+                LAZR_WEBSERVICE_MUTATORS)
+            error_prefix = (
+                'Duplicate mutator for interface "%s", field "%s":' % (
+                    interface.__name__, field.__name__))
+            mutated_by, mutated_by_annotations = _get_by_version(
+                mutator_annotations, version, versions[0],
+                error_prefix, (None, {}))
+            readonly = (field.readonly and mutated_by is None)
             attrs[tags['as']] = copy_field(
                 field, __name__=tags['as'], readonly=readonly)
         class_name = _versioned_class_name(
@@ -919,6 +941,7 @@
     adapters_by_version = {}
     for name, field in getFields(content_interface).items():
         tag_stack = field.queryTaggedValue(LAZR_WEBSERVICE_EXPORTED)
+        mutator_annotations = field.queryTaggedValue(LAZR_WEBSERVICE_MUTATORS)
         if tag_stack is None:
             continue
         for tags_version, tags in tag_stack.stack:
@@ -931,11 +954,16 @@
             # this version's adapter class dictionary.
             if tags.get('exported') is False:
                 continue
-            mutator = tags.get('mutated_by', None)
+            error_prefix = (
+                'Duplicate mutator for interface "%s", field "%s":' % (
+                    content_interface.__name__, field.__name__))
+            mutator, annotations = _get_by_version(
+                mutator_annotations, tags_version, webservice_interfaces[0][0],
+                error_prefix, (None, {}))
+
             if mutator is None:
                 property = Passthrough(name, 'context')
             else:
-                annotations = tags['mutated_by_annotations']
                 property = PropertyWithMutator(
                     name, 'context', mutator, annotations)
             adapter_dict[tags['as']] = property
@@ -1291,3 +1319,34 @@
         version = "__Earliest"
     name = "%s_%s" % (base_name, version.encode('utf8'))
     return make_identifier_safe(name)
+
+
+def _get_by_version(dictionary, version, earliest_version,
+                    error_prefix, default=None):
+    """Pull from a dictionary using `version` as the key.
+
+    Under normal circumstances, this will be an ordinary lookup. But
+    some values for the earliest version are stored under None
+    (because the name of the earliest version wasn't known at the
+    time). This helper function knows to look again under None if
+    a lookup for the earliest version fails.
+
+    It also knows to raise an exception signaling a conflict if a
+    value is present under the earliest version *and* under None.
+
+    :param dictionary: The dictionary to use for the lookup.
+    :param version: The dictionary key.
+    :param earliest_version: The version that might also be filed under None.
+    :param default: A default value to use if the lookup fails.
+    """
+    value = dictionary.get(version, None)
+    if version == earliest_version:
+        earliest_value = dictionary.get(None, None)
+        if value is not None and earliest_value is not None:
+            raise ValueError(
+                error_prefix + " Both implicit and explicit definitions found "
+                "for earliest version %s." % version)
+        value = value or earliest_value
+    if value is None:
+        value = default
+    return value

=== modified file 'src/lazr/restful/docs/webservice-declarations.txt'
--- src/lazr/restful/docs/webservice-declarations.txt	2010-02-04 21:22:50 +0000
+++ src/lazr/restful/docs/webservice-declarations.txt	2010-02-05 15:08:15 +0000
@@ -1465,8 +1465,8 @@
     ...         pass
     Traceback (most recent call last):
     ...
-    TypeError: A field can only have one mutator method; set_value_2
-    makes two.
+    TypeError: A field can only have one mutator method for version
+    (earliest version); set_value_2 makes two.
 
 Caching
 =======
@@ -1622,6 +1622,11 @@
 Entries
 -------
 
+The singular and plural name of an entry never changes between
+versions, because the names are a property of the original
+interface. But the published fields can change or be renamed from
+version to version.
+
 Here's a data model interface defining four fields which are published
 in some versions and not others, and which may have different names in
 different versions.
@@ -1803,11 +1808,9 @@
     AttributeError: 'MultiVersionEntryEntry_3_0Adapter' object has no
     attribute 'new_in_10'
 
-
-[XXX leonardr I'll add these tests in my next branch; this branch is
-already way too large.
-
- The singular and plural name of an entry will not change between versions.
+[XXX leonardr These tests need to be done in pagetests, since they
+ test the behavior of the webservice rather than whether certain
+ combinations of annotations are valid.
 
  If a field has a mutator in version n, later versions will inherit the
  same mutator.
@@ -2245,8 +2248,118 @@
     >>> attrs['return_type']
     <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.]
+Mutator operations
+******************
+
+Different versions can define different mutator methods for the same field.
+
+    >>> class IDifferentMutators(Interface):
+    ...     export_as_webservice_entry()
+    ...
+    ...     field = exported(TextLine(readonly=True))
+    ...
+    ...     @mutator_for(field)
+    ...     @export_write_operation()
+    ...     @operation_for_version('beta')
+    ...     @operation_parameters(new_value=TextLine())
+    ...     def set_value(new_value):
+    ...         pass
+    ...
+    ...     @mutator_for(field)
+    ...     @export_write_operation()
+    ...     @operation_for_version('1.0')
+    ...     @operation_parameters(new_value=TextLine())
+    ...     def set_value_2(new_value):
+    ...         pass
+
+    >>> ignored = generate_entry_interfaces(
+    ...     IDifferentMutators, 'beta', '1.0')
+
+But you can't define two mutators for the same field in the same version.
+
+    >>> class IDuplicateMutator(Interface):
+    ...     export_as_webservice_entry()
+    ...
+    ...     field = exported(TextLine(readonly=True))
+    ...
+    ...     @mutator_for(field)
+    ...     @export_write_operation()
+    ...     @operation_for_version('1.0')
+    ...     @operation_parameters(new_value=TextLine())
+    ...     def set_value(new_value):
+    ...         pass
+    ...
+    ...     @mutator_for(field)
+    ...     @export_write_operation()
+    ...     @operation_for_version('1.0')
+    ...     @operation_parameters(new_value=TextLine())
+    ...     def set_value_2(new_value):
+    ...         pass
+    Traceback (most recent call last):
+    ...
+    TypeError: A field can only have one mutator method for version
+    1.0; set_value_2 makes two.
+
+Here's a case that's a little trickier. You'll also get an error if
+you implicitly define a mutator for the earliest version (without
+giving its name), and then define another one explicitly (giving the
+name of the earliest version.)
+
+    >>> class IImplicitAndExplicitMutator(Interface):
+    ...     export_as_webservice_entry()
+    ...
+    ...     field = exported(TextLine(readonly=True))
+    ...
+    ...     @mutator_for(field)
+    ...     @export_write_operation()
+    ...     @operation_for_version('beta')
+    ...     @operation_parameters(new_value=TextLine())
+    ...     def set_value_2(new_value):
+    ...         pass
+    ...
+    ...     @mutator_for(field)
+    ...     @export_write_operation()
+    ...     @operation_parameters(new_value=TextLine())
+    ...     def set_value_2(new_value):
+    ...         pass
+
+    >>> generate_entry_interfaces(
+    ...     IImplicitAndExplicitMutator, 'beta', '1.0')
+    Traceback (most recent call last):
+    ...
+    ValueError: Duplicate mutator for interface
+    "IImplicitAndExplicitMutator", field "field": Both implicit and
+    explicit definitions found for earliest version beta.
+
+This error isn't detected until you try to generate the entry
+interfaces, because until that point lazr.restful doesn't know that
+'beta' is the earliest version. If the earliest version was 'alpha',
+the IImplicitAndExplicitMutator class would be valid.
+
+(Again, to test this hypothesis, we need to re-define the class,
+because the generate_entry_interfaces call modified the original
+class's annotations in place.)
+
+    >>> class IImplicitAndExplicitMutator(Interface):
+    ...     export_as_webservice_entry()
+    ...
+    ...     field = exported(TextLine(readonly=True))
+    ...
+    ...     @mutator_for(field)
+    ...     @export_write_operation()
+    ...     @operation_for_version('beta')
+    ...     @operation_parameters(new_value=TextLine())
+    ...     def set_value_2(new_value):
+    ...         pass
+    ...
+    ...     @mutator_for(field)
+    ...     @export_write_operation()
+    ...     @operation_parameters(new_value=TextLine())
+    ...     def set_value_2(new_value):
+    ...         pass
+
+    >>> ignored = generate_entry_interfaces(
+    ...     IImplicitAndExplicitMutator, 'alpha', 'beta', '1.0')
 
 Destructor operations
 *********************