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.

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

lambda_http: Make extension methods available for http::request::Parts #607

Merged
merged 1 commit into from
Apr 10, 2023
Merged

lambda_http: Make extension methods available for http::request::Parts #607

merged 1 commit into from
Apr 10, 2023

Conversation

dcormier
Copy link
Contributor

@dcormier dcormier commented Feb 20, 2023

Closes #606.

Adds extension methods to http::request::Parts and http::Extensions, which allows the methods to be used after calling Request::into_parts() in addition to on the request itself. Closes #606, which has more detailed info.

By submitting this pull request

  • I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • I confirm that I've made a best effort attempt to update all relevant documentation.

Copy link
Contributor

@calavera calavera left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution. It looks like there are some breaking changes. I think they need some more consideration.

lambda-http/src/ext/extensions.rs Outdated Show resolved Hide resolved
lambda-http/src/ext/extensions.rs Show resolved Hide resolved
lambda-http/src/ext/extensions.rs Outdated Show resolved Hide resolved
@dcormier
Copy link
Contributor Author

Yes, there are quite a few breaking changes. More than you've commented on. I've detailed them in #606. I just want to make sure you're aware.

Copy link
Contributor

@calavera calavera left a comment

Choose a reason for hiding this comment

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

These two breaking changes need to be reverted too:

fn request_context(&self) -> RequestContext becomes
fn request_context(&self) -> Option<&RequestContext>
fn lambda_context(&self) -> Context becomes
fn lambda_context(&self) -> Option<&Context>

@dcormier
Copy link
Contributor Author

Done.

Copy link
Contributor

@calavera calavera left a comment

Choose a reason for hiding this comment

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

I just noticed that the module's name also changed, from RequestExt to ExtensionsExt. Please revert that, we don't want to make such major breaking changes without good reasons. That can erode customer trust.

@dcormier
Copy link
Contributor Author

dcormier commented Apr 10, 2023

It's been broken into two traits. ExtensionsExt and RequestExt. The goal of this PR being to expose the additional methods even if the Request has been broken into parts.

pub trait ExtensionsExt {
    fn raw_http_path(&self) -> &str;
    fn with_raw_http_path<S>(self, path: S) -> Self
    where
        S: Into<String>;

    fn query_string_parameters(&self) -> QueryMap;
    fn query_string_parameters_ref(&self) -> Option<&QueryMap>;
    fn with_query_string_parameters<Q>(self, parameters: Q) -> Self
    where
        Q: Into<QueryMap>;

    fn path_parameters(&self) -> QueryMap;
    fn path_parameters_ref(&self) -> Option<&QueryMap>;
    fn with_path_parameters<P>(self, parameters: P) -> Self
    where
        P: Into<QueryMap>;

    fn stage_variables(&self) -> QueryMap;
    fn stage_variables_ref(&self) -> Option<&QueryMap>;
    #[cfg(test)]
    fn with_stage_variables<V>(self, variables: V) -> Self
    where
        V: Into<QueryMap>;

    fn request_context(&self) -> RequestContext;
    fn request_context_ref(&self) -> Option<&RequestContext>;
    fn with_request_context(self, context: RequestContext) -> Self;

    fn lambda_context(&self) -> Context;
    fn lambda_context_ref(&self) -> Option<&Context>;
    fn with_lambda_context(self, context: Context) -> Self;
}

impl ExtensionsExt for http::Extensions { ... }
impl ExtensionsExt for http::Parts { ... }
impl<B> ExtensionsExt for http::Request<B> { ... }
pub trait RequestExt {
    fn payload<D>(&self) -> Result<Option<D>, PayloadError>
    where
        for<'de> D: Deserialize<'de>;
}

impl RequestExt for lambda_http::Request { ... }

I can rename ExtensionsExt to RequestExt. Should current RequestExt in this PR become, say, RequestPayload?

@calavera
Copy link
Contributor

I can rename ExtensionsExt to RequestExt. Should current RequestExt in this PR become, say, RequestPayload?

RequestExt and RequestPayloadExt work for me.

Copy link
Contributor

@calavera calavera left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! LGTM

@calavera calavera merged commit 52e75e2 into awslabs:main Apr 10, 2023
@dcormier
Copy link
Contributor Author

Thank you for reviewing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lambda_http: Make extension methods available for http::request::Parts
2 participants