Builtin vs custom exceptions

[MVrachev]

Description of issue or feature request:
Before releasing TUF 1.0.0 we should review our code and make sure that:

1. We are using built-in exceptions (ValueError, TypeError, KeyError, etc.) consistently and idiomatically.
2. We are using custom exceptions when it makes sense and not in places that could be replaced by a built-in exception.

Initiated from comment #1725 (review)

[lukpueh]

IMO @jku's taxonomy from #1312 (comment) makes a lot of sense

[lukpueh]

One thing to check here is whether any of the builtins (above all ValueError) raised in metadata classes __init__ methods propagates to a place, e.g. from_(dict, bytes, file, data), where we'd actually expect a RepositoryError?

[lukpueh]

I just did a thorough review of all exceptions we raise in api and ngclient and it all looks good modulo some minor issues, mostly missing documentation.

metadata.py

• built-ins raised in __init__ and from_dict methods and verify_delegate are not documented.
• KeyError and SerializationError raised in verify_signature are not documented.
• from_securesystemslib_key may raise a securesystemslib.FormatError if the passed key dict is invalid. This is not documented and not ideal. Maybe we should catch all and re-raise as ValueError? It's about invalid inputs after all.

requests_fetcher.py
The FetcherInterface claims that it raises SlowRetrievalError and FetcherHTTPError, which seem rather specific to the concrete RequestsFetcher implementation, especially the latter.
OTOH, the DownloadError raised for an invalid url argument, which is mandated by the interface, is not documented.
Should we maybe create a custom generic FetcherInterface exception that all exceptions raised in implementations of the interface must inherit (or just re-use DownloadError) and only mention that in the docstring?
Also, and this is slightly contradictory the what I just said, but wouldn't ValueError also make sense for the invalid url argument?

cc @jku, @MVrachev

[jku]

Big thanks for doing this.

Metadata:

• on undocumented builtin errors (ValueError in particular but KeyError as well) -- I don't object to documenting them but on the other hand we could just say in module docstring that "invalid input to any functions in API can result in ValueError/KeyError": I think this is fairly standard in python libraries (or rather not documenting this at all is normal).
• SerializationError in verify_signature does seem undocumented
• from_securesystemslib_key suggestion seems good

I'll have a closer look at Fetcher situation early next week.

[MVrachev]

tuf/api/metadata.py:

• I prefer if we document ValueError and KeyError as we are already documenting everywhere where we know they can be thrown. If we want, we can add the module docstring, but I don't feel that it replaces the need for exceptions documentation.
• For verify_signature: there are two options:

1. leave it the way it is or more precisely the SerializationErrors are reraised as UnsignedMetadataError with the message Failed to verify..., but it could be unclear why it failed.
2. move signed_serializer.serialize outside the try block and document SerializationError the same way we did here. I think I prefer this as it shows the problem could not be in the signature, but when serializing the metadata.

• for from_securesystemslib_key: I agree it's good to catch securesystemslib.exceptions.FormatError and reraise it as ValueError

Fetcher Interface:
I think I agree with you that a general FetcherError which all others will inherit sounds logical and we document SlowRetrievalError and FetcherHTTPError inside the request fetcher implementation. Also, I do agree FetcherHTTPError sounds a little too specific for an interface.

For invalid URL I think it's better to just use ValueError as it seems the pythonic way to raise that exception on bad arguments

[jku]

I have a suggestion for the downloading/fetcher error handling in #1810, please have a look. I did include one rename to make things more consistent... SlowRetrievalError is still left as is (since it's just not very important) but I won't complain if others want it renamed to e.g. DownloadTimeoutError

[jku]

We went through this with Martin and came back with this list:

• verify_signature() should handle SerializationError and throw UnsignedMetadata (this is what callers are interested in)
• verify_delegate() should document ValueError and TypeError
• from_securesystemslib_key should raise ValueError if securesystemslib.FormatError is caught
• All init() and from_dict() should document ValueError (and Keyerror if needed) -- the reason this was not done is that this sort of makes it look like programmer should handle these errors... but as long as we are clear in the docstring that these are invalid argument errors, this should be fine

We will file issues for these so there is something actionable. After these issues are filed I believe this meta-issue can be closed

[MVrachev]

We went through this with Martin and came back with this list:

• verify_signature() should handle SerializationError and throw UnsignedMetadata (this is what callers are interested in)
• verify_delegate() should document ValueError and TypeError
• from_securesystemslib_key should raise ValueError if securesystemslib.FormatError is caught
• All init() and from_dict() should document ValueError (and Keyerror if needed) -- the reason this was not done is that this sort of makes it look like programmer should handle these errors... but as long as we are clear in the docstring that these are invalid argument errors, this should be fine

We will file issues for these so there is something actionable. After these issues are filed I believe this meta-issue can be closed

There are issues for all of those:

• Metadata API: verify_signature() should handle SerializationError #1816
• Metadata API: verify_delegate() should document errors #1817
• Metadata API: from_securesystemslib_key() should handle sslib error #1818
• Document exceptions for __init__ and from_dict() methods in Metadata API #1819

We can close this issue as it's a little vague.