Skip to content

fix: case-sensitive routing for sub-router mounting points (#17) #24

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.

Sign up for GitHub

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

Merged
jthomerson merged 1 commit into from
Mar 24, 2019
Merged

fix: case-sensitive routing for sub-router mounting points (#17) #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions src/Router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,14 +70,7 @@ export default class Router implements IRouter {
* code hinting / auto-completion.
*/
public addSubRouter(path: PathParams, router: Router): this {
// Note: this overriding of the case sensitivity of the passed-in router is likely
// ineffective for most usecases because the user probably created their router and
// added a bunch of routes to it before adding that router to this one. When the
// routes are created (technically the `IRequestMatchingProcessorChain` objects, in
// particular `RouteMatchingProcessorChain`), the case sensitivity is already set
// (inherited from the router that created the chain).
router.routerOptions.caseSensitive = this.routerOptions.caseSensitive;

@jthomerson jthomerson Mar 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we need to test what happens when a router is added to an app

@yokuze yokuze Mar 1, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I verified this using the following test case:

const express = require('express'),
      port = 3000,
      app = express();

// Test request: GET /World
// The 'case sensitive routing' application setting is NOT passed on to routers
// registered with app.use

const router = express.Router();

app.set('case sensitive routing', true);
app.use(router); // set this BEFORE adding any routes

router.get('/World', (req, resp, next) => {
   console.log('uppercase /World');
   console.log('subApp is case sens?', subApp.get('case sensitive routing'));
   next();
});
router.get('/world', (req, resp, next) => {
   console.log('lowercase /world');
   console.log('subApp is case sens?', subApp.get('case sensitive routing'));
   next();
});
router.get('*', (req, resp) => { resp.send('world'); });

app.listen(port, () => console.log(`Example app listening on port ${port}!`));

this._processors.push(new SubRouterProcessorChain(path, router));
this._processors.push(new SubRouterProcessorChain(path, router, this.routerOptions));
return this;
}

Expand Down
27 changes: 23 additions & 4 deletions src/chains/SubRouterProcessorChain.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { IRequestMatchingProcessorChain } from './ProcessorChain';
import { PathParams, IRouter, NextCallback } from '../interfaces';
import { PathParams, IRouter, NextCallback, RouterOptions } from '../interfaces';
import { Request, Response } from '..';
const pathToRegexp = require('path-to-regexp');

Expand All @@ -8,9 +8,28 @@ export class SubRouterProcessorChain implements IRequestMatchingProcessorChain {
private readonly _matcher: RegExp;
private readonly _router: IRouter;

public constructor(path: PathParams, router: IRouter) {
// TODO: case sensitivity settings (strict and end need to remain false here):
this._matcher = pathToRegexp(path, [], { sensitive: false, strict: false, end: false });
public constructor(path: PathParams, router: IRouter, parentRouterOptions: RouterOptions) {
// Although we use the subrouter to handle matched routes, we use the parent
// router's case-sensitivity setting to match the route path that this subrouter is
// mounted to because that's what Express does. For example:
//
// ```
// const express = require('express'),
// app = express(),
// router = express.Router({ caseSensitive: false });
//
// app.enable('case sensitive routing');
// app.use('/hello', router);
// router.get('/world', (req, resp) => { resp.send('Hello world'); });
// ```
//
// In the example above, the `/hello` part of the path is case-sensitive, and the
// `/world` part of the path is case-insensitive. Therefore, GET requests to
// `/hello/WORLD` would match the `/world` handler but requests to `/HELLO/world`
// would not. To replicate this in Lambda Express, we have to use the parent
// router's case sensitivity settings (`app`, in this example) for the mounting
// point (`/world`).
this._matcher = pathToRegexp(path, [], { sensitive: parentRouterOptions.caseSensitive, strict: false, end: false });
this._router = router;
}

Expand Down
4 changes: 2 additions & 2 deletions tests/chains/SubRouterProcessorChain.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe('SubRouterProcessorChain', () => {
let app = new Application(),
req = new Request(app, _.extend(apiGatewayRequest(), { path: path }), handlerContext()),
router = new TestRouter(spy()),
chain = new SubRouterProcessorChain(routes, router);
chain = new SubRouterProcessorChain(routes, router, app.routerOptions);

expect(chain.matches(req)).to.eql(expectation);
};
Expand Down Expand Up @@ -58,7 +58,7 @@ describe('SubRouterProcessorChain', () => {
done = spy(),
handle = spy(),
router = new TestRouter(handle),
chain = new SubRouterProcessorChain(routes, router);
chain = new SubRouterProcessorChain(routes, router, app.routerOptions);

chain.run('nothing', req, resp, done);
assert.calledOnce(handle);
Expand Down
90 changes: 75 additions & 15 deletions tests/integration-tests.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,21 @@ describe('integration tests', () => {
});
};

const testOutcome = (method: string, path: string, expectedBody: string): void => {
const cb = spy(),
evt = makeRequestEvent(path, apiGatewayRequest(), method);

app.run(evt, handlerContext(), cb);

assert.calledOnce(cb);
assert.calledWithExactly(cb, undefined, {
statusCode: 200,
body: expectedBody,
isBase64Encoded: false,
multiValueHeaders: { 'Content-Type': [ 'text/html' ] },
});
};

describe('very simple smoke test that hits a variety of touch points', () => {
const test = (evt: RequestEvent): SinonSpy => {
const cb = spy();
Expand Down Expand Up @@ -227,6 +242,66 @@ describe('integration tests', () => {
testWithLastResortHandler(200, '200 OK', [ 'Content-Type-HTML' ], 'will match');
});

it('matches sub-routes case-insensitively when parent route is case-sensitive and sub-route is case-insensitive', () => {
const router = new Router({ caseSensitive: false });

// App (parent, `/hello`): case-sensitive
// Router (`/world`): case-insensitive
expect(app.routerOptions.caseSensitive).to.eql(false);
app.enable('case sensitive routing');
expect(app.routerOptions.caseSensitive).to.eql(true);

app.addSubRouter('/hello', router);
router.get('/world', (_req: Request, resp: Response) => { resp.send('Hello world'); });

// Return a fallback response
app.use((_req: Request, resp: Response) => { resp.send('not found'); });

// Exact match
testOutcome('GET', '/hello/world', 'Hello world');

// App (parent, `/hello`): case-sensitive
testOutcome('GET', '/HELLO/world', 'not found');
testOutcome('GET', '/Hello/world', 'not found');
testOutcome('GET', '/HELLO/WORLD', 'not found');

// Router (`/world`): case-insensitive
testOutcome('GET', '/hello/WORLD', 'Hello world');
testOutcome('GET', '/hello/World', 'Hello world');

// Mismatch
testOutcome('GET', '/goodbye/world', 'not found');
});

it('matches sub-routes case-sensitively when parent route is case-insensitive and sub-route is case-sensitive', () => {
const router = new Router({ caseSensitive: true });

// App (parent, `/hello`): case-insensitive
// Router (`/world`): case-sensitive
expect(app.routerOptions.caseSensitive).to.eql(false);

app.addSubRouter('/hello', router);
router.get('/world', (_req: Request, resp: Response) => { resp.send('Hello world'); });

// Return a fallback response
app.use((_req: Request, resp: Response) => { resp.send('not found'); });

// Exact match
testOutcome('GET', '/hello/world', 'Hello world');

// App (parent, `/hello`): case-insensitive
testOutcome('GET', '/HELLO/world', 'Hello world');
testOutcome('GET', '/Hello/world', 'Hello world');

// Router (`/world`): case-sensitive
testOutcome('GET', '/hello/WORLD', 'not found');
testOutcome('GET', '/hello/World', 'not found');
testOutcome('GET', '/HELLO/WORLD', 'not found');

// Mismatch
testOutcome('GET', '/goodbye/world', 'not found');
});

});

describe('other HTTP methods', () => {
Expand Down Expand Up @@ -332,21 +407,6 @@ describe('integration tests', () => {

});

const testOutcome = (method: string, path: string, expectedBody: string): void => {
const cb = spy(),
evt = makeRequestEvent(path, apiGatewayRequest(), method);

app.run(evt, handlerContext(), cb);

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', () => {
Expand Down