Skip to content

Commit

Permalink
fix: review 1
Browse files Browse the repository at this point in the history 
review 1
  • Loading branch information
Hage Yaapa committed Oct 24, 2018
1 parent 89e8361 commit 97bd294
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 93 deletions.
44 changes: 16 additions & 28 deletions packages/rest/src/rest.server.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -33,7 +32,7 @@ import {
Route,
RouteEntry,
RoutingTable,
StaticRoute,
StaticAssetsRoute,
} from './router/routing-table';

import {DefaultSequence, SequenceFunction, SequenceHandler} from './sequence';
Expand All @@ -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');

Expand All @@ -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'
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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();

Expand All @@ -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();
}

/**
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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
*/
Expand Down
5 changes: 0 additions & 5 deletions packages/rest/src/router/router-base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
103 changes: 72 additions & 31 deletions packages/rest/src/router/routing-table.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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
Expand Down Expand Up @@ -74,8 +80,6 @@ export interface RestRouter {
*/
find(request: Request): ResolvedRoute | undefined;

getStaticAssetsRouter(): ResolvedRoute;

/**
* List all routes
*/
Expand All @@ -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
Expand Down Expand Up @@ -154,6 +160,9 @@ export class RoutingTable {

validateApiPath(route.path);
this._router.add(route);
if (route.path === '/*') {
this._staticAssetsRoute = createResolvedRoute(route, {});
}
}

describeApiPaths(): PathObject {
Expand Down Expand Up @@ -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.`,
);
}
}

Expand Down Expand Up @@ -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<OperationRetval> {
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
*/
Expand Down Expand Up @@ -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<OperationRetval> {
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,
Expand All @@ -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);
}
Expand Down
38 changes: 9 additions & 29 deletions packages/rest/test/integration/rest.server.integration.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,37 +80,20 @@ 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: {
port: 0,
},
});

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 () => {
Expand Down Expand Up @@ -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: {
Expand All @@ -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 () => {
Expand Down

0 comments on commit 97bd294

Please sign in to comment.