-
Notifications
You must be signed in to change notification settings - Fork 142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handling requests exceptions with friendly error messages #338
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #338 +/- ##
==========================================
- Coverage 69.51% 69.03% -0.48%
==========================================
Files 8 8
Lines 538 562 +24
==========================================
+ Hits 374 388 +14
- Misses 164 174 +10
Continue to review full report at Codecov.
|
Yeison, could you please terminate all lines with a period? For example: |
d853b0a
to
79c1120
Compare
Sure! |
79c1120
to
593b50a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good so far. Would you mind delivering the following changes?
try: | ||
r = requests.get(url=url, timeout=REQUEST_TIMEOUT, headers=headers, proxies=proxy) | ||
except requests.exceptions.ConnectionError: | ||
raise NetworkConnectionError() | ||
except requests.exceptions.Timeout: | ||
raise RequestTimeoutError() | ||
except requests.exceptions.RequestException: | ||
raise DatabaseFetchError() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thing you decided to add our own custom error classes here instead of just letting requests exceptions to slip through. Can you please pass the original exception as an inner
attribute here?
if 500 <= r.status_code < 600: | ||
raise ServerError() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you pass response reason
as a message to ServerError
, please?
elif r.status_code == 403: | ||
|
||
if r.status_code == 403: | ||
raise InvalidKeyError() | ||
elif r.status_code == 429: | ||
|
||
if r.status_code == 429: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these condition ranges doesn't look good. There are several codes that would not raise an error. Can you please review the ranges here including those beyond 599 or less than 200?
if r.status_code == 200: | ||
data = r.json() | ||
if cached: | ||
write_to_cache(db_name, data) | ||
return data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you modified the elif-statements below because of short-circuits, can we bring this to the end of the function without the if-statement? So, the "last case" would be the default, returning the data.
class NetworkConnectionError(DatabaseFetchError): | ||
pass | ||
|
||
|
||
class RequestTimeoutError(DatabaseFetchError): | ||
pass | ||
|
||
|
||
class ServerError(DatabaseFetchError): | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you catch all of them and just change the message, not to mention they have all the same class, give a custom message to these classes so you don't need to repeat when catching them.
except NetworkConnectionError: | ||
click.secho("Check your network connection, unable to reach the server", fg="red", file=sys.stderr) | ||
sys.exit(-1) | ||
except RequestTimeoutError: | ||
click.secho("Check your network connection, the request timed out.", fg="red", file=sys.stderr) | ||
sys.exit(-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we create a pattern for passing default messages to our error classes, you can just reuse the error message of the thrown exception here and maybe only catch the base class like DatabaseFetchError
.
@patch.object(requests, 'get', side_effect=requests.exceptions.ConnectionError) | ||
def test_check_fetch_database_url_connection_error(self, requests_mock): | ||
from safety.errors import NetworkConnectionError | ||
|
||
db_name = "insecure.json" | ||
mirror = 'https://safety.test' | ||
|
||
with self.assertRaises(NetworkConnectionError): | ||
safety.fetch_database_url(mirror, db_name=db_name, key="INVALID", cached=False, proxy={}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tests. 👏
Codecov provides some line-by-line comments as part of our PRs about test coverage. Please make sure we are watching them and respecting as much as possible.
This will add friendly error messages for the users of safety.
This covers requests exceptions: ConnectionError, Timeout and generic RequestException, also there is a handling for Bad HTTP codes with a custom message.
a few screenshots about how it looks: