Merge lp:~benji/lazr.restful/tweak-etag into lp:lazr.restful

Proposed by Benji York
Status: Merged
Approved by: Paul Hummer
Approved revision: 155
Merged at revision: 151
Proposed branch: lp:~benji/lazr.restful/tweak-etag
Merge into: lp:lazr.restful
Diff against target: 527 lines (+347/-51)
6 files modified
src/lazr/restful/_resource.py (+61/-48)
src/lazr/restful/testing/helpers.py (+12/-0)
src/lazr/restful/tests/test_etag.py (+253/-0)
src/lazr/restful/tests/test_utils.py (+15/-3)
src/lazr/restful/utils.py (+5/-0)
versions.cfg (+1/-0)
To merge this branch: bzr merge lp:~benji/lazr.restful/tweak-etag
Reviewer Review Type Date Requested Status
Paul Hummer (community) Approve
Review via email: mp+37194@code.launchpad.net

Description of the change

The ETag generation in lazr.restful was a bit too conservative. It used the revision number in every ETag when it is only needed in a subset. This branch moves the inclusion of revno "down" into the subclasses that need it. Tests for the various ETag generating method/functions were added as well as tests for the (existing) read/write ETag checking routines.

Leonard and I had pre-, intra-, and post-implementation discussions.

To post a comment you must log in.
Revision history for this message
Paul Hummer (rockstar) :
review: Approve
Revision history for this message
Stuart Bishop (stub) wrote :

I notice you are adding a roman numeral package to buildout, but are not using it.

Revision history for this message
Benji York (benji) wrote :

On Thu, Sep 30, 2010 at 11:15 PM, Stuart Bishop
<email address hidden> wrote:
> I notice you are adding a roman numeral package to buildout, but are not using it.

That's a change from another branch that I'll probably be reverting. I
think I got a hold of a bad docutils somehow that was missing roman.
--
Benji York

Revision history for this message
Leonard Richardson (leonardr) wrote :

I have some minor comments, but overall this is a good branch.

In addition to the 'roman' package, you've got a normalizer that makes Unicode strings look the same as regular strings. I don't think that's the best way to solve your problem, but you should talk it over with Gary.

make_entry_etag_cores no longer cares whether a field "might change over time", so you should remove that bit from the docstring.

Similarly, you don't need "modifiable" in test_writable_fields and the similar tests. And you don't need test_modifiable_fields at all. It would be nice to test fields with this property, but you can't test them in a unit test of make_entry_etag_cores, which no longer looks at the 'modifiable' attribute.

Do you want to add a test to test_cores_change_with_value that shows a case where the write core changes and the read core stays the same?

Revision history for this message
Benji York (benji) wrote :

I fixed the roman problem by nuking my docutils eggs and reinstalling.

lp:~benji/lazr.restful/tweak-etag updated
156. By Benji York <benji@benji-laptop>

remove unneeded reNormalizer

Revision history for this message
Benji York (benji) wrote :

I made all the changes Leonard suggested except for the last one which we discussed away.

lp:~benji/lazr.restful/tweak-etag updated
157. By Benji York <benji@benji-laptop>

make some changes suggested in review

158. By Benji York <benji@benji-laptop>

add a comment about read/write fields

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/lazr/restful/_resource.py'
2--- src/lazr/restful/_resource.py 2010-09-23 19:14:55 +0000
3+++ src/lazr/restful/_resource.py 2010-10-04 15:46:40 +0000
4@@ -89,7 +89,10 @@
5 IWebServiceConfiguration, IWebServiceLayer, IWebServiceVersion,
6 LAZR_WEBSERVICE_NAME)
7 from lazr.restful.utils import (
8- get_current_web_service_request, sorted_named_things)
9+ extract_write_portion,
10+ get_current_web_service_request,
11+ sorted_named_things,
12+ )
13
14
15 # The path to the WADL XML Schema definition.
16@@ -378,12 +381,6 @@
17 # different representations of a resource interchangeably.
18 core_hashes[-1].update("\0" + media_type)
19
20- # Append the revision number, because the algorithm for
21- # generating the representation might itself change across
22- # versions.
23- revno = getUtility(IWebServiceConfiguration).code_revision
24- core_hashes[-1].update("\0" + revno)
25-
26 etag = '"%s"' % "-".join([core.hexdigest() for core in core_hashes])
27 self.etags_by_media_type[media_type] = etag
28 return etag
29@@ -1255,14 +1252,21 @@
30
31 do_PATCH = do_PUT
32
33- def _getETagCores(self, unmarshalled_field_values=None):
34+ def _getETagCores(self, cache=None):
35 """Calculate the ETag for an entry field.
36
37 The core of the ETag is the field value itself.
38+
39+ :arg cache: is ignored.
40 """
41- name, value = self._unmarshallField(
42- self.context.name, self.context.field)
43- return [str(value)]
44+ value = self._unmarshallField(
45+ self.context.name, self.context.field)[1]
46+
47+ # Append the revision number, because the algorithm for
48+ # generating the representation might itself change across
49+ # versions.
50+ revno = getUtility(IWebServiceConfiguration).code_revision
51+ return [core.encode('utf-8') for core in [revno, unicode(value)]]
52
53 def _representation(self, media_type):
54 """Create a representation of the field value."""
55@@ -1304,6 +1308,29 @@
56 self.context = entryfield
57 self.request = request
58
59+def make_entry_etag_cores(field_details):
60+ """Given the details of an entry's fields, calculate its ETag cores.
61+
62+ :arg field_details: A list of field names and relevant field information,
63+ in particular whether or not the field is writable and its current value.
64+ """
65+ unwritable_values = []
66+ writable_values = []
67+ for name, details in field_details:
68+ if details['writable']:
69+ # The client can write to this value.
70+ bucket = writable_values
71+ else:
72+ # The client can't write to this value (it might be read-only or
73+ # it might just be non-web-service writable.
74+ bucket = unwritable_values
75+ bucket.append(decode_value(details['value']))
76+
77+ unwritable = "\0".join(unwritable_values).encode("utf-8")
78+ writable = "\0".join(writable_values).encode("utf-8")
79+ return [unwritable, writable]
80+
81+
82
83 class EntryResource(CustomOperationResourceMixin,
84 FieldUnmarshallerMixin, EntryManipulatingResource):
85@@ -1338,31 +1365,23 @@
86 unmarshalled values, obtained during some other operation such
87 as the construction of a representation.
88 """
89- unwritable_values = []
90- writable_values = []
91+ if unmarshalled_field_values is None:
92+ unmarshalled_field_values = {}
93+
94+ field_details = []
95 for name, field in getFieldsInOrder(self.entry.schema):
96- if self.isModifiableField(field, True):
97- # The client can write to this value.
98- bucket = writable_values
99- elif self.isModifiableField(field, False):
100- # The client can't write this value, but it still
101- # might change.
102- bucket = unwritable_values
103- else:
104- # This value can never change, and as such does not
105- # need to be included in the ETag.
106- continue
107- if (unmarshalled_field_values is not None
108- and unmarshalled_field_values.get(name)):
109- value = unmarshalled_field_values[name]
110- else:
111- ignored, value = self._unmarshallField(name, field)
112- bucket.append(decode_value(value))
113-
114- unwritable = "\0".join(unwritable_values).encode("utf-8")
115- writable = "\0".join(writable_values).encode("utf-8")
116- return [unwritable, writable]
117-
118+ details = {}
119+ # Add in any values provided by the caller.
120+ # The value of the field is either passed in, or extracted.
121+ details['value'] = unmarshalled_field_values.get(
122+ name, self._unmarshallField(name, field)[1])
123+
124+ # The client can write to this field.
125+ details['writable'] = self.isModifiableField(field, True)
126+
127+ field_details.append((name, details))
128+
129+ return make_entry_etag_cores(field_details)
130
131 def _etagMatchesForWrite(self, existing_etag, incoming_etags):
132 """Make sure no other client has modified this resource.
133@@ -1373,15 +1392,9 @@
134 on conditional writes where the only fields that changed are
135 read-only fields that can't possibly cause a conflict.
136 """
137- existing_write_portion = existing_etag.split('-', 1)[-1]
138- for etag in incoming_etags:
139- if '-' in etag:
140- incoming_write_portion = etag.rsplit('-', 1)[-1]
141- else:
142- incoming_write_portion = etag
143- if existing_write_portion == incoming_write_portion:
144- return True
145- return False
146+ incoming_write_portions = map(extract_write_portion, incoming_etags)
147+ existing_write_portion = extract_write_portion(existing_etag)
148+ return existing_write_portion in incoming_write_portions
149
150
151 def toDataForJSON(self):
152@@ -1412,7 +1425,7 @@
153 "Cannot create data structure for media type %s"
154 % media_type)
155 data[repr_name] = repr_value
156- unmarshalled_field_values[name] = repr_value
157+ unmarshalled_field_values[name] = repr_value
158
159 etag = self.getETag(media_type, unmarshalled_field_values)
160 data['http_etag'] = etag
161@@ -1778,10 +1791,10 @@
162 """Calculate an ETag for a representation of this resource.
163
164 The service root resource changes only when the software
165- itself changes. This information goes into the ETag already,
166- so there's no need to provide anything.
167+ itself changes.
168 """
169- return ['']
170+ revno = getUtility(IWebServiceConfiguration).code_revision
171+ return [revno.encode('utf-8')]
172
173 def __call__(self, REQUEST=None):
174 """Handle a GET request."""
175
176=== modified file 'src/lazr/restful/testing/helpers.py'
177--- src/lazr/restful/testing/helpers.py 2010-07-15 14:24:56 +0000
178+++ src/lazr/restful/testing/helpers.py 2010-10-04 15:46:40 +0000
179@@ -5,6 +5,12 @@
180 from zope.interface import implements
181
182 from lazr.restful.interfaces import IWebServiceConfiguration
183+from lazr.restful.simple import (
184+ Request,
185+ RootResource,
186+ )
187+from lazr.restful.testing.webservice import WebServiceTestPublication
188+from lazr.restful.utils import tag_request_with_version_name
189
190
191 def create_test_module(name, *contents):
192@@ -40,6 +46,12 @@
193 last_version_with_mutator_named_operations = "1.0"
194 code_revision = "1.0b"
195 default_batch_size = 50
196+ hostname = 'example.com'
197
198 def get_request_user(self):
199 return 'A user'
200+
201+ def createRequest(self, body_instream, environ):
202+ request = Request(body_instream, environ)
203+ request.setPublication(WebServiceTestPublication(RootResource))
204+ return request
205
206=== added file 'src/lazr/restful/tests/test_etag.py'
207--- src/lazr/restful/tests/test_etag.py 1970-01-01 00:00:00 +0000
208+++ src/lazr/restful/tests/test_etag.py 2010-10-04 15:46:40 +0000
209@@ -0,0 +1,253 @@
210+# Copyright 2008 Canonical Ltd. All rights reserved.
211+"""Tests for ETag generation."""
212+
213+__metaclass__ = type
214+
215+import unittest
216+
217+from zope.component import provideUtility
218+
219+from lazr.restful.interfaces import IWebServiceConfiguration
220+from lazr.restful.testing.helpers import TestWebServiceConfiguration
221+from lazr.restful.testing.webservice import create_web_service_request
222+from lazr.restful._resource import (
223+ EntryFieldResource,
224+ EntryResource,
225+ HTTPResource,
226+ ServiceRootResource,
227+ make_entry_etag_cores,
228+ )
229+
230+
231+class TestEntryResourceETags(unittest.TestCase):
232+ # The EntryResource uses the field values that can be written or might
233+ # othwerise change as the basis for its ETags. The make_entry_etag_cores
234+ # function is passed the data about the fields and returns the read and
235+ # write cores.
236+
237+ def test_no_field_details(self):
238+ # If make_entry_etag_cores is given no field details (because no
239+ # fields exist), the resulting cores empty strings.
240+ self.assertEquals(make_entry_etag_cores([]), ['', ''])
241+
242+ def test_writable_fields(self):
243+ # If there are writable fields, their values are incorporated into the
244+ # writable portion of the cores.
245+ field_details = [
246+ ('first_field',
247+ {'writable': True,
248+ 'value': 'first'}),
249+ ('second_field',
250+ {'writable': True,
251+ 'value': 'second'}),
252+ ]
253+ self.assertEquals(
254+ make_entry_etag_cores(field_details), ['', 'first\0second'])
255+
256+ def test_unchanging_fields(self):
257+ # If there are fields that are not writable their values are still
258+ # reflected in the generated cores because we want and addition or
259+ # removal of read-only fields to trigger a new ETag.
260+ field_details = [
261+ ('first_field',
262+ {'writable': False,
263+ 'value': 'the value'}),
264+ ]
265+ self.assertEquals(
266+ make_entry_etag_cores(field_details),
267+ ['the value', ''])
268+
269+ def test_combinations_of_fields(self):
270+ # If there are a combination of writable, changable, and unchanable
271+ # fields, their values are reflected in the resulting cores.
272+ field_details = [
273+ ('first_writable',
274+ {'writable': True,
275+ 'value': 'first-writable'}),
276+ ('second_writable',
277+ {'writable': True,
278+ 'value': 'second-writable'}),
279+ ('first_non_writable',
280+ {'writable': False,
281+ 'value': 'first-not-writable'}),
282+ ('second_non_writable',
283+ {'writable': False,
284+ 'value': 'second-not-writable'}),
285+ ]
286+ self.assertEquals(
287+ make_entry_etag_cores(field_details),
288+ ['first-not-writable\x00second-not-writable',
289+ 'first-writable\x00second-writable'])
290+
291+
292+class TestHTTPResourceETags(unittest.TestCase):
293+
294+ def test_getETag_is_a_noop(self):
295+ # The HTTPResource class implements a do-nothing _getETagCores in order to
296+ # be conservative (because it's not aware of the nature of all possible
297+ # subclasses).
298+ self.assertEquals(HTTPResource(None, None)._getETagCores(), None)
299+
300+
301+class TestHTTPResourceETags(unittest.TestCase):
302+
303+ def test_getETag_is_a_noop(self):
304+ # The HTTPResource class implements a do-nothing _getETagCores in order to
305+ # be conservative (because it's not aware of the nature of all possible
306+ # subclasses).
307+ self.assertEquals(HTTPResource(None, None)._getETagCores(), None)
308+
309+
310+class FauxEntryField:
311+ entry = None
312+ name = 'field_name'
313+ field = None
314+
315+
316+class EntryFieldResourceTests(unittest.TestCase):
317+ # Tests for ETags of EntryFieldResource objects.
318+
319+ # Because the ETag generation only takes into account the field value and
320+ # the web service revision number (and not whether the field is read-write
321+ # or read-only) these tests don't mention the read-write/read-only nature
322+ # of the field in question.
323+
324+ def setUp(self):
325+ self.config = TestWebServiceConfiguration()
326+ provideUtility(self.config, IWebServiceConfiguration)
327+ self.resource = EntryFieldResource(FauxEntryField(), None)
328+
329+ def set_field_value(self, value):
330+ """Set the value of the fake field the EntryFieldResource references.
331+ """
332+ self.resource._unmarshalled_field_cache['field_name'] = (
333+ 'field_name', value)
334+ # We have to clear the etag cache for a new value to be generated.
335+ # XXX benji 2010-09-30 [bug=652459] Does this mean there is an error
336+ # condition that occurs when something other than applyChanges (which
337+ # invalidates the cache) modifies a field's value?
338+ self.resource.etags_by_media_type = {}
339+
340+ def test_cores_change_with_revno(self):
341+ # The ETag cores should change if the revision (not the version) of
342+ # the web service change.
343+ self.set_field_value('this is the field value')
344+
345+ # Find the cores generated with a given revision...
346+ self.config.code_revision = u'42'
347+ first_cores = self.resource._getETagCores(self.resource.JSON_TYPE)
348+
349+ # ...find the cores generated with a different revision.
350+ self.config.code_revision = u'99'
351+ second_cores = self.resource._getETagCores(self.resource.JSON_TYPE)
352+
353+ # The cores should be different.
354+ self.assertNotEqual(first_cores, second_cores)
355+ # In particular, the read core should be the same between the two, but
356+ # the write core should be different.
357+ self.assertEqual(first_cores[1], second_cores[1])
358+ self.assertNotEqual(first_cores[0], second_cores[0])
359+
360+ def test_cores_change_with_value(self):
361+ # The ETag cores should change if the value of the field change.
362+ # Find the cores generated with a given value...
363+ self.set_field_value('first value')
364+ first_cores = self.resource._getETagCores(self.resource.JSON_TYPE)
365+
366+ # ...find the cores generated with a different value.
367+ self.set_field_value('second value')
368+ second_cores = self.resource._getETagCores(self.resource.JSON_TYPE)
369+
370+ # The cores should be different.
371+ self.assertNotEqual(first_cores, second_cores)
372+ # In particular, the read core should be different between the two,
373+ # but the write core should be the same.
374+ self.assertNotEqual(first_cores[1], second_cores[1])
375+ self.assertEqual(first_cores[0], second_cores[0])
376+
377+
378+class ServiceRootResourceTests(unittest.TestCase):
379+ # Tests for ETags of EntryFieldResource objects.
380+
381+ def setUp(self):
382+ self.config = TestWebServiceConfiguration()
383+ provideUtility(self.config, IWebServiceConfiguration)
384+ self.resource = ServiceRootResource()
385+
386+ def test_cores_change_with_revno(self):
387+ # The ETag core should change if the revision (not the version) of the
388+ # web service change.
389+
390+ # Find the cores generated with a given revision...
391+ self.config.code_revision = u'42'
392+ first_cores = self.resource._getETagCores(self.resource.JSON_TYPE)
393+
394+ # ...find the cores generated with a different revision.
395+ self.config.code_revision = u'99'
396+ second_cores = self.resource._getETagCores(self.resource.JSON_TYPE)
397+
398+ # The cores should be different.
399+ self.assertNotEqual(first_cores, second_cores)
400+
401+
402+class TestableHTTPResource(HTTPResource):
403+ """A HTTPResource that lest us set the ETags from the outside."""
404+
405+ def _parseETags(self, *args):
406+ return self.incoming_etags
407+
408+ def getETag(self, *args):
409+ return self.existing_etag
410+
411+
412+class TestConditionalGet(unittest.TestCase):
413+
414+ def setUp(self):
415+ self.config = TestWebServiceConfiguration()
416+ provideUtility(self.config, IWebServiceConfiguration)
417+ self.request = create_web_service_request('/1.0')
418+ self.resource = TestableHTTPResource(None, self.request)
419+
420+ def test_etags_are_the_same(self):
421+ # If one of the ETags present in an incoming request is the same as
422+ # the ETag that represents the current object's state, then
423+ # a conditional GET should return "Not Modified" (304).
424+ self.resource.incoming_etags = ['1', '2', '3']
425+ self.resource.existing_etag = '2'
426+ self.assertEquals(self.resource.handleConditionalGET(), None)
427+ self.assertEquals(self.request.response.getStatus(), 304)
428+
429+ def test_etags_differ(self):
430+ # If none of the ETags present in an incoming request is the same as
431+ # the ETag that represents the current object's state, then a
432+ # conditional GET should result in a new representation of the object
433+ # being returned.
434+ self.resource.incoming_etags = ['1', '2', '3']
435+ self.resource.existing_etag = '99'
436+ self.assertNotEquals(self.resource.handleConditionalGET(), None)
437+
438+
439+class TestConditionalWrite(unittest.TestCase):
440+
441+ def setUp(self):
442+ self.config = TestWebServiceConfiguration()
443+ provideUtility(self.config, IWebServiceConfiguration)
444+ self.request = create_web_service_request('/1.0')
445+ self.resource = TestableHTTPResource(None, self.request)
446+
447+ def test_etags_are_the_same(self):
448+ # If one of the ETags present in an incoming request is the same as
449+ # the ETag that represents the current object's state, then
450+ # the write should be applied.
451+ self.resource.incoming_etags = ['1', '2', '3']
452+ self.resource.existing_etag = '2'
453+ self.assertNotEquals(self.resource.handleConditionalWrite(), None)
454+
455+ def test_etags_differ(self):
456+ # If one of the ETags present in an incoming request is the same as
457+ # the ETag that represents the current object's state, then
458+ # the write should fail.
459+ self.resource.incoming_etags = ['1', '2', '3']
460+ self.resource.existing_etag = '99'
461+ self.assertEquals(self.resource.handleConditionalWrite(), None)
462+ self.assertEquals(self.request.response.getStatus(), 412)
463
464=== modified file 'src/lazr/restful/tests/test_utils.py'
465--- src/lazr/restful/tests/test_utils.py 2010-08-24 22:12:38 +0000
466+++ src/lazr/restful/tests/test_utils.py 2010-10-04 15:46:40 +0000
467@@ -11,12 +11,25 @@
468 from zope.security.management import (
469 endInteraction, newInteraction, queryInteraction)
470
471-from lazr.restful.utils import (get_current_browser_request,
472- is_total_size_link_active, sorted_named_things)
473+from lazr.restful.utils import (
474+ extract_write_portion,
475+ get_current_browser_request,
476+ is_total_size_link_active,
477+ sorted_named_things,
478+ )
479
480
481 class TestUtils(unittest.TestCase):
482
483+ def test_two_part_extract_write_portion(self):
484+ # ETags are sometimes two-part. A hyphen seperates the parts if so.
485+ self.assertEqual('write', extract_write_portion('read-write'))
486+
487+ def test_one_part_extract_write_portion(self):
488+ # ETags are sometimes one-part. If so, writes are predicated on the
489+ # whole ETag.
490+ self.assertEqual('etag', extract_write_portion('etag'))
491+
492 def test_get_current_browser_request_no_interaction(self):
493 # When there's no interaction setup, get_current_browser_request()
494 # returns None.
495@@ -77,4 +90,3 @@
496
497 # For the sake of convenience, test_get_current_web_service_request()
498 # and tag_request_with_version_name() are tested in test_webservice.py.
499-
500
501=== modified file 'src/lazr/restful/utils.py'
502--- src/lazr/restful/utils.py 2010-08-24 22:12:38 +0000
503+++ src/lazr/restful/utils.py 2010-10-04 15:46:40 +0000
504@@ -38,6 +38,11 @@
505 missing = object()
506
507
508+def extract_write_portion(etag):
509+ """Retrieve the portion of an etag that predicates writes."""
510+ return etag.split('-', 1)[-1]
511+
512+
513 def is_total_size_link_active(version, config):
514 versions = config.active_versions
515 total_size_link_version = config.first_version_with_total_size_link
516
517=== modified file 'versions.cfg'
518--- versions.cfg 2010-08-24 20:04:30 +0000
519+++ versions.cfg 2010-10-04 15:46:40 +0000
520@@ -24,6 +24,7 @@
521 lxml = 2.2.7
522 martian = 0.11
523 pytz = 2010h
524+roman = 1.4.0
525 setuptools = 0.6c11
526 simplejson = 2.0.9
527 transaction = 1.0.0

Subscribers

People subscribed via source and target branches