feat: introduce middleware based sequence for REST

[raymondfeng]

This PR adds a middleware based sequence and wrap existing actions as middleware.

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 👈

[deepakrkris]

@raymondfeng looks like we are on a roll for adding middleware features :-)

[deepakrkris]

why is next not called here ? for this to be an independent middleware function, it should have no knowledge that it is the last in the middleware chain. The invocation part should handle that.

[raymondfeng]

It's the middleware logic's decision not to call next as invokeMethod returns the result.

[deepakrkris]

ok I see; so next() is just a customary function for the middleware, it does not really link with the next middleware function in the chain ?

[deepakrkris]

that is why sequence.handle() is forced to invoke the middleware functions one by one ?

for (const opt of this.options) {
     // Invoke registered Express middleware
     debug(
       'Invoking middleware chain %s with groups %s',
       opt.chain,
       opt.orderedGroups,
     );
     const done = await this.invokeMiddleware(context, opt);
     if (done) break;
   } 

[raymondfeng]

ok I see; so next() is just a customary function for the middleware, it does not really link with the next middleware function in the chain ?

It does. But it's up the middleware logic to decide if the next one should be invoked. The invokeMethod is designed to invoke the controller method directly.

why sequence.handle() is forced to invoke the middleware functions one by one?

This is to allow multiple chains to be used one by one. If you need to cascade them, add the chain as a middleware or flatten the chains.

[bajtos]

A big -1 from me.

The current Sequence implementation is easy to understand (because it's based on await flow control) and extend. The new proposal is introducing a lot of hidden complexity and behavior that looks like auto-magic to me.

I am willing to consider withdrawing my rejection if there is detailed and clear explanation for:

• what problems in the current Sequence design are you trying to solve by this pull request
• why these problems cannot be solved within the current design
• how can application developers modify the sequence in the new middleware style
• what is the migration path for existing applications

If we want to go all-in on middleware, then it may be better move away from Sequence entirely and use pure middleware-based composition of request handling flow.

[raymondfeng]

@bajtos I know you might reject the PR :-).

Yes, I would like to move to a middleware based pipeline by composition. The sequence can stay as the integration point to the application and it can facilitate the transition too. Here are a few thoughts that drive me to come up this PR:

1. To avoid future issues similar as LB4 Cors is always enabled after full upgrade of loopback #5368. The framework needs to have some control over the sequence.

2. I never see a need to override the sequence code so far beyond adding additional actions. The extensibility of the sequence has already partially solved by the introduction of InvokeMiddleware. At bare minimum, we can either get rid of the generated sequence class or should simply have it extend DefaultSequence.

3. The dependency based ordering is inspired by https://hapipal.com/best-practices/handling-plugin-dependencies.

4. I intentionally didn't remove action providers. Instead, I created middleware provider to leverage existing action implementations. This is an effort to keep it backward-compatible.

5. Having a few hard-coded actions beyond other middleware is an inconsistency and it makes it hard to manage all steps in a consistent way.

6. The PR meant to propose another option and it can co-exist with the current sequence style - which can be removed later if our users are happy with the new one.

[raymondfeng]

Here is another reason to unify all actions as middleware - #5562 (comment)

[hacksparrow]

Why is this named orderedGroups? Do sendResponse, cors, apiSpec etc., resolve to arrays of middleware? If not, the name is misleading. If yes, the fact has to be clearly documented.

[hacksparrow]

After going through the codebase, it has become clear to me. A name like executionOrder or something like that might make it more obvious.

[raymondfeng]

We inherit orderedGroups from interceptors - middleware is a specialized interceptors that are invoked as part of the sequence.

[hacksparrow]

parseParams, findRoute and other strings are being repeated, I think we should be using constants.

[hacksparrow]

For this to work, user has to create a Provider<Middleware> and set its group value to authentication; and many such authentication middleware can be added and anyone of them can cause the authentication to fail, right?

[raymondfeng]

A group can have 0-N middleware and its name does not have semantic meaning. It's only for ordering purpose.

[raymondfeng]

@hacksparrow Thank you for the feedback. I believe that I have addressed your comments. PTAL.

[hacksparrow]

What purpose does this serve:

orderedGroups: [
  RestMiddlewareGroups.SEND_RESPONSE,
  RestMiddlewareGroups.CORS,
  RestMiddlewareGroups.API_SPEC,
  RestMiddlewareGroups.MIDDLEWARE,
  RestMiddlewareGroups.FIND_ROUTE,
  RestMiddlewareGroups.AUTHENTICATION,
  RestMiddlewareGroups.PARSE_PARAMS,
  RestMiddlewareGroups.INVOKE_METHOD,
],

if we still have to do this?

group: RestMiddlewareGroups.INVOKE_METHOD,
upstreamGroups: RestMiddlewareGroups.PARSE_PARAMS,

I can understand for middleware not in the ordered group, but for those in the group - shouldn't they "know" it? And what happens if there is a conflict:

group: RestMiddlewareGroups.INVOKE_METHOD,
upstreamGroups: RestMiddlewareGroups.SEND_RESPONSE

?

I think we should add tests for the expected behavior.

[raymondfeng]

@hacksparrow The final order of middleware is settled using both the orderedGroup and relative upstreamGroups/downstreamGroups.

The orderedGroups defines the overall order so that not all middleware are required to spell out upstreamGroups/downstreamGroups. Both settings contribute partial orders. We combine all rules and calculate the final order using topological sort. If there is a conflict (leading to circular dependencies), an error will be thrown.

I'll try to add some tests.

[raymondfeng]

@hacksparrow I added a test (https://github.com/strongloop/loopback-next/blob/sequence-action-as-middleware/packages/express/src/__tests__/acceptance/middleware-registeration.acceptance.ts#L86) and more documents (https://github.com/strongloop/loopback-next/blob/sequence-action-as-middleware/docs/site/REST-middleware-sequence.md#sort-middleware-by-groups).

[hacksparrow]

LGTM

[jannyHou]

Great effort! I followed the docs and reviewed the updated auth modules, don't see obvious problems.