Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(context): add support for method interceptors #2687

Open
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@raymondfeng
Copy link
Member

raymondfeng commented Apr 3, 2019

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
Copy link
Contributor

jannyHou left a comment

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

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Apr 3, 2019

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().

@raymondfeng raymondfeng force-pushed the interceptor branch from 0486447 to 6454db9 Apr 3, 2019

@b-admike
Copy link
Member

b-admike left a comment

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?

Show resolved Hide resolved docs/site/Interceptors.md Outdated
Show resolved Hide resolved docs/site/Interceptors.md Outdated

@raymondfeng raymondfeng force-pushed the interceptor branch from 6454db9 to 702261e Apr 3, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Apr 3, 2019

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

This comment has been minimized.

Copy link
Member

bajtos commented Apr 4, 2019

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 raymondfeng force-pushed the interceptor branch 5 times, most recently from 21b8576 to b9ced8d Apr 4, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Apr 5, 2019

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

@raymondfeng raymondfeng force-pushed the interceptor branch 2 times, most recently from 9c74ff2 to 3859148 Apr 5, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Apr 7, 2019

Implements #133

@bajtos
Copy link
Member

bajtos left a comment

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.

Show resolved Hide resolved docs/site/Interceptors.md Outdated
@intercept(logWithErrorHandling)
async helloWithError(name: string) {
throw new Error('error: ' + name);
}

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 8, 2019

Member

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?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Apr 8, 2019

Author Member

I did some refactoring and add more comments.

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 16, 2019

Member

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.

Show resolved Hide resolved docs/site/Interceptors.md
Show resolved Hide resolved docs/site/Interceptors.md Outdated
Show resolved Hide resolved docs/site/Interceptors.md Outdated
Show resolved Hide resolved docs/site/Interceptors.md Outdated
Show resolved Hide resolved docs/site/Interceptors.md Outdated
Show resolved Hide resolved docs/site/Interceptors.md Outdated
@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Apr 8, 2019

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 raymondfeng force-pushed the interceptor branch from 03fd4b8 to c217472 Apr 8, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Apr 8, 2019

(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.

@raymondfeng raymondfeng force-pushed the interceptor branch 2 times, most recently from 33973fa to 8c9e8dc Apr 8, 2019

@raymondfeng raymondfeng requested a review from bajtos Apr 8, 2019

@raymondfeng raymondfeng force-pushed the interceptor branch from 8c9e8dc to 89f0e2d Apr 9, 2019

@raymondfeng raymondfeng force-pushed the interceptor branch from 9e6a53c to f9257dd Apr 12, 2019

@raymondfeng raymondfeng referenced this pull request Apr 12, 2019

Merged

docs: add guideline doc for authorize component #2721

2 of 7 tasks complete

@raymondfeng raymondfeng force-pushed the interceptor branch 2 times, most recently from 248fb1e to 1a9cb11 Apr 12, 2019

@bajtos
Copy link
Member

bajtos left a comment

As I was anticipating, the addition of Proxy support is significantly increasing the complexity of this pull request, number of comments to address and also problematic design issues to discuss 馃槧

Show resolved Hide resolved packages/context/src/__tests__/acceptance/interceptor.acceptance.ts
Show resolved Hide resolved packages/context/src/interceptor.ts Outdated
Show resolved Hide resolved packages/context/src/interceptor.ts Outdated
Show resolved Hide resolved packages/context/src/interceptor.ts Outdated
Show resolved Hide resolved packages/context/src/interceptor.ts Outdated
Show resolved Hide resolved docs/site/Interceptors.md Outdated
Show resolved Hide resolved docs/site/Interceptors.md Outdated
Show resolved Hide resolved docs/site/Interceptors.md
Show resolved Hide resolved docs/site/Interceptors.md Outdated
can be created from the target class or object to apply interceptors. This is
useful for the case that a controller declares dependencies of repositories or
services and would like to allow repository or service methods to be
intercepted.

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 16, 2019

Member

While I see how compelling this idea is, I find it also problematic. Consider the situation when a sync function is decorated with an async interceptor. The proxy is changing the API of the decorated class, turning synchronous methods into asynchronous.

This comment has been minimized.

Copy link
@raymondfeng
@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Apr 16, 2019

@raymondfeng I posted 40+ review comments above, GitHub is collapsing them. Please make sure to expand the collapsed placeholder to see all comments.

@raymondfeng raymondfeng force-pushed the interceptor branch 2 times, most recently from a419b4f to 2ce1402 Apr 16, 2019

@raymondfeng raymondfeng requested a review from bajtos Apr 17, 2019

@raymondfeng raymondfeng force-pushed the interceptor branch 6 times, most recently from 8ee772e to 15f0cad Apr 17, 2019

@raymondfeng raymondfeng force-pushed the interceptor branch from 15f0cad to cb4a270 Apr 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.