feat: @Middleware support Advice #231
Conversation
WalkthroughThe changes introduce a new structure for managing Aspect-Oriented Programming (AOP) middleware in the Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant Middleware
participant AOP
participant Service
Controller->>Middleware: Handle request
Middleware->>AOP: Check AOP advice
AOP-->>Middleware: Apply AOP advice
Middleware-->>Controller: Return response
Controller->>Service: Call service method
Service-->>Controller: Return result
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 as PR comments)
Additionally, you can add 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
Outside diff range, codebase verification and nitpick comments (7)
plugin/controller/test/fixtures/apps/controller-app/config/plugin.js (1)
18-21: AOP Module Integration and Necessity QuestionableThe
@eggjs/tegg-aop-pluginpackage is declared in configuration files but is not referenced in the application logic. This suggests that the AOP module is not currently integrated or necessary for the application's functionality. Consider evaluating its necessity or removing it if not required.
- Files with declaration but no usage:
plugin/dal/test/fixtures/apps/dal-app/config/plugin.jsplugin/controller/test/fixtures/apps/controller-app/config/plugin.jsAnalysis chain
Verify the integration and necessity of the AOP module.
The addition of the
aopModuleenhances the application's functionality with AOP capabilities. Ensure that this module is correctly integrated and required for the application.Run the following script to verify the integration of the AOP module:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the `aopModule`. # Test: Search for the usage of the `aopModule`. Expect: The module should be used in the application. rg --type js '@eggjs/tegg-aop-plugin'Length of output: 235
Script:
#!/bin/bash # Description: Search for references to the AOP module in the application logic. # Test: Look for any usage of the AOP module in the application code. rg '@eggjs/tegg-aop-plugin' --type js -g '!config/plugin.js'Length of output: 258
plugin/controller/test/lib/EggControllerLoader.test.ts (1)
Line range hint
12-12: Enhance test description for clarity.The test description "should work" is vague. Consider providing a more descriptive name that reflects the purpose of the test.
Consider this modification:
it('should load all controllers including new AOP middleware', () => { // Test implementation });plugin/controller/test/fixtures/apps/controller-app/app/controller/AopMiddlewareController.ts (2)
26-30: Consider adding comments to explain middleware usage.Adding comments to explain the purpose of each middleware can improve code readability and maintainability.
// CountAdvice, FooControllerAdvice +// These advices apply cross-cutting concerns at the controller level.
39-41: Clarify method-level middleware application.Consider adding comments to explain the purpose of method-level middleware.
// FooMethodAdvice, BarMethodAdvice, CountAdvice, FooControllerAdvice +// These advices apply cross-cutting concerns at the method level.core/controller-decorator/test/fixtures/AopMiddlewareController.ts (2)
11-13: Implement advice logic inbeforeCall.Consider implementing the logic for the
beforeCallmethod to fulfill the advice's purpose.async beforeCall(): Promise<void> { - // ... + // Implement logic here }
47-52: Clarify method-level middleware application.Consider adding comments to explain the purpose of method-level middleware.
@Middleware(FooMethodAdvice, BarMethodAdvice) +// These advices apply cross-cutting concerns at the method level.core/controller-decorator/src/decorator/Middleware.ts (1)
Line range hint
28-72: LGTM! Enhanced middleware handling with type safety.The changes introduce a structured approach to handling middleware and AOP advice, enhancing type safety and maintainability.
Address the variable shadowing issue by renaming the
constructorparameter to avoid confusion with the globalconstructorproperty.Apply this diff to resolve the issue:
function functionTypeClassMiddleware(constructor: EggProtoImplClass) { - middlewares.forEach(mid => { + middlewares.forEach(middleware => { ControllerInfoUtil.addControllerMiddleware(mid, constructor); }); } function aopTypeClassMiddleware(constructor: EggProtoImplClass) { - for (const aopAdvice of middlewares as EggProtoImplClass<IAdvice>[]) { + for (const advice of middlewares as EggProtoImplClass<IAdvice>[]) { ControllerInfoUtil.addControllerAopMiddleware(aopAdvice, constructor); } }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (20)
- core/controller-decorator/package.json (1 hunks)
- core/controller-decorator/src/builder/ControllerMetaBuilderFactory.ts (2 hunks)
- core/controller-decorator/src/decorator/Middleware.ts (2 hunks)
- core/controller-decorator/src/util/ControllerInfoUtil.ts (2 hunks)
- core/controller-decorator/src/util/ControllerMetadataUtil.ts (1 hunks)
- core/controller-decorator/src/util/MethodInfoUtil.ts (4 hunks)
- core/controller-decorator/test/Middleware.test.ts (2 hunks)
- core/controller-decorator/test/fixtures/AopMiddlewareController.ts (1 hunks)
- core/controller-decorator/test/http/HTTPMeta.test.ts (5 hunks)
- core/types/controller-decorator/MetadataKey.ts (2 hunks)
- plugin/controller/lib/EggControllerPrototypeHook.ts (1 hunks)
- plugin/controller/test/fixtures/apps/controller-app/app/controller/AopMiddlewareController.ts (1 hunks)
- plugin/controller/test/fixtures/apps/controller-app/config/module.json (1 hunks)
- plugin/controller/test/fixtures/apps/controller-app/config/plugin.js (1 hunks)
- plugin/controller/test/fixtures/apps/controller-app/modules/multi-module-common/advice/BarMethodAdvice.ts (1 hunks)
- plugin/controller/test/fixtures/apps/controller-app/modules/multi-module-common/advice/CountAdvice.ts (1 hunks)
- plugin/controller/test/fixtures/apps/controller-app/modules/multi-module-common/advice/FooControllerAdvice.ts (1 hunks)
- plugin/controller/test/fixtures/apps/controller-app/modules/multi-module-common/advice/FooMethodAdvice.ts (1 hunks)
- plugin/controller/test/http/middleware.test.ts (1 hunks)
- plugin/controller/test/lib/EggControllerLoader.test.ts (1 hunks)
Additional context used
Biome
core/controller-decorator/src/decorator/Middleware.ts
[error] 29-29: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 35-35: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
core/controller-decorator/src/util/MethodInfoUtil.ts
[error] 95-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Additional comments not posted (28)
plugin/controller/test/fixtures/apps/controller-app/config/module.json (1)
2-3: Verify the module path and necessity.The addition of the
multi-module-commonmodule path expands the configuration. Ensure that this path is correct and the module is required for the application.Run the following script to verify the existence of the module path:
Verification successful
Module Path Verified and Necessary
The
multi-module-commonmodule path is correct and is actively used across multiple parts of the application, confirming its necessity.
- References found in various files, including
AppService.ts,AppRepo.ts, andAopMiddlewareController.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the `multi-module-common` module path. # Test: Check if the module path exists. Expect: The path should exist. if [ -d "plugin/controller/test/fixtures/apps/controller-app/modules/multi-module-common" ]; then echo "The module path exists." else echo "The module path does not exist." fiLength of output: 149
Script:
#!/bin/bash # Search for references to `multi-module-common` in the codebase to verify its necessity. # Look for import or require statements referencing `multi-module-common`. rg 'multi-module-common' --glob '!node_modules/*' --glob '!*dist/*' -A 3Length of output: 7599
plugin/controller/test/fixtures/apps/controller-app/modules/multi-module-common/advice/FooControllerAdvice.ts (1)
1-14: Verify the implementation and adherence to best practices.The
FooControllerAdviceclass implements an AOP advice pattern. Ensure that the implementation is correct and adheres to best practices for AOP in Egg.js.Run the following script to verify the implementation of the
FooControllerAdviceclass:Verification successful
Implementation of
FooControllerAdviceadheres to best practices.The
FooControllerAdviceclass is correctly implemented and integrated into the Egg.js application as middleware. It is also covered in tests, ensuring its functionality is verified.
- Integration: Used in
AopMiddlewareController.tswith@Middleware.- Testing: Referenced in
middleware.test.tsfor validation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `FooControllerAdvice` class. # Test: Search for the implementation and usage of the `FooControllerAdvice` class. Expect: The class should be correctly implemented and used. rg --type ts 'class FooControllerAdvice'Length of output: 206
Script:
#!/bin/bash # Description: Search for the usage of `FooControllerAdvice` in the codebase. # Test: Look for references to `FooControllerAdvice` to verify its integration and usage. rg 'FooControllerAdvice'Length of output: 1163
plugin/controller/test/fixtures/apps/controller-app/modules/multi-module-common/advice/BarMethodAdvice.ts (3)
1-5: Ensure proper imports.The imports from
@eggjs/tegg/aopare necessary for the advice functionality. Ensure these modules are correctly installed and available.
8-10: Correct use of the@Advicedecorator.The
@Advicedecorator is used correctly to set the access level to public.
11-17: Implementing thearoundmethod correctly.The
aroundmethod is implemented correctly, modifying theaopListproperty of thebodyobject.plugin/controller/test/fixtures/apps/controller-app/modules/multi-module-common/advice/FooMethodAdvice.ts (3)
1-5: Ensure proper imports.The imports from
@eggjs/tegg/aopare necessary for the advice functionality. Ensure these modules are correctly installed and available.
8-10: Correct use of the@Advicedecorator.The
@Advicedecorator is used correctly to set the access level to public.
11-17: Implementing thearoundmethod correctly.The
aroundmethod is implemented correctly, modifying theaopListproperty of thebodyobject.plugin/controller/lib/EggControllerPrototypeHook.ts (1)
10-12: Simplified control flow inpostCreatemethod.The direct use of
ControllerMetaBuilderFactory.buildsimplifies the code by removing unnecessary steps.plugin/controller/test/fixtures/apps/controller-app/modules/multi-module-common/advice/CountAdvice.ts (1)
12-16: Ensure thread safety forthis.index.If this middleware is used in a concurrent environment, consider using a more thread-safe approach for incrementing
this.index.Verify if
this.indexis shared across instances or if each request has its own instance. If shared, consider using a locking mechanism or atomic operations.core/controller-decorator/src/util/ControllerMetadataUtil.ts (1)
16-16: Refactoring improves clarity and reduces complexity.The direct call to
ControllerMetaBuilderFactory.buildsimplifies the code and enhances maintainability.plugin/controller/test/fixtures/apps/controller-app/app/controller/AopMiddlewareController.ts (1)
14-17: Good use of decorators for controller and middleware.The application of
@HTTPControllerand@Middlewaredecorators is consistent with the framework's style and enhances modularity.core/controller-decorator/test/fixtures/AopMiddlewareController.ts (2)
7-14: Well-structured advice class with appropriate decorator usage.The
FooAdviceclass is correctly using the@Advicedecorator and implements theIAdviceinterface.
43-45: Good use of decorators for controller and middleware.The application of
@Middlewareand@HTTPControllerdecorators is consistent with the framework's style and enhances modularity.core/controller-decorator/package.json (2)
41-41: Approved addition of new dependencies.The dependencies
@eggjs/aop-decoratorandis-type-ofare appropriately added to support new features.Also applies to: 48-48
41-41: Verify compatibility of new dependencies.Ensure that the new dependencies are compatible with the existing codebase and do not introduce conflicts.
Also applies to: 48-48
plugin/controller/test/http/middleware.test.ts (1)
59-69: LGTM! New test cases enhance coverage.The new test cases for AOP middleware are well-structured and enhance the test coverage.
Ensure that the endpoints
/aop/middleware/globaland/aop/middleware/methodare correctly implemented and return the expected responses.Run the following script to verify the implementation of the endpoints:
core/controller-decorator/src/util/ControllerInfoUtil.ts (1)
17-20: LGTM! New methods enhance AOP middleware management.The methods
addControllerAopMiddlewareandgetControllerAopMiddlewaresare consistent with existing patterns and enhance AOP middleware management.Ensure that the metadata utilities are correctly used and that the new methods are integrated into the system.
Run the following script to verify the usage of metadata utilities:
Verification successful
Metadata utilities are correctly used in
ControllerInfoUtil.ts. The methodsaddControllerAopMiddlewareandgetControllerAopMiddlewaresutilizeMetadataUtil.initOwnArrayMetaDataandMetadataUtil.getMetaDataappropriately, aligning with existing patterns for AOP middleware management.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of metadata utilities for AOP middleware. # Test: Search for the usage of metadata utilities. Expect: Correct usage of metadata utilities. rg --type typescript -A 5 $'MetadataUtil.initOwnArrayMetaData' rg --type typescript -A 5 $'MetadataUtil.getMetaData'Length of output: 184
Script:
#!/bin/bash # Description: Verify the usage of metadata utilities for AOP middleware. # Test: Search for the usage of metadata utilities. Expect: Correct usage of metadata utilities. rg --glob '*.ts' -A 5 $'MetadataUtil.initOwnArrayMetaData' rg --glob '*.ts' -A 5 $'MetadataUtil.getMetaData'Length of output: 33376
core/controller-decorator/src/util/MethodInfoUtil.ts (4)
52-56: LGTM!The method
addMethodAopMiddlewarecorrectly adds AOP middleware to the map.
58-61: LGTM!The method
getMethodAopMiddlewarescorrectly retrieves AOP middleware from the map.
88-91: LGTM!The method
getMethodscorrectly retrieves all method names for a class.
98-101: LGTM!The method
registerAopMiddlewarePointcutcorrectly registers an AOP middleware pointcut.core/controller-decorator/test/http/HTTPMeta.test.ts (2)
23-29: LGTM!The new imports for AOP middleware-related classes are necessary for the test cases.
153-181: LGTM!The new test case for AOP middleware correctly verifies the functionality and order of advice.
core/controller-decorator/test/Middleware.test.ts (2)
5-11: Imports for AOP middleware testing are correctly added.The new imports are necessary for the AOP middleware tests and are correctly included.
27-34: New test case for AOP middleware is well-implemented.The test case effectively verifies the behavior of AOP middleware for the controller and its methods.
core/types/controller-decorator/MetadataKey.ts (1)
5-5: New constants for AOP middleware are appropriately added.The constants enhance the middleware structure, allowing for more flexible AOP middleware management.
Also applies to: 21-22
core/controller-decorator/src/builder/ControllerMetaBuilderFactory.ts (1)
32-48: New static methodbuildis well-implemented.The method effectively supports AOP middleware registration, enhancing the control flow with significant functionality.
...ntroller/test/fixtures/apps/controller-app/modules/multi-module-common/advice/CountAdvice.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
core/controller-decorator/src/decorator/Middleware.ts (1)
Line range hint
28-72: Avoid shadowing the globalconstructorproperty.The variable
constructorshadows the global property. Consider renaming it to avoid confusion.Apply this diff to rename the variable:
function functionTypeClassMiddleware(constructor: EggProtoImplClass) { - middlewares.forEach(mid => { + middlewares.forEach(middleware => { ControllerInfoUtil.addControllerMiddleware(middleware, constructor); }); } function aopTypeClassMiddleware(constructor: EggProtoImplClass) { - for (const aopAdvice of middlewares as EggProtoImplClass<IAdvice>[]) { + for (const advice of middlewares as EggProtoImplClass<IAdvice>[]) { ControllerInfoUtil.addControllerAopMiddleware(advice, constructor); } }The rest of the
Middlewarefunction is well-structured and correctly applies middleware based on its type.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- core/controller-decorator/src/decorator/Middleware.ts (2 hunks)
- plugin/schedule/test/fixtures/schedule-app/config/plugin.js (1 hunks)
Files skipped from review due to trivial changes (1)
- plugin/schedule/test/fixtures/schedule-app/config/plugin.js
Additional context used
Biome
core/controller-decorator/src/decorator/Middleware.ts
[error] 29-29: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 35-35: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
Additional comments not posted (3)
core/controller-decorator/src/decorator/Middleware.ts (3)
8-11: EnumMiddlewareTypeis well-defined.The
MiddlewareTypeenum correctly distinguishes betweenAOPandMiddlewareFunc.
13-15: FunctionisAopis correctly implemented.The function accurately checks if a middleware is an AOP advice class.
17-26: FunctionisAopTypeOrMiddlewareTypeis well-implemented.The function accurately determines the middleware type and prevents mixing AOP and MiddlewareFunc types.
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores