Skip to content

Add new exceptions file for exceptions in the new code #1725

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

Merged
merged 11 commits into from
Jan 20, 2022

Conversation

MVrachev
Copy link
Collaborator

@MVrachev MVrachev commented Dec 13, 2021

Addresses @jku comment here: #1721 (comment)

Description of the changes being introduced by the pull request:

Add tuf/api/exceptions.py for exceptions in the new code.
I copied the exceptions from tuf/exceptions.py with a few important
decisions:

  1. I only added the exceptions that are used in the new code.
  2. I removed the general "Error" class as we can directly inherit
    Exceptions.
  3. I tried grouping the exceptions by relevance.
  4. I removed the second argument signable in UnsignedMetadataError as it was only
    kept for backward compatibility and is not used. (See 929b4b2 where it was added in tuf/exceptions.py)
  5. I tried following the new code style guidelines and linted the file
    with our linters.
  6. Made FetcherHTTPError a DownloadError
  7. Removed URLParsingError and replaced it with a DownloadError
  8. Removed ReplayedMetadataError and replaced it with BadVersionNumberError

Additionally, I stopped linting tuf/exceptions.py with mypy as we are going to use
tuf/api/exceptions.py for exceptions in the new code.

Signed-off-by: Martin Vrachev [email protected]

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@MVrachev
Copy link
Collaborator Author

The only question that is left is do we think that tuf/api/exceptions.py is the right place for that file as it will be used from tuf/ngclient as well?
One possibility is renaming the old exceptions file to exceptions.old for example and moving the new exception file there so its name will be tuf/exceptions.py.
What do you think @jku?

@coveralls
Copy link

coveralls commented Dec 13, 2021

Pull Request Test Coverage Report for Build 1693501800

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 38 of 38 (100.0%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.9%) to 98.609%

Totals Coverage Status
Change from base Build 1683264892: 0.9%
Covered Lines: 3945
Relevant Lines: 3972

💛 - Coveralls

@MVrachev MVrachev requested a review from jku December 14, 2021 07:53
@MVrachev
Copy link
Collaborator Author

I argument descriptions in the DownloadLengthMismatchError and SlowRetrievalError docstrings.

@jku
Copy link
Member

jku commented Dec 14, 2021

The only question that is left is do we think that tuf/api/exceptions.py is the right place for that file as it will be used from tuf/ngclient as well?
One possibility is renaming the old exceptions file to exceptions.old for example and moving the new exception file there so its name will be tuf/exceptions.py.

Let's not touch files that are part of the legacy code base.

I think the location of exceptions.py WRT ngclient is not an issue: ngclient already "depends" on tuf.api so exceptions being there is not a problem. There is an issue though (and you may also be thinking about this one): there are exceptions that are clearly client specific (at least FetcherHTTPError) and having those defined in non-client module is not great... but it's also not the worst thing in the world.

@@ -35,9 +35,9 @@ def fetch(self, url: str) -> Iterator[bytes]:
url: A URL string that represents a file location.

Raises:
tuf.exceptions.SlowRetrievalError: A timeout occurs while receiving
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this is more a generic comment)
I noticed that we refer to the full exception path; in other docstrings (including tuf/api/metadata), we use only the exception name. Not sure which should be the standard.

Copy link
Collaborator Author

@MVrachev MVrachev Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My opinion is that we shouldn't use the full exception path as it can change (as it happened now).
I think is best if we use the module name as I did in the mentioned example exceptions.SlowRetrievalError.
@jku what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally think including the "module path" at all is not very useful in docstrings: it just makes things harder to read. But if we do include them, then they should be complete

Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still want to have a quick look at all the errors (are they really something we want to keep) but I left some initial comments about removing a lot of the code

@MVrachev
Copy link
Collaborator Author

I addressed all comments by @jku and @kairoaraujo by:

  • removing the __str__ and __repr__ functions for a couple of exceptions
  • updating the DownloadLengthMismatchError exception message
  • I couldn't update the SlowRetrievalError exception message with information about average download speed as we don't have that information

@MVrachev MVrachev requested a review from jku December 14, 2021 12:22
@jku
Copy link
Member

jku commented Dec 16, 2021

Some thinking on the exceptions we have / should have :

  • FetcherHTTPError is currently not derived from DownloadError: it should be as we let it propagate from Updater. I think we want to document refresh, get_targetinfo and download_target as raises DownloadError
  • Same for URLParsingError -- except no-one will ever want to handle URLParsingError I think: Maybe we replace it with a generic DownloadError("Failed to parse URL")
  • UnsupportedAlgorithmError seems like it's not really a TUF problem... but could still be useful info for repository maintainers -- maybe we could re-export securesystemslib UnsupportedAlgorithmError instead (metadata API: avoid raising securesystemslib exceptions #1646) and then let the error pass through?

@MVrachev MVrachev force-pushed the new-exceptions.py branch 3 times, most recently from 6ab2bc3 to 921b99c Compare December 16, 2021 18:43
@MVrachev
Copy link
Collaborator Author

Some thinking on the exceptions we have / should have :

  • FetcherHTTPError is currently not derived from DownloadError: it should be as we let it propagate from Updater. I think we want to document refresh, get_targetinfo and download_target as raises DownloadError
  • Same for URLParsingError -- except no-one will ever want to handle URLParsingError I think: Maybe we replace it with a generic DownloadError("Failed to parse URL")
  • UnsupportedAlgorithmError seems like it's not really a TUF problem... but could still be useful info for repository maintainers -- maybe we could re-export securesystemslib UnsupportedAlgorithmError instead (metadata API: avoid raising securesystemslib exceptions #1646) and then let the error pass through?

After a conversation with @jku I agreed on both three points mentioned here and a few additional others.
The two points mentioned in his comment are part of the last three commits I added. The third point and a couple
of other enhancements will be in my pr about issue #1646.

Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe these are good changes overall, however we should make sure we do all of the exceptions API changes at roughly same time so it's not a constant churn: I am far less interested in the location of the exceptions file than how the exceptions are used in metadata API and ngclient. The remaining specific issues I know of are

It looks like we can do those separately (but the focus on this PR should be to verify that any changes are in line with these near future plans).

@MVrachev MVrachev force-pushed the new-exceptions.py branch 2 times, most recently from fe789ac to a956684 Compare December 22, 2021 18:22
@MVrachev
Copy link
Collaborator Author

MVrachev commented Dec 22, 2021

In a conversation with @lukpueh he mentioned that the name RepositoryError sounds a little odd when we consider we are throwing those exceptions in the client code as well.
I experimented with the name MetadataError instead of RepositoryError.
In my head, there were a couple of other names like MetadataValidationError or MetadataVerificationError which will explain better that all of these errors are related to problems during the validation/verification of a metadata file, but of course, they are longer.
@jku, @lukpueh what do you think?

@jku
Copy link
Member

jku commented Jan 7, 2022

RepositoryError sounds a little odd when we consider we are throwing those exceptions in the client code as well.
I experimented with the name MetadataError instead of RepositoryError.

Either one works for me: RepositoryError is not completely self explanatory but I don't see a specific problem with it: Updater uses the error to mean "this error happens because of metadata we got from repository: The issue cannot be resolved without new metadata from repository": the point is that this is not a client bug, it's a problem on the repository end. I'd personally avoid an API change just to change the name but I can live with change as well.

The other suggestions are really long.

@MVrachev MVrachev force-pushed the new-exceptions.py branch 2 times, most recently from a99b09d to a0171dc Compare January 12, 2022 13:36
@MVrachev
Copy link
Collaborator Author

I rebased on top of develop and added a0171dc. Those are the only real changes here.
I think there is a sense in keeping both commits Remove UnsupportedAlgorithmError and reexport it and Replace UnsupportedAlgorithmError with ValueError as it shows step by step the changes compared to tuf/exceptions.py.

@MVrachev
Copy link
Collaborator Author

I followed @jku advice and replaced UnsupportedAlgorithmError with ValueError.
@jku and @lukpueh you can have a look.

@MVrachev
Copy link
Collaborator Author

I added a couple of simplifications in my latest commits including making (De)Serialize errors a RepositoryErros.

On the question of the current exceptions:

RepositoryErrors

They allow us to test for those specific use cases without checking the exception messages.

UnsignedMetadataError is useful when we want to denote problems with the keys or signatures.

It’s used on multiple occasions to communicate different signing problems to the users such as Key failed to verify delegated role..., Delegated role was signed by x/y keys, No signature for key X found in the metadata and others.

Also, I think exceptions related to signatures and keys are useful to have their own exception as they show problems that need a more specific solution - resign, replace a key, etc.

Then we have the TrustedMetadataSet errors. We have three of them BadVersionNumberError, ExpiredMetadataError, and LengthOrHashMismatchError.

They are useful in order to test specific repository errors at the same time the user can just catch

BadVersionNumberError is needed to test when:

  • new metadata version is below the expected one
  • the new snapshot version is below the one requested in timestamp meta
  • any target version is below the one described in snaphshot meta.

ExpiredMetadataError is thrown when any of the metadata files has expired

LengthOrHashMismatchError is raised when:

  • a metadata length doesn’t match the expected length
  • metadata hashes don’t match the expected hashes

LengthOrHashMismatchError is caught twice in the ngclient:

  • when a local cached target is found, but cannot be verified and a new version of this target is needed
  • when a target is downloaded, but cannot be verified. In this case, it’s caught and reraised as RepositoryError, but I think there is a sense in keeping it as in the exception tree the user will be able to see that the problem is actually related to hashes and length verification.

On the question do we need additional exceptions?

Here are the possible new exceptions I see:

  • when the metadata types are not the ones we expect - maybe here it makes sense to use a new one
  • when information for a target is missing inside a new snapshot version compared to an older snapshot version- not sure we need a new one here
  • when updating a delegated target, but snapshot doesn’t contain information for it - not sure we need a new one here

DownloadErrors

DownloadLengthMismatchError it’s used as expected and I think it makes sense to keep it for tests.

SlowRetrievalError: I don’t have a strong opinion on this one. On one hand, I see it serves the purpose to unify all timeout exceptions into one on another hand I am asking how does that help us? Do we want to do that?

FetcherHTTPError:

I see that FetcherHTTPError is used almost everywhere for a variety of reasons:

  • hash mismatch with the downloaded target
  • unknown root version
  • unknown path

and others. Then, I see that DownloadError is not used anywhere.
Wondering is there a sense of keeping the general DownloadError and FetcherError at the same time? Can’t we replace the FectherError with DownloadError?

@lukpueh
Copy link
Member

lukpueh commented Jan 17, 2022

Thanks for the research and for sharing your detailed insights, @MVrachev!

I mostly agree with you:

  • Having two base errors RepositoryError for errors related to Metadata and target file verification, and DownloadError for communication errors, makes sense.

  • The specific RepositoryErrors make sense, although I don't like most of their names. They are both long but also inaccurate. Not sure if it's worth renaming (I suggested renames in the past and then took them back) but here are some suggestions:

    • UnsignedMetadataError suggests that there is no Signature, which is not what it is exclusively used for. What about a simple (Metadata)SignatureError?
    • BadVersionNumberError is probably okay, but I like (Metadata)VersionError better
    • LengthOrHashMismatchError seems both over-explicit, and also like it's withholding information, because the raiser does know if the length or the hash is the problem ... but maybe that detail is not relevant to the catcher? Would (Metadata)ContentError describe both case well? And/or should we split it into (Metadata)LengthError and (Metadata)HashError?
  • Regarding the new RepositoryErrors you suggest, how do we currently handle them? @ivanayov suggested that there were some tests where she regexed for error messages. That seems like a place to add new exception types. Regardless and without having looked at the code:

    • (Metadata)TypeError seems like a useful addition
    • I could see something like a MissingMetadataError for the other two scenarios you describe
  • Having only one of DownloadError and FetcherHTTPError seems to make sense from a user perspective. Maybe we need the separation internally, e.g. for internal exception handling or for testing?

  • DownloadLengthMismatchError again is a name I'm not particular fond of, but not sure if a rename is worth the trouble. Could it just be a LengthError?

  • SlowRetrievalError suggests that an attack is happing, but it could also just be a bad wire. What about a simple TimeoutError?

@MVrachev
Copy link
Collaborator Author

MVrachev commented Jan 17, 2022

  • The specific RepositoryErrors make sense, although I don't like most of their names. They are both long but also inaccurate. Not sure if it's worth renaming (I suggested renames in the past and then took them back) but here are some suggestions:

    • UnsignedMetadataError suggests that there is no Signature, which is not what it is exclusively used for. What about a simple (Metadata)SignatureError?

I agree with you. UnsignedMetadataError has been used in a variety of situations. I think SignatureError is a good suggestion.

  • BadVersionNumberError is probably okay, but I like (Metadata)VersionError better

Agree with you that the prefix bad is unnecessary given that this is an error, so of course, it's bad :D.
Again I am not sure that the Metadata prefix makes sense and VersionError sounds good.

  • LengthOrHashMismatchError seems both over-explicit, and also like it's withholding information, because the raiser does know if the length or the hash is the problem ... but maybe that detail is not relevant to the catcher? Would (Metadata)ContentError describe both case well? And/or should we split it into (Metadata)LengthError and (Metadata)HashError?

I think splitting them is better. Those are two different use cases and having two different errors probably could shorten our messages (it needs to be tried).

  • Regarding the new RepositoryErrors you suggest, how do we currently handle them? @ivanayov suggested that there were some tests where she regexed for error messages. That seems like a place to add new exception types. Regardless and without having looked at the code:
  • (Metadata)TypeError seems like a useful addition

Agree.

  • I could see something like a MissingMetadataError for the other two scenarios you describe

As the cases I described are more about missing information inside a metadata file it sounds to me it will be more appropriate to use MissingInformationError for example and in the message we can describe what information is exactly missing.

  • Having only one of DownloadError and FetcherHTTPError seems to make sense from a user perspective. Maybe we need the separation internally, e.g. for internal exception handling or for testing?

Can you emphasize more on that?

  • DownloadLengthMismatchError again is a name I'm not particular fond of, but not sure if a rename is worth the trouble. Could it just be a LengthError?

DownloadLengthMismatchError is used only once in the new code and we give good error messages as well:

image

so, I think it makes sense to rename it to LengthError.

  • SlowRetrievalError suggests that an attack is happing, but it could also just be a bad wire. What about a simple TimeoutError?

Good suggestion. All of the exceptions we are catching and rethrowing as SlowRetrievalError are TimeoutErrors.

@lukpueh
Copy link
Member

lukpueh commented Jan 17, 2022

Okay great, looks like we are pretty much on the same page. Let's have @jku, who's usually more cautious when it comes to renames, also weigh in. :)

  • Having only one of DownloadError and FetcherHTTPError seems to make sense from a user perspective. Maybe we need the separation internally, e.g. for internal exception handling or for testing?

Can you emphasize more on that?

Without looking at the client implementation, I'd say that DownloadError is broader than FetcherHTTPError, i.e. the former covers any error related to downloading, whereas the latter is something we raise (and maybe immediately handle) somewhere in the Fetcher class. A client user probably shouldn't care about fetcher errors, but our own code maybe needs it? Does that make sense? I should probably just look at the code and see for myself if that distinction is needed. Or we ask @jku once more. :P

@jku
Copy link
Member

jku commented Jan 18, 2022

  • I'm not keen on renaming exceptions unless they're really misleading but if you consider it important, go for it.

  • Adding new, so far unused, exceptions: I'm not against but can we handle these in separate issues? I think most of those won't be necessary for 1.0 (and will be trivial to add without API break)

  • FetcherHTTPError: We need the separate error internally so we can handle 403/404 when needed. Martin makes this error derive from DownloadError in this PR in order to make this easy for client to handle (since we do let other http errors propagate up to caller).

@lukpueh
Copy link
Member

lukpueh commented Jan 19, 2022

  • I'm not keen on renaming exceptions unless they're really misleading but if you consider it important, go for it.

Let's defer the decision and take one final look at the issue shortly before the release, when we have resolved everything else (also see #1745 (comment)) @MVrachev, can you ticketize?

  • Adding new, so far unused, exceptions: I'm not against but can we handle these in separate issues? I think most of those won't be necessary for 1.0 (and will be trivial to add without API break)

Agreed.

  • FetcherHTTPError: We need the separate error internally so we can handle 403/404 when needed. Martin makes this error derive from DownloadError in this PR in order to make this easy for client to handle (since we do let other http errors propagate up to caller).

Thanks for the insights.

Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a pretty good first iteration of cleaning up the exception taxonomy and suggest we merge as is.

But I'd like to pick up the discussion from this PR once more before 1.0. In particular I'd like to review wether:

  • generic errors (RepositoryError, DownloadError) that we raise should be more specific,
  • specific errors (BadVersionNumberError, LengthOrHashMismatchError`, etc.) really need to be that specific
  • built-in exceptions are used in an idiomatic fashion
  • exception names aren't misleading
  • all of above is consistent, i.e. there is no mix of generic and specific or custom and built-in exceptions, where there shouldn't be

Root.type,
new_root.signed.version,
self.root.signed.version,
raise exceptions.BadVersionNumberError(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General question for exceptions raised in methods in this module:
If none of them are documented in the Raises section of the method docstrings (they only mention the broad RepositoryError), do we expect anyone to handle them? If we do, we should probably document them. E.g. the BadVersionNumberError here, etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Point 2 of issue #1784 is about that.

Add tuf/api/exceptions.py for exceptions in the new code.
I copied the exceptions from tuf/exceptions.py with a few important
decisions:
1. I only added the exceptions that are used in the new code
2. I removed the general "Error" class as we can directly inherit
Exceptions
3. I tried grouping the exceptions by relevance
4. I removed the second argument "UnsignedMetadataError" as it's only
kept for backward compatibility and is not used
5. I tried following the new code style guidelines and linted the file
with our linters.

Signed-off-by: Martin Vrachev <[email protected]>
Stop linting tuf/exceptions.py with mypy as we are going to use
tuf/api/exceptions.py for exceptions in the new code.

Signed-off-by: Martin Vrachev <[email protected]>
Make FetcherHTTPError a DownloadError as the
error itself denotes an error happening during
the download process.

Signed-off-by: Martin Vrachev <[email protected]>
URLParsingError is a specific download error and
is not clear what benefit it provides.
It's used only once in the new code and the
message says everything you need to know about
the exception.

Signed-off-by: Martin Vrachev <[email protected]>
ReplayedMetadataError is a subset of
BadVersionNumberError and in a discussion with
Jussi we realized that ReplayedMetadataError can
be replaced by BadVersionNumberError with a
good message.

Signed-off-by: Martin Vrachev <[email protected]>
UnsupportedAlgorithmError is a detailed securesystemslib exception
and there is no need for TUF to redefine it.
Moreover which hash "algorithms" are allowed is work for
securesystemslib not for TUF.

It's only used once inside "Targetfile.from_data()" and there it's used
to denote that there is a problem with the given argument.
That's why this error can be just replaced with "ValueError".

Signed-off-by: Martin Vrachev <[email protected]>
LengthOrHashMismatchError is a thrown when there are problems with
metadata verification or problems from the repository side when looking
it from the user's perspective.

Signed-off-by: Martin Vrachev <[email protected]>
Signed-off-by: Martin Vrachev <[email protected]>
SerializationError and DeserializationError are both errors coming
from the repository side looking from the clients point of view.
That's why it makes sense to make them repository errors.

Signed-off-by: Martin Vrachev <[email protected]>
We should document that "DownloadError" is thrown inside
RequestsFetcher.fetch().

Signed-off-by: Martin Vrachev <[email protected]>
@MVrachev
Copy link
Collaborator Author

MVrachev commented Jan 19, 2022

Rebased on top of develop.

I think this is a pretty good first iteration of cleaning up the exception taxonomy and suggest we merge as is.

But I'd like to pick up the discussion from this PR once more before 1.0. In particular I'd like to review wether:

  • generic errors (RepositoryError, DownloadError) that we raise should be more specific,
  • specific errors (BadVersionNumberError, LengthOrHashMismatchError`, etc.) really need to be that specific

These two points can be discussed in #1784.

  • built-in exceptions are used in an idiomatic fashion
  • all of above is consistent, i.e. there is no mix of generic and specific or custom and built-in exceptions, where there shouldn't be

Created an issue about it: #1785.

  • exception names aren't misleading

Issue #1786 is about that.

@lukpueh lukpueh merged commit 1e4590b into theupdateframework:develop Jan 20, 2022
@MVrachev MVrachev deleted the new-exceptions.py branch January 20, 2022 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants