diff --git a/packages/rest/src/__tests__/unit/router/routing-table.unit.ts b/packages/rest/src/__tests__/unit/router/routing-table.unit.ts index b5b80b933c14..635d7dc39164 100644 --- a/packages/rest/src/__tests__/unit/router/routing-table.unit.ts +++ b/packages/rest/src/__tests__/unit/router/routing-table.unit.ts @@ -6,19 +6,19 @@ import {anOpenApiSpec} from '@loopback/openapi-spec-builder'; import {get, getControllerSpec, param} from '@loopback/openapi-v3'; import { - ShotRequestOptions, expect, + ShotRequestOptions, stubExpressContext, } from '@loopback/testlab'; +import * as HttpErrors from 'http-errors'; import { ControllerRoute, + RegExpRouter, Request, - RoutingTable, RestRouter, - RegExpRouter, + RoutingTable, TrieRouter, } from '../../..'; -import * as HttpErrors from 'http-errors'; describe('RoutingTable', () => { it('joins basePath and path', () => { @@ -220,10 +220,6 @@ function runTestsWithRouter(router: RestRouter) { async getOrderById(@param.path.number('id') id: number): Promise { return {id}; } - @get('/orders') - async findOrders(): Promise { - return []; - } // A path that overlaps with `/orders/{id}`. Please note a different var // name is used - `{orderId}` @get('/orders/{orderId}/shipments') @@ -232,6 +228,16 @@ function runTestsWithRouter(router: RestRouter) { ): Promise { return []; } + // With trailing `/` + @get('/orders/') + async findOrders(): Promise { + return []; + } + // Without trailing `/` + @get('/pendingOrders') + async findPendingOrders(): Promise { + return []; + } } const table = givenRoutingTable(); @@ -250,6 +256,9 @@ function runTestsWithRouter(router: RestRouter) { findAndCheckRoute('/orders/1', '/orders/{id}'); findAndCheckRoute('/orders/1/shipments', '/orders/{orderId}/shipments'); findAndCheckRoute('/orders', '/orders'); + findAndCheckRoute('/orders/', '/orders'); + findAndCheckRoute('/pendingOrders', '/pendingOrders'); + findAndCheckRoute('/pendingOrders/', '/pendingOrders'); }); it('throws if router is not found', () => { diff --git a/packages/rest/src/__tests__/unit/router/trie-router.unit.ts b/packages/rest/src/__tests__/unit/router/trie-router.unit.ts index ae02e14cdda0..9cf8eae767b2 100644 --- a/packages/rest/src/__tests__/unit/router/trie-router.unit.ts +++ b/packages/rest/src/__tests__/unit/router/trie-router.unit.ts @@ -10,6 +10,7 @@ import { stubExpressContext, } from '@loopback/testlab'; import {Request, ResolvedRoute, RouteEntry, TrieRouter} from '../../..'; +import {RestRouterOptions} from '../../../router'; class TestTrieRouter extends TrieRouter { get staticRoutes() { @@ -146,18 +147,6 @@ describe('trie router - overlapping paths with different var names', () => { }); }); - function getRouteInfo(r?: ResolvedRoute) { - return { - verb: r && r.verb, - path: r && r.path, - params: r && r.pathParams, - }; - } - - function givenRequest(options?: ShotRequestOptions): Request { - return stubExpressContext(options).request; - } - function givenRoutesWithDifferentVars() { const routes: RouteEntry[] = []; @@ -173,8 +162,104 @@ describe('trie router - overlapping paths with different var names', () => { } }); -function givenTrieRouter(routes: RouteEntry[]) { - const router = new TestTrieRouter(); +describe('trie router with options', () => { + let router: TestTrieRouter; + + describe('strict: true', () => { + before(givenStrictRouter); + it('finds route for GET /users/', () => { + testStrictRouter('/users/'); + }); + + it('fails to find route for GET /users', () => { + const req = givenRequest({method: 'get', url: '/users'}); + const route = router.find(req); + expect(route).to.be.undefined(); + }); + + it('finds route for GET /orders', () => { + testStrictRouter('/orders'); + }); + + it('fails to find route for GET /orders/', () => { + const req = givenRequest({method: 'get', url: '/orders/'}); + const route = router.find(req); + expect(route).to.be.undefined(); + }); + + function testStrictRouter(path: string) { + const req = givenRequest({method: 'get', url: path}); + const route = router.find(req); + expect(getRouteInfo(route)).to.containEql({ + verb: 'get', + path, + params: {}, + }); + } + + function givenStrictRouter() { + router = givenTrieRouter(givenRoutesWithDifferentVars(), {strict: true}); + } + }); + + describe('strict: false', () => { + before(givenNonStrictRouter); + it('finds route for GET /users/', () => { + testNonStrictRouter('/users/'); + }); + + it('finds route for GET /users', () => { + testNonStrictRouter('/users', '/users/'); + }); + + it('finds route for GET /orders/', () => { + testNonStrictRouter('/orders/', '/orders'); + }); + + it('finds route for GET /orders', () => { + testNonStrictRouter('/orders'); + }); + + function testNonStrictRouter(path: string, expected?: string) { + expected = expected || path; + const req = givenRequest({method: 'get', url: path}); + const route = router.find(req); + expect(getRouteInfo(route)).to.containEql({ + verb: 'get', + path: expected, + params: {}, + }); + } + + function givenNonStrictRouter() { + router = givenTrieRouter(givenRoutesWithDifferentVars(), {strict: false}); + } + }); + + function givenRoutesWithDifferentVars() { + const routes: RouteEntry[] = []; + + addRoute(routes, 'getUsers', 'get', '/users/'); + addRoute(routes, 'getOrders', 'get', '/orders'); + + return routes; + } +}); + +function getRouteInfo(r?: ResolvedRoute) { + return { + verb: r && r.verb, + path: r && r.path, + params: r && r.pathParams, + }; +} + +function givenRequest(options?: ShotRequestOptions): Request { + return stubExpressContext(options).request; +} + +function givenTrieRouter(routes: RouteEntry[], options?: RestRouterOptions) { + const router = new TestTrieRouter(options); for (const r of routes) { router.add(r); } diff --git a/packages/rest/src/keys.ts b/packages/rest/src/keys.ts index 3a35b3461173..d7ae46cbbe19 100644 --- a/packages/rest/src/keys.ts +++ b/packages/rest/src/keys.ts @@ -31,7 +31,7 @@ import { import {HttpProtocol} from '@loopback/http-server'; import * as https from 'https'; import {ErrorWriterOptions} from 'strong-error-handler'; -import {RestRouter} from './router'; +import {RestRouter, RestRouterOptions} from './router'; import {RequestBodyParser, BodyParser} from './body-parsers'; /** @@ -80,6 +80,10 @@ export namespace RestBindings { */ export const ROUTER = BindingKey.create('rest.router'); + export const ROUTER_OPTIONS = BindingKey.create( + 'rest.router.options', + ); + /** * Binding key for setting and injecting Reject action's error handling * options. diff --git a/packages/rest/src/rest.server.ts b/packages/rest/src/rest.server.ts index 3ed4dd1b4177..024ac3697ca5 100644 --- a/packages/rest/src/rest.server.ts +++ b/packages/rest/src/rest.server.ts @@ -42,6 +42,7 @@ import { RouteEntry, RoutingTable, StaticAssetsRoute, + RestRouterOptions, } from './router'; import {DefaultSequence, SequenceFunction, SequenceHandler} from './sequence'; import { @@ -194,6 +195,10 @@ export class RestServer extends Context implements Server, HttpServerLike { this.sequence(config.sequence); } + if (config.router) { + this.bind(RestBindings.ROUTER_OPTIONS).to(config.router); + } + this.basePath(config.basePath); this.bind(RestBindings.BASE_PATH).toDynamicValue(() => this._basePath); @@ -204,6 +209,9 @@ export class RestServer extends Context implements Server, HttpServerLike { if (this._expressApp) return; this._expressApp = express(); this._expressApp.set('query parser', 'extended'); + if (this.config.router && typeof this.config.router.strict === 'boolean') { + this._expressApp.set('strict routing', this.config.router.strict); + } this._requestHandler = this._expressApp; // Allow CORS support for all endpoints so that users @@ -937,6 +945,7 @@ export interface RestServerOptions { openApiSpec?: OpenApiSpecOptions; apiExplorer?: ApiExplorerOptions; sequence?: Constructor; + router?: RestRouterOptions; } /** diff --git a/packages/rest/src/router/regexp-router.ts b/packages/rest/src/router/regexp-router.ts index eaaf8864d002..e10ef3e0f411 100644 --- a/packages/rest/src/router/regexp-router.ts +++ b/packages/rest/src/router/regexp-router.ts @@ -3,13 +3,16 @@ // This file is licensed under the MIT License. // License text available at https://opensource.org/licenses/MIT -import {RouteEntry, createResolvedRoute, ResolvedRoute} from './route-entry'; -import {Request, PathParameterValues} from '../types'; +import {inject} from '@loopback/context'; import {inspect} from 'util'; +import {RestBindings} from '../keys'; +import {PathParameterValues, Request} from '../types'; +import {toExpressPath} from './openapi-path'; +import {RestRouterOptions} from './rest-router'; +import {createResolvedRoute, ResolvedRoute, RouteEntry} from './route-entry'; import {compareRoute} from './route-sort'; -import pathToRegExp = require('path-to-regexp'); import {BaseRouter} from './router-base'; -import {toExpressPath} from './openapi-path'; +import pathToRegExp = require('path-to-regexp'); const debug = require('debug')('loopback:rest:router:regexp'); @@ -36,25 +39,38 @@ export class RegExpRouter extends BaseRouter { } } + constructor( + @inject(RestBindings.ROUTER_OPTIONS, {optional: true}) + options?: RestRouterOptions, + ) { + super(options); + } + protected addRouteWithPathVars(route: RouteEntry) { const path = toExpressPath(route.path); const keys: pathToRegExp.Key[] = []; - const regexp = pathToRegExp(path, keys, {strict: false, end: true}); + const regexp = pathToRegExp(path, keys, { + strict: this.options.strict, + end: true, + }); const entry: RegExpRouteEntry = Object.assign(route, {keys, regexp}); this.routes.push(entry); this._sorted = false; } - protected findRouteWithPathVars(request: Request): ResolvedRoute | undefined { + protected findRouteWithPathVars( + verb: string, + path: string, + ): ResolvedRoute | undefined { this._sort(); for (const r of this.routes) { debug('trying endpoint %s', inspect(r, {depth: 5})); - if (r.verb !== request.method.toLowerCase()) { + if (r.verb !== verb.toLowerCase()) { debug(' -> verb mismatch'); continue; } - const match = r.regexp.exec(request.path); + const match = r.regexp.exec(path); if (!match) { debug(' -> path mismatch'); continue; diff --git a/packages/rest/src/router/rest-router.ts b/packages/rest/src/router/rest-router.ts index 3d6ac7e8fe48..0711f14662ba 100644 --- a/packages/rest/src/router/rest-router.ts +++ b/packages/rest/src/router/rest-router.ts @@ -29,3 +29,12 @@ export interface RestRouter { */ list(): RouteEntry[]; } + +export type RestRouterOptions = { + /** + * When true it allows an optional trailing slash to match. (default: false) + * + * See `strict routing` at http://expressjs.com/en/4x/api.html#app + */ + strict?: boolean; +}; diff --git a/packages/rest/src/router/router-base.ts b/packages/rest/src/router/router-base.ts index 482f12592cdf..58c862ff14c8 100644 --- a/packages/rest/src/router/router-base.ts +++ b/packages/rest/src/router/router-base.ts @@ -7,7 +7,7 @@ import {Request} from '../types'; import {getPathVariables} from './openapi-path'; import {createResolvedRoute, ResolvedRoute, RouteEntry} from './route-entry'; import {compareRoute} from './route-sort'; -import {RestRouter} from './rest-router'; +import {RestRouter, RestRouterOptions} from './rest-router'; /** * Base router implementation that only handles path without variables @@ -16,12 +16,12 @@ export abstract class BaseRouter implements RestRouter { /** * A map to optimize matching for routes without variables in the path */ - protected routesWithoutPathVars: {[path: string]: RouteEntry} = {}; + protected routesWithoutPathVars: {[key: string]: RouteEntry} = {}; + + constructor(protected options: RestRouterOptions = {strict: false}) {} protected getKeyForRoute(route: RouteEntry) { - const path = route.path.startsWith('/') ? route.path : `/${route.path}`; - const verb = route.verb.toLowerCase() || 'get'; - return `/${verb}${path}`; + return this.getKey(route.verb, route.path); } add(route: RouteEntry) { @@ -34,15 +34,33 @@ export abstract class BaseRouter implements RestRouter { } protected getKeyForRequest(request: Request) { - const method = request.method.toLowerCase(); - return `/${method}${request.path}`; + return this.getKey(request.method, request.path); } find(request: Request) { - const path = this.getKeyForRequest(request); - let route = this.routesWithoutPathVars[path]; + if (this.options.strict) { + return this.findRoute(request.method, request.path); + } + // Non-strict mode + let path = request.path; + // First try the exact match + let route = this.findRoute(request.method, path); + if (route || path === '/') return route; + if (path.endsWith('/')) { + // Fall back to the path without trailing slash + path = path.substring(0, path.length - 1); + } else { + // Fall back to the path with trailing slash + path = path + '/'; + } + return this.findRoute(request.method, path); + } + + private findRoute(verb: string, path: string) { + const key = this.getKey(verb, path); + let route = this.routesWithoutPathVars[key]; if (route) return createResolvedRoute(route, {}); - else return this.findRouteWithPathVars(request); + else return this.findRouteWithPathVars(verb, path); } list() { @@ -52,19 +70,31 @@ export abstract class BaseRouter implements RestRouter { return routes.sort(compareRoute); } + /** + * Build a key for verb+path as `//` + * @param verb HTTP verb/method + * @param path URL path + */ + protected getKey(verb: string, path: string) { + verb = normalizeVerb(verb); + path = normalizePath(path); + return `/${verb}${path}`; + } + // The following abstract methods need to be implemented by its subclasses /** * Add a route with path variables - * @param route + * @param route Route */ protected abstract addRouteWithPathVars(route: RouteEntry): void; /** - * Find a route with path variables - * @param route + * Find a route with path variables for a given request + * @param request Http request */ protected abstract findRouteWithPathVars( - request: Request, + verb: string, + path: string, ): ResolvedRoute | undefined; /** @@ -72,3 +102,23 @@ export abstract class BaseRouter implements RestRouter { */ protected abstract listRoutesWithPathVars(): RouteEntry[]; } + +/** + * Normalize http verb to lowercase + * @param verb Http verb + */ +function normalizeVerb(verb: string) { + // Use lower case, default to `get` + return (verb && verb.toLowerCase()) || 'get'; +} + +/** + * Normalize path to make sure it starts with `/` + * @param path Path + */ +function normalizePath(path: string) { + // Prepend `/` if needed + path = path || '/'; + path = path.startsWith('/') ? path : `/${path}`; + return path; +} diff --git a/packages/rest/src/router/trie-router.ts b/packages/rest/src/router/trie-router.ts index 7652b3693d3b..494e4ffa3634 100644 --- a/packages/rest/src/router/trie-router.ts +++ b/packages/rest/src/router/trie-router.ts @@ -3,11 +3,14 @@ // This file is licensed under the MIT License. // License text available at https://opensource.org/licenses/MIT -import {RouteEntry, createResolvedRoute} from './route-entry'; -import {Trie} from './trie'; -import {Request} from '../types'; +import {inject} from '@loopback/context'; import {inspect} from 'util'; +import {RestBindings} from '../keys'; +import {Request} from '../types'; +import {RestRouterOptions} from './rest-router'; +import {createResolvedRoute, RouteEntry} from './route-entry'; import {BaseRouter} from './router-base'; +import {Trie} from './trie'; const debug = require('debug')('loopback:rest:router:trie'); @@ -17,16 +20,23 @@ const debug = require('debug')('loopback:rest:router:trie'); export class TrieRouter extends BaseRouter { private trie = new Trie(); + constructor( + @inject(RestBindings.ROUTER_OPTIONS, {optional: true}) + options?: RestRouterOptions, + ) { + super(options); + } + protected addRouteWithPathVars(route: RouteEntry) { // Add the route to the trie const key = this.getKeyForRoute(route); this.trie.create(key, route); } - protected findRouteWithPathVars(request: Request) { - const path = this.getKeyForRequest(request); + protected findRouteWithPathVars(verb: string, path: string) { + const key = this.getKey(verb, path); - const found = this.trie.match(path); + const found = this.trie.match(key); debug('Route matched: %j', found); if (found) {