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

Dynamic binding/rebinding of controllers after app start #433

Closed
3 tasks
bajtos opened this issue Jul 13, 2017 · 21 comments
Closed
3 tasks

Dynamic binding/rebinding of controllers after app start #433

bajtos opened this issue Jul 13, 2017 · 21 comments

Comments

@bajtos
Copy link
Member

bajtos commented Jul 13, 2017

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.

  • 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.
@dhmlau dhmlau changed the title Hot-reloading of controllers after app start Dynamic binding/rebinding of controllers after app start Aug 23, 2017
@bajtos bajtos added the non-MVP label Nov 2, 2017
@lindoelio
Copy link

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

dhmlau commented Aug 20, 2018

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

@dhmlau
Copy link
Member

dhmlau commented Aug 22, 2018

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

bajtos commented Aug 30, 2018

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

oleg269 commented Dec 2, 2018

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

@oleg269 Please check out #2111

@oleg269
Copy link

oleg269 commented Dec 10, 2018

@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 #433 (comment)

@raymondfeng
Copy link
Contributor

@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
Copy link
Contributor

raymondfeng commented Dec 10, 2018

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

oleg269 commented Dec 11, 2018

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

oleg269 commented Dec 13, 2018

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

bajtos added a commit that referenced this issue Dec 9, 2019
A short-term workaround for the missing feature tracked by
#433

Signed-off-by: Miroslav Bajtoš <[email protected]>
@bajtos
Copy link
Member Author

bajtos commented Dec 9, 2019

6ce2228 shows a quick fix that may be good enough for short term.

For longer term, this feature should use ContextView watcher. Cross-posting #4235 (comment) by @raymondfeng:

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

@dhmlau
Copy link
Member

dhmlau commented Dec 18, 2019

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

@emonddr
Copy link
Contributor

emonddr commented Jan 7, 2020

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

bajtos commented Jan 23, 2020

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 #4451) could be a better solution, because it's simpler to use and possibly more performant.

@dhmlau dhmlau removed this from the Jan 2020 milestone Jan 24, 2020
@bajtos
Copy link
Member Author

bajtos commented Feb 3, 2020

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 dhmlau added this to the Feb 2020 milestone Feb 3, 2020
raymondfeng added a commit that referenced this issue Feb 18, 2020
raymondfeng added a commit that referenced this issue Feb 18, 2020
raymondfeng added a commit that referenced this issue Feb 18, 2020
raymondfeng added a commit that referenced this issue Feb 18, 2020
raymondfeng added a commit that referenced this issue Feb 19, 2020
raymondfeng added a commit that referenced this issue Feb 24, 2020
raymondfeng added a commit that referenced this issue Feb 24, 2020
… is started

Implements #433

- use tags to identify regular routes and controller routes
- use context observer to watch route related events
raymondfeng added a commit that referenced this issue Feb 24, 2020
… is started

Implements #433

- use tags to identify regular routes and controller routes
- use context observer to watch route related events
@dhmlau
Copy link
Member

dhmlau commented Feb 25, 2020

PR #4679 has landed. Closing as done.

@dhmlau dhmlau closed this as completed Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants