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

PoC showing how to mount an LB3 app and include its REST API in OpenAPI spec [DO NOT MERGE] #2318

Closed
wants to merge 29 commits into from

Conversation

Projects
None yet
5 participants
@bajtos
Copy link
Member

bajtos commented Jan 31, 2019

A proof of concept showing how to expose a LB3 app (loopback-getting-started) via LB4 REST API (see #2301). The solution presented in this pull request is a somewhat alternate approach to #2274, see the discussion in that PR for context.

Under the hood, the solution has the following major parts:

  • app.mountExpressRouter(basePath, router, spec) allows LB4 app
    developers to add arbitrary set of Express routes and provide OpenAPI spec.
  • Lb3AppBooter responsible for setting up LoopBack 3 application, obtaining
    OpenAPI spec describing app's REST API and mounting the app on LB4.
  • Example app

See SPIKE NOTES for further details.

Related: #1849 #2301

Checklist

  • 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

@bajtos bajtos self-assigned this Jan 31, 2019

@bajtos bajtos requested review from raymondfeng, hacksparrow and strongloop/loopback-next Jan 31, 2019

Show resolved Hide resolved examples/lb3app/CHANGELOG.md Outdated
Show resolved Hide resolved examples/lb3app/LICENSE Outdated
Show resolved Hide resolved examples/lb3app/README.md Outdated
@hacksparrow

This comment has been minimized.

Copy link
Member

hacksparrow commented Jan 31, 2019

Quick question first: What's the advantage of mounting an LB3 app on an LB4 app (esp, at this stage) over hosting a stand-alone LB3 app? Who are the target users?

@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Jan 31, 2019

What's the advantage of mounting an LB3 app on an LB4 app (esp, at this stage) over hosting a stand-alone LB3 app? Who are the target users?

Good question. Can you @raymondfeng and @richardpringle answer please?

In my understanding, when you mount an LB3 app inside an LB4 app, you can more easily migrate your codebase in small increments. For example, you can create LB4 controllers that call out to LB3 models for the actual implementation. Or maybe when adding a new feature, you can implement it in LB4 style (LB4 model, repository & controller), and keep the existing APIs served by the legacy LB3 codebase.

@richardpringle

This comment has been minimized.

Copy link

richardpringle commented Feb 3, 2019

@hacksparrow, the advantage of mounting an LB3 app inside an LB4 is that everything is packaged together. You don't have to have separate deployments processes etc. I highly doubt that a larger organization that already has experience maintaining multiple legacy applications would see any advantage to loading the two apps together, but for those smaller orgs with less resources, it makes maintenance easier.

You can also run both apis on the same port instead of having to set up nginx or proxy based on path prefix.

All of these things are fairly straight forward, but each of them contributes to a build up of friction when it comes to migrating over to LB4. Make sense?

@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Feb 4, 2019

Thank you for thoughtful comments.

As I was thinking about this PoC code some more, I realized it's pointing to a missing extension/composition point in LB4.

Essentially, we have a custom router (the LB3 app with its REST handler) providing a set of routes, and the accompanying OpenAPI spec describing those routes. In mind, this is similar to existing building blocks we already have in LB4: controller/handler routes managed by our RoutingTable, StaticAssetsRoute managing routes for static assets (with no OpenAPI spec).

Here is the request handling pipeline as configured in the top-level Express app:

  • Request preprocessing middleware, e.g. CORS.
  • Check RoutingTable if there is a controller/handler route matching the request. If not:
  • Check StaticAssetsRoute if there is a static asset matching the request.

I am thinking about inserting a "custom routers" extension into this list:

  • Request preprocessing middleware, e.g. CORS.
  • Check RoutingTable if there is a controller/handler route matching the request. If not:
  • EXTENSION POINT: allow custom routers to handle the request
  • Check StaticAssetsRoute if there is a static asset matching the request.

With this infrastructure in place, LB3-to-LB4 bridge can use a standard approach and simply export the custom router powered by LB3. No hacky integration with the REST layer is needed.

@raymondfeng what do you think?

@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Feb 4, 2019

Another use case for a custom router: GraphQL endpoints implemented by oasgraph, see #1905 (comment)

@hacksparrow

This comment has been minimized.

Copy link
Member

hacksparrow commented Feb 4, 2019

Thanks @richardpringle.

@bajtos bajtos force-pushed the spike/add-lb3app-to-lb4 branch 4 times, most recently from 0a57cf8 to ade46d8 Feb 7, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Feb 7, 2019

I am thinking about inserting a "custom routers" extension into this list:

  • Request preprocessing middleware, e.g. CORS.
  • Check RoutingTable if there is a controller/handler route matching the request. If not:
  • EXTENSION POINT: allow custom routers to handle the request
  • Check StaticAssetsRoute if there is a static asset matching the request.

In ade46d8, I introduced a new API app.mountExpressRouter(basePath, spec, router) that simplifies the code in the actual application a lot. What do you think about this approach, @raymondfeng @hacksparrow?

The implementation details are wrong, I need to move the code invoking express routers in between LB4 router and static assets. I am not expecting any changes in the user-facing API in Application and RestServer though.

@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Feb 8, 2019

@raymondfeng @hacksparrow ping 👋

In ade46d8, I introduced a new API app.mountExpressRouter(basePath, spec, router) that simplifies the code in the actual application a lot. What do you think about this approach, @raymondfeng @hacksparrow?

The implementation details are wrong, I need to move the code invoking express routers in between LB4 router and static assets. I am not expecting any changes in the user-facing API in Application and RestServer though.

@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Feb 8, 2019

In ade46d8, I introduced a new API app.mountExpressRouter(basePath, spec, router) that simplifies the code in the actual application a lot. What do you think about this approach, @raymondfeng @hacksparrow?

Maybe switch the order of spec and router? Also make spec optional?

IMO, exposing one extensible container for express routers is a good starting point. Ultimately, we still have to make the pipeline more flexible for composition - either phase based or something else.

bajtos added some commits Jan 31, 2019

bajtos added some commits Feb 12, 2019

@bajtos bajtos force-pushed the spike/add-lb3app-to-lb4 branch from 3a39c96 to 8e95629 Feb 12, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Feb 12, 2019

There are CI failures too - maybe related to Node 8.

loopback-getting-started uses externally hosted database and runs automigration at start. I think this creates a race condition where one Travis CI runner deletes old tables while another Travis CI runner is already trying to insert sample (seed) data.

@hacksparrow

This comment has been minimized.

Copy link
Member

hacksparrow commented Feb 12, 2019

@bajtos will there be any differences in the mounted app and an app that's running independently, feature and functionality wise?

@@ -144,14 +136,14 @@ export class RoutingTable {
}

// this._staticAssetsRoute will be set only if app.static() was called

This comment has been minimized.

Copy link
@hacksparrow

hacksparrow Feb 12, 2019

Member

Update this comment?

@hacksparrow
Copy link
Member

hacksparrow left a comment

Once that outdated comment is updated, it looks pretty good. 👏🏻

@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Feb 12, 2019

will there be any differences in the mounted app and an app that's running independently, feature and functionality wise?

I believe the behavior should be exactly the same. The only difference I am aware of is the version of OpenAPI spec - LB3 produces OpenAPI v2 (a.k.a Swagger), LB4 produces OpenAPI v3.

There may be a subtle difference in how CORS middleware installed by LB4 interacts with the CORS middleware installed by LB3. I don't know enough to be able to provide more details.

@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Feb 14, 2019

I have created the following follow-up issues and closing this spike as done.

  • Mount Express router with OpenAPI spec #2389
  • Docs: request-handling pipeline #2390
  • Boot and mount LB3 app #2391
  • Example: mount LB3 app in LB4 #2392
  • Migration guide: mount LB3 in LB4 #2393
  • Spike: dynamically updated LB3 API (routes + spec) #2394
@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Feb 26, 2019

I created a new Epic to keep track of all follow-up issues in one place: #2479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.