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

Correct path for non-custom endpoints #42

Merged
merged 3 commits into from Mar 26, 2018

Conversation

Projects
None yet
3 participants
@bsdkurt
Contributor

bsdkurt commented Mar 26, 2018

Non-custom domain endpoints (e.g. https://xyz.execute-api.region.amazonws.com/{stage}) strip off {stage} in event.path, but event.requestContext.path is correct for all cases I believe. Also should fix #35.

bsdkurt added some commits Mar 26, 2018

@dougmoscrop

This comment has been minimized.

Owner

dougmoscrop commented Mar 26, 2018

Thanks for this!

@dougmoscrop dougmoscrop merged commit 149f53f into dougmoscrop:master Mar 26, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dougmoscrop

This comment has been minimized.

Owner

dougmoscrop commented Mar 28, 2018

Hey, @bsdkurt I have to revert this, it doesn't seem to work as expected.

Now that I think about it, I don't think we want the path to contain the stage.

@bsdkurt

This comment has been minimized.

Contributor

bsdkurt commented Mar 28, 2018

@dougmoscrop What use case broke?

@bsdkurt

This comment has been minimized.

Contributor

bsdkurt commented Mar 28, 2018

@dougmoscrop Ok I see the bug reports. There does seem to be two use cases that would need to adjust for this change:

  • offline since the path is now obtained from event.requestContext.path vs event.path
  • users using non-custom domain endpoints (e.g. https://xyz.execute-api.region.amazonws.com/{stage}) since the path exposed to them will have a change and include the {stage} part

I do think this is the correct path to expose since for non-custom domain endpoints what is exposed now is incorrect (e.g. #35) and also would require the developer to make changes to the routing to account for the additional path segment if the same code is migrated to a custom domain with a basepath.

Due to the need for adjustments for the two cases above, perhaps this change is better suited for a 1.6 or 2.0 release with upgrade instructions for the two cases?

@dougmoscrop

This comment has been minimized.

Owner

dougmoscrop commented Mar 28, 2018

I'm not sure if it's correct to expose it.

The application routing logic won't match when the stage is exposed. You write app routers as "relative" to some assumed baseUrl (which is empty).

@bsdkurt

This comment has been minimized.

Contributor

bsdkurt commented Mar 28, 2018

Let's take three cases for a GET at the root where lambda attaches:

  • Custom Domain With Basepath

GET https://api.mydomain.com/mybasepath
event.path === event.requestContext.path === '/mybasepath'

  • Custom Domain Without Basepath (e.g. basepath === '')

GET https://api.mydomain.com
event.path === event.requestContext.path === '/'

  • Amazon Endpoint - stage 'dev'

GET https://xxxxx.execute-api.us-east-1.amazonaws.com/dev/
event.path !== event.requestContext.path
event.path === '/'
event.requestContext.path === '/dev'

I believe express expects to know the full path as can be seen in #35. That's why I think event.requestContext.path is the correct path to expose out.

@dougmoscrop

This comment has been minimized.

Owner

dougmoscrop commented Mar 28, 2018

If you app.use(/foo) are you saying express understands /dev/foo?

@dougmoscrop

This comment has been minimized.

Owner

dougmoscrop commented Mar 28, 2018

(baseUrl is a separate concept)

I think we want to set baseUrl to rc.path.replace(event.path, '')

@dougmoscrop

This comment has been minimized.

Owner

dougmoscrop commented Mar 28, 2018

Yeah bunch of people had to mount their app under the stage name which isn't ideal

@XuluWarrior

This comment has been minimized.

XuluWarrior commented Mar 28, 2018

@bsdkurt Did you try my PR #36?
It seems you and I are trying to fix similar issues and I tested my PR with a number of scenarios including

  • non-custom domains
  • custom domains
  • custom domains with base path
  • serverless-offline

If you have a use case that is missing and breaks then I'm sure that between the three of us, we can square the wheel.

@XuluWarrior

This comment has been minimized.

XuluWarrior commented Mar 28, 2018

It has been my intention for far too long now to create a sample project that demonstrates the different configurations but I have to admit that it still remains on my todo list. And I'm off on holiday tomorrow morning so it will have to wait a while longer.

@dougmoscrop

This comment has been minimized.

Owner

dougmoscrop commented Mar 28, 2018

@bsdkurt

This comment has been minimized.

Contributor

bsdkurt commented Mar 29, 2018

@dougmoscrop Sorry for disrupting the userbase with the PR. I think I'm not doing a very good job explaining why I think the change is correct.

Suppose we were running express or koa in node (not with AWS API GW). Is there a scenario where the user types this url in the browser http://www.mydomain.com/pathsegment1/ where express or koa would receive a request object from the listener where request.url is '/'? or would it always get '/pathsegement1/?

If it is correct to say that express and koa expect the request.url to include /pathsegment1/ from the listener always, then I would make a case that since AWS forces the stage on its endpoints, that it should be there for express or koa to see. If I was planning on using AWS endpoints I would have the following setup:

const router = express.Router();
router.get('/', rootget);
...
app.use('/' + process.env.STAGE_NAME, router);

For custom domains with basepaths, I would use the same solution but use process.env.BASE_PATH.

@XuluWarrior I didn't try #36 because I didn't think it was the correct fix. baseUrl is an express request object extension, koa doesn't support baseUrl on the request object. baseUrl is also set by express route.js. I thought from code inspection that it may be replaced with express's idea of what baseUrl should be, although I didn't test that to see if I can break it.

@dougmoscrop

This comment has been minimized.

Owner

dougmoscrop commented Mar 29, 2018

I certainly think there is a gap here! I'm just not sure where it is or what the best fix it. API Gateway is mounting the application in a non-transparent way only in certain scenarios.

@XuluWarrior

This comment has been minimized.

XuluWarrior commented Apr 23, 2018

I've finally created a demo repository (https://github.com/XuluWarrior/serverless-http-tests) that demonstrates the issue as I understand it.

It has three branches

master
This is a simple express server that uses the current version of serverless-http and serves a static index.html from /static.
It is live at
https://vjbhf8sye8.execute-api.us-east-1.amazonaws.com/dev/serverless-http-tests and
https://api.xuluwarrior.org/serverless-http-tests

The issue that started me looking at this is that when you visit
https://vjbhf8sye8.execute-api.us-east-1.amazonaws.com/dev/serverless-http-tests/static it redirects to
https://vjbhf8sye8.execute-api.us-east-1.amazonaws.com/serverless-http-tests/static/ rather than
https://vjbhf8sye8.execute-api.us-east-1.amazonaws.com/dev/serverless-http-tests/static/

This is because express.static depends on baseUrl to determine the full redirect url.

https://api.xuluwarrior.org/serverless-http-tests/static redirects as expected

set-baseurl
This branch uses my PR and sets both baseUrl and originalUrl so that the correct redirect can be calculated.
It is live at
https://lnm600m566.execute-api.us-east-1.amazonaws.com/dev/serverless-http-tests-use-baseurl and
https://api.xuluwarrior.org/serverless-http-tests-use-baseurl

koa
This is my attempt to set up a similar koa server (I am not familiar with koa). The static server doesn't behave the same as the express on and so is not a good example. But what is of interest is the properties on the context object. It includes originalUrl which would be of use to get the full url of the request.

However, the koa code sets originalUrl from req.url rather than req.originalUrl. So even though my PR sets req.originalUrl it is not copied onto the context object.

I have added a hacky middleware function that does the copy.

I have done this not to suggest that it is the correct solution but just to show possibilities.

The koa server is live at
https://api.xuluwarrior.org/serverless-http-tests-koa and
http://localhost:3000/serverless-http-tests-koa/

@dougmoscrop

This comment has been minimized.

Owner

dougmoscrop commented Apr 23, 2018

thanks! need to think about what the best course of action is so not to upend all libraries for the sake of one, or collide with fields that are used differently between libraries.

@dougmoscrop

This comment has been minimized.

Owner

dougmoscrop commented Apr 23, 2018

Thought: One way to figure this out might be to check if the Host: header contains the execute-api.blahblah and then we know we should add the stage name to the originalUrl/url.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment