Router shouldn't handle requests...

[ritch]

@bajtos @superkhau

So our new router is loosely inspired by Express' Router - especially router.handle.

The one thing that never made sense to me is - why is the Router in Express both locating the function to execute and executing it? That really puts the request into a black hole... Instead I think we should do something slightly different (in the spirit of extensibility)...

// loopback core
app.bind(OPERATION, () => {
  const ctx = this;
  return findOperationForContext(ctx);
});

app.bind(RESULT, () => {  
  const ctx = this;
  const args = ctx.get(OPERATION_ARGS);
  const operation = ctx.get(OPERATION);
  const controller = ctx.get(CONTROLLER);
  return operation.apply(controller, args));
});

class Application {
  public async handle(ctx) {
    const result = await ctx.get(RESULT);
    const writer = ctx.get(RESPONSE_WRITER);
    writer(result); // basically res.write(JSON.stringify(result)) 
  }
}

The benefits of this approach is that we build on top of a consistent overridable dependency resolution.

• You can always rebind what we've bound for you by default (eg. rebind RESULT, OPERATION, anything...) for mocking or extensibility
• We can add performance, debugging, and other enhancements that will also be picked up by the router if it also uses this same approach

Also consider how this could make it easier to implement extensions eg. Authorization...

app.bind(Bindings.AUTHORIZATION).to(() => {
  const ctx = this;
  const role = await ctx.get('currentUser.role');
  const operation = ctx.get(OPERATION); // <====
  const acl = ctx.get('${operation.name}.acl'); // <====

  for (ctrl of acl) {
    if (ctrl.denies(role) || ctrl.denies(ctx)) return ctrl.denial(ctx);
  }

  return new Authorization(Authorization.DENY, 'you do not have permission to...');
});

[raymondfeng]

@ritch IIUC, you are proposing to register all the points of control to ctx and use them to resolve and compose the aggregate request handling.

For an incoming request, we need to:

1. Resolve the http payload to a controller/method (operation) as the final handler
2. Figure out other handlers to be invoked (pipeline)
3. Build up more context, such as user/app/role
4. Resolve to a strategy to instantiate the controller (controller) with possible dependency injection
5. Map contextual information (including path, query, header, body, ...) to operation parameters
6. Invoke the controller/method
7. Post process the response through the pipeline
8. Map the response and other context information into http response
9. Send the http response

For each phase, we'll bind corresponding handlers to the ctx for maximum extensibility and flexibility.

[bajtos]

The one thing that never made sense to me is - why is the Router in Express both locating the function to execute and executing it? That really puts the request into a black hole...

This makes sense to me, at least from the theoretical point of view.

The code example does not look right to me, it's mixing dependency-resolution/service-locator pattern with the actual framework logic - I consider that as an anti-pattern too. Let's set that aside for now and treat the code just as a mock-up to illustrate your point.

One of my wishes for the new router is to make it so great that people will start using it outside of LoopBack. To make that happen, I'd like to keep router's API as much agnostic of lb-next IoC container (ctx.get()) as possible.

Now in order to satisfy your requirement of injecting custom logic before/after the controller method is invoked, and keeping the router simple to use, I think we can add an extension point that will allow router's users to specify how to invoke the controller method once it's found.

// the default implementation - easy to use
SwaggerRouter.invoke = function(Controller, method, args) {
  return method.apply(Controller, args);
}

// in LoopBack
class LoopBackSwaggerRouter extends SwaggerRouter {
  invoke: function(Controller, method, args) {
    // enhance per-invocation context
    ctx.bind(OPERATION_ARGS, args);
    
    // authorize, etc.

    const result = await super.invoke(Controller, method, args);

     // perform any post-processing
  }
}

[ritch]

We had an off-github discussion about this. I'll try to summarize the points we agreed on:

ctx.bind(OP_ARGS).to(() => {
  return parseArgsForOperation(ctx.get('req'), ctx.get(OP_NAME), ctx.get('swagger'));
});

ctx.bind(OP_NAME).to(() => {
  return findOperationForReq(ctx.get('req'), ctx.get('swagger'));
});

Principals

1. separate the IOC complexity from the simple parser, router behavior
2. everything the router / parser outputs should be bind-able
3. ctx complexity should not prevent simple debugging / code navigation

Next Steps

1. define an example that requires the extensibility mentioned earlier in this issue
2. decompose the router so the basic code above is possible
3. use the decomposed router to implement the 1) example

[ritch]

Here is a proposed high level version of the actual handle impl...

// loopback core
app.bind(OPERATION).to(() => {
  const ctx = this;
  return findOperationForContext(ctx);
});

app.bind(RESULT).to(
 @inject(OPERATION_ARGS) args,
 @inject(OPERATION) operation,
 @inject(CONTROLLER) controller) => {  
  return operation.apply(controller, args));
});

class Application {
  public async handle(ctx) {
    const result = await ctx.get(RESULT);
    const writer = ctx.get(RESPONSE_WRITER);
    writer(result); // basically res.write(JSON.stringify(result)) 
  }
}

[ritch]

@raymondfeng IIUC you are for this approach. Also your list LGTM.

[superkhau]

This is already happening via Sequences. Closing.