feat(context): add support for method interceptors

[raymondfeng]

See https://github.com/strongloop/loopback-next/blob/interceptor/docs/site/Interceptors.md

Implements #133

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

[jannyHou]

How would the interceptor affect the sequence? will they be executed when invoking the controller function?

[raymondfeng]

How would the interceptor affect the sequence? will they be executed when invoking the controller function?

They will be invoked by the InvokeMethod action. The hook is on the invokeMethod().

[b-admike]

I like the idea of interceptors 👍, and I can see how they can be used, but was wondering how we want to use them with LB4. Are they supposed to represent before/after hooks in LB3 for CRUD methods? Are there some use cases we can show users in the context of LB4?

[raymondfeng]

I like the idea of interceptors 👍, and I can see how they can be used, but was wondering how we want to use them with LB4. Are they supposed to represent before/after hooks in LB3 for CRUD methods? Are there some use cases we can show users in the context of LB4?

The interceptors can be used as around middleware at per method level. Typical usages:

1. Implement caching
2. Add logging/monitoring/..
3. Implement authorization
4. Enforce parameter validation
5. Transform input/output

They are close to remote hooks.

It's also possible for method decorators to return a proxy method. But that won't allow context access for such interceptor functions.

[bajtos]

I am not sure if I'll manage to review this pull request by the end of this week, please give me more time to catch up. I feel this is pull request is adding a fundamental building block that will be difficult to change later, therefore a careful review is needed.

[raymondfeng]

FYI: I also added an acceptance test for @loopback/rest to illustrate a mock-up caching interceptor.

[raymondfeng]

Implements #133

[bajtos]

I reviewed the documentation and I like the simplicity of the proposed design 💯

(1)
I have one major concern though: you are proposing to apply interceptors on controller methods only. However, LB4 supports other flavors for endpoint implementations, most notable route handler functions:

app.route('get', '/hello', {/*spec*/}, () => 'hello world');

How do you envision applying interceptors on these routes, especially the interceptors registered at global (application) level?I would find it very confusing if global interceptors were applied on a subset of routes only.

(2)
What is our recommendation for extension (component) developers? How should they structure their npm package to contribute interceptors? How are application developers going to consume such interceptors? I'd like to see a new section in Extending LoopBack 4 providing content for extension developers. I think we can also move many advanced details from your original content in Key Concepts to that new page too.

[bajtos]

I find this example too long. There are about 10 methods in an unstructured list, many of them are slight variations on the same theme. Could you please pick 3-5 most important cases and convert them into the form of a combination of short explanatory text with a code snippet?

[raymondfeng]

I did some refactoring and add more comments.

[bajtos]

I am afraid the changes are not enough. We are writing documentation here, the content should read as well-structured text. It's ok to use code snippets for illustrations, but these snippets should not serve as a replacement for the actual human-readable text.

[bajtos]

coverage/coveralls — Coverage decreased (-0.03%) to 91.018%

@raymondfeng please review the code coverage report, there may be valid use cases/scenarios that are not covered by your tests.

[raymondfeng]

(1)
I have one major concern though: you are proposing to apply interceptors on controller methods only. However, LB4 supports other flavors for endpoint implementations, most notable route handler functions:

It's now supported.

[bajtos]

@raymondfeng before I review the changes in details, I'd like to reach a shared high-level understanding of how we want to deal with asynchronicity.

At high level:

1. The target function (intercepted controller method) can be sync or async.
2. The interceptor can be sync or async.

This gives us four combinations:

1. sync target, sync interceptor
2. sync target, async interceptor
3. async target, sync interceptor (caveat: even if the interceptor is sync in principle, the outcome is async because next returns a promise)
4. async target, async interceptor

In theory, the first combination (sync target, sync interceptor) can preserve sync behavior of the invoked operation. The remaining three combinations will always cause the operation to be async. (Assuming interceptors wait for the outcome of the intercepted operations.)

As I understand your current proposal, you want to support sync behavior, which is reflected in the Interceptor API:

• The interceptor returns ValueOrPromise (i.e. can return synchronously, but does not have to!)
• The next callback returns ValueOrPromise (i.e. can return synchronously, but does not have to!)

IMO, this is complicating implementation a lot.

1. The code calling an interceptor (or a chain of interceptors for a method) cannot tell in advance whether the outcome will be sync or async and thus must be prepared to handle both cases.
2. Individual interceptor implementations cannot make any assumption on whether next is going to return the result (sync) or a promise (async), they must be prepared to handle both cases.

The first point is of lesser importance because interceptors are usually invoked by the framework, thus there are only few places where we need to be careful.

I am mostly concerned about the second point, about interceptors that will be written by regular LB users. IMO, most of these developers won't understand delicate details of sync vs. async flavors of interceptors and target methods. In the better case, they will write all interceptors as async functions and thus prevent the framework from leveraging benefits of sync interception. Worse, they will try to write their interceptor to support both sync & async workflows and possibly end up with complex code that does not work as expected.

In that light, I'd like us to reconsider whether the combination of sync target + sync interceptors is so important to justify burden of extra complexity placed on framework users.

Personally, I prefer the simplicity of fully-async approach:

export interface Interceptor {
  (
    context: InvocationContext,
    next: () => Promise<InvocationResult>,
  ): Promise <InvocationResult>;
}

@strongloop/loopback-maintainers thoughts?

[raymondfeng]

Personally, I prefer the simplicity of fully-async approach

I'm not against the idea. Here is the caveat for api compatibility:

1. Currently invokeMethod allows both sync/async
2. The PR improved invokeMethod to always apply interceptors if present. As a result, invokeMethod does not allow sync any more.

[bajtos]

Personally, I prefer the simplicity of fully-async approach

export interface Interceptor {
 (
   context: InvocationContext,
    next: () => Promise<InvocationResult>,
 ): Promise <InvocationResult>;
}

I'm not against the idea. Here is the caveat for api compatibility:

Currently invokeMethod allows both sync/async
The PR improved invokeMethod to always apply interceptors if present. As a result, invokeMethod does not allow sync any more.

I was thinking about this problem during the weekend.

My proposal has an important downside: it's not forward-compatible. If we ever decide to add support for sync methods & sync interceptors in the future, then such change could break existing code. For example:

function myInterceptor(context, next) {
  // a dummy logger using Promise API instead of async/await
  return next.then(result => {
    console.log('result', result);
    return result;
  });
}

const proxy = // create proxy with interceptors
proxy.someMethod().then(result => {
  // a dummy example invoking an intercepted method
  // while using Promise API instead of async/await
});

In that light, I think we should preserve the current implementation that supports both sync and async flavors. BUT! I still would like us to make interceptors easier to understand for new LB developers, I think this can be easily achieved by reworking our documentation to start with simplified information showing fully-async flavor only, and then explaining sync versions later in "advanced" section.

[bajtos]

Almost there. My main concern is about the sync/async aspect of the Proxy API and the documentation. There are also few older comments that have not been addressed yet.

[bajtos]

We should let the users know when this flag was ignored because the binding was not backed by a class.

1. You can print a debug statement, this way we don't pollute console with excessive output.
2. Alternatively, I am also fine with throwing an error to let the programmer know about the error they made in their code.

I slightly prefer the second option (throw an error).

☝️

[bajtos]

I want to start the app once

What is the reasoning behind the requirement? Can we find a different solution how to move the definition of controllers into individual tests/context blocks?

☝️

[raymondfeng]

I slightly prefer the second option (throw an error).

Fixed.

[raymondfeng]

What is the reasoning behind the requirement? Can we find a different solution how to move the definition of controllers into individual tests/context blocks?

Fixed.

[raymondfeng]

FYI: I extracted binding sorting functions to #2848.

[bajtos]

Looks mostly good, please consider addressing my comments below.

No further review is necessary as far as I am concerned, but please check with @hacksparrow @jannyHou and @b-admike before landing, they have participated in the review discussion but have not approved the changes yet.

[raymondfeng]

@strongloop/loopback-maintainers I have squashed all commits into one. PTAL before I merge it.

[b-admike]

Nice to see addition of global interceptors, changes LGTM. I like the fact that the README has a lot of information around the features.