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): make it possible to set source information for interceptions #4168

Merged
merged 2 commits into from Dec 3, 2019

Conversation

@raymondfeng
Copy link
Member

raymondfeng commented Nov 20, 2019

Interceptors can be invoked in the following cases:

  1. A route to a controller method
  2. A controller to a repository/service with injected proxy
  3. A repository/service method invoked explicitly with invokeMethod/invokeMethodWithInterceptors

Some interceptors want to check invocationContext.source to decide if its logic should be applied. For example, an http access logger only cares about invocations from the rest layer to the first controller.

This is also useful for metrics, tracing and logging.

This PR sets source as follows:

  • the ResolutionSession in ProxySource for injected proxies.
  • the Rest Route in RouteSource for REST

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 馃憟

@raymondfeng raymondfeng requested a review from bajtos as a code owner Nov 20, 2019
@raymondfeng raymondfeng force-pushed the allow-source-information-for-interception branch from 12157c2 to 7c26a75 Nov 20, 2019
Copy link
Member

bajtos left a comment

Interesting. I don't fully understand what you are trying to accomplish, can you please add some high-level documentation for consumers of this feature to docs/site?

I am also concerned about ease of use, please see my comments below.

packages/context/src/invocation.ts Outdated Show resolved Hide resolved
packages/context/src/invocation.ts Outdated Show resolved Hide resolved
@raymondfeng raymondfeng force-pushed the allow-source-information-for-interception branch 2 times, most recently from 909416c to e810411 Nov 21, 2019
@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Nov 21, 2019

@raymondfeng raymondfeng force-pushed the allow-source-information-for-interception branch 3 times, most recently from d5cdca7 to 4b42ff0 Nov 21, 2019
@raymondfeng raymondfeng requested a review from bajtos Nov 21, 2019
@raymondfeng raymondfeng force-pushed the allow-source-information-for-interception branch from 4b42ff0 to 58ff2eb Nov 21, 2019
packages/context/src/interceptor.ts Outdated Show resolved Hide resolved
docs/site/Interceptors.md Outdated Show resolved Hide resolved
@raymondfeng raymondfeng force-pushed the allow-source-information-for-interception branch from 58ff2eb to 2598f76 Nov 28, 2019
@raymondfeng raymondfeng requested a review from bajtos Nov 29, 2019
@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Dec 2, 2019

@bajtos PTAL

@raymondfeng raymondfeng force-pushed the allow-source-information-for-interception branch from 2598f76 to c8fbff1 Dec 2, 2019
Copy link
Member

bajtos left a comment

Almost there!

docs/site/Interceptors.md Outdated Show resolved Hide resolved
docs/site/Interceptors.md Outdated Show resolved Hide resolved
@raymondfeng raymondfeng force-pushed the allow-source-information-for-interception branch from c8fbff1 to c0ffc9b Dec 3, 2019
@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Dec 3, 2019

@bajtos Doc fixed.

@raymondfeng raymondfeng force-pushed the allow-source-information-for-interception branch from c0ffc9b to b1c208a Dec 3, 2019
@bajtos
bajtos approved these changes Dec 3, 2019
raymondfeng added 2 commits Nov 20, 2019
鈥tions

Some interceptors want to check invocationContext.source to decide if its
logic should be applied. For example, an http access logger only cares about
invocations from the rest layer to the first controller.

This is also useful for metrics, tracing and logging.
@raymondfeng raymondfeng force-pushed the allow-source-information-for-interception branch from b1c208a to 334643d Dec 3, 2019
@raymondfeng raymondfeng merged commit 77cbd01 into master Dec 3, 2019
4 checks passed
4 checks passed
Travis CI - Pull Request Build Passed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.01%) to 92.187%
Details
@raymondfeng raymondfeng deleted the allow-source-information-for-interception branch Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.