Merge lp:~leonardr/lazr.restful/disable-cache into lp:lazr.restful

[Leonard Richardson]

Argh, I got so used to running Launchpad integration and performance tests in my previous branch that I forgot to run the lazr.restful test suite before landing. My optimized implementation of redacted_fields fails in three cases, each of which was covered by one or another of the various example web services defined in lazr.restful:

1. when there is no Zope permission checker registered for an IEntry implementation
2. when a failure results in a ForbiddenAttribute exception rather than an Unauthorized exception
3. when an IEntry implementation was defined from scratch and as such lacks tagged values (specifically, lacks 'original_name')

I changed redacted_fields to handle all these cases. I'm not really sure why sometimes a permission failure would result in ForbiddenAttribute and sometimes in Unauthorized, but it does happen.

[lp:~leonardr/lazr.restful/disable-cache]

134. By Leonard Richardson on 2010-06-03

Removed the special hack for ForbiddenAttribute errors.

[Gary Poster (gary)]

Looks good, thank you.

In 'src/lazr/restful/_resource.py', please revert ``from zope.security.interfaces import ForbiddenAttribute, Unauthorized`` to ``from zope.security.interfaces import Unauthorized``.

In the same file, why is ``tagged_values = field.getTaggedValue('lazr.restful.exported')`` within the try-except block? It seems like the check for original_name is the only line that should be there.

Gary

review: Approve

[Leonard Richardson (leonardr)]

> In 'src/lazr/restful/_resource.py', please revert ``from
> zope.security.interfaces import ForbiddenAttribute, Unauthorized`` to ``from
> zope.security.interfaces import Unauthorized``.

Done.

> In the same file, why is ``tagged_values =
> field.getTaggedValue('lazr.restful.exported')`` within the try-except block?
> It seems like the check for original_name is the only line that should be
> there.

If there are no tagged values at all, then field.getTaggedValue() will raise a KeyError. So both of the lines within the try-except block can raise KeyError--there might be no tagged values, or there might be a particular value missing.

Leonard

[lp:~leonardr/lazr.restful/disable-cache]

135. By Leonard Richardson on 2010-06-03

Response to feedback.

=== modified file 'src/lazr/restful/NEWS.txt'
--- src/lazr/restful/NEWS.txt	2010-06-01 15:19:54 +0000
+++ src/lazr/restful/NEWS.txt	2010-06-03 13:03:24 +0000
@@ -2,6 +2,11 @@
 NEWS for lazr.restful
 =====================
 
+0.9.28 (Development)
+====================
+
+Fixed some test failures.
+
 0.9.27 (2010-06-01)
 ====================
 

=== modified file 'src/lazr/restful/_resource.py'
--- src/lazr/restful/_resource.py	2010-05-24 13:52:30 +0000
+++ src/lazr/restful/_resource.py	2010-06-03 13:03:24 +0000
@@ -69,7 +69,7 @@
 from zope.schema import ValidationError, getFieldsInOrder
 from zope.schema.interfaces import (
     ConstraintNotSatisfied, IBytes, IField, IObject, RequiredMissing)
-from zope.security.interfaces import Unauthorized
+from zope.security.interfaces import ForbiddenAttribute, Unauthorized
 from zope.security.proxy import getChecker, removeSecurityProxy
 from zope.security.management import checkPermission
 from zope.traversing.browser import absoluteURL, AbsoluteURL
@@ -1458,7 +1458,11 @@
     def redacted_fields(self):
         """Names the fields the current user doesn't have permission to see."""
         failures = []
-        checker = getChecker(self.context)
+        try:
+            checker = getChecker(self.context)
+        except TypeError:
+            # There's no permission checker.
+            checker = None
         for name, field in getFieldsInOrder(self.entry.schema):
             try:
                 # Can we view the field's value? We check the
@@ -1466,9 +1470,29 @@
                 # checker, because doing it indirectly by fetching the
                 # value may have very slow side effects such as
                 # database hits.
-                tagged_values = field.getTaggedValue('lazr.restful.exported')
-                original_name = tagged_values['original_name']
-                checker.check(self.context, original_name)
+                try:
+                    tagged_values = field.getTaggedValue('lazr.restful.exported')
+                    original_name = tagged_values['original_name']
+                except KeyError, e:
+                    # This field has no tagged values, or is missing
+                    # the 'original_name' value. Its entry class was
+                    # probably created by hand rather than by tagging
+                    # an interface. In that case, it's the
+                    # programmer's responsibility to set
+                    # 'original_name' if the web service field name
+                    # differs from the underlying interface's field
+                    # name. Since 'original_name' is not present, assume the
+                    # names are the same.
+                    original_name = name
+                if checker is None:
+                    # This is more expensive than using a Zope
+                    # checker, but there is no checker, so either
+                    # there is no permission control on this object,
+                    # or permission control is implemented some other
+                    # way.
+                    getattr(self.context, original_name)
+                else:
+                    checker.check(self.context, original_name)
             except Unauthorized:
                 # This is an expensive operation that will make this
                 # request more expensive still, but it happens

=== modified file 'src/lazr/restful/docs/multiversion.txt'
--- src/lazr/restful/docs/multiversion.txt	2010-02-18 15:17:09 +0000
+++ src/lazr/restful/docs/multiversion.txt	2010-06-03 13:03:24 +0000
@@ -259,6 +259,19 @@
     ...     taggedValue(LAZR_WEBSERVICE_NAME,
     ...                 dict(singular="contact", plural="contacts"))
 
+IContactEntry10's "phone_number" and "fax_number" fields correspond to
+IContact's 'phone' and 'fax' fields. Since we changed the name, we
+must set the tags on the fields object giving the corresponding field
+name in IContact. Ordinarily, the lazr.restful declarations take care
+of this for us, but here we need to do it ourselves because we're
+defining the IEntry classes by hand.
+
+    >>> from lazr.restful.declarations import LAZR_WEBSERVICE_EXPORTED
+    >>> IContactEntry10['phone_number'].setTaggedValue(
+    ...     LAZR_WEBSERVICE_EXPORTED, dict(original_name="phone"))
+    >>> IContactEntry10['fax_number'].setTaggedValue(
+    ...     LAZR_WEBSERVICE_EXPORTED, dict(original_name="fax"))
+
 The "dev" version drops the "fax_number" field because fax machines
 are obsolete.
 
@@ -267,6 +280,8 @@
     ...     phone_number = TextLine(title=u"Phone number", required=True)
     ...     taggedValue(LAZR_WEBSERVICE_NAME,
     ...                 dict(singular="contact", plural="contacts"))
+    >>> IContactEntryDev['phone_number'].setTaggedValue(
+    ...     LAZR_WEBSERVICE_EXPORTED, dict(original_name="phone"))
 
 Implementing the entry resources
 ================================

=== modified file 'src/lazr/restful/version.txt'
--- src/lazr/restful/version.txt	2010-05-21 13:11:29 +0000
+++ src/lazr/restful/version.txt	2010-06-03 13:03:24 +0000
@@ -1,1 +1,1 @@
-0.9.27
+0.9.28