awslabs / aws-lambda-rust-runtime

A Rust runtime for AWS Lambda
Apache License 2.0
3.29k stars 335 forks source link

Relax blanket implementation of Diagnostic #897

Closed calavera closed 2 months ago

calavera commented 2 months ago

Issue #, if available:

Fixes #880

Description of changes:

Instead of implementing Diagnostic for everything that implements Display, implement the trait only for a few well known types. This gives people more flexibility to implement Diagnostic.

By submitting this pull request

bnusunny commented 2 months ago

@calavera is this ready for review?

calavera commented 2 months ago

@bnusunny yes. I've tested it manually with a bunch of the examples in the repo and it work fine. I've also tested it with some apps without any issue.

TethysSvensson commented 1 month ago

This is quite an intrusive change, as it means you can no longer use e.g. anyhow or eyre in your handlers, at least without doing additional boxing.

Would you be open to doing From<...> impls for those crates specifically, as I don't think they would be interesting in adding a dependency for lambda_runtime themselves.

TethysSvensson commented 1 month ago

Also, the requirement for<'b> Into<Diagnostic<'b>> is effectively the same as the requirement Into<Diagnostic<'static>> but less ergonomic.

For instance you will not be able to specify Diagnostic<'static>. This type does implement Into<Diagnostic<'static>>, but does not fulfill the requirement for<'b> Into<Diagnostic<'b>>.

TethysSvensson commented 1 month ago

For anyone else having a hard time upgrading to v0.12 because of this PR, here is my work-around:

struct DiagnosticWrapper(Diagnostic<'static>);

impl<'a> From<DiagnosticWrapper> for Diagnostic<'a> {
    fn from(value: DiagnosticWrapper) -> Self {
        value.0
    }
}

impl std::fmt::Debug for DiagnosticWrapper {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        self.0.fmt(f)
    }
}

impl DiagnosticWrapper {
    fn new<E>(err: E) -> Self
    where
        E: Display,
    {
        DiagnosticWrapper(Diagnostic {
            error_type: Cow::Borrowed(std::any::type_name::<E>()),
            error_message: Cow::Owned(err.to_string()),
        })
    }
}

I then use handler_future.map_err(DiagnosticWrapper::new) inside my service function.

calavera commented 1 month ago

I don't think adding dependencies ourselves for those crates makes sense either. I don't know what they return, but I'm guessing the implement the standard Error trait. If that's the case, it should not be necessary to do any conversion. Feel free to test it and send improvements.

TethysSvensson commented 1 month ago

@calavera I've created #901 to continue this discussion.