lazr.restfulclient

Merge lp:~james-w/lazr.restfulclient/fix-caching into lp:lazr.restfulclient

  1. fix-caching
  2. Merge into trunk
Proposed by James Westby
Status: Merged
Approved by: Robert Collins
Approved revision: 85
Merged at revision: 121
Proposed branch: lp:~james-w/lazr.restfulclient/fix-caching
Merge into: lp:lazr.restfulclient
Diff against target: 20 lines (+8/-1)
1 file modified
src/lazr/restfulclient/resource.py (+8/-1)
To merge this branch: bzr merge lp:~james-w/lazr.restfulclient/fix-caching
Reviewer Review Type Date Requested Status
Abel Deuring (community) code Approve
Review via email: mp+15635@code.launchpad.net

Commit message

Use the same HTTP methods httplib2 does.

To post a comment you must log in.
Revision history for this message
James Westby (james-w) wrote :

Investigating why I was seeing lots of repeated requests I looked in to
the caching. This showed that lplib was requesting "get" methods, and
because this wasn't literally "GET" httplib2 wasn't caching the responses.

I don't think this has ever worked, but only affected calling a NamedOperation,
so the cache isn't totally useless.

Thanks,

James

Revision history for this message
Abel Deuring (adeuring) wrote :

(17:45:06) adeuring: james_w: this looks like a really good bug fix! Just two formal nitpicks: (1) Could you move the last parameter in the changed call on a new line? we try to keep the length <= 78 characters. (2) I think most of your commit message is worth to appear as a comment for the changed method call so that readers know why http_method.upper() makes sense here?
(17:45:36) james_w: makes sense

review: Approve (code)
lp:~james-w/lazr.restfulclient/fix-caching updated
85. By James Westby

Add a comment explaining why the .upper() is there and fix formatting.

Thanks Abel.

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

I don't think this branch will help performance. I believe a named operation that returns an entry will not serve an ETag or respect a conditional GET, which means that a conditional GET will never succeed. And because Launchpad doesn't serve any cache directives, httplib2 will always try to get the freshest version of the resource, which means invoking the named operation again. Those are the two ways a client-side cache can help performance, and neither is applicable.

I could be wrong, and that's not a reason not to land this branch, because it does improve the code. But I would like to see a test (maybe in caching.txt) that shows what the effect of this branch is. Even if the only effect is that the outgoing HTTP request looks different.

Revision history for this message
James Westby (james-w) wrote :

Hi,

Could you give some clue as to how to set up the test service
to have a NamedOperation?

Thanks,

James

Revision history for this message
Francis J. Lacoste (flacoste) wrote :

Actually, after merging the branch and running the existing tests, it becomes obvious the effect of this change does as some tracing tests need to be updated for the upper casing.

I'm merging this branch now.

Revision history for this message
Francis J. Lacoste (flacoste) wrote :

I apologize that this stayed in limbo for nearly two years.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/lazr/restfulclient/resource.py'
2--- src/lazr/restfulclient/resource.py 2009-10-23 13:22:44 +0000
3+++ src/lazr/restfulclient/resource.py 2009-12-04 16:54:09 +0000
4@@ -471,8 +471,15 @@
5 in_representation) = self.wadl_method.build_representation(
6 **args)
7 extra_headers = { 'Content-type' : media_type }
8+ # Pass uppercase method names to httplib2, as that is what it works
9+ # with. If you pass a lowercase method name to httplib then it doesn't
10+ # consider it to be a GET, PUT, etc., and so will do things like not
11+ # cache. Wadl Methods return their method lower cased, which is how it
12+ # is compared in this method, but httplib2 expects the opposite, hence
13+ # the .upper() call.
14 response, content = self.root._browser._request(
15- url, in_representation, http_method, extra_headers=extra_headers)
16+ url, in_representation, http_method.upper(),
17+ extra_headers=extra_headers)
18
19 if response.status == 201:
20 return self._handle_201_response(url, response, content)

Subscribers

People subscribed via source and target branches