lazr.restful

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

  1. disable-cache
  2. Merge into trunk
Proposed by Leonard Richardson
Status: Merged
Approved by: Gary Poster
Approved revision: 134
Merged at revision: 132
Proposed branch: lp:~leonardr/lazr.restful/disable-cache
Merge into: lp:lazr.restful
Diff against target: 114 lines (+50/-6)
4 files modified
src/lazr/restful/NEWS.txt (+5/-0)
src/lazr/restful/_resource.py (+29/-5)
src/lazr/restful/docs/multiversion.txt (+15/-0)
src/lazr/restful/version.txt (+1/-1)
To merge this branch: bzr merge lp:~leonardr/lazr.restful/disable-cache
Reviewer Review Type Date Requested Status
Gary Poster Approve
Review via email: mp+26612@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
lp:~leonardr/lazr.restful/disable-cache updated
134. By Leonard Richardson

Removed the special hack for ForbiddenAttribute errors.

Revision history for this message
Gary Poster (gary) wrote :

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
Revision history for this message
Leonard Richardson (leonardr) wrote :

> 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 updated
135. By Leonard Richardson

Response to feedback.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/lazr/restful/NEWS.txt'
2--- src/lazr/restful/NEWS.txt 2010-06-01 15:19:54 +0000
3+++ src/lazr/restful/NEWS.txt 2010-06-03 13:03:24 +0000
4@@ -2,6 +2,11 @@
5 NEWS for lazr.restful
6 =====================
7
8+0.9.28 (Development)
9+====================
10+
11+Fixed some test failures.
12+
13 0.9.27 (2010-06-01)
14 ====================
15
16
17=== modified file 'src/lazr/restful/_resource.py'
18--- src/lazr/restful/_resource.py 2010-05-24 13:52:30 +0000
19+++ src/lazr/restful/_resource.py 2010-06-03 13:03:24 +0000
20@@ -69,7 +69,7 @@
21 from zope.schema import ValidationError, getFieldsInOrder
22 from zope.schema.interfaces import (
23 ConstraintNotSatisfied, IBytes, IField, IObject, RequiredMissing)
24-from zope.security.interfaces import Unauthorized
25+from zope.security.interfaces import ForbiddenAttribute, Unauthorized
26 from zope.security.proxy import getChecker, removeSecurityProxy
27 from zope.security.management import checkPermission
28 from zope.traversing.browser import absoluteURL, AbsoluteURL
29@@ -1458,7 +1458,11 @@
30 def redacted_fields(self):
31 """Names the fields the current user doesn't have permission to see."""
32 failures = []
33- checker = getChecker(self.context)
34+ try:
35+ checker = getChecker(self.context)
36+ except TypeError:
37+ # There's no permission checker.
38+ checker = None
39 for name, field in getFieldsInOrder(self.entry.schema):
40 try:
41 # Can we view the field's value? We check the
42@@ -1466,9 +1470,29 @@
43 # checker, because doing it indirectly by fetching the
44 # value may have very slow side effects such as
45 # database hits.
46- tagged_values = field.getTaggedValue('lazr.restful.exported')
47- original_name = tagged_values['original_name']
48- checker.check(self.context, original_name)
49+ try:
50+ tagged_values = field.getTaggedValue('lazr.restful.exported')
51+ original_name = tagged_values['original_name']
52+ except KeyError, e:
53+ # This field has no tagged values, or is missing
54+ # the 'original_name' value. Its entry class was
55+ # probably created by hand rather than by tagging
56+ # an interface. In that case, it's the
57+ # programmer's responsibility to set
58+ # 'original_name' if the web service field name
59+ # differs from the underlying interface's field
60+ # name. Since 'original_name' is not present, assume the
61+ # names are the same.
62+ original_name = name
63+ if checker is None:
64+ # This is more expensive than using a Zope
65+ # checker, but there is no checker, so either
66+ # there is no permission control on this object,
67+ # or permission control is implemented some other
68+ # way.
69+ getattr(self.context, original_name)
70+ else:
71+ checker.check(self.context, original_name)
72 except Unauthorized:
73 # This is an expensive operation that will make this
74 # request more expensive still, but it happens
75
76=== modified file 'src/lazr/restful/docs/multiversion.txt'
77--- src/lazr/restful/docs/multiversion.txt 2010-02-18 15:17:09 +0000
78+++ src/lazr/restful/docs/multiversion.txt 2010-06-03 13:03:24 +0000
79@@ -259,6 +259,19 @@
80 ... taggedValue(LAZR_WEBSERVICE_NAME,
81 ... dict(singular="contact", plural="contacts"))
82
83+IContactEntry10's "phone_number" and "fax_number" fields correspond to
84+IContact's 'phone' and 'fax' fields. Since we changed the name, we
85+must set the tags on the fields object giving the corresponding field
86+name in IContact. Ordinarily, the lazr.restful declarations take care
87+of this for us, but here we need to do it ourselves because we're
88+defining the IEntry classes by hand.
89+
90+ >>> from lazr.restful.declarations import LAZR_WEBSERVICE_EXPORTED
91+ >>> IContactEntry10['phone_number'].setTaggedValue(
92+ ... LAZR_WEBSERVICE_EXPORTED, dict(original_name="phone"))
93+ >>> IContactEntry10['fax_number'].setTaggedValue(
94+ ... LAZR_WEBSERVICE_EXPORTED, dict(original_name="fax"))
95+
96 The "dev" version drops the "fax_number" field because fax machines
97 are obsolete.
98
99@@ -267,6 +280,8 @@
100 ... phone_number = TextLine(title=u"Phone number", required=True)
101 ... taggedValue(LAZR_WEBSERVICE_NAME,
102 ... dict(singular="contact", plural="contacts"))
103+ >>> IContactEntryDev['phone_number'].setTaggedValue(
104+ ... LAZR_WEBSERVICE_EXPORTED, dict(original_name="phone"))
105
106 Implementing the entry resources
107 ================================
108
109=== modified file 'src/lazr/restful/version.txt'
110--- src/lazr/restful/version.txt 2010-05-21 13:11:29 +0000
111+++ src/lazr/restful/version.txt 2010-06-03 13:03:24 +0000
112@@ -1,1 +1,1 @@
113-0.9.27
114+0.9.28

Subscribers

People subscribed via source and target branches