Merge lp:~gary/lazr.restful/bug871944 into lp:lazr.restful

[Gary Poster]

Be more careful in obtaining old and new values when determining cache invalidation.

The related bug describes the problem that this branch solves. The solution I chose is fairly straightforward and involves fairly isolated changes to the codebase: keep the field and original value around in the cache too, so we can get the new value with the expected name, and compare it against the real original value. I added a test which failed before my change, and passed with it.

Launchpad appears to be happy with these changes.

[Graham Binns (gmb)]

review: Approve (code)

=== modified file 'src/lazr/restful/NEWS.txt'
--- src/lazr/restful/NEWS.txt	2011-09-20 18:26:06 +0000
+++ src/lazr/restful/NEWS.txt	2011-10-11 14:10:35 +0000
@@ -2,6 +2,12 @@
 NEWS for lazr.restful
 =====================
 
+0.19.4 (2011-10-11)
+===================
+
+Fixed bug 871944: a successful write with an If-Match would sometimes
+return stale values.
+
 0.19.3 (2011-09-20)
 ===================
 

=== modified file 'src/lazr/restful/_resource.py'
--- src/lazr/restful/_resource.py	2011-09-20 14:36:24 +0000
+++ src/lazr/restful/_resource.py	2011-10-11 14:10:35 +0000
@@ -797,15 +797,17 @@
             cached_value = self._unmarshalled_field_cache.get(
                 field_name, MISSING)
         if cached_value is not MISSING:
-            return cached_value
+            return cached_value[0]
 
         field = field.bind(self.context)
         marshaller = getMultiAdapter((field, self.request), IFieldMarshaller)
         try:
             if IUnmarshallingDoesntNeedValue.providedBy(marshaller):
                 value = None
+                flag = IUnmarshallingDoesntNeedValue
             else:
                 value = getattr(self.entry, field.__name__)
+                flag = None
             if detail is NORMAL_DETAIL:
                 repr_value = marshaller.unmarshall(self.entry, value)
             elif detail is CLOSEUP_DETAIL:
@@ -820,11 +822,14 @@
             # access to the resource altogether, use our special
             # 'redacted' tag: URI for the field's value.
             repr_value = self.REDACTED_VALUE
+            value = None
+            flag = Unauthorized
 
         unmarshalled = (marshaller.representation_name, repr_value)
 
         if detail is NORMAL_DETAIL:
-            self._unmarshalled_field_cache[field_name] = unmarshalled
+            self._unmarshalled_field_cache[field_name] = (
+                unmarshalled, (field, value, flag))
         return unmarshalled
 
     def _field_with_html_renderer(self, field_name, field):
@@ -1215,14 +1220,16 @@
             # 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():
+            for name, cached in self._unmarshalled_field_cache.items():
+                cached, (field, old_value, flag) = cached
+                if flag is IUnmarshallingDoesntNeedValue:
+                    continue
                 force_clear = False
                 try:
-                    old_value = getattr(entry_before_modification, name, None)
-                    new_value = getattr(self.entry.context, name, None)
+                    new_value = getattr(self.entry, field.__name__)
                 except Unauthorized:
-                    # Clear the cache, just to be safe.
-                    force_clear = True
+                    if flag is not Unauthorized:
+                        force_clear = True
                 if force_clear or new_value != old_value:
                     del(self._unmarshalled_field_cache[name])
 

=== modified file 'src/lazr/restful/tests/test_etag.py'
--- src/lazr/restful/tests/test_etag.py	2010-10-04 15:42:42 +0000
+++ src/lazr/restful/tests/test_etag.py	2011-10-11 14:10:35 +0000
@@ -121,7 +121,7 @@
         """Set the value of the fake field the EntryFieldResource references.
         """
         self.resource._unmarshalled_field_cache['field_name'] = (
-            'field_name', value)
+            ('field_name', value), (FauxEntryField(), value, None))
         # 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

=== modified file 'src/lazr/restful/tests/test_webservice.py'
--- src/lazr/restful/tests/test_webservice.py	2011-09-08 20:18:30 +0000
+++ src/lazr/restful/tests/test_webservice.py	2011-10-11 14:10:35 +0000
@@ -374,9 +374,24 @@
         self.a_field = value
 
 
+class IHasFieldExportedAsDifferentName(Interface):
+    """An entry with a field exported as a different name."""
+    export_as_webservice_entry()
+    a_field = exported(TextLine(title=u"A field."), exported_as='field')
+
+
+class HasFieldExportedAsDifferentName:
+    """An implementation of IHasFieldExportedAsDifferentName."""
+    implements(IHasFieldExportedAsDifferentName)
+    def __init__(self, value):
+        self.a_field = value
+
+
 class TestEntryWrite(EntryTestCase):
 
-    testmodule_objects = [IHasOneField, HasOneField]
+    testmodule_objects = [
+        IHasOneField, HasOneField,
+        IHasFieldExportedAsDifferentName, HasFieldExportedAsDifferentName]
 
     def test_applyChanges_rejects_nonexistent_web_link(self):
         # If web_link is not published, applyChanges rejects a request
@@ -418,6 +433,30 @@
             representation = simplejson.loads(resource.applyChanges({}))
             self.assertEquals(representation, existing_representation)
 
+    def test_applyChanges_returns_modified_representation_on_change(self):
+        # If a change is made, the representation that is returned
+        # represents the new state.  Bug fix: this should happen even
+        # if the resource and its context have different names for the
+        # same value.
+        self._register_website_url_space(IHasFieldExportedAsDifferentName)
+
+        with self.entry_resource(
+            IHasFieldExportedAsDifferentName,
+            HasFieldExportedAsDifferentName,
+            u"initial value") as resource:
+            # We have an entry that has a different name on the
+            # entry than on its context.
+            entry = resource.entry
+            self.assertEquals(entry.field, entry.context.a_field)
+            # This populates the cache.
+            self.assertEquals(
+                u'initial value', resource.toDataForJSON()['field'])
+            # This returns the changed value.
+            representation = simplejson.loads(
+                resource.applyChanges(dict(field=u'new value')))
+            self.assertEquals(
+                u'new value', representation['field'])
+
 
 class TestEntryWriteForRestrictedField(EntryTestCase):
 

=== modified file 'src/lazr/restful/version.txt'
--- src/lazr/restful/version.txt	2011-09-20 18:26:06 +0000
+++ src/lazr/restful/version.txt	2011-10-11 14:10:35 +0000
@@ -1,1 +1,1 @@
-0.19.3
+0.19.4