-
Notifications
You must be signed in to change notification settings - Fork 44
feat: add FactoryQualifier #379
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| export * from '@eggjs/tegg-types/dynamic-inject'; | ||
| export * from './src/QualifierImplUtil'; | ||
| export * from './src/QualifierImplDecoratorUtil'; | ||
| export * from './src/FactoryQualifier'; | ||
| export * from './src/FactoryQualifierUtil'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| import type { EggAbstractClazz, EggProtoImplClass } from '@eggjs/tegg-types'; | ||
| import { FactoryQualifierUtil } from './FactoryQualifierUtil'; | ||
|
|
||
| export function FactoryQualifier(dynamics: EggAbstractClazz | EggAbstractClazz[]) { | ||
| return function(target: any, propertyKey?: PropertyKey, parameterIndex?: number) { | ||
| FactoryQualifierUtil.addProperQualifier(target as EggProtoImplClass, propertyKey, parameterIndex, dynamics); | ||
| }; | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,31 @@ | ||||||||||||||||||||||||
| import { MetadataUtil } from '@eggjs/core-decorator'; | ||||||||||||||||||||||||
| import { MapUtil, ObjectUtils } from '@eggjs/tegg-common-util'; | ||||||||||||||||||||||||
| import { DYNAMIC_RANGE_META_DATA } from '@eggjs/tegg-types'; | ||||||||||||||||||||||||
| import type { EggAbstractClazz, EggProtoImplClass } from '@eggjs/tegg-types'; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| export class FactoryQualifierUtil { | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| 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); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+14
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Clarify type handling and validate property parameter. Two concerns:
🔎 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+8
to
+18
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The type of To improve type safety and clarity, the signature and implementation could be adjusted to correctly reflect the different 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);
}
} |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| static getProperQualifiers(clazz: EggProtoImplClass, property: PropertyKey): EggAbstractClazz[] { | ||||||||||||||||||||||||
| const properQualifiers: Map<PropertyKey, EggAbstractClazz | EggAbstractClazz[]> | undefined = MetadataUtil.getMetaData(DYNAMIC_RANGE_META_DATA, clazz); | ||||||||||||||||||||||||
| const dynamics = properQualifiers?.get(property); | ||||||||||||||||||||||||
| if (!dynamics) { | ||||||||||||||||||||||||
| return []; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| if (Array.isArray(dynamics)) { | ||||||||||||||||||||||||
| return dynamics; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| return [ dynamics ]; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| import { ContextProto, Inject } from '@eggjs/core-decorator'; | ||
| import { EggObjectFactory } from '@eggjs/tegg-dynamic-inject'; | ||
| import { FactoryQualifier } from '../../../../src/FactoryQualifier'; | ||
| import { AbstractContextHello } from '../base/AbstractContextHello'; | ||
| import { ContextHelloType } from '../base/FooType'; | ||
|
|
||
| @ContextProto() | ||
| export class HelloService { | ||
| @Inject() | ||
| @FactoryQualifier([ AbstractContextHello ]) | ||
| private readonly eggObjectFactory: EggObjectFactory; | ||
|
|
||
| async hello(): Promise<string[]> { | ||
| const helloImpls = await Promise.all([ | ||
| this.eggObjectFactory.getEggObject(AbstractContextHello, ContextHelloType.FOO), | ||
| this.eggObjectFactory.getEggObject(AbstractContextHello, ContextHelloType.BAR), | ||
| ]); | ||
| const msgs = helloImpls.map(helloImpl => helloImpl.hello()); | ||
| return msgs; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import type { | |
| EggObjectLifecycle, | ||
| EggObjectLifeCycleContext, | ||
| EggObjectName, | ||
| EggProtoImplClass, | ||
| EggPrototype, ObjectInfo, QualifierInfo, | ||
| } from '@eggjs/tegg-types'; | ||
| import { EggObjectStatus, InjectType, ObjectInitType } from '@eggjs/tegg-types'; | ||
|
|
@@ -12,6 +13,7 @@ import { EggObjectLifecycleUtil } from '../model/EggObject'; | |
| import { EggContainerFactory } from '../factory/EggContainerFactory'; | ||
| import { EggObjectUtil } from './EggObjectUtil'; | ||
| import { ContextHandler } from '../model/ContextHandler'; | ||
| import { FactoryQualifierUtil } from '@eggjs/tegg-dynamic-inject'; | ||
|
|
||
| export default class EggObjectImpl implements EggObject { | ||
| private _obj: object; | ||
|
|
@@ -59,7 +61,30 @@ export default class EggObjectImpl implements EggObject { | |
| if (this.proto.initType !== ObjectInitType.CONTEXT && injectObject.proto.initType === ObjectInitType.CONTEXT) { | ||
| this.injectProperty(injectObject.refName, EggObjectUtil.contextEggObjectGetProperty(proto, injectObject.objName)); | ||
| } else { | ||
| const injectObj = await EggContainerFactory.getOrCreateEggObject(proto, injectObject.objName); | ||
| let injectObj = await EggContainerFactory.getOrCreateEggObject(proto, injectObject.objName); | ||
| if (injectObj.proto.name === 'eggObjectFactory') { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 这个逻辑应该写在 eggObjectFactory 内部,不应该写在 EggObjectImpl。 |
||
| const dynamics = FactoryQualifierUtil.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); | ||
|
Comment on lines
+73
to
+77
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add validation and fix reference equality check. Two issues:
🔎 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 |
||
| }; | ||
| injectObj = Object.create(injectObj); | ||
| Object.defineProperty(injectObj, 'obj', { | ||
| value: obj, | ||
| writable: true, | ||
| enumerable: true, | ||
| configurable: true, | ||
| }); | ||
| } | ||
| } | ||
| this.injectProperty(injectObject.refName, EggObjectUtil.eggObjectGetProperty(injectObj)); | ||
| } | ||
| })); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import { AccessLevel, Inject, EggObjectFactory, ContextProto, FactoryQualifier } from '@eggjs/tegg'; | ||
| import { ContextHelloType } from './FooType'; | ||
| import { AbstractSingletonHello } from './AbstractSingletonHello'; | ||
| import { AbstractContextHello } from './AbstractContextHello'; | ||
|
|
||
| @ContextProto({ | ||
| accessLevel: AccessLevel.PUBLIC, | ||
| }) | ||
| export class FactoryQualifierService { | ||
| @Inject() | ||
| @FactoryQualifier(AbstractSingletonHello) | ||
| private readonly eggObjectFactory: EggObjectFactory; | ||
|
|
||
| async hello(): Promise<string[]> { | ||
| try { | ||
| const helloImpl = await this.eggObjectFactory.getEggObject(AbstractContextHello, ContextHelloType.FOO); | ||
| const msgs = [ helloImpl.hello() ]; | ||
| return msgs; | ||
| } catch (err) { | ||
| return [ err.message ]; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add bounds checking for parameterIndex.
Line 11 accesses
argNames[parameterIndex]without verifying thatparameterIndexis within bounds. IfparameterIndex >= argNames.length,argNamewill beundefined, 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
🤖 Prompt for AI Agents