diff --git a/src/Request.ts b/src/Request.ts index 315c768..3a008fe 100644 --- a/src/Request.ts +++ b/src/Request.ts @@ -87,31 +87,29 @@ export default class Request { public readonly method: string; /** - * This property is much like `req.url`; however, it retains the original request URL, - * allowing you to rewrite `req.url` freely for internal routing purposes. For example, - * the "mounting" feature of `app.use()` will rewrite `req.url` to strip the mount - * point. + * This property is much like `req.url`; however, it always retains the original URL + * from the event that triggered the request, allowing you to rewrite `req.url` freely + * for internal routing purposes. For example, the "mounting" feature of `app.use()` + * will rewrite `req.url` to strip the mount point: * - * TODO: We still don't support internal re-routing mid-request. We need to investigate - * how, exactly, Express does this, and what it would take to support it. - * - * ``` - * // GET /search?q=something - * req.originalUrl - * // => "/search?q=something" * ``` + * // GET 'http://www.example.com/admin/new' * - * In a middleware function, `req.originalUrl` is a combination of `req.baseUrl` and - * `req.path`, as shown in the following example. + * let router = new Router(); * - * ``` - * app.use('/admin', function(req, res, next) { // GET 'http://www.example.com/admin/new' + * router.get('/new', function(req, res, next) { * console.log(req.originalUrl); // '/admin/new' * console.log(req.baseUrl); // '/admin' * console.log(req.path); // '/new' + * console.log(req.url); // '/new' * next(); * }); + * + * app.addSubRouter('/admin', router); * ``` + * + * `req.originalUrl` stays the same even when a route handler changes `req.url` for + * internal re-routing. See `req.url` for an example of internal re-routing. */ public readonly originalUrl: string; @@ -142,24 +140,6 @@ export default class Request { */ public readonly params: Readonly; - /** - * Contains the path part of the request URL. - * - * ``` - * // example.com/users?sort=desc - * req.path // => "/users" - * ``` - * - * When called from a middleware, the mount point is not included in req.path. See - * `req.originalUrl` for more details. - */ - public readonly path: string; - - /** - * Synonymous with `req.path`. - */ - public readonly url: string; - /** * Contains the request protocol string: either `http` or (for TLS requests) `https` * (always lowercase). @@ -226,10 +206,6 @@ export default class Request { */ public readonly eventSourceType: ('ALB' | 'APIGW'); - // TODO: maybe some of those properties should not be read-only ... for example, how - // would some middleware do the equivalent of an internal redirect? How does Express - // handle that? - /** * The body of the request. If the body is an empty value (e.g. `''`), `req.body` will * be `null` to make body-exists checks (e.g. `if (req.body)`) simpler. @@ -239,10 +215,29 @@ export default class Request { */ public body?: unknown; + protected _parentRequest?: Request; + protected _url: string; + protected _path: string; + private readonly _headers: StringArrayOfStringsMap; private readonly _event: RequestEvent; - public constructor(app: Application, event: RequestEvent, context: HandlerContext, baseURL: string = '', params: StringMap = {}) { + public constructor(app: Application, eventOrRequest: RequestEvent | Request, context: HandlerContext, + baseURL: string = '', params: StringMap = {}) { + let event: RequestEvent, + path: string; + + if (eventOrRequest instanceof Request) { + // Make this request a sub-request of the request passed into the constructor + this._parentRequest = eventOrRequest; + path = this._parentRequest.path.substring(baseURL.length); + baseURL = this._parentRequest.baseUrl + baseURL; + event = this._parentRequest._event; + } else { + event = eventOrRequest; + path = event.path; + } + this.app = app; this._event = event; this._headers = this._parseHeaders(event); @@ -267,19 +262,108 @@ export default class Request { // Fields related to routing: this.baseUrl = baseURL; - this.path = this.url = event.path; - this.originalUrl = baseURL + event.path; + this._path = this._url = path; + // Despite the fact that the Express docs say that the `originalUrl` is `baseUrl + + // path`, it's actually always equal to the original URL that initiated the request. + // If, for example, a route handler changes the `url` of a request, the `path` is + // changed too, *but* `originalUrl` stays the same. This would not be the case if + // `originalUrl = `baseUrl + path`. See the documentation on the `url` getter for + // more details. + this.originalUrl = event.path; this.params = Object.freeze(params); } + /** PUBLIC PROPERTIES: GETTERS AND SETTERS */ + + /** + * `req.url` is the same as `req.path` in most cases. + * + * However, route handlers and other middleware may change the value of `req.url` to + * redirect the request to other registered middleware. For example: + * + * ``` + * // GET example.com/admin/users/1337 + * + * const router1 = new express.Router(), + * router2 = new express.Router(); + * + * router1.get('/users/:userID', function(req, res, next) { + * // ... + * if (req.params.userID === authenticatedUser.id) { + * // User ID is the same as the authenticated user's. Re-route to user profile + * // handler: + * req.url = '/profile'; + * return next(); + * } + * // ... + * }); + * + * router2.get('/profile', function(req, res) { + * console.log(req.originalUrl); // '/admin/users/1337' + * console.log(req.baseUrl); // '/admin' + * console.log(req.path); // '/profile' + * console.log(req.url); // '/profile' + * // ... + * }); + * + * app.addSubRouter('/admin', router1); + * app.addSubRouter('/admin', router2); + * ``` + * + * In the example above, the `GET` request to `/admin/users/1337` is re-routed to the + * `/profile` handler in `router2`. Any other route handlers on `router1` that would + * have handled the `/users/1337` route are skiped. Also, notice that `req.url` keeps + * the value given to it by `router1`'s route handler, but `req.originalUrl` stays the + * same. + * + * If the route handler or middleware that changes `req.url` adds a query string to + * `req.url`, the query string is retained on the `req.url` property but the query + * string keys and values are *not* parsed and `req.params` is *not* updated. This + * follows Express' apparent behavior when handling internal re-routing with URLs that + * contain query strings. + */ + public get url(): string { + return this._url; + } + + public set url(url: string) { + url = url || ''; + + // Update the parent request's URL with the new URL value + if (this._parentRequest) { + let indexOfCurrentURL = this._parentRequest.url.length - this._url.length; + + this._parentRequest.url = this._parentRequest.url.substring(0, indexOfCurrentURL) + url; + } + this._url = url; + // Remove query parameters from the URL to form the new path + this._path = url.split('?')[0]; + } + + /** + * Contains the path part of the request URL. + * + * ``` + * // example.com/users?sort=desc + * req.path // => "/users" + * ``` + * + * When referenced from middleware, the mount point is not included in `req.path`. See + * `req.originalUrl` for more details. + * + * When any middleware changes the value of `req.url` for internal re-routing, + * `req.path` is updated also. See `req.url` for an example of internal re-routing. + */ + public get path(): string { + return this._path; + } + + // Disable changing the `path` via the public API by not implementing a setter here. + /** CLONING FUNCTION */ public makeSubRequest(baseURL: string, params?: StringMap): Request { - const restOfPath = this._event.path.substring(baseURL.length), - // TODO: this isn't a deep clone - does it need to be? - event = _.extend({}, this._event, { path: restOfPath }); - - return new Request(this.app, event, this.context, baseURL, params); + return new Request(this.app, this, this.context, baseURL, params); } /** CONVENIENCE FUNCTIONS */ diff --git a/src/Router.ts b/src/Router.ts index 18174dc..dba41fa 100644 --- a/src/Router.ts +++ b/src/Router.ts @@ -8,7 +8,7 @@ import { NextCallback, ErrorHandlingRequestProcessor, } from './interfaces'; -import { IRequestMatchingProcessorChain, IProcessorChain } from './chains/ProcessorChain'; +import { IRequestMatchingProcessorChain } from './chains/ProcessorChain'; import { Request, Response } from '.'; import { wrapRequestProcessor, wrapRequestProcessors } from './utils/wrapRequestProcessor'; import { RouteMatchingProcessorChain } from './chains/RouteMatchingProcessorChain'; @@ -35,15 +35,33 @@ export default class Router implements IRouter { // using the case-sensitivity setting of this router. public handle(originalErr: unknown, req: Request, resp: Response, done: NextCallback): void { - const processors = _.filter(this._processors, (p) => { return p.matches(req); }); - - const go = _.reduce(processors.reverse(), (next: NextCallback, p: IProcessorChain): NextCallback => { - return (err) => { - p.run(err, req, resp, next); - }; - }, done); - - go(originalErr); + const processors = this._processors; + + let index = 0; + + const processRequest = (err: unknown, processorReq: Request, processorResp: Response, next: NextCallback): void => { + let processor = processors[index]; + + index += 1; + + if (processor === undefined) { + // We've looped through all available processors. + return next(err); + } else if (processor.matches(processorReq)) { + // ^^^^ User-defined route handlers may change the request object's URL to + // re-route the request to other route handlers. Therefore, we must re-check + // whether the current processor matches the request object after every + // processor is run. + processor.run(err, processorReq, processorResp, (processorError) => { + processRequest(processorError, processorReq, processorResp, next); + }); + } else { + // Current processor does not match. Continue. + processRequest(err, processorReq, processorResp, next); + } + }; + + processRequest(originalErr, req, resp, done); } /** diff --git a/tests/Request.test.ts b/tests/Request.test.ts index dd83a26..7cd4d44 100644 --- a/tests/Request.test.ts +++ b/tests/Request.test.ts @@ -36,6 +36,32 @@ describe('Request', () => { expect(new Request(app, evt2, handlerContext()).method).to.strictlyEqual(''); }); + it('sets URL related fields correctly, when created from an event', () => { + const event = albRequest(), + request = new Request(app, event, handlerContext()); + + expect(request.url).to.strictlyEqual(event.path); + expect(request.path).to.strictlyEqual(event.path); + expect(request.originalUrl).to.strictlyEqual(event.path); + }); + + it('sets URL related fields correctly, when created from a parent request', () => { + const event = albRequest(); + + let parentRequest, request; + + event.path = '/a/b/c'; + + parentRequest = new Request(app, event, handlerContext()); + request = new Request(app, parentRequest, handlerContext(), '/a/b'); + + expect(request.url).to.strictlyEqual('/c'); + expect(request.path).to.strictlyEqual('/c'); + expect(request.baseUrl).to.strictlyEqual('/a/b'); + expect(request.originalUrl).to.strictlyEqual(event.path); + expect(request.originalUrl).to.strictlyEqual(parentRequest.url); + }); + }); describe('makeSubRequest', () => { @@ -470,4 +496,69 @@ describe('Request', () => { }); + describe('`url` property', () => { + + it('should be able to be updated', () => { + let req = new Request(app, apiGatewayRequest(), handlerContext()), + newURL = '/test'; + + // Assert that we have a valid test + expect(req.url).to.not.strictlyEqual(newURL); + + req.url = newURL; + expect(req.url).to.strictlyEqual(newURL); + }); + + it('should accept blank values', () => { + let req = new Request(app, apiGatewayRequest(), handlerContext()), + newURL = ''; + + // Assert that we have a valid test + expect(req.url).to.not.strictlyEqual(newURL); + + req.url = newURL; + expect(req.url).to.strictlyEqual(newURL); + }); + + it('should update `path` when `url` changes', () => { + let req = new Request(app, apiGatewayRequest(), handlerContext()), + newURL = '/test'; + + // Assert that we have a valid test + expect(req.path).to.not.strictlyEqual(newURL); + + req.url = newURL; + expect(req.path).to.strictlyEqual(newURL); + }); + + it('should update the parent request\'s `url` and related properties when a sub-request\'s `url` is updated', () => { + let event = apiGatewayRequest(), + req, subReq, subSubReq; + + // Assert that we have a valid test + expect(event.path).to.not.strictlyEqual('/path/path/old'); + + event.path = '/path/path/old'; + + req = new Request(app, event, handlerContext()); + subReq = req.makeSubRequest('/path'); + subSubReq = subReq.makeSubRequest('/path'); + + subSubReq.url = '/new'; + + expect(subSubReq.url).to.strictlyEqual('/new'); + expect(subSubReq.baseUrl).to.strictlyEqual('/path/path'); + expect(subSubReq.originalUrl).to.strictlyEqual('/path/path/old'); + + expect(subReq.url).to.strictlyEqual('/path/new'); + expect(subReq.baseUrl).to.strictlyEqual('/path'); + expect(subReq.originalUrl).to.strictlyEqual('/path/path/old'); + + expect(req.url).to.strictlyEqual('/path/path/new'); + expect(req.baseUrl).to.strictlyEqual(''); + expect(req.originalUrl).to.strictlyEqual('/path/path/old'); + }); + + }); + }); diff --git a/tests/integration-tests.test.ts b/tests/integration-tests.test.ts index e596eac..3208dde 100644 --- a/tests/integration-tests.test.ts +++ b/tests/integration-tests.test.ts @@ -5,7 +5,7 @@ import { Application, Request, Response, Router } from '../src'; import { RequestEvent } from '../src/request-response-types'; import { NextCallback } from '../src/interfaces'; import { expect } from 'chai'; -import { StringArrayOfStringsMap, StringMap } from '../src/utils/common-types'; +import { StringArrayOfStringsMap, StringMap, KeyValueStringObject } from '../src/utils/common-types'; describe('integration tests', () => { let testBody = { a: 'xyz' }, @@ -332,22 +332,22 @@ describe('integration tests', () => { }); + const testOutcome = (method: string, path: string, expectedBody: string): void => { + const cb = spy(), + evt = makeRequestEvent(path, apiGatewayRequest(), method); - describe('sub-routing', () => { - const testOutcome = (method: string, path: string, expectedBody: string): void => { - const cb = spy(), - evt = makeRequestEvent(path, apiGatewayRequest(), method); + app.run(evt, handlerContext(), cb); - app.run(evt, handlerContext(), cb); + assert.calledOnce(cb); + assert.calledWithExactly(cb, undefined, { + statusCode: 200, + body: expectedBody, + isBase64Encoded: false, + multiValueHeaders: { 'Content-Type': [ 'text/html' ] }, + }); + }; - assert.calledOnce(cb); - assert.calledWithExactly(cb, undefined, { - statusCode: 200, - body: expectedBody, - isBase64Encoded: false, - multiValueHeaders: { 'Content-Type': [ 'text/html' ] }, - }); - }; + describe('sub-routing', () => { it('works when we have added a subrouter and subsubrouter to the main app', () => { const r1 = new Router(), @@ -409,4 +409,142 @@ describe('integration tests', () => { }); }); + describe('internal re-routing with `request.url`', () => { + + // eslint-disable-next-line @typescript-eslint/explicit-function-return-type + function testRoutesWithRedirect(changeReqFn = (req: Request) => { req.url = '/goodbye'; }, expectedResponse: string = 'goodbye') { + const router = new Router(); + + let reqProps = [ 'url', 'path', 'query' ], + helloRouteHandler: SinonSpy, + helloRouteHandler2: SinonSpy, + goodbyeRouteHandler: SinonSpy, + helloReq: { url: string; path: string; query: KeyValueStringObject }, + hello2Req: { url: string; path: string; query: KeyValueStringObject }, + goodbyeReq: { url: string; path: string; query: KeyValueStringObject }; + + helloRouteHandler = spy((req: Request, _resp: Response, next: NextCallback): void => { + helloReq = _.pick(req, ...reqProps); + changeReqFn(req); + next(); + }); + + helloRouteHandler2 = spy((req: Request, resp: Response): void => { + hello2Req = _.pick(req, ...reqProps); + resp.send('hello'); + }); + + goodbyeRouteHandler = spy((req: Request, resp: Response): void => { + goodbyeReq = _.pick(req, ...reqProps); + resp.send('goodbye'); + }); + + router.get('/hello', helloRouteHandler) + .get('/goodbye', goodbyeRouteHandler) + .get('/hello', helloRouteHandler2); + + app.addSubRouter('/path', router); + + testOutcome('GET', '/path/hello', expectedResponse); + + return { + hello: helloRouteHandler, + getHelloReq: () => { return helloReq; }, + hello2: helloRouteHandler2, + getHello2Req: () => { return hello2Req; }, + goodbye: goodbyeRouteHandler, + getGoodbyeReq: () => { return goodbyeReq; }, + }; + } + + it('redirects to a different route handler when req.url is changed', () => { + const handlers = testRoutesWithRedirect(); + + assert.calledOnce(handlers.hello); + assert.calledOnce(handlers.goodbye); + assert.notCalled(handlers.hello2); + assert.callOrder(handlers.hello, handlers.goodbye); + }); + + it('continues executing routes in the current chain before redirecting to the next match', () => { + const router = new Router(); + + let h1, h2Redirects, h3, h4ShouldBeSkipped, g1ShouldBeSkipped, g2Ends; + + h1 = spy((_req: Request, _resp: Response, next: NextCallback): void => { next(); }); + + h2Redirects = spy((req: Request, _resp: Response, next: NextCallback): void => { + req.url = '/goodbye'; + next(); + }); + + h3 = spy((_req: Request, _resp: Response, next: NextCallback): void => { next(); }); + h4ShouldBeSkipped = spy((_req: Request, _resp: Response, next: NextCallback): void => { next(); }); + g1ShouldBeSkipped = spy((_req: Request, _resp: Response, next: NextCallback): void => { next(); }); + g2Ends = spy((_req: Request, resp: Response): void => { resp.send('goodbye'); }); + + router.get('/goodbye', g1ShouldBeSkipped) // skipped because request starts at /hello + .get('/hello', h1, h2Redirects, h3) // even though h2 redirects to /goodbye, the chain should still run h3 + .get('/hello', h4ShouldBeSkipped) // by now the request is for /goodbye, so skip this + .get('/goodbye', g2Ends); // this /goodbye handler gets called because of the redirect above + + app.addSubRouter('/path', router); + + testOutcome('GET', '/path/hello', 'goodbye'); + + // This should be skipped because it was registered *before* the `h2Redirects` + // handler that causes the re-route to '/goodbye'. + assert.notCalled(g1ShouldBeSkipped); + assert.calledOnce(h1); + // This route handler re-routes the request to '/goodbye'. + assert.calledOnce(h2Redirects); + // This is the last route handler in the `.get('/hello', h1, h2Redirects, h3)` + // chain. The main point of this test is to ensure that it's NOT skipped. + assert.calledOnce(h3); + assert.notCalled(h4ShouldBeSkipped); + assert.calledOnce(g2Ends); + assert.callOrder(h1, h2Redirects, h3, g2Ends); + }); + + it('ignores query string params when `request.url` changes to a URL with query params', () => { + const handlers = testRoutesWithRedirect((req: Request) => { req.url = '/goodbye?to=you'; }); + + expect(handlers.getHelloReq().url).to.strictlyEqual('/hello'); + // Expect that the query parameters were stripped from the URL + expect(handlers.getGoodbyeReq().url).to.strictlyEqual('/goodbye'); + + // Expect that the query parameter `to` was ignored + expect(handlers.getGoodbyeReq().query.to).to.strictlyEqual(undefined); + }); + + it('updates path params when `request.url` changes to a URL with different path params', () => { + const router1 = new Router(), + router2 = new Router(), + USER_ID = '1337', + USERNAME = 'mluedke'; + + let router1Params, router2Params; + + router1.get('/users/:userID', (req: Request, _resp: Response, next: NextCallback) => { + router1Params = req.params; + req.url = `/profile/${USERNAME}`; + next(); + }); + + router2.get('/profile/:username', (req: Request, resp: Response) => { + router2Params = req.params; + resp.send(`${req.params.username} profile`); + }); + + app.addSubRouter('/admin', router1); + app.addSubRouter('/admin', router2); + + testOutcome('GET', `/admin/users/${USER_ID}`, `${USERNAME} profile`); + + expect(router1Params).to.eql({ userID: USER_ID }); + expect(router2Params).to.eql({ username: USERNAME }); + }); + + }); + });