Skip to content

Commit

Permalink
fix: case-sensitive routing for sub-router mounting points (silvermin…
Browse files Browse the repository at this point in the history
…e#17)

When a sub-router was mounted to a given path, the
SubRouterProcessorChain did not respect the router's case-sensitivity
settings. Additionally, sub-routers should NOT inherit their
case-sensitivity settings from their parent router.
  • Loading branch information
yokuze committed Mar 1, 2019
1 parent c625d58 commit 3da1699
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 24 deletions.
7 changes: 0 additions & 7 deletions src/Router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,6 @@ 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;
this._processors.push(new SubRouterProcessorChain(path, router));
return this;
}
Expand Down
3 changes: 1 addition & 2 deletions src/chains/SubRouterProcessorChain.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,7 @@ export class SubRouterProcessorChain implements IRequestMatchingProcessorChain {
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 });
this._matcher = pathToRegexp(path, [], { sensitive: router.routerOptions.caseSensitive, strict: false, end: false });
this._router = router;
}

Expand Down
98 changes: 83 additions & 15 deletions tests/integration-tests.test.ts
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,74 @@ describe('integration tests', () => {
testWithLastResortHandler(200, '200 OK', [ 'Content-Type-HTML' ], 'will match');
});

it('matches the mounting point path case-insensitively for sub-routes when disabled', () => {
// Disable case-sensitive routing
const router = new Router({ caseSensitive: false });

let handler = spy((_req: Request, _resp: Response, next: NextCallback) => { next(); });

// Case sensitive routing should be *off* for the sub-router, but we turn
// case-sensitive routing *on* for the parent router (app) so that we know that
// the case-sensitivity settings for the mounting point path are coming from the
// sub-router and not the parent router.
expect(app.routerOptions.caseSensitive).to.eql(false);
app.enable('case sensitive routing');
expect(app.routerOptions.caseSensitive).to.eql(true);

// `/hello/world` is case-insensitive
app.addSubRouter('/hello', router);
router.get('/world', handler);

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

// Test that the mounting point (`/hello`) is NOT case-sensitive
testOutcome('GET', '/hello/world', 'Hello world');
assert.calledOnce(handler);
handler.resetHistory();

testOutcome('GET', '/Hello/world', 'Hello world');
assert.calledOnce(handler);
handler.resetHistory();

testOutcome('GET', '/HELLO/world', 'Hello world');
assert.calledOnce(handler);
});

it('matches the mounting point path case-sensitively for sub-routes when enabled', () => {
// Enable case-sensitive routing
const router = new Router({ caseSensitive: true });

let handler;

// Case sensitive routing should be *on* for the sub-router, but we leave
// case-sensitive routing *off* for the parent router (app) so that we know that
// the case-sensitivity settings for the mounting point path are coming from the
// sub-router and not the parent router.
expect(app.routerOptions.caseSensitive).to.eql(false);

handler = spy((_req: Request, _resp: Response, next: NextCallback) => { next(); });

// `/Hello/world` is case-sensitive
app.addSubRouter('/hello', router);
router.get('/world', handler);

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

// Test case-sensitivity in the mounting point (`/hello`)
testOutcome('GET', '/HELLO/world', 'Hello world');
assert.notCalled(handler);
handler.resetHistory();

testOutcome('GET', '/Hello/world', 'Hello world');
assert.notCalled(handler);
handler.resetHistory();

testOutcome('GET', '/hello/world', 'Hello world');
assert.calledOnce(handler);
});

});

describe('other HTTP methods', () => {
Expand Down Expand Up @@ -332,21 +415,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

0 comments on commit 3da1699

Please sign in to comment.