Improve API error responses and fix error-related crashes.

Review Request #12244 — Created April 20, 2022 and submitted

Information

RBTools
release-3.x

Reviewers

When attempting to debug an authentication issue, I encountered a case
where formatting an API response would crash. This was due to the
particular error case raising an AuthorizationError with
http_status=None, and string building expecting a valid integer.

In the process of fixing this, I decided to change how we handle API
error reporting, in order to reduce some corner cases that could occur
with less-than-helpful errors.

The following improvements have been made to the error message string
building:

  • If the API error code is available, the HTTP status code will not be
    shown (as codes like "Internal Server Error" confuse people).

  • If neither an API error code nor HTTP status code are available,
    the code details will not be shown (and will not cause a crash).

  • When showing either an API or HTTP status code, the name of the error
    or HTTP status is shown alongside the code, if known. This uses a
    built-in map of codes to names.

  • An explicit message is now always shown. In order of priority, this
    can be a message passed to the constructor, the message from the
    response payload, or a default message set on the APIError class or
    subclass.

There was some back-and-forth on my end with whether we should continue
to even show the API error code information, but it can aid in
debugging, so it's being left in. When present, the HTTP status code can
be inferred, hence its removal in this situation.

Unit tests now exist for all error classes and their string building.

Unit tests passed.

Triggered several API errors (including the one one that originally
crashed) and saw improved results.

Summary ID
Improve API error responses and fix error-related crashes.
When attempting to debug an authentication issue, I encountered a case where formatting an API response would crash. This was due to the particular error case raising an `AuthorizationError` with `http_status=None`, and string building expecting a valid integer. In the process of fixing this, I decided to change how we handle API error reporting, in order to reduce some corner cases that could occur with less-than-helpful errors. The following improvements have been made to the error message string building: * If the API error code is available, the HTTP status code will not be shown (as codes like "Internal Server Error" confuse people). * If neither an API error code nor HTTP status code are available, the code details will not be shown (and will not cause a crash). * When showing either an API or HTTP status code, the name of the error or HTTP status is shown alongside the code, if known. This uses a built-in map of codes to names. * An explicit message is now always shown. In order of priority, this can be a message passed to the constructor, the message from the response payload, or a default message set on the `APIError` class or subclass. There was some back-and-forth on my end with whether we should continue to even show the API error code information, but it can aid in debugging, so it's being left in. When present, the HTTP status code can be inferred, hence its removal in this situation. Unit tests now exist for all error classes and their string building.
9a110def60a65819209e2ac3523035959c93457a
Description From Last Updated

F811 redefinition of unused 'test_str_with_message' from line 149

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

chipx86
david
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-3.x (5d3b388)