Use thiserror for error handling #156

MikailBag · 2020-06-14T13:03:35Z

Motivation

error-chain generates Error type which is not Sync, so it is hard to use with many other libraries.
Libraries are encouraged to provide structured error details instead of just formatted message.

Changes

errors::Error type is now plain enum, using thiserror for Display and From impls.
It is very large, but I don't think it is significant drawback.
Some functions return custom Error type (which is more convenient to use), but such types can be converted to errors::Error.
Unfortunately new approach does not play well with backtraces. I think this can be fixed if needed.
In several places I replaced ? with expect because there should not be any errors in fact.

lucab · 2020-06-19T14:23:46Z

@steveej I think you were looking at refreshing error handling in other places too, do you maybe have some cycles to review and land this one? I'm +1 on thiserror here.

steveej · 2020-06-19T15:56:06Z

I think you were looking at refreshing error handling in other places too, do you maybe have some cycles to review and land this one? I'm +1 on thiserror here.

Yes, I'll be happy to take a look at this. I migrated away from failure on openshift/cincinnati recently. That PR was less invasive than this one, although I do understand the motivation of adding more specific and structured errors.

From glancing at this it looks like it'll be in conflict with #159, so I'd prefer to land that first.

lucab · 2020-06-19T15:59:26Z

@steveej ack, let's serialize these. I started going through #159 but didn't manage to finish, I'll hope to finish that on Monday.

MikailBag · 2020-06-19T17:49:41Z

Thanks for positive feedback!
I will be happy to rebase onto master and fix merge conflicts if needed.

steveej · 2020-07-09T13:47:04Z

@MikailBag #159 has landed. I'll review this PR when it's rebased!

MikailBag · 2020-07-10T15:55:15Z

I have rebased onto master; tests pass.

steveej

Thanks again for this work!

While this change is very invasive I think it's worth to have structured error types. Can you explain the effect on consumers of the public API?

Also, I'm leaning towards having all errors in the crate's error module instead of having module specific errors. Do you have a convincing reason for not doing that except the comfort you mentioned in the first comment?

steveej · 2020-07-15T08:10:00Z

src/mediatypes.rs

@@ -79,6 +79,6 @@ impl MediaTypes {
                }
            }
        }
-        .map_err(|e| Error::from(e.to_string()))
+        .expect("to_mime should be always successful")


I don't follow why this cannot fail. I'd prefer to leave the Result in place here, and let the caller make use of the ? operator which isn't expensive.

src/v2/auth.rs

steveej · 2020-07-15T08:21:27Z

src/mediatypes.rs

        }
    }
-    pub fn to_mime(&self) -> Result<mime::Mime> {
+    pub fn to_mime(&self) -> mime::Mime {


Suggested change

pub fn to_mime(&self) -> mime::Mime {

pub fn to_mime(&self) -> Result<mime::Mime> {

Please see the comment below

steveej · 2020-07-15T08:23:39Z

src/v2/content_digest.rs

 /// Implements types and methods for content verification
 use sha2::{self, Digest};

 /// ContentDigest stores a digest and its DigestAlgorithm
 #[derive(Clone, Debug, PartialEq)]
-pub(crate) struct ContentDigest {
+pub struct ContentDigest {


Does this need to be pub?

I changed visibility to pub because I included it into ContestDigestError::Verify variant details.
I think this can be useful for library user.
However, I see alternative option: we can include format!-ed digests into error instead. this way we can keep ContestDigest private.

steveej · 2020-07-15T08:23:43Z

src/v2/content_digest.rs

    digest: String,
    algorithm: DigestAlgorithm,
 }

 /// DigestAlgorithm declares the supported algorithms
 #[derive(Display, Clone, Debug, PartialEq, EnumString)]
-enum DigestAlgorithm {
+pub enum DigestAlgorithm {


Does this need to be pub?

steveej · 2020-07-15T08:26:56Z

src/v2/content_digest.rs

 impl ContentDigest {
    /// try_new attempts to parse the digest string and create a ContentDigest instance from it
    ///
    /// Success depends on
    /// - the string having a "algorithm:" prefix
    /// - the algorithm being supported by DigestAlgorithm
-    pub fn try_new(digest: String) -> Result<Self> {
+    pub fn try_new(digest: String) -> std::result::Result<Self, ContentDigestError> {


What's the advantage of declaring this specific error type in this module vs. adding the variants to the crate's error module?

If one wishes to use specifically ContestDigest, they can exhaustively match on ContestDigestError and handle all possible errors. If this function returns crate::Error instead, such a user would have to

add wildcard check

write justyfing comment

carefully read through docs of dkregistry::Error (or source code) and decide for each variant if it is possible.
It is time-consuming process.

In short: IMHO, if some function returns E as error enum, and it contains variants that are never returned by this function, it is inconvenient for API user who would like to do exhaustive match.

That said, I'm ready to merge this domain-specific small enums into crate::Error, if you find that behavior more maintainable or readable,

Thanks for elaborating, the reasoning makes sense to me! It would be nice if Rust allowed a convenient way to specify a subset of variants the of the error enum a function returns, but I can't come up with an easy way to do that. In this case I prefer the specific error type as implemented in this PR.

steveej

I think this is good to merge after the rebase 👍

MikailBag · 2020-08-07T12:34:03Z

@steveej rebased.

steveej

I spotted two unresolved comment threads and bumped them. I strongly prefer not to panic if we can avoid it.

src/v2/auth.rs

MikailBag · 2020-08-28T12:43:36Z

Correct me if I'm wrong, but now I can see that the two mentioned lines are in tests (sorry that I have not noticed it earlier). Usually it is OK for test code to panic.
I can change expect to ? in that code, but I'd like to get some confirmation from you.

steveej

Correct me if I'm wrong, but now I can see that the two mentioned lines are in tests (sorry that I have not noticed it earlier). Usually it is OK for test code to panic.

I overlooked the fact that it's in test as well. I agree that we can stick with it there.

Thank you for your work on this!

src/v2/auth.rs

steveej self-requested a review June 19, 2020 15:30

MikailBag force-pushed the thiserror branch 2 times, most recently from b98b9a9 to c88a974 Compare July 10, 2020 15:45

steveej reviewed Jul 15, 2020

View reviewed changes

steveej reviewed Jul 28, 2020

View reviewed changes

MikailBag force-pushed the thiserror branch from c88a974 to 267c2f8 Compare August 7, 2020 12:19

Use thiserror for error handling

1775482

MikailBag force-pushed the thiserror branch from 267c2f8 to 1775482 Compare August 7, 2020 12:27

steveej suggested changes Aug 28, 2020

View reviewed changes

src/v2/auth.rs Show resolved Hide resolved

src/v2/auth.rs Show resolved Hide resolved

steveej approved these changes Aug 31, 2020

View reviewed changes

src/v2/auth.rs Show resolved Hide resolved

steveej merged commit 95eb5a3 into camallo:master Aug 31, 2020

MikailBag deleted the thiserror branch September 1, 2020 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use thiserror for error handling #156

Use thiserror for error handling #156

MikailBag commented Jun 14, 2020 •

edited

Loading

lucab commented Jun 19, 2020

steveej commented Jun 19, 2020

lucab commented Jun 19, 2020

MikailBag commented Jun 19, 2020

steveej commented Jul 9, 2020

MikailBag commented Jul 10, 2020

steveej left a comment

steveej Jul 15, 2020

steveej Jul 15, 2020

steveej Jul 15, 2020

MikailBag Jul 15, 2020

steveej Jul 15, 2020

steveej Jul 15, 2020

MikailBag Jul 15, 2020

steveej Jul 28, 2020

steveej left a comment

MikailBag commented Aug 7, 2020

steveej left a comment

MikailBag commented Aug 28, 2020

steveej left a comment

	pub fn to_mime(&self) -> mime::Mime {
	pub fn to_mime(&self) -> Result<mime::Mime> {

Use thiserror for error handling #156

Use thiserror for error handling #156

Conversation

MikailBag commented Jun 14, 2020 • edited Loading

Motivation

Changes

lucab commented Jun 19, 2020

steveej commented Jun 19, 2020

lucab commented Jun 19, 2020

MikailBag commented Jun 19, 2020

steveej commented Jul 9, 2020

MikailBag commented Jul 10, 2020

steveej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveej left a comment

Choose a reason for hiding this comment

MikailBag commented Aug 7, 2020

steveej left a comment

Choose a reason for hiding this comment

MikailBag commented Aug 28, 2020

steveej left a comment

Choose a reason for hiding this comment

MikailBag commented Jun 14, 2020 •

edited

Loading