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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

fix: fix aop in constructor inject type #247

Merged
merged 1 commit into from
Oct 10, 2024
Merged

fix: fix aop in constructor inject type #247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion core/aop-runtime/src/EggObjectAopHook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { ASPECT_LIST } from '@eggjs/tegg-types';
import { ASPECT_LIST, InjectType } from '@eggjs/tegg-types';
import type { EggObject, EggObjectLifeCycleContext, LifecycleHook } from '@eggjs/tegg-types';
import { Aspect } from '@eggjs/aop-decorator';
import { AspectExecutor } from './AspectExecutor';
import { PrototypeUtil } from '@eggjs/core-decorator';
import assert from 'node:assert';
import { EggContainerFactory } from '@eggjs/tegg-runtime';

export class EggObjectAopHook implements LifecycleHook<EggObjectLifeCycleContext, EggObject> {
private hijackMethods(obj: any, aspectList: Array<Aspect>) {
Expand All @@ -11,6 +14,25 @@ export class EggObjectAopHook implements LifecycleHook<EggObjectLifeCycleContext
}
}

// constructor inject only paas obj to constructor
// should manually define obj to property
private injectAdvice(eggObject: EggObject, obj: any, aspectList: Array<Aspect>) {
if (eggObject.proto.getMetaData(PrototypeUtil.INJECT_TYPE) !== InjectType.CONSTRUCTOR) {
return;
}
for (const aspect of aspectList) {
for (const advice of aspect.adviceList) {
const injectObject = eggObject.proto.injectObjects.find(t => t.objName === advice.name);
assert(injectObject, `not found inject advice ${advice.name}`);
const adviceObj = EggContainerFactory.getEggObject(injectObject!.proto, advice.name);
Object.defineProperty(obj, advice.name, {
value: adviceObj.obj,
enumerable: false,
});
}
}
}

async postCreate(_: EggObjectLifeCycleContext, eggObject: EggObject): Promise<void> {
const aspectList: Array<Aspect> | undefined = eggObject.proto.getMetaData(ASPECT_LIST);
if (!aspectList || !aspectList.length) return;
Expand All @@ -25,12 +47,14 @@ export class EggObjectAopHook implements LifecycleHook<EggObjectLifeCycleContext
if (!obj) {
obj = Reflect.apply(propertyDesc.get!, eggObject, []);
self.hijackMethods(obj, aspectList);
self.injectAdvice(eggObject, obj, aspectList);
}
return obj;
},
});
} else {
this.hijackMethods(eggObject.obj, aspectList);
this.injectAdvice(eggObject, eggObject.obj, aspectList);
}
}
}
98 changes: 98 additions & 0 deletions core/aop-runtime/test/aop-runtime.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { EggPrototypeCrossCutHook } from '../src/EggPrototypeCrossCutHook';
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';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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


describe('test/aop-runtime.test.ts', () => {
describe('succeed call', () => {
Expand Down Expand Up @@ -160,4 +161,101 @@ describe('test/aop-runtime.test.ts', () => {
}, /Aop Advice\(PointcutAdvice\) not found in loadUnits/);
});
});

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);
});
Comment on lines +165 to +200
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.


it('should work', async () => {
await EggTestContext.mockContext(async () => {
const hello = await CoreTestHelper.getObject(HelloConstructorInject);
const callTrace = await CoreTestHelper.getObject(CallTrace);
const msg = await hello.hello('aop');
const traceMsg = callTrace.msgs;
console.log('msg: ', msg, traceMsg);
assert.deepStrictEqual(msg, `withPointAroundResult(hello withPointAroundParam(aop)${JSON.stringify(pointcutAdviceParams)})`);
assert.deepStrictEqual(traceMsg, [
{
className: 'PointcutAdvice',
methodName: 'beforeCall',
id: 233,
name: 'aop',
adviceParams: pointcutAdviceParams,
},
{
className: 'PointcutAdvice',
methodName: 'afterReturn',
id: 233,
name: 'withPointAroundParam(aop)',
result: `withPointAroundResult(hello withPointAroundParam(aop)${JSON.stringify(pointcutAdviceParams)})`,
adviceParams: pointcutAdviceParams,
},
{
className: 'PointcutAdvice',
methodName: 'afterFinally',
id: 233,
name: 'withPointAroundParam(aop)',
adviceParams: pointcutAdviceParams,
},
]);

await assert.rejects(async () => {
await hello.helloWithException('foo');
}, new Error('ops, exception for withPointAroundParam(foo)'));
assert.deepStrictEqual(callTrace.msgs[callTrace.msgs.length - 2], {
className: 'PointcutAdvice',
methodName: 'afterThrow',
id: 233,
name: 'withPointAroundParam(foo)',
result: 'ops, exception for withPointAroundParam(foo)',
adviceParams: pointcutAdviceParams,
});
});
});

it('mock should work', async () => {
await EggTestContext.mockContext(async () => {
const hello = await CoreTestHelper.getObject(HelloConstructorInject);
let helloMocked = false;
mm(HelloConstructorInject.prototype, 'hello', async () => {
helloMocked = true;
});
Comment on lines +253 to +255
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

await hello.hello('aop');
assert(helloMocked);
});
});
});
});
26 changes: 26 additions & 0 deletions core/aop-runtime/test/fixtures/modules/constructor_inject_aop/Hello.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { ContextProto, Inject, SingletonProto } from '@eggjs/core-decorator';
import { Pointcut } from '@eggjs/aop-decorator';
import { PointcutAdvice, pointcutAdviceParams } from '../hello_point_cut/HelloPointCut';

@SingletonProto()
export class Foo {
}

@ContextProto()
export class HelloConstructorInject {
id = 233;

constructor(@Inject() readonly foo: Foo) {
}

@Pointcut(PointcutAdvice, { adviceParams: pointcutAdviceParams })
async hello(name: string) {
return `hello ${name}`;
}

@Pointcut(PointcutAdvice, { adviceParams: pointcutAdviceParams })
async helloWithException(name: string) {
throw new Error(`ops, exception for ${name}`);
}

}
6 changes: 6 additions & 0 deletions core/aop-runtime/test/fixtures/modules/constructor_inject_aop/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"name": "aop-module",
"eggModule": {
"name": "aopModule"
}
}
11 changes: 10 additions & 1 deletion plugin/aop/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,14 @@
import { Application } from 'egg';
import { CrosscutAdviceFactory } from '@eggjs/tegg/aop';
import { EggObjectAopHook, EggPrototypeCrossCutHook, LoadUnitAopHook } from '@eggjs/tegg-aop-runtime';
import {
crossCutGraphHook,
EggObjectAopHook,
EggPrototypeCrossCutHook,
LoadUnitAopHook,
pointCutGraphHook,
} from '@eggjs/tegg-aop-runtime';
import { AopContextHook } from './lib/AopContextHook';
import { GlobalGraph } from '@eggjs/tegg-metadata';

export default class AopAppHook {
private readonly app: Application;
Expand All @@ -24,6 +31,8 @@ export default class AopAppHook {
this.app.eggPrototypeLifecycleUtil.registerLifecycle(this.eggPrototypeCrossCutHook);
this.app.loadUnitLifecycleUtil.registerLifecycle(this.loadUnitAopHook);
this.app.eggObjectLifecycleUtil.registerLifecycle(this.eggObjectAopHook);
GlobalGraph.instance!.registerBuildHook(crossCutGraphHook);
GlobalGraph.instance!.registerBuildHook(pointCutGraphHook);
}

async didLoad() {
Expand Down
3 changes: 1 addition & 2 deletions plugin/tegg/lib/EggModuleLoader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,11 @@ export class EggModuleLoader {
});
}
const graph = GlobalGraph.create(moduleDescriptors);
graph.build();
return graph;
}

private async loadModule() {
this.buildAppGraph();
this.globalGraph.build();
this.globalGraph.sort();
const moduleConfigList = this.globalGraph.moduleConfigList;
for (const moduleConfig of moduleConfigList) {
Expand Down
2 changes: 1 addition & 1 deletion standalone/standalone/src/EggModuleLoader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,12 @@ export class EggModuleLoader {
});
}
const globalGraph = GlobalGraph.create(moduleDescriptors);
globalGraph.build();
return globalGraph;
}

async load(): Promise<LoadUnit[]> {
const loadUnits: LoadUnit[] = [];
this.globalGraph.build();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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/catch blocks 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 within try/catch blocks 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:

  1. 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.
  2. If load() could potentially be called multiple times, consider adding a check to prevent unnecessary rebuilding of the graph.
  3. 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

this.globalGraph.sort();
const moduleConfigList = GlobalGraph.instance!.moduleConfigList;
for (const moduleConfig of moduleConfigList) {
Expand Down
11 changes: 9 additions & 2 deletions standalone/standalone/src/Runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { ModuleConfigUtil, ModuleReference, ReadModuleReferenceOptions, RuntimeConfig } from '@eggjs/tegg-common-util';
import {
EggPrototype,
EggPrototypeLifecycleUtil,
EggPrototypeLifecycleUtil, GlobalGraph,
LoadUnit,
LoadUnitFactory,
LoadUnitLifecycleUtil,
Expand All @@ -26,7 +26,12 @@ import {
} from '@eggjs/tegg';
import { StandaloneUtil, MainRunner } from '@eggjs/tegg/standalone';
import { CrosscutAdviceFactory } from '@eggjs/tegg/aop';
import { EggObjectAopHook, EggPrototypeCrossCutHook, LoadUnitAopHook } from '@eggjs/tegg-aop-runtime';
import {
crossCutGraphHook,
EggObjectAopHook,
EggPrototypeCrossCutHook,
LoadUnitAopHook, pointCutGraphHook,
} from '@eggjs/tegg-aop-runtime';

import { EggModuleLoader } from './EggModuleLoader';
import { InnerObject, StandaloneLoadUnit, StandaloneLoadUnitType } from './StandaloneLoadUnit';
Expand Down Expand Up @@ -143,6 +148,8 @@ export class Runner {
logger: ((this.innerObjects.logger && this.innerObjects.logger[0])?.obj as Logger) || console,
baseDir: this.cwd,
});
GlobalGraph.instance!.registerBuildHook(crossCutGraphHook);
GlobalGraph.instance!.registerBuildHook(pointCutGraphHook);
const configSourceEggPrototypeHook = new ConfigSourceLoadUnitHook();
LoadUnitLifecycleUtil.registerLifecycle(configSourceEggPrototypeHook);

Expand Down
Loading