Fundamentally the trigger framework is flawed

[docbill]

The trigger handler framework is fundamentally flawed.

The most major flaw, is it should be possible to UNIT test trigger code without depending on whether the trigger itself is active. Afterall, it is a unit test, not an end-to-end test.

The next flaw is the overloading of beforeInsert, beforeUpdate, ... methods, means code MUST be replicated, and leads for overly complex code.

Consider for example, I want three triggers. One that validates data populated on an object. One that sends an e-mail with those values after validation. And one that changes a status to avoid repeated processing.

Since it all very tightly related logic, it is reasonable I put it all in the same trigger handler. Now the same logic would run before insert and before update. So I would have methods:

void beforeInsert() { validate(); email();, statusUpdate(); } void beforeUpdate() { validate(); email(); statusUpdate(); }

So I'm needlessly replicated code. It also does not allow you an easy way to say deactivate the email(); method without pushing an code update. Worse the design encourages me to put afterInsert and afterUpdate in the same trigger. But 99.9% of time code logic needs to be in the before trigger or the after trigger but not both.

Now I have written a trigger framework that addresses these and other issues. I'm going to see if I can get permission to open source it. But I think even though I don't like one method per event, it is a common design pattern. So I can see some developers would still prefer to use this frame work. Some of these definencies are realtively easy to solve.

For example, I have a TriggerArguments class that I use in my trigger handlers. That abstracts away direct access to the Trigger class and allows test methods to set the arguments without invoking the DML. Thus, true trigger tests. If I were to submit merge requests to augment these recepies to try and fix these design flaws, without changing the fundamental design, would you be willing to consider them?

[github-actions[bot]]

Welcome! 👋

Thank you for posting this issue. 🙇🏼‍♂️ We will come back to you latest within the next 48h (working days). Stay tuned!

[github-actions[bot]]

Flagging this as stale, as there was no recent activity within the last 14 days. 🧐

[codefriar]

@docbill

There's a lot to unpack here, and I want to make sure to discuss this with you, so there will likely be a breaking out of your comments into several replies. Thank you for your patience while I collect my thoughts on this.

First though, I want to establish a couple of 'ground rules' here.

1. I assume that everyone in this conversation has the best interests of this project in mind.
2. I assume that not everyone will agree, and that we all acknowledge a variety of opinions and thoughts on any given subject can only help, in the long run.

I also want to clarify what Apex Recipes is. There is a belief in the community that this sample app exists as a finished product representing best practices. While this repository does represent best practices, its primary purpose is to teach developers. Not just the syntax or architecture of Apex code - but also development topics like progressive enhancement of an existing codebase.

I draw attention to this, because the TriggerHandler itself, has at least one enhancement built on top of it - MetadataDrivenTriggerHandler.

Given this, I don't believe it's fair to call the trigger handler fundamentally flawed. It demonstrates a very common need to make what you inherited, better.

More in a second.

[codefriar]

@docbill

You wrote: 'The most major flaw, is it should be possible to UNIT test trigger code without depending on whether the trigger itself is active. Afterall, it is a unit test, not an end-to-end test.'

I'm not entirely sure what you mean here? This trigger framework is designed in 3 parts:

1. Triggers
2. Classes extending TriggerHandler
3. MetadataTriggerHandler

The platform's rules on code coverage are straightforward and to my knowledge they've not changed in ~15 years. Every trigger must have some code coverage. The aggregate code coverage for classes must meet or exceed 75%.

There is no way to UNIT test a trigger. To test a trigger, you must invoke DML. This is a limitation of the platform, not Apex Recipes Trigger framework. This is one of the driving points for this trigger framework. This framework asks developers to place their trigger logic in a class extending TriggerHandler. These handlers can and should be unit tested. You can, and should unit test not only the brokering methods: beforeUpdate, afterUpdate etc. but also your individual logic methods.

Lastly, with the addition of MetadataDrivenTriggerHandler, the above idea doesn't change - you'll still be UNIT testing your TriggerHandler classes. The key difference here, is that MetadataDrivenTriggerHandler enables UI based activation and deactivation of what triggers are actually run.

Thoughts?

[codefriar]

@docbill

You wrote: "The next flaw is the overloading of beforeInsert, beforeUpdate, ... methods, means code MUST be replicated, and leads for overly complex code.

Consider for example, I want three triggers. One that validates data populated on an object. One that sends an e-mail with those values after validation. And one that changes a status to avoid repeated processing.

Since it all very tightly related logic, it is reasonable I put it all in the same trigger handler. Now the same logic would run before insert and before update. So I would have methods:

void beforeInsert() { validate(); email();, statusUpdate(); } void beforeUpdate() { validate(); email(); statusUpdate(); }

So I'm needlessly replicated code. It also does not allow you an easy way to say deactivate the email(); method without pushing an code update. Worse the design encourages me to put afterInsert and afterUpdate in the same trigger. But 99.9% of time code logic needs to be in the before trigger or the after trigger but not both."

Ok, here I think we can get into the meet of some things. The over-arching idea behind this trigger framework is that you have:

• One Trigger per object. This trigger necessarily requires activation in all DML contexts. Trigger has exactly one line of code - new TriggerHandlerNameHere().run()
• One TriggerHandler per object. This trigger handler has 'brokering' methods for each DML context. Ideally, these brokering methods would invoke other classes and/or at the minimum, invoke other 'helper' methods in the triggerHandler class.

It's entirely fair to say that this requires some duplication of code.

However this is one of the problems solved by MetadataDrivenTriggerHandler. Instead of invoking a specific TriggerHandler class, you invoke the new MetadataTriggerHandler().run(). This allows the MetadataDrivenTriggerHandler to query custom metadata objects for what, and in what order, to run things. This architecture benefits from a large number of single-focused triggerHandler classes. In other words, for your example, you could have AccountValidateTriggerHandler, AccountEmailTriggerHandler and AccountStatusUpdateTriggerHandler as classes extending the basic TriggerHandler class. You'd then add Custom Metadata Records for Account that invoke Validate, Email and statusUpdate in that order.

Thoughts?

[docbill]

Well thought out objection. However, you are misinterpreting the objective. If you can think of a better way to word it that would be greatly appreciated...

At RedHat, most of our triggers achieve this objective...

I'll give you a very simple example of one in the our October release:

/**
 * This is a special trigger used to prevent code from being being called twice,
 * unless an error was set.  This respective code is expected to use thread locking as follows:
 * 
 * <pre>
 *  String lockKey = ThreadLockCallableTrigger.getThreadLockKEY(<MY UNIQUE KEY>,triggerArguments);
 *  if(ThreadLock.lock(lockKey)) {
 *      try {
 *          // do some work
 *      }
 *      finally {
 *          ThreadLock.unlock(lockKey);
 *      }
 *  }
 * </pre>
 * 
 * The name of this trigger should be the the value of <MY UNIQUE KEY> appended with '.lock'.
 * A null may be passed as triggerArguments, if there is no triggerArguments available.
 *
 * @version 2021-07-16
 * 
 * @author Bill Riemers <briemers@redhat.com>
 * @since 2021-07-16 - Created
 */
public  
without sharing 
class ThreadLockCallableTrigger
implements Callable
{
    TriggerArguments triggerArguments;

    /**
     * Standard constructor.
     */
    public ThreadLockCallableTrigger() {}

    private static Set<String> firstCalledSet = new Set<String>();

    /**
     * Called to get a unique string to lock this a trigger.
     * 
     * @param action name of the trigger such as 'Opp_Before.updateProducts'
     * @param triggerArguments passing a null is equivalent to passing new TriggerArguments(null)
     */
    public static String getThreadLockKey(String action,TriggerArguments triggerArguments) {
        if(triggerArguments == null) {
            triggerArguments = new TriggerArguments(null);
        }
        return action
            +','+triggerArguments.isBefore
            +','+triggerArguments.isUpdate
            +','+triggerArguments.isDelete
            +','+triggerArguments.isUndelete;
    }

    /**
     * This method will check if a there are no records in error, and if so lock the thread for
     * the respective action.
     * 
     * @param action name of trigger to lock
     */
    @TestVisible
    private void threadLock(String action) {
        if(firstCalledSet.contains(action)) {
            List<SObject> recordList = triggerArguments.oldList;
            if(triggerArguments.newList != null && ! triggerArguments.newList.isEmpty()) {
                recordList = triggerArguments.newList;
            }
            if(recordList != null) {
                for(SObject record : recordList) {
                    if(record.hasErrors()) { return; }
                }
                String key = getThreadLockKey(action,triggerArguments);
                ThreadLock.lock(key);
            }
        }
        firstCalledSet.add(action);
    }

    /**
     * This is the callable method to invoke the triggers in this class.
     * 
     * @param action the trigger method to invoke
     * @param args a map of Trigger values
     */
    public Object call(String action, Map<String,Object> args) {
        action = ''+action; // just to avoid null exceptions
        triggerArguments = new TriggerArguments(args);
        final String lockedAction = getLockedAction(action);
        if(lockedAction != null) {
            threadLock(lockedAction);
        }
        else {
            throw new ExtensionMalformedCallException('Method not implemented');
        }
        return null;
    }

    static String getLockedAction(String action) {
        for(String suffix : new List<String>{'.lock','!'}) {
            if(action.endsWithIgnoreCase(suffix)) {
                return action.left(action.length()-suffix.length());
            }
        }
        return null;
    }

    public class ExtensionMalformedCallException extends Exception {}
}

This gives us 100% code coverage of:

/**
 * Test class for ThreadLockCallableTrigger.
 * 
 * @version 2021-07-16
 * 
 * @author Bill Riemers <briemers@redhat.com>
 * @since 2021-07-16 - Created
 */
@isTest
private class ThreadLockCallableTriggerTest
{
    @isTest
    static void threadLockTest() {
        ThreadLockCallableTrigger callableTrigger = new ThreadLockCallableTrigger();
        List<Opportunity> newList = ( new List<Opportunity>{
            new Opportunity(OwnerId=UserInfo.getUserId())
        } );
        TriggerArguments ta = TriggerArguments.createBeforeInsert(newList);
        String triggerName = 'Opp_Before.assignLegacyCallable'; 
        String key = ThreadLockCallableTrigger.getThreadLockKey(triggerName,ta);
        Boolean lockValue = ThreadLock.lock(key);
        System.assertEquals(true,lockValue,'Expected lock on '+key);
        ThreadLock.unlock(key);

        Test.startTest();
        // the first time simply adds this name to the firstCalledSet
        callableTrigger.call(triggerName+'.lock',ta.callableArguments);
        // the second time actually does the locking
        callableTrigger.call(triggerName+'.lock',ta.callableArguments);
        Test.stopTest();

        lockValue = ThreadLock.lock(key);
        System.assertEquals(false,lockValue,'Unexpected lock on '+key);

        // for coverage only
        String key2 = ThreadLockCallableTrigger.getThreadLockKey(triggerName,null);
    }

    @isTest
    static void callExceptionTest() {
        ThreadLockCallableTrigger callableTrigger = new ThreadLockCallableTrigger();
        List<User> newList = ( new List<User>{
            new User()
        } );
        TriggerArguments ta = TriggerArguments.createBeforeInsert(newList);
        try {
            // user will not have owner id now owner c
            callableTrigger.call('The dog ate my homework.',ta.callableArguments);
            System.assert(true,'Expected ExtensionMalformedCallException to be thrown.');
        }
        catch(ThreadLockCallableTrigger.ExtensionMalformedCallException ex) {
            System.debug('Expected exception: '+ex);
        }
    }
}

and the respective triggers that call our framework are covered by ANY dml. They do not test if we have the Boolean Metadata setting to actually call this code in order to achieve 100% coverage.

Our typical trigger looks something like:

trigger Foo_TriggerManagement on Foo__c (before delete, before insert,
before update, after delete, after insert, after update, after undelete) {
    new TriggerManagement().call('Foo_TriggerManagement',null);
}

In the rare case we need trigger method explicitly to do DML we can do something like:

@isTest
static void deployTest() {
List<Foo__c> newList = (new List<IE_Account_Transaction_Log__c>{ Foo_
TestDataFactory.createFoo() });
    // Turn on all the triggers, and make sure everything is tested.
    BooleanValuesHelper.setValuesWhichStartWith('Foo_Before.',true);
    BooleanValuesHelper.setValuesWhichStartWith('Foo_After.',true);
    insert newList;
}

This invokes the one line of the trigger. Whether the rest gets invoked depends on the metadata configuration and this test is not intended to check our current configuration. The two BooleanValueHelper calls really are not needed in this example, but we opted to have them to make sure should all triggers be enabled, we are not getting an exception from this simple DML. As part of our goal is not just 100% code coverage in this particular package, but we want a very high level of functional coverage.

I can of course provide more examples if you want. Most of our test classes for triggers do not require an actual DML in those that do often require no asserts that depend on whether or not the trigger is active.

If you have any further questions or feedback, please feel free to contribute your thoughts.

Regards,

Bill

[codefriar]

@docbill would you mind putting your code in triple backticks please? it'll format it better for easier reading.

[docbill]

Ok, here I think we can get into the meet of some things. The over-arching idea behind this trigger framework is that you have:

• One Trigger per object. This trigger necessarily requires activation in all DML contexts. Trigger has exactly one line of code - new TriggerHandlerNameHere().run()
• One TriggerHandler per object. This trigger handler has 'brokering' methods for each DML context. Ideally, these brokering methods would invoke other classes and/or at the minimum, invoke other 'helper' methods in the triggerHandler class.

It's entirely fair to say that this requires some duplication of code.

However this is one of the problems solved by MetadataDrivenTriggerHandler. Instead of invoking a specific TriggerHandler class, you invoke the new MetadataTriggerHandler().run(). This allows the MetadataDrivenTriggerHandler to query custom metadata objects for what, and in what order, to run things. This architecture benefits from a large number of single-focused triggerHandler classes. In other words, for your example, you could have AccountValidateTriggerHandler, AccountEmailTriggerHandler and AccountStatusUpdateTriggerHandler as classes extending the basic TriggerHandler class. You'd then add Custom Metadata Records for Account that invoke Validate, Email and statusUpdate in that order.

Different projects different goals. The biggest problem writing unit tests without DML, is salesforce gives us no way to create System.Trigger. There are two different ways to try and resolve this. One you create classes where you pass in the arguments from Trigger, or two you create a class like TriggerArguments that replicates from Trigger.

From one perspective it makes sense to think lets make a method for each trigger event. After all that is what other architectures do. However, after few years of working on salesforce I noticed it was extremely rare to need before and after in the same class. And even the need for an Trigger.isInsert or Trigger.isUpdate is extremely rare. As such, it makes sense to have features developers rarely need more in the background. e.g. In the rare case you need it, do the if check. This is one the things salesforce got right in their architecture.

However, the main advantage I see of the separate methods is all those lightning developers that are used to seeing events done that way will find it easier to understand.

To me though the bigger problem is not having that replacement of Trigger, so that the test classes don't need to do DML, and so it is actually possible to call the trigger classes without actually invoking DML.

The other problem is using this framework from packages. Either a developer puts this class in their package, or they put a dependency on the package with it. That means package providers have to replicate the framework, or depend on it in every package.

Our solution to this was to use the Callable interface. As then it is possible to make the use of the trigger framework optional in our packages.

But I know at least some of our developers are going to want to use this trigger framework. So ultimately my goal is to try and find a way to give them the option and still meet our normal design objectives.

[docbill]

@docbill would you mind putting your code in triple backticks please? it'll format it better for easier reading.

It appears the triple backtick doesn't do much when the reply was sent by e-mail. I will try a few more things to fix the formatting.

[codefriar]

@docbill - Have any interest in doing a codeLive episode (livestream) with me? We can meet ahead of time, and pull the best of our ideas together and build a TriggerFramework 4.0, give it a cool name and write a blog post about it? I'd love to move to using the Callable interface, for example. It'd be wonderful to build something that could easily be dropped into an org as a package, and yet still meet the goals of a declarative, one-trigger-per-object and one-handler-per-object.

[codefriar]

@docbill -

You wrote: "To me though the bigger problem is not having that replacement of Trigger, so that the test classes don't need to do DML, and so it is actually possible to call the trigger classes without actually invoking DML."

Sadly, I'm not in a position where I can demand the engineering teams build a constructable System.trigger object 😭 . However, I have taken this idea to the appropriate internal product owners.

[codefriar]

@docbill -

The other problem is using this framework from packages. Either a developer puts this class in their package, or they put a dependency on the package with it. That means package providers have to replicate the framework, or depend on it in every package.

Our solution to this was to use the Callable interface. As then it is possible to make the use of the trigger framework optional in our packages.

I agree with you 100% about package dev. Sadly, I don't know how to fix this problem with packages as we know it. Ideally, I'd love to see a solution as simple/elegant as RubyGems, PythonEggs, or NPM.

I'd love to hear more about how using the callable interface makes it possible to make the framework optional.

[docbill]

@docbill - Have any interest in doing a codeLive episode (livestream) with me? We can meet ahead of time, and pull the best of our ideas together and build a TriggerFramework 4.0, give it a cool name and write a blog post about it? I'd love to move to using the Callable interface, for example. It'd be wonderful to build something that could easily be dropped into an org as a package, and yet still meet the goals of a declarative, one-trigger-per-object and one-handler-per-object.

I submitted a request to Red Hat legal to open source the code. If you have a timeframe in mind, I can push to see if I can get the approval done faster. Without the code like working with one hand tied behind my back to try and explain ideas. But yes, it would be great to collaborate and try to and get mixture of the best ideas we've had so far.

[docbill]

https://github.com/docbill/Apex-Framework

[docbill]

Going though the entire code base is probably overkill to understand the basic ideas behind it. The following documentation:

https://github.com/docbill/Apex-Framework/tree/main/docs/topics/callabletriggers

Is probably good enough.

I do see a few minor mistakes in the documentation. For example I reference BooleanSettingc, but you won't find that anywhere in the package, because I decided not to include that in the open source. As BooleanSettingc is just what we used before salesforce introduced custom metadata, and I moved that to a separate package for backwards compatibility with our production org.

BooleanHierarchy__c on the other hand gets used regularly. It is our way of deactivating triggers for particular users or profiles. We'll commonly do this when debugging issues or performing data migrations. I saw this trigger framework has a list of user ids that allow simmilar usage.

[docbill]

Kevin,

Is codeLive a YouTube channel, podcast, ... ? If you send me a link I can get the general idea of the format and have a better idea on how you would probably like to proceed.

Regards,

Bill

On Thu, Oct 7, 2021, 12:45 Kevin Poorman @.***> wrote:

@docbill https://github.com/docbill - Have any interest in doing a codeLive episode (livestream) with me? We can meet ahead of time, and pull the best of our ideas together and build a TriggerFramework 4.0, give it a cool name and write a blog post about it? I'd love to move to using the Callable interface, for example. It'd be wonderful to build something that could easily be dropped into an org as a package, and yet still meet the goals of a declarative, one-trigger-per-object and one-handler-per-object.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/trailheadapps/apex-recipes/issues/200#issuecomment-937972576, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYFYWAKPZDZOOFJLEEC6T3UFXFDVANCNFSM5ERBI34Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[docbill]

@codefriar

I never heard back from you. This has been low priority. But we are building out the new salesforce instance. One of our principle architect really likes the TriggerHandler framework, in as he puts it, it much simplier. Since we are building from source we can manage our dependencies better than our legacy org, so it is not a significant pain to say triggers depend on extending this class. Except when it comes to integrating third party packages and such...

I'm pretty much agnostic on what framework our developers use. Naturally I like our Callable Trigger Framework, as it matches the way I'm used to developer code. And the ability to fully control the trigger execution from metadata, is a huge win when it comes to support and maintenence. We simply cannot get by without Boolean Heirarchy to allow selective activation and deactivation during data migrations.

I think the best way to have my cake and eat it to in this case, is to write a very simple trigger handler implementation class. That allows the callable framework to call triggers that implement TriggerHandler. I've already created a fork and written such a class. But I need to write a test class, and convince one of our developers using the TriggerHandler to test it out with there triggers.

When the new fork is ready, I plan to submit a pull request. But please let me know if you would want to jump into this process earlier on.

[pozil]

Hi @docbill there's been some changes to our organization since this conversation stated. @codefriar decided to change company so he's no longer working on Apex Recipes. Past and upcoming codeLive episodes can be found on our YouTube channel.