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

4.3.10 is a major breaking change in behavior #454

Closed
alexanderbh opened this issue Aug 30, 2021 · 6 comments
Closed

4.3.10 is a major breaking change in behavior #454

alexanderbh opened this issue Aug 30, 2021 · 6 comments
Labels

Comments

@alexanderbh
Copy link
Contributor

alexanderbh commented Aug 30, 2021

4.3.10 is breaking ALL our services. It is a patch version and we they are updated automatically. Why is this change not a major version bump? It might have fixed an "issue". But if the issue is how everyone have setup their existing routes then fixing this is a breaking change.

45edbfa

It might "error out with 404" if you are setting up a new service. But for everyone else using this library. And have routes that works. This will totally mess it up.

In my event handler I have staff/admin/{proxy+} and in my serverless-express server I have a route that simply matches on for example /list. That works great in 4.3.9 or below. That is how everyone have managed until now. It will match the path staff/admin/list. Great.

It might "error out with 404" if you make your route like /staff/admin/list. But you have to think about existing users. We are using 4.3.9 as it is. Changing this "issue" (I like it better if I can just route on the proxy part alone so it is not an improvement for me) breaks it for everyone using proxy on 4.3.9.

@selvendranayyaswamy
@selvendran.ayyaswamy

@yeiniel
Copy link

yeiniel commented Aug 31, 2021

i'm facing the same issue @alexanderbh is facing right now

if you have staff/admin/{proxy+} at your SAM template it doesn't make sense to use /staff/admin/list at the lambda code level. That means that your lambda can't be setup to handle a different event on a different path. Your lambda code should be agnostic of the mount path you are using on the Api Gateway

@alexanderbh
Copy link
Contributor Author

alexanderbh commented Aug 31, 2021

i'm facing the same issue @alexanderbh is facing right now

if you have staff/admin/{proxy+} at your SAM template it doesn't make sense to use /staff/admin/list at the lambda code level. That means that your lambda can't be setup to handle a different event on a different path. Your lambda code should be agnostic of the mount path you are using on the Api Gateway

Exactly this. We have the same lambda function on different API Gateway proxy endpoints. We have one that has an authorizer called staff/admin/list. But then we have another resource on API gateway that is pointing to the SAME lambda function. But it does not have an authorizer (or it could be a different one). In our case we have some manual header authentication. That resource is called staff/internal/list. But they are both pointing to the same lambda function.

But now I cannot make a single lambda listening for the proxy part (/list) and use it both places. Now the lambda is forever bound to the exact path prefix for the custom domain + what ever URL path you have used in the API Gateway resource.

That lambda function is just listening for the proxy part: /list. It does (and should) not care about the path leading up to it.

github-actions bot pushed a commit that referenced this issue Aug 31, 2021
## [4.3.11](v4.3.10...v4.3.11) (2021-08-31)

### Bug Fixes

* revert [#441](#441) ([#455](#455)) ([87aa26f](87aa26f)), closes [#454](#454)
@github-actions
Copy link

🎉 This issue has been resolved in version 4.3.11 🎉

The release is available on:

Your semantic-release bot 📦🚀

@yeiniel
Copy link

yeiniel commented Aug 31, 2021

thank you @brett-vendia and @alexanderbh for fixing this situation so soon.

@lafiosca
Copy link

lafiosca commented Nov 22, 2022

Not trying to dig up an ancient thread, but I upgraded from the old aws-serverless-express library to this one and had my API broken by this change. In my API Gateway config, I have / and /{proxy+} which both have Cognito auth configured and /public/{proxy+} which does not require any auth. All three of these paths point to the same Express Lambda. The old library preserved the paths, and in my Express app I knew not to expect authentication headers in any routes under /public/. But with this library now, requests to /public/stats show up as requests for /stats to the Express app. I guess to make this work without completely reconfiguring my API Gateway I will need to either build a custom request mapper or locally patch the library.

--
Edited to add: turns out the custom mapper is a bit of a hassle, probably easier to just delete event.pathParameters before passing the event to the handler. 🙃 Thinking about this more deeply, it seems silly to me to replace the path with the value of event.pathParameters.proxy by default. That's just a specific route parameter value that comes from the convention of having a catch-all route parameter named proxy. It conveys nothing about how the developer wants to map those routes to their Lambda function(s).

OneDev0411 added a commit to OneDev0411/serverless-express that referenced this issue Mar 16, 2023
@eran-medan
Copy link

Not trying to dig up an ancient thread, but I upgraded from the old aws-serverless-express library to this one and had my API broken by this change. In my API Gateway config, I have / and /{proxy+} which both have Cognito auth configured and /public/{proxy+} which does not require any auth. All three of these paths point to the same Express Lambda. The old library preserved the paths, and in my Express app I knew not to expect authentication headers in any routes under /public/. But with this library now, requests to /public/stats show up as requests for /stats to the Express app. I guess to make this work without completely reconfiguring my API Gateway I will need to either build a custom request mapper or locally patch the library.

-- Edited to add: turns out the custom mapper is a bit of a hassle, probably easier to just delete event.pathParameters before passing the event to the handler. 🙃 Thinking about this more deeply, it seems silly to me to replace the path with the value of event.pathParameters.proxy by default. That's just a specific route parameter value that comes from the convention of having a catch-all route parameter named proxy. It conveys nothing about how the developer wants to map those routes to their Lambda function(s).

Having the same exact issue, I think we should reopen / create a new issue for this.

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

No branches or pull requests

4 participants