Skip to content

Commit

Permalink
fix: fix for non-root proxy+
Browse files Browse the repository at this point in the history 
Co-authored-by: selvendran.ayyaswamy <selvendran.ayyaswamy@toyota.com>
  • Loading branch information
@selvendranayyaswamy
selvendranayyaswamy and selvendran.ayyaswamy authored Aug 28, 2021
1 parent 50f83d0 commit 45edbfa
Showing 1 changed file with 6 additions and 2 deletions.
8 changes: 6 additions & 2 deletions src/event-sources/utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,12 @@ const url = require('url')
function getPathWithQueryStringParams ({
event,
query = event.multiValueQueryStringParameters,
// NOTE: Use `event.pathParameters.proxy` if available ({proxy+}); fall back to `event.path`
path = (event.pathParameters && event.pathParameters.proxy && `/${event.pathParameters.proxy}`) || event.path,
// NOTE: Always use event.path, if the API gateway has custom route setup, for example if my controllers path is
// something like employee/services/service1, employee/services/service2 etc, if I dont have any custom path/resources setup
// and directly have root/{proxy+} it works as expected, if i have a resource like /employee and child to that if there
// is a resource like {proxy+} it is not working as expected and errors out with 404. This change is required to address that
// specific issue.
path = event.path,
// NOTE: Strip base path for custom domains
stripBasePath = '',
replaceRegex = new RegExp(`^${stripBasePath}`)
Expand Down

3 comments on commit 45edbfa

@alexanderbh
Copy link
Contributor

@alexanderbh alexanderbh commented on 45edbfa Aug 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is a major breaking change. All our services are not gonna break. And this is a patch version upgrade? It should have been a major version bump. Our services are auto updating patch versions. So they will break if they are build with this new version.

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 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.

@yeiniel
Copy link

Choose a reason for hiding this comment

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

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

@yeiniel
Copy link

Choose a reason for hiding this comment

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

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

Please sign in to comment.