Merge lp:~andrea.corbellini/lazr.restful/fix-375353 into lp:lazr.restful

[Andrea Corbellini]

[Andrea Corbellini (andrea.corbellini)]

Here's my attempt to fix bug 375353.

This branch adds a new method annotator (cache_for) that allows to specify how much seconds the response should be cached by the browser. BaseResourceOperationAdapter.call() then sets the correct Cache-control HTTP header.

Note that Cache-control is not supported by HTTP/1.0 clients:
http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9

[Francis J. Lacoste (flacoste)]

On Thursday 04 June 2009, Andrea Corbellini wrote:
> You have been requested to review the proposed merge of
> lp:~andrea-bs/lazr.restful/fix-375353 into lp:lazr.restful.
>
> Here's my attempt to fix bug 375353.
>
> This branch adds a new method annotator (cache_for) that allows to specify
> how much seconds the response should be cached by the browser.
> BaseResourceOperationAdapter.call() then sets the correct Cache-control
> HTTP header.
>
> Note that Cache-control is not supported by HTTP/1.0 clients:
> http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9

Thank you very much Andrea!

This branch is great and matches the implementation plan we discussed.

I have a few minor comments:

>=== modified file 'src/lazr/restful/declarations.py'

>
>+class cache_for(_method_annotator):
>+ """Decorator specifying how much a response may be cached by a
> client."""

s/how much/how long/

>+
>+ def __init__(self, duration):
>+ """Specify the duration, in seconds, of the resource."""

s/the/the caching/

>+ self.duration = duration

I think it would be a good idea to add error checking here. So that passing in
a strings gives an error message instead of generating invalid headers.

    try:
  self.duration = int(duration)
 except (TypeError, ValueError):
    raise TypeError("Caching duration should be a number of seconds: %s" %
duration)

(And please add a test for that behaviour.)

>+
>+ # For responses to GET requests, tell the client to cache the
> response

Missing a period at the end of the comment.

If you implement the above changes, I'll merge the branch on your behalf.

Again, thanks a lot!

 status approved
    review approve

--
Francis J. Lacoste
<email address hidden>

review: Approve

[Andrea Corbellini (andrea.corbellini)]

Thanks for your review.

On Thu, 2009-06-04 at 19:12 +0000, Francis J. Lacoste wrote:
> I think it would be a good idea to add error checking here. So that passing in
> a strings gives an error message instead of generating invalid headers.
>
> try:
> self.duration = int(duration)
> except (TypeError, ValueError):
> raise TypeError("Caching duration should be a number of seconds: %s" %
> duration)
>
> (And please add a test for that behaviour.)

I would prefer to do something like this:
  if not isinstance(duration, (int, long)): raise ...

I think that this is a better way to do this because it should be safer:
if, for mistake, the wrong object is passed to @cache_for and if it has
a __int__() method, it may pass the check, causing an incorrect behavior
instead of raising an exception.

[Francis J. Lacoste (flacoste)]

On Friday 05 June 2009, Andrea Corbellini wrote:
> Thanks for your review.
>
> On Thu, 2009-06-04 at 19:12 +0000, Francis J. Lacoste wrote:
> > I think it would be a good idea to add error checking here. So that
> > passing in a strings gives an error message instead of generating invalid
> > headers.
> >
> > try:
> > self.duration = int(duration)
> > except (TypeError, ValueError):
> > raise TypeError("Caching duration should be a number of seconds:
> > %s" % duration)
> >
> > (And please add a test for that behaviour.)
>
> I would prefer to do something like this:
> if not isinstance(duration, (int, long)): raise ...
>
> I think that this is a better way to do this because it should be safer:
> if, for mistake, the wrong object is passed to @cache_for and if it has
> a __int__() method, it may pass the check, causing an incorrect behavior
> instead of raising an exception.

Ok. Should we check that it's a positive integer?

--
Francis J. Lacoste
<email address hidden>

[lp:~andrea.corbellini/lazr.restful/fix-375353]

41. By Andrea Corbellini on 2009-06-05

Check the value given to @cache_for.

[Andrea Corbellini (andrea.corbellini)]

On Fri, 2009-06-05 at 13:15 +0000, Francis J. Lacoste wrote:
> Ok. Should we check that it's a positive integer?
Yes: numbers <= 0 should never be given.

I've added all the checks and fixed all the other problems. The branch
should be OK now. Do you need anything else?

[Francis J. Lacoste (flacoste)]

On Friday 05 June 2009, Andrea Corbellini wrote:
> On Fri, 2009-06-05 at 13:15 +0000, Francis J. Lacoste wrote:
> > Ok. Should we check that it's a positive integer?
>
> Yes: numbers <= 0 should never be given.
>
> I've added all the checks and fixed all the other problems. The branch
> should be OK now. Do you need anything else?

Nope, thanks, that was nice.

I merged it with the following small changes:

- Added cache_for to __all__
- Interpolate the value in the ValueError message when the value isn't
positive.

Thanks a lot for your contribution!

--
Francis J. Lacoste
<email address hidden>

=== modified file 'src/lazr/restful/declarations.py'
--- src/lazr/restful/declarations.py	2009-04-14 06:08:51 +0000
+++ src/lazr/restful/declarations.py	2009-05-29 15:32:33 +0000
@@ -591,6 +591,18 @@
         annotations['return_type'] = ObjectLink(schema=self.interface)
 
 
+class cache_for(_method_annotator):
+    """Decorator specifying how much a response may be cached by a client."""
+
+    def __init__(self, duration):
+        """Specify the duration, in seconds, of the resource."""
+        self.duration = duration
+
+    def annotate_method(self, method, annotations):
+        """See `_method_annotator`."""
+        annotations['cache_for'] = self.duration
+
+
 class export_read_operation(_export_operation):
     """Decorator marking a method for export as a read operation."""
     type = 'read_operation'
@@ -829,6 +841,14 @@
     def call(self, **kwargs):
         """See `ResourceOperation`."""
         params = self._getMethodParameters(kwargs)
+
+        # For responses to GET requests, tell the client to cache the response
+        if (IResourceGETOperation.providedBy(self)
+            and 'cache_for' in self._export_info):
+            self.request.response.setHeader(
+                'Cache-control', 'max-age=%i'
+                % self._export_info['cache_for'])
+
         result = getattr(self.context, self._method_name)(**params)
         return self.encodeResult(result)
 

=== modified file 'src/lazr/restful/docs/webservice-declarations.txt'
--- src/lazr/restful/docs/webservice-declarations.txt	2009-04-17 17:59:18 +0000
+++ src/lazr/restful/docs/webservice-declarations.txt	2009-05-29 15:39:08 +0000
@@ -1406,6 +1406,43 @@
     TypeError: A field can only have one mutator method; set_value_2
     makes two.
 
+Caching
+=======
+
+It is possible to cache a server response in the browser cache using
+the @cache_for decorator:
+
+    >>> from lazr.restful.declarations import cache_for
+    >>> from lazr.restful.testing.webservice import FakeRequest
+    >>>
+    >>> class ICachedBookSet(IBookSet):
+    ...     """IBookSet supporting caching."""
+    ...     export_as_webservice_collection(IBook)
+    ...
+    ...     @collection_default_content()
+    ...     @export_read_operation()
+    ...     @cache_for(60)
+    ...     def getAllBooks():
+    ...         """Return all books."""
+    ...
+    ...
+    >>> class CachedBookSet(BookSet):
+    ...     """Simple ICachedBookSet implementation."""
+    ...     implements(IBookSet)
+    ...
+    ...     def getAllBooks(self):
+    ...         return self.books
+    >>>
+    >>> request = FakeRequest()
+    >>> read_method_adapter_factory = generate_operation_adapter(
+    ...     ICachedBookSet['getAllBooks'])
+    >>> read_method_adapter = read_method_adapter_factory(
+    ...     CachedBookSet(['Cool book']), request)
+    >>> print read_method_adapter.call()
+    ["Cool book"]
+    >>> print request.response.headers
+    {'Content-Type': 'application/json', 'Cache-control': 'max-age=60'}
+
 Security
 ========