Improve the design and extensibility of the hosting service HTTP support.
Review Request #1453 — Created Nov. 10, 2018 and updated
Information | |
---|---|
guest2316 | |
Review Board | |
55ef0ca... | |
Reviewers | |
demo | |
The hosting service HTTP support has always been little more than a wrapper around urllib2, with `HostingServiceClient` 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 to `HostingServiceHTTPRequest`, and no longer subclasses `urllib2.Request`. Instead, it keeps all the state it needs for the reuqest and then builds a `urllib2.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 a `json` property that will attempt to deserialize the payload contents as JSON, which replaces the need for the `json_*()` wrappers in `HostingServiceClient` (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 old `http_*()` and `json_*()` 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` through `build_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 the `http_request_cls` attribute). The request is then fed into `open_http_request()`, which is a thin wrapper around `HostingServiceHTTPRequest.open()` (mostly useful for unit testing, but subclasses can customize opening behavior as well). If successful, the result is processed in `process_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. Testing Done: 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. Reviewed at https://reviews.reviewboard.org/r/10212/