feat: add FactoryQualifier #379
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds factory-qualifier metadata, a decorator and utility to declare allowed dynamic qualifiers, enforces those ranges at runtime for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @akitaSummer, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the dependency injection system by introducing a 'dynamic range' mechanism. This feature allows developers to explicitly define a whitelist of abstract classes that an Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a DynamicRange decorator to restrict the types that can be dynamically injected using EggObjectFactory. The implementation involves metadata storage for the range and runtime checks during dependency injection. The changes are well-tested with unit and integration tests. My review includes suggestions to improve type safety in the decorator implementation, and to refactor a complex part of the injection logic for better readability and maintainability, for instance by avoiding magic strings.
| return function(target: any, propertyKey?: PropertyKey, parameterIndex?: number) { | ||
| DynamicRangeUtil.addProperQualifier(target as EggProtoImplClass, propertyKey, parameterIndex, dynamics); | ||
| }; |
There was a problem hiding this comment.
To align with the suggested changes in DynamicRangeUtil.addProperQualifier for improved type safety, the decorator implementation should be updated. This involves changing the target type from any to object and passing it directly to addProperQualifier without casting.
| return function(target: any, propertyKey?: PropertyKey, parameterIndex?: number) { | |
| DynamicRangeUtil.addProperQualifier(target as EggProtoImplClass, propertyKey, parameterIndex, dynamics); | |
| }; | |
| return function(target: object, propertyKey?: PropertyKey, parameterIndex?: number) { | |
| DynamicRangeUtil.addProperQualifier(target, propertyKey, parameterIndex, dynamics); | |
| }; |
| static addProperQualifier(clazz: EggProtoImplClass, property: PropertyKey | undefined, parameterIndex: number | undefined, value: EggAbstractClazz | EggAbstractClazz[]) { | ||
| if (typeof parameterIndex === 'number') { | ||
| const argNames = ObjectUtils.getConstructorArgNameList(clazz); | ||
| const argName = argNames[parameterIndex]; | ||
| const properQualifiers = MetadataUtil.initOwnMapMetaData(DYNAMIC_RANGE_META_DATA, clazz, new Map<PropertyKey, EggAbstractClazz | EggAbstractClazz[]>()); | ||
| MapUtil.getOrStore(properQualifiers, argName, value); | ||
| } else { | ||
| const properQualifiers = MetadataUtil.initOwnMapMetaData(DYNAMIC_RANGE_META_DATA, (clazz as any).constructor, new Map<PropertyKey, EggAbstractClazz | EggAbstractClazz[]>()); | ||
| MapUtil.getOrStore(properQualifiers, property, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
The type of clazz is declared as EggProtoImplClass, which is a constructor type. However, when used with a property decorator, this method receives the class's prototype, not the constructor. The code currently handles this with (clazz as any).constructor, but the type signature is misleading and could lead to bugs if this code is refactored.
To improve type safety and clarity, the signature and implementation could be adjusted to correctly reflect the different target types from property and parameter decorators. This change would also require a small adjustment in DynamicRange.ts.
static addProperQualifier(target: object, property: PropertyKey | undefined, parameterIndex: number | undefined, value: EggAbstractClazz | EggAbstractClazz[]) {
if (typeof parameterIndex === 'number') {
// Parameter decorator, target is the constructor
const clazz = target as EggProtoImplClass;
const argNames = ObjectUtils.getConstructorArgNameList(clazz);
const argName = argNames[parameterIndex];
const properQualifiers = MetadataUtil.initOwnMapMetaData(DYNAMIC_RANGE_META_DATA, clazz, new Map<PropertyKey, EggAbstractClazz | EggAbstractClazz[]>());
MapUtil.getOrStore(properQualifiers, argName, value);
} else {
// Property decorator, target is the prototype
const clazz = (target as any).constructor as EggProtoImplClass;
const properQualifiers = MetadataUtil.initOwnMapMetaData(DYNAMIC_RANGE_META_DATA, clazz, new Map<PropertyKey, EggAbstractClazz | EggAbstractClazz[]>());
MapUtil.getOrStore(properQualifiers, property, value);
}
}| if (injectObj.proto.name === 'eggObjectFactory') { | ||
| const dynamics = DynamicRangeUtil.getProperQualifiers(this._obj.constructor as unknown as EggProtoImplClass, injectObject.refName); | ||
| if (dynamics.length > 0) { | ||
| const obj: any = { | ||
| dynamics, | ||
| }; | ||
| const originalGetEggObject = (injectObj.obj as any).getEggObject; | ||
| Object.setPrototypeOf(obj, injectObj.obj); | ||
| obj.getEggObject = function(...args: any[]) { | ||
| if (!this.dynamics.includes(args[0])) { | ||
| throw new Error(`${args[0].name} is not in dynamic range: ${this.dynamics.map(item => item.name).join(', ')}`); | ||
| } | ||
| return originalGetEggObject.apply(this, args); | ||
| }; | ||
| injectObj = Object.create(injectObj); | ||
| Object.defineProperty(injectObj, 'obj', { | ||
| value: obj, | ||
| writable: true, | ||
| enumerable: true, | ||
| configurable: true, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of logic for wrapping eggObjectFactory has a few areas for improvement:
- Hardcoded String:
injectObj.proto.name === 'eggObjectFactory'uses a magic string. It would be more robust to use a shared constant to avoid potential issues if the name changes. - Object Creation: Using
Object.setPrototypeOfcan have performance implications. It's generally better to useObject.create()to establish the prototype chain when creating a new object. - Complexity: The logic is quite dense and nested within the
mapcallback, which harms readability. Consider extracting this logic into a separate private helper method to makeinitWithInjectPropertyeasier to understand and maintain.
![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
🧹 Nitpick comments (6)
core/dynamic-inject/test/typing.test.ts (1)
31-34: Test is synchronous but marked asasync.The test body contains no await expressions, so the
asynckeyword is unnecessary. Consider removing it for clarity.🔎 Suggested fix
- it('should dynamic range run', async () => { + it('should dynamic range run', () => { const dynamics = DynamicRangeUtil.getProperQualifiers(HelloService, 'eggObjectFactory'); assert(dynamics.includes(AbstractContextHello)); });core/dynamic-inject/src/DynamicRange.ts (1)
4-8: Consider adding explicit return type annotations.The decorator factory and its returned function lack explicit type annotations. Adding them would improve type safety and documentation.
🔎 Suggested improvement
-export function DynamicRange(dynamics: EggAbstractClazz | EggAbstractClazz[]) { - return function(target: any, propertyKey?: PropertyKey, parameterIndex?: number) { +export function DynamicRange(dynamics: EggAbstractClazz | EggAbstractClazz[]): PropertyDecorator & ParameterDecorator { + return function(target: any, propertyKey?: PropertyKey, parameterIndex?: number): void { DynamicRangeUtil.addProperQualifier(target as EggProtoImplClass, propertyKey, parameterIndex, dynamics); }; }core/runtime/src/impl/EggObjectImpl.ts (2)
73-78: Consider adding defensive checks forgetEggObjectarguments.The override assumes
args[0]is defined and has a.nameproperty. IfgetEggObjectis called without arguments or with an unexpected type, this could throw an unclear error.🔎 Suggested improvement
obj.getEggObject = function(...args: any[]) { + const clazz = args[0]; + if (!clazz) { + throw new Error('getEggObject requires a class argument'); + } - if (!this.dynamics.includes(args[0])) { - throw new Error(`${args[0].name} is not in dynamic range: ${this.dynamics.map(item => item.name).join(', ')}`); + if (!this.dynamics.includes(clazz)) { + throw new Error(`${clazz.name ?? clazz} is not in dynamic range: ${this.dynamics.map(item => item.name).join(', ')}`); } return originalGetEggObject.apply(this, args); };
64-87: Dynamic range enforcement missing in constructor injection path.The
initWithInjectConstructormethod (lines 113-184) processes injected objects without checking dynamic range restrictions, unlike the property injection path (lines 64-87) which enforces them. The@DynamicRangedecorator architecturally supports both field and constructor parameter injection via the decorator'sparameterIndexparameter, andDynamicRangeUtilstores constructor parameter metadata. For architectural consistency, if constructor injection witheggObjectFactoryshould enforce dynamic ranges, add similar validation ininitWithInjectConstructorwheninjectObject.proto.name === 'eggObjectFactory'.plugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/DynamicRangeService.ts (2)
19-20: Ensure safe access to error message.Accessing
err.messageassumeserris an Error object. Consider using optional chaining or String coercion for robustness.🔎 Safer error handling
} catch (err) { - return [ err.message ]; + return [ err?.message ?? String(err) ]; }
10-12: The @dynamicrange constraint mismatch is intentional and tests error handling.The decorator specifies
AbstractSingletonHello, but line 16 callsgetEggObject(AbstractContextHello, ...), which intentionally violates the dynamic range constraint. The test confirms this is expected behavior: the framework throws "AbstractContextHello is not in dynamic range: AbstractSingletonHello" at runtime. Consider adding a comment like// Test that dynamic range constraint is enforced at runtimeto clarify the intent.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
core/dynamic-inject/index.ts(1 hunks)core/dynamic-inject/src/DynamicRange.ts(1 hunks)core/dynamic-inject/src/DynamicRangeUtil.ts(1 hunks)core/dynamic-inject/test/fixtures/modules/dynamicRange/HelloService.ts(1 hunks)core/dynamic-inject/test/typing.test.ts(2 hunks)core/runtime/src/impl/EggObjectImpl.ts(3 hunks)core/types/core-decorator/enum/Qualifier.ts(1 hunks)plugin/tegg/test/DynamicInject.test.ts(1 hunks)plugin/tegg/test/fixtures/apps/dynamic-inject-app/app/controller/app.ts(1 hunks)plugin/tegg/test/fixtures/apps/dynamic-inject-app/app/router.ts(1 hunks)plugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/DynamicRangeService.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
plugin/tegg/test/DynamicInject.test.ts (1)
benchmark/http/app/router.js (1)
app(4-4)
plugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/DynamicRangeService.ts (2)
core/dynamic-inject/test/fixtures/modules/dynamicRange/HelloService.ts (1)
ContextProto(7-21)core/dynamic-inject/src/DynamicRange.ts (1)
DynamicRange(4-8)
core/dynamic-inject/src/DynamicRangeUtil.ts (4)
core/types/core-decorator/model/EggPrototypeInfo.ts (1)
EggProtoImplClass(5-5)core/types/dynamic-inject.ts (1)
EggAbstractClazz(3-3)core/common-util/src/ObjectUtils.ts (1)
ObjectUtils(3-54)core/types/core-decorator/enum/Qualifier.ts (1)
DYNAMIC_RANGE_META_DATA(13-13)
core/dynamic-inject/src/DynamicRange.ts (3)
core/types/dynamic-inject.ts (1)
EggAbstractClazz(3-3)core/dynamic-inject/src/DynamicRangeUtil.ts (1)
DynamicRangeUtil(6-31)core/types/core-decorator/model/EggPrototypeInfo.ts (1)
EggProtoImplClass(5-5)
core/dynamic-inject/test/fixtures/modules/dynamicRange/HelloService.ts (2)
plugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/DynamicRangeService.ts (1)
ContextProto(6-23)core/dynamic-inject/src/DynamicRange.ts (1)
DynamicRange(4-8)
core/dynamic-inject/test/typing.test.ts (1)
core/dynamic-inject/src/DynamicRangeUtil.ts (1)
DynamicRangeUtil(6-31)
plugin/tegg/test/fixtures/apps/dynamic-inject-app/app/router.ts (1)
benchmark/http/app/router.js (1)
app(4-4)
core/runtime/src/impl/EggObjectImpl.ts (4)
core/runtime/src/factory/EggContainerFactory.ts (1)
EggContainerFactory(18-88)core/dynamic-inject/src/DynamicRangeUtil.ts (1)
DynamicRangeUtil(6-31)core/types/core-decorator/model/EggPrototypeInfo.ts (1)
EggProtoImplClass(5-5)plugin/tegg/lib/EggCompatibleObject.ts (1)
obj(32-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (18)
🔇 Additional comments (8)
plugin/tegg/test/fixtures/apps/dynamic-inject-app/app/router.ts (1)
6-6: LGTM!The new route registration follows the established pattern used for the other dynamic inject routes in this file.
plugin/tegg/test/fixtures/apps/dynamic-inject-app/app/controller/app.ts (1)
19-24: LGTM!The new controller method follows the established pattern. The type annotation
HelloServicefor the resolveddynamicRangeServiceis consistent with howdynamicInjecthandles its service resolution.core/dynamic-inject/index.ts (1)
4-5: LGTM!The new exports for
DynamicRangeandDynamicRangeUtilfollow the existing export pattern and properly expose the public API surface for the dynamic range feature.core/types/core-decorator/enum/Qualifier.ts (1)
13-14: LGTM!The new metadata symbol follows the established naming convention and uses
Symbol.forfor cross-module identity consistency.plugin/tegg/test/DynamicInject.test.ts (1)
52-67: LGTM!The test case effectively validates:
- Dynamic range enforcement by checking the expected error message when requesting a dynamic outside the allowed range
- State continuity by verifying the singleton counter incremented as expected
The test structure is consistent with the existing test cases in this file.
core/dynamic-inject/test/fixtures/modules/dynamicRange/HelloService.ts (2)
9-11: LGTM! Consistent DynamicRange usage.The
@DynamicRange([ AbstractContextHello ])decorator correctly matches theAbstractContextHellotype retrieved in thehello()method, establishing a clear constraint-to-usage pattern.
13-20: LGTM! Efficient concurrent retrieval.Using
Promise.allfor concurrent object retrieval is appropriate. Note that unlikeDynamicRangeService.ts, this test fixture doesn't include error handling, which is acceptable for test code but means failures will propagate.core/dynamic-inject/src/DynamicRangeUtil.ts (1)
20-30: LGTM! Proper null safety and normalization.The method correctly:
- Uses optional chaining to handle undefined metadata
- Returns an empty array when no qualifiers are found
- Normalizes single values to arrays for consistent return type
The method expects
clazzto be the class constructor (not prototype), which aligns with all call sites:EggObjectImpl.tspassesthis._obj.constructorand tests pass the class directly.
| static addProperQualifier(clazz: EggProtoImplClass, property: PropertyKey | undefined, parameterIndex: number | undefined, value: EggAbstractClazz | EggAbstractClazz[]) { | ||
| if (typeof parameterIndex === 'number') { | ||
| const argNames = ObjectUtils.getConstructorArgNameList(clazz); | ||
| const argName = argNames[parameterIndex]; | ||
| const properQualifiers = MetadataUtil.initOwnMapMetaData(DYNAMIC_RANGE_META_DATA, clazz, new Map<PropertyKey, EggAbstractClazz | EggAbstractClazz[]>()); | ||
| MapUtil.getOrStore(properQualifiers, argName, value); |
There was a problem hiding this comment.
Add bounds checking for parameterIndex.
Line 11 accesses argNames[parameterIndex] without verifying that parameterIndex is within bounds. If parameterIndex >= argNames.length, argName will be undefined, which could lead to incorrect metadata storage or silent failures.
🔎 Add validation
static addProperQualifier(clazz: EggProtoImplClass, property: PropertyKey | undefined, parameterIndex: number | undefined, value: EggAbstractClazz | EggAbstractClazz[]) {
if (typeof parameterIndex === 'number') {
const argNames = ObjectUtils.getConstructorArgNameList(clazz);
+ if (parameterIndex >= argNames.length) {
+ throw new Error(`Parameter index ${parameterIndex} is out of bounds for constructor with ${argNames.length} arguments`);
+ }
const argName = argNames[parameterIndex];
const properQualifiers = MetadataUtil.initOwnMapMetaData(DYNAMIC_RANGE_META_DATA, clazz, new Map<PropertyKey, EggAbstractClazz | EggAbstractClazz[]>());
MapUtil.getOrStore(properQualifiers, argName, value);📝 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.
| static addProperQualifier(clazz: EggProtoImplClass, property: PropertyKey | undefined, parameterIndex: number | undefined, value: EggAbstractClazz | EggAbstractClazz[]) { | |
| if (typeof parameterIndex === 'number') { | |
| const argNames = ObjectUtils.getConstructorArgNameList(clazz); | |
| const argName = argNames[parameterIndex]; | |
| const properQualifiers = MetadataUtil.initOwnMapMetaData(DYNAMIC_RANGE_META_DATA, clazz, new Map<PropertyKey, EggAbstractClazz | EggAbstractClazz[]>()); | |
| MapUtil.getOrStore(properQualifiers, argName, value); | |
| static addProperQualifier(clazz: EggProtoImplClass, property: PropertyKey | undefined, parameterIndex: number | undefined, value: EggAbstractClazz | EggAbstractClazz[]) { | |
| if (typeof parameterIndex === 'number') { | |
| const argNames = ObjectUtils.getConstructorArgNameList(clazz); | |
| if (parameterIndex >= argNames.length) { | |
| throw new Error(`Parameter index ${parameterIndex} is out of bounds for constructor with ${argNames.length} arguments`); | |
| } | |
| const argName = argNames[parameterIndex]; | |
| const properQualifiers = MetadataUtil.initOwnMapMetaData(DYNAMIC_RANGE_META_DATA, clazz, new Map<PropertyKey, EggAbstractClazz | EggAbstractClazz[]>()); | |
| MapUtil.getOrStore(properQualifiers, argName, value); |
🤖 Prompt for AI Agents
In core/dynamic-inject/src/DynamicRangeUtil.ts around lines 8 to 13,
parameterIndex is used to index argNames without bounds checking which can yield
undefined argName; add a guard that validates parameterIndex is a number within
0 <= parameterIndex < argNames.length before using it, and if it is out of range
either throw a clear RangeError (including clazz.name and the invalid index) or
return early (no-op) depending on expected behavior; ensure argName is only
read/used after this validation so metadata is not stored under an undefined
key.
| } else { | ||
| const properQualifiers = MetadataUtil.initOwnMapMetaData(DYNAMIC_RANGE_META_DATA, (clazz as any).constructor, new Map<PropertyKey, EggAbstractClazz | EggAbstractClazz[]>()); | ||
| MapUtil.getOrStore(properQualifiers, property, value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clarify type handling and validate property parameter.
Two concerns:
-
Type inconsistency: The parameter is typed as
clazz: EggProtoImplClass(a class constructor), but whenparameterIndexisundefined(property decorator case),clazzis actually the prototype, and line 15 uses(clazz as any).constructorto get the class. This makes the API unclear. Consider either:- Typing the parameter as
target: anyto reflect the actual decorator contract - Normalizing the input at the function start to always work with the class
- Adding a JSDoc comment explaining the dual nature
- Typing the parameter as
-
Undefined property: When
parameterIndexisundefined,propertycould also beundefined(per its typePropertyKey | undefined), but line 16 uses it directly as a Map key without validation.
🔎 Add property validation
} else {
+ if (property === undefined) {
+ throw new Error('Property key is required when parameterIndex is undefined');
+ }
const properQualifiers = MetadataUtil.initOwnMapMetaData(DYNAMIC_RANGE_META_DATA, (clazz as any).constructor, new Map<PropertyKey, EggAbstractClazz | EggAbstractClazz[]>());
MapUtil.getOrStore(properQualifiers, property, value);
}📝 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.
| } else { | |
| const properQualifiers = MetadataUtil.initOwnMapMetaData(DYNAMIC_RANGE_META_DATA, (clazz as any).constructor, new Map<PropertyKey, EggAbstractClazz | EggAbstractClazz[]>()); | |
| MapUtil.getOrStore(properQualifiers, property, value); | |
| } | |
| } else { | |
| if (property === undefined) { | |
| throw new Error('Property key is required when parameterIndex is undefined'); | |
| } | |
| const properQualifiers = MetadataUtil.initOwnMapMetaData(DYNAMIC_RANGE_META_DATA, (clazz as any).constructor, new Map<PropertyKey, EggAbstractClazz | EggAbstractClazz[]>()); | |
| MapUtil.getOrStore(properQualifiers, property, value); | |
| } |
🤖 Prompt for AI Agents
In core/dynamic-inject/src/DynamicRangeUtil.ts around lines 14 to 17, the
function treats its first argument as both a class constructor and a prototype
and uses property as a Map key without checking for undefined; normalize the
target at the top (e.g., detect if the passed value is a prototype and replace
it with its constructor) or change the parameter name/type to reflect it accepts
either prototype or constructor and add a brief JSDoc explaining the dual
nature, then validate that property is not undefined when parameterIndex is
undefined (throw or handle the error) before calling
MetadataUtil.initOwnMapMetaData/MapUtil.getOrStore so you never use an undefined
key.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
core/runtime/src/impl/EggObjectImpl.ts (1)
65-87: Refactor to address complexity, hardcoded string, and performance concerns.This block has several areas for improvement that align with previous review feedback:
- Hardcoded string:
injectObj.proto.name === 'eggObjectFactory'should use a shared constant- Performance:
Object.setPrototypeOf(line 72) can have performance implications; preferObject.create()for establishing prototype chains- Complexity: The nested logic within the
mapcallback harms readability; extract into a separate helper methodBased on previous review feedback at lines 65-87.
core/dynamic-inject/src/FactoryQualifierUtil.ts (2)
8-13: Add bounds checking for parameterIndex.Line 11 accesses
argNames[parameterIndex]without verifying thatparameterIndexis within bounds. IfparameterIndex >= argNames.length,argNamewill beundefined, leading to incorrect metadata storage or silent failures.This issue was previously identified in past review comments.
🔎 Add validation
static addProperQualifier(clazz: EggProtoImplClass, property: PropertyKey | undefined, parameterIndex: number | undefined, value: EggAbstractClazz | EggAbstractClazz[]) { if (typeof parameterIndex === 'number') { const argNames = ObjectUtils.getConstructorArgNameList(clazz); + if (parameterIndex < 0 || parameterIndex >= argNames.length) { + throw new Error(`Parameter index ${parameterIndex} is out of bounds for constructor with ${argNames.length} arguments`); + } const argName = argNames[parameterIndex]; const properQualifiers = MetadataUtil.initOwnMapMetaData(DYNAMIC_RANGE_META_DATA, clazz, new Map<PropertyKey, EggAbstractClazz | EggAbstractClazz[]>()); MapUtil.getOrStore(properQualifiers, argName, value);
14-17: Clarify type handling and validate property parameter.Two concerns previously identified:
Type inconsistency: The parameter is typed as
clazz: EggProtoImplClass(a class constructor), but in the property decorator case (line 15),clazzis actually the prototype, requiring(clazz as any).constructor. This makes the API unclear.Undefined property: When
parameterIndexisundefined,propertycould also beundefined, but line 16 uses it directly as a Map key without validation.These issues were previously identified in past review comments.
🔎 Add property validation
} else { + if (property === undefined) { + throw new Error('Property key is required when parameterIndex is undefined'); + } const properQualifiers = MetadataUtil.initOwnMapMetaData(DYNAMIC_RANGE_META_DATA, (clazz as any).constructor, new Map<PropertyKey, EggAbstractClazz | EggAbstractClazz[]>()); MapUtil.getOrStore(properQualifiers, property, value); }
🧹 Nitpick comments (3)
core/dynamic-inject/package.json (1)
49-49: Remove unusedcross-envdependency.The
cross-envpackage is no longer used after simplifying the test script but remains in devDependencies. Consider removing it to keep dependencies clean.🔎 Proposed fix
"coffee": "^5.4.0", - "cross-env": "^7.0.3", "mocha": "^10.2.0",plugin/tegg/package.json (2)
65-65: Consider removing unusedcross-envdependency.Since the test script no longer uses
cross-env, this dependency can be removed to keep the dependency list clean.🔎 Proposed fix
- "cross-env": "^7.0.3",
29-29: Remove unusedcross-envdependency from devDependencies.The test script has been simplified to just
mocha, which no longer requires cross-env. Since cross-env is not used anywhere else in the codebase, it should be removed from devDependencies at line 65 as part of this cleanup.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
core/dynamic-inject/index.ts(1 hunks)core/dynamic-inject/package.json(1 hunks)core/dynamic-inject/src/FactoryQualifier.ts(1 hunks)core/dynamic-inject/src/FactoryQualifierUtil.ts(1 hunks)core/dynamic-inject/test/fixtures/modules/dynamicRange/HelloService.ts(1 hunks)core/dynamic-inject/test/typing.test.ts(2 hunks)core/runtime/src/impl/EggObjectImpl.ts(3 hunks)core/types/core-decorator/enum/Qualifier.ts(1 hunks)plugin/tegg/package.json(1 hunks)plugin/tegg/test/DynamicInject.test.ts(1 hunks)plugin/tegg/test/fixtures/apps/dynamic-inject-app/app/controller/app.ts(1 hunks)plugin/tegg/test/fixtures/apps/dynamic-inject-app/app/router.ts(1 hunks)plugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/FactoryQualifierService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- core/dynamic-inject/index.ts
- core/dynamic-inject/test/fixtures/modules/dynamicRange/HelloService.ts
- core/types/core-decorator/enum/Qualifier.ts
- plugin/tegg/test/DynamicInject.test.ts
- plugin/tegg/test/fixtures/apps/dynamic-inject-app/app/controller/app.ts
- core/dynamic-inject/test/typing.test.ts
🧰 Additional context used
🧬 Code graph analysis (5)
core/dynamic-inject/src/FactoryQualifier.ts (3)
core/types/dynamic-inject.ts (1)
EggAbstractClazz(3-3)core/dynamic-inject/src/FactoryQualifierUtil.ts (1)
FactoryQualifierUtil(6-31)core/types/core-decorator/model/EggPrototypeInfo.ts (1)
EggProtoImplClass(5-5)
core/runtime/src/impl/EggObjectImpl.ts (3)
core/runtime/src/factory/EggContainerFactory.ts (1)
EggContainerFactory(18-88)core/dynamic-inject/src/FactoryQualifierUtil.ts (1)
FactoryQualifierUtil(6-31)core/types/core-decorator/model/EggPrototypeInfo.ts (1)
EggProtoImplClass(5-5)
plugin/tegg/test/fixtures/apps/dynamic-inject-app/app/router.ts (1)
benchmark/http/app/router.js (1)
app(4-4)
core/dynamic-inject/src/FactoryQualifierUtil.ts (5)
core/types/core-decorator/model/EggPrototypeInfo.ts (1)
EggProtoImplClass(5-5)core/types/dynamic-inject.ts (1)
EggAbstractClazz(3-3)core/common-util/src/ObjectUtils.ts (1)
ObjectUtils(3-54)core/core-decorator/src/util/MetadataUtil.ts (1)
MetadataUtil(5-97)core/types/core-decorator/enum/Qualifier.ts (1)
DYNAMIC_RANGE_META_DATA(13-13)
plugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/FactoryQualifierService.ts (2)
core/dynamic-inject/test/fixtures/modules/dynamicRange/HelloService.ts (1)
ContextProto(7-21)core/dynamic-inject/src/FactoryQualifier.ts (1)
FactoryQualifier(4-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (18)
🔇 Additional comments (5)
plugin/tegg/test/fixtures/apps/dynamic-inject-app/app/router.ts (1)
6-6: LGTM!The new route registration follows the established pattern and correctly maps the endpoint to the controller method.
core/runtime/src/impl/EggObjectImpl.ts (1)
7-7: LGTM!The new imports are necessary for the dynamic range functionality and are properly typed.
Also applies to: 16-16
core/dynamic-inject/src/FactoryQualifier.ts (1)
4-8: LGTM!The decorator implementation is clean and follows the standard TypeScript decorator pattern by delegating to the utility class.
core/dynamic-inject/src/FactoryQualifierUtil.ts (1)
20-30: LGTM!The
getProperQualifiersmethod correctly retrieves and normalizes dynamic qualifiers, handling both single values and arrays, with appropriate fallback to an empty array.plugin/tegg/test/fixtures/apps/dynamic-inject-app/modules/dynamic-inject-module/FactoryQualifierService.ts (1)
10-16: The qualifier mismatch is intentional for testing dynamic range validation errors.The
@FactoryQualifier(AbstractSingletonHello)decorator on line 11 intentionally restricts the factory to a different qualifier than theAbstractContextHellorequested on line 16. The error handling at lines 19-20 captures this expected mismatch and returns the error message. The test in DynamicInject.test.ts confirms this is a negative test case, expecting the error: "AbstractContextHello is not in dynamic range: AbstractSingletonHello".
core/dynamic-inject/package.json
Outdated
| ], | ||
| "scripts": { | ||
| "test": "cross-env NODE_ENV=test NODE_OPTIONS='--no-deprecation' mocha", | ||
| "test": "mocha", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check package.json for test script and dependencies
echo "=== Checking core/dynamic-inject/package.json ==="
cat core/dynamic-inject/package.json | jq '.scripts.test, .devDependencies | has("cross-env")'
# Look for mocha config files in core/dynamic-inject
echo -e "\n=== Checking for mocha configuration files ==="
fd -e json -e js -e yml -e yaml 'mocharc|mocha.opts' core/dynamic-inject 2>/dev/null || echo "No mocha config files found"
# Check if mocha config exists in package.json
echo -e "\n=== Checking for mocha property in package.json ==="
cat core/dynamic-inject/package.json | jq '.mocha' 2>/dev/null || echo "No mocha property in package.json"
# Show test script and relevant dependencies
echo -e "\n=== Full test script and devDependencies ==="
cat core/dynamic-inject/package.json | jq '.scripts.test, .devDependencies'Repository: eggjs/tegg
Length of output: 531
🏁 Script executed:
#!/bin/bash
cd core/dynamic-inject || exit 1
echo "=== Test script ==="
cat package.json | jq -r '.scripts.test'
echo -e "\n=== cross-env in devDependencies? ==="
cat package.json | jq 'has("devDependencies") and .devDependencies | has("cross-env")'
echo -e "\n=== Mocha config files ==="
ls -la .mocharc* 2>/dev/null || echo "No .mocharc files found"
ls -la mocha.opts 2>/dev/null || echo "No mocha.opts found"
echo -e "\n=== Mocha property in package.json ==="
cat package.json | jq '.mocha // "No mocha property"'Repository: eggjs/tegg
Length of output: 340
Simplification of test script removes critical environment configuration without proper migration.
The test script has been simplified from cross-env NODE_ENV=test NODE_OPTIONS='--no-deprecation' mocha to just mocha. This removes explicit environment variable configuration:
NODE_ENV=test: Required for tests that check the environment modeNODE_OPTIONS='--no-deprecation': Suppresses Node.js deprecation warnings
No mocha configuration file (.mocharc.json, .mocharc.js, etc.) or mocha property in package.json was created to preserve these settings. Additionally, cross-env remains in devDependencies but is now unused.
Verify that tests pass without these settings, or configure them via a mocha config file or the package.json test script. Remove the unused cross-env dependency.
🤖 Prompt for AI Agents
In core/dynamic-inject/package.json around line 18, the test script was
simplified to "mocha" which removed required environment setup (NODE_ENV=test
and NODE_OPTIONS='--no-deprecation') and left cross-env unused; restore the
environment configuration by either: 1) reverting the script to use cross-env
(e.g. cross-env NODE_ENV=test NODE_OPTIONS='--no-deprecation' mocha), or 2)
create a Mocha config file (e.g. .mocharc.json/.mocharc.js) and set NODE_OPTIONS
via an npm script that exports NODE_ENV=test (or use cross-env only for NODE_ENV
while moving NODE_OPTIONS into the Mocha config), then remove cross-env from
devDependencies if you choose the config-file approach and verify all tests pass
locally and in CI before committing.
| obj.getEggObject = function(...args: any[]) { | ||
| if (!this.dynamics.includes(args[0])) { | ||
| throw new Error(`${args[0].name} is not in dynamic range: ${this.dynamics.map(item => item.name).join(', ')}`); | ||
| } | ||
| return originalGetEggObject.apply(this, args); |
There was a problem hiding this comment.
Add validation and fix reference equality check.
Two issues:
-
Missing argument validation: Line 73 accesses
args[0]without checking if arguments were provided, which could cause runtime errors. -
Reference equality issue: Line 74 uses
this.dynamics.includes(args[0])which performs reference equality checks. If the abstract class passed at runtime is a different reference to the same class (e.g., re-imported or loaded from different module context), the check will fail even though it logically represents the same type.
🔎 Proposed fix
obj.getEggObject = function(...args: any[]) {
+ if (args.length === 0 || !args[0]) {
+ throw new Error('Dynamic class argument is required');
+ }
- if (!this.dynamics.includes(args[0])) {
+ const requestedDynamic = args[0];
+ const isAllowed = this.dynamics.some((allowedDynamic: any) =>
+ allowedDynamic === requestedDynamic ||
+ allowedDynamic.name === requestedDynamic.name
+ );
+ if (!isAllowed) {
throw new Error(`${args[0].name} is not in dynamic range: ${this.dynamics.map(item => item.name).join(', ')}`);
}
return originalGetEggObject.apply(this, args);
};🤖 Prompt for AI Agents
In core/runtime/src/impl/EggObjectImpl.ts around lines 73 to 77, add argument
validation and replace the strict reference equality check: first ensure args[0]
exists and is a valid constructor/type (throw a clear error if missing or
invalid), then determine membership in this.dynamics by comparing logical
identity rather than reference — e.g., search this.dynamics for an entry that
either is the same reference or has the same name (or other stable identifier)
as args[0]; if no match is found throw the existing error message using the
provided argument name, and only then call originalGetEggObject.apply(this,
args).
akitaSummer
force-pushed
the
feat/add_dynamic_range
branch
from
9040cfe to
8ba4dcf
Compare
| } else { | ||
| const injectObj = await EggContainerFactory.getOrCreateEggObject(proto, injectObject.objName); | ||
| let injectObj = await EggContainerFactory.getOrCreateEggObject(proto, injectObject.objName); | ||
| if (injectObj.proto.name === 'eggObjectFactory') { |
There was a problem hiding this comment.
这个逻辑应该写在 eggObjectFactory 内部，不应该写在 EggObjectImpl。
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.