Skip to content

Fix sam local support for missing headers (#38) #332

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

Merged
merged 1 commit into from
Jul 27, 2021
Merged

Fix sam local support for missing headers (#38) #332

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
105 changes: 92 additions & 13 deletions lambda-runtime/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::{Config, Error};
use http::HeaderMap;
use http::{HeaderMap, HeaderValue};
use serde::{Deserialize, Serialize};
use std::{collections::HashMap, convert::TryFrom};

Expand Down Expand Up @@ -120,28 +120,107 @@ impl TryFrom<HeaderMap> for Context {
type Error = Error;
fn try_from(headers: HeaderMap) -> Result<Self, Self::Error> {
let ctx = Context {
request_id: headers["lambda-runtime-aws-request-id"]
.to_str()
.expect("Missing Request ID")
request_id: headers
.get("lambda-runtime-aws-request-id")
Copy link
Contributor Author

@kenshih kenshih Jun 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

instead of implicitly failing (by panic-ing on a missing key), i retain the original panic behavior, but now we panic at the expect as expected :) -- I think in the original code we don't ever make it to the expect because it panics on the attempt to dereference headers["lambda-runtime-aws-request-id"]. However, I kept the panic language to refer to "lambda-runtime-aws-request-id", because I found the specificity was very helpful in tracking down the bug.

I chose not to change the panic to an Error here, since sam local does fulfill this requirement, so no need to be prematurely lax.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, thanks for fixing this.

.expect("missing lambda-runtime-aws-request-id header")
.to_str()?
.to_owned(),
deadline: headers["lambda-runtime-deadline-ms"]
deadline: headers
.get("lambda-runtime-deadline-ms")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

took same approach as "lambda-runtime-aws-request-id" here.

.expect("missing lambda-runtime-deadline-ms header")
.to_str()?
.parse::<u64>()?,
invoked_function_arn: headers
.get("lambda-runtime-invoked-function-arn")
.unwrap_or(&HeaderValue::from_static(
"No header lambda-runtime-invoked-function-arn found.",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of honoring ARN format, I only honored the "String" type of ARN. In production, of course, it should be a real ARN. But in development (such as sam local), it should be clear that something is wrong with the environment, if the developer somehow needs to inspect this context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good call. One of my original worries was if someone was using the ARN in some way it would be annoying to debug if the default looked like a real value.

))
.to_str()?
.parse()
.expect("Missing deadline"),
invoked_function_arn: headers["lambda-runtime-invoked-function-arn"]
.to_str()
.expect("Missing arn; this is a bug")
.to_owned(),
xray_trace_id: headers["lambda-runtime-trace-id"]
.to_str()
.expect("Invalid XRayTraceID sent by Lambda; this is a bug")
xray_trace_id: headers
.get("lambda-runtime-trace-id")
.unwrap_or(&HeaderValue::from_static("No header lambda-runtime-trace-id found."))
Copy link
Contributor Author

@kenshih kenshih Jun 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

As reported in other issues, as well as confirming this myself, sam local invoke also leaves off this header.

.to_str()?
.to_owned(),
..Default::default()
};
Ok(ctx)
}
}

#[cfg(test)]
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 wasn't sure exactly where to put these tests, so I just placed them right after the thing they are testing.

I noticed the other test in this file does not use #[cfg(test)]... I wasn't sure if I should have left it off here or not. As I understand, without adding #[cfg(test)], it would artificially bloat the release build as well. As I said, I'm still new enough to rust though, so please let me know if I misunderstand!

mod test {
use super::*;

#[test]
fn context_with_expected_values_and_types_resolves() {
let mut headers = HeaderMap::new();
headers.insert("lambda-runtime-aws-request-id", HeaderValue::from_static("my-id"));
headers.insert("lambda-runtime-deadline-ms", HeaderValue::from_static("123"));
headers.insert(
"lambda-runtime-invoked-function-arn",
HeaderValue::from_static("arn::myarn"),
);
headers.insert("lambda-runtime-trace-id", HeaderValue::from_static("arn::myarn"));
let tried = Context::try_from(headers);
assert!(tried.is_ok());
}

#[test]
fn context_with_certain_missing_headers_still_resolves() {
let mut headers = HeaderMap::new();
headers.insert("lambda-runtime-aws-request-id", HeaderValue::from_static("my-id"));
headers.insert("lambda-runtime-deadline-ms", HeaderValue::from_static("123"));
let tried = Context::try_from(headers);
assert!(tried.is_ok());
}

#[test]
fn context_with_bad_deadline_type_is_err() {
let mut headers = HeaderMap::new();
headers.insert("lambda-runtime-aws-request-id", HeaderValue::from_static("my-id"));
headers.insert(
"lambda-runtime-deadline-ms",
HeaderValue::from_static("BAD-Type,not <u64>"),
);
headers.insert(
"lambda-runtime-invoked-function-arn",
HeaderValue::from_static("arn::myarn"),
);
headers.insert("lambda-runtime-trace-id", HeaderValue::from_static("arn::myarn"));
let tried = Context::try_from(headers);
assert!(tried.is_err());
}

#[test]
#[should_panic]
#[allow(unused_must_use)]
fn context_with_missing_request_id_should_panic() {
let mut headers = HeaderMap::new();
headers.insert("lambda-runtime-aws-request-id", HeaderValue::from_static("my-id"));
headers.insert(
"lambda-runtime-invoked-function-arn",
HeaderValue::from_static("arn::myarn"),
);
headers.insert("lambda-runtime-trace-id", HeaderValue::from_static("arn::myarn"));
Context::try_from(headers);
}

#[test]
#[should_panic]
#[allow(unused_must_use)]
fn context_with_missing_deadline_should_panic() {
let mut headers = HeaderMap::new();
headers.insert("lambda-runtime-deadline-ms", HeaderValue::from_static("123"));
headers.insert(
"lambda-runtime-invoked-function-arn",
HeaderValue::from_static("arn::myarn"),
);
headers.insert("lambda-runtime-trace-id", HeaderValue::from_static("arn::myarn"));
Context::try_from(headers);
}
}

impl Context {
/// Add environment details to the context by setting `env_config`.
pub fn with_config(self, config: &Config) -> Self {
Expand Down