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

[Leonard Richardson]

This branch stops lazr.restful from sending out an ObjectModifiedEvent if the object was not actually modified. A representation of the object is still returned. This should fix the oops seen in bug 449561, though not necessarily the bug itself.

[Abel Deuring (adeuring)]

review: Approve (code)

=== modified file 'src/lazr/restful/NEWS.txt'
--- src/lazr/restful/NEWS.txt	2011-03-24 20:02:41 +0000
+++ src/lazr/restful/NEWS.txt	2011-03-25 11:56:29 +0000
@@ -2,11 +2,14 @@
 NEWS for lazr.restful
 =====================
 
-0.18.1 (2011-03-24)
+0.18.1 (2011-03-25)
 ===================
 
 Fixed minor test failures.
 
+The object modification event will not be fired if a client sends an
+empty changeset via PATCH.
+
 0.18.0 (2011-03-23)
 ===================
 

=== modified file 'src/lazr/restful/_resource.py'
--- src/lazr/restful/_resource.py	2011-03-15 17:13:48 +0000
+++ src/lazr/restful/_resource.py	2011-03-25 11:56:29 +0000
@@ -1174,41 +1174,45 @@
             self.request.response.setHeader('Content-type', 'text/plain')
             return "\n".join(errors)
 
-        # Make a snapshot of the entry to use in a notification event.
-        entry_before_modification = Snapshot(
-            self.entry.context, providing=providedBy(self.entry.context))
-
         # Store the entry's current URL so we can see if it changes.
         original_url = absoluteURL(self.entry.context, self.request)
 
-        # Make the changes, in the same order obtained from
-        # get_entry_fields_in_write_order.
-        for field, value in validated_changeset:
-            field.set(self.entry, value)
-        # The representation has changed, and etags will need to be
-        # recalculated.
-        self.etags_by_media_type = {}
-
-        # Send a notification event.
-        event = ObjectModifiedEvent(
-            object=self.entry.context,
-            object_before_modification=entry_before_modification,
-            edited_fields=[field for field, value in validated_changeset])
-        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 len(validated_changeset) > 0:
+            # The representation will actually change.
+
+            # Make a snapshot of the entry to use in a notification event.
+            entry_before_modification = Snapshot(
+                self.entry.context, providing=providedBy(self.entry.context))
+
+            # Make the changes, in the same order obtained from
+            # get_entry_fields_in_write_order.
+            for field, value in validated_changeset:
+                field.set(self.entry, value)
+
+            # ETags will need to be recalculated.
+            self.etags_by_media_type = {}
+
+            # Send a notification event.
+            event = ObjectModifiedEvent(
+                object=self.entry.context,
+                object_before_modification=entry_before_modification,
+                edited_fields=[
+                    field for field, value in validated_changeset])
+            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])
 
         self._applyChangesPostHook()
 

=== modified file 'src/lazr/restful/tests/test_webservice.py'
--- src/lazr/restful/tests/test_webservice.py	2011-03-07 15:16:57 +0000
+++ src/lazr/restful/tests/test_webservice.py	2011-03-25 11:56:29 +0000
@@ -14,7 +14,11 @@
 import simplejson
 import unittest
 
-from zope.component import getGlobalSiteManager, getUtility
+from zope.component import (
+    eventtesting,
+    getGlobalSiteManager,
+    getUtility,
+    )
 from zope.interface import implements, Interface
 from zope.interface.interface import InterfaceClass
 from zope.publisher.browser import TestRequest
@@ -30,6 +34,7 @@
 from wadllib.application import Application
 
 from lazr.enum import EnumeratedType, Item
+from lazr.lifecycle.interfaces import IObjectModifiedEvent
 from lazr.restful import (
     EntryField,
     ResourceOperation,
@@ -403,6 +408,16 @@
                 resource.applyChanges({'web_link': existing_web_link}))
             self.assertEquals(representation['web_link'], existing_web_link)
 
+    def test_applyChanges_returns_representation_on_empty_changeset(self):
+        # Even if the changeset is empty, applyChanges returns a
+        # representation of the (unchanged) resource.
+        self._register_website_url_space(IHasOneField)
+
+        with self.entry_resource(IHasOneField, HasOneField, "") as resource:
+            existing_representation = resource.toDataForJSON()
+            representation = simplejson.loads(resource.applyChanges({}))
+            self.assertEquals(representation, existing_representation)
+
 
 class TestEntryWriteForRestrictedField(EntryTestCase):
 
@@ -908,3 +923,31 @@
         return ResourceOperation(None, request)
 
 
+class EventTestCase(EntryTestCase):
+
+    testmodule_objects = [IHasOneField]
+
+    def setUp(self):
+        super(EventTestCase, self).setUp()
+        self._register_url_adapter(IHasOneField)
+        eventtesting.setUp()
+
+    def test_event_fired_when_changeset_is_not_empty(self):
+        # Passing in a non-empty changeset spawns an
+        # IObjectModifiedEvent.
+        with self.entry_resource(
+                IHasOneField, HasOneField, "") as resource:
+            resource.applyChanges({'a_field': u'Some value'})
+        events = eventtesting.getEvents()
+        self.assertEquals(len(events), 1)
+        event = events[0]
+        self.assertEquals(event.object_before_modification.a_field, "")
+        self.assertEquals(event.object.a_field, "Some value")
+
+    def test_event_not_fired_when_changeset_is_empty(self):
+        # Passing in an empty changeset does not spawn an
+        # IObjectModifiedEvent.
+        with self.entry_resource(
+                IHasOneField, HasOneField, "") as resource:
+            resource.applyChanges({})
+        self.assertEquals(len(eventtesting.getEvents()), 0)