lazr.restfulclient

Merge lp://staging/~james-w/lazr.restfulclient/reject-non-json-responses into lp://staging/lazr.restfulclient

  1. reject-non-json-responses
  2. Merge into trunk
Proposed by James Westby
Status: Rejected
Rejected by: Francis J. Lacoste
Proposed branch: lp://staging/~james-w/lazr.restfulclient/reject-non-json-responses
Merge into: lp://staging/lazr.restfulclient
Diff against target: 88 lines (+38/-9)
2 files modified
src/lazr/restfulclient/errors.py (+15/-0)
src/lazr/restfulclient/resource.py (+23/-9)
To merge this branch: bzr merge lp://staging/~james-w/lazr.restfulclient/reject-non-json-responses
Reviewer Review Type Date Requested Status
Francis J. Lacoste (community) Needs Resubmitting
Graham Binns (community) code Approve
Review via email: mp+16960@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
James Westby (james-w) wrote :

bug shows what happens if you get an unexpected content-type
from the server. This change is an attempt to at least give it
a clearer error message and make it easier to debug.

It firstly uses the content-type returned by the response to
interpret it, rather than hardcoding application/json. It's
conceivable that wadllib knows how to deal with such a content-type,
so it's better to let it find the error.

The second change catches wadllib's error and throws an error
that explains the situation, so you would get something like

   https://api.launchpad.net/beta/ubuntu did not return a usable
   content type: This resource doesn't define a representation for
   text/html.

with a .content attribute you can print as usual to find out what
you got instead.

I would appreciate some guidance in how to write tests for this
change.

Thanks,

James

Revision history for this message
Graham Binns (gmb) wrote :

Hi James,

Thanks for this branch. Some of the formatting seemed a bit odd to me, so I've run up a patch to address it: http://pastebin.ubuntu.com/558072/.

review: Approve (code)
Revision history for this message
Graham Binns (gmb) wrote :

(Er, that should have ended with "otherwise, this looks fine.")

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

Again, I apologize for the limbo around this branch.

This branch now conflits. It appears that the underlying implementation of _ensure_representation changed in what is probably a worse direction. It now always decode the representation as JSON.

Even after twisting the branch to match the new implementation, I'm getting tests error because the WrongContentException is thrown in case where it wasn't expected. It might be problem in the tests, but since the underlying bug that prompted this fix in the first place has been resolved on the LP side, this change is less needed.

review: Needs Resubmitting

Unmerged revisions

87. By James Westby

Give a more specific error that will help with debugging.

86. By James Westby

Use the content-type returned by the response to interpret it.

Using a hardcoded application/json value means that it tries to interpret
any response as JSON, even if it is HTML say. While this doesn't matter for
normal operation with LP it does mean that some errors are masked.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/lazr/restfulclient/errors.py'
2--- src/lazr/restfulclient/errors.py 2009-10-21 12:01:41 +0000
3+++ src/lazr/restfulclient/errors.py 2010-01-07 11:51:25 +0000
4@@ -68,3 +68,18 @@
5 "Response headers:\n---\n%s\n---\n"
6 "Response body:\n---\n%s\n---\n") % (
7 self.response.status, self.response.reason, headers, self.content)
8+
9+
10+class WrongContentTypeError(ResponseError):
11+ """An HTTP non-2xx response code was received."""
12+
13+ def __init__(self, response, content, url, extra):
14+ ResponseError.__init__(self)
15+ self.response = response
16+ self.content = content
17+ self.url = url
18+ self.extra = extra
19+
20+ def __str__(self):
21+ return ("%s did not return a usable content type: %s"
22+ % (self.url, self.extra))
23
24=== modified file 'src/lazr/restfulclient/resource.py'
25--- src/lazr/restfulclient/resource.py 2009-12-17 15:03:38 +0000
26+++ src/lazr/restfulclient/resource.py 2010-01-07 11:51:25 +0000
27@@ -36,10 +36,11 @@
28 from urlparse import urlparse
29
30 from lazr.uri import URI
31-from wadllib.application import Resource as WadlResource
32+from wadllib.application import (Resource as WadlResource,
33+ UnsupportedMediaTypeError)
34 from _browser import Browser, RestfulHttp
35 from _json import DatetimeJSONEncoder
36-from errors import HTTPError
37+from errors import HTTPError, WrongContentTypeError
38
39 missing = object()
40
41@@ -283,8 +284,8 @@
42 if etag is not None:
43 headers['If-None-Match'] = etag
44 try:
45- representation = self._root._browser.get(
46- self._wadl_resource, headers=headers)
47+ response, representation = self._root._browser.get(
48+ self._wadl_resource, headers=headers, return_response=True)
49 except HTTPError, e:
50 if e.response['status'] == '304':
51 # The entry wasn't modified. No need to do anything.
52@@ -293,8 +294,14 @@
53 raise e
54 # __setattr__ assumes we're setting an attribute of the resource,
55 # so we manipulate __dict__ directly.
56- self.__dict__['_wadl_resource'] = self._wadl_resource.bind(
57- representation, self.JSON_MEDIA_TYPE)
58+ try:
59+ self.__dict__['_wadl_resource'] = self._wadl_resource.bind(
60+ representation, response['content-type'])
61+ except UnsupportedMediaTypeError, e:
62+ method = self._wadl_resource.get_method('get')
63+ url = method.build_request_url()
64+ raise WrongContentTypeError(response, representation, url,
65+ str(e))
66
67 def __getattr__(self, attr):
68 """Try to retrive a named operation or parameter of the given name."""
69@@ -329,9 +336,16 @@
70 """Make sure this resource has a representation fetched."""
71 if self._wadl_resource.representation is None:
72 # Get a representation of the linked resource.
73- representation = self._root._browser.get(self._wadl_resource)
74- self.__dict__['_wadl_resource'] = self._wadl_resource.bind(
75- representation, self.JSON_MEDIA_TYPE)
76+ response, representation = self._root._browser.get(
77+ self._wadl_resource, return_response=True)
78+ try:
79+ self.__dict__['_wadl_resource'] = self._wadl_resource.bind(
80+ representation, response['content-type'])
81+ except UnsupportedMediaTypeError, e:
82+ method = self._wadl_resource.get_method('get')
83+ url = method.build_request_url()
84+ raise WrongContentTypeError(response, representation, url,
85+ str(e))
86
87 def __ne__(self, other):
88 """Inequality operator."""

Subscribers

People subscribed via source and target branches