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

[Leonard Richardson]

This branch fixes bug 561521, in which a Launchpad test was found to fail on some Lucid installs because modifications dictated by PATCH requests happened in a nondeterministic order. This branch enforces a 1) deterministic order that's 2) less likely to cause problems.

The new order is less likely to cause problems because all of an entry's fields that are handled by mutator methods are saved until last. For testability purposes, I defined a helper function get_entry_fields_in_write_order(). (It's not in __all__, but it's only used in _resource.py and in the test, so I don't think it needs to be.) To make the order deterministic, I changed validated_changeset from a dict to a list of tuples.

I also did some drive-by cleanup of test headings.

I have one slight misgiving about the get_entry_fields_in_write_order(). To determine whether an entry's field is handled by a mutator method it checks for a subclass of lazr.delegates "Passthrough" class. Checking isinstance(Passthrough) doesn't work because every field of a generated adapter is a Passthrough.

You could argue that I'm really looking for an instance of PropertyWithMutator, the Passthrough subclass defined in declarations.py. This is less general, such that if someone made their own PWM-like Passthrough subclass, get_entry_fields_in_write_order() wouldn't pick it up. But people should really be using the autogenerated entry adapters, which always use PWM. I could go either way on this. I could also go either way on whether the helper method should be in _resource.py or declarations.py. (If it's in declarations.py, I'll have to do some fancy importing in _resource.py to avoid an import loop.)

[Leonard Richardson (leonardr)]

Oh, I should mention that in addition to testing the unit tests, I tested this branch in conjunction with Launchpad to make sure it made the test failure go away. (The test that failed was webservice/xx-distribution.txt)

[Edwin Grubbs (edwin-grubbs)]

Hi Leonard,

This branch looks good. I just have one comment below.

merge-conditional

-Edwin

>=== modified file 'src/lazr/restful/_resource.py'
>--- src/lazr/restful/_resource.py 2010-04-13 15:35:24 +0000
>+++ src/lazr/restful/_resource.py 2010-04-14 17:30:37 +0000
>@@ -1988,3 +1988,36 @@
> """The URL to the description of the object's full representation."""
> return "%s#%s-full" % (
> self._service_root_url(), self.singular_type)
>+
>+
>+def get_entry_fields_in_write_order(entry):
>+ """Return the entry's fields in the order they should be written to.
>+
>+ The ordering is intended to 1) be deterministic for a given schema
>+ and 2) minimize the chance of conflicts. Fields that are just
>+ fields come before fields (believed to be) controlled by
>+ mutators. Within each group, fields are returned in the order they
>+ appear in the schema.
>+
>+ :param entry: An object that provides IEntry.
>+ :return: A list of 2-tuples (field name, field object)
>+ """
>+ non_mutator_fields = []
>+ mutator_fields = []
>+ field_implementations = entry.__class__.__dict__
>+ for name, field in getFieldsInOrder(entry.schema):
>+ if name.startswith('_'):
>+ # This field is not part of the web service interface.
>+ continue
>+
>+ add_to = non_mutator_fields
>+ # If this field is secretly a subclass of lazr.delegates
>+ # Passthrough (but not a direct instance of Passthrough), put
>+ # it at the end -- it's probably controlled by a mutator.
>+ implementation = field_implementations[name]
>+ if (issubclass(implementation.__class__, Passthrough)
>+ and not implementation.__class__ is Passthrough):
>+ add_to = mutator_fields
>+ add_to.append((name, field))
>+ return non_mutator_fields + mutator_fields

Setting the add_to variable makes it a little harder to follow the
logic. I would prefer getting rid of it and using an if/else with
mutator_fields.append() and non_mutator_fields.append() instead.

review: Approve

=== modified file 'src/lazr/restful/NEWS.txt'
--- src/lazr/restful/NEWS.txt	2010-04-07 15:01:33 +0000
+++ src/lazr/restful/NEWS.txt	2010-04-14 15:29:18 +0000
@@ -2,8 +2,8 @@
 NEWS for lazr.restful
 =====================
 
-Development
-===========
+0.9.25 (2010-04-14)
+===================
 
 Special note: This version introduces a new configuration element,
 'caching_policy'. This element starts out simple but may become more
@@ -12,7 +12,12 @@
 
 Service root resources are now client-side cacheable for an amount of
 time that depends on the server configuration and the version of the
-web service requested.
+web service requested. To get the full benefit, clients will need to
+upgrade to lazr.restfulclient 0.9.14.
+
+When a PATCH or PUT request changes multiple fields at once, the
+changes are applied in a deterministic order designed to minimize
+possible conflicts.
 
 0.9.24 (2010-03-17)
 ====================
 
=== modified file 'src/lazr/restful/_resource.py'
--- src/lazr/restful/_resource.py	2010-04-13 15:35:24 +0000
+++ src/lazr/restful/_resource.py	2010-04-14 15:29:18 +0000
@@ -76,6 +76,7 @@
 from zope.traversing.browser.interfaces import IAbsoluteURL
 
 from lazr.batchnavigator import BatchNavigator
+from lazr.delegates import Passthrough
 from lazr.enum import BaseItem
 from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.lifecycle.snapshot import Snapshot
@@ -956,7 +957,7 @@
         propagated to the client.
         """
         changeset = copy.copy(changeset)
-        validated_changeset = {}
+        validated_changeset = []
         errors = []
 
         # The self link and resource type link aren't part of the
@@ -983,12 +984,7 @@
 
         # For every field in the schema, see if there's a corresponding
         # field in the changeset.
-        # Get the fields ordered by name so that we always evaluate them in
-        # the same order. This is needed to predict errors when testing.
-        for name, field in getFieldsInOrder(self.entry.schema):
-            if name.startswith('_'):
-                # This field is not part of the web service interface.
-                continue
+        for name, field in get_entry_fields_in_write_order(self.entry):
             field = field.bind(self.entry.context)
             marshaller = getMultiAdapter((field, self.request),
                                          IFieldMarshaller)
@@ -1114,7 +1110,7 @@
                             error = "Validation error"
                         errors.append("%s: %s" % (repr_name, error))
                         continue
-                validated_changeset[field] = (name, value)
+                validated_changeset.append((field, value))
         # If there are any fields left in the changeset, they're
         # fields that don't correspond to some field in the
         # schema. They're all errors.
@@ -1134,8 +1130,10 @@
 
         # Store the entry's current URL so we can see if it changes.
         original_url = absoluteURL(self.entry.context, self.request)
-        # Make the changes.
-        for field, (name, value) in validated_changeset.items():
+
+        # 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.
@@ -1145,7 +1143,7 @@
         event = ObjectModifiedEvent(
             object=self.entry.context,
             object_before_modification=entry_before_modification,
-            edited_fields=validated_changeset.keys())
+            edited_fields=[field for field, value in validated_changeset])
         notify(event)
 
         # The changeset contained new values for some of this object's
@@ -1430,8 +1428,10 @@
 
         # Make sure the representation includes values for all
         # writable attributes.
-        # Get the fields ordered by name so that we always evaluate them in
-        # the same order. This is needed to predict errors when testing.
+        #
+        # Get the fields ordered by schema order so that we always
+        # evaluate them in the same order. This is needed to predict
+        # errors when testing.
         for name, field in getFieldsInOrder(self.entry.schema):
             if not self.isModifiableField(field, True):
                 continue
@@ -1988,3 +1988,36 @@
         """The URL to the description of the object's full representation."""
         return "%s#%s-full" % (
             self._service_root_url(), self.singular_type)
+
+
+def get_entry_fields_in_write_order(entry):
+    """Return the entry's fields in the order they should be written to.
+
+    The ordering is intended to 1) be deterministic for a given schema
+    and 2) minimize the chance of conflicts. Fields that are just
+    fields come before fields (believed to be) controlled by
+    mutators. Within each group, fields are returned in the order they
+    appear in the schema.
+
+    :param entry: An object that provides IEntry.
+    :return: A list of 2-tuples (field name, field object)
+    """
+    non_mutator_fields = []
+    mutator_fields = []
+    field_implementations = entry.__class__.__dict__
+    for name, field in getFieldsInOrder(entry.schema):
+        if name.startswith('_'):
+            # This field is not part of the web service interface.
+            continue
+
+        add_to = non_mutator_fields
+        # If this field is secretly a subclass of lazr.delegates
+        # Passthrough (but not a direct instance of Passthrough), put
+        # it at the end -- it's probably controlled by a mutator.
+        implementation = field_implementations[name]
+        if (issubclass(implementation.__class__, Passthrough)
+            and not implementation.__class__ is Passthrough):
+            add_to = mutator_fields
+        add_to.append((name, field))
+    return non_mutator_fields + mutator_fields
+
 
=== modified file 'src/lazr/restful/docs/webservice-declarations.txt'
--- src/lazr/restful/docs/webservice-declarations.txt	2010-03-03 13:38:05 +0000
+++ src/lazr/restful/docs/webservice-declarations.txt	2010-04-14 15:29:18 +0000
@@ -5,7 +5,6 @@
 with some decorators. From this tagging the web service API will be
 created automatically.
 
-========================
 Exporting the data model
 ========================
 
@@ -770,13 +769,11 @@
     return_type: None
     type: 'write_operation'
 
-=========================
 Generating the webservice
 =========================
 
-
 Entry
-=====
+-----
 
 The webservice can be generated from tagged interfaces.  For every
 version in the web service, generate_entry_interfaces() will create a
@@ -969,9 +966,8 @@
       ...
     TypeError: 'IBookSet' isn't exported as an entry.
 
-
 Collection
-==========
+----------
 
 An ICollection adapter for content interface tagged as being exported as
 collections on the webservice can be generated by using the
@@ -1046,7 +1042,7 @@
     TypeError: 'IBook' isn't exported as a collection.
 
 Methods
-=======
+-------
 
 IResourceOperation adapters can be generated for exported methods by
 using the generate_operation_adapter() function. Using it on a method
@@ -1259,10 +1255,8 @@
     >>> print destructor_method_adapter_factory.__name__
     DELETE_IBookOnSteroids_destroy_beta
 
-
-
 Destructor
-==========
+----------
 
 A method can be designated as a destructor for the entry. Here, the
 destroy() method is designated as the destructor for IHasText.
@@ -1322,9 +1316,8 @@
     TypeError: An entry can only have one destructor method for
     version (earliest version); destroy and destroy2 make two.
 
-
 Mutators
-========
+--------
 
 A method can be designated as a mutator for some field. Here, the
 set_text() method is designated as the mutator for the 'text' field.
@@ -1471,7 +1464,7 @@
     (earliest version); set_value_2 makes two.
 
 Caching
-=======
+-------
 
 It is possible to cache a server response in the browser cache using
 the @cache_for decorator:
 
=== modified file 'src/lazr/restful/docs/webservice.txt'
--- src/lazr/restful/docs/webservice.txt	2010-02-11 17:57:16 +0000
+++ src/lazr/restful/docs/webservice.txt	2010-04-14 15:29:18 +0000
@@ -1,3 +1,4 @@
+********************
 RESTful Web Services
 ********************
 
@@ -6,7 +7,6 @@
 a model for managing recipes, and then publishing the model objects as
 resources through a web service.
 
-=====================
 Example model objects
 =====================
 
@@ -473,7 +473,6 @@
     >>> setSecurityPolicy(SimpleSecurityPolicy)
     <class ...>
 
-=========================================
 Web Service Infrastructure Initialization
 =========================================
 
@@ -532,7 +531,6 @@
     ...     register_versioned_request_utility(cls, version)
 
 
-======================
 Defining the resources
 ======================
 
@@ -624,7 +622,6 @@
     ...     [IChoice, IWebServiceClientRequest, AuthorVocabulary],
     ...     IFieldMarshaller)
 
-==========================
 Implementing the resources
 ==========================
 
@@ -806,7 +803,67 @@
     >>> scoped_collection.entry_schema
     <InterfaceClass __builtin__.IRecipeEntry>
 
-=================
+Field ordering
+--------------
+
+When an entry's fields are modified, it's important that the
+modifications happen in a deterministic order, to minimize (or at
+least make deterministic) bad interactions between fields. The helper
+function get_entry_fields_in_write_order() handles this.
+
+Ordinarily, fields are written to in the same order they are found in
+the underlying schema.
+
+    >>> author_entry = getMultiAdapter((A1, request), IEntry)
+    >>> from lazr.restful._resource import get_entry_fields_in_write_order
+    >>> def print_fields_in_write_order(entry):
+    ...     for name, field in get_entry_fields_in_write_order(entry):
+    ...         print name
+
+    >>> print_fields_in_write_order(author_entry)
+    name
+    favorite_recipe
+    popularity
+
+The one exception is if a field is wrapped in a subclass of the
+Passthrough class defined by the lazr.delegates library. Classes
+generated through lazr.restful's annotations use a Passthrough
+subclass to control a field that triggers complex logic when its value
+changes. To minimize the risk of bad interactions, all the simple
+fields are changed before any of the complex fields.
+
+Here's a simple subclass of Passthrough.
+
+    >>> from lazr.delegates import Passthrough
+    >>> class MyPassthrough(Passthrough):
+    ...     pass
+
+When we replace 'favorite_recipe' with an instance of this subclass,
+that field shows up at the end of the list of fields.
+
+    >>> old_favorite_recipe = AuthorEntry.favorite_recipe
+    >>> AuthorEntry.favorite_recipe = MyPassthrough('favorite_recipe', A1)
+    >>> print_fields_in_write_order(author_entry)
+    name
+    popularity
+    favorite_recipe
+
+When we replace 'name' with a Passthrough subclass, it also shows up
+at the end--but it still shows up before 'favorite_recipe', because it
+comes before 'favorite_recipe' in the schema.
+
+    >>> old_name = AuthorEntry.name
+    >>> AuthorEntry.name = MyPassthrough('name', A1)
+    >>> print_fields_in_write_order(author_entry)
+    popularity
+    name
+    favorite_recipe
+
+Cleanup to restore the old AuthorEntry implementation:
+
+    >>> AuthorEntry.favorite_recipe = old_favorite_recipe
+    >>> AuthorEntry.name = old_name
+
 Custom operations
 =================
 
@@ -923,7 +980,6 @@
     ...     RecipeDeleteOperation, name="")
 
 
-================
 Resource objects
 ================
 
@@ -944,7 +1000,6 @@
 Similarly, you can implement ``RecipeEntry`` to the ``IEntry`` interface, and
 expose it through the web as an ``EntryResource``.
 
-=========================
 The Service Root Resource
 =========================
 
@@ -1046,7 +1101,6 @@
     AssertionError: There must be one (and only one) adapter
     from DishCollection to ICollection.
 
-====================
 Collection resources
 ====================
 
@@ -1234,7 +1288,6 @@
     u'Nouvelle Brazilian'
 
 
-===============
 Entry resources
 ===============
 
@@ -1381,7 +1434,6 @@
      u'Draw, singe, stuff, and truss...',
      u'You can always judge...']
 
-=============================
 Named operation return values
 =============================
 
@@ -1514,7 +1566,6 @@
     ...
     TypeError: Could not serialize object [<object object...>] to JSON.
 
-=====
 ETags
 =====
 
@@ -1580,7 +1631,6 @@
     >>> etag_after_readonly_change == etag_original
     False
 
-===================
 Resource Visibility
 ===================
 
@@ -1684,7 +1734,6 @@
     ...
     Unauthorized: (<Recipe object...>, 'dish', ...)
 
-=====================
 Stored file resources
 =====================
 
@@ -1753,7 +1802,6 @@
     >>> print C2.cover
     None
 
-===============
 Field resources
 ===============
 
@@ -1764,7 +1812,6 @@
     >>> print field_resource()
     "The Joy of Cooking"
 
-==================================
 Requesting non available resources
 ==================================
 
@@ -1793,7 +1840,6 @@
     ...
     NotFound: ... name: u'comments/10'
 
-====================
 Manipulating entries
 ====================
 
@@ -1965,7 +2011,6 @@
     NotFound: ... name: u'recipes/Foies de voilaille en aspic'
 
 
-=================
 Within a template
 =================
 
 
=== modified file 'src/lazr/restful/version.txt'
--- src/lazr/restful/version.txt	2010-03-15 19:44:45 +0000
+++ src/lazr/restful/version.txt	2010-04-14 15:29:18 +0000
@@ -1,1 +1,1 @@
-0.9.24
+0.9.25