Repository navigation
fix: case-sensitive routing for sub-router mounting points (#17) - #24
Conversation
| // 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; |
There was a problem hiding this comment.
we need to test what happens when a router is added to an app
There was a problem hiding this comment.
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}!`));|
After my response to your one comment, this is ready for re-review. |
| 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 |
There was a problem hiding this comment.
Wording could be simpler:
App (parent, `/hello`): case-sensitive
Router (`/world`): case-insensitive
| app.enable('case sensitive routing'); | ||
| expect(app.routerOptions.caseSensitive).to.eql(true); | ||
|
|
||
| // `/hello/world` is case-insensitive |
There was a problem hiding this comment.
Comment clarification or removal.
| app.addSubRouter('/hello', router); | ||
| router.get('/world', handler); | ||
|
|
||
| // Return a response |
There was a problem hiding this comment.
ADIRL, let's return a not found response or something that makes it easier to read the test - the outcome is either that it matched ("Hello world") or that it did not match ("not found"). It's confusing for non-matching tests below to say "hello world".
| // Disable case-sensitive routing | ||
| const router = new Router({ caseSensitive: false }); | ||
|
|
||
| let handler = spy((_req: Request, _resp: Response, next: NextCallback) => { next(); }); |
There was a problem hiding this comment.
as commented on below. this is the place that should say resp.send('hello world')
| handler.resetHistory(); | ||
|
|
||
| testOutcome('GET', '/HELLO/world', 'Hello world'); | ||
| assert.calledOnce(handler); |
There was a problem hiding this comment.
It's not clear to me at all if we're doing the same thing as Express here. Here's a sample using Express. The parent (/hello) portion of the URL maintains its case sensitivity even though the rest of the URL (the /world) part is not case sensitive.
Express example:
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({ caseSensitive: false });
app.enable('case sensitive routing');
app.use('/hello', router);
router.get('/world', (req, resp) => { resp.send('Hello world'); });
app.use((req, resp) => { resp.send('not found'); });
app.listen(port, () => console.log(`Example app listening on port ${port}!`));
Expected outcomes:
'/hello/world' => 'hello world'
'/hello/WORLD' => 'hello world'
'/HELLO/WORLD' => 'not found'
'/goodbye/world' => 'not found (hanging spider)'
|
|
||
| handler = spy((_req: Request, _resp: Response, next: NextCallback) => { next(); }); | ||
|
|
||
| // `/Hello/world` is case-sensitive |
There was a problem hiding this comment.
Same comments on this test (also, this comment is unclear).
aacfc53 to
dcc622e
Compare
|
Great feedback, thank you. After fixing the wording of the tests and associated comments, I saw that I had indeed implemented this incorrectly. Thank you! |
…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.
dcc622e to
d7dfe46
Compare
|
Ok, this should be good for re-review. I also added a comment to the |
|
I love how you still get penalized in this codebase for a PR that has a net reduction in the lines of code. 😆 |
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.