Repository: awslabs/aws-lambda-rust-runtime (language: rust)

Issue:
Could we make `raw_http_path` panic or return an `Option`?
It looks like, from looking at [this file](https://github.com/awslabs/aws-lambda-rust-runtime/blob/393d6447bea0502e1f939d197f4facc228e6e007/lambda-http/src/request.rs#L1-L1), that `RawHttpPath` is always inserted in the HTTP extensions bag except when using API Gateway websocket APIs (`into_websocket_request`).

* Why is that the case? Since [`aws_lambda_events::apigw::ApiGatewayWebsocketProxyRequest.path`](https://docs.rs/aws_lambda_events/latest/aws_lambda_events/apigw/struct.ApiGatewayWebsocketProxyRequest.html#structfield.path) exists.
    - If this is a mistake and `RawHttpPath` always needs to be inserted, can we make [`raw_http_path()`] panic?
* Can we make [`raw_http_path()`] return an `Option` instead of defaulting to an empty string if not present? This would allow the caller application to distinguish the case where the extension is not inserted from the case where raw http path is actually empty.
    - For motivation, see this [hard-to-debug issue in smithy-rs](https://github.com/awslabs/smithy-rs/issues/2676) where the smithy-rs framework relies on the raw HTTP path to route the request; if smithy-rs got `None` here, it could panic with an explanatory message indicating a misconfigured setup like the one in the issue, where two incompatible versions of `lambda_http` were being used in the application's closure.

[`raw_http_path()`]: https://docs.rs/lambda_http/0.8.0/lambda_http/ext/extensions/trait.RequestExt.html#tymethod.raw_http_path






Discussion / hints:
> * Can we make [`raw_http_path()`](https://docs.rs/lambda_http/0.8.0/lambda_http/ext/extensions/trait.RequestExt.html#tymethod.raw_http_path) return an `Option` instead of defaulting to an empty string if not present? This would allow the caller application to distinguish the case where the extension is not inserted from the case where raw http path is actually empty.

this sounds reasonable to me.
Do you know the answer to the first question? Why do API Gateway websocket APIs don't insert `RawHttpPath`?
I have no idea, sorry
It seems like it's a plain oversight, since [`aws_lambda_events::apigw::ApiGatewayWebsocketProxyRequest.path`](https://docs.rs/aws_lambda_events/latest/aws_lambda_events/apigw/struct.ApiGatewayWebsocketProxyRequest.html#structfield.path) exists. In which case I'd prefer we not do #652 and simply make `raw_http_path()` panic after we fix this, since we expect the extension to always be in the bag.
I'm not a big fan of APIs that panic, tbh. 

> after we fix this, since we expect the extension to always be in the bag.

Are you referring to other changes here, or in smithy-rs?
> Are you referring to other changes here, or in smithy-rs?

Here, in this library.

---

On second thought, other parties outside this library could pull out `RawHttpPath` from the extensions bag. I think we should document that that's the only case where this library could return `None`.

Fix patch:
diff --git a/lambda-http/src/request.rs b/lambda-http/src/request.rs
index fe9b5fb1..5ed3effe 100644
--- a/lambda-http/src/request.rs
+++ b/lambda-http/src/request.rs
@@ -277,7 +277,8 @@ fn into_websocket_request(ag: ApiGatewayWebsocketProxyRequest) -> http::Request<
         .get(http::header::HOST)
         .and_then(|s| s.to_str().ok())
         .or(ag.request_context.domain_name.as_deref());
-    let path = apigw_path_with_stage(&ag.request_context.stage, &ag.path.unwrap_or_default());
+    let raw_path = ag.path.unwrap_or_default();
+    let path = apigw_path_with_stage(&ag.request_context.stage, &raw_path);
 
     let builder = http::Request::builder()
         .uri(build_request_uri(
@@ -286,6 +287,7 @@ fn into_websocket_request(ag: ApiGatewayWebsocketProxyRequest) -> http::Request<
             host,
             Some((&ag.multi_value_query_string_parameters, &ag.query_string_parameters)),
         ))
+        .extension(RawHttpPath(raw_path))
         // multi-valued query string parameters are always a super
         // set of singly valued query string parameters,
         // when present, multi-valued query string parameters are preferred


Test patch: