-
Notifications
You must be signed in to change notification settings - Fork 272
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
ngclient: Make DownloadErrors more consistent #1810
Conversation
Pull Request Test Coverage Report for Build 1794078066
💛 - Coveralls |
linter failure is #1811 |
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.
Liked the changes overall.
I agree with the reasoning behind the rename of FetcherHTTPError
to DownloadHTTPError
.
One question and wanted to ask for a favor.
I just noticed I have forgotten to add an f
for f-string when fixing the DownloadError
here:
raise exceptions.DownloadError("Failed to parse URL {url}") |
tuf/ngclient/fetcher.py
Outdated
exceptions.DownloadError: An error occurred during download. | ||
exceptions.DownloadLengthMismatchError: downloaded bytes exceed | ||
'max_length'. | ||
exceptions.FetcherHTTPError: An HTTP error code was received. |
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.
Here as well as inside the download_bytes()
docstrings we document specificDownloadError
s - DownloadLengthMismatchError
and FetcherHTTPError
next to the more general DownloadError
.
Do we think that's okay?
@lukpueh what do you think as you have been working on #1784.
I see why we do that here as those errors could be relevant to the user and handled by him/her, but wanted to make sure we discuss this.
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.
yeah, there's no great solutions here, I just picked the one I dislike least:
- I don't want to create new sub-errors just for the sake of it
- in this case it makes sense to point out the specific errors and not just the generic one (because callers will be interested)
- but the problem you mention does make the docstring less clear than it could be
8260a72
to
2ca3be8
Compare
force-pushed, only changes are:
|
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.
LGTM
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.
I like this! Just added one minor coding style suggestion inline, feel free to ignore and merge as is.
@@ -73,8 +73,8 @@ def download(target: str) -> bool: | |||
path = updater.download_target(info) | |||
print(f"Target downloaded and available in {path}") | |||
|
|||
except (OSError, RepositoryError) as e: | |||
print(str(e)) | |||
except (OSError, RepositoryError, DownloadError) as e: |
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.
Do I understand correctly that any download related errors were just not caught here before, and that re-raising all of them as DownloadErrors allows us to easily handle them too here?
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.
they were handled, but only because requests errors all happen to be derived from OSError -- so this was relying on a fetcher implementation detail.
@@ -122,7 +121,7 @@ def _get_session(self, url: str) -> requests.Session: | |||
parsed_url = parse.urlparse(url) | |||
|
|||
if not parsed_url.scheme or not parsed_url.hostname: | |||
raise exceptions.DownloadError("Failed to parse URL {url}") | |||
raise exceptions.DownloadError(f"Failed to parse URL {url}") |
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 catching this, @MVrachev!
Fetcher interface should only raise DownloadErrors, regardless of the implementation. * Make sure fetch() wraps non-DownloadError errors in a DownloadError * Make the abstract function private _fetch() * Try to be more consistent in doscstrings This now makes the example client more sensible (when server does not respond): $ ./client_example.py download qwerty ... Failed to download target qwerty: Failed to download url http://127.0.0.1:8000/metadata/2.root.json (here the latter part of the error string comes from DownloadError raised by FetcherInterface.fetch()) Signed-off-by: Jussi Kukkonen <[email protected]>
I've not supported many renames but I'm suggesting this one: FetcherHTTPError was created because we needed to signal 403/404 from the fetcher to updater. At that time the download error hierarchy in general was not thought out. Now we have a couple of different errors all derived from DownloadError. I believe it does not make sense to point out "Fetcher" in one of their names: DownloadHTTPError makes it clearer this is a specific type of DownloadError. Signed-off-by: Jussi Kukkonen <[email protected]>
Make the dosctring match the similar argument in download_bytes() Signed-off-by: Jussi Kukkonen <[email protected]>
Signed-off-by: Jussi Kukkonen <[email protected]>
Signed-off-by: Jussi Kukkonen <[email protected]>
bb58178
to
e6f3632
Compare
rebased to get the build fix from develop, no changes. |
As a result calling any public fetcher methods is now guaranteed to only raise DownloadError (or something derived from it). I think this looks cleaner, makes it easier understand and implementing a fetcher is not more complex: Fetcher implementations are allowed to raise whatever errors (like RequestsError in the default case) but
FetcherInterface.fetch()
will automatically wrap errors so that only DownloadErrors are raised to calling code.This is strictly speaking an API change (because of the rename) but that error is meant for mostly internal use.
Fixes #1714