- Notifications
You must be signed in to change notification settings - Fork178
Safer type.forName#149
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Please sign the CLA so I can review these contributions. https://github.com/mitchspano/apex-trigger-actions-framework/blob/main/docs/contributing.md |
| try { | ||
| dynamicInstance = Type.forName(className).newInstance(); | ||
| try { |
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.
Double nested try blocks?
| dynamicInstance = Type.forName(className).newInstance(); | ||
| try { | ||
| dynamicInstance = Type.forName(namespace, className).newInstance(); | ||
| } catch (Exception e) { |
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.
Can we catch an explicit type of exception here?
| triggerAction = Type.forName(triggerMetadata.Apex_Class_Name__c) | ||
| .newInstance(); | ||
| String namespace = triggerMetadata.Apex_Class_Namespace__c; | ||
| try { |
mitchspanoJul 1, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Double nested try blocks?
| String namespace = triggerMetadata.Apex_Class_Namespace__c; | ||
| try { | ||
| triggerAction = Type.forName(namespace, triggerMetadata.Apex_Class_Name__c).newInstance(); | ||
| } catch (Exception e) { |
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.
Can we catch an explicit type of exception here?
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.
Please add corresponding unit tests withinFinalizerHandlerTest andMetadataTriggerHandlerTest which explicitly define the specifications for how the system is supposed to behave in all scenarios including but not limited to:
- Namespace provided in
Apex_Class_Namespace__c - Namespace included in
Apex_Class_Name__c - Namespace provided in both
- etc.
@mitchspano I added some unit tests and realized my try/catch was unnecessary so I fixed that. I also think I signed the CLA. However, I have the awkward situation that I don't think I have any way to test one scenario I wrote code for. The fallback of the namespace being included in the Apex_Class_Name__c field. To test this, I would need a valid Apex class from a different Namespace available to the unit test. In otherwords, the test would need to depend on another, namespaced, package. It couldn't be fully self contained. Not sure how you would like to handle this. One option is to not handle backward compatability and simply straight require proper usage of the new namespace field so if someone pulls in the lastest changes to Trigger Action Framework they might need to also adjust their configuration of it to not break. |
| // try shared Namespace | ||
| if( actionType == null ) { | ||
| // Get the namespace of the current class. | ||
| String[] parts = String.valueOf(MetadataTriggerHandler.class).split('\\.', 2); |
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.
This looks a bit odd to me.MetadataTriggerHandler class will always have no namespace in the unlocked package.
Is this intended for when someone uses the Trigger Actions Framework within their own Managed package?
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.
Or namespace unlocked package, but yes, that is exactly the intent here as that is how we are using the framework internally (in Namepsaced packages)
Fixes#152 |
If packaged, Actions and Finalizers use an API that cannot access classes outside of the same package. Salesforce Documentation on the Type.forName warns of this and suggests using an alternative APIhttps://developer.salesforce.com/docs/atlas.en-us.apexref.meta/apexref/apex_methods_system_type.htm#apex_System_Type_forName
This adds new Apex_Class_Namespace fields and uses the more explicit and safer version of Type.ForName. I also included a fallback to make sure no existing functionality is lost.