Safer type.forName #149
Safer type.forName #149
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 { |
| dynamicInstance = Type.forName(className).newInstance(); | ||
| try { | ||
| dynamicInstance = Type.forName(namespace, className).newInstance(); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
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 { |
| 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.
Can we catch an explicit type of exception here?
There was a problem hiding this comment.
Please add corresponding unit tests within FinalizerHandlerTest and MetadataTriggerHandlerTest 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.
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.
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 API https://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.