feat: rest explorer now hosts its own copy of oas by default #3133
Conversation
There was a problem hiding this comment.
Thank you @mgabeler-lee-6rs for the pull request, this is great!
I have mixed feelings about the new public API RestServer.addOpenApiSpecEndpoint. I would prefer to implement the new endpoint as a regular controller route in REST Explorer:
In ExplorerController, add a new endpoint to server the spec:
class ExplorerController {
// ...
spec(): {
const specObj = this.request.getApiSpec(this.requestContext);
const spec = JSON.stringify(specObj, null, 2);
response.setHeader('content-type', 'application/json; charset=utf-8');
response.end(spec, 'utf-8');
return response;
}
}In RestExplorerComponent, conditionally register this new controller route:
this.registerControllerRoute('get', explorerPath, 'indexRedirect');
this.registerControllerRoute('get', explorerPath + '/', 'index');
if (explorerConfig.useOwnOpenApiSpecEndpoint !== false) {
const endpointPath = explorerPath + '/' + ExplorerController.OPENAPI_RELATIVE_URL;
this.registerControllerRoute('get', endpointPath, 'spec');
}The missing pieces:
- Modify
RestServer.getApiSpecto accept an optionalRequestContextparameter. If the parameter is set, then modify servers and basePath as we are doing in_serveOpenApiSpecnow. - Find a way how to access the RestServer instance from controllers. For example, you can add a new binding key, similar to
RestBindings.Http.CONTEXTandCoreBindings.APPLICATION_INSTANCE.
Thoughts?
|
I've updated according to the review notes, and also have done some testing with an actual reverse proxy config, which found two things related to finding the proper server URL for the explorer to talk to when behind a path-modifying proxy:
I'm working on an update to try to address these by (conditionally) inserting an entry in |
|
Unfortunately it seems |
|
One hack that I considered and explicitly rejected: When loading the spec from the explorer, it is (usually) possible to infer the external URL in use from the I could add logic to infer a |
|
I gave a whirl at writing the referer thing to see how ugly it is ... here's what I came up with. I'm not at all sure that it correctly handles all the possible cases of base/mount paths however, and it still seems ... brittle. diff --git a/packages/rest-explorer/src/rest-explorer.controller.ts b/packages/rest-explorer/src/rest-explorer.controller.ts
index 5f6c0b59..b2d1953b 100644
--- a/packages/rest-explorer/src/rest-explorer.controller.ts
+++ b/packages/rest-explorer/src/rest-explorer.controller.ts
@@ -33,6 +33,7 @@ export class ExplorerController {
private openApiSpecUrl: string;
private useOwnOpenApiSpecEndpoint: boolean;
+ private explorerPath: string;
constructor(
@inject(RestBindings.CONFIG, {optional: true})
@@ -47,6 +48,10 @@ export class ExplorerController {
) {
this.useOwnOpenApiSpecEndpoint =
explorerConfig.useOwnOpenApiSpecEndpoint !== false;
+ this.explorerPath = explorerConfig.path || '/explorer';
+ if (!this.explorerPath.endsWith('/')) {
+ this.explorerPath += '/';
+ }
this.openApiSpecUrl = this.getOpenApiSpecUrl(restConfig);
}
@@ -89,6 +94,13 @@ export class ExplorerController {
spec() {
const specObj = this.restServer.getApiSpec(this.requestContext);
+ const referer = this.request.headers.referer;
+ if (this.useOwnOpenApiSpecEndpoint && referer && specObj.servers) {
+ // evil: infer the external server url from the referer header, insert it in the servers list
+ if (referer.endsWith(this.explorerPath)) {
+ specObj.servers.unshift({url: referer.slice(0, -this.explorerPath.length + 1)});
+ }
+ }
const spec = JSON.stringify(specObj, null, 2);
this.response.setHeader('content-type', 'application/json; charset=utf-8');
this.response.end(spec, 'utf-8'); |
|
Thank you for a thorough investigation, @mgabeler-lee-6rs. I really appreciate the effort you are putting into this work, especially considering all different dead-ends we have encountered so far.
This looks like a bug in swagger-ui to me. Quoting from https://github.com/OAI/OpenAPI-Specification/blob/3.0.1/versions/3.0.1.md#server-object, emphasis is mine: A URL to the target host. This URL supports Server Variables and MAY be relative, to indicate that the host location is relative to the location where the OpenAPI document is being served. Would you mind reporting an issue to swagger-ui and/or swagger-js? Maybe I am reading the spec incorrectly and the term "host location" is referring only to the scheme+hostname+port part of the URL? Let's find out :)
Hmm.
I agree it's rather brittle. I guess I can live with it if we don't find any better solution. It makes me wonder. As the developer setting up the LB app & the reverse proxy, do you know the URL where the OpenAPI spec can be loaded from? What if we added a new configuration option allowing developers to tell the Explorer where to load the spec from? At the moment, Explorer accepts a single option - the path where to server the front-end: What if we added a new option, e.g. As for the second pard (using My idea is that if the app is running behind a reverse proxy, then the developer should:
Obviously, that won't work well if there are some clients accessing the server through a reverse proxy and other clients that are accessing the server directly. Is this a relevant use case for you? |
You're welcome. Enlightened self-interest and all that 😃
Interesting, I was reading https://swagger.io/docs/specification/api-host-and-base-path/#relative-urls which explicitly says "The URLs in the servers array can be relative, such as /v2. In this case, the URL is resolved against the server that hosts the given OpenAPI definition". Clearly the spec is the authoritative source of proper behavior.
Filed swagger-api/swagger-ui#5401
That's basically what this PR addresses -- making the explorer handle that for itself by having a copy of the spec available to it at a constant relative path.
Well, that's the issue "self-hosting the spec" in this PR resolves -- it allows the explorer to always use a fixed URL to get the spec, and not care for that aspect about any base URLs that are involved, whether they are within loopback or from an external proxy.
Not really, no. A minor annoyance only.
The relative url hosting of the spec in this PR will allow that to be handled automagically.
Yup 👍
Our use case requires supporting both direct & proxied access, but not at the same time -- developers run with direct access, test/integration & production run behind the proxy. We use the common pattern of a So, for us, having to set Fixing the relative path handling in TL;DR: The relative spec hosting in this PR is sufficient to address our use case. Will take a swing through and work on bringing up the coverage, and also will take a pass at adding a section to the documentation on how to configure this stuff for path-prefixing reverse proxies like we have. |
|
@mgabeler-lee-6rs what's the status of this pull request? Is there anything I can help you with to move this work forward? |
|
I've got local work to sort out the test coverage, but the test are currently broken in my merge from |
|
OK, I think I have this in decent shape ... notes/thoughts:
Here's my nginx config snippet I used for that testing: |
There was a problem hiding this comment.
Thank you for the updates. I quickly skimmed through the new diff, I think conceptually your solution looks good! Now I'll need to set aside a larger amount of time to review it in detail.
@raymondfeng could you please review these changes too?
| const specObj = this.restServer.getApiSpec(this.requestContext); | ||
| const spec = JSON.stringify(specObj, null, 2); | ||
| this.response.setHeader('content-type', 'application/json; charset=utf-8'); | ||
| this.response.end(spec, 'utf-8'); |
There was a problem hiding this comment.
Is it necessary for this endpoint to deal with JSON serialization to HTTP response? I believe that by default, LoopBack is automatically serializing controller return values as JSON. I think this controller method can be simplified as follows:
spec() {
return this.restServer.getApiSpec(this.requestContext);
}There was a problem hiding this comment.
You may well be right. This code is copy-pasta from RestServer._serveOpenApiSpec.
There was a problem hiding this comment.
Doing some git history spelunking, it looks like this code comes from back when it was done at the express level, instead of being a controller level (3286bc6).
A quick local test indicates that your simplification does work, so I'll clean up both copies of this code 👍
Edit: oops, the other copy of this code still is an express handler not a controller endpoint, so that copy needs to stay as-is.
There was a problem hiding this comment.
One thing I notice happens when I do this: the response is no longer explicitly listed (in the HTTP headers) as being in the UTF-8 encoding (no specific encoding is listed).
I suspect this is largely a non-issue, but let me know if you disagree.
There was a problem hiding this comment.
One thing I notice happens when I do this: the response is no longer explicitly listed (in the HTTP headers) as being in the UTF-8 encoding (no specific encoding is listed).
That's because of the following line:
I think the missing charset information is fine as far as this pull request is concerned.
It would be nice to eventually fix the line above to include charset/encoding too - would you like to open a new pull request?
| Note also that you cannot use a url-relative path for the `servers` entry, as | ||
| the Swagger UI does not support that (yet). You may use a _host_-relative path | ||
| however. | ||
|
|
There was a problem hiding this comment.
To make it easy to understand, maybe we should build a table to show different cases with corresponding configurations, such as:
| Use case | Recommended configuration | Exposed spec/UI URL |
|---|---|---|
| Running as standalone server | ||
| Running behind a reverse proxy |
There was a problem hiding this comment.
👍 will work on that
I think the scenarios to consider are:
- Whether the app is exposed directly or not (i.e. does the request the app received have the original request host directly or in a reverse proxy provided header)
- Whether there is a path-modifying reverse proxy in front of the app
- Whether you have advanced an (undetermined) advanced use case where you need the explorer to use a custom OpenAPI spec instead of the LB4 generated one
There was a problem hiding this comment.
I'd like to add to more well-known cases to the list:
- The app is configured with a custom
basePath, e.g.app.basePath('/v1') - The LB4 app is mounted inside an Express instance, see e.g. https://github.com/strongloop/loopback-next/blob/6b4808084ce86e67a3a2982db2bc2ea350c6768e/examples/express-composition/src/server.ts#L24
|
Note: The commit-lint check in Travis is angry because it only clones a limited history depth ... which is causing the commitlint to not see the branch point for this PR, and thus it's running commit lint against a giant pile of commits that aren't actually part of this PR. I can rebase this PR (again) if cleaning that up is important. |
|
@mgabeler-lee-6rs We use |
| example, in the default configuration, it will expose `/explorer/openapi.json`, | ||
| or in the examples above with the Explorer path configured, it would expose | ||
| `/openapi/ui/openapi.json`. This is to allow it to use a fixed relative path to | ||
| load the spec, to be tolerant of running behind reverse proxies. |
There was a problem hiding this comment.
Does it disable the spec endpoints exposed by @loopback/rest if the explorer component has its own?
There was a problem hiding this comment.
No, it leaves those un-touched
| <sup>2</sup> Due to limitations in the OpenAPI spec and what information is | ||
| provided by the reverse proxy to the app, this is a scenario without a clear | ||
| means of getting a working explorer. | ||
|
|
| * the externally visible path, as that information is not systematically | ||
| * forwarded to the application behind the proxy. | ||
| */ | ||
| relativeOpenApiSpecEndpoint?: false; |
There was a problem hiding this comment.
So valid values of relativeOpenApiSpecEndpoint:
falseundefined
true is not allowed?
There was a problem hiding this comment.
That was the idea here for now. The default is enabled, you can use this flag to explicitly disable it. If this is too obtuse, can make this a simple boolean.
There was a problem hiding this comment.
If it's meant to only allow explicitly set relativeOpenApiSpecEndpoint to false, I'm fine with it.
| Note also that you cannot use a url-relative path for the `servers` entry, as | ||
| the Swagger UI does not support that (yet). You may use a _host_-relative path | ||
| however. | ||
|
|
There was a problem hiding this comment.
I'd like to add to more well-known cases to the list:
- The app is configured with a custom
basePath, e.g.app.basePath('/v1') - The LB4 app is mounted inside an Express instance, see e.g. https://github.com/strongloop/loopback-next/blob/6b4808084ce86e67a3a2982db2bc2ea350c6768e/examples/express-composition/src/server.ts#L24
| @inject(RestBindings.Http.REQUEST) private request: Request, | ||
| @inject(RestBindings.Http.CONTEXT) private requestContext: RequestContext, | ||
| @inject(RestBindings.Http.RESPONSE) private response: Response, |
There was a problem hiding this comment.
Do we still need to inject request and response when we are injecting the full requestContext? Those two objects can be accessed as requestContext.request and requestContext.response.
| @@ -688,6 +710,24 @@ export class RestServer extends Context implements Server, HttpServerLike { | |||
| } | |||
|
|
|||
| assignRouterSpec(spec, this._externalRoutes.routerSpec); | |||
|
|
|||
| if (requestContext) { | |||
There was a problem hiding this comment.
Let's use reverse logic & early return to make the code easier to reason about and keep the level of indentation low.
if (!requestContext) {
return spec;
}
if (this.config.openApiSpec.setServersFromRequest)
// ...
return spec;Alternatively, and I think that's a better solution, can you extract this code into a helper function?
if (requestContext) {
spec = updateSpecFromRequest(spec, requestContext);
}
return spec;
|
Quoting a few since a prior rebase means GitHub won't let me reply to some review comments directly...
Fixing
Fixing
I'm less familiar with these use cases, will read up a bit and update the PR with info in a followup commit soon |
|
@mgabeler-lee-6rs Thank you for the update. But CI is failing - https://travis-ci.com/strongloop/loopback-next/jobs/236909000. Please investigate. |
|
Yup, working on it -- the bug I fixed related to having the explorer work in some of the base path related scenarios broke some test expectations, fix will be easy |
There was a problem hiding this comment.
Thank you @mgabeler-lee-6rs for the updates, the patch is almost good to get merged, at least as far as I am concerned.
I have few minor comments to consider, PTAL.
examples/todo/package.json
Outdated
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "@loopback/example-todo", | |||
| "version": "1.7.4", | |||
| "version": "1.8.0", | |||
There was a problem hiding this comment.
We are bumping up version numbers during the release process, please revert this line.
There was a problem hiding this comment.
Will do -- but note that it will cause CI to fail, I think? It causes test to fail locally at any rate
There was a problem hiding this comment.
That's weird and not expected at all. Can you try to rebase your PR on top of the latest master please?
There was a problem hiding this comment.
Oh, oops ... forgot to switch git reflexes to fork mode and thought I was already up to date there... I suspect that will do the trick
| const url = this.request.originalUrl || this.request.url; | ||
| this.response.redirect(301, url + '/'); | ||
| let url = | ||
| this.requestContext.request.originalUrl || |
There was a problem hiding this comment.
Repeating this.requestContext. prefix is tedious, have you considered the following refactoring?
const {request, response} = requestContext;
let url = request.originalUrl || request.url;
// etc.| @@ -28,6 +28,13 @@ export class RestExplorerComponent implements Component { | |||
|
|
|||
| this.registerControllerRoute('get', explorerPath, 'indexRedirect'); | |||
| this.registerControllerRoute('get', explorerPath + '/', 'index'); | |||
| if (restExplorerConfig.relativeOpenApiSpecEndpoint !== false) { | |||
There was a problem hiding this comment.
I have mixed feelings about the option name relativeOpenApiSpecEndpoint, I feel it's describing implementation details instead of the intent. How about useSelfHostedSpec or selfHostSpec?
There was a problem hiding this comment.
Not sure why I didn't think of this before, useSelfHostedSpec is much clearer & simpler 😀
There was a problem hiding this comment.
LGTM 👏 💯
Let's get @raymondfeng feedback too.
The last step will be to squash all commits into a single one and write a descriptive commit message - see https://github.com/strongloop/loopback-next/blob/master/docs/site/DEVELOPING.md#commit-message-guidelines
This makes it much easier to use the explorer with more complex configurations such as base paths, express composition, and path-modifying reverse proxies.
|
Rebased & squashed Thanks for the patience & feedback! |
|
@mgabeler-lee-6rs PR landed 🥇 Thank you for the great contribution! |
|
Thank you @mgabeler-lee-6rs for your perseverance ❤️ It's great to see this work finished and landed 🎉 |
This PR updates the
rest-explorerto, by default, cause the mainRestServerto expose an additional copy of the OpenAPI spec at path that is fixed relative to the explorer itself. The explorer then gives the path to the spec as a relative one.The goal of this is to make the explorer work reliably when behind a reverse proxy that does non-trivial URL transforms (such as placing the app under a path prefix). Since such reverse proxy configurations are generally external to the app, it may be difficult for the app to reliably know the absolute path to itself.
Making it use a relative URL for the OpenAPI spec means that it doesn't need to care at all about path or hostname transformations a proxy might apply.
This behavior is enabled by default, but possible to disable in case some corner case I've not thought of requires the old behavior.
This should fix #2285.
This replaces #2293. See that PR for useful background, discussion, and links to more of the same.
Checklist
npm testpasses on your machinepackages/cliwere updatedexamples/*were updatedexamples/todo/package.jsonto make the tests happy, not sure if this was "proper"