Generic exceptions vs Specific exceptions

[MVrachev]

Description of issue or feature request:
We want to use a shortlist of exceptions that will be thrown from our APIs, so that our users can handle them easily.
On another hand that doesn't stop us from creating other more specific exceptions that derive from those main exceptions.
This allows us to easily test that specific errors were thrown when we expected them.
For example, we are using BadVersionError and LengthOrHashMismatchError which derive from RepositoryError.

Still, there are two use cases that are worth discussing:

1. There are multiple places inside TrustedMetadataSet where in one API call we are throwing generic and specific exceptions.
   That's happening inside each of the functions update* inside TrustedMetadataSet we throw RepositoryError, ReplayedMetadataError, ExpiredMetadataError and UnsignedMetadataError (from verify_delegate()) at the same time.
   One idea could be to define new specific exceptions (which derive from RepositoryError and replace the instances of RepositoryError with them.

Read and synchronize with issues:

• Do we need a new MissingInformationError/MissingMetadataError? #1778
• Do we need a new (Metadata)TypeError? #1779
• ngclient: decide what to do with requests (Fetcher impl) errors #1714
• review/document client exceptions #1312

2. Inside the TrustedMetadataSet update* function docstrings we only document that we are throwing RepositoryError without specifying the specific exceptions. 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?

Inspired by #1725 (comment).

3. Inside _get_session in module tuf/ngclient/_internal/request_fetcher.py we currently throw URLParsingError:

   python-tuf/tuf/ngclient/_internal/requests_fetcher.py

   Line 135 in 0e26364

   raise exceptions.URLParsingError(

   
   In my commit e8f26eb inside pr Add new exceptions file for exceptions in the new code #1725 I am replacing it with DownloadError.
   The question is do we want to keep the specific exception - URLParsingError and make it derive from DownloadError?
   Does my change in this commit make sense?

[jku]

The question is do we want to keep the specific exception - URLParsingError and make it derive from DownloadError?
Does my change in this commit make sense?

I believe no-one is ever going to handle a URLParsingError so there should be no need for it. If that statement turns out wrong and a need arises later, adding a derived error is easy. The commit is good.

[jku]

Inside the TrustedMetadataSet update* function docstrings we only document that we are throwing RepositoryError without specifying the specific exceptions. 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?

Updater does use some of the specific exceptions. I would not object to TrustedMetadataSet having better exception documentation, the reason it doesn't is probably the fact that it's currently private (documenting too much does lead to outdated documentation...)

For updater itself I think documenting just RepositoryError (and not the more specific ones) is reasonable: Updater users are not going to care about the details.

[lukpueh]

I just did a thorough review of all exceptions we raise in api and ngclient and it all looks good modulo some missing documentation (will post related comments to #1785, where they seem more appropriate).

So I agree with what Jussi says about TrustedMetadataSet exceptions and I suggest we remove the following TODO comment?

python-tuf/tuf/ngclient/_internal/trusted_metadata_set.py

Lines 62 to 65 in cb7bd6a

	* exceptions are not final: the idea is that client could just handle
	a generic RepositoryError that covers every issue that server provided
	metadata could inflict (other errors would be user errors), but this is not
	yet the case

Let's do that and close this issue?

[jku]

Yes: PR #1805