[Refactoring] Use a generic error type instead of the macro `error_impls` #1047

nepalez · 2023-07-21T15:14:50Z

While trying to recognize how the gem works I found this TODO and decided to implement it (well, this is how I learn things).

But at the moment the implementation got ready, I asked myself, why the same functionality cannot be expressed better with generic structs, and found no answer. That's why instead of implementing another macro (derive or attribute), I restarted my refactoring from scratch and removed the very necessity of the macro in favor of NatsError<T>.

This type represents any error to be raised by the crate. It is generic over the enum representing the kind of the error.

Unlike the previous code where binding between the kind and the error used to be provided by the macro error_impls, this definition makes the corresponding error bound to the kind more idiomatically.

The previous syntax:

#[derive(Clone, Debug, PartialEq)]
pub enum FooErrorKind {
  Bar,
  Baz,
}

#[derive(Debug)]
pub struct FooError {
  kind: FooErrorKind,
  source: Box<dyn Error + Send + Sync + 'static>,
}

error_impls!("FooError", "FooErrorKind");

The new definition:

#[derive(Clone, Debug, PartialEq)]
pub enum FooErrorKind {
  Bar,
  Baz,
}

pub type FooError = NatsError<FooErrorKind>;

In addition to being more expressive (which is, I agree, too opinionated), the new syntax lets IDE-s to understand the code better (most of them cannot analyze the code inside the macro).

Jarema · 2023-07-22T08:31:14Z

Hey!

That's a very nice approach and one of the options we were debating as an internals refactor.

Will review soon.
Thank you!

caspervonb · 2023-07-24T13:29:05Z

I'm for it (as previously discussed when you were working on the errors @Jarema 😁 )

nepalez · 2023-07-24T14:46:55Z

I've rebased the branch over the fresh main branch and also fixed formatting along with a typo in the error description.

Jarema

In general, this is a really nice change, but there are a few nits to be more Rust-idiomatic and consistent with the rest of library.

Thanks a lot for the work you've done here!

async-nats/src/nats_error.rs

Jarema · 2023-07-27T08:54:12Z

async-nats/src/nats_error.rs

+// Notice: see unit tests to see how to use this generic error class.
+
+/// The error type for the NATS client, generic by the kind of error.
+pub struct NatsError<Kind>


It's echoey (async_nats::nats_error::NatsError) and not very Rust-idomatic.

Make it Error. Same for the file name.

I'm not sure about this. The reason is nats already have the Error type which is a part of the public interface.

Maybe it is better to use something like CustomError<T> here to prevent echoing?

Alternatively, I could customize all errors like this as a types of NatsError, so that the crate would only provide the NatsError<T>, and then make 2 renamings:

crate::Error -> crate::SourceError (dynamic)

crate::NatsError -> crate::Error (static)

The crate::Error was added just for having simple alias for typical Rust Boxed error with intention to be used like in all doc examples. It became irrelevant after introducing concrete errors for whole API.

Then it was used for source, to have less verbose and consistent syntax at each source field. This became irrelevant too, as your improvement gets rid of repeated structs with source field.

Changing crate::Error name would be a breaking change without good reason. I would prefer to deprecate it.

Also, this new NatsError does not have to be public, as we make all aliases (type Foo<FooKind> public).
I was considering if it would "voldemort" the type, but it should be fine.

Oh, and yes, we need to add concrete errors to service API. We want the whole API of this crate to be based of concrete errors.

Tangentially, crate::Error should in the long term, be removed when nothing returns it anymore as explicitly saying what it is in say main makes for better documentation examples.

This type error type introduced here being public depends on how rustdoc renders it if it was voldemorted.

actually, it has to be public, otherwise we would get:

error[E0446]: crate-private type `nats_error::NatsError<jetstream::stream::ConsumerErrorKind>` in public interface --> async-nats/src/jetstream/stream.rs:1782:1 | 1782 | pub type ConsumerError = NatsError<ConsumerErrorKind>; | ^^^^^^^^^^^^^^^^^^^^^^ can't leak crate-private type | ::: async-nats/src/nats_error.rs:6:1 | 6 | pub(crate) struct NatsError<Kind> | --------------------------------- `nats_error::NatsError<jetstream::stream::ConsumerErrorKind>` declared as crate-private

But it does not have to be re-exported as crate::Error, but can remain crate::error::Error, while deprecating crate::Error.

WDYT @caspervonb ?

I think I need to work on this PR a bit more :)

Instead of defining ::new and ::with_source(kind: ..., source: ...) it's better to define impl<Kind> From<Kind> for NatsError<Kind>, to exclude repetition of the error kind:

Instead of

FooError::with_source(FooErrorKind::Baz, source)

we could get

FooErrorKind::Baz.into().with_source(source)

Then another improvement. Here and there there is a code like this, where some additional message is added to the source error. Wouldn't it be better if those kinds would be specialized instead and move these error messages into the implementation of the Display trait?

Then instead of

FooErrorKind::SharedProblem.into().with_source(format!("some custom problem: {}", err))

we would get

FooErrorKind::CustomProblem.into().with_source(err)

And finally the aforementioned switch to concrete errors everywhere in the package

wdyt?

To be fair, I would not overthink it.
Keep in mind it's an internal API, and often simple and explicit one is better than a smart one. Especially if you want to encourage contributions. The lower the library learning curve, the better :).

Regarding kinds - this was deliberate decision:
We expose error enum variants only if it makes sense for users to match against them, which is when user can take some action because of error kind returned.

In case of those errors you linked as example it does not make sense for the source kind to exist as separate kind, nor as a kind on returning error level. It's just giving contextual information when logging, which is really easy with those literal &str's.

Ahh, I missed your comment yesterday. Anyway, I don't use those new methods into() and with_kind() so they can be dropped without much sadness ))

async-nats/src/nats_error.rs

This type represents any error to be raised by the crate. It is generic over the enum representing the kind of the error. Unlike the previous code where binding between the kind and the error used to be provided by the macro `error_impls`, this definition makes the corresponding error bound to the kind more idiomatically through the generic type parameter. The previous syntax: #[derive(Clone, Debug, PartialEq)] pub enum FooErrorKind { Bar, Baz, } #[derive(Debug)] pub struct FooError { kind: FooErrorKind, source: Box<dyn Error + Send + Sync + 'static>, } error_impls!("FooError", "FooErrorKind"); The new definition: #[derive(Clone, Debug, PartialEq)] pub enum FooErrorKind { Bar, Baz, } pub type FooError = NatsError<FooErrorKind>; In addition to being more idiomatic, the new syntax lets IDE-s to understand the code better (most of them cannot analyze the code inside the macro).

This commit applies the parameterized NatsError to all errors previously used the `error_impls` macro.

After switching to the `NatsError<T>` this macro is not necessary any more.

nepalez · 2023-07-28T14:41:18Z

I have added 3 big changes into the PR

"normalized" errors so that the source is separated from the kind (previously JetStream(err) version of some kinds were keeping the source inside)
added the default implementation of Display right to the error, and in the code moved impl Display to the kinds (this is becasue after the previous step this became a common pattern)
(probably this should be rolled back to follow our previous discussion) implemented 2 constructors available out of the box to reflect the fact an error is defined by its kind:

FooErrorKind::SomeError.into() // instead of FooError::new(FooErrorKind::SomeError)
source.with_kind(FooErrorKind::SomeError) // instead of FooError::with_source(FooErrorKind::SomeError, source)

Jarema · 2023-07-28T15:04:12Z

I have added 3 big changes into the PR

"normalized" errors so that the source is separated from the kind (previously JetStream(err) version of some kinds were keeping the source inside)

That was done by purpose.

Users (and library itself) need a clean way to get to the underlying JetStream Error easily.
It does not look nice if user matches against kind, sees that it is JetStream Kind, yet they need to unwrap if source is Some and downcast error to get the JetStream Error. And the JetStream error is what in many cases user or we need to match against. It also adds possibility to introduce bugs by forgetting to add that source.

I know it's not perfect, but to be fair, JetStream errors on server side are not very clean, and new ones are added over time, so adding them directly to Kind would require all kinds to be non_exhaustive plus it's really hard to find all possible JS errors that can happen for given API call (to not call it impossible).

nepalez · 2023-07-28T15:26:33Z

@Jarema thank you for the explanation. Well, I need to think a bit more about it. Maybe it would block the generalization of the Display.

Another question is about your previous comment. As far, as I understood, you're not a big fan of making all errors wrapped and standardized in this way? To be honest, I'm not experienced enough for rust codebases, so I have no any opinion about what approach is preferable in Rust.

In Ruby world where I'm from, I always prefer the library to provide its own errors, even though it is just a thin wrapper around some third-party ones. This simplifies the debugging by pointing out to the specific package. Following this approach, I could potentially wrap all std::io::Error-s into the corresponding crate::Error, but should I?

From now and on it is enough to implement the Display trait for the kind of the error. Then the Display trait will be automatically implemented for the corresponding error. In case the error contains the source, its formatting will include both the kind and the source, otherwise only the kind will be used for the display. The method `formatted_source` is dropped.

nepalez · 2023-07-28T16:42:10Z

I've dropped the commit and returned to the JetStream(err). The implementation of the Display trait survived :)

Jarema · 2023-07-28T18:19:47Z

In Ruby world where I'm from, I always prefer the library to provide its own errors, even though it is just a thin wrapper around some third-party ones. This simplifies the debugging by pointing out to the specific package. Following this approach, I could potentially wrap all std::io::Error-s into the corresponding crate::Error, but should I?

It's the same in Rust - you usually do not want to expose the types of dependencies of your library.
However, JetStreamError IS a type from this crate :).

For learning context: There are exceptions though. One of few non-library types you can see exposed in the wild a lot is std::io::Error.

Jarema · 2023-07-28T20:39:00Z

@nepalez Looks good now! Rename the file and error and it's LGTM :).

nepalez · 2023-07-29T08:54:22Z

@Jarema done ))

caspervonb

Do we want this to be a ref instead of cloning? cc @Jarema

caspervonb · 2023-07-29T15:12:33Z

async-nats/src/error/mod.rs

+    }
+
+    // In some cases the kind doesn't implement `Copy` trait
+    pub fn kind(&self) -> Kind {


This can just return ref? no clone needed.

Suggested change

pub fn kind(&self) -> Kind {

pub fn kind(&self) -> &Kind {

I asked myself the same question, but left this method for the backward compatibitlity only. Without it we could just make the kind attribute public ))

Its Copy most of the time. Clone only in case or JetStream error. And if you look at std::io::Error its also returning copy.

Not only the JetStream, but also an ErrorResponse keeping a string inside, which is not copy.

Jarema

LGTM!

Jarema

Oh, that LGTM was a little premature.

Not sure for reason on implementing Display on Kinds, instead of Error?
That way, we loose quite a lot of useful context,

like here:
https://github.com/nats-io/nats.rs/pull/1047/files#diff-09d2b3ba6ced37b50fa193faa24082492564d93fffcf3068af4ee69a4be7e54dL884-R862

or here:
https://github.com/nats-io/nats.rs/pull/1047/files#diff-014b24d4d8bf5b62ffeed0269754925ee12069d078d371058be69986cb926ea8L963-L972

Plus, the default display rather hurts than helps - without it, if you add new error compiler will remind you that you need a nice display impl for it. Without it - it's very easy to skip it and have the very basic one, which is too basic in my opinion.

Sorry for all those change requests.
That's why we prefer iterative work with smaller PRs. Much easier to merge in changes that way :).

nepalez · 2023-07-29T16:35:51Z

we loose quite a lot of useful context,

I believe we don't because this context is still available in the error's Display. As long as the error has a source, its description is added to the error.

The only thing I am not sure about is this. For now all kinds require the Display to be implemented for them.

I could loose this requirement and let the developer to decide if he is going to imlement the Display for the kind or for the whole error only. Only if he implements the Display for the kind, the error would be displayed as well out of the box, otherwise it should be implemented for the specific error to satisfy its std::error::Error trait.

Jarema · 2023-07-29T17:10:30Z

we loose quite a lot of useful context,

I believe we don't because this context is still available in the error's Display. As long as the error has a source, its description is added to the error.

The only thing I am not sure about is this. For now all kinds require the Display to be implemented for them.

I could loose this requirement and let the developer to decide if he is going to imlement the Display for the kind or for the whole error only. Only if he implements the Display for the kind, the error would be displayed as well out of the box, otherwise it should be implemented for the specific error to satisfy its std::error::Error trait.

Ah, yes, you're right! The context is available, though printed as

failed publishing object chunks:: failed chunk publish: something

so all : at the end at the errors should go away. But rather in in-code impls, not in generic error impl.

Let's get it merged and discuss potential further improvements outside of this PR.

nepalez · 2023-07-29T17:34:32Z

Yeah, my bad. Fixed (removed excessive :)

Jarema

LGTM!

nepalez marked this pull request as ready for review July 21, 2023 15:15

caspervonb requested review from caspervonb and Jarema July 22, 2023 10:21

nepalez force-pushed the nats_error branch from 71669fb to 689f763 Compare July 24, 2023 14:45

nepalez force-pushed the nats_error branch from 689f763 to ad15424 Compare July 26, 2023 20:02

Jarema requested changes Jul 27, 2023

View reviewed changes

nepalez force-pushed the nats_error branch from ad15424 to 47fc5db Compare July 27, 2023 12:26

nepalez requested a review from Jarema July 27, 2023 12:28

nepalez force-pushed the nats_error branch from 47fc5db to 195bc4a Compare July 27, 2023 12:37

nepalez added 3 commits July 28, 2023 15:00

Use NatsError generic type instead of the macro

ee56990

This commit applies the parameterized NatsError to all errors previously used the `error_impls` macro.

Drop unused macro error_impls

523724e

After switching to the `NatsError<T>` this macro is not necessary any more.

nepalez force-pushed the nats_error branch from 195bc4a to 7dacb48 Compare July 28, 2023 14:39

nepalez force-pushed the nats_error branch from 7dacb48 to 4a48094 Compare July 28, 2023 16:40

Rename NatsError -> error::Error

1bf44fa

caspervonb reviewed Jul 29, 2023

View reviewed changes

Jarema approved these changes Jul 29, 2023

View reviewed changes

Jarema requested changes Jul 29, 2023

View reviewed changes

nepalez requested a review from Jarema July 29, 2023 16:37

Follow-up code review (fix typos)

5350fd0

Jarema approved these changes Jul 29, 2023

View reviewed changes

Jarema merged commit f20934d into nats-io:main Jul 29, 2023

This was referenced Aug 14, 2023

Convert AuthError to Error<AuthErrorKind> #1088

Open

Refactor ParseError #1089

Open

Jarema mentioned this pull request Sep 21, 2023

Release 0.32.0 #1118

Merged

nepalez deleted the nats_error branch September 22, 2023 12:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactoring] Use a generic error type instead of the macro `error_impls` #1047

[Refactoring] Use a generic error type instead of the macro `error_impls` #1047

nepalez commented Jul 21, 2023

Jarema commented Jul 22, 2023

caspervonb commented Jul 24, 2023

nepalez commented Jul 24, 2023

Jarema left a comment

Jarema Jul 27, 2023

nepalez Jul 27, 2023

nepalez Jul 27, 2023 •

edited

Loading

Jarema Jul 27, 2023

Jarema Jul 27, 2023

caspervonb Jul 27, 2023 •

edited

Loading

Jarema Jul 27, 2023

nepalez Jul 27, 2023

Jarema Jul 27, 2023

nepalez Jul 28, 2023

nepalez commented Jul 28, 2023 •

edited

Loading

Jarema commented Jul 28, 2023

nepalez commented Jul 28, 2023

nepalez commented Jul 28, 2023

Jarema commented Jul 28, 2023

Jarema commented Jul 28, 2023

nepalez commented Jul 29, 2023

caspervonb left a comment •

edited

Loading

caspervonb Jul 29, 2023

nepalez Jul 29, 2023

Jarema Jul 29, 2023

nepalez Jul 29, 2023 •

edited

Loading

Jarema left a comment

Jarema left a comment

nepalez commented Jul 29, 2023 •

edited

Loading

Jarema commented Jul 29, 2023

nepalez commented Jul 29, 2023

Jarema left a comment

[Refactoring] Use a generic error type instead of the macro error_impls #1047

[Refactoring] Use a generic error type instead of the macro error_impls #1047

Conversation

nepalez commented Jul 21, 2023

Jarema commented Jul 22, 2023

caspervonb commented Jul 24, 2023

nepalez commented Jul 24, 2023

Jarema left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nepalez Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caspervonb Jul 27, 2023 • edited Loading

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

nepalez commented Jul 28, 2023 • edited Loading

Jarema commented Jul 28, 2023

nepalez commented Jul 28, 2023

nepalez commented Jul 28, 2023

Jarema commented Jul 28, 2023

Jarema commented Jul 28, 2023

nepalez commented Jul 29, 2023

caspervonb left a comment • edited Loading

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

nepalez Jul 29, 2023 • edited Loading

Choose a reason for hiding this comment

Jarema left a comment

Choose a reason for hiding this comment

Jarema left a comment

Choose a reason for hiding this comment

nepalez commented Jul 29, 2023 • edited Loading

Jarema commented Jul 29, 2023

nepalez commented Jul 29, 2023

Jarema left a comment

Choose a reason for hiding this comment

[Refactoring] Use a generic error type instead of the macro `error_impls` #1047

[Refactoring] Use a generic error type instead of the macro `error_impls` #1047

nepalez Jul 27, 2023 •

edited

Loading

caspervonb Jul 27, 2023 •

edited

Loading

nepalez commented Jul 28, 2023 •

edited

Loading

caspervonb left a comment •

edited

Loading

nepalez Jul 29, 2023 •

edited

Loading

nepalez commented Jul 29, 2023 •

edited

Loading