Page MenuHomeSoftware Heritage

RPCClient: check HTTP status code for errors also when streaming
ClosedPublic

Authored by zack on Nov 13 2019, 9:36 AM.

Details

Reviewers
olasd
douardda
Group Reviewers
Reviewers
Summary

To that end, refactor HTTP status checking logic so that it is reused in
various places.

As part of this we also strengthen the unpickling logic.

Diff Detail

Repository
rDCORE Foundations and core functionalities
Branch
bug/rpc-http-check-status
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9058
Build 13245: tox-on-jenkinsJenkins
Build 13244: arc lint + arc unit

Event Timeline

douardda added a subscriber: douardda.

This needs one or 2 tests.

This revision now requires changes to proceed.Nov 13 2019, 10:20 AM
swh/core/api/__init__.py
269

Not everyone may agree, but I would prefer a slightly simpler version of this method : we do not need most else statements, and since the method does inherently exit from everywhere, it makes more sense to *show* this pattern bu not using these else statements. Like:

def check_status(self, response) -> None:
    status_code = response.status_code
    status_class = response.status_code // 100
    if status_code == 404:
        raise RemoteException('404 not found')
    try:
        if status_class == 4:
            data = decode_response(response)
            raise pickle.loads(data)

        if status_class == 5:
            data = decode_response(response)
            if 'exception_pickled' in data:
                raise pickle.loads(data['exception_pickled'])
            else:
                raise RemoteException(data['exception'])

    except (TypeError, pickle.UnpicklingError):
        raise RemoteException(data)

    if status_class != 2:
        raise RemoteException(
            f'API HTTP error: {status_code} {response.content}')

TypeError and unpickling errors are a bit less focused, but I think it worth it.

swh/core/api/__init__.py
239

I think this should be called raise_for_status (this is the name of the equivalent method in requests.Response)

269

+1 to that. It'be nice to have the response object as an attribute to RemoteException so you can (programmatically) check it and whatnot.

swh/core/api/__init__.py
239

+1

  • RPCClent: rename and refactor check_status (now raise_for_status)
  • RPCClient: add response attribute to RemoteException
This revision is now accepted and ready to land.Nov 18 2019, 1:24 PM