Merge lp:~leonardr/lazr.restful/representation_cache into lp:lazr.restful

[Leonard Richardson]

[Leonard Richardson (leonardr)]

Test: bin/test
Diff: https://pastebin.canonical.com/16818/

This branch fixes a lazr.restful bug that was manifesting in Launchpad
as bug 336866: sending 412 ("Precondition Failed") to people who
messed with bugs.

The problem was that IBug.date_last_modified was being modified behind
the scened by an event listener. The old value for date_last_modified
wasn't removed from the cache of field representations, and so it got
reused when we sent the representation in response to the PATCH
request. We sent a transitional representation, containing all the changes
the client had made, but with the old value for date_last_modified.

The ETag we calculated for that representation was also
transitional. When the client used it on its next request, the
transitional ETag was compared against the ETag for the fullly new
representation (including the change to date_last_modified), and the
result was a 412 error.

date_last_modified is changed by an ObjectModifiedEvent listener. The
listeners run synchronously, so by the time control returns to
lazr.restful, the field has been changed. The trick is figuring out
which fields were changed.

This branch fixes the problem by changing the way the cache is
invalidated. Instead of iterating over the changeset, we iterate over
the cache. We have a snapshot of the object before it was changed, and
for each field in the cache, we compare the old object's value for
that field to the new object's value. If they're different, we clear
that value from the cache.

To properly test the branch, I introduced a date_last_modified-like field
to the sample web service. It's updated behind the scenes every time a
client changes a cookbook.

I had to tweak the tests a little bit because they were constantly sending PATCH requests that didn't change anything and assuming that the old ETags were still valid because nothing changed. Now that revision_number is updated every time you make a PATCH request, you can't reuse ETags.

[Guilherme Salgado (salgado)]

Hi Leonard,

This is a very nice and clean branch. Just one suggestion below.

 review approve
 status approved

On Fri, 2009-04-24 at 15:40 +0000, Leonard Richardson wrote:
> Leonard Richardson has proposed merging
> lp:~leonardr/lazr.restful/representation_cache into lp:lazr.restful.

> === modified file 'src/lazr/restful/example/interfaces.py'
> --- src/lazr/restful/example/interfaces.py 2009-04-22 20:36:10 +0000
> +++ src/lazr/restful/example/interfaces.py 2009-04-24 15:14:07 +0000
> @@ -110,6 +110,8 @@
> description=u"The copyright date for this work."))
> description = exported(
> WhitespaceStrippingTextLine(title=u"Description", required=False))
> + revision_number = exported(Int(title=u"A record of the number of times "
> + "this cookbook has been modified."))

The line above should be indented to the same level of the beginning of
the argument's value on the previous line, like

    Int(foo='Bar and '
            'Baz')

> confirmed = exported(Bool(
> title=u"Whether this information has been confirmed",
> default=False))
>

>
--
Guilherme Salgado <email address hidden>

review: Approve

=== modified file 'src/lazr/restful/_resource.py'
--- src/lazr/restful/_resource.py	2009-04-15 00:24:52 +0000
+++ src/lazr/restful/_resource.py	2009-04-24 15:28:26 +0000
@@ -1309,11 +1309,6 @@
         # Make the changes.
         for field, (name, value) in validated_changeset.items():
             field.set(self.entry, value)
-            # Clear any marshalled value for this field from the
-            # cache, so that the upcoming representation generation doesn't
-            # use the cached value.
-            if name in self._unmarshalled_field_cache:
-                del(self._unmarshalled_field_cache[name])
         # The representation has changed, and etags will need to be
         # recalculated.
         self.etags_by_media_type = {}
@@ -1325,6 +1320,20 @@
             edited_fields=validated_changeset.keys())
         notify(event)
 
+        # The changeset contained new values for some of this object's
+        # fields, and the notification event may have changed others.
+        # Clear out any fields that changed.
+        for name, ignored in self._unmarshalled_field_cache.items():
+            force_clear = False
+            try:
+                old_value = getattr(entry_before_modification, name, None)
+                new_value = getattr(self.entry.context, name, None)
+            except Unauthorized:
+                # Clear the cache, just to be safe.
+                force_clear = True
+            if force_clear or new_value != old_value:
+                del(self._unmarshalled_field_cache[name])
+
         # If the modification caused the entry's URL to change, tell
         # the client about the new URL.
         new_url = absoluteURL(self.context, self.request)

=== modified file 'src/lazr/restful/example/configure.zcml'
--- src/lazr/restful/example/configure.zcml	2009-04-22 17:56:52 +0000
+++ src/lazr/restful/example/configure.zcml	2009-04-24 15:14:07 +0000
@@ -15,9 +15,9 @@
   <class class="lazr.restful.example.root.Cookbook">
     <require attributes="name copyright_date cover cuisine description
                          recipes find_recipe_for find_recipes last_printing
-                         make_more_interesting price get"
+                         make_more_interesting price revision_number get"
              set_attributes="name copyright_date cover cuisine description
-                             last_printing price"
+                             last_printing price revision_number"
              permission="zope.Public" />
     <require attributes="confirmed"
              set_attributes="confirmed"
@@ -105,4 +105,10 @@
       name="absolute_url"
       />
 
+  <subscriber
+   for="lazr.restful.example.interfaces.ICookbook
+        lazr.lifecycle.interfaces.IObjectModifiedEvent"
+   handler="lazr.restful.example.subscribers.update_cookbook_revision_number" />
+
+
 </configure>

=== modified file 'src/lazr/restful/example/interfaces.py'
--- src/lazr/restful/example/interfaces.py	2009-04-22 20:36:10 +0000
+++ src/lazr/restful/example/interfaces.py	2009-04-24 15:14:07 +0000
@@ -110,6 +110,8 @@
              description=u"The copyright date for this work."))
     description = exported(
         WhitespaceStrippingTextLine(title=u"Description", required=False))
+    revision_number = exported(Int(title=u"A record of the number of times "
+                                   "this cookbook has been modified."))
     confirmed = exported(Bool(
             title=u"Whether this information has been confirmed",
             default=False))

=== modified file 'src/lazr/restful/example/root.py'
--- src/lazr/restful/example/root.py	2009-04-22 19:52:53 +0000
+++ src/lazr/restful/example/root.py	2009-04-24 15:14:07 +0000
@@ -88,6 +88,7 @@
         self.price = price
         self.confirmed = confirmed
         self.cover = None
+        self.revision_number = 0
 
     @property
     def __name__(self):

=== added file 'src/lazr/restful/example/subscribers.py'
--- src/lazr/restful/example/subscribers.py	1970-01-01 00:00:00 +0000
+++ src/lazr/restful/example/subscribers.py	2009-04-24 15:14:07 +0000
@@ -0,0 +1,13 @@
+# Copyright 2009 Canonical Ltd.  All rights reserved.
+
+"""Event listeners for the example web service."""
+
+__metaclass__ = type
+__all__ = ['update_cookbook_revision_number']
+
+from lazr.restful.example.interfaces import ICookbook
+
+def update_cookbook_revision_number(object, event):
+    """Increment ICookbook.revision_number."""
+    if ICookbook.providedBy(object):
+        object.revision_number += 1

=== modified file 'src/lazr/restful/example/tests/collection.txt'
--- src/lazr/restful/example/tests/collection.txt	2009-04-22 19:52:53 +0000
+++ src/lazr/restful/example/tests/collection.txt	2009-04-24 15:14:07 +0000
@@ -136,6 +136,7 @@
     price: 20
     recipes_collection_link: u'http://.../cookbooks/The%20Joy%20of%20Cooking/recipes'
     resource_type_link: u'http://...#cookbook'
+    revision_number: 0
     self_link: u'http://.../cookbooks/The%20Joy%20of%20Cooking'
 
 A collection may be scoped to an element:

=== modified file 'src/lazr/restful/example/tests/entry.txt'
--- src/lazr/restful/example/tests/entry.txt	2009-04-16 20:45:55 +0000
+++ src/lazr/restful/example/tests/entry.txt	2009-04-24 15:14:07 +0000
@@ -266,17 +266,28 @@
 Greens" cookbook. The data returned is the new JSON representation of
 the object.
 
-    >>> print modify_cookbook('Everyday Greens', {'cuisine' : 'American'},
-    ...                       'PATCH')
+    >>> print webservice.get(greens_url).jsonBody()['revision_number']
+    0
+
+    >>> result = modify_cookbook('Everyday Greens', {'cuisine' : 'American'},
+    ...                          'PATCH')
+    >>> print result
     HTTP/1.1 209 Content Returned
     ...
     Content-Type: application/json
     ...
     {...}
 
-    >>> print webservice.get(greens_url).jsonBody()['cuisine']
+    >>> greens = result.jsonBody()
+    >>> print greens['cuisine']
     American
 
+Whenever a client modifies a cookbook, the revision_number is
+incremented behind the scenes.
+
+    >>> print greens['revision_number']
+    1
+
 A modification might cause an entry's address to change. Here we use
 the web service to change the cookbook's name to 'Everyday Greens 2'.
 
@@ -363,7 +374,7 @@
 When making a PATCH, you don't have to get a JSON representation
 back. You can also get an HTML representation.
 
-    >>> print modify_cookbook('Everyday Greens', greens, 'PATCH',
+    >>> print modify_cookbook('Everyday Greens', {}, 'PATCH',
     ...                       headers={'Accept': 'application/xhtml+xml'})
     HTTP/1.1 209 Content Returned
     ...
@@ -375,7 +386,7 @@
 You can even get a WADL representation, though that's pretty useless.
 
     >>> headers = {'Accept':'application/vd.sun.wadl+xml'}
-    >>> print modify_cookbook('Everyday Greens', greens, 'PATCH',
+    >>> print modify_cookbook('Everyday Greens', {}, 'PATCH',
     ...                       headers=headers)
     HTTP/1.1 209 Content Returned
     ...
@@ -497,7 +508,8 @@
 If you specify a number of ETags, and any of them match, your request
 will go through.
 
-    >>> match = '"an-old-etag", %s' % greens_etag
+    >>> greens = webservice.get(greens_url).jsonBody()
+    >>> match = '"an-old-etag", %s' % greens['http_etag']
     >>> print modify_cookbook('Everyday Greens', greens, 'PATCH',
     ...                       {'If-Match' : match})
     HTTP/1.1 209 Content Returned