fix: fix static assets router blocking controller registration

Fix static assets router blocking registration of controllers
loopbackio · Nov 8, 2018 · 1b725b3 · 1b725b3
1 parent e431178
commit 1b725b3
Show file tree

Hide file tree

Showing 10 changed files with 141 additions and 43 deletions.
diff --git a/packages/rest/fixtures/index.html → packages/rest/fixtures/assets/index.html b/packages/rest/fixtures/index.html → packages/rest/fixtures/assets/index.html
diff --git a/packages/rest/fixtures/other-assets/index.html b/packages/rest/fixtures/other-assets/index.html
@@ -0,0 +1,8 @@
+<html>
+  <header>
+    <title>Other Assets</title>
+  </header>
+  <body>
+    <h1>Hello, World!</h1>
+  </body>
+</html>
diff --git a/packages/rest/fixtures/other-assets/robots.txt b/packages/rest/fixtures/other-assets/robots.txt
@@ -0,0 +1,2 @@
+User-agent: *
+Allow: /
diff --git a/packages/rest/src/http-handler.ts b/packages/rest/src/http-handler.ts
@@ -19,8 +19,6 @@ 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;
@@ -50,14 +48,6 @@ 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
@@ -34,6 +34,7 @@ import {
   Route,
   RouteEntry,
   RoutingTable,
+  StaticAssetsRoute,
 } from './router';
 import {DefaultSequence, SequenceFunction, SequenceHandler} from './sequence';
 import {
@@ -270,7 +271,7 @@ export class RestServer extends Context implements Server, HttpServerLike {
      * Check if there is custom router in the context
      */
     const router = this.getSync(RestBindings.ROUTER, {optional: true});
-    const routingTable = new RoutingTable(router);
+    const routingTable = new RoutingTable(router, this._staticAssetRoute);
 
     this._httpHandler = new HttpHandler(this, routingTable);
     for (const b of this.find('controllers.*')) {
@@ -605,6 +606,9 @@ export class RestServer extends Context implements Server, HttpServerLike {
     );
   }
 
+  // The route for static assets
+  private _staticAssetRoute = new StaticAssetsRoute();
+
   /**
    * Mount static assets to the REST server.
    * See https://expressjs.com/en/4x/api.html#express.static
@@ -614,7 +618,7 @@ export class RestServer extends Context implements Server, HttpServerLike {
    * @param options Options for serve-static
    */
   static(path: PathParams, rootDir: string, options?: ServeStaticOptions) {
-    this.httpHandler.registerStaticAssets(path, rootDir, options);
+    this._staticAssetRoute.registerAssets(path, rootDir, options);
   }
 
   /**

diff --git a/packages/rest/src/router/routing-table.ts b/packages/rest/src/router/routing-table.ts
@@ -11,9 +11,7 @@ import {
 } from '@loopback/openapi-v3-types';
 import * as assert from 'assert';
 import * as debugFactory from 'debug';
-import {PathParams} from 'express-serve-static-core';
 import * as HttpErrors from 'http-errors';
-import {ServeStaticOptions} from 'serve-static';
 import {inspect} from 'util';
 import {Request} from '../types';
 import {
@@ -33,19 +31,17 @@ const debug = debugFactory('loopback:rest:routing-table');
  * Routing table
  */
 export class RoutingTable {
-  constructor(private readonly _router: RestRouter = new TrieRouter()) {}
-
+  /**
+   * A route for static assets
+   */
   private _staticAssetsRoute: StaticAssetsRoute;
-
-  registerStaticAssets(
-    path: PathParams,
-    rootDir: string,
-    options?: ServeStaticOptions,
+  constructor(
+    private readonly _router: RestRouter = new TrieRouter(),
+    staticAssetsRoute?: StaticAssetsRoute,
   ) {
-    if (!this._staticAssetsRoute) {
-      this._staticAssetsRoute = new StaticAssetsRoute();
+    if (staticAssetsRoute) {
+      this._staticAssetsRoute = staticAssetsRoute;
     }
-    this._staticAssetsRoute.registerAssets(path, rootDir, options);
   }
 
   /**

diff --git a/packages/rest/src/router/static-assets-route.ts b/packages/rest/src/router/static-assets-route.ts
@@ -5,7 +5,7 @@
 
 import {Context} from '@loopback/context';
 import {OperationObject, SchemasObject} from '@loopback/openapi-v3-types';
-import * as express from 'express';
+import {Router, RequestHandler, static as serveStatic} from 'express';
 import {PathParams} from 'express-serve-static-core';
 import * as HttpErrors from 'http-errors';
 import {ServeStaticOptions} from 'serve-static';
@@ -15,6 +15,7 @@ import {
   OperationRetval,
   PathParameterValues,
   Request,
+  Response,
 } from '../types';
 import {ResolvedRoute, RouteEntry} from './route-entry';
 
@@ -32,14 +33,14 @@ export class StaticAssetsRoute implements RouteEntry, ResolvedRoute {
     responses: {},
   };
 
-  private readonly _expressRouter: express.Router = express.Router();
+  constructor(private readonly _expressRouter: Router = Router()) {}
 
   public registerAssets(
     path: PathParams,
     rootDir: string,
     options?: ServeStaticOptions,
   ) {
-    this._expressRouter.use(path, express.static(rootDir, options));
+    this._expressRouter.use(path, serveStatic(rootDir, options));
   }
 
   updateBindings(requestContext: Context): void {
@@ -81,9 +82,9 @@ export class StaticAssetsRoute implements RouteEntry, ResolvedRoute {
  *    handler (middleware) in the chain.
  */
 function executeRequestHandler(
-  handler: express.RequestHandler,
+  handler: RequestHandler,
   request: Request,
-  response: express.Response,
+  response: Response,
 ): Promise<boolean> {
   return new Promise((resolve, reject) => {
     const onceFinished = () => resolve(true);

diff --git a/packages/rest/test/integration/rest.application.integration.ts b/packages/rest/test/integration/rest.application.integration.ts
@@ -9,7 +9,7 @@ import * as path from 'path';
 import * as fs from 'fs';
 import {RestServer, RestServerConfig} from '../../src';
 
-const FIXTURES = path.resolve(__dirname, '../../../fixtures');
+const ASSETS = path.resolve(__dirname, '../../../fixtures/assets');
 
 describe('RestApplication (integration)', () => {
   let restApp: RestApplication;
@@ -23,10 +23,10 @@ describe('RestApplication (integration)', () => {
 
   it('serves static assets from root path', async () => {
     givenApplication();
-    restApp.static('/', FIXTURES);
+    restApp.static('/', ASSETS);
     await restApp.start();
     const content = fs
-      .readFileSync(path.join(FIXTURES, 'index.html'))
+      .readFileSync(path.join(ASSETS, '', 'index.html'))
       .toString('utf-8');
     client = createRestAppClient(restApp);
     await client
@@ -37,10 +37,10 @@ describe('RestApplication (integration)', () => {
 
   it('serves static assets from non-root path', async () => {
     givenApplication();
-    restApp.static('/public', FIXTURES);
+    restApp.static('/public', ASSETS);
     await restApp.start();
     const content = fs
-      .readFileSync(path.join(FIXTURES, 'index.html'))
+      .readFileSync(path.join(ASSETS, 'index.html'))
       .toString('utf-8');
     client = createRestAppClient(restApp);
     await client
@@ -51,7 +51,7 @@ describe('RestApplication (integration)', () => {
 
   it('returns 404 if asset is not found', async () => {
     givenApplication();
-    restApp.static('/', FIXTURES);
+    restApp.static('/', ASSETS);
     await restApp.start();
     client = createRestAppClient(restApp);
     await client.get('/404.html').expect(404);
@@ -60,9 +60,9 @@ describe('RestApplication (integration)', () => {
   it('allows static assets via api after start', async () => {
     givenApplication();
     await restApp.start();
-    restApp.static('/', FIXTURES);
+    restApp.static('/', ASSETS);
     const content = fs
-      .readFileSync(path.join(FIXTURES, 'index.html'))
+      .readFileSync(path.join(ASSETS, 'index.html'))
       .toString('utf-8');
     client = createRestAppClient(restApp);
     await client

diff --git a/packages/rest/test/integration/rest.server.integration.ts b/packages/rest/test/integration/rest.server.integration.ts
@@ -12,14 +12,18 @@ import {
   httpsGetAsync,
   givenHttpServerConfig,
 } from '@loopback/testlab';
-import {RestBindings, RestServer, RestComponent} from '../..';
+import {RestBindings, RestServer, RestComponent, get} from '../..';
 import {IncomingMessage, ServerResponse} from 'http';
 import * as yaml from 'js-yaml';
 import * as path from 'path';
 import * as fs from 'fs';
+import * as util from 'util';
+const readFileAsync = util.promisify(fs.readFile);
+
 import {RestServerConfig} from '../..';
 
 const FIXTURES = path.resolve(__dirname, '../../../fixtures');
+const ASSETS = path.resolve(FIXTURES, 'assets');
 
 describe('RestServer (integration)', () => {
   it('exports url property', async () => {
@@ -81,7 +85,7 @@ describe('RestServer (integration)', () => {
   });
 
   it('allows static assets to be mounted at /', async () => {
-    const root = FIXTURES;
+    const root = ASSETS;
     const server = await givenAServer({
       rest: {
         port: 0,
@@ -97,7 +101,7 @@ describe('RestServer (integration)', () => {
   });
 
   it('allows static assets via api', async () => {
-    const root = FIXTURES;
+    const root = ASSETS;
     const server = await givenAServer({
       rest: {
         port: 0,
@@ -114,8 +118,57 @@ describe('RestServer (integration)', () => {
       .expect(200, content);
   });
 
+  it('allows static assets to be mounted on multiple paths', async () => {
+    const root = ASSETS;
+    const server = await givenAServer({
+      rest: {
+        port: 0,
+      },
+    });
+
+    server.static('/html-0', root);
+    server.static('/html-1', root);
+
+    const content = fs
+      .readFileSync(path.join(root, 'index.html'))
+      .toString('utf-8');
+    await createClientForHandler(server.requestHandler)
+      .get('/html-0/index.html')
+      .expect('Content-Type', /text\/html/)
+      .expect(200, content);
+    await createClientForHandler(server.requestHandler)
+      .get('/html-1/index.html')
+      .expect('Content-Type', /text\/html/)
+      .expect(200, content);
+  });
+
+  it('merges different static asset directories when mounted on the same path', async () => {
+    const root = ASSETS;
+    const otherAssets = path.join(FIXTURES, 'other-assets');
+    const server = await givenAServer({
+      rest: {
+        port: 0,
+      },
+    });
+
+    server.static('/html', root);
+    server.static('/html', otherAssets);
+
+    let content = await readFileFromDirectory(root, 'index.html');
+    await createClientForHandler(server.requestHandler)
+      .get('/html/index.html')
+      .expect('Content-Type', /text\/html/)
+      .expect(200, content);
+
+    content = await readFileFromDirectory(otherAssets, 'robots.txt');
+    await createClientForHandler(server.requestHandler)
+      .get('/html/robots.txt')
+      .expect('Content-Type', /text\/plain/)
+      .expect(200, content);
+  });
+
   it('allows static assets via api after start', async () => {
-    const root = FIXTURES;
+    const root = ASSETS;
     const server = await givenAServer({
       rest: {
         port: 0,
@@ -133,7 +186,7 @@ describe('RestServer (integration)', () => {
   });
 
   it('allows non-static routes after assets', async () => {
-    const root = FIXTURES;
+    const root = ASSETS;
     const server = await givenAServer({
       rest: {
         port: 0,
@@ -148,7 +201,7 @@ describe('RestServer (integration)', () => {
   });
 
   it('gives precedence to API routes over static assets', async () => {
-    const root = FIXTURES;
+    const root = ASSETS;
     const server = await givenAServer({
       rest: {
         port: 0,
@@ -162,6 +215,21 @@ describe('RestServer (integration)', () => {
       .expect(200, 'Hello');
   });
 
+  it('registers controllers defined later than static assets', async () => {
+    const root = ASSETS;
+    const server = await givenAServer({
+      rest: {
+        port: 0,
+      },
+    });
+    server.static('/html', root);
+    server.controller(DummyController);
+
+    await createClientForHandler(server.requestHandler)
+      .get('/html')
+      .expect(200, 'Hi');
+  });
+
   it('allows cors', async () => {
     const server = await givenAServer({rest: {port: 0}});
     server.handler(dummyRequestHandler);
@@ -597,4 +665,21 @@ paths:
     response.write('Hello');
     response.end();
   }
+
+  class DummyController {
+    constructor() {}
+    @get('/html', {
+      responses: {},
+    })
+    ping(): string {
+      return 'Hi';
+    }
+  }
+
+  function readFileFromDirectory(
+    dirname: string,
+    filename: string,
+  ): Promise<string> {
+    return readFileAsync(path.join(dirname, filename), {encoding: 'utf-8'});
+  }
 });
diff --git a/packages/rest/test/unit/router/routing-table.unit.ts b/packages/rest/test/unit/router/routing-table.unit.ts
@@ -18,6 +18,7 @@ import {
   RegExpRouter,
   TrieRouter,
 } from '../../..';
+import * as HttpErrors from 'http-errors';
 
 describe('RoutingTable', () => {
   it('joins basePath and path', () => {
@@ -187,6 +188,17 @@ function runTestsWithRouter(router: RestRouter) {
     expect(route.pathParams).to.containEql({arg1: '3', arg2: '2'});
   });
 
+  it('throws if router is not found', () => {
+    const table = givenRoutingTable();
+
+    const request = givenRequest({
+      method: 'get',
+      url: '/hi',
+    });
+
+    expect(() => table.find(request)).to.throwError(HttpErrors.NotFound);
+  });
+
   function givenRequest(options?: ShotRequestOptions): Request {
     return stubExpressContext(options).request;
   }