0

I have a trigger on lead after insert that query all accounts and all contacts created the past 30 days to compare 4 fields (external id and legal number on accounts / mobile and email on contacts) and automatically convert the lead if a match is found.

I'm a beginner with apex and, even if I have the expected result, I think the way I wrote my code is bad since I get "Apex CPU time limit exceeded" error triggered sometimes on lead insert (not always).

    trigger LeadDeduplicate on Lead (after insert) {

    List<Account> accounts = [SELECT ID, Account_ID__c, Siret__c, OwnerId FROM Account WHERE Account_ID__c != NULL AND CreatedDate > :System.Today() - 30];
    List<Contact> contacts = [SELECT ID, Email, MobilePhone, AccountId, OwnerId FROM Contact WHERE CreatedDate > :System.Today() - 30];
    Boolean leadIsConverted = false;

    for(Lead lead : trigger.new) {
        // Trigger only if the lead comes from external
        if(lead.IsExternal__c) {
            for(Account account : accounts) {
                if(!leadIsConverted) {
                    if(Utils.hasTheSameAccountId(lead, account) || Utils.hasTheSameSiret(lead, account)) {
                        Utils.mergeLeadWithAccount(lead, account);
                        leadIsConverted = true;
                    }
                }
            }

            if(!leadIsConverted && !contacts.isEmpty()) {
                for(Contact contact : contacts) {
                    if(!leadIsConverted) {
                        if(Utils.hasTheSameEmail(lead, contact) || Utils.hasTheSameMobile(lead, contact)) {
                            Utils.mergeLeadWithContact(lead, contact);
                            leadIsConverted = true;
                        }   
                    }
                }
            }
        }
        leadIsConverted = false;
    }
}

Utils being the apex class used for matching and conversion.

Anyone out there know a workaround to avoid this error ?

Any help would be appreciated.

UPDATE : Thank you both for your first answers, I'll dig into this and come back if I find a solution. I also add my Utils class to provide more context.

public class Utils {
    public static boolean hasTheSameAccountId(Lead lead, Account account) {
        return acccountIdIsNotNull(lead, account) && account.Account_ID__c == lead.Account_ID__c;
    }

    public static boolean hasTheSameSiret(Lead lead, Account account) {
        return siretIsNotNull(lead, account) && account.Siret__c == lead.Siret__c;
    }

    public static boolean acccountIdIsNotNull(Lead lead, Account account) {
        return account.Account_ID__c != null && lead.Account_ID__c != null;
    }

    public static boolean siretIsNotNull(Lead lead, Account account) {
        return account.Siret__c != null && lead.Siret__c != null;
    }

    public static boolean hasTheSameEmail(Lead lead, Contact contact) {
        return emailIsNotNull(lead, contact) && lead.Email == contact.Email;
    }

    public static boolean hasTheSameMobile(Lead lead, Contact contact) {
        return mobileIsNotNull(lead, contact) && Utils.mobilePhoneWithoutCodeCountry(lead.MobilePhone) == Utils.mobilePhoneWithoutCodeCountry(contact.MobilePhone);
    }

    public static String mobilePhoneWithoutCodeCountry(String phoneNumber) {
        return phoneNumber.substring(phoneNumber.length() - 9, phoneNumber.length());
    }

    public static boolean emailIsNotNull(Lead lead, Contact contact) {
        return lead.Email != null && contact.Email != null;
    }

    public static boolean mobileIsNotNull(Lead lead, Contact contact) {
        return lead.MobilePhone != null && contact.MobilePhone != null;
    }

    public static void mergeLeadWithAccount(Lead lead, Account account) {
        Database.LeadConvert lc = new Database.LeadConvert();
        lc.setLeadId(lead.Id);

        lc.setConvertedStatus('Qualifié');
        lc.setDoNotCreateOpportunity(true);
        lc.setAccountId(account.Id);
        lc.setOwnerId(account.OwnerId);

        try {
            Database.LeadConvertResult lcr = Database.convertLead(lc);
            System.assert(lcr.isSuccess());
        } catch (DmlException e) {
            System.debug('The following exception has occurred: ' + e.getMessage());
        }
    }

    public static void mergeLeadWithContact(Lead lead, Contact contact) {
        Database.LeadConvert lc = new Database.LeadConvert();
        lc.setLeadId(lead.Id);

        if(contact.AccountId != null) {
            lc.setConvertedStatus('Qualifié');
            lc.setDoNotCreateOpportunity(true);
            lc.setAccountId(contact.AccountId);
            lc.setOwnerId(contact.OwnerId);
            lc.setContactId(contact.Id);

            try {
                Database.LeadConvertResult lcr = Database.convertLead(lc);
                System.assert(lcr.isSuccess());
            } catch (DmlException e) {
                System.debug('The following exception has occurred: ' + e.getMessage());
            }
        }
    }
}
1

Debugging a CPU time issue is probably one of the more frustrating things to try to track down and resolve.

One thing to keep in mind here is that the line of code that your error points to isn't necessarily the problem, it's just where you happened to go over the CPU limit.

Measuring CPU time usage is more of a pain than, say, Query Rows (since each query that gets run tells you how many rows were returned). You can look at the USAGE_LIMITS statements in your debug log to get a rough idea of what's taking time to run, but beyond that I think you just need to add some System.debug(Limits.getCPUTime()); to track down where your issue lies.

It might be important to see your Utils class (at least the code and appropriate context for the methods that you're using) too, but I get the feeling that the issue is in how you're looping over records.

Code that looks like the following can be a red flag:

List<MyObject> otherRecords = [SELECT Id, Thing__c, etc... FROM MyObject];

for(TriggerObject triggerRec :trigger.new){
    for(MyObject otherRec :otherRecords){
        if(triggerRec.field == otherRec.field){
            // do work
        }
    }
}

If you have 200 records in trigger.new, and 400 records from the other query, you end up iterating 200 * 400 = 80,000 times. Even if the work you're doing in each iteration is small/fast, when you get into tens of thousands of iterations, it starts to add up.

The normal recommendation to get around that is to make use of collections, and iterating over the object you're trying to compare against in a loop outside of the loop over your other records.

List<MyObject> otherRecords = [SELECT Id, Thing__c, etc... FROM MyObject];

// The key of the map doesn't need to be an Id, but it's typically that or a string
// If you only care about the fact that such a value exists, and don't need to extract
//   data from a particular record, then a Set<String> would suffice.
Map<Id, MyObject> commonRelationshipToMyObj = new Map<Id, MyObject>();

for(MyObject myObj :otherRecords){
    commonRelationshipToMyObj.put(myObj.relationship__c, myObj);
}

for(TriggerObject triggerRec :trigger.new){
    if(commonRelationshipToMyObj.containsKey(triggerRec.relationToSameObject__c)){
        // do work
        // The specific MyObject record that shares the key value can be pulled from the
        //   map using .get() if you need it
}

With this approach, you only end up looping 200 + 400 = 600 times.

For your checks against Account, you can pretty much use the second code snippet as-is (make yourself a Map<Id, Account>, loop over Account records to populate that map). The call of your Utils class to check for the same Id could be removed. You can get the specific Account to pass to your other Utils method call through the map (if it exists).

The check against Contact records needs a little creativity to make the pattern we used for the Account checks work, but only just a little creativity.

While it's generally not recommended to use SObjects as the key in a Map or Set, doing just that is very helpful if you're trying to match multiple fields on an SObject (i.e. making something similar to a composite key).

The general idea is this:

// Setting up a Map<SObject, List<SObject>> here for safety
// If you're not working with Ids or some other unique identifier, it's possible that
//   you could have multiple records that have the same values you are trying to compare against
Map<MyObject1__c, List<MyObject1__c>> obj1CompositeKeyToMyObjects = new Map<MyObject1__c, List<MyObject1__c>>();

for(MyObject1__c myObj: myObjList){
    // We use the SObject constructor here because it's fast, convenient, and
    //   allows us to set the Id field (if we need to do that).
    // We aren't going to insert/update any of our partial objects, we're just using it as
    //   a convenient (and stupidly fast) way to do object comparison.
    // The idea here is that myObj contains more than just 2 fields, but we only want to
    //   do a comparison against these 2 fields
    // Using multiple fields in the "key" object is the same as matching field1 AND field2
    // If you want to compare field1 OR field 2, you'd make a partialObject with one field or the
    //   other populated
    MyObject1__c partialObject = new MyObject1__c(
        Field1__c = myObj.Field1__c,
        Field2__c = myObj.Field2__c
    );

    // Maps and Sets determine if an item exists in it already by hashing the data
    //   that you try to pass as the key.
    // For SObjects, this means every populated field (even if it's populated with null)
    //   gets included in the hash.
    // Part of why this is normally dangerous with SObjects is that doing DML or updating
    //   a single field after using it as a map key can make the values for that particular
    //   map key inaccessible.
    // As a side note, this pattern, checking if a key exists, and putting a new object into
    //   a map if they key doesn't yet exist, is a good way to populate maps
    if(!obj1CompositeKeyToMyObjects.containsKey(partialObject)){
        obj1CompositeKeyToMyObjects.put(partialObject, new List<MyObject1__c>());
    }

    // The reason why it's a good approach is because it allows us to use this one line
    //   to update the map value regardless of whether this is our first time encountering this
    //   particular map key or not.
    obj1CompositeKeyToMyObjects.get(partialObject).add(myObj);
}

for(OtherObject__c otherObj :trigger.new){
    // Here's the nice part.
    // If we can perfectly reconstruct the information we used to generate any key for
    //   obj1CompositeKeyToMyObjects, we can use .containsKey() to accomplish the same thing
    //   as explicitly comparing fields does.
    MyObject__c testKey = new MyObject__c(
        Field1__c = otherObj.FieldA__c,
        Field2__c = otherObj.FieldB__c
    );

    if(obj1CompositeKeyToMyObjects.containsKey(testKey)){
        // comparison succeeded, pull the list of matching values from the map
        //   and do whatever work you need to do
    }
}

Yeah, there's some extra hoops to jump through there, but the important point is that this allows you to avoid doing a bunch of extraneous looping when you don't have a unique identifier to use in a comparison (as is the case with your check against Contact records)

Bonus tips

There's an (arguably) easier way to get records from a specific period of time using Date literals.

Specifically, WHERE CreatedDate >= N_DAYS_AGO:30

N_DAYS_AGO might not be documented, but it is available. The closest documented date literal to that would be LAST_N_DAYS

You also might want to check your leadIsConverted = false towards the end of your code. It might undo the work you're trying to do (it would set all your leads to leadIsConverted = false)

1

There are a couple of possibilities that I can think of off the top of my head. The first is that your account and contact queries are returning a bunch of records and that's slowing it down. I also don't know what's happening in that Util class, so that may be running very inefficiently as well.

My first suggestion would be to figure out what's causing the problem. Try taking a look at the Log Inspector in developer console. It can help identify sections of code that are taking a long time to run and queries that are returning a lot of records or running more times than necessary.

Your other option would be to run this code asynchronously (if it doesn't need to happen real-time). You could either use a @future method or create a class that implements Queueable, depending on your needs.

My final recommendation would be to take a look at the best practices for triggers, particularly the idea of "logic-less triggers." That may not be the cause of your timeout, but it's going to make your code easier to maintain in the long run.

1

In addition to all the other great answers

  • You should filter out converted leads in the top level loop
  • The Utils.mergeLeadWithXXX has to be called with a list of items and at the end of the main loop - otherwise you will blow up DML statements in any batch operation. See Trailhead resources on bulkification
  • Once you have matched against an account or contact , you most likely can break out of the matching logic as you can't convert a single lead to more than one account+contact
0

To add to @JanetKang's answer:

You have nested for loops; these are pretty much a complete no-no in Apex, especially when there are a lot of items to iterate at both levels, as this is highly inefficient and rapidly uses up your CPU limit (as you are seeing).

You should look to remove the loop nesting by "pre-processing" the details from the nested iteration to construct a map (or maps) that you can simply query using the outer loop iteration detail. This is the typical way to "flatten" nested loops into a sequence of standalone loops.

I can't say what you would want to do in this case because you didn't include the Util method details.

As to making the processing asynchronous, it may be applicable to use a Batchable (rather than future or Queueable) since I note that the queries for Accounts and Contacts are actually unlimited - it may become important to consider splitting the data domain into chunks. This is made more complex by the need to query two different types of object (a batch typically iterates over instances of one object type, at least when using a query locator). However, perhaps the necessary refactoring to flatten the iteration would resolve this complexity.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.