fix: fix aop in constructor inject type #247
Conversation
WalkthroughThe changes in this pull request primarily enhance the Aspect-Oriented Programming (AOP) capabilities within the application. Key modifications include the addition of a new method, Changes
Possibly related PRs
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: 4
🧹 Outside diff range and nitpick comments (3)
plugin/aop/app.ts (1)
34-35: Approve changes with a suggestion for safer GlobalGraph access.The addition of
crossCutGraphHookandpointCutGraphHooktoGlobalGraph.instanceenhances the AOP capabilities of the application. However, the use of the non-null assertion operator (!) might lead to runtime errors ifGlobalGraph.instanceis null or undefined.Consider adding a null check or using optional chaining for safer access:
if (GlobalGraph.instance) { GlobalGraph.instance.registerBuildHook(crossCutGraphHook); GlobalGraph.instance.registerBuildHook(pointCutGraphHook); } else { // Handle the case where GlobalGraph.instance is null or undefined console.warn('GlobalGraph.instance is not available'); }Or using optional chaining:
GlobalGraph.instance?.registerBuildHook(crossCutGraphHook); GlobalGraph.instance?.registerBuildHook(pointCutGraphHook);This approach would make the code more robust and prevent potential runtime errors.
core/aop-runtime/src/EggObjectAopHook.ts (1)
17-18: Clarify comments for better readabilityThe comments at lines 17~ and 18~ contain grammatical errors and might be unclear to readers. Consider rephrasing them to enhance clarity and understanding.
Suggested rephrasing:
-// constructor inject only paas obj to constructor -// should manually define obj to property +// For constructor injection, the injected object is only passed to the constructor. +// We need to manually define the advice objects as properties on the instance.core/aop-runtime/test/aop-runtime.test.ts (1)
202-247: Remove Unnecessaryconsole.logStatement in TestThe
console.log('msg: ', msg, traceMsg);statement on line 208 is primarily used for debugging purposes. Leaving console statements in committed test code can clutter test outputs and is not recommended in production code.Suggested Change:
// Remove the console.log statement - console.log('msg: ', msg, traceMsg);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- core/aop-runtime/src/EggObjectAopHook.ts (3 hunks)
- core/aop-runtime/test/aop-runtime.test.ts (2 hunks)
- core/aop-runtime/test/fixtures/modules/constructor_inject_aop/Hello.ts (1 hunks)
- core/aop-runtime/test/fixtures/modules/constructor_inject_aop/package.json (1 hunks)
- plugin/aop/app.ts (2 hunks)
- plugin/tegg/lib/EggModuleLoader.ts (1 hunks)
- standalone/standalone/src/EggModuleLoader.ts (1 hunks)
- standalone/standalone/src/Runner.ts (3 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/aop-runtime/test/fixtures/modules/constructor_inject_aop/package.json
🧰 Additional context used
🔇 Additional comments (16)
plugin/tegg/lib/EggModuleLoader.ts (1)
47-48: Approved changes, but clarification needed onsort()methodThe changes look good and potentially improve performance by avoiding rebuilding the entire graph each time
loadModule()is called. However, I have a few points to address:
- Can you provide more information about the
sort()method ofglobalGraph? Understanding its purpose and impact would be helpful.- Consider updating the class documentation to reflect that the graph is now built once in the constructor and updated in
loadModule().- It might be worth adding a comment explaining why sorting the graph is necessary at this point in the code.
To verify the impact of these changes, let's check how
GlobalGraphis used elsewhere in the codebase:✅ Verification successful
Approved changes with verification
The changes have been verified and the usage of the
sort()method inglobalGraphis appropriate and confined to the intended areas withinEggModuleLoader.ts. No further issues were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of GlobalGraph in the codebase # Search for GlobalGraph usage echo "GlobalGraph usage:" rg --type typescript "GlobalGraph" -A 5 # Search for sort method calls on GlobalGraph echo "\nGlobalGraph sort method usage:" rg --type typescript "globalGraph\.sort\(\)" -A 5Length of output: 283
Script:
#!/bin/bash # Description: Check usage of GlobalGraph in TypeScript (.ts) files # Search for GlobalGraph usage in .ts files echo "GlobalGraph usage in .ts files:" rg "GlobalGraph" -g "*.ts" -A 5 # Search for sort method calls on globalGraph in .ts files echo "\nglobalGraph.sort() method usage in .ts files:" rg "globalGraph\.sort\(\)" -g "*.ts" -A 5Length of output: 32223
plugin/aop/app.ts (3)
3-9: LGTM: Import statements are well-organized and relevant.The new imports for
crossCutGraphHook,pointCutGraphHook, andGlobalGraphare appropriately added and grouped with related imports. This change enhances the AOP functionality of the application and improves code organization.Also applies to: 11-11
Line range hint
1-52: Summary: AOP enhancements well-integrated, minor improvements suggested.Overall, the changes to
plugin/aop/app.tsenhance the AOP capabilities of the application by introducing new graph hooks (crossCutGraphHookandpointCutGraphHook) and registering them with the global graph. The modifications are well-integrated and consistent with the file's purpose.Key points:
- Import statements are appropriately updated and organized.
- New hooks are registered in the
configDidLoadmethod.- The overall structure of the
AopAppHookclass remains consistent.Suggestions for improvement:
- Consider adding null checks when accessing
GlobalGraph.instance.- Update documentation to explain the purpose and impact of the new hooks.
These changes appear to be a positive enhancement to the AOP system. Once the minor suggestions are addressed, this code should be ready for merging.
3-9: Request for additional context and documentation updates.The introduction of
crossCutGraphHookandpointCutGraphHookappears to enhance the AOP capabilities of the application. However, the full impact of these changes on the rest of the application is not immediately clear from this file alone.Could you provide more context on how these new hooks are used in other parts of the application? This information would help in assessing the overall impact of these changes.
Additionally, consider updating the documentation to reflect these new AOP enhancements. This could include:
- A brief explanation of the purpose of
crossCutGraphHookandpointCutGraphHook.- How these hooks interact with the rest of the AOP system.
- Any potential performance implications or considerations for developers using these new hooks.
To help understand the usage of these new hooks, we can search for their occurrences in the codebase:
This will help identify other files that might need to be updated or reviewed in light of these changes.
Also applies to: 11-11, 34-35
✅ Verification successful
AOP Hooks Integration Verified Successfully.
The imports of
crossCutGraphHookandpointCutGraphHookinplugin/aop/app.tsare utilized across multiple parts of the application, including:
standalone/standalone/src/Runner.tscore/aop-runtime/test/aop-runtime.test.tscore/aop-runtime/src/CrossCutGraphHook.tscore/aop-runtime/src/PointCutGraphHook.tsThis confirms that the introduction of these hooks enhances the AOP capabilities as intended without disrupting existing functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of crossCutGraphHook and pointCutGraphHook echo "Occurrences of crossCutGraphHook:" rg --type typescript "crossCutGraphHook" echo "\nOccurrences of pointCutGraphHook:" rg --type typescript "pointCutGraphHook"Length of output: 309
Script:
#!/bin/bash # Search for usages of crossCutGraphHook and pointCutGraphHook in TypeScript files echo "Occurrences of crossCutGraphHook:" rg "crossCutGraphHook" -g "*.ts" -g "*.tsx" echo "\nOccurrences of pointCutGraphHook:" rg "pointCutGraphHook" -g "*.ts" -g "*.tsx"Length of output: 1510
core/aop-runtime/test/fixtures/modules/constructor_inject_aop/Hello.ts (6)
1-3: Import statements are correct and necessaryThe import statements correctly include the required decorators and advice classes, ensuring that the aspect-oriented programming and dependency injection functionalities are available.
5-7: SingletonFooclass is properly declaredThe
Fooclass is appropriately annotated with@SingletonProto(), which ensures a single instance ofFoothroughout the application.
9-12:HelloConstructorInjectclass is correctly defined with context scopeThe
HelloConstructorInjectclass is annotated with@ContextProto(), indicating it is context-scoped. Theidproperty is initialized correctly.
13-15: Constructor dependency injection is correctly implementedThe constructor properly injects the
Fooinstance using the@Inject()decorator. Declaringfooasreadonlyensures the injected dependency remains immutable after construction.
16-19:hellomethod correctly applies pointcut adviceThe
hellomethod is appropriately decorated with@Pointcut, utilizingPointcutAdviceandpointcutAdviceParams. The method implementation correctly returns a greeting message.
21-24:helloWithExceptionmethod correctly simulates exception handling with pointcut adviceThe
helloWithExceptionmethod is also decorated with@Pointcutand correctly throws an error, which is suitable for testing exception scenarios in aspect-oriented programming.core/aop-runtime/src/EggObjectAopHook.ts (3)
1-7: Necessary imports are correctly addedThe newly added imports at lines 1~, 5~, 6~, and 7~ are appropriate and required for the implementation of the
injectAdvicemethod.
19-34:injectAdvicemethod is well-implementedThe
injectAdvicemethod is correctly implemented to inject advice objects into the target object when constructor injection is used. The logic ensures that advice objects are properly assigned and made non-enumerable.
50-50: Proper integration ofinjectAdviceinpostCreatemethodThe calls to
injectAdviceat lines 50~ and 57~ appropriately ensure that advice objects are injected after method hijacking. This integration maintains the intended behavior of the AOP framework.Also applies to: 57-57
standalone/standalone/src/Runner.ts (3)
4-4: Addition of 'GlobalGraph' import is appropriate and necessaryImporting
GlobalGraphfrom@eggjs/tegg-metadatais needed for registering build hooks later in the code.
29-34: Appropriate addition of AOP-related imports from '@eggjs/tegg-aop-runtime'Importing
crossCutGraphHook,pointCutGraphHook,EggObjectAopHook,EggPrototypeCrossCutHook, andLoadUnitAopHookenhances the AOP capabilities of theRunnerclass.
151-152: Ensure 'GlobalGraph.instance' is initialized before registering build hooksThe use of the non-null assertion operator
!onGlobalGraph.instanceassumes it is initialized. To prevent potential runtime errors, verify thatGlobalGraph.instanceis properly initialized before these calls.Run the following script to verify the initialization of
GlobalGraph.instance:
| return globalGraph; | ||
| } | ||
|
|
||
| async load(): Promise<LoadUnit[]> { | ||
| const loadUnits: LoadUnit[] = []; | ||
| this.globalGraph.build(); |
There was a problem hiding this comment.
💡 Codebase verification
Multiple globalGraph.build() Calls Lacking Error Handling
The verification identified multiple instances of globalGraph.build() within the codebase that are not surrounded by error handling mechanisms. Specifically:
plugin/tegg/lib/EggModuleLoader.ts: this.globalGraph.build();standalone/standalone/src/EggModuleLoader.ts: this.globalGraph.build();
Concerns:
- Exception Handling: The absence of
try/catchblocks around these calls may lead to unhandled exceptions, potentially destabilizing the module loading process. - Redundant Builds: Multiple invocations of
build()could result in unnecessary performance overhead if not managed correctly.
Recommendations:
- Implement Error Handling: Wrap each
globalGraph.build()call withintry/catchblocks to gracefully handle potential exceptions. - Review Invocation Frequency: Assess whether multiple
build()calls are necessary or if they can be optimized to prevent redundant executions.
🔗 Analysis chain
Ensure graph consistency by building before use.
The addition of this.globalGraph.build() at the beginning of the load method ensures that the graph is properly built before it's sorted and used for creating load units. This change improves the consistency and reliability of the module loading process.
Consider the following points:
- This change shifts the responsibility of building the graph from the initial generation phase to the loading phase, which might affect the overall flow of the application.
- If
load()could potentially be called multiple times, consider adding a check to prevent unnecessary rebuilding of the graph. - Error handling for
build()might be beneficial to gracefully handle any exceptions that could occur during the build process.
To ensure this change doesn't introduce any unintended side effects, please run the following verification:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other occurrences of globalGraph.build() in the codebase
# Test: Search for other occurrences of globalGraph.build(). Expect: No other occurrences.
rg --type typescript 'globalGraph\.build\(\)'
# Test: Check if there are any error handling mechanisms for build(). Expect: Some form of error handling.
rg --type typescript 'globalGraph\.build\(\).*try|catch'
Length of output: 176
Script:
#!/bin/bash
# Description: Check for any other occurrences of globalGraph.build() in the codebase
# Test: Search for other occurrences of globalGraph.build(). Expect: No other occurrences.
rg --type ts 'globalGraph\.build\(\)'
# Test: Check if there are any error handling mechanisms for build(). Expect: Some form of error handling.
rg --type ts 'globalGraph\.build\(\).*try|catch'
Length of output: 2409
| describe('aop constructor should work', () => { | ||
| let modules: Array<LoadUnitInstance>; | ||
| let crosscutAdviceFactory: CrosscutAdviceFactory; | ||
| let eggObjectAopHook: EggObjectAopHook; | ||
| let loadUnitAopHook: LoadUnitAopHook; | ||
| let eggPrototypeCrossCutHook: EggPrototypeCrossCutHook; | ||
|
|
||
| beforeEach(async () => { | ||
| crosscutAdviceFactory = new CrosscutAdviceFactory(); | ||
| eggObjectAopHook = new EggObjectAopHook(); | ||
| loadUnitAopHook = new LoadUnitAopHook(crosscutAdviceFactory); | ||
| eggPrototypeCrossCutHook = new EggPrototypeCrossCutHook(crosscutAdviceFactory); | ||
| EggPrototypeLifecycleUtil.registerLifecycle(eggPrototypeCrossCutHook); | ||
| LoadUnitLifecycleUtil.registerLifecycle(loadUnitAopHook); | ||
| EggObjectLifecycleUtil.registerLifecycle(eggObjectAopHook); | ||
|
|
||
| modules = await CoreTestHelper.prepareModules([ | ||
| path.join(__dirname, '..'), | ||
| path.join(__dirname, 'fixtures/modules/constructor_inject_aop'), | ||
| path.join(__dirname, 'fixtures/modules/hello_point_cut'), | ||
| path.join(__dirname, 'fixtures/modules/hello_cross_cut'), | ||
| ], [ | ||
| crossCutGraphHook, | ||
| pointCutGraphHook, | ||
| ]); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| for (const module of modules) { | ||
| await LoadUnitFactory.destroyLoadUnit(module.loadUnit); | ||
| await LoadUnitInstanceFactory.destroyLoadUnitInstance(module); | ||
| } | ||
| EggPrototypeLifecycleUtil.deleteLifecycle(eggPrototypeCrossCutHook); | ||
| LoadUnitLifecycleUtil.deleteLifecycle(loadUnitAopHook); | ||
| EggObjectLifecycleUtil.deleteLifecycle(eggObjectAopHook); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to Eliminate Redundant beforeEach and afterEach Blocks
The beforeEach and afterEach blocks in this test suite are nearly identical to those in the previous suites. To enhance maintainability and reduce code duplication, consider abstracting the shared setup and teardown logic into reusable helper functions or moving them to a higher scope if applicable.
Suggested Refactor:
Create helper functions for setup and teardown:
// Define a type for the setup components
interface TestEnvironmentSetup {
modules: Array<LoadUnitInstance>;
crosscutAdviceFactory: CrosscutAdviceFactory;
eggObjectAopHook: EggObjectAopHook;
loadUnitAopHook: LoadUnitAopHook;
eggPrototypeCrossCutHook: EggPrototypeCrossCutHook;
}
// Extract setup logic
async function initializeTestEnvironment(modulePaths: string[]): Promise<TestEnvironmentSetup> {
const crosscutAdviceFactory = new CrosscutAdviceFactory();
const eggObjectAopHook = new EggObjectAopHook();
const loadUnitAopHook = new LoadUnitAopHook(crosscutAdviceFactory);
const eggPrototypeCrossCutHook = new EggPrototypeCrossCutHook(crosscutAdviceFactory);
EggPrototypeLifecycleUtil.registerLifecycle(eggPrototypeCrossCutHook);
LoadUnitLifecycleUtil.registerLifecycle(loadUnitAopHook);
EggObjectLifecycleUtil.registerLifecycle(eggObjectAopHook);
const modules = await CoreTestHelper.prepareModules(modulePaths, [
crossCutGraphHook,
pointCutGraphHook,
]);
return {
modules,
crosscutAdviceFactory,
eggObjectAopHook,
loadUnitAopHook,
eggPrototypeCrossCutHook,
};
}
// Extract teardown logic
async function cleanupTestEnvironment(setup: TestEnvironmentSetup): Promise<void> {
for (const module of setup.modules) {
await LoadUnitFactory.destroyLoadUnit(module.loadUnit);
await LoadUnitInstanceFactory.destroyLoadUnitInstance(module);
}
EggPrototypeLifecycleUtil.deleteLifecycle(setup.eggPrototypeCrossCutHook);
LoadUnitLifecycleUtil.deleteLifecycle(setup.loadUnitAopHook);
EggObjectLifecycleUtil.deleteLifecycle(setup.eggObjectAopHook);
}
// Use the helper functions in your test suite
let testSetup: TestEnvironmentSetup;
beforeEach(async () => {
testSetup = await initializeTestEnvironment([
path.join(__dirname, '..'),
path.join(__dirname, 'fixtures/modules/constructor_inject_aop'),
path.join(__dirname, 'fixtures/modules/hello_point_cut'),
path.join(__dirname, 'fixtures/modules/hello_cross_cut'),
]);
});
afterEach(async () => {
await cleanupTestEnvironment(testSetup);
});This refactor promotes code reuse and simplifies future maintenance.
| mm(HelloConstructorInject.prototype, 'hello', async () => { | ||
| helloMocked = true; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Mocking Prototypes Directly
Mocking methods on the prototype directly using mm(HelloConstructorInject.prototype, 'hello', ...) can have unintended side effects on other tests that rely on HelloConstructorInject. It's better to mock the method on the instance to ensure test isolation.
Suggested Modification:
let helloMocked = false;
- mm(HelloConstructorInject.prototype, 'hello', async () => {
+ mm(hello, 'hello', async () => {
helloMocked = true;
});By mocking the hello method on the hello instance, you limit the scope of the mock to this test case.
Committable suggestion was skipped due to low confidence.
| @@ -15,6 +15,7 @@ | |||
| import { crossCutGraphHook } from '../src/CrossCutGraphHook'; | |||
| import { pointCutGraphHook } from '../src/PointCutGraphHook'; | |||
| import { CallTrace } from './fixtures/modules/hello_cross_cut/CallTrace'; | |||
| import { HelloConstructorInject } from './fixtures/modules/constructor_inject_aop/Hello'; | |||
There was a problem hiding this comment.
💡 Codebase verification
Import Path Incorrect: HelloConstructorInject Not Exported from Specified Module
The module './fixtures/modules/constructor_inject_aop/Hello' does not export HelloConstructorInject. Please verify that HelloConstructorInject is correctly exported in core/aop-runtime/test/fixtures/modules/constructor_inject_aop/Hello.ts or update the import path to point to the correct module exporting HelloConstructorInject.
- File to Verify:
core/aop-runtime/test/fixtures/modules/constructor_inject_aop/Hello.ts
🔗 Analysis chain
Import Path Verification for HelloConstructorInject
Please verify that the import path './fixtures/modules/constructor_inject_aop/Hello' correctly leads to a module exporting HelloConstructorInject. This ensures that the module is correctly resolved at runtime and prevents potential import errors.
Run the following script to confirm the export:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if 'HelloConstructorInject' is exported from the specified module.
# Test: Search for the export statement of 'HelloConstructorInject'. Expect: Export declaration to be present.
ast-grep --lang typescript --pattern 'export class HelloConstructorInject { $$$ }' './fixtures/modules/constructor_inject_aop/Hello.ts'
Length of output: 233
Script:
#!/bin/bash
# Description: Search for files named 'Hello.ts' in the repository to verify their existence and paths.
fd 'Hello\.ts$' --type f
Length of output: 2768
Script:
#!/bin/bash
# Description: Check if 'HelloConstructorInject' is exported from the correct 'Hello.ts' module.
# Test: Search for the export statement of 'HelloConstructorInject'. Expect: Export declaration to be present.
ast-grep --lang typescript --pattern 'export class HelloConstructorInject { $$$ }' 'core/aop-runtime/test/fixtures/modules/constructor_inject_aop/Hello.ts'
Length of output: 155
Script:
#!/bin/bash
# Description: Search the entire repository for the export of 'HelloConstructorInject'.
# Test: Look for any export statements of 'HelloConstructorInject' across all TypeScript files.
ast-grep --lang typescript --pattern 'export (class|const|function|interface) HelloConstructorInject { $$$ }' '**/*.ts'
Length of output: 176
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
package.jsonfile created for the "aop-module" to define module configuration.