From 89e8361dbd433cf58700e7a9826868fabcc0fd2c Mon Sep 17 00:00:00 2001 From: Hage Yaapa Date: Mon, 15 Oct 2018 10:03:54 +0530 Subject: [PATCH 1/6] fix: optimize serving static files Optimize serving static files --- packages/rest/src/rest.server.ts | 28 ++++++++++++----- packages/rest/src/router/router-base.ts | 5 +++ packages/rest/src/router/routing-table.ts | 37 ++++++++++++++++++++--- 3 files changed, 58 insertions(+), 12 deletions(-) diff --git a/packages/rest/src/rest.server.ts b/packages/rest/src/rest.server.ts index 12d835f3b950..bce1b89ec757 100644 --- a/packages/rest/src/rest.server.ts +++ b/packages/rest/src/rest.server.ts @@ -33,6 +33,7 @@ import { Route, RouteEntry, RoutingTable, + StaticRoute, } from './router/routing-table'; import {DefaultSequence, SequenceFunction, SequenceHandler} from './sequence'; @@ -46,6 +47,7 @@ import { Send, } from './types'; import {ServerOptions} from 'https'; +import * as HttpErrors from 'http-errors'; const debug = require('debug')('loopback:rest:server'); @@ -190,6 +192,25 @@ export class RestServer extends Context implements Server, HttpServerLike { this._setupRequestHandler(); this.bind(RestBindings.HANDLER).toDynamicValue(() => this.httpHandler); + + // LB4's static assets serving router + const staticAssetsRouter = new StaticRoute( + (req: Request, res: Response) => { + return new Promise((resolve, reject) => { + const onFinished = () => resolve(); + res.once('finish', onFinished); + this._routerForStaticAssets.handle(req, res, (err: Error) => { + if (err) { + return reject(err); + } + // Express router called next, which means no route was matched + return reject(new HttpErrors.NotFound()); + }); + }); + }, + ); + + this.httpHandler.registerRoute(staticAssetsRouter); } protected _setupRequestHandler() { @@ -243,7 +264,6 @@ export class RestServer extends Context implements Server, HttpServerLike { protected _setupRouterForStaticAssets() { if (!this._routerForStaticAssets) { this._routerForStaticAssets = express.Router(); - this._expressApp.use(this._routerForStaticAssets); } } @@ -631,12 +651,6 @@ export class RestServer extends Context implements Server, HttpServerLike { * @param options Options for serve-static */ static(path: PathParams, rootDir: string, options?: ServeStaticOptions) { - const re = pathToRegExp(path, [], {end: false}); - if (re.test('/')) { - throw new Error( - 'Static assets cannot be mount to "/" to avoid performance penalty.', - ); - } this._routerForStaticAssets.use(path, express.static(rootDir, options)); } diff --git a/packages/rest/src/router/router-base.ts b/packages/rest/src/router/router-base.ts index cc600c07be6a..8738edc60871 100644 --- a/packages/rest/src/router/router-base.ts +++ b/packages/rest/src/router/router-base.ts @@ -49,6 +49,11 @@ export abstract class BaseRouter implements RestRouter { else return this.findRouteWithPathVars(request); } + getStaticAssetsRouter() { + const route = this.routesWithoutPathVars['/get/*']; + return createResolvedRoute(route, {}); + } + list() { let routes = Object.values(this.routesWithoutPathVars); routes = routes.concat(this.listRoutesWithPathVars()); diff --git a/packages/rest/src/router/routing-table.ts b/packages/rest/src/router/routing-table.ts index 59dbbcc6c4c3..f8393579458b 100644 --- a/packages/rest/src/router/routing-table.ts +++ b/packages/rest/src/router/routing-table.ts @@ -27,6 +27,8 @@ import { OperationRetval, } from '../types'; +import {RestBindings} from '../keys'; + import {ControllerSpec} from '@loopback/openapi-v3'; import * as assert from 'assert'; @@ -72,6 +74,8 @@ export interface RestRouter { */ find(request: Request): ResolvedRoute | undefined; + getStaticAssetsRouter(): ResolvedRoute; + /** * List all routes */ @@ -181,10 +185,8 @@ export class RoutingTable { return found; } - debug('No route found for %s %s', request.method, request.path); - throw new HttpErrors.NotFound( - `Endpoint "${request.method} ${request.path}" not found.`, - ); + const staticAssetsRouter = this._router.getStaticAssetsRouter(); + return staticAssetsRouter; } } @@ -239,6 +241,31 @@ export interface ResolvedRoute extends RouteEntry { readonly schemas: SchemasObject; } +export class StaticRoute implements RouteEntry { + public readonly verb: string = 'get'; + public readonly path: string = '*'; + public readonly spec: OperationObject = {responses: {}}; + + constructor(private _handler: Function) {} + + updateBindings(requestContext: Context): void { + // no-op + } + + async invokeHandler( + requestContext: Context, + args: OperationArgs, + ): Promise { + const req = await requestContext.get(RestBindings.Http.REQUEST); + const res = await requestContext.get(RestBindings.Http.RESPONSE); + return this._handler(req, res); + } + + describe(): string { + return 'final route to handle static assets'; + } +} + /** * Base implementation of RouteEntry */ @@ -291,7 +318,7 @@ export class Route extends BaseRoute { verb: string, path: string, public readonly spec: OperationObject, - protected readonly _handler: Function, + protected readonly _handler: Function, // <-- doesn't this Function have a signature? ) { super(verb, path, spec); } From 97bd2945c7238fd15434486433fa69fb605707b3 Mon Sep 17 00:00:00 2001 From: Hage Yaapa Date: Wed, 17 Oct 2018 19:08:46 +0530 Subject: [PATCH 2/6] fix: review 1 review 1 --- packages/rest/src/rest.server.ts | 44 +++----- packages/rest/src/router/router-base.ts | 5 - packages/rest/src/router/routing-table.ts | 103 ++++++++++++------ .../integration/rest.server.integration.ts | 38 ++----- 4 files changed, 97 insertions(+), 93 deletions(-) diff --git a/packages/rest/src/rest.server.ts b/packages/rest/src/rest.server.ts index bce1b89ec757..89b14a65bf72 100644 --- a/packages/rest/src/rest.server.ts +++ b/packages/rest/src/rest.server.ts @@ -18,7 +18,6 @@ import * as express from 'express'; import {PathParams} from 'express-serve-static-core'; import {IncomingMessage, ServerResponse} from 'http'; import {safeDump} from 'js-yaml'; -import * as pathToRegExp from 'path-to-regexp'; import {ServeStaticOptions} from 'serve-static'; import {HttpHandler} from './http-handler'; import {RestBindings} from './keys'; @@ -33,7 +32,7 @@ import { Route, RouteEntry, RoutingTable, - StaticRoute, + StaticAssetsRoute, } from './router/routing-table'; import {DefaultSequence, SequenceFunction, SequenceHandler} from './sequence'; @@ -47,7 +46,6 @@ import { Send, } from './types'; import {ServerOptions} from 'https'; -import * as HttpErrors from 'http-errors'; const debug = require('debug')('loopback:rest:server'); @@ -60,6 +58,10 @@ export interface HttpServerLike { requestHandler: HttpRequestListener; } +interface ExpressRouter extends express.Router { + handle: express.RequestHandler; +} + const SequenceActions = RestBindings.SequenceActions; // NOTE(bajtos) we cannot use `import * as cloneDeep from 'lodash/cloneDeep' @@ -129,7 +131,7 @@ export class RestServer extends Context implements Server, HttpServerLike { protected _httpServer: HttpServer | undefined; protected _expressApp: express.Application; - protected _routerForStaticAssets: express.Router; + protected _routerForStaticAssets: ExpressRouter; get listening(): boolean { return this._httpServer ? this._httpServer.listening : false; @@ -192,25 +194,6 @@ export class RestServer extends Context implements Server, HttpServerLike { this._setupRequestHandler(); this.bind(RestBindings.HANDLER).toDynamicValue(() => this.httpHandler); - - // LB4's static assets serving router - const staticAssetsRouter = new StaticRoute( - (req: Request, res: Response) => { - return new Promise((resolve, reject) => { - const onFinished = () => resolve(); - res.once('finish', onFinished); - this._routerForStaticAssets.handle(req, res, (err: Error) => { - if (err) { - return reject(err); - } - // Express router called next, which means no route was matched - return reject(new HttpErrors.NotFound()); - }); - }); - }, - ); - - this.httpHandler.registerRoute(staticAssetsRouter); } protected _setupRequestHandler() { @@ -238,9 +221,6 @@ export class RestServer extends Context implements Server, HttpServerLike { }; this._expressApp.use(cors(corsOptions)); - // Place the assets router here before controllers - this._setupRouterForStaticAssets(); - // Set up endpoints for OpenAPI spec/ui this._setupOpenApiSpecEndpoints(); @@ -255,6 +235,9 @@ export class RestServer extends Context implements Server, HttpServerLike { this._onUnhandledError(req, res, err); }, ); + + // LB4's static assets serving router + this._setupRouterForStaticAssets(); } /** @@ -263,7 +246,13 @@ export class RestServer extends Context implements Server, HttpServerLike { */ protected _setupRouterForStaticAssets() { if (!this._routerForStaticAssets) { - this._routerForStaticAssets = express.Router(); + this._routerForStaticAssets = express.Router() as ExpressRouter; + + const staticAssetsRouter = new StaticAssetsRoute( + this._routerForStaticAssets, + ); + + this.route(staticAssetsRouter); } } @@ -646,7 +635,6 @@ export class RestServer extends Context implements Server, HttpServerLike { * See https://expressjs.com/en/4x/api.html#express.static * @param path The path(s) to serve the asset. * See examples at https://expressjs.com/en/4x/api.html#path-examples - * To avoid performance penalty, `/` is not allowed for now. * @param rootDir The root directory from which to serve static assets * @param options Options for serve-static */ diff --git a/packages/rest/src/router/router-base.ts b/packages/rest/src/router/router-base.ts index 8738edc60871..cc600c07be6a 100644 --- a/packages/rest/src/router/router-base.ts +++ b/packages/rest/src/router/router-base.ts @@ -49,11 +49,6 @@ export abstract class BaseRouter implements RestRouter { else return this.findRouteWithPathVars(request); } - getStaticAssetsRouter() { - const route = this.routesWithoutPathVars['/get/*']; - return createResolvedRoute(route, {}); - } - list() { let routes = Object.values(this.routesWithoutPathVars); routes = routes.concat(this.listRoutesWithPathVars()); diff --git a/packages/rest/src/router/routing-table.ts b/packages/rest/src/router/routing-table.ts index f8393579458b..a6624bf7d7d9 100644 --- a/packages/rest/src/router/routing-table.ts +++ b/packages/rest/src/router/routing-table.ts @@ -21,13 +21,14 @@ import * as HttpErrors from 'http-errors'; import {inspect} from 'util'; import { + Response, Request, PathParameterValues, OperationArgs, OperationRetval, } from '../types'; -import {RestBindings} from '../keys'; +import * as express from 'express'; import {ControllerSpec} from '@loopback/openapi-v3'; @@ -37,6 +38,11 @@ const debug = require('debug')('loopback:rest:routing-table'); import {CoreBindings} from '@loopback/core'; import {validateApiPath} from './openapi-path'; import {TrieRouter} from './trie-router'; +import {RequestContext} from '../request-context'; + +export interface ExpressRouter extends express.Router { + handle: express.RequestHandler; +} /** * A controller instance with open properties/methods @@ -74,8 +80,6 @@ export interface RestRouter { */ find(request: Request): ResolvedRoute | undefined; - getStaticAssetsRouter(): ResolvedRoute; - /** * List all routes */ @@ -88,6 +92,8 @@ export interface RestRouter { export class RoutingTable { constructor(private readonly _router: RestRouter = new TrieRouter()) {} + private _staticAssetsRoute: ResolvedRoute; + /** * Register a controller as the route * @param spec @@ -154,6 +160,9 @@ export class RoutingTable { validateApiPath(route.path); this._router.add(route); + if (route.path === '/*') { + this._staticAssetsRoute = createResolvedRoute(route, {}); + } } describeApiPaths(): PathObject { @@ -185,8 +194,21 @@ export class RoutingTable { return found; } - const staticAssetsRouter = this._router.getStaticAssetsRouter(); - return staticAssetsRouter; + debug( + 'No API route found for %s %s, trying to find a static asset', + request.method, + request.path, + ); + + // this._staticAssetsRoute will be set only if app.static() was called + if (this._staticAssetsRoute) { + return this._staticAssetsRoute; + } + + debug('No static asset found for %s %s', request.method, request.path); + throw new HttpErrors.NotFound( + `Endpoint "${request.method} ${request.path}" not found.`, + ); } } @@ -241,31 +263,6 @@ export interface ResolvedRoute extends RouteEntry { readonly schemas: SchemasObject; } -export class StaticRoute implements RouteEntry { - public readonly verb: string = 'get'; - public readonly path: string = '*'; - public readonly spec: OperationObject = {responses: {}}; - - constructor(private _handler: Function) {} - - updateBindings(requestContext: Context): void { - // no-op - } - - async invokeHandler( - requestContext: Context, - args: OperationArgs, - ): Promise { - const req = await requestContext.get(RestBindings.Http.REQUEST); - const res = await requestContext.get(RestBindings.Http.RESPONSE); - return this._handler(req, res); - } - - describe(): string { - return 'final route to handle static assets'; - } -} - /** * Base implementation of RouteEntry */ @@ -298,6 +295,50 @@ export abstract class BaseRoute implements RouteEntry { } } +export class StaticAssetsRoute implements RouteEntry { + public readonly verb: string = 'get'; + public readonly path: string = '/*'; + public readonly spec: OperationObject = { + description: 'LoopBack static assets route', + 'x-visibility': 'undocumented', + responses: {}, + }; + private _handler: Function; + constructor(private _routerForStaticAssets: ExpressRouter) { + this._handler = (request: Request, response: Response) => { + return new Promise((resolve, reject) => { + const onFinished = () => resolve(); + response.once('finish', onFinished); + this._routerForStaticAssets.handle(request, response, (err: Error) => { + if (err) { + reject(err); + } else { + // Express router called next, which means no route was matched + reject( + new HttpErrors.NotFound( + `Endpoint "${request.method} ${request.path}" not found.`, + ), + ); + } + }); + }); + }; + } + updateBindings(requestContext: Context): void { + // no-op + } + async invokeHandler( + requestContext: RequestContext, + args: OperationArgs, + ): Promise { + const {request, response} = requestContext; + return this._handler(request, response); + } + describe(): string { + return 'final route to handle static assets'; + } +} + export function createResolvedRoute( route: RouteEntry, pathParams: PathParameterValues, @@ -318,7 +359,7 @@ export class Route extends BaseRoute { verb: string, path: string, public readonly spec: OperationObject, - protected readonly _handler: Function, // <-- doesn't this Function have a signature? + protected readonly _handler: Function, ) { super(verb, path, spec); } diff --git a/packages/rest/test/integration/rest.server.integration.ts b/packages/rest/test/integration/rest.server.integration.ts index 99a6df3c586a..21ddeb21096a 100644 --- a/packages/rest/test/integration/rest.server.integration.ts +++ b/packages/rest/test/integration/rest.server.integration.ts @@ -80,7 +80,7 @@ describe('RestServer (integration)', () => { .expect(500); }); - it('does not allow static assets to be mounted at /', async () => { + it('allows static assets to be mounted at /', async () => { const root = FIXTURES; const server = await givenAServer({ rest: { @@ -88,29 +88,12 @@ describe('RestServer (integration)', () => { }, }); - expect(() => server.static('/', root)).to.throw( - 'Static assets cannot be mount to "/" to avoid performance penalty.', - ); - - expect(() => server.static('', root)).to.throw( - 'Static assets cannot be mount to "/" to avoid performance penalty.', - ); - - expect(() => server.static(['/'], root)).to.throw( - 'Static assets cannot be mount to "/" to avoid performance penalty.', - ); - - expect(() => server.static(['/html', ''], root)).to.throw( - 'Static assets cannot be mount to "/" to avoid performance penalty.', - ); - - expect(() => server.static(/.*/, root)).to.throw( - 'Static assets cannot be mount to "/" to avoid performance penalty.', - ); - - expect(() => server.static('/(.*)', root)).to.throw( - 'Static assets cannot be mount to "/" to avoid performance penalty.', - ); + expect(() => server.static('/', root)).to.not.throw(); + expect(() => server.static('', root)).to.not.throw(); + expect(() => server.static(['/'], root)).to.not.throw(); + expect(() => server.static(['/html', ''], root)).to.not.throw(); + expect(() => server.static(/.*/, root)).to.not.throw(); + expect(() => server.static('/(.*)', root)).to.not.throw(); }); it('allows static assets via api', async () => { @@ -164,7 +147,7 @@ describe('RestServer (integration)', () => { .expect(200, 'Hello'); }); - it('serve static assets if matches before other routes', async () => { + it('does not serve static assets even if matches before other routes', async () => { const root = FIXTURES; const server = await givenAServer({ rest: { @@ -174,12 +157,9 @@ describe('RestServer (integration)', () => { server.static('/html', root); server.handler(dummyRequestHandler); - const content = fs - .readFileSync(path.join(root, 'index.html')) - .toString('utf-8'); await createClientForHandler(server.requestHandler) .get('/html/index.html') - .expect(200, content); + .expect(200, 'Hello'); }); it('allows cors', async () => { From a13be07241de98595a35f63e38a553bc76c14df3 Mon Sep 17 00:00:00 2001 From: Hage Yaapa Date: Thu, 25 Oct 2018 15:13:52 +0530 Subject: [PATCH 3/6] fix: review 2 review 2 --- packages/rest/src/rest.server.ts | 8 +-- packages/rest/src/router/routing-table.ts | 55 +++++++++---------- .../integration/rest.server.integration.ts | 2 +- 3 files changed, 28 insertions(+), 37 deletions(-) diff --git a/packages/rest/src/rest.server.ts b/packages/rest/src/rest.server.ts index 89b14a65bf72..9ddef5d1a15c 100644 --- a/packages/rest/src/rest.server.ts +++ b/packages/rest/src/rest.server.ts @@ -58,10 +58,6 @@ export interface HttpServerLike { requestHandler: HttpRequestListener; } -interface ExpressRouter extends express.Router { - handle: express.RequestHandler; -} - const SequenceActions = RestBindings.SequenceActions; // NOTE(bajtos) we cannot use `import * as cloneDeep from 'lodash/cloneDeep' @@ -131,7 +127,7 @@ export class RestServer extends Context implements Server, HttpServerLike { protected _httpServer: HttpServer | undefined; protected _expressApp: express.Application; - protected _routerForStaticAssets: ExpressRouter; + protected _routerForStaticAssets: express.Router; get listening(): boolean { return this._httpServer ? this._httpServer.listening : false; @@ -246,7 +242,7 @@ export class RestServer extends Context implements Server, HttpServerLike { */ protected _setupRouterForStaticAssets() { if (!this._routerForStaticAssets) { - this._routerForStaticAssets = express.Router() as ExpressRouter; + this._routerForStaticAssets = express.Router(); const staticAssetsRouter = new StaticAssetsRoute( this._routerForStaticAssets, diff --git a/packages/rest/src/router/routing-table.ts b/packages/rest/src/router/routing-table.ts index a6624bf7d7d9..d21bae78be68 100644 --- a/packages/rest/src/router/routing-table.ts +++ b/packages/rest/src/router/routing-table.ts @@ -40,10 +40,6 @@ import {validateApiPath} from './openapi-path'; import {TrieRouter} from './trie-router'; import {RequestContext} from '../request-context'; -export interface ExpressRouter extends express.Router { - handle: express.RequestHandler; -} - /** * A controller instance with open properties/methods */ @@ -159,9 +155,11 @@ export class RoutingTable { } validateApiPath(route.path); - this._router.add(route); + if (route.path === '/*') { this._staticAssetsRoute = createResolvedRoute(route, {}); + } else { + this._router.add(route); } } @@ -303,36 +301,33 @@ export class StaticAssetsRoute implements RouteEntry { 'x-visibility': 'undocumented', responses: {}, }; - private _handler: Function; - constructor(private _routerForStaticAssets: ExpressRouter) { - this._handler = (request: Request, response: Response) => { - return new Promise((resolve, reject) => { - const onFinished = () => resolve(); - response.once('finish', onFinished); - this._routerForStaticAssets.handle(request, response, (err: Error) => { - if (err) { - reject(err); - } else { - // Express router called next, which means no route was matched - reject( - new HttpErrors.NotFound( - `Endpoint "${request.method} ${request.path}" not found.`, - ), - ); - } - }); - }); - }; - } + + constructor(private _routerForStaticAssets: express.Router) {} updateBindings(requestContext: Context): void { // no-op } - async invokeHandler( - requestContext: RequestContext, + + invokeHandler( + {request, response}: RequestContext, args: OperationArgs, ): Promise { - const {request, response} = requestContext; - return this._handler(request, response); + return new Promise((resolve, reject) => { + const onceFinished = () => resolve(); + response.once('finish', onceFinished); + this._routerForStaticAssets(request, response, (err: Error) => { + response.removeListener('finish', onceFinished); + if (err) { + reject(err); + } else { + // Express router called next, which means no route was matched + reject( + new HttpErrors.NotFound( + `Endpoint "${request.method} ${request.path}" not found.`, + ), + ); + } + }); + }); } describe(): string { return 'final route to handle static assets'; diff --git a/packages/rest/test/integration/rest.server.integration.ts b/packages/rest/test/integration/rest.server.integration.ts index 21ddeb21096a..8933871f56a5 100644 --- a/packages/rest/test/integration/rest.server.integration.ts +++ b/packages/rest/test/integration/rest.server.integration.ts @@ -147,7 +147,7 @@ describe('RestServer (integration)', () => { .expect(200, 'Hello'); }); - it('does not serve static assets even if matches before other routes', async () => { + it('gives precedence to API routes over static assets', async () => { const root = FIXTURES; const server = await givenAServer({ rest: { From f516b31f5067a09928ced0201d2169a1a73c33a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Fri, 26 Oct 2018 10:18:07 +0200 Subject: [PATCH 4/6] fixup! cleanup --- packages/rest/src/http-handler.ts | 10 ++ packages/rest/src/rest.server.ts | 29 +----- packages/rest/src/router/routing-table.ts | 109 ++++++++++++++++------ 3 files changed, 93 insertions(+), 55 deletions(-) diff --git a/packages/rest/src/http-handler.ts b/packages/rest/src/http-handler.ts index 7fe5d9d5dbdc..bf705bbdbbf4 100644 --- a/packages/rest/src/http-handler.ts +++ b/packages/rest/src/http-handler.ts @@ -19,6 +19,8 @@ import {Request, Response} from './types'; import {RestBindings} from './keys'; import {RequestContext} from './request-context'; +import {PathParams} from 'express-serve-static-core'; +import {ServeStaticOptions} from 'serve-static'; export class HttpHandler { protected _apiDefinitions: SchemasObject; @@ -48,6 +50,14 @@ export class HttpHandler { this._apiDefinitions = Object.assign({}, this._apiDefinitions, defs); } + registerStaticAssets( + path: PathParams, + rootDir: string, + options?: ServeStaticOptions, + ) { + this._routes.registerStaticAssets(path, rootDir, options); + } + getApiDefinitions() { return this._apiDefinitions; } diff --git a/packages/rest/src/rest.server.ts b/packages/rest/src/rest.server.ts index 9ddef5d1a15c..3e698daad76d 100644 --- a/packages/rest/src/rest.server.ts +++ b/packages/rest/src/rest.server.ts @@ -14,9 +14,11 @@ import { } from '@loopback/openapi-v3-types'; import {AssertionError} from 'assert'; import * as cors from 'cors'; +import * as debugFactory from 'debug'; import * as express from 'express'; import {PathParams} from 'express-serve-static-core'; import {IncomingMessage, ServerResponse} from 'http'; +import {ServerOptions} from 'https'; import {safeDump} from 'js-yaml'; import {ServeStaticOptions} from 'serve-static'; import {HttpHandler} from './http-handler'; @@ -32,9 +34,7 @@ import { Route, RouteEntry, RoutingTable, - StaticAssetsRoute, } from './router/routing-table'; - import {DefaultSequence, SequenceFunction, SequenceHandler} from './sequence'; import { FindRoute, @@ -45,9 +45,8 @@ import { Response, Send, } from './types'; -import {ServerOptions} from 'https'; -const debug = require('debug')('loopback:rest:server'); +const debug = debugFactory('loopback:rest:server'); export type HttpRequestListener = ( req: IncomingMessage, @@ -127,7 +126,6 @@ export class RestServer extends Context implements Server, HttpServerLike { protected _httpServer: HttpServer | undefined; protected _expressApp: express.Application; - protected _routerForStaticAssets: express.Router; get listening(): boolean { return this._httpServer ? this._httpServer.listening : false; @@ -231,25 +229,6 @@ export class RestServer extends Context implements Server, HttpServerLike { this._onUnhandledError(req, res, err); }, ); - - // LB4's static assets serving router - this._setupRouterForStaticAssets(); - } - - /** - * Set up an express router for all static assets so that middleware for - * all directories are invoked at the same phase - */ - protected _setupRouterForStaticAssets() { - if (!this._routerForStaticAssets) { - this._routerForStaticAssets = express.Router(); - - const staticAssetsRouter = new StaticAssetsRoute( - this._routerForStaticAssets, - ); - - this.route(staticAssetsRouter); - } } /** @@ -635,7 +614,7 @@ export class RestServer extends Context implements Server, HttpServerLike { * @param options Options for serve-static */ static(path: PathParams, rootDir: string, options?: ServeStaticOptions) { - this._routerForStaticAssets.use(path, express.static(rootDir, options)); + this.httpHandler.registerStaticAssets(path, rootDir, options); } /** diff --git a/packages/rest/src/router/routing-table.ts b/packages/rest/src/router/routing-table.ts index d21bae78be68..0c230a3452c3 100644 --- a/packages/rest/src/router/routing-table.ts +++ b/packages/rest/src/router/routing-table.ts @@ -21,7 +21,6 @@ import * as HttpErrors from 'http-errors'; import {inspect} from 'util'; import { - Response, Request, PathParameterValues, OperationArgs, @@ -39,6 +38,8 @@ import {CoreBindings} from '@loopback/core'; import {validateApiPath} from './openapi-path'; import {TrieRouter} from './trie-router'; import {RequestContext} from '../request-context'; +import {ServeStaticOptions} from 'serve-static'; +import {PathParams} from 'express-serve-static-core'; /** * A controller instance with open properties/methods @@ -88,7 +89,18 @@ export interface RestRouter { export class RoutingTable { constructor(private readonly _router: RestRouter = new TrieRouter()) {} - private _staticAssetsRoute: ResolvedRoute; + private _staticAssetsRoute: StaticAssetsRoute; + + registerStaticAssets( + path: PathParams, + rootDir: string, + options?: ServeStaticOptions, + ) { + if (!this._staticAssetsRoute) { + this._staticAssetsRoute = new StaticAssetsRoute(); + } + this._staticAssetsRoute.registerAssets(path, rootDir, options); + } /** * Register a controller as the route @@ -156,11 +168,7 @@ export class RoutingTable { validateApiPath(route.path); - if (route.path === '/*') { - this._staticAssetsRoute = createResolvedRoute(route, {}); - } else { - this._router.add(route); - } + this._router.add(route); } describeApiPaths(): PathObject { @@ -293,47 +301,88 @@ export abstract class BaseRoute implements RouteEntry { } } -export class StaticAssetsRoute implements RouteEntry { - public readonly verb: string = 'get'; - public readonly path: string = '/*'; - public readonly spec: OperationObject = { +export class StaticAssetsRoute implements RouteEntry, ResolvedRoute { + // ResolvedRoute API + readonly pathParams: PathParameterValues = []; + readonly schemas: SchemasObject = {}; + + // RouteEntry implementation + readonly verb: string = 'get'; + readonly path: string = '/*'; + readonly spec: OperationObject = { description: 'LoopBack static assets route', 'x-visibility': 'undocumented', responses: {}, }; - constructor(private _routerForStaticAssets: express.Router) {} + private readonly _expressRouter: express.Router = express.Router(); + + public registerAssets( + path: PathParams, + rootDir: string, + options?: ServeStaticOptions, + ) { + this._expressRouter.use(path, express.static(rootDir, options)); + } + updateBindings(requestContext: Context): void { // no-op } - invokeHandler( + async invokeHandler( {request, response}: RequestContext, args: OperationArgs, ): Promise { - return new Promise((resolve, reject) => { - const onceFinished = () => resolve(); - response.once('finish', onceFinished); - this._routerForStaticAssets(request, response, (err: Error) => { - response.removeListener('finish', onceFinished); - if (err) { - reject(err); - } else { - // Express router called next, which means no route was matched - reject( - new HttpErrors.NotFound( - `Endpoint "${request.method} ${request.path}" not found.`, - ), - ); - } - }); - }); + const handled = await executeRequestHandler( + this._expressRouter, + request, + response, + ); + + if (!handled) { + // Express router called next, which means no route was matched + throw new HttpErrors.NotFound( + `Endpoint "${request.method} ${request.path}" not found.`, + ); + } } + describe(): string { return 'final route to handle static assets'; } } +/** + * Execute an Express-style callback-based request handler. + * + * @param handler + * @param request + * @param response + * @returns A promise resolved to: + * - `true` when the request was handled + * - `false` when the handler called `next()` to proceed to the next + * handler (middleware) in the chain. + */ +function executeRequestHandler( + handler: express.RequestHandler, + request: Request, + response: express.Response, +): Promise { + return new Promise((resolve, reject) => { + const onceFinished = () => resolve(true); + response.once('finish', onceFinished); + handler(request, response, (err: Error) => { + response.removeListener('finish', onceFinished); + if (err) { + reject(err); + } else { + // Express router called next, which means no route was matched + resolve(false); + } + }); + }); +} + export function createResolvedRoute( route: RouteEntry, pathParams: PathParameterValues, From 79b268c4fc54eb03b96c744bae7d9c89651a9d0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Fri, 26 Oct 2018 10:21:35 +0200 Subject: [PATCH 5/6] fixup! improve debug logs --- packages/rest/src/router/routing-table.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/rest/src/router/routing-table.ts b/packages/rest/src/router/routing-table.ts index 0c230a3452c3..51ca3aa01b31 100644 --- a/packages/rest/src/router/routing-table.ts +++ b/packages/rest/src/router/routing-table.ts @@ -200,18 +200,18 @@ export class RoutingTable { return found; } - debug( - 'No API route found for %s %s, trying to find a static asset', - request.method, - request.path, - ); - // this._staticAssetsRoute will be set only if app.static() was called if (this._staticAssetsRoute) { + debug( + 'No API route found for %s %s, trying to find a static asset', + request.method, + request.path, + ); + return this._staticAssetsRoute; } - debug('No static asset found for %s %s', request.method, request.path); + debug('No route found for %s %s', request.method, request.path); throw new HttpErrors.NotFound( `Endpoint "${request.method} ${request.path}" not found.`, ); From 17deb274ce458d6ef78edf32a1a7c77a22fe6709 Mon Sep 17 00:00:00 2001 From: Hage Yaapa Date: Fri, 26 Oct 2018 19:44:48 +0530 Subject: [PATCH 6/6] fix: tests for RestApplication Tests for RestApplication --- .../rest.application.integration.ts | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 packages/rest/test/integration/rest.application.integration.ts diff --git a/packages/rest/test/integration/rest.application.integration.ts b/packages/rest/test/integration/rest.application.integration.ts new file mode 100644 index 000000000000..50577c5e4674 --- /dev/null +++ b/packages/rest/test/integration/rest.application.integration.ts @@ -0,0 +1,48 @@ +// Copyright IBM Corp. 2017,2018. All Rights Reserved. +// Node module: @loopback/rest +// This file is licensed under the MIT License. +// License text available at https://opensource.org/licenses/MIT + +import {createRestAppClient, Client} from '@loopback/testlab'; +import {RestApplication} from '../..'; +import * as path from 'path'; +import * as fs from 'fs'; + +const FIXTURES = path.resolve(__dirname, '../../../fixtures'); + +describe('RestApplication (integration)', () => { + let restApp: RestApplication; + let client: Client; + + beforeEach(givenAnApplication); + beforeEach(givenClient); + afterEach(async () => { + await restApp.stop(); + }); + + it('serves static assets', async () => { + const root = FIXTURES; + const content = fs + .readFileSync(path.join(root, 'index.html')) + .toString('utf-8'); + await client + .get('/index.html') + .expect(200) + .expect(content); + }); + + it('returns 404 if asset is not found', async () => { + await client.get('/404.html').expect(404); + }); + + function givenAnApplication() { + const root = FIXTURES; + restApp = new RestApplication({rest: {port: 0, host: '127.0.0.1'}}); + restApp.static('/', root); + } + + async function givenClient() { + await restApp.start(); + return (client = createRestAppClient(restApp)); + } +});