fix: fix miss MultiInstance proper qualifiers #241
Conversation
WalkthroughThe changes encompass modifications across several files, primarily focusing on the handling of qualifiers within the prototype management system. New attributes for qualifiers are introduced, adjustments are made to class constructors and methods to accommodate these qualifiers, and the structure of certain classes is simplified by removing unnecessary components. Additionally, configuration files are updated to include new properties, enhancing the overall functionality and clarity of the codebase. Changes
Possibly related PRs
Poem
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 (
|
|
AppGraph/ModuleGraph/EggPrototypeFactory 有重复逻辑，修改起来很困难，需要重构。
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (8)
core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app/module.yml (1)
Line range hint
1-11: Summary of review and final recommendationsThe changes to this configuration file introduce client-specific secrets and a corresponding list of valid secret keys. While the YAML structure is correct, there are significant security concerns that need to be addressed:
- Avoid storing secrets in plain text in configuration files.
- Use more complex, randomly generated values for secrets instead of simple sequential numbers.
- Consider using environment variables or a secure secret management system for handling sensitive information.
- Clarify the purpose of the
secret.keyssection and evaluate if it's necessary or if there's a more secure alternative.Before finalizing these changes:
- Review and update any code that interacts with these client secrets to ensure compatibility with the new structure.
- Document the purpose and usage of both the client secrets and the
secret.keyssection.- Implement proper secret management practices suitable for your development and production environments.
- Consider adding tests to verify that the secret handling behaves as expected without exposing the actual secret values.
As this change introduces new security-sensitive elements, it would be beneficial to conduct a security review of the entire secret handling process in your application. This might involve:
- Auditing how secrets are used throughout the application.
- Implementing encryption for secrets at rest and in transit.
- Setting up proper access controls for who can view or modify these secrets.
- Establishing a process for rotating secrets regularly.
These steps will help ensure that your application handles sensitive information securely and follows best practices for secret management.
core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/foo/Secret.ts (1)
Line range hint
42-44: Consider implementing a more robust secret retrieval mechanism.The current
getSecretmethod appears to be a placeholder implementation. For a production-ready secret management system, consider implementing a more secure and flexible mechanism.Here's a suggested improvement:
export class Secret { private secrets: Map<string, string>; constructor(config: any) { this.secrets = new Map(config.secret.keys.map(key => [key, config.secret[key]])); } getSecret(key: string): string { const secret = this.secrets.get(key); if (!secret) { throw new Error(`Secret not found for key: ${key}`); } return secret; } }This implementation:
- Stores secrets in a private Map for efficient lookup.
- Throws an error if a requested secret is not found.
- Allows for easy extension to integrate with external secret management systems in the future.
Remember to update the
MultiInstanceProtodecorator to pass the configuration to the constructor if you implement this change.plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/bar/BizManager.ts (2)
Line range hint
28-31: Address TODO and improve error handling ingetObjectsmethod.
The comment suggests that configuration is dynamically loaded from
module.yml. Consider implementing this TODO to improve code completeness.Error handling in the
getObjectsmethod could be enhanced. Currently, it returns an empty array ifclientsis falsy, which might mask configuration errors.Consider refactoring the
getObjectsmethod for better error handling:getObjects(ctx: MultiInstancePrototypeGetObjectsContext) { const config = ModuleConfigUtil.loadModuleConfigSync(ctx.unitPath) as any; const name = ModuleConfigUtil.readModuleNameSync(ctx.unitPath); const clients = config?.BizManager?.clients; if (!clients || typeof clients !== 'object') { console.warn(`No valid clients configuration found for BizManager in module ${name}`); return []; } return Object.entries(clients).map(([clientName, clientConfig]) => ({ name: BizManagerInjectName, qualifiers: [{ attribute: BizManagerQualifierAttribute, value: clientName, }], properQualifiers: { secret: [{ attribute: SecretQualifierAttribute, value: name, }], }, })); }This refactoring adds a type check for
clientsand logs a warning if the configuration is missing or invalid.
Line range hint
52-62: Enhance type safety inBizManagerclass.The
BizManagerclass could benefit from improved type safety.Consider the following enhancements:
- Define an interface for the object info:
interface BizManagerObjectInfo extends ObjectInfo { qualifiers: Array<{ attribute: symbol; value: string; }>; }
- Use this interface in the constructor:
constructor( @Inject() secret: Secret, @MultiInstanceInfo([ BizManagerQualifierAttribute ]) objInfo: BizManagerObjectInfo, ) { const qualifier = objInfo.qualifiers.find(t => t.attribute === BizManagerQualifierAttribute); if (!qualifier) { throw new Error('BizManager qualifier not found'); } this.name = qualifier.value; this.secret = secret.getSecret(this.name); }These changes will improve type checking and make the code more robust against potential runtime errors.
core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/bar/BizManager.ts (2)
44-48: LGTM: Additional qualifier enhances object context.The addition of the LoadUnitNameQualifierAttribute provides valuable context to the object creation. This is a good improvement for more precise object management and potential debugging.
Consider adding a comment explaining the purpose of this new qualifier for better code readability. For example:
{ attribute: LoadUnitNameQualifierAttribute, value: name, // Module name to provide additional context for the BizManager instance },
Line range hint
1-65: Great improvements! Consider adding tests for the new qualifier behavior.The changes in this file enhance the specificity and context of the BizManager, which is a positive improvement. The code maintains good TypeScript practices and effectively uses advanced features.
To ensure the robustness of these changes, it would be beneficial to add or update tests that cover the new qualifier behavior, especially the LoadUnitNameQualifierAttribute usage. Would you like me to help draft some test cases or open a GitHub issue to track this task?
core/metadata/src/impl/EggPrototypeBuilder.ts (2)
60-60: Remove debugging console.log statementThe
console.logstatement on line 60 appears to be for debugging purposes. It's recommended to remove it or replace it with an appropriate logging mechanism to avoid unintended output and potential performance issues in production code.
70-70: Remove debugging console.log statementThe
console.logstatement on line 70 seems to be a leftover from debugging. Consider removing it or using a proper logging framework to handle logging at appropriate levels (e.g., debug, info).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- core/core-decorator/src/util/PrototypeUtil.ts (2 hunks)
- core/metadata/src/factory/EggPrototypeCreatorFactory.ts (2 hunks)
- core/metadata/src/impl/EggPrototypeBuilder.ts (2 hunks)
- core/metadata/src/impl/ModuleLoadUnit.ts (4 hunks)
- core/metadata/src/model/AppGraph.ts (2 hunks)
- core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app/module.yml (1 hunks)
- core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app2/App.ts (1 hunks)
- core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/bar/BizManager.ts (2 hunks)
- core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/foo/Secret.ts (1 hunks)
- core/types/core-decorator/model/EggPrototypeInfo.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/bar/BizManager.ts (1 hunks)
🔇 Additional comments (20)
core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app/module.yml (2)
Line range hint
8-11: Review the purpose and security of thesecretsectionThe
secretsection contains keys that match the client secrets. This raises a few points for consideration:
- The purpose of this separate
secretsection is not clear from the configuration alone. It might be used for validation or as a master list of valid secrets.- Storing these values in plain text, especially if they correspond to the client secrets, presents a security risk.
Consider the following recommendations:
- Document the purpose of this
secretsection in a comment to improve clarity.- If these keys are meant to be a list of valid secrets, consider using a more secure validation method that doesn't involve storing the secrets themselves.
- If this section is necessary, use environment variables or a secure secret management system instead of hardcoding the values.
Example with environment variables and added documentation:
# List of valid client secret keys for validation purposes secret: keys: - ${VALID_SECRET_1} - ${VALID_SECRET_2}To better understand the usage of this
secretsection, please run the following script:#!/bin/bash # Description: Check for usages of the secret.keys in the codebase # Test: Search for usages of secret.keys echo "Searching for usages of secret.keys:" rg --type-add 'code:*.{js,ts,jsx,tsx}' --type code 'secret\.keys' # Test: Check for any code that might be validating secrets echo "Checking for potential secret validation code:" rg --type-add 'code:*.{js,ts,jsx,tsx}' --type code 'validate.*secret|secret.*validate'This script will help identify how the
secret.keysare being used in the codebase, which can inform decisions about how to handle this configuration more securely.
3-6:⚠️ Potential issueLGTM for YAML structure, but consider security implications
The changes to add
secretvalues for each client are structurally correct and maintain consistent YAML formatting. However, there are some important considerations:
- Storing secrets in plain text in a configuration file is generally not a secure practice, especially if this file is committed to version control.
- Using simple sequential values ('1' and '2') as secrets is not secure for production environments.
Consider the following recommendations:
- Use environment variables or a secure secret management system to handle sensitive information.
- If secrets must be stored in the file for testing purposes, use more complex, randomly generated values.
- Ensure this file is not committed to version control if it contains actual secrets.
Example of using environment variables:
BizManager: clients: foo: secret: ${FOO_CLIENT_SECRET} bar: secret: ${BAR_CLIENT_SECRET}To ensure these changes don't introduce breaking changes, please run the following script:
This script will help identify any places in the codebase that might be affected by these changes, allowing you to update them accordingly.
core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app2/App.ts (1)
7-8: Clarification needed on multiple qualifiers and SecretQualifier argument changeThe addition of
@ModuleQualifier('app2')aligns with the PR objective of fixing proper qualifiers for MultiInstance. However, I have a few questions:
- Can you explain the rationale behind using both
@ModuleQualifierand@SecretQualifieron the same property?- Why was the argument for
@SecretQualifierchanged from 'app2' to '1'?- How do these changes improve the handling of MultiInstance qualifiers?
To ensure consistency across the codebase, let's check for similar usage patterns:
✅ Verification successful
Verified: Proper use of multiple qualifiers and consistent SecretQualifier argument
The usage of both
@ModuleQualifier('app2')and@SecretQualifier('1')aligns with existing patterns in the codebase, ensuring consistency and adherence to the PR objectives for fixing MultiInstance qualifiers.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage of ModuleQualifier and SecretQualifier decorators echo "Searching for ModuleQualifier usage:" rg --type typescript '@ModuleQualifier' -C 3 echo "\nSearching for SecretQualifier usage:" rg --type typescript '@SecretQualifier' -C 3 echo "\nSearching for combined usage of ModuleQualifier and SecretQualifier:" rg --type typescript -e '@ModuleQualifier' -e '@SecretQualifier' --multilineLength of output: 590
Script:
#!/bin/bash # Description: Search for usage of ModuleQualifier and SecretQualifier decorators in TypeScript files echo "Searching for ModuleQualifier usage in .ts and .tsx files:" rg '@ModuleQualifier' --glob '*.ts' --glob '*.tsx' -C 3 echo "\nSearching for SecretQualifier usage in .ts and .tsx files:" rg '@SecretQualifier' --glob '*.ts' --glob '*.tsx' -C 3 echo "\nSearching for combined usage of ModuleQualifier and SecretQualifier in the same file:" rg '@ModuleQualifier' --glob '*.ts' --glob '*.tsx' -l | xargs rg '@SecretQualifier' -C 3Length of output: 5599
core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/foo/Secret.ts (1)
28-36: LGTM! Improved flexibility for handling multiple secrets.The changes in the
getObjectsmethod enhance the functionality by allowing multiple secrets to be defined and used. This aligns well with the MultiInstance pattern and provides more flexibility in secret management.To ensure this change doesn't introduce any unexpected behavior, please verify the following:
- Check if any existing code relies on the previous single-secret implementation.
- Update any documentation or usage examples to reflect this new multi-secret capability.
- Consider adding a test case that verifies the behavior with multiple secrets defined in the configuration.
Run the following script to find potential usage of the
Secretclass:This will help identify areas of the codebase that might need updates to accommodate the new multi-secret functionality.
plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/bar/BizManager.ts (1)
14-14: LGTM! Verify usage in other files.The change from 'Qualifier.ChatModel' to 'Qualifier.BizManager' improves code clarity by aligning the symbol name with the entity it represents. This is a good improvement.
To ensure this change doesn't break anything elsewhere in the codebase, please run the following script:
This will help identify any places where the old symbol might still be in use or where the new symbol should be used consistently.
✅ Verification successful
Verification Successful. No remaining references to
Qualifier.ChatModelwere found. TheQualifier.BizManagersymbol is used consistently in the expected files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to 'Qualifier.ChatModel' and verify 'Qualifier.BizManager' usage # Search for any remaining 'Qualifier.ChatModel' references echo "Searching for 'Qualifier.ChatModel' references:" rg --type typescript --type javascript "Qualifier\.ChatModel" # Search for 'Qualifier.BizManager' usage echo "Searching for 'Qualifier.BizManager' usage:" rg --type typescript --type javascript "Qualifier\.BizManager"Length of output: 412
Script:
#!/bin/bash # Description: Check for any remaining references to 'Qualifier.ChatModel' and verify 'Qualifier.BizManager' usage # Search for any remaining 'Qualifier.ChatModel' references echo "Searching for 'Qualifier.ChatModel' references:" rg --type ts --type js "Qualifier\.ChatModel" # Search for 'Qualifier.BizManager' usage echo "Searching for 'Qualifier.BizManager' usage:" rg --type ts --type js "Qualifier\.BizManager"Length of output: 643
core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/bar/BizManager.ts (2)
11-11: LGTM: Import addition is consistent with later usage.The addition of
LoadUnitNameQualifierAttributeto the import statement is appropriate and aligns with its usage later in the file.
14-14: Verify the impact of the qualifier change across the codebase.The change from 'Qualifier.ChatModel' to 'Qualifier.BizManager' looks correct and more aligned with the context of this file. However, ensure that this change doesn't break any existing code that might be relying on the old qualifier.
Run the following script to check for any remaining usage of the old qualifier:
✅ Verification successful
Qualifier Change Verified Successfully
The update from 'Qualifier.ChatModel' to 'Qualifier.BizManager' has been thoroughly checked across the codebase. No instances of the old qualifier remain, and the new qualifier is correctly utilized in all relevant locations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of the old 'Qualifier.ChatModel' qualifier # Test: Search for 'Qualifier.ChatModel' in all TypeScript files rg --type typescript "Qualifier\.ChatModel" # Test: Search for uses of BizManagerQualifierAttribute rg --type typescript "BizManagerQualifierAttribute"Length of output: 167
Script:
#!/bin/bash # Description: Check for any remaining usage of the old 'Qualifier.ChatModel' qualifier # Test: Search for 'Qualifier.ChatModel' in all TypeScript files rg --type ts "Qualifier\.ChatModel" # Test: Search for uses of BizManagerQualifierAttribute rg --type ts "BizManagerQualifierAttribute"Length of output: 1514
core/metadata/src/factory/EggPrototypeCreatorFactory.ts (2)
31-45: Addition of default qualifiers to multi-instance objectsThe code correctly adds default qualifiers to each multi-instance object's qualifiers, ensuring that essential attributes like
InitTypeQualifierAttributeandLoadUnitNameQualifierAttributeare included. This enhances the handling of prototype instances and maintains consistency across objects.
53-53: Inclusion ofproperQualifiersin prototype propertiesIncluding
properQualifiers: obj.properQualifiersin thepropertiesarray ensures that each prototype carries its specific qualifiers. This is a valuable addition that maintains the integrity and specificity of prototype information.core/core-decorator/src/util/PrototypeUtil.ts (3)
6-6: Import ofInitTypeQualifierAttributeadded correctlyThe addition of
InitTypeQualifierAttributeto the imports is necessary for its usage in the code changes below.
9-9: Import ofLoadUnitNameQualifierAttributeadded correctlyThe addition of
LoadUnitNameQualifierAttributeto the imports is appropriate and supports the new logic implemented.
161-161: Verify the impact of addingobjectsto the returned metadataBy spreading
...callBackMetadataand addingobjects, ensure that there are no naming conflicts with existing properties incallBackMetadata. Confirm thatobjectsis not already a property ofEggMultiInstanceCallbackPrototypeInfoto prevent unintentional overwriting.Run the following script to check if
objectsis an existing property:✅ Verification successful
No naming conflicts found with existing properties in
EggMultiInstanceCallbackPrototypeInfo. Addingobjectsto the returned metadata is safe.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if `objects` is a property of `EggMultiInstanceCallbackPrototypeInfo` # Test: Search for the definition of `EggMultiInstanceCallbackPrototypeInfo` and list its properties. rg --type=typescript -A 5 'interface EggMultiInstanceCallbackPrototypeInfo'Length of output: 112
Script:
#!/bin/bash # Description: Verify if `objects` is a property of `EggMultiInstanceCallbackPrototypeInfo` # Corrected Test: Search for the definition of `EggMultiInstanceCallbackPrototypeInfo` and list its properties using the correct TypeScript file type. rg --type=ts -A 5 'interface EggMultiInstanceCallbackPrototypeInfo'Length of output: 590
Script:
#!/bin/bash # Description: Retrieve the full definition of `EggMultiInstanceCallbackPrototypeInfo` to list all its properties. # Search for the interface definition and extract all property names. rg --type=ts 'interface EggMultiInstanceCallbackPrototypeInfo' -A 50 | grep -E '^\s*\w+.*;'Length of output: 599
core/metadata/src/impl/ModuleLoadUnit.ts (8)
47-48: Assignments in constructor are correctThe assignments of
this.qualifiersandthis.initTypeare appropriately set.
71-71: Updated graph type toGraph<ProtoNode>The graph type has been correctly updated to
Graph<ProtoNode>after the removal ofMultiInstanceProtoNode.
78-78: Graph initialization updated appropriatelyThe graph is now instantiated as
new Graph<ProtoNode>(), reflecting the updated type.
84-84: Method return type updated appropriatelyThe method
findInjectNodenow returnsGraphNode<ProtoNode> | undefined, aligning with the updated graph node types.
105-105: Local variable type updatedThe type of
temphas been updated toMap<EggProtoImplClass, GraphNode<ProtoNode>>, which is consistent with the graph's node type.
119-119: Updated array type toGraphNode<ProtoNode>[]The array
protoGraphNodesnow correctly holdsGraphNode<ProtoNode>instances.
121-140: Logic for buildingprotoGraphNodesupdated appropriatelyThe
buildmethod now integrates the handling of multi-instance prototypes directly within theModuleGraphclass. The changes correctly createProtoNodeinstances for both multi-instance and regular prototypes.
38-43: Ensure allProtoNodeconstructor calls are updated to the new signatureThe
ProtoNodeconstructor now acceptsinitTypeandqualifiersinstead ofunitPathandmoduleName. Please verify that all instances whereProtoNodeis instantiated have been updated to reflect this change.Run the following script to find all usages of the
ProtoNodeconstructor and check for compliance with the new signature:✅ Verification successful
All
ProtoNodeconstructor calls are updated to the new signature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all `ProtoNode` constructor calls use the updated signature. # Test: Search for `new ProtoNode` instances. Expect: All constructor calls use `clazz, objName, initType, qualifiers`. rg --type ts 'new ProtoNode\(' -A 5Length of output: 1040
| @@ -33,5 +33,5 @@ export interface EggPrototypeInfo { | |||
| /** | |||
| * EggPrototype properties qualifiers | |||
| */ | |||
| properQualifiers?: QualifierInfo[]; | |||
| properQualifiers?: Record<string, QualifierInfo[]>; | |||
There was a problem hiding this comment.
💡 Codebase verification
Potential Issues Detected with properQualifiers Type Change
The modification of properQualifiers to Record<string, QualifierInfo[]> affects multiple parts of the codebase. Please review and update the following files to ensure compatibility with the new type definition:
core/types/core-decorator/model/EggMultiInstancePrototypeInfo.tscore/core-decorator/test/decorators.test.tscore/core-decorator/src/decorator/Prototype.tsplugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/bar/BizManager.tscore/metadata/src/factory/EggPrototypeCreatorFactory.tscore/metadata/src/impl/ModuleLoadUnit.tscore/metadata/src/impl/EggPrototypeBuilder.tscore/metadata/src/model/AppGraph.tscore/core-decorator/src/util/QualifierUtil.ts
Ensure that all accesses to properQualifiers are updated to handle the Record<string, QualifierInfo[]> structure appropriately.
🔗 Analysis chain
Approve the change to properQualifiers type.
The modification from QualifierInfo[] to Record<string, QualifierInfo[]> for the properQualifiers property is a good improvement. It allows for more structured and granular organization of qualifiers, which aligns with the PR objective of fixing "miss MultiInstance proper qualifiers".
This change enables easier lookup and management of qualifiers associated with specific keys, which could be particularly useful for handling MultiInstance scenarios.
To ensure this change doesn't introduce any issues, please verify the following:
- Update any code that sets or retrieves
properQualifiersto use the new structure. - Check for any type errors in files that import and use
EggPrototypeInfo.
Run the following script to identify potential areas that need updates:
Review the output and update the identified areas as necessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of EggPrototypeInfo and properQualifiers
echo "Files importing EggPrototypeInfo:"
rg --type typescript "import.*EggPrototypeInfo" --glob '!{core/types/core-decorator/model/EggPrototypeInfo.ts}'
echo "\nUsages of properQualifiers:"
rg --type typescript "properQualifiers" --glob '!{core/types/core-decorator/model/EggPrototypeInfo.ts}'
Length of output: 433
Script:
#!/bin/bash
# Description: Find usages of EggPrototypeInfo and properQualifiers
echo "Files importing EggPrototypeInfo:"
rg --type ts "import.*EggPrototypeInfo" --glob '!core/types/core-decorator/model/EggPrototypeInfo.ts'
echo "\nUsages of properQualifiers:"
rg --type ts "properQualifiers" --glob '!core/types/core-decorator/model/EggPrototypeInfo.ts'
Length of output: 4540
| @@ -34,6 +34,7 @@ export class EggPrototypeBuilder { | |||
| private injectObjects: Array<InjectObject | InjectConstructor> = []; | |||
| private loadUnit: LoadUnit; | |||
| private qualifiers: QualifierInfo[] = []; | |||
| private properQualifiers: Record<string, QualifierInfo[]> = {}; | |||
There was a problem hiding this comment.
Inconsistent naming: 'properQualifiers' vs 'propertyQualifiers'
There is inconsistency in variable names between properQualifiers and propertyQualifiers. This can lead to confusion and potential errors. Consider standardizing the naming throughout the codebase to either properQualifiers or propertyQualifiers for clarity and maintainability.
| const multiInstancePropertyQualifiers = this.properQualifiers[injectObject.refName as string] ?? []; | ||
| console.log('multi instance: ', this.properQualifiers, injectObject.refName); | ||
| return EggPrototypeFactory.instance.getPrototype(injectObject.objName, this.loadUnit, [ | ||
| ...propertyQualifiers, | ||
| ...multiInstancePropertyQualifiers, | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate code duplication in prototype retrieval
The logic for retrieving qualifiers and calling EggPrototypeFactory.instance.getPrototype is duplicated across the methods tryFindDefaultPrototype, tryFindContextPrototype, and tryFindSelfInitTypePrototype. Refactoring this repeated code into a shared helper method would improve maintainability and reduce duplication.
Suggested refactoring:
Add a private helper method to combine qualifiers:
private getCombinedQualifiers(injectObject: InjectObject): QualifierInfo[] {
const propertyQualifiers = QualifierUtil.getProperQualifiers(this.clazz, injectObject.refName);
const multiInstancePropertyQualifiers = this.properQualifiers[injectObject.refName as string] ?? [];
return [
...propertyQualifiers,
...multiInstancePropertyQualifiers,
];
}Modify the methods to use this helper:
// In tryFindDefaultPrototype
-const propertyQualifiers = QualifierUtil.getProperQualifiers(this.clazz, injectObject.refName);
-const multiInstancePropertyQualifiers = this.properQualifiers[injectObject.refName as string] ?? [];
return EggPrototypeFactory.instance.getPrototype(injectObject.objName, this.loadUnit, [
- ...propertyQualifiers,
- ...multiInstancePropertyQualifiers,
+ ...this.getCombinedQualifiers(injectObject),
]);
// Similar changes in tryFindContextPrototype and tryFindSelfInitTypePrototypeAlso applies to: 78-80, 91-93
| const objects = callBackMetadata.getObjects(ctx); | ||
| const defaultQualifier = [{ | ||
| attribute: InitTypeQualifierAttribute, | ||
| value: callBackMetadata.initType, | ||
| }, { | ||
| attribute: LoadUnitNameQualifierAttribute, | ||
| value: ctx.moduleName, | ||
| }]; | ||
| for (const object of objects) { | ||
| defaultQualifier.forEach(qualifier => { | ||
| if (!object.qualifiers.find(t => t.attribute === qualifier.attribute)) { | ||
| object.qualifiers.push(qualifier); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Ensure object.qualifiers is initialized before usage
The code assumes that object.qualifiers is already an array. If object.qualifiers is undefined, accessing find or pushing to it will result in a runtime error. It's important to ensure that object.qualifiers is initialized.
Apply this diff to initialize object.qualifiers if it's not already:
for (const object of objects) {
+ if (!object.qualifiers) {
+ object.qualifiers = [];
+ }
defaultQualifier.forEach(qualifier => {
if (!object.qualifiers.find(t => t.attribute === qualifier.attribute)) {
object.qualifiers.push(qualifier);
}
});
}📝 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.
| const objects = callBackMetadata.getObjects(ctx); | |
| const defaultQualifier = [{ | |
| attribute: InitTypeQualifierAttribute, | |
| value: callBackMetadata.initType, | |
| }, { | |
| attribute: LoadUnitNameQualifierAttribute, | |
| value: ctx.moduleName, | |
| }]; | |
| for (const object of objects) { | |
| defaultQualifier.forEach(qualifier => { | |
| if (!object.qualifiers.find(t => t.attribute === qualifier.attribute)) { | |
| object.qualifiers.push(qualifier); | |
| } | |
| }); | |
| } | |
| const objects = callBackMetadata.getObjects(ctx); | |
| const defaultQualifier = [{ | |
| attribute: InitTypeQualifierAttribute, | |
| value: callBackMetadata.initType, | |
| }, { | |
| attribute: LoadUnitNameQualifierAttribute, | |
| value: ctx.moduleName, | |
| }]; | |
| for (const object of objects) { | |
| if (!object.qualifiers) { | |
| object.qualifiers = []; | |
| } | |
| defaultQualifier.forEach(qualifier => { | |
| if (!object.qualifiers.find(t => t.attribute === qualifier.attribute)) { | |
| object.qualifiers.push(qualifier); | |
| } | |
| }); | |
| } |
| const instanceQualifiers = [ | ||
| ...qualifiers, | ||
| ...info.qualifiers, | ||
| ]; |
There was a problem hiding this comment.
Ensure info.qualifiers is defined before using spread operator
When using the spread operator on info.qualifiers, if it is undefined, it will cause a runtime error. To prevent this, ensure that info.qualifiers is always an array.
Consider providing a default empty array if info.qualifiers might be undefined:
const instanceQualifiers = [
...qualifiers,
- ...info.qualifiers,
+ ...(info.qualifiers || []),
];📝 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.
| const instanceQualifiers = [ | |
| ...qualifiers, | |
| ...info.qualifiers, | |
| ]; | |
| const instanceQualifiers = [ | |
| ...qualifiers, | |
| ...(info.qualifiers || []), | |
| ]; |
| if (!PrototypeUtil.isEggMultiInstancePrototype(clazz)) { | ||
| const defaultQualifier = [{ | ||
| attribute: InitTypeQualifierAttribute, | ||
| value: PrototypeUtil.getInitType(clazz, { | ||
| unitPath: this.moduleConfig.path, | ||
| moduleName: this.moduleConfig.name, | ||
| })!, | ||
| }, { | ||
| attribute: LoadUnitNameQualifierAttribute, | ||
| value: this.name, | ||
| }]; | ||
| for (const qualifier of defaultQualifier) { | ||
| QualifierUtil.addProtoQualifier(clazz, qualifier.attribute, qualifier.value); | ||
| } |
There was a problem hiding this comment.
Handle possible undefined return value from getInitType
The PrototypeUtil.getInitType method is called with a non-null assertion operator !. If getInitType returns undefined, this could lead to runtime errors. It's important to handle the possibility that initType might be undefined.
Consider checking if initType is undefined before proceeding:
const initType = PrototypeUtil.getInitType(clazz, {
unitPath: this.moduleConfig.path,
moduleName: this.moduleConfig.name,
});
+if (!initType) {
+ // Handle undefined initType appropriately
+ throw new Error(`InitType is undefined for ${clazz.name}`);
+}
const defaultQualifier = [{
attribute: InitTypeQualifierAttribute,
- value: PrototypeUtil.getInitType(clazz, {
- unitPath: this.moduleConfig.path,
- moduleName: this.moduleConfig.name,
- })!,
+ value: initType,
}, {
attribute: LoadUnitNameQualifierAttribute,
value: this.name,
}];📝 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.
| if (!PrototypeUtil.isEggMultiInstancePrototype(clazz)) { | |
| const defaultQualifier = [{ | |
| attribute: InitTypeQualifierAttribute, | |
| value: PrototypeUtil.getInitType(clazz, { | |
| unitPath: this.moduleConfig.path, | |
| moduleName: this.moduleConfig.name, | |
| })!, | |
| }, { | |
| attribute: LoadUnitNameQualifierAttribute, | |
| value: this.name, | |
| }]; | |
| for (const qualifier of defaultQualifier) { | |
| QualifierUtil.addProtoQualifier(clazz, qualifier.attribute, qualifier.value); | |
| } | |
| if (!PrototypeUtil.isEggMultiInstancePrototype(clazz)) { | |
| const initType = PrototypeUtil.getInitType(clazz, { | |
| unitPath: this.moduleConfig.path, | |
| moduleName: this.moduleConfig.name, | |
| }); | |
| if (!initType) { | |
| // Handle undefined initType appropriately | |
| throw new Error(`InitType is undefined for ${clazz.name}`); | |
| } | |
| const defaultQualifier = [{ | |
| attribute: InitTypeQualifierAttribute, | |
| value: initType, | |
| }, { | |
| attribute: LoadUnitNameQualifierAttribute, | |
| value: this.name, | |
| }]; | |
| for (const qualifier of defaultQualifier) { | |
| QualifierUtil.addProtoQualifier(clazz, qualifier.attribute, qualifier.value); | |
| } |
killagu
changed the title
|
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
New Features
BizManagermodule to include distinct secret values for clients.Bug Fixes
ClazzMapandModuleNodeclasses.Documentation
Refactor
ModuleGraphclass.Style
Tests
BizManagerand qualifier handling logic.