Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow error response customization #828

Merged
merged 2 commits into from
Feb 21, 2024
Merged

Allow error response customization #828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
10b65ae
Feat: custom error type support
kikuomax Feb 20, 2024
7347add
Chore: add example to comments on Diagnostic
kikuomax Feb 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 21 additions & 14 deletions lambda-runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@ use hyper::{body::Incoming, http::Request};
use lambda_runtime_api_client::{body::Body, BoxError, Client};
use serde::{Deserialize, Serialize};
use std::{
borrow::Cow,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use Cow? Why not just use &str?

Copy link
Contributor Author

@kikuomax kikuomax Mar 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evbo I introduced Cow to overcome a lifetime issue rather than performance concern. Since the fallback conversion from Error (Display) into Diagnostic had to dynamically format error_message, I was not able to simply return &str:

aws-lambda-rust-runtime/lambda-runtime/src/types.rs

Lines 67 to 77 in e0e95e6

impl<'a, T> From<T> for Diagnostic<'a>
where
T: Display,
{
fn from(value: T) -> Self {
Diagnostic {
error_type: Cow::Borrowed(std::any::type_name::<T>()),
error_message: Cow::Owned(format!("{value}")),
}
}
}

However, regarding error_type, it could have been &str. I did not notice that until you pointed it out.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error_type can't always be &str in the case where you're borrowing the variant name (such as with strum::AsRefStr), which is helpful for auto-converting Variants to error types.

I'm not sure why both can't just be String? Just seems like a lot of extra typing using Cow, sorry to nitpick

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some might say the opposite "why String? they are &'static str most of the time."

Btw, you may avoid explicit use of Cow as long as you are dealing with &str or String:

let msg = "message".to_string();
Diagnostic {
    error_type: "SomeError".into(), // &str → Cow::Borrowed
    error_message: msg.into(), // String → Cow::Owned
};

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the into - that's much cleaner. I hate to burn up so much time, but why would someone say: "why String"? If we're creating millions of exceptions, then I understand the cost of heap allocation would eventually cost noticeably, but maybe at most 1 exception gets generated once in a blue moon, so isn't it fair using String?

env,
fmt::{self, Debug, Display},
fmt::{self, Debug},
future::Future,
panic,
sync::Arc,
Expand All @@ -33,7 +34,9 @@ pub mod streaming;
mod types;

use requests::{EventCompletionRequest, EventErrorRequest, IntoRequest, NextEventRequest};
pub use types::{Context, FunctionResponse, IntoFunctionResponse, LambdaEvent, MetadataPrelude, StreamResponse};
pub use types::{
Context, Diagnostic, FunctionResponse, IntoFunctionResponse, LambdaEvent, MetadataPrelude, StreamResponse,
};

use types::invoke_request_id;

Expand Down Expand Up @@ -96,7 +99,7 @@ impl Runtime {
where
F: Service<LambdaEvent<A>>,
F::Future: Future<Output = Result<R, F::Error>>,
F::Error: fmt::Debug + fmt::Display,
F::Error: for<'a> Into<Diagnostic<'a>> + fmt::Debug,
A: for<'de> Deserialize<'de>,
R: IntoFunctionResponse<B, S>,
B: Serialize,
Expand Down Expand Up @@ -173,7 +176,14 @@ impl Runtime {
} else {
"Lambda panicked".to_string()
};
EventErrorRequest::new(request_id, error_type, &msg).into_req()
EventErrorRequest::new(
request_id,
Diagnostic {
error_type: Cow::Borrowed(error_type),
error_message: Cow::Owned(msg),
},
)
.into_req()
}
}
}
Expand Down Expand Up @@ -224,7 +234,7 @@ pub async fn run<A, F, R, B, S, D, E>(handler: F) -> Result<(), Error>
where
F: Service<LambdaEvent<A>>,
F::Future: Future<Output = Result<R, F::Error>>,
F::Error: fmt::Debug + fmt::Display,
F::Error: for<'a> Into<Diagnostic<'a>> + fmt::Debug,
A: for<'de> Deserialize<'de>,
R: IntoFunctionResponse<B, S>,
B: Serialize,
Expand All @@ -249,15 +259,12 @@ fn type_name_of_val<T>(_: T) -> &'static str {
std::any::type_name::<T>()
}

fn build_event_error_request<T>(request_id: &str, err: T) -> Result<Request<Body>, Error>
fn build_event_error_request<'a, T>(request_id: &'a str, err: T) -> Result<Request<Body>, Error>
where
T: Display + Debug,
T: Into<Diagnostic<'a>> + Debug,
{
error!("{:?}", err); // logs the error in CloudWatch
let error_type = type_name_of_val(&err);
let msg = format!("{err}");

EventErrorRequest::new(request_id, error_type, &msg).into_req()
EventErrorRequest::new(request_id, err).into_req()
}

#[cfg(test)]
Expand All @@ -274,7 +281,7 @@ mod endpoint_tests {
use httpmock::prelude::*;

use lambda_runtime_api_client::Client;
use std::{env, sync::Arc};
use std::{borrow::Cow, env, sync::Arc};
use tokio_stream::StreamExt;

#[tokio::test]
Expand Down Expand Up @@ -341,8 +348,8 @@ mod endpoint_tests {
#[tokio::test]
async fn test_error_response() -> Result<(), Error> {
let diagnostic = Diagnostic {
error_type: "InvalidEventDataError",
error_message: "Error parsing event data",
error_type: Cow::Borrowed("InvalidEventDataError"),
error_message: Cow::Borrowed("Error parsing event data"),
};
let body = serde_json::to_string(&diagnostic)?;

Expand Down
11 changes: 4 additions & 7 deletions lambda-runtime/src/requests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,13 +194,10 @@ pub(crate) struct EventErrorRequest<'a> {
}

impl<'a> EventErrorRequest<'a> {
pub(crate) fn new(request_id: &'a str, error_type: &'a str, error_message: &'a str) -> EventErrorRequest<'a> {
pub(crate) fn new(request_id: &'a str, diagnostic: impl Into<Diagnostic<'a>>) -> EventErrorRequest<'a> {
EventErrorRequest {
request_id,
diagnostic: Diagnostic {
error_type,
error_message,
},
diagnostic: diagnostic.into(),
}
}
}
Expand All @@ -226,8 +223,8 @@ fn test_event_error_request() {
let req = EventErrorRequest {
request_id: "id",
diagnostic: Diagnostic {
error_type: "InvalidEventDataError",
error_message: "Error parsing event data",
error_type: std::borrow::Cow::Borrowed("InvalidEventDataError"),
error_message: std::borrow::Cow::Borrowed("Error parsing event data"),
},
};
let req = req.into_req().unwrap();
Expand Down
68 changes: 62 additions & 6 deletions lambda-runtime/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,75 @@ use http::{header::ToStrError, HeaderMap, HeaderValue, StatusCode};
use lambda_runtime_api_client::body::Body;
use serde::{Deserialize, Serialize};
use std::{
borrow::Cow,
collections::HashMap,
env,
fmt::Debug,
fmt::{Debug, Display},
time::{Duration, SystemTime},
};
use tokio_stream::Stream;
use tracing::Span;

/// Diagnostic information about an error.
///
/// `Diagnostic` is automatically derived for types that implement
/// [`Display`][std::fmt::Display]; e.g., [`Error`][std::error::Error].
///
/// [`error_type`][`Diagnostic::error_type`] is derived from the type name of
/// the original error with [`std::any::type_name`] as a fallback, which may
/// not be reliable for conditional error handling.
/// You can define your own error container that implements `Into<Diagnostic>`
/// if you need to handle errors based on error types.
///
/// Example:
/// ```
/// use lambda_runtime::{Diagnostic, Error, LambdaEvent};
/// use std::borrow::Cow;
///
/// #[derive(Debug)]
/// struct ErrorResponse(Error);
///
/// impl<'a> Into<Diagnostic<'a>> for ErrorResponse {
/// fn into(self) -> Diagnostic<'a> {
/// Diagnostic {
/// error_type: Cow::Borrowed("MyError"),
/// error_message: Cow::Owned(self.0.to_string()),
/// }
/// }
/// }
///
/// async fn function_handler(_event: LambdaEvent<()>) -> Result<(), ErrorResponse> {
/// // ... do something
/// Ok(())
/// }
/// ```
#[derive(Debug, Eq, PartialEq, Clone, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
pub(crate) struct Diagnostic<'a> {
pub(crate) error_type: &'a str,
pub(crate) error_message: &'a str,
pub struct Diagnostic<'a> {
/// Error type.
///
/// `error_type` is derived from the type name of the original error with
/// [`std::any::type_name`] as a fallback.
/// Please implement your own `Into<Diagnostic>` if you need more reliable
/// error types.
pub error_type: Cow<'a, str>,
/// Error message.
///
/// `error_message` is the output from the [`Display`][std::fmt::Display]
/// implementation of the original error as a fallback.
pub error_message: Cow<'a, str>,
}

impl<'a, T> From<T> for Diagnostic<'a>
where
T: Display,
{
fn from(value: T) -> Self {
Diagnostic {
error_type: Cow::Borrowed(std::any::type_name::<T>()),
error_message: Cow::Owned(format!("{value}")),
}
}
}

/// Client context sent by the AWS Mobile SDK.
Expand Down Expand Up @@ -315,8 +371,8 @@ mod test {
});

let actual = Diagnostic {
error_type: "InvalidEventDataError",
error_message: "Error parsing event data.",
error_type: Cow::Borrowed("InvalidEventDataError"),
error_message: Cow::Borrowed("Error parsing event data."),
};
let actual: Value = serde_json::to_value(actual).expect("failed to serialize diagnostic");
assert_eq!(expected, actual);
Expand Down
Loading