Merge lp:~benji/lazr.restful/tweak-etag into lp:lazr.restful

[Benji York]

The ETag generation in lazr.restful was a bit too conservative. It used the revision number in every ETag when it is only needed in a subset. This branch moves the inclusion of revno "down" into the subclasses that need it. Tests for the various ETag generating method/functions were added as well as tests for the (existing) read/write ETag checking routines.

Leonard and I had pre-, intra-, and post-implementation discussions.

[Paul Hummer (rockstar)]

review: Approve

[Stuart Bishop (stub)]

I notice you are adding a roman numeral package to buildout, but are not using it.

[Benji York (benji)]

On Thu, Sep 30, 2010 at 11:15 PM, Stuart Bishop
<email address hidden> wrote:
> I notice you are adding a roman numeral package to buildout, but are not using it.

That's a change from another branch that I'll probably be reverting. I
think I got a hold of a bad docutils somehow that was missing roman.
--
Benji York

[Leonard Richardson (leonardr)]

I have some minor comments, but overall this is a good branch.

In addition to the 'roman' package, you've got a normalizer that makes Unicode strings look the same as regular strings. I don't think that's the best way to solve your problem, but you should talk it over with Gary.

make_entry_etag_cores no longer cares whether a field "might change over time", so you should remove that bit from the docstring.

Similarly, you don't need "modifiable" in test_writable_fields and the similar tests. And you don't need test_modifiable_fields at all. It would be nice to test fields with this property, but you can't test them in a unit test of make_entry_etag_cores, which no longer looks at the 'modifiable' attribute.

Do you want to add a test to test_cores_change_with_value that shows a case where the write core changes and the read core stays the same?

[Benji York (benji)]

I fixed the roman problem by nuking my docutils eggs and reinstalling.

[lp:~benji/lazr.restful/tweak-etag]

156. By Benji York <benji@benji-laptop> on 2010-10-01

remove unneeded reNormalizer

[Benji York (benji)]

I made all the changes Leonard suggested except for the last one which we discussed away.

[lp:~benji/lazr.restful/tweak-etag]

157. By Benji York <benji@benji-laptop> on 2010-10-04

make some changes suggested in review

158. By Benji York <benji@benji-laptop> on 2010-10-04

add a comment about read/write fields

=== modified file 'src/lazr/restful/_resource.py'
--- src/lazr/restful/_resource.py	2010-09-23 19:14:55 +0000
+++ src/lazr/restful/_resource.py	2010-10-04 15:46:40 +0000
@@ -89,7 +89,10 @@
     IWebServiceConfiguration, IWebServiceLayer, IWebServiceVersion,
     LAZR_WEBSERVICE_NAME)
 from lazr.restful.utils import (
-    get_current_web_service_request, sorted_named_things)
+    extract_write_portion,
+    get_current_web_service_request,
+    sorted_named_things,
+    )
 
 
 # The path to the WADL XML Schema definition.
@@ -378,12 +381,6 @@
         # different representations of a resource interchangeably.
         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
-        core_hashes[-1].update("\0" + revno)
-
         etag = '"%s"' % "-".join([core.hexdigest() for core in core_hashes])
         self.etags_by_media_type[media_type] = etag
         return etag
@@ -1255,14 +1252,21 @@
 
     do_PATCH = do_PUT
 
-    def _getETagCores(self, unmarshalled_field_values=None):
+    def _getETagCores(self, cache=None):
         """Calculate the ETag for an entry field.
 
         The core of the ETag is the field value itself.
+
+        :arg cache: is ignored.
         """
-        name, value = self._unmarshallField(
-            self.context.name, self.context.field)
-        return [str(value)]
+        value = self._unmarshallField(
+            self.context.name, self.context.field)[1]
+
+        # Append the revision number, because the algorithm for
+        # generating the representation might itself change across
+        # versions.
+        revno = getUtility(IWebServiceConfiguration).code_revision
+        return [core.encode('utf-8') for core in [revno, unicode(value)]]
 
     def _representation(self, media_type):
         """Create a representation of the field value."""
@@ -1304,6 +1308,29 @@
         self.context = entryfield
         self.request = request
 
+def make_entry_etag_cores(field_details):
+    """Given the details of an entry's fields, calculate its ETag cores.
+
+    :arg field_details: A list of field names and relevant field information,
+    in particular whether or not the field is writable and its current value.
+    """
+    unwritable_values = []
+    writable_values = []
+    for name, details in field_details:
+        if details['writable']:
+            # The client can write to this value.
+            bucket = writable_values
+        else:
+            # The client can't write to this value (it might be read-only or
+            # it might just be non-web-service writable.
+            bucket = unwritable_values
+        bucket.append(decode_value(details['value']))
+
+    unwritable = "\0".join(unwritable_values).encode("utf-8")
+    writable = "\0".join(writable_values).encode("utf-8")
+    return [unwritable, writable]
+
+
 
 class EntryResource(CustomOperationResourceMixin,
                     FieldUnmarshallerMixin, EntryManipulatingResource):
@@ -1338,31 +1365,23 @@
         unmarshalled values, obtained during some other operation such
         as the construction of a representation.
         """
-        unwritable_values = []
-        writable_values = []
+        if unmarshalled_field_values is None:
+            unmarshalled_field_values = {}
+
+        field_details = []
         for name, field in getFieldsInOrder(self.entry.schema):
-            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]
-
+            details = {}
+            # Add in any values provided by the caller.
+            # The value of the field is either passed in, or extracted.
+            details['value'] = unmarshalled_field_values.get(
+                name, self._unmarshallField(name, field)[1])
+
+            # The client can write to this field.
+            details['writable'] = self.isModifiableField(field, True)
+
+            field_details.append((name, details))
+
+        return make_entry_etag_cores(field_details)
 
     def _etagMatchesForWrite(self, existing_etag, incoming_etags):
         """Make sure no other client has modified this resource.
@@ -1373,15 +1392,9 @@
         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
+        incoming_write_portions = map(extract_write_portion, incoming_etags)
+        existing_write_portion = extract_write_portion(existing_etag)
+        return existing_write_portion in incoming_write_portions
 
 
     def toDataForJSON(self):
@@ -1412,7 +1425,7 @@
                         "Cannot create data structure for media type %s"
                         % media_type)
             data[repr_name] = repr_value
-            unmarshalled_field_values[name] =  repr_value
+            unmarshalled_field_values[name] = repr_value
 
         etag = self.getETag(media_type, unmarshalled_field_values)
         data['http_etag'] = etag
@@ -1778,10 +1791,10 @@
         """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.
+        itself changes.
         """
-        return ['']
+        revno = getUtility(IWebServiceConfiguration).code_revision
+        return [revno.encode('utf-8')]
 
     def __call__(self, REQUEST=None):
         """Handle a GET request."""

=== modified file 'src/lazr/restful/testing/helpers.py'
--- src/lazr/restful/testing/helpers.py	2010-07-15 14:24:56 +0000
+++ src/lazr/restful/testing/helpers.py	2010-10-04 15:46:40 +0000
@@ -5,6 +5,12 @@
 from zope.interface import implements
 
 from lazr.restful.interfaces import IWebServiceConfiguration
+from lazr.restful.simple import (
+    Request,
+    RootResource,
+    )
+from lazr.restful.testing.webservice import WebServiceTestPublication
+from lazr.restful.utils import tag_request_with_version_name
 
 
 def create_test_module(name, *contents):
@@ -40,6 +46,12 @@
     last_version_with_mutator_named_operations = "1.0"
     code_revision = "1.0b"
     default_batch_size = 50
+    hostname = 'example.com'
 
     def get_request_user(self):
         return 'A user'
+
+    def createRequest(self, body_instream, environ):
+        request = Request(body_instream, environ)
+        request.setPublication(WebServiceTestPublication(RootResource))
+        return request

=== added file 'src/lazr/restful/tests/test_etag.py'
--- src/lazr/restful/tests/test_etag.py	1970-01-01 00:00:00 +0000
+++ src/lazr/restful/tests/test_etag.py	2010-10-04 15:46:40 +0000
@@ -0,0 +1,253 @@
+# Copyright 2008 Canonical Ltd.  All rights reserved.
+"""Tests for ETag generation."""
+
+__metaclass__ = type
+
+import unittest
+
+from zope.component import provideUtility
+
+from lazr.restful.interfaces import IWebServiceConfiguration
+from lazr.restful.testing.helpers import TestWebServiceConfiguration
+from lazr.restful.testing.webservice import create_web_service_request
+from lazr.restful._resource import (
+    EntryFieldResource,
+    EntryResource,
+    HTTPResource,
+    ServiceRootResource,
+    make_entry_etag_cores,
+    )
+
+
+class TestEntryResourceETags(unittest.TestCase):
+    # The EntryResource uses the field values that can be written or might
+    # othwerise change as the basis for its ETags.  The make_entry_etag_cores
+    # function is passed the data about the fields and returns the read and
+    # write cores.
+
+    def test_no_field_details(self):
+        # If make_entry_etag_cores is given no field details (because no
+        # fields exist), the resulting cores empty strings.
+        self.assertEquals(make_entry_etag_cores([]), ['', ''])
+
+    def test_writable_fields(self):
+        # If there are writable fields, their values are incorporated into the
+        # writable portion of the cores.
+        field_details = [
+            ('first_field',
+             {'writable': True,
+              'value': 'first'}),
+            ('second_field',
+             {'writable': True,
+              'value': 'second'}),
+            ]
+        self.assertEquals(
+            make_entry_etag_cores(field_details), ['', 'first\0second'])
+
+    def test_unchanging_fields(self):
+        # If there are fields that are not writable their values are still
+        # reflected in the generated cores because we want and addition or
+        # removal of read-only fields to trigger a new ETag.
+        field_details = [
+            ('first_field',
+             {'writable': False,
+              'value': 'the value'}),
+            ]
+        self.assertEquals(
+            make_entry_etag_cores(field_details),
+            ['the value', ''])
+
+    def test_combinations_of_fields(self):
+        # If there are a combination of writable, changable, and unchanable
+        # fields, their values are reflected in the resulting cores.
+        field_details = [
+            ('first_writable',
+             {'writable': True,
+              'value': 'first-writable'}),
+            ('second_writable',
+             {'writable': True,
+              'value': 'second-writable'}),
+            ('first_non_writable',
+             {'writable': False,
+              'value': 'first-not-writable'}),
+            ('second_non_writable',
+             {'writable': False,
+              'value': 'second-not-writable'}),
+            ]
+        self.assertEquals(
+            make_entry_etag_cores(field_details),
+            ['first-not-writable\x00second-not-writable',
+             'first-writable\x00second-writable'])
+
+
+class TestHTTPResourceETags(unittest.TestCase):
+
+    def test_getETag_is_a_noop(self):
+        # The HTTPResource class implements a do-nothing _getETagCores in order to
+        # be conservative (because it's not aware of the nature of all possible
+        # subclasses).
+        self.assertEquals(HTTPResource(None, None)._getETagCores(), None)
+
+
+class TestHTTPResourceETags(unittest.TestCase):
+
+    def test_getETag_is_a_noop(self):
+        # The HTTPResource class implements a do-nothing _getETagCores in order to
+        # be conservative (because it's not aware of the nature of all possible
+        # subclasses).
+        self.assertEquals(HTTPResource(None, None)._getETagCores(), None)
+
+
+class FauxEntryField:
+    entry = None
+    name = 'field_name'
+    field = None
+
+
+class EntryFieldResourceTests(unittest.TestCase):
+    # Tests for ETags of EntryFieldResource objects.
+
+    # Because the ETag generation only takes into account the field value and
+    # the web service revision number (and not whether the field is read-write
+    # or read-only) these tests don't mention the read-write/read-only nature
+    # of the field in question.
+
+    def setUp(self):
+        self.config = TestWebServiceConfiguration()
+        provideUtility(self.config, IWebServiceConfiguration)
+        self.resource = EntryFieldResource(FauxEntryField(), None)
+
+    def set_field_value(self, value):
+        """Set the value of the fake field the EntryFieldResource references.
+        """
+        self.resource._unmarshalled_field_cache['field_name'] = (
+            'field_name', value)
+        # We have to clear the etag cache for a new value to be generated.
+        # XXX benji 2010-09-30 [bug=652459] Does this mean there is an error
+        # condition that occurs when something other than applyChanges (which
+        # invalidates the cache) modifies a field's value?
+        self.resource.etags_by_media_type = {}
+
+    def test_cores_change_with_revno(self):
+        # The ETag cores should change if the revision (not the version) of
+        # the web service change.
+        self.set_field_value('this is the field value')
+
+        # Find the cores generated with a given revision...
+        self.config.code_revision = u'42'
+        first_cores = self.resource._getETagCores(self.resource.JSON_TYPE)
+
+        # ...find the cores generated with a different revision.
+        self.config.code_revision = u'99'
+        second_cores = self.resource._getETagCores(self.resource.JSON_TYPE)
+
+        # The cores should be different.
+        self.assertNotEqual(first_cores, second_cores)
+        # In particular, the read core should be the same between the two, but
+        # the write core should be different.
+        self.assertEqual(first_cores[1], second_cores[1])
+        self.assertNotEqual(first_cores[0], second_cores[0])
+
+    def test_cores_change_with_value(self):
+        # The ETag cores should change if the value of the field change.
+        # Find the cores generated with a given value...
+        self.set_field_value('first value')
+        first_cores = self.resource._getETagCores(self.resource.JSON_TYPE)
+
+        # ...find the cores generated with a different value.
+        self.set_field_value('second value')
+        second_cores = self.resource._getETagCores(self.resource.JSON_TYPE)
+
+        # The cores should be different.
+        self.assertNotEqual(first_cores, second_cores)
+        # In particular, the read core should be different between the two,
+        # but the write core should be the same.
+        self.assertNotEqual(first_cores[1], second_cores[1])
+        self.assertEqual(first_cores[0], second_cores[0])
+
+
+class ServiceRootResourceTests(unittest.TestCase):
+    # Tests for ETags of EntryFieldResource objects.
+
+    def setUp(self):
+        self.config = TestWebServiceConfiguration()
+        provideUtility(self.config, IWebServiceConfiguration)
+        self.resource = ServiceRootResource()
+
+    def test_cores_change_with_revno(self):
+        # The ETag core should change if the revision (not the version) of the
+        # web service change.
+
+        # Find the cores generated with a given revision...
+        self.config.code_revision = u'42'
+        first_cores = self.resource._getETagCores(self.resource.JSON_TYPE)
+
+        # ...find the cores generated with a different revision.
+        self.config.code_revision = u'99'
+        second_cores = self.resource._getETagCores(self.resource.JSON_TYPE)
+
+        # The cores should be different.
+        self.assertNotEqual(first_cores, second_cores)
+
+
+class TestableHTTPResource(HTTPResource):
+    """A HTTPResource that lest us set the ETags from the outside."""
+
+    def _parseETags(self, *args):
+        return self.incoming_etags
+
+    def getETag(self, *args):
+        return self.existing_etag
+
+
+class TestConditionalGet(unittest.TestCase):
+
+    def setUp(self):
+        self.config = TestWebServiceConfiguration()
+        provideUtility(self.config, IWebServiceConfiguration)
+        self.request = create_web_service_request('/1.0')
+        self.resource = TestableHTTPResource(None, self.request)
+
+    def test_etags_are_the_same(self):
+        # If one of the ETags present in an incoming request is the same as
+        # the ETag that represents the current object's state, then
+        # a conditional GET should return "Not Modified" (304).
+        self.resource.incoming_etags = ['1', '2', '3']
+        self.resource.existing_etag = '2'
+        self.assertEquals(self.resource.handleConditionalGET(), None)
+        self.assertEquals(self.request.response.getStatus(), 304)
+
+    def test_etags_differ(self):
+        # If none of the ETags present in an incoming request is the same as
+        # the ETag that represents the current object's state, then a
+        # conditional GET should result in a new representation of the object
+        # being returned.
+        self.resource.incoming_etags = ['1', '2', '3']
+        self.resource.existing_etag = '99'
+        self.assertNotEquals(self.resource.handleConditionalGET(), None)
+
+
+class TestConditionalWrite(unittest.TestCase):
+
+    def setUp(self):
+        self.config = TestWebServiceConfiguration()
+        provideUtility(self.config, IWebServiceConfiguration)
+        self.request = create_web_service_request('/1.0')
+        self.resource = TestableHTTPResource(None, self.request)
+
+    def test_etags_are_the_same(self):
+        # If one of the ETags present in an incoming request is the same as
+        # the ETag that represents the current object's state, then
+        # the write should be applied.
+        self.resource.incoming_etags = ['1', '2', '3']
+        self.resource.existing_etag = '2'
+        self.assertNotEquals(self.resource.handleConditionalWrite(), None)
+
+    def test_etags_differ(self):
+        # If one of the ETags present in an incoming request is the same as
+        # the ETag that represents the current object's state, then
+        # the write should fail.
+        self.resource.incoming_etags = ['1', '2', '3']
+        self.resource.existing_etag = '99'
+        self.assertEquals(self.resource.handleConditionalWrite(), None)
+        self.assertEquals(self.request.response.getStatus(), 412)

=== modified file 'src/lazr/restful/tests/test_utils.py'
--- src/lazr/restful/tests/test_utils.py	2010-08-24 22:12:38 +0000
+++ src/lazr/restful/tests/test_utils.py	2010-10-04 15:46:40 +0000
@@ -11,12 +11,25 @@
 from zope.security.management import (
     endInteraction, newInteraction, queryInteraction)
 
-from lazr.restful.utils import (get_current_browser_request,
-    is_total_size_link_active, sorted_named_things)
+from lazr.restful.utils import (
+    extract_write_portion,
+    get_current_browser_request,
+    is_total_size_link_active,
+    sorted_named_things,
+    )
 
 
 class TestUtils(unittest.TestCase):
 
+    def test_two_part_extract_write_portion(self):
+        # ETags are sometimes two-part.  A hyphen seperates the parts if so.
+        self.assertEqual('write', extract_write_portion('read-write'))
+
+    def test_one_part_extract_write_portion(self):
+        # ETags are sometimes one-part.  If so, writes are predicated on the
+        # whole ETag.
+        self.assertEqual('etag', extract_write_portion('etag'))
+
     def test_get_current_browser_request_no_interaction(self):
         # When there's no interaction setup, get_current_browser_request()
         # returns None.
@@ -77,4 +90,3 @@
 
     # For the sake of convenience, test_get_current_web_service_request()
     # and tag_request_with_version_name() are tested in test_webservice.py.
-

=== modified file 'src/lazr/restful/utils.py'
--- src/lazr/restful/utils.py	2010-08-24 22:12:38 +0000
+++ src/lazr/restful/utils.py	2010-10-04 15:46:40 +0000
@@ -38,6 +38,11 @@
 missing = object()
 
 
+def extract_write_portion(etag):
+    """Retrieve the portion of an etag that predicates writes."""
+    return etag.split('-', 1)[-1]
+
+
 def is_total_size_link_active(version, config):
     versions = config.active_versions
     total_size_link_version = config.first_version_with_total_size_link

=== modified file 'versions.cfg'
--- versions.cfg	2010-08-24 20:04:30 +0000
+++ versions.cfg	2010-10-04 15:46:40 +0000
@@ -24,6 +24,7 @@
 lxml = 2.2.7
 martian = 0.11
 pytz = 2010h
+roman = 1.4.0
 setuptools = 0.6c11
 simplejson = 2.0.9
 transaction = 1.0.0