feat(di): add support chain interceptor on the same method #2915
Conversation
WalkthroughThe changes introduce a new interceptor class, Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
packages/di/src/common/decorators/intercept.spec.ts(4 hunks)packages/di/src/common/decorators/intercept.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/di/src/common/decorators/intercept.spec.ts
[error] 71-71: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
packages/di/src/common/decorators/intercept.ts
[error] 57-57: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 60-60: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🔇 Additional comments (3)
packages/di/src/common/decorators/intercept.ts (1)
Line range hint 22-53: LGTM: Robust implementation of the interceptor chain
The implementation correctly:
- Preserves the original method context via
thisbinding - Constructs proper interceptor context with all required properties
- Handles error propagation through the
nextcallback - Maintains invoke options for dependency injection
packages/di/src/common/decorators/intercept.spec.ts (2)
20-27: LGTM: Well-structured second interceptor implementation
The implementation of MyInterceptor2 follows the same pattern as MyInterceptor, ensuring consistency.
69-79: Verify the order of interceptor execution
The test verifies that both interceptors are executed, but let's ensure the order is consistent with the decorator order.
✅ Verification successful
The interceptor execution order is correct
Looking at the implementation and test, the execution order is working as expected. When multiple @Intercept decorators are applied, they are executed in reverse order (bottom-up), which is the standard decorator behavior in TypeScript/JavaScript. This is verified by the test's expected output: "Chained - param data - options data - intercepted 2 - options data - intercepted 1", showing MyInterceptor2 (second) executes before MyInterceptor (first).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of multiple @Intercept decorators to verify consistent ordering
ast-grep --pattern $'@Intercept($_)
@Intercept($_)
$_'
Length of output: 55
Script:
#!/bin/bash
# Let's try a different approach to find the interceptor declarations and their usage
rg "@Intercept" -B 2 -A 2
Length of output: 7723
Script:
#!/bin/bash
# Let's examine the implementation of the intercept decorator to understand the execution order
cat packages/di/src/common/decorators/intercept.ts
Length of output: 2643
🧰 Tools
🪛 Biome (1.9.4)
[error] 71-71: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
Romakita
force-pushed
the
feat-v8-chained-interceptor
branch
from
1f8db0b
to
f8833ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
packages/di/src/common/decorators/intercept.spec.ts(4 hunks)packages/di/src/common/decorators/intercept.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/di/src/common/decorators/intercept.spec.ts
[error] 77-77: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 89-89: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
packages/di/src/common/decorators/intercept.ts
[error] 55-55: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 58-58: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🔇 Additional comments (1)
packages/di/src/common/decorators/intercept.ts (1)
Line range hint 22-53: Verify error propagation in interceptor chain
The error handling in the interceptor chain needs careful consideration. Currently, if an error occurs in one interceptor, it immediately breaks the chain and throws the error. Consider whether this is the intended behavior or if we should:
- Allow subsequent interceptors to handle/transform the error
- Collect errors from all interceptors before propagating
✅ Verification successful
Based on the search results, I can now provide a final response regarding the error propagation in the interceptor chain:
Error handling behavior is correctly implemented and follows established patterns
The current error handling implementation in the interceptor chain is the intended behavior, as evidenced by:
- The test cases in
intercept.spec.tsexplicitly verify this behavior with test cases like "should intercept the method and throw error" - The implementation follows a fail-fast pattern where errors are immediately propagated up the chain through the
next(err)callback - The test assertions confirm that errors are properly propagated with their original messages intact
This approach aligns with common interceptor patterns where error conditions should immediately halt the chain to prevent potentially unsafe operations from continuing. If an interceptor encounters an error condition, it's typically desirable to stop processing and handle the error at a higher level rather than continue with potentially invalid state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in interceptors
rg -A 5 "catch|throw|Error" packages/di/src/common/decorators/
Length of output: 15507
| descriptor!.value = newMethod; | ||
|
|
||
| Reflect.deleteProperty(klass.prototype, propertyKey); | ||
| Reflect.defineProperty(klass.prototype, propertyKey, descriptor!); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider preserving method properties during replacement
The current implementation might lose method metadata during property replacement. Consider using Object.getOwnPropertyDescriptor to preserve all property attributes.
- Reflect.deleteProperty(klass.prototype, propertyKey);
- Reflect.defineProperty(klass.prototype, propertyKey, descriptor!);
+ const originalDescriptor = Object.getOwnPropertyDescriptor(klass.prototype, propertyKey);
+ Reflect.defineProperty(klass.prototype, propertyKey, {
+ ...originalDescriptor,
+ value: newMethod
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| descriptor!.value = newMethod; | |
| Reflect.deleteProperty(klass.prototype, propertyKey); | |
| Reflect.defineProperty(klass.prototype, propertyKey, descriptor!); | |
| descriptor!.value = newMethod; | |
| const originalDescriptor = Object.getOwnPropertyDescriptor(klass.prototype, propertyKey); | |
| Reflect.defineProperty(klass.prototype, propertyKey, { | |
| ...originalDescriptor, | |
| value: newMethod | |
| }); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 55-55: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 58-58: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
| @Interceptor() | ||
| class MyInterceptor2 implements InterceptorMethods { | ||
| intercept(context: InterceptorContext<any>) { | ||
| const r = typeof context.args[0] === "string" ? undefined : new Error(`Error message`); | ||
| const retValue = context.next(r); | ||
|
|
||
| return `${retValue} - ${context.options || ""} - intercepted 2`; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding validation in MyInterceptor2
MyInterceptor2 duplicates error checking logic from MyInterceptor. Consider extracting common validation into a base class or utility function.
// Example base class or utility
class BaseInterceptor implements InterceptorMethods {
protected validateInput(args: any[]): Error | undefined {
return typeof args[0] === "string" ? undefined : new Error("Error message");
}
}
@Interceptor()
class MyInterceptor2 extends BaseInterceptor {
intercept(context: InterceptorContext<any>) {
const error = this.validateInput(context.args);
const retValue = context.next(error);
return `${retValue} - ${context.options || ""} - intercepted 2`;
}
}| it("should chained interceptor", async () => { | ||
| // GIVEN | ||
| const serviceTest = await DITest.invoke<ServiceTest>(ServiceTest)!; | ||
|
|
||
| // WHEN | ||
| const result = serviceTest.chained("param data"); | ||
|
|
||
| expect(getInterceptorOptions(ServiceTest, "chained")).toEqual("options data"); | ||
|
|
||
| // THEN | ||
| expect(result).toEqual("Chained - param data - options data - intercepted 2 - options data - intercepted 1"); | ||
| }); | ||
| it("should chained interceptor (reversed)", async () => { | ||
| // GIVEN | ||
| const serviceTest = await DITest.invoke<ServiceTest>(ServiceTest)!; | ||
|
|
||
| // WHEN | ||
| const result = serviceTest.reverseChained("param data"); | ||
|
|
||
| expect(getInterceptorOptions(ServiceTest, "reverseChained")).toEqual("options data"); | ||
|
|
||
| // THEN | ||
| expect(result).toEqual("Chained - param data - options data - intercepted 1 - options data - intercepted 2"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for interceptor chaining
While the basic chaining tests are good, consider adding:
- Tests with more than two interceptors
- Tests for error scenarios in the middle of the chain
- Tests with different options for each interceptor
Example additional test:
it("should handle errors in middle of chain", async () => {
// GIVEN
const serviceTest = await DITest.invoke<ServiceTest>(ServiceTest, [
{
token: MyInterceptor2,
use: {
intercept: vi.fn().mockImplementation(() => {
throw new Error("Chain error");
})
}
}
]);
// WHEN
const error = catchError(() => serviceTest.chained("param data"));
// THEN
expect(error?.message).toEqual("Chain error");
});🧰 Tools
🪛 Biome (1.9.4)
[error] 77-77: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 89-89: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
|
🎉 This PR is included in version 8.1.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Closes: #2914
Information
Add chained interceptor support
Todos
Summary by CodeRabbit
New Features
MyInterceptor2class for enhanced interception capabilities.chainedmethod inServiceTestto apply multiple interceptors in sequence.reverseChainedmethod inServiceTestto apply interceptors in reverse order.Bug Fixes
Refactor
bindInterceptfunction for better error handling and token management.