loopbackio / loopback-next

LoopBack makes it easy to build modern API applications that require complex integrations.
https://loopback.io
Other
4.94k stars 1.06k forks source link

Dynamic binding/rebinding of controllers after app start #433

Closed bajtos closed 4 years ago

bajtos commented 7 years ago

Support hot-reloading of controllers after the app started. See Application.prototype._setupHandlerIfNeeded().

Example test case:

const app = new Application();
await app.start();
app.controller(MyController);
// MyController is available via REST API now

Acceptance criteria

When the application binds a controller after the app has started, the application picks up the endpoints contributed by the new controller.

Nice to have:

lindoelio commented 6 years ago

Hey guys! I'll working on this issue. As soon as possible I will send my suggestion on a PR. Let me know if you have any advise for me about it :-)

dhmlau commented 6 years ago

@lindoelio, are you still interested in submitting a PR?

dhmlau commented 6 years ago

@bajtos , what is the use case for hot-reloading? I'm thinking users would add controller during development time, so it's not so critical to have the hot reloading feature.

Discussed with @virkt25, this is a nice-to-have feature, but we can leave it post-GA given there are higher priority items for GA.

bajtos commented 6 years ago

I am fine with removing this item from 4.0 GA scope.

what is the use case for hot-reloading?

For example, IBM AppConnect uses LoopBack to define models & REST APIs on the fly. Their LoopBack server provides REST APIs to define (create) and remove connections to 3rd-party databases/services. When a new 3rd party database/service is connected, AppConnect need to create a set of models & controllers allowing its clients to access the new database/service through the LoopBack runtime.

oleg269 commented 5 years ago

Hi,

I'll working on this issue.

I tried load controller after the app started and used app.controller(myController).

Controller was added to app registry, but routes of controller were not created. I found the solution to add routes of controller:

Change in loopback-next/packages/rest/src/rest.server.ts

protected get httpHandler(): HttpHandler {

to

public get httpHandler(): HttpHandler {

and add:

app.controller(TestController);
var restServer: RestServer = await app.get<RestServer>('servers.RestServer');

const b: any = await restServer.find('controllers.TestController');
const ctor = b[0].valueConstructor;
const apiSpec = await getControllerSpec(ctor);
if (apiSpec.components && apiSpec.components.schemas) {
  restServer.httpHandler.registerApiDefinitions(apiSpec.components.schemas);
}
const controllerFactory = await createControllerFactoryForBinding('controllers.TestController');
await restServer.httpHandler.registerController(apiSpec, ctor, controllerFactory);

After adding this code routes of controller were created.

I tried add this code to Application.controller function (loopback-next/packages/core/src/application.ts). But I can't do it. loopback-next/packages/rest/src/rest.server.ts from rest package depends from '@loopback/core' and after adding the code to Application.controller I receive circular dependency between core and rest packages.

The issue 433 was opened one year ago, and currently outdated, bacause _httpHandler from Application class (core package) was moved to RestServer class from rest package (loopback-next/packages/rest/src/rest.server.ts, https://github.com/strongloop/loopback-next/blob/master/packages/rest/src/rest.server.ts#L265).

For resolve this issue looks like we have to do some changes:

  1. add unregisterController function to Application class (loopback-next/packages/core/src/application.ts)
  2. add registerController, unregisterController functions to RestServer (loopback-next/packages/rest/src/rest.server.ts);
  3. add unregisterController, unregisterApiDefinitions, unregisterRoute to HttpHandler class (loopback-next/packages/rest/src/http-handler.ts)

And for dynamic binding/rebinding of controllers after app start we can use

const app = new Application();
await app.start();
app.controller(MyController);
var restServer: RestServer = await app.get<RestServer>('servers.RestServer');
restServer.registerController('MyController');

or

var restServer: RestServer = await app.get<RestServer>('servers.RestServer');
restServer.unregisterController('MyController');
app.unregisterController(MyController);
raymondfeng commented 5 years ago

@oleg269 Please check out https://github.com/strongloop/loopback-next/pull/2111

oleg269 commented 5 years ago

@bajtos @raymondfeng

@raymondfeng Your solution is not helpful to add routes of the controller. app.controller method only add binding to the context but doesn't add routing and doesn't update api definitions.

Please see solution in my previous comment https://github.com/strongloop/loopback-next/issues/433#issuecomment-443512327

raymondfeng commented 5 years ago

@oleg269 #2122 Will be the plumbing to fully support dynamic routes. We'll have to improve RestServer/RoutingTable implementation to respect the requirement that routes cannot be discovered once upon start.

raymondfeng commented 5 years ago

In your case, the current implementation has the following class hierarchy:

RestServer --> HttpHandler --> RoutingTable --> TrieRouter

The RestServer discovers all controllers from the context and calls HttpHandler register* APIs which in turn delegates to RoutingTable and TrieRouter. The current implementation is a bit messy as it has too many APIs that can change the routes, some of them create route bindings while others directly touch the TrieRouter. IMO, they should be unified under route bindings as the source of truth for routing.

To fully allow the dynamic registration of controllers, we need to make sure one of the following happens:

  1. RestServer subscribes to context events for controller bindings. Upon controller binding events, RestServer resets HttpHandler and rebuilds it with the latest list of controllers
  2. Turns TrieRouter into a ContextListener, and reacts on controller bindings.
oleg269 commented 5 years ago

@raymondfeng

You are want after add controller reset HttpHandler and rebuilds it with the latest list of controllers. Are you think is a good flow if I want to add dynamically 50 controllers? After each addition of a new controller we have to reset HttpHandler or we can add/delete routes and openapi definitions?

oleg269 commented 5 years ago

@raymondfeng

I checkout your pull request #2122 and updated RestServer to subscribes to context events for controller bindings. After start application RestServer listening on 'bind' event of all loading controllers. I found out solution how to redefine routes in express js. Possible solution: we have to refactor RestServer._setupHandlerIfNeeded function to cleanup and recreate loopback-next configuration of routes, openapi definitions, etc. and call this function after receive 'bind'/'unbind' events of all loading controllers.

raymondfeng commented 5 years ago

@oleg269 That's the intent of #2122. We need to clean up RestServer --> HttpHandler --> RoutingTable --> TrieRouter to see how routes can be best refreshed.

bajtos commented 5 years ago

FWIW, I think this feature is pretty much blocked by https://github.com/strongloop/loopback-next/pull/2122 and has to wait until that foundational pull request is landed.

t-zhao commented 5 years ago

Hey there. Just wondering any plans for this issue?

bajtos commented 5 years ago

2122 has been landed, I think this feature is no longer blocked.

@t-zhao we don't have any immediate plans to implement this feature, would you like to contribute it yourself? We are happy to help you along the way.

bajtos commented 4 years ago

https://github.com/strongloop/loopback-next/pull/4235/commits/6ce22282ba92cd103a22195879f4ca3e38e0b6d1 shows a quick fix that may be good enough for short term.

For longer term, this feature should use ContextView watcher. Cross-posting https://github.com/strongloop/loopback-next/pull/4235#discussion_r353903856 by @raymondfeng:

I would rather to use a ContextView to invalidate the routing cache.

dhmlau commented 4 years ago

@bajtos, could you please update the ticket with acceptance criteria? Thanks!

emonddr commented 4 years ago

REST server sees all the routes. Routes built from all controllers Routes get set in routing table Need to find a cleaner way to refresh the cache.

The context view is a mechanism to watch and refresh. Perhaps this angle should be used to manage the cache.

bajtos commented 4 years ago

The context view is a mechanism to watch and refresh. Perhaps this angle should be used to manage the cache.

I think the recently introduced ContextEventListener (see https://github.com/strongloop/loopback-next/pull/4451) could be a better solution, because it's simpler to use and possibly more performant.

bajtos commented 4 years ago

FYI: I added the following "Acceptance Criteria":

When the application binds a controller after the app has started, the application picks up the endpoints contributed by the new controller.

  • [ ] REST server is updated with new endpoints, so that clients can access the new API.
  • [ ] OpenAPI specification of the application is updated to describe the new endpoints.

Nice to have:

  • [ ] When a controller is removed from the application (e.g. via app.unbind() API), the endpoints are removed from the REST server and the OpenAPI spec.

/cc @dhmlau @emonddr

dhmlau commented 4 years ago

PR https://github.com/strongloop/loopback-next/pull/4679 has landed. Closing as done.