Improve the design and extensibility of the hosting service HTTP support.
Review Request #10212 — Created Oct. 9, 2018 and submitted
The hosting service HTTP support has always been little more than a
wrapper around urllib2, withHostingServiceClient
a loose wrapper
around that. Over time, we bolted on new specializations, adding things
like JSON versions of the HTTP methods to the client, working around the
design to allow for Digest Auth and better error reporting in Gerrit,
and trusting that string types were being passed around correctly.This change takes us a couple steps forward toward a better design
that's less directly dependent on urllib2 (though it still does require
it, and it would take more work to remove it fully).
URLRequest
has been renamed toHostingServiceHTTPRequest
, and no
longer subclassesurllib2.Request
. Instead, it keeps all the state it
needs for the reuqest and then builds aurllib2.Request
when
performing the request, turning the result into a
HostingServiceHTTPResponse
.The response object contains the response URL, payload contents,
headers, and HTTP status code. It also contains ajson
property that
will attempt to deserialize the payload contents as JSON, which replaces
the need for thejson_*()
wrappers inHostingServiceClient
(all of
which are now deprecated).The response object can be treated like a tuple, returning the response
data and headers, in order to emulate the behavior of the oldhttp_*()
andjson_*()
methods. This behavior is considered deprecated.It also type-checks the payload data and headers, logging and raising
exceptions if they don't contain byte strings. This is designed with
Python 3 compatibility in mind (though this still needs real testing, as
more of the codebase is made compatible). The intent is to help catch
issues in unit tests, which it already has.
HostingServiceClient
itself has new methods to better construct
requests and process responses.First off, clients can now turn on/off support for HTTP Basic Auth and
Digest Auth through flags on the client, which means we can now remove a
lot of logic from Gerrit.
HostingServiceClient.http_request
now builds a
HostingServiceHTTPRequest
throughbuild_http_request()
. This passes
the request arguments to an instance of the class and then adds auth
headers, based on the above flags. Subclasses can override the building
behavior if they need anything custom (and can specify a subclass of the
request class through thehttp_request_cls
attribute).The request is then fed into
open_http_request()
, which is a thin
wrapper aroundHostingServiceHTTPRequest.open()
(mostly useful for
unit testing, but subclasses can customize opening behavior as well).
If successful, the result is processed inprocess_http_response()
,
which by default will just return the response (subclasses can add
additional logic here, such as rate limiting header checks). If the
request is not successful, the error is passed to
process_http_error()
, where it can be handled specially by the hosting
service.Except for specialized services (like Gerrit, which had to be updated in
this change) or in unit tests where types were wrong, this is
backwards-compatible with most existing code. Future changes will
update more hosting services to benefit more from these changes,
simplifying logic.
Unit tests pass on Python 2.7 (and 3.x along with upcoming changes).
Tested some standard operations on a few repositories. Didn't see any
breakages.
Description | From | Last Updated |
---|---|---|
Need to update this line wrt the new class name. |
brennie | |
"something is overridden" |
brennie | |
s/returned/requested/ ? |
brennie | |
If these are internal APIs, shouldn't we prefix them with an underscore to make that clear? Overriding "private" methods should … |
brennie | |
Do we want to do something like the following class URLRequest(HostingServiceHTTPRequest): def __init__(self, *args, **kwargs): warnings.warn("....", DeprecationWarning) super(...).__init__(...) or might … |
brennie |
-
-
-
-
-
If these are internal APIs, shouldn't we prefix them with an underscore to make that clear? Overriding "private" methods should be fine.
-
Do we want to do something like the following
class URLRequest(HostingServiceHTTPRequest): def __init__(self, *args, **kwargs): warnings.warn("....", DeprecationWarning) super(...).__init__(...)
or might that get too annoying? I am worried that 3rd parties will complain that the deprecation wasn't visible enough with just a doc commen when this is eventually removed.