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

[Leonard Richardson]

[Leonard Richardson (leonardr)]

This branch adds more multi-version capabilities to the example multiversion web service (fields that are not published in all versions, a field that's manipulated with a mutator, and two destructor methods for different versions), and adds pagetests to make sure all the features I've been writing work in a real multi-version web service.

I found one part of the system which did _not_ work in a real web service. If you define a field's mutator method for version 1.0, that works fine for version 1.0, but the mutator is not inherited in version 2.0--the field goes back to being read-only. There was no inheritance mechanism to make sure that version 2.0's mutator defaults to version 1.0's mutator.

Fortunately, I've already dealt with this situation. I had the exact same problem on the field level, where version 2.0's field was not published under version 1.0's name, and I resolved it with _normalize_field_annotations. In this branch, I removed the _get_by_version helper method which I was using to retrieve mutator information from the special mutator dictionary. Instead, I changed _normalize_field_annotations to merge the special mutator dictionary into the standard annotation dictionary as it normalizes that dictionary. This is where inheritance is implemented for fields, and now that's also where it's implemented for field mutators.

[Paul Hummer (rockstar)]

<rockstar> leonardr, hi. Something is fishy about this diff.
<rockstar> leonardr, is it not based on one of the previous branches I reviewed?
<rockstar> leonardr, nevermind. I plead temporary insanity.
<rockstar> leonardr, so, if there's no mutator specified for 2.0, it uses the mutator specified from 1.0?
<leonardr> rockstar: right
<leonardr> same as other named operations
<rockstar> leonardr, so is "beta" the most recent version, and then 3.0, 2.0, and 1.0?
<leonardr> rockstar: right, according to the list of versions defined in the iwebserviceconfiguration

review: Approve (code)

[Leonard Richardson (leonardr)]

Wait, actually, the list of versions goes beta, 1.0, 2.0, 3.0. Not beta, 3.0, 2.0, 1.0. I hope you haven't found a problem.

[Paul Hummer (rockstar)]

<rockstar> leonardr, actually, that would be a problem, wouldn't it?
<rockstar> beta should be newer than 3.0, which is newer than 2.0, etc.
<leonardr> rockstar: actually beta is the _earliest_ version, but 3.0 is newer than 2.0, etc
<rockstar> leonardr, what is meant by "earliest" though.
<rockstar> It shouldn't mean that 1.0 overrides it.
<leonardr> rockstar: well, right now launchpad has a 'beta' web service and nothing else
<leonardr> now that i have multi-version working i'm going to create a '1.0' version that's different
<rockstar> leonardr, yes, but then we'll go 1.0, and beta will be what 2.0 is created from.
<leonardr> that's what i mean by saying beta is the earliest
<leonardr> rockstar: no, we do have a 'floating development version', but in launchpad's case that is called 'trunk'
<leonardr> 'beta' is a specific version
<leonardr> that will become obsolete and stop changing
<rockstar> leonardr, huh, that's odd.
<rockstar> leonardr, okay, I guess it makes sense to have 1.0 supersede beta then.
<leonardr> rockstar: if you are confused it's likely others will be confused and think that 'beta' is the permanent cutting edge
<leonardr> but i don't know what we can do about that
<leonardr> it's more like gmail, which was in 'beta' for 5 years and now it's not
<leonardr> the only differnece is that we have to keep 'beta' around for a while
<rockstar> leonardr, yeah. Having worked on the other side of the REST API (on Tarmac), when the API changes now, baby Jesus cries.
<leonardr> thus the multi-version project
<rockstar> leonardr, I think "trunk" for bleeding edge is probably misnamed, since it's likely it could be in production as "trunk"
<leonardr> rockstar: it's not too late to change it
<leonardr> if you have a better idea
<rockstar> leonardr, well, just what I was assuming where beta is the floating development version.
<leonardr> my only alternative is 'dev' or 'development'
<rockstar> leonardr, are we going to release an API version on every release of Launchpad?
<leonardr> rockstar: no, API versions will correspond roughly with ubuntu releases
<leonardr> and will be retired at the same rate as ubuntu releases
<rockstar> Ah, okay.
<rockstar> So maybe "dev" would be appropriate, since (at this point in time), it would correspond to the scripts in Lucid.
<leonardr> flacoste: ^- what do you think?
<leonardr> incidentally, flacoste, the multiversion lazr.restful work is complete as of yesterday
<flacoste> leonardr: i saw good progress on that, great! but complete? i guess we still need some changes in the client support, no?
<flacoste> leonardr: btw, i don't have an opinion on dev vs trunk
<flacoste> we could call it 'unstable'
<leonardr> flacoste: yes, we need to add multiversion to lazr.restfulclient

=== modified file 'src/lazr/restful/declarations.py'
--- src/lazr/restful/declarations.py	2010-02-05 14:30:30 +0000
+++ src/lazr/restful/declarations.py	2010-02-08 19:15:22 +0000
@@ -894,14 +894,8 @@
             tags = tags[0]
             if tags.get('exported') is False:
                 continue
-            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, {}))
+            mutated_by, mutated_by_annotations = tags.get(
+                'mutator_annotations', (None, {}))
             readonly = (field.readonly and mutated_by is None)
             attrs[tags['as']] = copy_field(
                 field, __name__=tags['as'], readonly=readonly)
@@ -941,7 +935,6 @@
     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:
@@ -954,12 +947,8 @@
             # this version's adapter class dictionary.
             if tags.get('exported') is False:
                 continue
-            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, {}))
+            mutator, annotations = tags.get(
+                'mutator_annotations', (None, {}))
 
             if mutator is None:
                 property = Passthrough(name, 'context')
@@ -1222,13 +1211,18 @@
     Since we have the list of versions available, this is also a good
     time to do some error checking: make sure that version annotations
     are not duplicated or in the wrong order.
+
+    Finally, this is a good time to integrate the mutator annotations
+    into the field annotations.
     """
     versioned_dict = field.queryTaggedValue(LAZR_WEBSERVICE_EXPORTED)
+    mutator_annotations = field.queryTaggedValue(LAZR_WEBSERVICE_MUTATORS)
 
     # If the first version is None and the second version is the
     # earliest version, consolidate the two versions.
     earliest_version = versions[0]
     stack = versioned_dict.stack
+
     if (len(stack) >= 2
         and stack[0][0] is None and stack[1][0] == earliest_version):
 
@@ -1253,6 +1247,20 @@
     if stack[0][0] is None:
         stack[0] = (earliest_version, stack[0][1])
 
+    # Make sure there is at most one mutator for the earliest version.
+    # If there is one, move it from the mutator-specific dictionary to
+    # the normal tag stack.
+    implicit_earliest_mutator = mutator_annotations.get(None, None)
+    explicit_earliest_mutator = mutator_annotations.get(earliest_version, None)
+    if (implicit_earliest_mutator is not None
+        and explicit_earliest_mutator is not None):
+        raise ValueError(
+            error_prefix + " Both implicit and explicit mutator definitions "
+            "found for earliest version %s." % earliest_version)
+    earliest_mutator = implicit_earliest_mutator or explicit_earliest_mutator
+    if earliest_mutator is not None:
+        stack[0][1]['mutator_annotations'] = earliest_mutator
+
     # Do some error checking.
     max_index = -1
     for version, tags in versioned_dict.stack:
@@ -1279,7 +1287,10 @@
         # The field is not initially published.
         new_stack = (earliest_version, dict(published=False))
     most_recent_tags = new_stack[0][1]
+    most_recent_mutator_tags = earliest_mutator
     for version in versions[1:]:
+        most_recent_mutator_tags = mutator_annotations.get(
+            version, most_recent_mutator_tags)
         match = [(stack_version, stack_tags)
                  for stack_version, stack_tags in stack
                  if stack_version == version]
@@ -1293,6 +1304,12 @@
             new_stack.append(match[0])
             most_recent_tags = match[0][1]
 
+        # Install a (possibly inherited) mutator for this field in
+        # this version.
+        if most_recent_mutator_tags is not None:
+            new_stack[-1][1]['mutator_annotations'] = copy.deepcopy(
+                most_recent_mutator_tags)
+
     versioned_dict.stack = new_stack
     return field
 
@@ -1319,34 +1336,3 @@
         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-05 14:30:30 +0000
+++ src/lazr/restful/docs/webservice-declarations.txt	2010-02-08 19:15:22 +0000
@@ -1808,16 +1808,6 @@
     AttributeError: 'MultiVersionEntryEntry_3_0Adapter' object has no
     attribute 'new_in_10'
 
-[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.
-
- An entry can have different operations marked as the destructor for
- different versions.]
-
 Why the list of version strings?
 ********************************
 
@@ -2327,9 +2317,9 @@
     ...     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.
+    ValueError: Field "field" in interface
+    "IImplicitAndExplicitMutator": Both implicit and explicit mutator
+    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

=== modified file 'src/lazr/restful/example/multiversion/resources.py'
--- src/lazr/restful/example/multiversion/resources.py	2010-01-27 15:05:56 +0000
+++ src/lazr/restful/example/multiversion/resources.py	2010-02-08 19:15:22 +0000
@@ -6,26 +6,57 @@
            'PairSet']
 
 from zope.interface import implements
-from zope.schema import Text
+from zope.schema import Bool, Text
 from zope.location.interfaces import ILocation
 
 from lazr.restful.declarations import (
     collection_default_content, export_as_webservice_collection,
-    export_as_webservice_entry, export_operation_as,
-    export_read_operation, exported, operation_for_version,
-    operation_parameters, operation_removed_in_version)
+    export_as_webservice_entry, export_destructor_operation,
+    export_operation_as, export_read_operation, export_write_operation,
+    exported, mutator_for, operation_for_version, operation_parameters,
+    operation_removed_in_version)
 
 # Our implementations of these classes can be based on the
 # implementations from the WSGI example.
 from lazr.restful.example.wsgi.resources import (
     PairSet as BasicPairSet, KeyValuePair as BasicKeyValuePair)
 
+
 # Our interfaces _will_ diverge from the WSGI example interfaces, so
 # define them separately.
 class IKeyValuePair(ILocation):
     export_as_webservice_entry()
     key = exported(Text(title=u"The key"))
     value = exported(Text(title=u"The value"))
+    a_comment = exported(Text(title=u"A comment on this key-value pair.",
+                              readonly=True),
+                         ('1.0', dict(exported=True, exported_as='comment')))
+    deleted = exported(Bool(title=u"Whether this key-value pair has been "
+                            "deleted"),
+                       ('3.0', dict(exported=True)), exported=False)
+    @mutator_for(a_comment)
+    @export_write_operation()
+    @operation_parameters(comment=Text())
+    @operation_for_version('1.0')
+    def comment_mutator_1(comment):
+        """A comment mutator that adds some junk on the end."""
+
+    @mutator_for(a_comment)
+    @export_write_operation()
+    @operation_parameters(comment=Text())
+    @operation_for_version('3.0')
+    def comment_mutator_2(comment):
+        """A comment mutator that adds different junk on the end."""
+
+    @export_destructor_operation()
+    @operation_for_version('1.0')
+    def total_destruction():
+        """A destructor that removes the key-value pair altogether."""
+
+    @export_destructor_operation()
+    @operation_for_version('3.0')
+    def mark_as_deleted():
+        """A destructor that simply sets .deleted to True."""
 
 
 class IPairSet(ILocation):
@@ -69,5 +100,27 @@
     def getNonEmptyPairs(self):
         return [pair for pair in self.pairs if pair.value is not None]
 
+
 class KeyValuePair(BasicKeyValuePair):
     implements(IKeyValuePair)
+
+    def __init__(self, pairset, key, value):
+        super(KeyValuePair, self).__init__(pairset, key, value)
+        self.a_comment = ''
+        self.deleted = False
+
+    def comment_mutator_1(self, comment):
+        """A comment mutator."""
+        self.a_comment = comment + " (modified by mutator #1)"
+
+    def comment_mutator_2(self, comment):
+        """A comment mutator."""
+        self.a_comment = comment + " (modified by mutator #2)"
+
+    def total_destruction(self):
+        """Remove the pair from the pairset."""
+        self.set.pairs.remove(self)
+
+    def mark_as_deleted(self):
+        """Set .deleted to True."""
+        self.deleted = True

=== modified file 'src/lazr/restful/example/multiversion/root.py'
--- src/lazr/restful/example/multiversion/root.py	2010-01-27 15:05:56 +0000
+++ src/lazr/restful/example/multiversion/root.py	2010-02-08 19:15:22 +0000
@@ -47,7 +47,9 @@
         pairset.pairs = [
             KeyValuePair(pairset, "foo", "bar"),
             KeyValuePair(pairset, "1", "2"),
-            KeyValuePair(pairset, "Some", None)
+            KeyValuePair(pairset, "Some", None),
+            KeyValuePair(pairset, "Delete", "me"),
+            KeyValuePair(pairset, "Also delete", "me")
             ]
         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-27 15:05:56 +0000
+++ src/lazr/restful/example/multiversion/tests/introduction.txt	2010-02-08 19:15:22 +0000
@@ -68,8 +68,8 @@
     HTTP/1.1 404 Not Found
     ...
 
-Collections and entries
-=======================
+Collections
+===========
 
 The web service presents a single collection of key-value pairs. In
 versions previous to 2.0, the collection omits key-value pairs where
@@ -83,25 +83,163 @@
 
     >>> show_pairs('beta')
     1: 2
+    Also delete: me
+    Delete: me
     foo: bar
 
     >>> show_pairs('1.0')
     1: 2
+    Also delete: me
+    Delete: me
     foo: bar
 
     >>> show_pairs('2.0')
     1: 2
+    Also delete: me
+    Delete: me
     Some: None
     foo: bar
 
     >>> show_pairs('3.0')
     1: 2
+    Also delete: me
+    Delete: me
     Some: None
     foo: bar
 
-    >>> body = webservice.get('/pairs/foo').jsonBody()
-    >>> print body['key'], body['value']
-    foo bar
+Entries
+=======
+
+Let's take a look at 'comment' and 'deleted', two fields with
+interesting properties.
+
+The 'comment' field is not modified directly, but by internal mutator
+methods which append some useless text to your comment.
+
+    >>> import simplejson
+    >>> def get_comment(version):
+    ...     response = webservice.get("/pairs/foo", api_version=version)
+    ...     return response.jsonBody()['comment']
+
+    >>> def change_comment(comment, version, get_comment_afterwards=True):
+    ...     ignored = webservice.patch(
+    ...         "/pairs/foo/", 'application/json',
+    ...         simplejson.dumps({"comment": comment}),
+    ...         api_version=version)
+    ...     if get_comment_afterwards:
+    ...         return get_comment(version)
+    ...     return None
+
+    >>> get_comment('1.0')
+    u''
+    >>> print change_comment('I changed 1.0', '1.0')
+    I changed 1.0 (modified by mutator #1)
+
+    >>> print change_comment('I changed 2.0', '2.0')
+    I changed 2.0 (modified by mutator #1)
+
+    >>> print change_comment('I changed 3.0', '3.0')
+    I changed 3.0 (modified by mutator #2)
+
+You can try to modify the 'comment' field from a version that doesn't
+publish that field, but lazr.restful will ignore your request.
+
+    >>> change_comment('I changed beta', 'beta', False)
+
+    >>> print get_comment('1.0')
+    I changed 3.0 (modified by mutator #2)
+
+A field called 'deleted' is published starting in version '3.0'. A
+comment field is called 'a_comment' in version 'beta' and 'comment' in
+all later versions.
+
+    >>> def show_fields(version):
+    ...     entry_body = webservice.get(
+    ...         '/pairs/foo', api_version=version).jsonBody()
+    ...     for key in sorted(entry_body.keys()):
+    ...         print key
+
+    >>> show_fields('beta')
+    a_comment
+    http_etag
+    key
+    resource_type_link
+    self_link
+    value
+
+    >>> show_fields('1.0')
+    comment
+    http_etag
+    key
+    resource_type_link
+    self_link
+    value
+
+    >>> show_fields('3.0')
+    comment
+    deleted
+    http_etag
+    key
+    resource_type_link
+    self_link
+    value
+
+In the 'beta' version, attempting to delete a key-value pair will
+result in a status code of 405 ("Method Not Available").
+
+    >>> response = webservice.delete('/pairs/Delete', api_version='beta')
+    >>> response.status
+    405
+
+As of '1.0', attempting to delete a key-value pair results in the
+key-value pair being totally removed from the web service.
+
+    >>> ignore = webservice.delete('/pairs/Delete', api_version='1.0')
+    >>> show_pairs('beta')
+    1: 2
+    Also delete: me
+    foo: bar
+
+In '3.0', deleting a key-value pair simply sets its 'deleted' field
+to True. (This is an abuse of the HTTP DELETE method, but it makes a
+good demo.)
+
+    >>> body = webservice.get(
+    ...         '/pairs/Also%20delete', api_version='3.0').jsonBody()
+    >>> body['deleted']
+    False
+
+    >>> ignore = webservice.delete('/pairs/Also%20delete', api_version='3.0')
+
+The "deleted" key-value pair is still visible in all versions:
+
+    >>> show_pairs('beta')
+    1: 2
+    Also delete: me
+    foo: bar
+
+And in a version which publishes the 'delete' field, we can check the
+key-value pair's value for that field.
+
+    >>> body = webservice.get(
+    ...         '/pairs/Also%20delete', api_version='3.0').jsonBody()
+    >>> body['deleted']
+    True
+
+Fields
+======
+
+If an entry field is not published in a certain version, the
+corresponding field resource does not exist for that version.
+
+    >>> webservice.get('/pairs/foo/deleted', api_version='beta')
+    Traceback (most recent call last):
+    ...
+    NotFound: ... name: u'deleted'
+
+    >>> print webservice.get(
+    ...     '/pairs/foo/deleted', api_version='3.0').jsonBody()
+    False
 
 Named operations
 ================