fix(rest): use context.get() to resolve current controller #993

raymondfeng · 2018-02-13T23:04:47Z

The PR ensures the existing controller binding is used to resolve/invoke the controller method. It allows scope and configuration of a controller is honored. Please note instantiateClass always creates a new instance.

If a route is set up without binding the controller to a context (app), we now create a binding in the request context.

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
Related API Documentation was updated
Affected artifact templates in packages/cli were updated
Affected example projects in packages/example-* were updated

bajtos · 2018-02-14T09:17:31Z

I have several reservations against the changes proposed here.

First of all, let's discuss the high-level design. As I understand this problem, it's caused by the fact that our RoutingTable does not require controllers to be bound in the parent of requestContext. At the same time, most of the controllers are registered for routing via app.controller that calls app.bind() under the hood. As a result, we have two different ways how to register a controller, each one may have different expectations in regards to the behaviour of ctx.get(controllerKey):
```
app.controller(ProductController);
app.getSync('servers.RestServer').route('GET', '/products', {/*spec*/}, ProductController, 'findAll');
```
In your proposal, the second use case will bind ProductController in the request context under controllers.ProductController key. I have mixed feelings about that approach - should't the route() API have created that binding in the first place? Do our users actually want us to register such binding? Wouldn't they use app.controller API in such case?

It would be great if we could clean up this inconsistency, instead of adding more code built on top of it.

I am also concerned that your proposal is introducing deep coupling between the way how controllers are registered via app.controller & how the binding key is created, and the internal implementation of REST routing table. So far, the REST routing table was pretty independent and unopinionated about the way how controllers are registered.

One option that comes to my mind is to modify ControllerRoute to accept a controller factory function instead of a controller constructor. That way controllers registered via app.controller can be instantiated via ctx.get (while preserving scope), and controllers registered via route API can use any custom factory implementation.
What is the use case you have in your mind where the current controller instance would be retrieved via it's controllers.{name} binding key? Should we add controller.current.instance binding instead/in addition to your proposed changes? We already have controller.current.ctor.
If we stick to your currently proposed design: I am always reluctant to expose Binding outside of the context module. As I see it, it's should be an internal implementation detail of our context module .
(EDITED 2018-02-14 10:29)
~~I have a simpler solution leveraging ctx.contains that passes your newly added test - see 472d9da. Are there any edge cases I missed?~~
I have a simpler solution leveraging ctx.isBound that passes your newly added test - see 40ac04d. Are there any edge cases I missed?

bajtos

Let's address my high-level comments above first.

Once we get to implementation details, I have one comment below.

Additionally, I'd like to see more test coverage for this change:

A controller bound via app.bind() in the default TRANSIENT scope. (Even if we change app.controller() to bind in CONTEXT scope, then it's still possible to bind TRANSIENT controller via app.bind() directly.)
A controller bound via app.bind() in CONTEXT (child context?) scope.
A controller route bound via restServer.route API, where the controller class is not bound.

UPDATED:
By test coverage, I mean tests specifically verifying whether this is the same object as what's injected by @inject.

bajtos · 2018-02-14T09:33:39Z

packages/rest/test/acceptance/routing/routing.acceptance.ts

@@ -499,7 +525,7 @@ describe('Routing', () => {
  }

  function givenControllerInApp(app: Application, controller: ControllerClass) {
-    app.controller(controller);
+    app.controller(controller).inScope(BindingScope.CONTEXT);


IMO, all controllers should be scoped to the request context. Can you think of a use case where we would want multiple controller instances in a single request context?

Originally, I wanted to propose to add inScope call to app.controller(). On the second thought, I am not sure if it's a good idea - what happens when somebody calls app.get('controllers.ProductController'), will future calls of requestContext.get('controllers.ProductController') return the value cached at app-level? Maybe we need a new scope called CHILD_CONTEXT?

The CONTEXT scope should be the default for controllers. It is designed to work nicely with your use case:

app.get('controllers.ProductController') --> create and cache an instance at app level

requestContext.get('controllers.ProductController') --> Now it's the req context and a new instance will be created and cached at requestContext level

requestContext.get('controllers.ProductController') --> Hits the cache, get the same instance as 2

anotherRequestContext.get('controllers.ProductController') --> Now it's another req context and a new instance will be created and cached at anotherRequestContext level

Cool. In that case, I am proposing to:

Call inScope from app.controller() to automatically register all controllers in CONTEXT scope.

Add a test to verify the behavior described in your comment. For example:

Register a controller class that stores state in the instance.

Call app.get, mutate the state stored in the controller instance.

Make an HTTP request (this will get controller instance from the request context), verify that the response contains original (not mutated) state.

raymondfeng · 2018-02-14T16:42:31Z

@bajtos Thanks for the feedback.

Yes, I agree with you it's better to clean up the design by answering the following questions:

What is x-controller-name? Is it the name of the controller class? The current implementation uses controllers.${x-controller-name} to find the bound controller.
How does the route create an instance of the controller within the current context?

What's the minimal metadata required? Hopefully it can be independent of Context.
Can we unify all the following cases into a factory function?
- a controller instance (singleton)
- controller class constructor
- a factory to create controller (it has to take ctx argument for the req context)
- a binding key
How are controller instances shared by contexts? (scope)

Do we always require a controller to be bound in the context? What's the default scope?

bajtos · 2018-02-15T14:52:41Z

@raymondfeng great questions! 👍 I'll need more time to think them through, please allow few more days until I come back again.

bajtos · 2018-03-01T12:29:25Z

What is x-controller-name? Is it the name of the controller class? The current implementation uses controllers.${x-controller-name} to find the bound controller.

At high-level, x-controller-name is a reference allowing the runtime to locate the controller class that's handling the given OpenAPI endpoint. I think we are free to choose how to interpret this value, as long as we make this feature easy to use and understand.

In that light, it makes a lot of sense to me to treat x-controller-name as the name of the controller class. Do you have any better counter-proposal in your mind?

How does the route create an instance of the controller within the current context?
What's the minimal metadata required? Hopefully it can be independent of Context.

As a LoopBack user building an application, I'd like my controller-based routes to use our IoC container to obtain an instance of the controller class.

As a developer maintaining LoopBack modules, I'd like the Route implementation to be independent of Context (as you wrote), to keep our routing as much decoupled from IoC as possible.

Can we unify all the following cases into a factory function?

I believe this should be possible. Let's assume a controller route accepts a factory function accepting a single argument ctx.

a controller instance (singleton)

ctx => controllerInstance;

controller class constructor

ctx => instantiateClass(controllerClass, ctx);

This raises the question whether we want to allow Context users to instantiate classes that are not registered in the container by calling instantiateClass directly; or whether we want to force such users to register the classes in the Context first (as you are doing in this pull request) and then use ctx.get to obtain the class instance.

a factory to create controller (it has to take ctx argument for the req context)

ctx => controllerFactory(ctx);

a binding key

ctx => ctx.get(controllerBindingKey);

How are controller instances shared by contexts? (scope)

I don't have an answer for this yet, I think it depends on how we solve the previous problem of registering & obtaining the controller instance.

Do we always require a controller to be bound in the context? What's the default scope?

Ideally, I'd say controllers should not be required to be bound in the context. This will force us to keep our design clean and not coupled with our IoC container too much.

OTOH, depending on how we want to share the controller instances by contexts, it may be more practical to require all controllers to be bound to the context.

As I am thinking about this, I think it can be reasonably easy to achieve by modifying .route() method to bind the controller class in the context for the caller.

app.getSync('servers.RestServer').route('GET', '/products', {/*spec*/}, ProductController, 'findAll');
// under the hood:
app.controller(ProductController);
this.route(
  new ControllerRoute('GET', '/products', {/*spec*/}, ProductController, 'findAll'),
);

@raymondfeng what are your thoughts?

raymondfeng · 2018-03-02T19:07:29Z

What is x-controller-name? Is it the name of the controller class? The current implementation uses controllers.${x-controller-name} to find the bound controller.

At high-level, x-controller-name is a reference allowing the runtime to locate the controller class that's handling the given OpenAPI endpoint. I think we are free to choose how to interpret this value, as long as we make this feature easy to use and understand.

In that light, it makes a lot of sense to me to treat x-controller-name as the name of the controller class. Do you have any better counter-proposal in your mind?

There are two options to interpret x-controller-name I have in mind:

The 2nd part of binding name for a controller, for example, controllers.my-controller will have x-controller-name to be my-controller.
The controller class name.

Option 1 gives us one more level of abstraction and treat x-controller-name as the address. It allows the same controller class to be configured and bound under different keys.

Option 2 is easier to explain but it requires us to use the class name to find the corresponding controller (mostly from the context).

How does the route create an instance of the controller within the current context?
What's the minimal metadata required? Hopefully it can be independent of Context.

As a LoopBack user building an application, I'd like my controller-based routes to use our IoC container to obtain an instance of the controller class.

+1. I see Context as the knowledge base for my app. It should be able to provision/resolve an instance of the controller class when needed.

As a developer maintaining LoopBack modules, I'd like the Route implementation to be independent of Context (as you wrote), to keep our routing as much decoupled from IoC as possible.

+1.

Can we unify all the following cases into a factory function?

I believe this should be possible. Let's assume a controller route accepts a factory function accepting a single argument ctx.

+1.

a controller instance (singleton)

ctx => controllerInstance;

controller class constructor

ctx => instantiateClass(controllerClass, ctx);

This raises the question whether we want to allow Context users to instantiate classes that are not registered in the container by calling instantiateClass directly; or whether we want to force such users to register the classes in the Context first (as you are doing in this pull request) and then use ctx.get to obtain the class instance.

I'll try to avoid direct invocation of instantiateClass (a backdoor) as much as we can. At least we should bind it to a child context to enforce scoping and configuration.

a factory to create controller (it has to take ctx argument for the req context)

ctx => controllerFactory(ctx);

a binding key

ctx => ctx.get(controllerBindingKey);

How are controller instances shared by contexts? (scope)

I don't have an answer for this yet, I think it depends on how we solve the previous problem of registering & obtaining the controller instance.

CONTEXT scope makes sense to me.

Do we always require a controller to be bound in the context? What's the default scope?

Ideally, I'd say controllers should not be required to be bound in the context. This will force us to keep our design clean and not coupled with our IoC container too much.

OTOH, depending on how we want to share the controller instances by contexts, it may be more practical to require all controllers to be bound to the context.

I prefer to have controllers to be bound to the context to avoid knowledge leakage. All controllers should be managed artifacts by LoopBack context.

As I am thinking about this, I think it can be reasonably easy to achieve by modifying .route() method to bind the controller class in the context for the caller.

app.getSync('servers.RestServer').route('GET', '/products', {/*spec*/}, ProductController, 'findAll');
// under the hood:
app.controller(ProductController);
this.route(
  new ControllerRoute('GET', '/products', {/*spec*/}, ProductController, 'findAll'),
);

I'm fine with on-demand binding of controllers.

@raymondfeng what are your thoughts?

See my comments inline.

bajtos · 2018-03-05T13:19:09Z

I think we are pretty much in agreement then!

There are two options to interpret x-controller-name I have in mind:

The 2nd part of binding name for a controller, for example, controllers.my-controller will have x-controller-name to be my-controller.

The controller class name.

Option 1 gives us one more level of abstraction and treat x-controller-name as the address. It allows the same controller class to be configured and bound under different keys.

Option 2 is easier to explain but it requires us to use the class name to find the corresponding controller (mostly from the context).

Can we perhaps unify those two approaches by treating Option2 as the default way how to obtain the address for Option1? Something along the following line:

x-controller-name is an address used to retrieve a controller instance from the context, it refers to the second part of the binding name controllers.{controller-name}. By default, the address is set to controller's class name.

Thoughts?

bajtos · 2018-03-05T13:29:03Z

As I am thinking about this, I think it can be reasonably easy to achieve by modifying .route() method to bind the controller class in the context for the caller.

app.getSync('servers.RestServer').route('GET', '/products', {/*spec*/}, ProductController, 'findAll');
// under the hood:
app.controller(ProductController);
this.route(
  new ControllerRoute('GET', '/products', {/*spec*/}, ProductController, 'findAll'),
);

I'm fine with on-demand binding of controllers.

My only concern with this approach is an edge case when there are two different controller classes with the same name - we need to be prepared to handle this situation.

An example to illustrate my point is below. It's modeled after CQRS pattern. I know it's a bit silly but but the code is short and easy to understand this way.

// this would typically live in a package/extension
let status: string = 'ok';

function registerStatusQueries(server: RestServer) {
  class StatusController {
    async getStatus() { return status; }
  }
  server.route('GET', '/status', {/*spec*/}, StatusController, 'getStatus');
}

function registerStatusCommands(server: RestServer) {
  class StatusController {
    async updateStatus(newStatus: string) { status = newStatus; }
  }
  server.route('PUT', '/status', {/*spec*/}, StatusController, 'updateStatus');
}

// this lives inside the main application setup
registerStatusQueries(restServer);
registerStatusCommands(restServer);

raymondfeng · 2018-03-07T21:20:37Z

My only concern with this approach is an edge case when there are two different controller classes with the same name - we need to be prepared to handle this situation.
This edge case can be covered by #992.

raymondfeng · 2018-03-09T19:20:00Z

@bajtos I have updated the PR to implement what we have agreed. PTAL.

bajtos · 2018-03-19T14:38:32Z

packages/rest/src/router/routing-table.ts

-      methodName = this.spec['x-operation-name'];
-    }
+    this._controllerCtor = controllerCtor;
+    this._controllerName = controllerName || controllerCtor.name;


controllerName is always set, see L371.

bajtos · 2018-03-19T14:42:57Z

packages/rest/src/http-handler.ts

+  registerController(
+    controllerCtor: ControllerClass,
+    spec: ControllerSpec,
+    factory?: ControllerFactory,


This API is too loose to my taste. For example, it's possible to register a controller in such way that factory is returning an instance of a different class than controllerCtor:

registerController( ProductController, spec, ctx => new CategoryController());

IIUC, the only reason you need a direct access to controllerCtor is to obtain the controller name if the spec does not provide it. If that's correct, then I am proposing to modify this API as follows:

Always require the controller factory.

Replace required controllerCtor with an optional controller name argument.

registerController( factory: ControllerFactory, spec: ControllerSpec, name?: string, )

A similar change should be applied to ControllerRoute class.

Thoughts?

Controller constructor is still critical to look up corresponding metadata for some of the actions. That's why we bind it to controller.current.ctor at the moment.

Controller constructor is still critical to look up corresponding metadata for some of the actions.

Could you be more specific please and list the required metadata?

How do you envision to address the possible edge case issue I pointed above, where the factory may be returning an instance of a different controller class than the class used for registration?

We typically decorate controller classes and their members to provider metadata for actions such as authenticate. Being able to access the controller constructor gives us access to metadata associated with the resolved controller class.

Did you see 2adcaff?

bajtos · 2018-03-19T14:44:18Z

packages/rest/src/http-handler.ts

+    spec: ControllerSpec,
+    factory?: ControllerFactory,
+  ) {
+    this._routes.registerController(controllerCtor, spec);


factory argument is ignored, I think you need to pass it to this._routes.registerController? Please add a test that's failing with the current implementation because of that bug, it will serve as a regression guard for the future.

raymondfeng · 2018-03-23T16:37:53Z

@bajtos PTAL

bajtos · 2018-03-27T12:29:53Z

packages/rest/src/http-handler.ts

+  registerController<T>(
+    controllerCtor: ControllerClass<T>,
+    spec: ControllerSpec,
+    controllerFactory?: ControllerFactory<T>,


I find it a bit weird to have two controller-related arguments interleaved with other non-controller arguments.

I am proposing to reorder the arguments of this method as follows:

registerController<T>( spec: ControllerSpec, controllerCtor: ControllerClass<T>, controllerFactory?: ControllerFactory<T>, ) {}

The same comment applies to other places where controllerFactory argument as added to a function/method accepting controllerCtor.

I would also like to make controllerFactory as a required argument, and treat controllerCtor more like a reference for obtaining controller metadata. This will open more possibilities for us, for example allow controllerFactory to return a plain object (no classes involved) and then craft controllerCtor in such way that allows the rest of the framework to fetch the required metadata.

If we agree on this second change, then I would swap the order of factory/ctor argument:

registerController<T>( spec: ControllerSpec, controllerFactory?: ControllerFactory<T>, controllerCtor: ControllerClass<T>, ) {}

This will also allow us to preserve required methodName while grouping controller-related args in route<T>() API below.

route<T>( verb: string, path: string, spec: OperationObject, controllerFactory: ControllerFactory<T>, controllerCtor: ControllerClass<T>, methodName: string, ){}

Thoughts?

I move controllerFactory to be next to controllerCtor.

In most cases, we have ControllerClass and the controller factory is for the class. With the intention to customize by exception, I leave controllerFactory to be after controlCtor and optional in most cases.

bajtos · 2018-03-27T12:40:07Z

packages/rest/src/router/routing-table.ts

+): ControllerFactory<T> {
+  if (typeof source === 'string') {
+    return ctx => ctx.get<T>(source);
+  } else if (typeof source === 'function') {


These function is doing sort of two things: provide a string-based and class-based factory. I'd like it more to extract these two factories into standalone functions and require users to pick the right factory function when creating a controller route (see my previous comment about a required controllerFactory arg), instead of letting the framework to create the factory function automagically. When I write "users", it does not necessary mean framework users building applications. I can imagine the high-level application API providing the factory function for callers, but keeping our internal design cleaner by making the factory function required.

I would be ok with leaving this out of the scope of this pull request, if it was not affecting the public API in a backwards-incompatible way. If backwards compatibility is not a concern, then I am ok with landing the currently proposed API.

Thoughts?

I split it into three specific functions

raymondfeng · 2018-03-29T22:26:17Z

@bajtos I have addressed your comments. PTAL.

bajtos

I am not able to fully comprehend all implications of the current design. I don't see any obvious problem, so let's move on and land this pull request. We can always fix any problems later, if/when they arise.

bajtos · 2018-04-11T15:54:09Z

packages/rest/src/router/routing-table.ts

-    }
-
+    this._controllerFactory =
+      controllerFactory || createControllerFactoryForClass(controllerCtor);


This is the part I'd prefer to avoid - can we modify ControllerRoute to require the factory argument, and let app.controller and friends to supply the right value?

- move controllFactory param next to controllerCtor - split createControllerFactory to three functions

raymondfeng requested review from bajtos and kjdelisle as code owners February 13, 2018 23:04

raymondfeng added the needs-priority label Feb 13, 2018

raymondfeng force-pushed the use-context-for-controller-instance branch from 9dbdf89 to 48629be Compare February 14, 2018 00:10

bajtos suggested changes Feb 14, 2018

View reviewed changes

raymondfeng force-pushed the use-context-for-controller-instance branch from 48629be to f066509 Compare February 26, 2018 17:46

raymondfeng force-pushed the use-context-for-controller-instance branch 3 times, most recently from d580f20 to 6cfea88 Compare March 9, 2018 19:19

raymondfeng force-pushed the use-context-for-controller-instance branch 2 times, most recently from 6916fcf to bff4a47 Compare March 16, 2018 16:17

bajtos reviewed Mar 19, 2018

View reviewed changes

raymondfeng force-pushed the use-context-for-controller-instance branch 2 times, most recently from 35cd971 to 2adcaff Compare March 20, 2018 17:00

raymondfeng force-pushed the use-context-for-controller-instance branch from 2adcaff to fb62f4b Compare March 27, 2018 04:05

bajtos reviewed Mar 27, 2018

View reviewed changes

raymondfeng force-pushed the use-context-for-controller-instance branch from fb62f4b to 3d4bf2c Compare March 29, 2018 22:23

raymondfeng force-pushed the use-context-for-controller-instance branch from 3d4bf2c to 23db7f9 Compare March 30, 2018 22:27

raymondfeng force-pushed the use-context-for-controller-instance branch from 23db7f9 to 5782ab8 Compare April 9, 2018 15:18

raymondfeng force-pushed the use-context-for-controller-instance branch 3 times, most recently from 61eb6b3 to a7d716c Compare April 10, 2018 16:04

bajtos approved these changes Apr 11, 2018

View reviewed changes

raymondfeng added 3 commits April 11, 2018 08:59

feat(rest): allow factory for controller routes

309a1cf

feat(rest): add typing for controller instance/class/factory

7de8e00

refactor(rest): tidy up controller related methods

d48c062

- move controllFactory param next to controllerCtor - split createControllerFactory to three functions

raymondfeng force-pushed the use-context-for-controller-instance branch from a7d716c to d48c062 Compare April 11, 2018 16:01

raymondfeng merged commit 5446cea into master Apr 11, 2018

raymondfeng removed the needs-priority label Apr 11, 2018

raymondfeng deleted the use-context-for-controller-instance branch April 11, 2018 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rest): use context.get() to resolve current controller #993

fix(rest): use context.get() to resolve current controller #993

raymondfeng commented Feb 13, 2018 •

edited

Loading

bajtos commented Feb 14, 2018 •

edited

Loading

bajtos left a comment •

edited

Loading

bajtos Feb 14, 2018

raymondfeng Feb 14, 2018

bajtos Mar 1, 2018

raymondfeng commented Feb 14, 2018 •

edited

Loading

bajtos commented Feb 15, 2018

bajtos commented Mar 1, 2018

raymondfeng commented Mar 2, 2018

bajtos commented Mar 5, 2018

bajtos commented Mar 5, 2018

raymondfeng commented Mar 7, 2018

raymondfeng commented Mar 9, 2018

bajtos Mar 19, 2018

bajtos Mar 19, 2018

raymondfeng Mar 19, 2018

bajtos Mar 22, 2018

raymondfeng Mar 22, 2018

raymondfeng Mar 22, 2018

bajtos Mar 19, 2018

raymondfeng commented Mar 23, 2018

bajtos Mar 27, 2018

raymondfeng Mar 29, 2018

bajtos Mar 27, 2018

raymondfeng Mar 29, 2018

raymondfeng commented Mar 29, 2018

bajtos left a comment

bajtos Apr 11, 2018

fix(rest): use context.get() to resolve current controller #993

fix(rest): use context.get() to resolve current controller #993

Conversation

raymondfeng commented Feb 13, 2018 • edited Loading

Checklist

bajtos commented Feb 14, 2018 • edited Loading

bajtos left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Feb 14, 2018 • edited Loading

bajtos commented Feb 15, 2018

bajtos commented Mar 1, 2018

raymondfeng commented Mar 2, 2018

bajtos commented Mar 5, 2018

bajtos commented Mar 5, 2018

raymondfeng commented Mar 7, 2018

raymondfeng commented Mar 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Mar 29, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Feb 13, 2018 •

edited

Loading

bajtos commented Feb 14, 2018 •

edited

Loading

bajtos left a comment •

edited

Loading

raymondfeng commented Feb 14, 2018 •

edited

Loading