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

Open
bajtos opened this issue Jul 13, 2017 · 19 comments
Open

Dynamic binding/rebinding of controllers after app start #433

bajtos opened this issue Jul 13, 2017 · 19 comments

Comments

@bajtos
Copy link
Member

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

This comment has been minimized.

Copy link

@lindoelio lindoelio commented Jan 18, 2018

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

@bajtos bajtos removed the needs-priority label Jun 25, 2018
@dhmlau

This comment has been minimized.

Copy link
Member

@dhmlau dhmlau commented Aug 20, 2018

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

@dhmlau

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

@raymondfeng raymondfeng commented Dec 4, 2018

@oleg269 Please check out #2111

@oleg269

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

@raymondfeng raymondfeng commented Dec 10, 2018

@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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

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

@raymondfeng

This comment has been minimized.

Copy link
Member

@raymondfeng raymondfeng commented Dec 17, 2018

@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

This comment has been minimized.

Copy link
Member Author

@bajtos bajtos commented Jan 4, 2019

FWIW, I think this feature is pretty much blocked by #2122 and has to wait until that foundational pull request is landed.

@t-zhao

This comment has been minimized.

Copy link

@t-zhao t-zhao commented May 23, 2019

Hey there. Just wondering any plans for this issue?

@bajtos

This comment has been minimized.

Copy link
Member Author

@bajtos bajtos commented Jun 14, 2019

#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 added a commit that referenced this issue Dec 2, 2019
A short-term workaround for the missing feature tracked by
#433

Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
@bajtos bajtos mentioned this issue Dec 5, 2019
2 of 2 tasks complete
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š <mbajtoss@gmail.com>
@bajtos

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

@dhmlau dhmlau commented Dec 18, 2019

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

@dhmlau dhmlau mentioned this issue Dec 18, 2019
1 of 21 tasks complete
@dhmlau dhmlau added this to the Jan 2020 milestone Dec 19, 2019
@emonddr emonddr mentioned this issue Jan 7, 2020
12 of 24 tasks complete
@dhmlau dhmlau added the 2020Q1 label Jan 7, 2020
@emonddr

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member Author

@bajtos 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
@dhmlau dhmlau removed the needs grooming label Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.