Fix check bom http error reporting - #237
Conversation
The initial tests used HTTP 500 errors which would lead to multiple urllib3 retries and long timeouts, so use 403 (forbidden) instead. Also fix some responses so we don't run into unrelated errors and remove two unneeded responses.
The current code tries to assure HTTP response objects are valid before trying to print them, but due to the HTTP requests API, error responses are considered False so they were not printed. Thanks to Raul Fuentes for reporting. Fixes #227
0a9ea0b to
e36d96e
Compare
Same as in 543bd2d, requests.Response objects are considered "False" for HTTP errors, so we must not check for True. And there's no need for the other else paths as the first "if" already tells us that we have swex.response so there can't be another error case.
e36d96e to
f08d637
Compare
|
Claude Opus identified a comparable problem in create_project.py which should now also be fixed. |
| if swex.response: | ||
| print_red(" " + str(swex.response.status_code) + ": " + swex.response.text) | ||
| sys.exit(ResultCode.RESULT_ERROR_ACCESSING_SW360) | ||
| if swex.details: |
There was a problem hiding this comment.
Why has this code been removed?
response.text is one possible problem description.
SW360s real description is in swex.details["error"] and swex.details["message"]
There was a problem hiding this comment.
This is the current control flow from https://github.com/sw360/capycli/blob/v2.11.1/capycli/project/create_project.py#L139:
if swex.response is None: # 1
...
sys.exit(...)
if swex.response.status_code == requests.codes["unauthorized"]: # 2
...
sys.exit(...)
if swex.response.status_code == requests.codes["forbidden"]: # 3
...
sys.exit(...)
if swex.response: # 4
...
sys.exit(...)
if swex.details: # 5
...
sys.exit(...)
print_red(" Unknown error ...) # 6
sys.exit(...)
Now as in the other error described by Raul, # 4 will never be triggered as .response will evaluate to False for any HTTP error. So we would need to replace # 4 by if swex.response is not None which is what I did first. But then# 4 is the exact opposite of # 1, so mypy shouted at me that neither # 5 nor # 6 are reachable.
Or in other words: # 1 is already the "general error" part which is called when we have no valid .response, so we don't need # 6.
And looking into https://github.com/sw360/sw360python/blob/master/sw360/sw360error.py, swex.details is only populated when we have a valid .response, so # 4 will already cover that case, so we also don't need # 5.
Or do I overlook something?
There was a problem hiding this comment.
The patch is also somewhat confusing, but what I actually did was removing # 5 and # 6 and making the code from # 4 the fall-back if other if's don't mach.
As reported by @rfuentess in #227, the current HTTP response evaluation in "bom check" was wrong leading to incomplete error output on e.g. HTTP 403:
This PR fixes it as suggested by Raul so errors are printed as intended:
While SW360 usually doesn't answer with HTTP 403 (to my knowledge), we should still be prepared for it - and the fix will also help reporting a HTTP token which gets invalid while CaPyCli runs (resulting in HTTP 401) which is probably what Raul observed.