H4ad / serverless-adapter

Run REST APIs and other web applications using your existing Node.js application framework (NestJS, Express, Koa, tRPC, Fastify and many others), on top of AWS, Azure, Huawei and many other clouds.
https://serverless-adapter.viniciusl.com.br/
MIT License
132 stars 8 forks source link

fix(apig-v1-adapter): lowercase headers #259

Closed tcandens closed 1 week ago

tcandens commented 1 week ago

Description of change

Adding back the default behavior of emulating Node.js http by lowercasing all request headers. Large parts of JS ecosystem assume that these headers, particularly cookie header, can be accessed with the lowercase key.

This behavior was present in past versions of the ApiGatewayV1Adapter so please correct me if there is good reason that this was changed, but from my perspective it appears it may have just been missed. This past PR is an example, https://github.com/H4ad/serverless-adapter/pull/25.

Also, potentially related to #73

Pull-Request Checklist

H4ad commented 1 week ago

Did you had any issue with the headers not being flattered?

I changed to not lowercase because the headers are always lowercase when came from API Gateway V1, so for performance reasons, I just skipped the lowercase.

tcandens commented 1 week ago

Thank you for looking!

Did you had any issue with the headers not being flattered?

We haven't had any issues with unflattened headers, was just attempting to reuse function that was already available to ensure the lowercasing. Happy to take a another pass to limit the operations to just casing in the interest of perf.

I changed to not lowercase because the headers are always lowercase when came from API Gateway V1, so for performance reasons, I just skipped the lowercase.

Very interesting! This has not been our observation. After updating from v2.17.0 to v4.2.1, we noticed our services were not receiving cookies anymore which ended up being due to the header now being the capitalized Cookie instead of just cookie. Downgrading serverless adapter resolved the issue.

H4ad commented 1 week ago

Downgrading serverless adapter resolved the issue.

You can just copy the adapter that you modified to your own codebase and keep the newer version of the library.

which ended up being due to the header now being the capitalized Cookie instead of just cookie.

I'm fine with having back the behavior but this will require a breaking change release.

If you want to merge this PR and release as a feat, you can modify this PR and create a new flag option for this adapter called shouldLowercaseHeaders and make it default to false. In this way, you can mark it as true for your use-case and we can see if more people expect the header to be lowercase, what do you think?

tcandens commented 1 week ago

Thanks @H4ad, I've reworked the PR to involve an option to pass to the adapter constructor to avoid a breaking change.

Please let me know what you think.

tcandens commented 1 week ago

Thanks again @H4ad, Made two small tweaks. Please review

H4ad commented 1 week ago

LGTM, thanks for the contribution!