From 97bd2945c7238fd15434486433fa69fb605707b3 Mon Sep 17 00:00:00 2001 From: Hage Yaapa Date: Wed, 17 Oct 2018 19:08:46 +0530 Subject: [PATCH] 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 () => {