• 
      

    Improve the design and extensibility of the hosting service HTTP support.

    Review Request #10212 — Created Oct. 9, 2018 and submitted

    Information

    Review Board
    release-4.0.x
    b5e50da...

    Reviewers

    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.

    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.

    brenniebrennie

    "something is overridden"

    brenniebrennie

    s/returned/requested/ ?

    brenniebrennie

    If these are internal APIs, shouldn't we prefix them with an underscore to make that clear? Overriding "private" methods should …

    brenniebrennie

    Do we want to do something like the following class URLRequest(HostingServiceHTTPRequest): def __init__(self, *args, **kwargs): warnings.warn("....", DeprecationWarning) super(...).__init__(...) or might …

    brenniebrennie
    brennie
    1. 
        
    2. reviewboard/hostingsvcs/service.py (Diff revision 1)
       
       
      Show all issues

      Need to update this line wrt the new class name.

    3. reviewboard/hostingsvcs/service.py (Diff revision 1)
       
       
       
      Show all issues

      "something is overridden"

    4. reviewboard/hostingsvcs/service.py (Diff revision 1)
       
       
      Show all issues

      s/returned/requested/ ?

    5. reviewboard/hostingsvcs/service.py (Diff revision 1)
       
       
      Show all issues

      If these are internal APIs, shouldn't we prefix them with an underscore to make that clear? Overriding "private" methods should be fine.

      1. It's okay to override these to customize behavior (adding enhanced logging around a request, or something), and unit tests reference them. Might change this later to be private, but for now I think it's okay if they're public.

    6. reviewboard/hostingsvcs/service.py (Diff revision 1)
       
       
      Show all issues

      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.

      1. While I'd prefer that, it won't serve the same purpose, since it's less about instantiating and more about referencing. Even with an older HostingService implementation, the new HostingServiceHTTPRequest is going to be used. URLRequest is rarely going to be created manually, if ever, but it could be used for, say, isinstance checks in unit tests. In practice, I don't think it'll ever really be used by third-parties, though.

    7. 
        
    chipx86
    david
    1. Wonderful cleanup!

    2. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-4.0.x (55ef0ca)