fix(rest-explorer): exclude basePath from /openapi URL

[bajtos]

Endpoints serving OpenAPI spec ignore basePath setting, i.e. even if basePath is set to /api, the spec is served at /openapi.json.

This pull request is fixing the problem and supersedes #2554 and #2854.

The following four use cases are correctly supported now (when sending requests directly to the application):

1. No base path, everything is at /.
2. basePath is set via app.basePath(). Explorer is served at /api/explorer, OpenAPI spec is served at /openapi.json.
3. LB app mounted on Express at /api, no LB basePath is set. Explorer is served at /api/explorer, OpenAPI spec is served at /api/openapi.json.
4. LB app mounted on Express at /api, basePath is set to v1. Explorer is served at /api/v1/explorer, OpenAPI spec is served at /api/openapi.json.

Support for reverse proxies like nginx is OUT OF SCOPE of this pull request.

/cc @gordancso @dkrantsberg @sanadHaj

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

[raymondfeng]

I'll validate this PR with my Nginx + LB4 setup.

[raymondfeng]

The PR does not fix the issue with the following setup:

1. Configure nginx as a reverse proxy for a LB4 application (http://127.0.0.1)

location /api {
            proxy_set_header        Host               $host;
            proxy_set_header        X-Real-IP          $remote_addr;
            proxy_set_header        X-Forwarded-For    $proxy_add_x_forwarded_for;
            proxy_set_header        X-Forwarded-Host   $host:$server_port;
            proxy_set_header        X-Forwarded-Server $host;
            proxy_set_header        X-Forwarded-Port   $server_port;
            proxy_set_header        X-Forwarded-Proto  $scheme;
            proxy_pass   http://127.0.0.1:3000/;
        }

2. Start with nginx at http://localhost:8080
3. Open http://localhost:8080/api

The API Explorer UI now resolves /explorer to http://localhost:8080 (correct one should be http://localhost:8080/api.

Without nginx passes the original request url /api to the LB4 application, LB4 cannot infer urls in its responses. It has to be done on the reverse proxy side, such as using sub_filter. But I was not able to achieve that.

One thing I can imagine is to add request headers so that LB4 app knows the proxy url:

proxy_set_header        X-Forwarded-URL    $request_uri;

# or

proxy_set_header        X-Forwarded-Base-Path    /api;

If the nginx is dedicated for one LB4 app, we can change the conf to be:

location / {
# ...
}

This configuration works out of box.

[bajtos]

The PR does not fix the issue with the following setup:

@raymondfeng It is not my intention to fix nginx setup. My intention is to fix LoopBack to correctly support the following four use cases when sending requests directly to the application:

1. No base path, everything is at /.
2. basePath is set via app.basePath. Explorer is served at /api/explorer, OpenAPI spec is served at /openapi.json.
3. LB app mounted on Express at /api, no LB basePath is set. Explorer is served at /api/explorer, OpenAPI spec is served at /api/openapi.json.
4. LB app mounted on Express at /api, basePath is set to v1. Explorer is served at /api/v1/explorer, OpenAPI spec is served at /api/openapi.json.

Everything else is out of scope of this pull request.

I agree it would be great to improve support for running behind a reverse proxy like nginx, but let's open a new pull request for that and don't delay landing of this fix, which I useful on its own IMO.

[nabdelgadir]

I tried making an application and added this.basePath('/api') and the console printed the correct urls:

Server is running at http://[::1]:3000/api
Try http://[::1]:3000/api/ping

The home page is correctly served at http://[::1]:3000/api, but the explorer doesn't work:

It's still looking for /api/openapi.json.

[bajtos]

Thank you @nabdelgadir for checking. I guess I should not rely on my automated tests only and run an example app too. I'll take a look next week.

[bajtos]

I tried making an application and added this.basePath('/api')

@nabdelgadir I am not able to reproduce :( Is it possible that your example app is perhaps using @loopback/rest-explorer from npm, not the version from my pull request?

I am testing the changes using Todo example app an everything works fine. Here is the patch to apply:

diff --git a/examples/todo/public/index.html b/examples/todo/public/index.html
index 861687005..54b32126e 100644
--- a/examples/todo/public/index.html
+++ b/examples/todo/public/index.html
@@ -59,7 +59,7 @@
     <h1>@loopback/example-todo</h1>

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

   <footer class="power">
diff --git a/examples/todo/src/application.ts b/examples/todo/src/application.ts
index 9ee0dc685..4022b893a 100644
--- a/examples/todo/src/application.ts
+++ b/examples/todo/src/application.ts
@@ -18,6 +18,8 @@ export class TodoListApplication extends BootMixin(
   constructor(options: ApplicationConfig = {}) {
     super(options);

+    this.basePath('/api');
+
     // Set up the custom sequence
     this.sequence(MySequence);

[nabdelgadir]

@bajtos ah you're right. I was using npm link but I guess it didn't link it properly.

1. No base path, everything is at /.
2. basePath is set via app.basePath. Explorer is served at /api/explorer, OpenAPI spec is served at /openapi.json.

I tested the above two with the todo example and they both work.

3. LB app mounted on Express at /api, no LB basePath is set. Explorer is served at /api/explorer, OpenAPI spec is served at /api/openapi.json.

I tested this one with express-composition and it works.

4. LB app mounted on Express at /api, basePath is set to v1. Explorer is served at /api/v1/explorer, OpenAPI spec is served at /api/openapi.json.

I also tested this one with express-composition and OpenAPI spec is correctly served at /api/openapi.json, but the explorer looks for /api/v1/openapi.json:

[bajtos]

I also tested this one with express-composition and OpenAPI spec is correctly served at /api/openapi.json, but the explorer looks for /api/v1/openapi.json

Good catch! I turns out my new tests were written incorrectly�. They were calling expect(response.body).to.match(...), but because the response was HTML, response.body was set to {}, which somehow always passed match check. (The actual response body is in response.text.)

I reworked my new tests to use expect from supertest, as we already do in other tests. That revealed the problem you discovered by manual testing, after which I fixed the code to correctly compute OpenAPI spec URL when a LB4 app with basePath is mounted on Express.

To make the review easier, I created a new commit for these changes. I'll squash it with the first commit before landing.

@nabdelgadir LGTY now?

[bajtos]

Here is the patch I used to test express composition with a custom basePath:

diff --git a/examples/express-composition/public/express.html b/examples/express-composition/public/express.html
index 0dd105688..673be559e 100644
--- a/examples/express-composition/public/express.html
+++ b/examples/express-composition/public/express.html
@@ -15,7 +15,7 @@
   <body>
     <h2>Express</h2>
     <h4 style="font-weight:normal;">This is the main Express page.
-      Click <a href='/api/'>here</a> for the LoopBack main page and
+      Click <a href='/api/v1'>here</a> for the LoopBack main page and
       <a href='/hello/'>here</a> to say hi.</h4>
   </body>

diff --git a/examples/express-composition/src/server.ts b/examples/express-composition/src/server.ts
index a60f1f577..80684405a 100644
--- a/examples/express-composition/src/server.ts
+++ b/examples/express-composition/src/server.ts
@@ -19,6 +19,7 @@ export class ExpressServer {
   constructor(options: ApplicationConfig = {}) {
     this.app = express();
     this.lbApp = new NoteApplication(options);
+    this.lbApp.basePath('/v1');

     // Expose the front-end assets via Express, not as LB4 route
     this.app.use('/api', this.lbApp.requestHandler);

[hacksparrow]

👍

[nabdelgadir]

💯

[emonddr]

some mocha test title state supports basePath but comments within a test state basepath not supported . Please update test titles to make things clearer; where possible. Thanks :)