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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from May 13, 2019

Conversation

@bajtos
Copy link
Member

commented May 9, 2019

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 馃憟

@bajtos bajtos requested review from raymondfeng and nabdelgadir May 9, 2019

@bajtos bajtos self-assigned this May 9, 2019

@bajtos bajtos added this to the May 2019 milestone milestone May 9, 2019

@bajtos bajtos force-pushed the fix/base-path-in-explorer branch from 94102bd to c8db7eb May 9, 2019

@bajtos bajtos referenced this pull request May 9, 2019

Closed

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

3 of 4 tasks complete
@raymondfeng

This comment has been minimized.

Copy link
Member

commented May 9, 2019

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

@raymondfeng

This comment has been minimized.

Copy link
Member

commented May 9, 2019

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/;
        }
  1. Start with nginx at http://localhost:8080
  2. 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

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

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.

@bajtos bajtos requested a review from hacksparrow May 10, 2019

@nabdelgadir

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

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:

Screen Shot 2019-05-10 at 9 06 44 AM

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

@bajtos

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

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

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

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);

@bajtos bajtos force-pushed the fix/base-path-in-explorer branch from c8db7eb to e3516db May 13, 2019

@nabdelgadir

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

@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.

  1. 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.

  1. 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:

Screen Shot 2019-05-13 at 8 37 42 AM

bajtos added some commits May 9, 2019

fix(rest-explorer): exclude basePath from /openapi URL
Endpoints serving OpenAPI spec ignore basePath setting, i.e. even if
basePath is set to `/api`, the spec is served at `/openapi.json`.

Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
fixup! rework tests and fix express composition
Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
@bajtos

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

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 bajtos force-pushed the fix/base-path-in-explorer branch from e3516db to 2fdc30f May 13, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

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
Copy link
Member

left a comment

馃憤

@nabdelgadir
Copy link
Contributor

left a comment

馃挴

@bajtos bajtos merged commit 842415b into master May 13, 2019

4 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.007%) to 91.69%
Details

@bajtos bajtos deleted the fix/base-path-in-explorer branch May 13, 2019

@emonddr emonddr self-requested a review May 13, 2019

@emonddr
Copy link
Contributor

left a comment

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 :)

@bajtos bajtos referenced this pull request May 14, 2019

Merged

refactor(rest-explorer): improve test names #2884

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.