feat(rest): introduce an interceptor-chain based sequence of actions for REST

[raymondfeng]

The PR adds another way to build a sequence of actions for REST request/response processing. It works in a similar way as global interceptors with RequestContext and next.

The PR manages to allow backward compatibility. Two flavors can both be used.

It depends on #2908.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

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

👉 Check out how to submit a PR 👈

[bajtos]

@raymondfeng how can I review the proposed changes at high level?

[bajtos]

When we decided to build LB4 from scratch, one of our primary design goals was to avoid the concept of a handler chain, because we have had a lot of bad experience with composition of Express middleware in LB1-LB3. The concept of a sequence of actions was meant to supersede the concept of a handler chain and provide better UX.

I feel that re-introduction of the concept of a handler chain requires a deeper discussion and careful consideration of the wider context.

I don't have time to review the proposed changes now, will try to find time in the next few weeks 🤞

[bajtos]

@raymondfeng this pull request is quite large (+1,517 −613 lines). Please add a high-level description of the proposed user experience, so that we can keep the initial rounds of review at design level and don't deal with implementation details yet.

[raymondfeng]

@bajtos See https://github.com/strongloop/loopback-next/blob/handler-chain-sequence/docs/site/Sequence-with-interceptors.md

[bajtos]

I don't understand what problems are you trying to solve by the new design. My impression is that you are adding a lot of additional complexity, both in our implementation and for LB4 users, and I am not convinced that the benefits outweigh the downsides.

Let's discuss.

[bajtos]

Please explain how to define the order in which individual REST actions are invoked. Is this order defined by the order in which the actions are bound? That does not seem very practical to me - it's one of the pain points of Express/Koa middleware that we want to avoid in LB4.

[bajtos]

What are the implications for existing code inheriting from RejectProvider? This looks like a breaking change to me.

[bajtos]

How can LB4 applications customize the order of the actions?

Let's say I am an app developer and want to call a new action in my sequence, e.g. authenticate. What are the instructions?

[bajtos]

What is the implication of this change on existing code that's using RestBindings.SequenceActions.FIND_ROUTE?

(The same comment applies to other binding keys that you are renaming in this PR.)

[bajtos]

IIUC, when the middleware handler function accepts next callback but does not call it, then the interceptor does not call next either. I find that problematic, because it breaks the assumptions that interceptors can rely on await next() mechanism to run code before and after request processing.

Example middleware:

function static(req, res, next) {
  const asset = path.join(assetsRoot, req.path);
  if (fs.existsSync()) {
    res.sendFile(assert)
  } else {
    next();
  }
}
// or also
const static = new MiddlewareAction(require('serve-static'), assetsRoot);

Example interceptor (a sequence action):

async function logRequest(ctx, next) {
  const start = process.hrtime();
  await next();
  const delta = process.hrtime(start);
  console.log(
    '%s %s %s %s ms',
    ctx.req.method, 
    ctx.req.path, 
    ctx.res.statusCode, 
    // time in milliseconds
    Math.round(delta[0]*1e3 + delta[1]/1e6) 
  );
}

In this example, no log messages are printed for requests served as static assets.

[bajtos]

This is problematic. Consider the following example.

// middleware
function myRoute(req, res) {
  doSomeAsyncWork((err, result) => {
    console.log('async work is done');
    res.json(result);
  });
}

// interceptor/sequence action
async function logRequest(ctx, next) {
  console.log(
    'START %s %s',
    ctx.req.method, 
    ctx.req.path, 
  );

  await next();

  console.log(
    'DONE %s %s %s',
    ctx.req.method, 
    ctx.req.path, 
    ctx.res.statusCode, 
  );
}

This will produce the following console logs:

START GET /api/users
DONE GET /api/users
async work is done

Expected output:

START GET /api/users
async work is done
DONE GET /api/users

IMO, when the middleware does not call next, you should use on-finished module to wait for the response to be written before you call next.

See the existing implementation here:

https://github.com/strongloop/loopback-next/blob/0b5a47227ac0ab71bf67cd8836dfe3ecbbe9a17d/packages/rest/src/router/external-express-routes.ts#L160-L177

[bajtos]

The application code needs to be fully aware of all elements for a sequence and how they are composed using the waterfall style.

IMO, this problem is present in your proposal too. The application still needs to be aware of all elements to ensure they are invoked in the correct order.

The application code needs to understand the signature and responsibility of each function representing an action.

This is intentional! In the current design, it's explicitly clear what is the sequence action expecting to receive from the previous sequence steps and what it contributes back.

When authenticate action requires the resolved route to be available, then we can verify at the compile time that it is called only after findRoute has been called.

In your design, we have a single god-like object context and the compiler cannot verify that CURRENT_ROUTE was bound to the context before authenticate action was invoked.

It's cumbersome to add new actions, especially for existing applications as code change is required.

I am afraid your proposal does not show how it allows actions to be added to existing applications with no code change. Please show us!

Isn't such behavior dangerous though? Let's say if components have a mechanism allowing them to contribute new actions to the sequence automatically, then app developers can be caught by a surprise that a new version of a component they are using in their project is suddenly adding a new action to the sequence, especially if the application does not want to execute such sequence!

If actions can be added to the sequence with no code change, then please update your proposal to show how application can prevent such action to be added automatically.

It's not easy to monitor or trap actions within a sequence.

Please explain this in more details. The sequence action is a regular JavaScript/TypeScript function, users are free to use any monitoring/trapping solution as they like.

It's not easy to reuse Express middleware as actions.

This is very simple to address in the current framework. We just need a helper to invoke a middleware as a sequence action.

We can leverage the existing implementation: https://github.com/strongloop/loopback-next/blob/0b5a47227ac0ab71bf67cd8836dfe3ecbbe9a17d/packages/rest/src/router/external-express-routes.ts#L160-L177

Usage (assuming we extract executeRequestHandler into a standalone function):

const corsHandler = cors(/*cors options*/):

export class MySequence implements SequenceHandler {
  async handle(context: RequestContext) {
    try {
      const {request, response} = context
      if (await executeRequestHandler(corsHandler, request, response))) return;
      const route = this.findRoute(request);
      // etc.
    } catch (error) {
      this.reject(context, error);
    }
  }
}

[raymondfeng]

@bajtos Thank you for the feedback. Let me clarify a bit:

1. Both approaches have pros and cons. This PR adds a different style and keep the possibility to use one of them.

2. The key difference is functional composition with parameter passing vs. command composition with context sharing. The first style grants full control and full responsibility for application developers while the second one allows better extensibility and divide of responsibility between application and framework/extension developers.

3. Prior art for the second style is Koa middleware. It's also consistent with our interceptor design.

I'll go into more details later.

[raymondfeng]

Closing it to favor #5366