Merge lp:~leonardr/lazr.restful/multi-part-etag into lp:lazr.restful

[Leonard Richardson]

OK, this branch will hopefully go a long way towards avoiding the 412 errors that results in bugs like bug 336866 and bug 534066. The solution is as described here: https://bugs.edge.launchpad.net/lazr.restful/+bug/336866/comments/41

An entry resource's ETag now has two components: a component describing the state of the read-only fields, and a component describing the state of the read-write fields as well as global information like the current code revision. Conditional GET requests still check the entire ETag, but conditional PUT and PATCH requests only check the read-write portion. This way, server-side changes to read-only fields don't cause PATCH requests to fail. A PATCH request should only fail if there's some possibility that _some other user_ made a conflicting change.

So, let's say a bug's Etag is "foo-bar". If you make a GET request with an If-None-Match of "foo-bar", you'll get 304 Not Modified. If you make a PATCH request with an If-Match of "foo-bar" your request will succeed.

Now let's say some behind-the-scenes cron job runs, and the bug's bug_heat changes. Now the bug's ETag is "baz-bar". Only the first part changed, because bug_heat is not writable. Now, if you make a GET request with an If-None-Match of "foo-bar", you'll get 200 Ok and a new representation with the new value of bug_heat. The request condition will fail. But if you make a PATCH request with an If-Match of "foo-bar", the condition will succeed and your PATCH will go through. After your PATCH goes through, the bug's ETag will be "baz-quux", and a PATCH with an If-Match of "foo-bar" or "baz-bar" will fail.

Hopefully the doctests will make it clear how this system works.

[Graham Binns (gmb)]

review: Approve (code)

=== modified file 'src/lazr/restful/_resource.py'
--- src/lazr/restful/_resource.py	2010-02-18 15:02:10 +0000
+++ src/lazr/restful/_resource.py	2010-03-15 20:24:26 +0000
@@ -276,9 +276,11 @@
         """Handle a possible conditional PUT or PATCH request.
 
         This method has side effects. If the write operation should
-        not continue, because the incoming ETag doesn't match the
-        generated ETag, it sets the response code to 412
-        ("Precondition Failed").
+        not continue, because the read-write portion of the incoming
+        ETag doesn't match the read-write portion of the generated
+        ETag, it sets the response code to 412 ("Precondition
+        Failed"). It's okay for the read-only portion of the incoming
+        ETag not to match.
 
         If the PUT or PATCH request is being tunneled through POST
         with X-HTTP-Method-Override, the media type of the incoming
@@ -307,7 +309,7 @@
             # This is not a conditional write.
             return media_type
         existing_etag = self.getETag(media_type)
-        if existing_etag in incoming_etags:
+        if self._etagMatchesForWrite(existing_etag, incoming_etags):
             # The conditional write can continue.
             return media_type
         # The resource has changed since the client requested it.
@@ -316,10 +318,17 @@
         self.request.response.setStatus(412)
         return None
 
-    def _getETagCore(self, cache=None):
+    def _etagMatchesForWrite(self, existing_etag, incoming_etags):
+        """Perform an ETag match to see if conditional write is OK.
+
+        By default, we match the full ETag strings against each other.
+        """
+        return existing_etag in incoming_etags
+
+    def _getETagCores(self, cache=None):
         """Calculate the core ETag for a representation.
 
-        :return: a string that will be used to calculate the full
+        :return: a list of string that will be used to calculate the full
             ETag. If None is returned, no ETag at all will be
             calculated for the given resource and media type.
         """
@@ -346,29 +355,32 @@
             # The WADL representation of a resource only changes when
             # the software itself changes. Thus, we don't need any
             # special information for its ETag core.
-            etag_core = ''
+            etag_cores = ['']
         else:
             # For other media types, we calculate the ETag core by
             # delegating to the subclass.
-            etag_core = self._getETagCore(cache)
+            etag_cores = self._getETagCores(cache)
 
-        if etag_core is None:
+        if etag_cores is None:
             return None
 
-        hash_object = sha_constructor()
-        hash_object.update(etag_core)
+        core_hashes = []
+        for core in etag_cores:
+            hash_object = sha_constructor()
+            hash_object.update(core)
+            core_hashes.append(hash_object)
 
         # Append the media type, so that web browsers won't treat
         # different representations of a resource interchangeably.
-        hash_object.update("\0" + media_type)
+        core_hashes[-1].update("\0" + media_type)
 
         # Append the revision number, because the algorithm for
         # generating the representation might itself change across
         # versions.
         revno = getUtility(IWebServiceConfiguration).code_revision
-        hash_object.update("\0" + revno)
+        core_hashes[-1].update("\0" + revno)
 
-        etag = '"%s"' % hash_object.hexdigest()
+        etag = '"%s"' % "-".join([core.hexdigest() for core in core_hashes])
         self.etags_by_media_type[media_type] = etag
         return etag
 
@@ -1200,14 +1212,14 @@
 
     do_PATCH = do_PUT
 
-    def _getETagCore(self, unmarshalled_field_values=None):
+    def _getETagCores(self, unmarshalled_field_values=None):
         """Calculate the ETag for an entry field.
 
         The core of the ETag is the field value itself.
         """
         name, value = self._unmarshallField(
             self.context.name, self.context.field)
-        return str(value)
+        return [str(value)]
 
     def _representation(self, media_type):
         """Create a representation of the field value."""
@@ -1265,23 +1277,58 @@
         super(EntryResource, self).__init__(context, request)
         self.entry = getMultiAdapter((context, request), IEntry)
 
-    def _getETagCore(self, unmarshalled_field_values=None):
+    def _getETagCores(self, unmarshalled_field_values=None):
         """Calculate the ETag for an entry.
 
         :arg unmarshalled_field_values: A dict mapping field names to
         unmarshalled values, obtained during some other operation such
         as the construction of a representation.
         """
-        values = []
+        unwritable_values = []
+        writable_values = []
         for name, field in getFieldsInOrder(self.entry.schema):
-            if self.isModifiableField(field, False):
-                if (unmarshalled_field_values is not None
-                    and unmarshalled_field_values.get(name)):
-                    value = unmarshalled_field_values[name]
-                else:
-                    ignored, value = self._unmarshallField(name, field)
-                values.append(decode_value(value))
-        return "\0".join(values).encode("utf-8")
+            if self.isModifiableField(field, True):
+                # The client can write to this value.
+                bucket = writable_values
+            elif self.isModifiableField(field, False):
+                # The client can't write this value, but it still
+                # might change.
+                bucket = unwritable_values
+            else:
+                # This value can never change, and as such does not
+                # need to be included in the ETag.
+                continue
+            if (unmarshalled_field_values is not None
+                and unmarshalled_field_values.get(name)):
+                value = unmarshalled_field_values[name]
+            else:
+                ignored, value = self._unmarshallField(name, field)
+            bucket.append(decode_value(value))
+
+        unwritable = "\0".join(unwritable_values).encode("utf-8")
+        writable = "\0".join(writable_values).encode("utf-8")
+        return [unwritable, writable]
+
+
+    def _etagMatchesForWrite(self, existing_etag, incoming_etags):
+        """Make sure no other client has modified this resource.
+
+        For a conditional read to succeed, the entire ETag must
+        match. But for a conditional write to succeed, only the second
+        half of the ETag must match. This prevents spurious 412 errors
+        on conditional writes where the only fields that changed are
+        read-only fields that can't possibly cause a conflict.
+        """
+        existing_write_portion = existing_etag.split('-', 1)[-1]
+        for etag in incoming_etags:
+            if '-' in etag:
+                incoming_write_portion = etag.rsplit('-', 1)[-1]
+            else:
+                incoming_write_portion = etag
+            if existing_write_portion == incoming_write_portion:
+                return True
+        return False
+
 
     def toDataForJSON(self):
         """Turn the object into a simple data structure."""
@@ -1552,14 +1599,14 @@
         """Fetch the current web service request."""
         return get_current_web_service_request()
 
-    def _getETagCore(self, cache=None):
+    def _getETagCores(self, cache=None):
         """Calculate an ETag for a representation of this resource.
 
         The service root resource changes only when the software
         itself changes. This information goes into the ETag already,
         so there's no need to provide anything.
         """
-        return ''
+        return ['']
 
     def __call__(self, REQUEST=None):
         """Handle a GET request."""

=== modified file 'src/lazr/restful/example/base/tests/entry.txt'
--- src/lazr/restful/example/base/tests/entry.txt	2010-01-25 20:56:57 +0000
+++ src/lazr/restful/example/base/tests/entry.txt	2010-03-15 20:24:26 +0000
@@ -543,6 +543,151 @@
     HTTP/1.1 412 Precondition Failed
     ...
 
+Conditional writes are a little more complicated
+************************************************
+
+OK, but consider the 'copyright_date' field of a cookbook. This is a
+read-only field; the client can't change it. But what if it changes on
+the server-side? What happens to the ETag then? Does it change,
+causing a conditional PATCH to fail, even if the PATCH doesn't touch
+that read-only field?
+
+    >>> greens = webservice.get(greens_url).jsonBody()
+    >>> print greens['copyright_date']
+    2003-01-01
+    >>> etag_before_server_modification = greens['http_etag']
+
+    >>> from lazr.restful.example.base.root import C4
+    >>> greens_object = C4
+    >>> old_date = greens_object.copyright_date
+    >>> old_date
+    datetime.date(2003, 1, 1)
+
+Let's change the server-side value and find out.
+
+    >>> import datetime
+    >>> greens_object.copyright_date = datetime.date(2005, 12, 12)
+
+    >>> new_greens = webservice.get(greens_url).jsonBody()
+    >>> print new_greens['copyright_date']
+    2005-12-12
+    >>> etag_after_server_modification = new_greens['http_etag']
+
+The ETag has indeed changed.
+
+    >>> etag_before_server_modification == etag_after_server_modification
+    False
+
+So if we try to modify the cookbook using the old ETag, it should
+fail, right?
+
+    >>> body = {'description' : 'New description.'}
+    >>> print modify_cookbook('Everyday Greens', body, 'PATCH',
+    ...                       {'If-Match' : etag_before_server_modification})
+    HTTP/1.1 209 Content Returned
+    ...
+
+Actually, it succeeds! How does that work? Well, the ETag consists of
+two parts, separated by a dash.
+
+    >>> read_before, write_before = etag_before_server_modification.split('-')
+    >>> read_after, write_after = etag_after_server_modification.split('-')
+
+The first part of the ETag only changes when a field the client can't
+modify changes. This is the part of the ETag that changed when
+copyright_date changed.
+
+    >>> read_before == read_after
+    False
+
+The second part only changes when a field changes that the client can
+modify. This part of the ETag hasn't changed.
+
+    >>> write_before == write_after
+    True
+
+When you make a conditional write, the second part of the ETag you
+provide is checked against the second part of the ETag generated on
+the server. The first part of the ETag is ignored.
+
+The point of checking the ETag is to avoid conflicts where two clients
+modify the same resource. But no client can modify any of those
+read-only fields, so changes to them don't matter for purposes of
+avoiding conflicts.
+
+If you hack the ETag to something that's not "two parts, separated by
+a dash", lazr.restful will still handle it. (Of course, since that
+ETag will never match anything, you'll always get a 412 error.)
+
+    >>> body = {'description' : 'New description.'}
+    >>> print modify_cookbook('Everyday Greens', body, 'PATCH',
+    ...                       {'If-Match' : "Weird etag"})
+    HTTP/1.1 412 Precondition Failed
+    ...
+
+Conditional PUT fails where a PATCH would succeed, but not because
+lazr.restful rejects an old ETag. To verify this, let's change the
+cookbook's copyright_date again, behind the scenes.
+
+    >>> greens = webservice.get(greens_url).jsonBody()
+    >>> old_etag = greens['http_etag']
+    >>> greens_object.copyright_date = datetime.date(2005, 11, 11)
+
+A totally bogus ETag fails with a 412 error.
+
+    >>> greens['description'] = 'Another new description'
+    >>> print modify_cookbook('Everyday Greens', greens, 'PUT',
+    ...                       {'If-Match' : "Not the old ETag"})
+    HTTP/1.1 412 Precondition Failed
+    ...
+
+When we use the original ETag, we don't cause a 412 error, but the PUT
+request fails anyway.
+
+    >>> print modify_cookbook('Everyday Greens', greens, 'PUT',
+    ...                       {'If-Match' : old_etag})
+    HTTP/1.1 400 Bad Request
+    ...
+    http_etag: You tried to modify a read-only attribute.
+    copyright_date: You tried to modify a read-only attribute.
+
+Rather, it's because a PUT request includes a representation of the
+entire resource, and lazr.restful thinks the client is trying to
+modify the fields that changed behind the scenes--in this case,
+copyright_date and http_etag.
+
+Conditional reads are *not* more complicated
+********************************************
+
+The point of the two-part ETag is to avoid spurious 412 errors when
+doing conditional writes. When making a conditional _read_ request,
+the condition will fail if _any_ part of the ETag is different.
+
+    >>> new_etag = webservice.get(greens_url).jsonBody()['http_etag']
+
+    >>> print webservice.get(
+    ...     greens_url,
+    ...     headers={'If-None-Match': new_etag})
+    HTTP/1.1 304 Not Modified
+    ...
+
+    >>> print webservice.get(
+    ...     greens_url,
+    ...     headers={'If-None-Match': "changed" + new_etag})
+    HTTP/1.1 200 Ok
+    ...
+
+lazr.restful checks the entire ETag on conditional GET because the
+purpose of a conditional read is to avoid getting data that hasn't
+changed. A server-side change to a read-only field like copyright_date
+doesn't affect future writes, but it _is_ a change to the
+representation.
+
+A bit of cleanup: restore the old value for the cookbook's
+copyright_date.
+
+    >>> greens_object.copyright_date = old_date
+
 Changing object relationships
 =============================
 
@@ -621,6 +766,7 @@
 The tests that follow make a number of PATCH requests that include
 values for a cookbook's 'copyright_date' attribute.
 
+    >>> greens = webservice.get(greens_url).jsonBody()
     >>> greens['copyright_date']
     u'2003-01-01'
 

=== modified file 'src/lazr/restful/version.txt'
--- src/lazr/restful/version.txt	2010-03-11 20:33:25 +0000
+++ src/lazr/restful/version.txt	2010-03-15 20:24:26 +0000
@@ -1,1 +1,1 @@
-0.9.23
+0.9.24