0

Caviat: I'm very new to apex. This is only the 4th trigger I've written.

Basically, the code updates records' lookup fields with the id of the Opportunity being updated by the user. And it works great, 99% of the time. This has been running just fine for weeks until yesterday, when a user sent me this error, which I can easily replicate. I get this error message when trying to update an opportunity with a value that references 117 soon to be child records.

Full Error Message:

Error:Apex trigger putOppIdOnSalesOrder2 caused an unexpected exception, contact your administrator: putOppIdOnSalesOrder2: System.LimitException: Too many query rows: 50001

The apex code:

//after opp update of cilio, triggers update of sales orders
trigger putOppIdOnSalesOrder2 on Opportunity (after update) {

    for (Opportunity PrNoId : trigger.new){

        if (PrNoId.cilioproject__c!=Null && 
        PrNoId.cilioproject__c!=''){

            //selects the cilio# from the opportunity being updated and puts it in a set
            Set<String> OP = new set<string>();
            for (Opportunity o : trigger.new) OP.add(o.cilioproject__c);

            //makes a list of sales_orders that have a project_no that matches cilio#
            List<SAP_Sales_Order__C> SO = [select SAP_Project_no__c from SAP_Sales_Order__C
            Where SAP_Sales_Order__C.SAP_Project_no__c IN : OP];

            //updates the list of sales orders' lookup field with the id of the opportunity being updated
            //Project__c is the lookup field    
            for (SAP_Sales_Order__c Pno : SO){
                Pno.Project__c=PrNoId.id;
                update (Pno);           
            }
        }
        //if the cilio number is deleted, then this code erases the id off the associated sales orders' lookup field
        else {

            Set<string> codes = new set<string>();
            for(Opportunity trip: trigger.new) codes.add(trip.Id);

            List<SAP_Sales_Order__c> ProjNo = [SELECT Project__c FROM SAP_Sales_Order__c
            WHERE SAP_Sales_Order__C.Project__c IN : codes
            ]; 

            for (SAP_Sales_Order__c rem : ProjNo){
                rem.Project__c=Null;
                update (rem);
            }

        }
    }
}

Error happens when: The user is trying to input a cilioproject__c number on an Opportunity for which there are 117 SAP_Sales_order__c records with the SAP_Sales_Order__c.SAP_Project_No__c that equals the Opportunity.Cilioproject__c. This means that the list of SAP_Sales_order__c records in the trigger should only be 117 long, and i shouldn't be anywhere close to hitting governor limits. There are 80k+ sales orders our org, and the trigger works great for many (all) other cilio#'s.

EDIT: I've removed the DML statement from the loop, as suggested, and an now just updating the list.

        //makes a list of sales_orders that have a project_no that matches cilio#
        List<SAP_Sales_Order__C> SO = [select SAP_Project_no__c from SAP_Sales_Order__C
        Where SAP_Sales_Order__C.SAP_Project_no__c IN : OP];

        //updates the list of sales orders' lookup field with the id of the opportunity being updated
        //Project__c is the lookup field    
        for (SAP_Sales_Order__c Pno : SO){
            Pno.Project__c=PrNoId.id;
        //removed DML statement from this loop
        }
        update (SO);

This is working in sandbox and is now updating 100's of records as needed.

  • 1
    Are you sure the error is not Too many SOQL queries? Because there is a big problem about your update DML statements. There are in for loops. Also, are you sure there is not other triggers fired at the same time? – Martin Lezer Sep 26 '17 at 14:25
  • As I understand it, the update DML statements are only executing for the records specified in the loop. At most, that would be 117 records, as that's the largest number of SAP_Sales_order__c that share a cilioproject__c value. I'm sorry, but I don't see the "big problem". – Brandon Hurley Sep 26 '17 at 14:32
  • The big problem is that the limit is 100 queries max. – Martin Lezer Sep 26 '17 at 14:34
  • I would dig in the first question of Martin Lezer : are there no other triggers, ie on SAP_Sales_Order__c ? Also, the error is it happening while adding the single record (from salesforce UI), or this record among others (like in code or data loader) ? Finally, you should definitely add a few System.debug(everyList.size()) after every query, and examine logs. They will tell you where your problem is. Ah, and listen to Martin, put this update out of the loop ;) – Emmanuel BRUNO Sep 26 '17 at 14:38
  • Sorry for my confusion, but how do i update the records if I remove the DML from the loop? How do I update 100+ records without hitting the governor limits? – Brandon Hurley Sep 26 '17 at 14:48
4

As people have mentioned in the comments already, I think the big issue here is that your trigger isn't what Salesforce calls "Bulk Friendly". "bulkifying" code pretty much boils down to making sure you don't have queries (SOQL or SOSL) or DML inside of loops.

It's hard to say what, precisely, is causing your error here (and why you aren't running into the DML or Query limit first) because there are other factors that can affect the outcome (Process Builder and Triggers don't mix well, is there a trigger on SAP_Sales_Order__c?, what other triggers might be run?, workflow rules with field updates cause update triggers to run [potentially for a second time], etc...).

At any rate though, what will help is to bulkify your trigger. The general 'bulk' pattern involves using collections (Sets, Lists, and Maps), and usually looks like this:

trigger MyTrigger on MyObject__c (before update){
    // When you need to query something, it's a good idea to gather all of the data 
    //   you'll be using in your WHERE clause ahead of time.
    // We do that by declaring a List (a Set would work too), and iterating over data
    //   that we already have.
    // In this loop, we're only concerned about extracting data for use in our query.
    List<Id> topLevelIds = new List<Id>();

    for(MyObject__c myRec :Trigger.new){
        // Note that if your 'someField__c' is really the Id field of the object
        //   you're iterating over, you don't need this loop (or the List<Id>
        //   for that matter) because that information is available through
        //   Trigger.newMap.keySet() (on all update triggers, after insert triggers,
        // and after undelete triggers)

        // This approach (iterating over trigger.new and storing field information)
        //   is useful when, for example, someField__c points to a common record of
        //   a third object (say, TopLevel__c) that both MyObject__c and ChildObject__c
        //   have a relationship with.
        topLevelIds.add(myRec.someField__c);
    }

    // Yes, looping over Trigger.new incurs some overhead, but we have a _lot_ more
    // CPU time to work with than we do queries.
    // Also, the above loop is 'tight'.
    // There isn't much happening in it, and the things that are happening are fast
    //   operations.

    // Now that we've gathered our information, we can query once to gather all the 
    //   supplemental data we need.
    List<ChildObject__c> relatedData = [SELECT Id, special_field__c FROM ChildObject__c WHERE TopLevel__c IN :topLevelIds];

    // If the field you're filtering your query on matches the Ids of the records
    //   in your trigger, as is the case with child records (lookup or master-detail),
    //   you could simply use trigger.newMap.keySet() directly in your query.
    //List<ChildObject__c> relatedData = [SELECT Id, special_field__c FROM ChildObject__c WHERE Parent_Lookup__c IN :Trigger.newMap.keySet()];

    // Now, if we want to update records, the thing to do is to store the changes
    //   we're making in-memory in another collection.
    // After we're done making changes, we can DML update all the records we changed
    //   in one shot.
    List<OtherObject__c> otherObjsToUpdate = new List<OtherObject__c>();

    for(OtherObject__c otherObj :relatedData){
        MyObject__c parentRecord = Trigger.newMap.get(otherObj.MyObject_Lookup__c);

        if(parentRecord.some_field__c == 'some value'){
            otherObj.different_field__c = 'different value';
            otherObjsToUpdate.add(otherObj);
        }
    }

    // Now that we've made all of our changes, it's safe to update our records.
    update otherObjsToUpdate;
}

Let's apply that to your trigger

trigger putOppIdOnSalesOrder2 on Opportunity (after update) {

    // First, some commentary on your existing trigger
    /*
    for (Opportunity PrNoId : trigger.new){

        if (PrNoId.cilioproject__c!=Null && 
        PrNoId.cilioproject__c!=''){
            Set<String> OP = new set<string>();

            // It's a good idea to always include curly braces, even if you can
            //   omit them.
            // It's very easy to miss that something is part of a loop or an
            //   if/else otherwise.
            // On top of that, you're already iterating over trigger.new.
            // Iterating over the same thing twice is wasteful.
            for (Opportunity o : trigger.new){
                OP.add(o.cilioproject__c);
            }

            // Queries in loops are bad.
            // This one will be easy to extract out of the loop
            List<SAP_Sales_Order__C> SO = [select SAP_Project_no__c from SAP_Sales_Order__C
            Where SAP_Sales_Order__C.SAP_Project_no__c IN : OP];

            //updates the list of sales orders' lookup field with the id of the opportunity being updated
            //Project__c is the lookup field    
            for (SAP_Sales_Order__c Pno : SO){
                Pno.Project__c=PrNoId.id;

                // DML inside of a loop (a nested loop, at that) is bad.
                // Having a collection declared/initialized outside of the loop
                //   is the way to go.
                update (Pno);
            }
        }
        else {

            Set<string> codes = new set<string>();
            // Again, you have the right idea, adding Ids to a set.
            // Easy enough to extract out of the outer loop.
            for(Opportunity trip: trigger.new){
                codes.add(trip.Id);
            }

            // Another query in a loop, another query that's easy to extract
            List<SAP_Sales_Order__c> ProjNo = [SELECT Project__c FROM SAP_Sales_Order__c
            WHERE SAP_Sales_Order__C.Project__c IN : codes
            ]; 

            for (SAP_Sales_Order__c rem : ProjNo){
                rem.Project__c=Null;

                // Again, putting rem into a List<SAP_Sales_Order__c> and doing
                //   the DML outside of all loops is the way to go.
                update (rem);
            }

        }
    }*/

    // Begin bulkified code
    // I can't discern precisely what your trigger is actually trying to accomplish.
    // What follows is my best guess.

    // Declaring/initializing collections outside of loops.
    Set<String> OP = new set<string>();
    Set<string> codes = new set<string>();

    // We can populate both of your collections with a single loop
    for(Opportunity trip: trigger.new){
        if (trip.cilioproject__c!=Null && PrNoId.cilioproject__c!=''){
            OP.add(trip.cilioproject__c);
        } else {
            codes.add(trip.Id);
        }
    }

    // Query outside of loops
    // There's no need to prefix fields of SAP_Sales_Order__c in a query on
    //   'SAP_Sales_Order__c'
    List<SAP_Sales_Order__C> SO = [SELECT SAP_Project_no__c FROM SAP_Sales_Order__C WHERE SAP_Project_no__c IN :OP];
    List<SAP_Sales_Order__c> ProjNo = [SELECT Project__c FROM SAP_Sales_Order__c WHERE Project__c IN :codes];

    // Declare collection(s) to hold the updates to the records so we can update
    //   in a single shot.
    List<SAP_Sales_Order__c> soRecsToSetProject = List<SAP_Sales_Order__c>();
    List<SAP_Sales_Order__c> soRecsToResetProject = List<SAP_Sales_Order__c>();

    // This is where your trigger starts not making sense to me.
    // Your original trigger was updating each Sales Order multiple times (one
    //   time per Opportunity being updated).
    // If you had code that actually did something with that, you should reconsider
    //   your approach.
    // If you don't have code that made use of each new Project__c value, then
    //   the code I have here is equivalent to what you had.
    for(SAP_Sales_Order__c so :SO){
        so.Project__c = trigger.new[trigger.new.size()].Id;
        soRecsToSetProject.add(so);
    }

    // Is there any overlap between the Sales Orders in this list and the ones in
    //   your other List (SO)?
    // If so, that's another indication that you should reconsider your approach
    //   to solving whatever problem you're trying to solve.
    for(SAP_Sales_Order__c so :ProjNo){
        so.Project__c = null;
        soRecsToResetProject.add(so);
    }

    // DML outside of loops
    update soRecsToSetProject;
    update soRecsToResetProject;
}
  • No need to loop over the trigger.new use newMap.keySet() in your query – Eric Sep 26 '17 at 18:34
  • @Eric well, for the second query at least. Good catch, I think I'll update my answer. – Derek F Sep 26 '17 at 19:10
  • Actually, now that I look at this again, the loop over trigger.new is likely required, because OP doesn't want to null out the Project__c field of all the Sales Orders, just the ones where the related Opp's cilioproject__c field is null/empty...at least that's how I'm interpreting the code w/OP's comments. – Derek F Sep 26 '17 at 19:24
  • All you are doing is adding Ids of the records in trigger. First loop is not required. You could simply use the key set to populate if you need a separate list – Eric Sep 26 '17 at 19:26
  • @Eric OP is only using part of Trigger.newMap.keySet() for only one of the two queries. The other query is driven by Opportunity.cilioproject__c, which does necessitate a loop. – Derek F Sep 26 '17 at 19:32

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.