Skip to content

Openapi and api explorer to honor basePath #2554

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.

Sign up for GitHub

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

Closed
dkrantsberg wants to merge 2 commits into from
Closed

Openapi and api explorer to honor basePath #2554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9373ea1
feat: openepi.json and API explorer honor basePath
dkrantsberg Mar 7, 2019
289b34e
chore: added tests
dkrantsberg Mar 7, 2019
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
4 changes: 2 additions & 2 deletions packages/cli/generators/app/templates/public/index.html.ejs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@
<h1><%= project.name %></h1>
<p>Version <%= project.version || '1.0.0' %></p>

<h3>OpenAPI spec: <a href="/openapi.json">/openapi.json</a></h3>
<h3>API Explorer: <a href="/explorer">/explorer</a></h3>
<h3>OpenAPI spec: <a href="openapi.json">openapi.json</a></h3>
<h3>API Explorer: <a href="explorer">explorer</a></h3>
</div>

<footer class="power">
Expand Down
16 changes: 15 additions & 1 deletion packages/rest/src/__tests__/integration/rest.server.integration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,11 +771,25 @@ paths:

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

it('controls /explorer route', async () => {
const response = await createClientForHandler(server.requestHandler).get(
'/api/explorer',
);
expect(response.status).to.eql(308);
});

it('controls /swagger-ui route', async () => {
const response = await createClientForHandler(server.requestHandler).get(
'/api/swagger-ui',
);
expect(response.status).to.eql(308);
});

it('controls redirect locations', async () => {
server.controller(DummyController);
server.redirect('/page/html', '/html');
Expand Down
23 changes: 14 additions & 9 deletions packages/rest/src/rest.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,13 @@ export class RestServer extends Context implements Server, HttpServerLike {
config.host = undefined;
}

this.basePath(config.basePath);

const OPENAPI_SPEC_MAPPING: {[key: string]: OpenApiSpecForm} = {
[`${this._basePath}/openapi.json`]: {version: '3.0.0', format: 'json'},
[`${this._basePath}/openapi.yaml`]: {version: '3.0.0', format: 'yaml'},
Copy link
Member

@bajtos bajtos Mar 8, 2019

Choose a reason for hiding this comment

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

I am also not sure we want these endpoints to honor basePath option.

@raymondfeng @strongloop/loopback-maintainers what's your opinion? When the application is configured with a custom basePath, e.g. /api, should we apply the base path to openapi endpoints too?

  • Now we have /openapi.json and e.g. /api/products
  • Proposed change: /api/openapi.json and e.g. /api/products

I am afraid the proposed implementation is a breaking change requiring a semver-major release. I think we should preserve backwards compatibility and introduce a new feature flag allowing users to decide whether to apply basePath configuration to openapi endpoints or not. By default, we should fall back to old behavior, but new projects scaffolded by lb4 cli should enable the new behavior.

Copy link
Author

@dkrantsberg dkrantsberg Mar 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@bajtos Having /explorer always running at the root creates problems when there's a reverse proxy running in front of the API. Here's our situation and I'm sure this is pretty common case for many production environments.

We have both UI and API applications running on the same host behind nginx.

UI runs at: http://www.mysite.com
Loopback API runs at: http://www.mysite.com/_api

Whenever a requests come for /_api/* nginx forwards it to Loopback API. All other requests to to the UI application.

As a result requests to http://www.mysite.com/explorer fail because they don't even reach the Loopback app. Hence my fix, which allows explorer to run at http://www.mysite.com/_api/explorer.

Copy link
Contributor

@raymondfeng raymondfeng Mar 8, 2019

Choose a reason for hiding this comment

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

I don't think basePath was introduced for this purpose. There are 3 paths that build a route to a controller method:

  1. server-level basePath: it's used as _expressApp.use(basePath, <lb4-rest-router>);, such as /api
  2. controller-class-level basePath (set by @api): it's prepended to the path from step 3, such as /products
  3. controller-method-level path (set by @get, @post etc): local path of the operation, such as /{id}

Please note 1 is for express middleware mounting and it is transparent to our REST API routes. Express will strip /api from request.url property, which becomes /products/{id}.
2 + 3 is for internal routing by @loopback/rest.

Putting reverse proxy aside, the http url for a method is: /<serverBasePath>/<controllerBasePath>/<operationPath>, such as /api/products/{id}and api explorer is exposed at /explorer by default.

Copy link
Author

@dkrantsberg dkrantsberg Mar 9, 2019

Choose a reason for hiding this comment

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

@raymondfeng Then my question is: is there a way to expose the API explorer at some different route (like /myprefix/explorer)?

I did create a workaround in my project but it's quite hacky and involves messing with the express route stack directly. I'd like to avoid doing it.

Copy link
Member

@bajtos bajtos Mar 12, 2019

Choose a reason for hiding this comment

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

@dkrantsberg I'd like to better understand your scenario.

We have both UI and API applications running on the same host behind nginx.

Do you have two servers, one for UI and another for LoopBack API? If that's the case, then I would expect you should be able to configure your nginx instance as follows:

  • API requests (e.g. http://www.mysite.com/_api/products/{id}) are forwarded to LB instance and _api is stripped from the path (e.g. /products/{id}).
  • As a result, explorer will become available at http://www.mysite.com/_api/explorer. Alternatively, I believe it should be possible to configure nginx and define a new virtual route (e.g. http://www.mysite.com/api-explorer) that will be forwarded to /explorer in LB.

I think the key is to keep empty basePath setting in LoopBack.

In case you are running both UI and LB API in the same server, how do you compose these two sets of routes?

Copy link
Member

@bajtos bajtos Mar 12, 2019

Choose a reason for hiding this comment

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

is there a way to expose the API explorer at some different route (like /myprefix/explorer)?

The self-hosted version of API Explorer that's provided by @loopback/rest-explorer is already honoring basePath setting and allows customization of the Explorer URL, see
https://github.com/strongloop/loopback-next/tree/master/packages/rest-explorer#basic-use

By default, API Explorer is mounted at /explorer. This path can be customized via RestExplorer configuration as follows:

this.bind(RestExplorerBindings.CONFIG).to({
  path: '/openapi/ui',
});

The redirection to externally-hosted API Explorer, which is built into @loopback/rest, does not support customization of the URL and it also ignores basePath setting. See here:
https://github.com/strongloop/loopback-next/blob/6e0eeb66b6a8c79d736fcc2dc7d9c76bfd57abd3/packages/rest/src/rest.server.ts#L288-L291

Please note that these redirects are deprecated, we want our users to use the self-hosted variant provided by @loopback/rest.

};

config.openApiSpec = config.openApiSpec || {};
config.openApiSpec.endpointMapping =
config.openApiSpec.endpointMapping || OPENAPI_SPEC_MAPPING;
Expand All @@ -206,8 +213,6 @@ export class RestServer extends Context implements Server, HttpServerLike {
this.sequence(config.sequence);
}

this.basePath(config.basePath);

this.bind(RestBindings.BASE_PATH).toDynamicValue(() => this._basePath);
this.bind(RestBindings.HANDLER).toDynamicValue(() => this.httpHandler);
}
Expand Down Expand Up @@ -277,7 +282,10 @@ export class RestServer extends Context implements Server, HttpServerLike {
);
}

const explorerPaths = ['/swagger-ui', '/explorer'];
const explorerPaths = [
`${this._basePath}/swagger-ui`,
`${this._basePath}/explorer`,
];
this._expressApp.get(explorerPaths, (req, res, next) =>
this._redirectToSwaggerUI(req, res, next),
);
Expand Down Expand Up @@ -510,7 +518,9 @@ export class RestServer extends Context implements Server, HttpServerLike {
debug('Redirecting to swagger-ui from %j.', request.originalUrl);
const protocol = this._getProtocolForRequest(request);
const baseUrl = protocol === 'http' ? config.httpUrl : config.url;
const openApiUrl = `${this._getUrlForClient(request)}/openapi.json`;
const openApiUrl = `${this._getUrlForClient(request)}${
this._basePath
}/openapi.json`;
const fullUrl = `${baseUrl}?url=${openApiUrl}`;
response.redirect(308, fullUrl);
}
Expand Down Expand Up @@ -920,11 +930,6 @@ export interface OpenApiSpecForm {
format?: string;
}

const OPENAPI_SPEC_MAPPING: {[key: string]: OpenApiSpecForm} = {
'/openapi.json': {version: '3.0.0', format: 'json'},
Copy link
Member

@bajtos bajtos Mar 8, 2019

Choose a reason for hiding this comment

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

IMO, we should preserve OPENAPI_SPEC_MAPPING as a constant config independent on basePath. The basePath setting should be applied when we are processing the mapping entries.

In your proposal, when app developers specify custom mapping, then their custom endpoints won't be prefixed with basePath. I would find that surprising.

In my proposal, basePath will be prepended to all mapping entries, regardless of whether they are provided by the user or come from the default mapping.

Copy link
Author

@dkrantsberg dkrantsberg Mar 8, 2019

Choose a reason for hiding this comment

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

Leaving OPENAPI_SPEC_MAPPING where it is and applying basePath to it inside the RestServer constructor will mean recreating the entire OPENAPI_SPEC_MAPPING object with basePath added to the key names.
Is there a reason to do it considering that OPENAPI_SPEC_MAPPING constant isn't used anywhere else in the code other than RestServer constructor?

Yes, I propose that custom mapping should be truly custom and not be altered once it is set by a developer. IMO this will give developers more flexibility. But I'm fine either way as long as we have this PR moving forward.

My main blocker is inability to customize /explorer path.

Copy link
Contributor

@raymondfeng raymondfeng Mar 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

My main blocker is inability to customize /explorer path.

Yes, that's what I'm seeing now. Please note @loopback/rest-explorer mounts the UI as follows:

// explorePath is default to `/explorer`
application.static(explorerPath, swaggerUI.getAbsoluteFSPath());

But RestApplication.static() adds it to a sub router of the root router (/api). As a result, the UI is only available under /api/explorer while the spec is served at /openapi.json. Here is the express pipeline:

/ (RestServer._expressApp)
  /openapi.json
  /openapi.yaml
  /api
    /products
    /explorer (Explorer UI)

If we want keep /openapi.json and /explorer regardless of basePath, we need to allow /explorer to be mounted at root RestServer._expressApp level.

Copy link
Member

@bajtos bajtos Mar 12, 2019

Choose a reason for hiding this comment

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

Leaving OPENAPI_SPEC_MAPPING where it is and applying basePath to it inside the RestServer constructor will mean recreating the entire OPENAPI_SPEC_MAPPING object with basePath added to the key names.

I am envisioning this differently. Instead of re-creating a new mapping object, we should apply basePath when creating Express routes here:

https://github.com/strongloop/loopback-next/blob/6e0eeb66b6a8c79d736fcc2dc7d9c76bfd57abd3/packages/rest/src/rest.server.ts#L281-L286

-    this._expressApp.get(p, (req, res) => 
+    this._expressApp.get(basePath + p, (req, res) =>

'/openapi.yaml': {version: '3.0.0', format: 'yaml'},
};

/**
* Options to customize how OpenAPI specs are served
*/
Expand Down