Merge lp:~leonardr/lazr.restful/stop-using-object into lp:lazr.restful

[Leonard Richardson]

This branch prohibits the use of IObject in exported interfaces. The validation code associated with IObject causes infinite recursion. We previously had special code to avoid validation if validation would cause infinite recursion, but this meant that validation code wasn't running. We have a perfectly acceptable alternative to IObject that doesn't cause the problem (IReference). This branch replaces all our Objects with References, and adds code to raise an exception if you try to export an IObject that's not an IReference.

[lp:~leonardr/lazr.restful/stop-using-object]

166. By Leonard Richardson on 2011-01-21

Removed unused method.

167. By Leonard Richardson on 2011-01-21

Fixed test failures.

[Tim Penhey (thumper)]

Looks good.

review: Approve

=== modified file 'src/lazr/restful/NEWS.txt'
--- src/lazr/restful/NEWS.txt	2011-01-20 20:25:31 +0000
+++ src/lazr/restful/NEWS.txt	2011-01-21 21:12:04 +0000
@@ -2,6 +2,15 @@
 NEWS for lazr.restful
 =====================
 
+0.15.3 (2010-01-21)
+===================
+
+lazr.restful will now complain if you try to export an IObject, as
+this causes infinite recursion during field validation. We had code
+that worked around the infinite recursion, but it wasn't reliable and
+we've now removed it to simplify. Use IReference whenever you would
+use IObject.
+
 0.15.2 (2010-01-20)
 ===================
 

=== modified file 'src/lazr/restful/_resource.py'
--- src/lazr/restful/_resource.py	2011-01-19 19:00:10 +0000
+++ src/lazr/restful/_resource.py	2011-01-21 21:12:04 +0000
@@ -65,7 +65,7 @@
 from zope.publisher.interfaces.http import IHTTPRequest
 from zope.schema import ValidationError, getFieldsInOrder
 from zope.schema.interfaces import (
-    ConstraintNotSatisfied, IBytes, IField, IObject, RequiredMissing)
+    ConstraintNotSatisfied, IBytes, IField, RequiredMissing)
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import getChecker, removeSecurityProxy
 from zope.security.management import checkPermission
@@ -82,12 +82,12 @@
 from lazr.restful.interfaces import (
     ICollection, ICollectionField, ICollectionResource, IEntry, IEntryField,
     IEntryFieldResource, IEntryResource, IFieldHTMLRenderer, IFieldMarshaller,
-    IHTTPResource, IJSONPublishable, IReferenceChoice, IRepresentationCache,
-    IResourceDELETEOperation, IResourceGETOperation, IResourcePOSTOperation,
-    IScopedCollection, IServiceRootResource, ITopLevelEntryLink,
-    IUnmarshallingDoesntNeedValue, IWebServiceClientRequest,
-    IWebServiceConfiguration, IWebServiceLayer, IWebServiceVersion,
-    LAZR_WEBSERVICE_NAME)
+    IHTTPResource, IJSONPublishable, IReference, IReferenceChoice,
+    IRepresentationCache, IResourceDELETEOperation, IResourceGETOperation,
+    IResourcePOSTOperation, IScopedCollection, IServiceRootResource,
+    ITopLevelEntryLink, IUnmarshallingDoesntNeedValue,
+    IWebServiceClientRequest, IWebServiceConfiguration, IWebServiceLayer,
+    IWebServiceVersion, LAZR_WEBSERVICE_NAME)
 from lazr.restful.utils import (
     extract_write_portion,
     get_current_web_service_request,
@@ -463,16 +463,6 @@
             etags.append(etag)
         return etags
 
-    def _fieldValueIsObject(self, field):
-        """Does the given field expect a data model object as its value?
-
-        Obviously an IObject field is expected to have a data model
-        object as its value. But an IChoice field might also have a
-        vocabulary drawn from the set of data model objects.
-        """
-        return (IObject.providedBy(field)
-                or IReferenceChoice.providedBy(field))
-
     def _parseContentDispositionHeader(self, value):
         """Parse a Content-Disposition header.
 
@@ -1068,12 +1058,12 @@
 
             # If the new value is an object, make sure it provides the correct
             # interface.
-            if value is not None and IObject.providedBy(field):
+            if value is not None and IReference.providedBy(field):
                 # XXX leonardr 2008-04-12 spec=api-wadl-description:
                 # This should be moved into the
                 # ObjectLookupFieldMarshaller, once we make it
                 # possible for Vocabulary fields to specify a schema
-                # class the way IObject fields can.
+                # class the way IReference fields can.
                 if value != None and not field.schema.providedBy(value):
                     errors.append(u"%s: Your value points to the "
                                    "wrong kind of object" % repr_name)
@@ -1096,39 +1086,30 @@
                     continue
 
             if change_this_field is True and value != current_value:
-                if not IObject.providedBy(field):
-                    # We don't validate IObject values because that
-                    # can lead to infinite recursion. We don't _need_
-                    # to validate IObject values because a client
-                    # isn't changing anything about the IObject; it's
-                    # just associating one IObject or another with an
-                    # entry. We're already checking the type of the
-                    # new IObject, and that's the only error the
-                    # client can cause.
-                    try:
-                        # Do any field-specific validation.
-                        field.validate(value)
-                    except ConstraintNotSatisfied, e:
-                        # Try to get a string error message out of
-                        # the exception; otherwise use a generic message
-                        # instead of whatever object the raise site
-                        # thought would be a good idea.
-                        if (len(e.args) > 0 and
-                            isinstance(e.args[0], basestring)):
-                            error = e.args[0]
-                        else:
-                            error = "Constraint not satisfied."
-                        errors.append(u"%s: %s" % (repr_name, error))
-                        continue
-                    except RequiredMissing, e:
-                        error = "Missing required value."
-                        errors.append(u"%s: %s" % (repr_name, error))
-                    except (ValueError, ValidationError), e:
-                        error = str(e)
-                        if error == "":
-                            error = "Validation error"
-                        errors.append(u"%s: %s" % (repr_name, error))
-                        continue
+                try:
+                    # Do any field-specific validation.
+                    field.validate(value)
+                except ConstraintNotSatisfied, e:
+                    # Try to get a string error message out of
+                    # the exception; otherwise use a generic message
+                    # instead of whatever object the raise site
+                    # thought would be a good idea.
+                    if (len(e.args) > 0 and
+                        isinstance(e.args[0], basestring)):
+                        error = e.args[0]
+                    else:
+                        error = "Constraint not satisfied."
+                    errors.append(u"%s: %s" % (repr_name, error))
+                    continue
+                except RequiredMissing, e:
+                    error = "Missing required value."
+                    errors.append(u"%s: %s" % (repr_name, error))
+                except (ValueError, ValidationError), e:
+                    error = str(e)
+                    if error == "":
+                        error = "Validation error"
+                    errors.append(u"%s: %s" % (repr_name, error))
+                    continue
                 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

=== modified file 'src/lazr/restful/declarations.py'
--- src/lazr/restful/declarations.py	2010-08-26 19:49:20 +0000
+++ src/lazr/restful/declarations.py	2011-01-21 21:12:04 +0000
@@ -47,7 +47,11 @@
 from zope.interface.interface import fromFunction, InterfaceClass, TAGGED_DATA
 from zope.interface.interfaces import IInterface, IMethod
 from zope.schema import getFields
-from zope.schema.interfaces import IField, IText
+from zope.schema.interfaces import (
+    IField,
+    IObject,
+    IText,
+    )
 from zope.security.checker import CheckerPublic
 from zope.traversing.browser import absoluteURL
 
@@ -56,9 +60,9 @@
 from lazr.restful.fields import CollectionField, Reference
 from lazr.restful.interface import copy_field
 from lazr.restful.interfaces import (
-    ICollection, IEntry, IResourceDELETEOperation, IResourceGETOperation,
-    IResourcePOSTOperation, IWebServiceConfiguration, IWebServiceVersion,
-    LAZR_WEBSERVICE_NAME, LAZR_WEBSERVICE_NS)
+    ICollection, IEntry, IReference, IResourceDELETEOperation,
+    IResourceGETOperation, IResourcePOSTOperation, IWebServiceConfiguration,
+    IWebServiceVersion, LAZR_WEBSERVICE_NAME, LAZR_WEBSERVICE_NS)
 from lazr.restful import (
     Collection, Entry, EntryAdapterUtility, ResourceOperation, ObjectLink)
 from lazr.restful.security import protect_schema
@@ -204,6 +208,9 @@
     if not IField.providedBy(field):
         raise TypeError("exported() can only be used on IFields.")
 
+    if IObject.providedBy(field) and not IReference.providedBy(field):
+        raise TypeError("Object exported; use Reference instead.")
+
     # The first step is to turn the arguments into a
     # VersionedDict. We'll start by collecting annotations for the
     # earliest version, which we refer to as None because we don't

=== modified file 'src/lazr/restful/docs/webservice-declarations.txt'
--- src/lazr/restful/docs/webservice-declarations.txt	2010-08-26 19:49:20 +0000
+++ src/lazr/restful/docs/webservice-declarations.txt	2011-01-21 21:12:04 +0000
@@ -98,6 +98,23 @@
       ...
     TypeError: exported() can only be used on IFields.
 
+Object fields cannot be exported because they cause validation problems.
+
+    >>> from zope.schema import Object
+    >>> class UsesIObject(Interface):
+    ...     export_as_webservice_entry()
+    ...     object = exported(Object(schema=Interface))
+    Traceback (most recent call last):
+    TypeError: Object exported; use Reference instead.
+
+Instead you should use Reference, a subclass of Object designed to
+avoid the validation problems.
+
+    >>> from lazr.restful.fields import Reference
+    >>> class UsesIReference(Interface):
+    ...     export_as_webservice_entry()
+    ...     object = exported(Reference(schema=Interface))
+
 In the same vein, export_as_webservice_entry() can only be used on
 Interface.
 

=== modified file 'src/lazr/restful/docs/webservice.txt'
--- src/lazr/restful/docs/webservice.txt	2010-10-25 13:48:07 +0000
+++ src/lazr/restful/docs/webservice.txt	2011-01-21 21:12:04 +0000
@@ -22,7 +22,8 @@
     >>> __metaclass__ = type
 
     >>> from zope.interface import Interface, Attribute
-    >>> from zope.schema import Bool, Bytes, Int, Text, TextLine, Object
+    >>> from zope.schema import Bool, Bytes, Int, Text, TextLine
+    >>> from lazr.restful.fields import Reference
 
     >>> class ITestDataObject(Interface):
     ...     """A marker interface for data objects."""
@@ -33,7 +34,7 @@
     ...     name = TextLine(title=u"Name", required=True)
     ...     # favorite_recipe.schema will be set to IRecipe once
     ...     # IRecipe is defined.
-    ...     favorite_recipe = Object(schema=Interface)
+    ...     favorite_recipe = Reference(schema=Interface)
     ...     popularity = Int(readonly=True)
 
     >>> class ICommentTarget(ITestDataObject):
@@ -45,7 +46,7 @@
 
     >>> class ICookbook(ICommentTarget):
     ...     name = TextLine(title=u"Name", required=True)
-    ...     author = Object(schema=IAuthor)
+    ...     author = Reference(schema=IAuthor)
     ...     cuisine = TextLine(title=u"Cuisine", required=False, default=None)
     ...     recipes = Attribute("List of recipes published in this cookbook.")
     ...     cover = Bytes(0, 5000, title=u"An image of the cookbook's cover.")
@@ -60,8 +61,8 @@
 
     >>> class IRecipe(ICommentTarget):
     ...     id = Int(title=u"Unique ID", required=True)
-    ...     dish = Object(schema=IDish)
-    ...     cookbook = Object(schema=ICookbook)
+    ...     dish = Reference(schema=IDish)
+    ...     cookbook = Reference(schema=ICookbook)
     ...     instructions = Text(title=u"How to prepare the recipe.",
     ...         required=True)
     ...     private = Bool(title=u"Whether the public can see this recipe.",
@@ -579,7 +580,7 @@
     >>> from lazr.restful.fields import CollectionField
     >>> class IDishEntry(IEntry):
     ...     "The part of a dish that we expose through the web service."
-    ...     recipes = CollectionField(value_type=Object(schema=IRecipe))
+    ...     recipes = CollectionField(value_type=Reference(schema=IRecipe))
     ...     taggedValue(LAZR_WEBSERVICE_NAME,
     ...         dict(singular="dish", plural="dishes"))
 
@@ -590,10 +591,10 @@
 
     >>> class IRecipeEntry(IEntry):
     ...     "The part of a recipe that we expose through the web service."
-    ...     cookbook = Object(schema=ICookbook)
-    ...     dish = Object(schema=IDish)
+    ...     cookbook = Reference(schema=ICookbook)
+    ...     dish = Reference(schema=IDish)
     ...     instructions = Text(title=u"Name", required=True)
-    ...     comments = CollectionField(value_type=Object(schema=IComment))
+    ...     comments = CollectionField(value_type=Reference(schema=IComment))
     ...     taggedValue(LAZR_WEBSERVICE_NAME,
     ...         dict(singular="recipe", plural="recipes"))
 
@@ -603,8 +604,8 @@
     ...     cuisine = TextLine(title=u"Cuisine", required=False, default=None)
     ...     author = ReferenceChoice(
     ...         schema=IAuthor, vocabulary=AuthorVocabulary())
-    ...     recipes = CollectionField(value_type=Object(schema=IRecipe))
-    ...     comments = CollectionField(value_type=Object(schema=IComment))
+    ...     recipes = CollectionField(value_type=Reference(schema=IRecipe))
+    ...     comments = CollectionField(value_type=Reference(schema=IComment))
     ...     cover = Bytes(0, 5000, title=u"An image of the cookbook's cover.")
     ...     taggedValue(LAZR_WEBSERVICE_NAME,
     ...         dict(singular="cookbook", plural="cookbooks"))
@@ -962,7 +963,7 @@
     ...         TextLine(
     ...             __name__='cuisine', default=u'Brazilian', required=False),
     ...     )
-    ...     return_type = Object(schema=IRecipe)
+    ...     return_type = Reference(schema=IRecipe)
     ...
     ...     def call(self, author_name, title, cuisine):
     ...         cookbook = CookbookSet().newCookbook(

=== modified file 'src/lazr/restful/marshallers.py'
--- src/lazr/restful/marshallers.py	2010-10-25 16:31:09 +0000
+++ src/lazr/restful/marshallers.py	2011-01-21 21:12:04 +0000
@@ -525,7 +525,7 @@
     """A marshaller that turns URLs into data model objects.
 
     This marshaller can be used with a IChoice field (initialized
-    with a vocabulary) or with an IObject field (no vocabulary).
+    with a vocabulary) or with an IReference field (no vocabulary).
     """
 
     def __init__(self, field, request, vocabulary=None):

=== modified file 'src/lazr/restful/publisher.py'
--- src/lazr/restful/publisher.py	2011-01-17 23:48:11 +0000
+++ src/lazr/restful/publisher.py	2011-01-21 21:12:04 +0000
@@ -25,7 +25,7 @@
 from zope.interface import alsoProvides, implementer, implements
 from zope.publisher.interfaces import NotFound
 from zope.publisher.interfaces.browser import IBrowserRequest
-from zope.schema.interfaces import IBytes, IObject
+from zope.schema.interfaces import IBytes
 from zope.security.checker import ProxyFactory
 
 from lazr.uri import URI
@@ -35,8 +35,9 @@
     EntryResource, ScopedCollection, ServiceRootResource)
 from lazr.restful.interfaces import (
     IByteStorage, ICollection, ICollectionField, IEntry, IEntryField,
-    IHTTPResource, IServiceRootResource, IWebBrowserInitiatedRequest,
-    IWebServiceClientRequest, IWebServiceConfiguration, IWebServiceVersion)
+    IHTTPResource, IReference, IServiceRootResource,
+    IWebBrowserInitiatedRequest, IWebServiceClientRequest,
+    IWebServiceConfiguration, IWebServiceVersion)
 from lazr.restful.utils import tag_request_with_version_name
 
 
@@ -83,7 +84,7 @@
                 elif IBytes.providedBy(field):
                     return self._traverseToByteStorage(
                         request, entry, field, name)
-                elif IObject.providedBy(field):
+                elif IReference.providedBy(field):
                     sub_entry = getattr(entry, name, None)
                     if sub_entry is None:
                         raise NotFound(ob, name, request)

=== modified file 'src/lazr/restful/tales.py'
--- src/lazr/restful/tales.py	2011-01-19 19:00:10 +0000
+++ src/lazr/restful/tales.py	2011-01-21 21:12:04 +0000
@@ -25,7 +25,13 @@
 from zope.interface import implements
 from zope.interface.interfaces import IInterface
 from zope.schema import getFields
-from zope.schema.interfaces import IBytes, IChoice, IDate, IDatetime, IObject
+from zope.schema.interfaces import (
+    IBytes,
+    IChoice,
+    IDate,
+    IDatetime,
+    IObject,
+    )
 from zope.security.proxy import removeSecurityProxy
 from zope.publisher.interfaces.browser import IBrowserRequest
 from zope.traversing.browser import absoluteURL
@@ -38,7 +44,7 @@
 from lazr.restful._resource import UnknownEntryAdapter
 from lazr.restful.interfaces import (
     ICollection, ICollectionField, IEntry, IJSONRequestCache,
-    IReferenceChoice, IResourceDELETEOperation, IResourceGETOperation,
+    IReference, IResourceDELETEOperation, IResourceGETOperation,
     IResourceOperation, IResourcePOSTOperation, IScopedCollection,
     ITopLevelEntryLink, IWebServiceClientRequest, IWebServiceConfiguration,
     IWebServiceVersion, LAZR_WEBSERVICE_NAME)
@@ -512,8 +518,8 @@
         name = self.field.__name__
         if ICollectionField.providedBy(self.field):
             return name + '_collection_link'
-        elif (IObject.providedBy(self.field) or IBytes.providedBy(self.field)
-              or IReferenceChoice.providedBy(self.field)):
+        elif (IReference.providedBy(self.field)
+              or IBytes.providedBy(self.field)):
             return name + '_link'
         else:
             return name
@@ -548,8 +554,7 @@
     @property
     def is_represented_as_link(self):
         """Is this field represented as a link to another resource?"""
-        return (IObject.providedBy(self.field) or
-                IReferenceChoice.providedBy(self.field) or
+        return (IReference.providedBy(self.field) or
                 ICollectionField.providedBy(self.field) or
                 IBytes.providedBy(self.field) or
                 self.is_link)
@@ -582,9 +587,8 @@
         """Find an entry adapter for this field."""
         if ICollectionField.providedBy(self.field):
             schema = self.field.value_type.schema
-        elif (IObject.providedBy(self.field)
-              or IObjectLink.providedBy(self.field)
-              or IReferenceChoice.providedBy(self.field)):
+        elif (IReference.providedBy(self.field)
+              or IObjectLink.providedBy(self.field)):
             schema = self.field.schema
         else:
             raise TypeError("Field is not of a supported type.")

=== modified file 'src/lazr/restful/tests/test_declarations.py'
--- src/lazr/restful/tests/test_declarations.py	2010-11-04 18:21:58 +0000
+++ src/lazr/restful/tests/test_declarations.py	2011-01-21 21:12:04 +0000
@@ -13,6 +13,7 @@
     export_as_webservice_entry, exported, export_read_operation,
     export_write_operation, mutator_for, operation_for_version,
     operation_parameters)
+from lazr.restful.fields import Reference
 from lazr.restful.interfaces import (
     IEntry, IResourceGETOperation, IWebServiceConfiguration,
     IWebServiceVersion)
@@ -306,7 +307,7 @@
     export_as_webservice_entry(contributes_to=[IProduct])
     not_exported = TextLine(title=u'Not exported')
     development_branch = exported(
-        Object(schema=IBranch, readonly=True),
+        Reference(schema=IBranch, readonly=True),
         ('1.0', dict(exported_as='development_branch_10')),
         ('beta', dict(exported_as='development_branch')))
 

=== modified file 'src/lazr/restful/tests/test_navigation.py'
--- src/lazr/restful/tests/test_navigation.py	2010-01-11 17:45:07 +0000
+++ src/lazr/restful/tests/test_navigation.py	2011-01-21 21:12:04 +0000
@@ -9,10 +9,14 @@
 from zope.component import getSiteManager
 from zope.interface import Interface, implements
 from zope.publisher.interfaces import NotFound
-from zope.schema import Text, Object
+from zope.schema import Text
 from zope.testing.cleanup import cleanUp
 
-from lazr.restful.interfaces import IEntry, IWebServiceClientRequest
+from lazr.restful.fields import Reference
+from lazr.restful.interfaces import (
+    IEntry,
+    IWebServiceClientRequest,
+    )
 from lazr.restful.simple import Publication
 from lazr.restful.testing.webservice import FakeRequest
 
@@ -26,7 +30,7 @@
 class IParent(Interface):
     """Interface for a simple entry that contains another entry."""
     three = Text(title=u'Three')
-    child = Object(schema=IChild)
+    child = Reference(schema=IChild)
 
 
 class Child:

=== modified file 'src/lazr/restful/version.txt'
--- src/lazr/restful/version.txt	2011-01-20 20:25:31 +0000
+++ src/lazr/restful/version.txt	2011-01-21 21:12:04 +0000
@@ -1,1 +1,1 @@
-0.15.2
+0.15.3