Metadata API: Avoid raising securesystemslib exceptions #1734

MVrachev · 2021-12-17T16:53:12Z

Description of the changes being introduced by the pull request:

In this pr, I aim to remove the need of our users to import securesystemslib
in order to catch the exceptions thrown by tuf/metadata/api.py.
I either reexport securesystemslib exceptions or
reraise them by using tuf exceptions.

Reexporting securesystemslib exceptions is required so that users
of code that throws these exceptions doesn't have to import
securesystemslib to handle them.

A couple of additional fixes are added to this pr as well.

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

coveralls · 2021-12-17T16:58:46Z

Pull Request Test Coverage Report for Build 1722983367

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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
5 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.9%) to 98.612%

Files with Coverage Reduction	New Missed Lines	%
tuf/api/metadata.py	5	98.13%

Totals
Change from base Build 1722589044:	0.9%
Covered Lines:	3955
Relevant Lines:	3982

💛 - Coveralls

tuf/api/metadata.py

jku · 2022-01-07T12:36:05Z

I think we can remove the last SSLIB error as well #1725 (comment)

opinions?

joshuagl · 2022-01-07T12:59:48Z

I think we can remove the last SSLIB error as well #1725 (comment)

opinions?

👍
Removing all sslib exceptions from the python-tuf public API seems like a good goal.

MVrachev · 2022-01-12T14:22:06Z

@jku and @lukpueh you can have a look into my list commit where I actually get rid of all securesystemslibs exceptions and there is no longer need for reexporting.

lukpueh · 2022-01-13T11:27:56Z

@jku and @lukpueh you can have a look into my list commit where I actually get rid of all securesystemslibs exceptions and there is no longer need for reexporting.

Is this ready for review? Or should I wait until we land #1725?

MVrachev · 2022-01-13T12:25:23Z

You can review the last three commits.
Even though 8403a89 and 9090da2 should be squashed.

MVrachev · 2022-01-20T10:58:10Z

@jku @lukpueh this pr is ready for review after the merge of #1725.

tests/test_api.py

jku · 2022-01-25T10:21:02Z

I think there's a misunderstanding. My comments on securesystemslib.StorageError vs OSError were made in the context of the client: there a securesystemslib error just makes life harder for no good reason. In Metadata API there was a conscious decision to abstract storage (for the repository use case). Removing StorageError completely makes no sense here: we might want to expose StorageError in our exceptions though so that users don't need to import SSLIB explicitly.

I agree about the other SSLIB errors.

MVrachev · 2022-01-26T18:21:54Z

@jku the pr is updated.
You are correct we do want to keep the StorageError exceptions as we do provide an option to use the storage_backend argument and we want to be able to throw exceptions a StorageError which generalizes exceptions from different storage interfaces.

jku

I agree with this. Would appreciate other maintainer opinions (@lukpueh ?) on my suggestion to re-export StorageError: I feel like that's a good strategy but maybe a little unusual?

lukpueh

Regarding StorageError, I think re-exporting makes sense. See inline for more comments...

lukpueh · 2022-01-27T14:18:36Z

tuf/api/metadata.py

@@ -305,9 +304,7 @@ def sign(
        Raises:
            tuf.api.serialization.SerializationError:
                'signed' cannot be serialized.


This is not raised anymore. Can you move the serialize call out of the try block?

My comment from above is nonsense. What I wanted to say is, if we except all below when calling sign(serialize...) and re-raise as UnsignedMetadataError, then SerializationError won't be raised anymore.

So either we take the call to serialize out of the try block below or we update the docstring

I decided to move the serialization outside the try block so that the serialization error will propagate as I don't think it's good to hide it with UnsignedMetadataError exception.

wow I completely missed this. Good solution I think.

tuf/api/metadata.py

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

Reexport securesystemslib StorageError, so that our users can catch it without importing securesystemslib. The securesystemslib StorageError makes sense in the context of metadata API, because it supports different storage interfaces and this exception is denoting all possible errors that could arrise from using any kind of storage interface. Additionally, I changed the places where we mention that StorageError is thrown, so that our users will know they can directly import it from tuf/api/exceptions.py instead of importing securesystemslib. Signed-off-by: Martin Vrachev <[email protected]>

Catch Metadata.sign() securesystemslib exceptions and instead throw a more general UnsignedMetadataError exception. We don't want to expose securesystemslib exceptions and it's better to replace them with a more general exception that could be easily handled. As the signer is an argument implementing securesystemslib.signer.Signer interface we don't know what exception will it throw. That's why we need to catch all possible exceptions during signing and raise UnsignedMetadataError. That is the same reason why we should move the serialization outside the "try" block, so a tuf.api.serialization.SerializationError can propagate and warn the user that 'signed' cannot be serialized. Signed-off-by: Martin Vrachev <[email protected]>

Add missing tests testing raising documented exceptions for "Metadata.sign()", "Metadata.to_file()" and "Metadata.from_file()". Signed-off-by: Martin Vrachev <[email protected]>

MVrachev changed the title ~~Securesystemslib exceptions~~ Metadata API: Avoid raising securesystemslib exceptions Dec 17, 2021

MVrachev mentioned this pull request Dec 17, 2021

What to do with tuf/exceptions before 1.0.0? #1729

Closed

jku reviewed Dec 21, 2021

View reviewed changes

tuf/api/metadata.py Outdated Show resolved Hide resolved

MVrachev force-pushed the securesystemslib-exceptions branch from 2130319 to fd541d8 Compare December 22, 2021 17:51

MVrachev force-pushed the securesystemslib-exceptions branch from fd541d8 to 195fb0d Compare January 4, 2022 15:36

MVrachev force-pushed the securesystemslib-exceptions branch from 195fb0d to 9090da2 Compare January 12, 2022 14:21

MVrachev force-pushed the securesystemslib-exceptions branch from 9090da2 to 1301807 Compare January 20, 2022 10:56

MVrachev marked this pull request as ready for review January 20, 2022 10:58

jku reviewed Jan 25, 2022

View reviewed changes

tests/test_api.py Outdated Show resolved Hide resolved

MVrachev force-pushed the securesystemslib-exceptions branch from 1301807 to 08ba94a Compare January 26, 2022 18:19

MVrachev requested a review from jku January 27, 2022 12:35

jku approved these changes Jan 27, 2022

View reviewed changes

lukpueh reviewed Jan 27, 2022

View reviewed changes

MVrachev added 4 commits January 27, 2022 17:05

Fix type annotation in test_api

0666520

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

Metadata API: add exception tests

9533c3f

Add missing tests testing raising documented exceptions for "Metadata.sign()", "Metadata.to_file()" and "Metadata.from_file()". Signed-off-by: Martin Vrachev <[email protected]>

MVrachev force-pushed the securesystemslib-exceptions branch from 08ba94a to 9533c3f Compare January 27, 2022 15:35

MVrachev requested a review from lukpueh January 27, 2022 15:36

lukpueh approved these changes Jan 27, 2022

View reviewed changes

lukpueh merged commit cb7bd6a into theupdateframework:develop Jan 27, 2022

MVrachev deleted the securesystemslib-exceptions branch January 27, 2022 16:07

Metadata API: Avoid raising securesystemslib exceptions #1734

Metadata API: Avoid raising securesystemslib exceptions #1734

Uh oh!

Conversation

MVrachev commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1722983367

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

Uh oh!

jku commented Jan 7, 2022

Uh oh!

joshuagl commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MVrachev commented Jan 12, 2022

Uh oh!

lukpueh commented Jan 13, 2022

Uh oh!

MVrachev commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MVrachev commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jku commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MVrachev commented Jan 26, 2022

Uh oh!

jku left a comment

Choose a reason for hiding this comment

Uh oh!

lukpueh left a comment

Choose a reason for hiding this comment

Uh oh!

lukpueh Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

lukpueh Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

MVrachev Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

jku Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MVrachev commented Dec 17, 2021 •

edited

Loading

coveralls commented Dec 17, 2021 •

edited

Loading

joshuagl commented Jan 7, 2022 •

edited

Loading

MVrachev commented Jan 13, 2022 •

edited

Loading

MVrachev commented Jan 20, 2022 •

edited

Loading

jku commented Jan 25, 2022 •

edited

Loading