Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(rest): allow basePath for rest servers #2097

Merged
merged 1 commit into from
Dec 4, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions docs/site/Server.md
Original file line number Diff line number Diff line change
Expand Up @@ -211,12 +211,38 @@ export async function main() {
For a complete list of CORS options, see
https://github.com/expressjs/cors#configuration-options.

### Configure the Base Path

Sometime it's desirable to expose REST endpoints using a base path, such as
`/api`. The base path can be set as part of the RestServer configuration.

```ts
const app = new RestApplication({
rest: {
basePath: '/api',
},
});
```

The `RestApplication` and `RestServer` both provide a `basePath()` API:

```ts
const app: RestApplication;
// ...
app.basePath('/api');
```

With the `basePath`, all REST APIs and static assets are served on URLs starting
with the base path.

### `rest` options

| Property | Type | Purpose |
| ----------- | ------------------- | --------------------------------------------------------------------------------------------------------- |
| port | number | Specify the port on which the RestServer will listen for traffic. |
| protocol | string (http/https) | Specify the protocol on which the RestServer will listen for traffic. |
| host | string | Specify the hostname or ip address on which the RestServer will listen for traffic. |
| port | number | Specify the port on which the RestServer listens for traffic. |
| protocol | string (http/https) | Specify the protocol on which the RestServer listens for traffic. |
| basePath | string | Specify the base path that RestServer exposes http endpoints. |
| key | string | Specify the SSL private key for https. |
| cert | string | Specify the SSL certificate for https. |
| cors | CorsOptions | Specify the CORS options. |
Expand Down
6 changes: 6 additions & 0 deletions packages/rest/src/keys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@ export namespace RestBindings {
export const HTTPS_OPTIONS = BindingKey.create<https.ServerOptions>(
'rest.httpsOptions',
);

/**
* Internal binding key for basePath
*/
export const BASE_PATH = BindingKey.create<string>('rest.basePath');

/**
* Internal binding key for http-handler
*/
Expand Down
8 changes: 8 additions & 0 deletions packages/rest/src/rest.application.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,14 @@ export class RestApplication extends Application implements HttpServerLike {
return this.restServer.bodyParser(bodyParserClass, address);
}

/**
* Configure the `basePath` for the rest server
* @param path Base path
*/
basePath(path: string = '') {
this.restServer.basePath(path);
}

/**
* Register a new Controller-based route.
*
Expand Down
54 changes: 48 additions & 6 deletions packages/rest/src/rest.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,18 @@ export class RestServer extends Context implements Server, HttpServerLike {
* @param req The request.
* @param res The response.
*/
public requestHandler: HttpRequestListener;

protected _requestHandler: HttpRequestListener;
public get requestHandler(): HttpRequestListener {
if (this._requestHandler == null) {
this._setupRequestHandlerIfNeeded();
}
return this._requestHandler;
}

public readonly config: RestServerConfig;
private _basePath: string;

protected _httpHandler: HttpHandler;
protected get httpHandler(): HttpHandler {
this._setupHandlerIfNeeded();
Expand Down Expand Up @@ -185,15 +194,17 @@ export class RestServer extends Context implements Server, HttpServerLike {
this.sequence(config.sequence);
}

this._setupRequestHandler();
this.basePath(config.basePath);

this.bind(RestBindings.BASE_PATH).toDynamicValue(() => this._basePath);
this.bind(RestBindings.HANDLER).toDynamicValue(() => this.httpHandler);
}

protected _setupRequestHandler() {
protected _setupRequestHandlerIfNeeded() {
if (this._expressApp) return;
this._expressApp = express();
this._expressApp.set('query parser', 'extended');
this.requestHandler = this._expressApp;
this._requestHandler = this._expressApp;

// Allow CORS support for all endpoints so that users
// can test with online SwaggerUI instance
Expand All @@ -211,7 +222,7 @@ export class RestServer extends Context implements Server, HttpServerLike {
this._setupOpenApiSpecEndpoints();

// Mount our router & request handler
this._expressApp.use((req, res, next) => {
this._expressApp.use(this._basePath, (req, res, next) => {
this._handleHttpRequest(req, res).catch(next);
});

Expand Down Expand Up @@ -365,6 +376,15 @@ export class RestServer extends Context implements Server, HttpServerLike {
specObj.servers = [{url: this._getUrlForClient(request)}];
}

if (specObj.servers && this._basePath) {
for (const s of specObj.servers) {
// Update the default server url to honor `basePath`
if (s.url === '/') {
s.url = this._basePath;
}
}
}

if (specForm.format === 'json') {
const spec = JSON.stringify(specObj, null, 2);
response.setHeader('content-type', 'application/json; charset=utf-8');
Expand Down Expand Up @@ -433,7 +453,7 @@ export class RestServer extends Context implements Server, HttpServerLike {
// add port number of present
host += port !== '' ? ':' + port : '';

return protocol + '://' + host;
return protocol + '://' + host + this._basePath;
}

private async _redirectToSwaggerUI(
Expand Down Expand Up @@ -732,13 +752,31 @@ export class RestServer extends Context implements Server, HttpServerLike {
return binding;
}

/**
* Configure the `basePath` for the rest server
* @param path Base path
*/
basePath(path: string = '') {
if (this._requestHandler) {
throw new Error(
bajtos marked this conversation as resolved.
Show resolved Hide resolved
'Base path cannot be set as the request handler has been created',
);
}
// Trim leading and trailing `/`
path = path.replace(/(^\/)|(\/$)/, '');
if (path) path = '/' + path;
this._basePath = path;
}

/**
* Start this REST API's HTTP/HTTPS server.
*
* @returns {Promise<void>}
* @memberof RestServer
*/
async start(): Promise<void> {
// Set up the Express app if not done yet
this._setupRequestHandlerIfNeeded();
// Setup the HTTP handler so that we can verify the configuration
// of API spec, controllers and routes at startup time.
this._setupHandlerIfNeeded();
Expand Down Expand Up @@ -875,6 +913,10 @@ export interface ApiExplorerOptions {
* Options for RestServer configuration
*/
export interface RestServerOptions {
/**
* Base path for API/static routes
*/
basePath?: string;
cors?: cors.CorsOptions;
openApiSpec?: OpenApiSpecOptions;
apiExplorer?: ApiExplorerOptions;
Expand Down
29 changes: 25 additions & 4 deletions packages/rest/test/integration/rest.application.integration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {createRestAppClient, Client, expect} from '@loopback/testlab';
import {RestApplication} from '../..';
import * as path from 'path';
import {anOperationSpec} from '@loopback/openapi-spec-builder';
import {Client, createRestAppClient, expect} from '@loopback/testlab';
import * as fs from 'fs';
import {RestServer, RestServerConfig} from '../../src';
import * as path from 'path';
import {RestApplication, RestServer, RestServerConfig} from '../..';

const ASSETS = path.resolve(__dirname, '../../../fixtures/assets');

Expand Down Expand Up @@ -92,6 +92,27 @@ describe('RestApplication (integration)', () => {
.expect('Hello');
});

it('honors basePath for static assets', async () => {
givenApplication();
restApp.basePath('/html');
restApp.static('/', ASSETS);
await restApp.start();
client = createRestAppClient(restApp);
await client.get('/html/index.html').expect(200);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this test very confusing. As far as I understand givenApplication() helper, it creates an app with no API routes configured. As I understand app.basePath, it's intended primarily for customizing the base path of REST APIs.

I see that there are other tests verifying the impact of basePath on different kinds of routes, I agree there is no need to duplicate those tests in rest.application.integration.ts.

Can you please add a comment here? Explain that in this test, we are invoking a route for static assets, because basePath applies to all kinds of routes, including static assets.

Alternatively, rework this test to use a handler-function route (see the example below) - I think it will make the intent of this test much easier to understand even without comments.

app.route('get', '/status', {/*spec*/}, () => {running: true});
// ...
client.get('/api/status').expect(200, {running: true});

});

it('honors basePath for routes', async () => {
givenApplication();
restApp.basePath('/api');
restApp.route('get', '/status', anOperationSpec().build(), () => ({
running: true,
}));

await restApp.start();
client = createRestAppClient(restApp);
await client.get('/api/status').expect(200, {running: true});
});

it('returns RestServer instance', async () => {
givenApplication();
const restServer = restApp.restServer;
Expand Down
50 changes: 50 additions & 0 deletions packages/rest/test/integration/rest.server.integration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -687,6 +687,56 @@ paths:
await server.stop();
});

describe('basePath', () => {
const root = ASSETS;
let server: RestServer;

beforeEach(async () => {
server = await givenAServer({
rest: {
basePath: '/api',
port: 0,
},
});
});

it('controls static assets', async () => {
server.static('/html', root);

const content = fs
.readFileSync(path.join(root, 'index.html'))
.toString('utf-8');
await createClientForHandler(server.requestHandler)
.get('/api/html/index.html')
.expect('Content-Type', /text\/html/)
.expect(200, content);
});

it('controls controller routes', async () => {
server.controller(DummyController);

await createClientForHandler(server.requestHandler)
.get('/api/html')
.expect(200, 'Hi');
});

it('reports 404 if not found', async () => {
server.static('/html', root);
server.controller(DummyController);

await createClientForHandler(server.requestHandler)
.get('/html')
.expect(404);
});

it('controls server urls', async () => {
const response = await createClientForHandler(server.requestHandler).get(
'/openapi.json',
);
expect(response.body.servers).to.containEql({url: '/api'});
});
});

async function givenAServer(
options: {rest: RestServerConfig} = {rest: {port: 0}},
) {
Expand Down
37 changes: 37 additions & 0 deletions packages/rest/test/unit/rest.server/rest.server.unit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,43 @@ describe('RestServer', () => {
expect(server.getSync(RestBindings.PORT)).to.equal(4000);
expect(server.getSync(RestBindings.HOST)).to.equal('my-host');
});

it('honors basePath in config', async () => {
const app = new Application({
rest: {port: 0, basePath: '/api'},
});
app.component(RestComponent);
const server = await app.getServer(RestServer);
expect(server.getSync(RestBindings.BASE_PATH)).to.equal('/api');
});

it('honors basePath via api', async () => {
const app = new Application({
rest: {port: 0},
});
app.component(RestComponent);
const server = await app.getServer(RestServer);
server.basePath('/api');
expect(server.getSync(RestBindings.BASE_PATH)).to.equal('/api');
});

it('rejects basePath if request handler is created', async () => {
const app = new Application({
rest: {port: 0},
});
app.component(RestComponent);
const server = await app.getServer(RestServer);
expect(() => {
// Force the `getter` function to be triggered by referencing
// `server.requestHandler` so that the servers has `requestHandler`
// populated to prevent `basePath` to be set.
if (server.requestHandler) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this condition happens to be false, then server.basePath is not invoked at all and the test fails with a confusing error.

Please convert this if into an explicit assertion. For example:

expect.assert(!!server.requestHandler, 'requestHandler should have been set up by now');
expect(() => server.basePath('/api')).to.throw(/*...*/);

server.basePath('/api');
}
}).to.throw(
/Base path cannot be set as the request handler has been created/,
);
});
});

async function givenRequestContext() {
Expand Down