3

I'm just starting out in apex. I want to create a trigger that counts the number of attachments in a record of a custom object. I came across an example here. Here's the trigger code:

trigger countattachment on Attachment (after insert, after update, after delete, after undelete) {
  Map<Id,List<Attachment>> parent = new Map<Id,List<Attachment>>();
  set<id> attids = new set<id>();

   if(Trigger.new<>null){
       for(Attachment c:Trigger.new){
           TestObject__c l;
           if(c.ParentId != null)
               attids.add(c.parentid);
       }

   }else if(Trigger.old != null){
       for(Attachment c:Trigger.old){
           if(c.ParentId<>null)      
               attids.add(Trigger.oldMap.get(c.id).parentid);
       }
   }
   if(attids.size()>0){
       try{
           List<Attachment> a = new List<Attachment>();
           Map<id,TestObject__c> testmap = new Map<id,TestObject__c>([select id,CountAttachment__c from TestObject__c where id IN: attids]);
           a = [select id,parentid from Attachment where parentid IN:attids];

           for(Attachment at: a){
               List<Attachment> llist = new List<Attachment>();
               if(parent.get(at.parentid) == null){
                   llist = new List<Attachment>();
                   llist.add(at);
                   parent.put(at.parentid,llist);
               }else if(parent.get(at.parentid) != null){
                   llist = new List<Attachment>();
                   llist = parent.get(at.parentid);
                   llist.add(at);
                   parent.put(at.parentid,llist);
               }
           }

           for(Id i: attids){
               if(testmap.get(i) != null && parent.get(i) != null){
                  testmap.get(i).CountAttachment__c = parent.get(i).size(); 

               }else if(testmap.get(i) != null && parent.get(i) == null){
                  testmap.get(i).CountAttachment__c = 0; 
               }
           }

           update testmap.values();
           System.Debug(testmap.values());
       }catch(Exception e){}
    }

}

Things I don't get:

  1. In the first part, the attid is being populated with the id of parent objects linked with the new attachments list in Trigger.new. But I don't get why we are using Trigger.old to get the parentids of the attachments in the list. Besides it's used only in case the Trigger.new is null. Is is for the case when an attachment is deleted?
  2. Also, the way ids are extracted in case of trigger.new(attids.add(c.parentid);) and trigger.old(attids.add(Trigger.oldMap.get(c.id).parentid);) is pretty different. Couldn't we have just inserted the parentid directly like we did for trigger.new?
  3. In line 7, I see that variable l of type testObject__c is not being used. Is it unnecessary?

I do understand the remaining part of the code. First a map(called parent) mapping the parent object ids with the list of total attachments linked to respective parent objects. The map only has those parentid that are linked with the attachments in attid. Then depending on the value mapped in parent, the count_attachment__c field is either assigned the size of the list or assigned 0. I assume the case of assigning 0 to the count__attachment__c occurs in case of delete action.

3

Wow, that code you found is...not good (putting it mildly). For someone who claims to just be starting out with Apex, you did a fine job analyzing that code.

For your first question, Pranay nailed it on the head. We need to use trigger.old because trigger.new is null in delete trigger contexts.

Your suggestions for your second and third questions are also correct.

I'd now like to spend some time showing you a much better implementation. You can probably use Andrew Fawcett's Declarative Lookup Rollup Summaries to do this without the need to write code yourself. The code I'll end up writing will likely bear some resemblance to the code generated by the DLRS tool.

For most people's use cases, the DLRS tool is sufficient. I, however, like to know/understand what's going on. So, with that, let's get to the code.

trigger countattachment on Attachment (after insert, after update, after delete, after undelete) {
    // First step, we need to gather the parentIds that appear in the Attachment records
    //   taking part in this invocation of the trigger.

    // I prefer using a Set over a List when gathering data that can potentially be null.
    // The reason for that is that a Set will allow us to remove a null value without the need
    //   to check for null inside the loop.
    // Honestly, this is a micro-optimization that will likely never make any appreciable
    //   difference.
    Set<Id> parentIdsSet = new Set<Id>();

    // Generally speaking, DRY (Don't Repeat Yourself) code is better than WET (Write Everything Twice)
    //   code.
    // Instead of duplicating the loop to iterate over trigger context variables, we can gather
    //   everything into one list, and write just a single loop.
    List<Attachment> attachmentList = new List<Attachment>();
    if(trigger.new  != null){
        attachmentList.addAll(trigger.new);
    }else if(trigger.old != null){
        attachmentList.addAll(trigger.old);
    }

    for(Attachment att :attachmentList){
        parentIdsSet.add(att.ParentId);
    }

    // For Attachments, a parentId is required.
    // We won't find any nulls in parentIdsSet, but if you apply this to another object
    //   there may be nulls
    parentIdsSet.remove(null);

    // Second step, count the number of attachments for each parentId.
    // We can use COUNT() and GROUP BY to have SOQL do the heavy lifting here, which
    //   means we don't need to write as much code
    Map<Id, Integer> objecttIdToAttachmentCount = new Map<Id, Integer>();
    for(AggregateResult ar :[SELECT COUNT(Id) attachmentCount, ParentId FROM Attachment WHERE ParentId IN :parentIdsSet GROUP BY ParentId]){
        // Using aggregate functions like COUNT(), SUM(), AVG(), etc... means the result of the 
        //   query will be an AggregateResult
        // AggregateResult requires us to use get() to fetch field values, which returns
        //   a plain 'ol Object (that we need to explicitly type-cast)
        Id objId = (Id)ar.get('ParentId');

        Integer count = (Integer)ar.get('attachmentCount');
        // This shouldn't be an issue with COUNT(), but if you're using this pattern to 
        //   sum a field value from an SObject (e.g. Amount from Opportunity) there is
        //   the possibility that the SUM() function will return null (if all values that you
        //   sum are null)
        // This quick ternary is a safeguard against null values.
        // Basically just a shorter version of an if/else
        count = count == null ? 0 : count;

        objectIdToAttachmentCount.put(objId, count);
    }

    // One of the gotchas with this pattern is that, if you delete the last record for a given parent,
    //   your resulting map won't have an entry for that parent (because no more records
    //   for a parent means that parentId won't appear in the query results).
    // To cause those parents to have their count updated to zero, we need another loop.
    for(Id parentId :parentIdsSet){
        if(!objectIdToAttachmentCount.containsKey(parentId)){
            objectIdToAttachmentCount.put(parentId, 0);
        }
    }

    // Third step, updating the relevant records.

    // The ParentId field on Attachment is special because it can point to one of a handful
    //   of SObjects (as opposed to a normal Master-Detail or Lookup relationship which is
    //   only ever pointing to a single SObject type).
    // Thus, we need to exercise more caution than usual.


    // To allow for (easy) future expansion of this count to other objects, we create a map
    //   that tells us which field to update for each type of object.
    // This could be made into a Custom Setting or Custom Metadata Type instead to allow
    //   you to change which SObjects are updated without the need to modify code / deploy
    Map<String, String> sobjectNameToTargetField = new Map<String, String>{
        // You could just put 'Account' here instead of going through the describe information
        //  ...but doing things this way is marginally safer since it gives us
        //  a static type reference that the compiler can check for us.
        Account.SObjectType.getDescribe().getName() => 'attachment_count__c',
        Opportunity.SObjectType.getDescribe().getName() => 'other_count_field__c'
    }

    List<SObject> parentRecordsToUpdate = new List<SObject>();

    // In this case, we want to iterate over the keyset of the map so that we can
    //   access both the Id and the count value that we want to update;
    for(Id parentId :objectIdToAttachmentCount.keySet()){
        // The Id class gives us a getSObjectType() method, which allows us to 
        //   pull our data from the map (or custom setting/custom metadata type) that
        //   we defined above.
        // It also allows us to generate an SObject instance.
        // Working with multiple SObjects at once (Account, Opportunity, etc...) almost
        //   always means you'll need to work with the more generic "SObject" type
        SObjectType currentType = parentId.getSObjectType();

        // If we don't find the current SObject type in our map, we can simply ignore this id
        if(!sobjecttNameToTargetField.containsKey(currentType.getDescribe().getName()){
          continue; // skips the rest of the code for this iteration of the loop
        }

        SObject parent = currentType.newSObject(parentId);

        // When working with SObjects, getting/setting fields is done using .get()
        //   and .put() (just like if the SObject were a Map)
        parent.put(sobjectNameToTargetField.get(currentType.getDescribe().getName(), objectIdToAttachmentCount.get(parentId));

        parentRecordsToUpdate.add(parent);
    }

    // One last gotcha
    // We can perform DML on a List<SObject> that contains more than one type of object.
    // However, we can only do this if there are 10 or fewer different SObject types in the list.
    // Furthermore, the list can only switch between different SObject types a maximum of 10 times.
    // A List<SObject>{Account, Opportunity, Account, Opportunity, Account, Opportunity, Account, 
    //   Opportunity, Account, Opportunity, Account, Opportunity} will generate an error
    //   if you try to perform DML on it.
    // A List<SObject>{Account, Account, Account, Account, Account, Account, Opportunity,
    //   Opportunity, Opportunity, Opportunity, Opportunity, Opportunity}, on the other
    //   hand, would be perfectly fine.
    // We can guarantee that SObject types are grouped together instead of spread out in the list
    //   by calling list.sort()
    parentRecordsToUpdate.sort();

    update parentRecordsToUpdate;
}

45 lines of code (from the blog you found) compared to 41 lines of code (counting lines with just open/close braces, not counting completely blank lines or comments), and we had a considerable amount of extra work that needed to be done. If this were a more "normal" rollup, my code would have been a good deal shorter.

Like I mentioned, there are still improvements that could be made (like using a custom setting to define/store the mapping between SObjects you want to consider and the fields to roll up to), but this approach is miles ahead of the one you found.

Hope you can learn a lot from this.

  • Your comments made it really easy to understand the code. Thank you. – Aanchal Adhikari Jan 21 at 7:51
  • I still don't get the reason behind using Account.SObjectType.getDescribe().getName() instead of just 'Account'. Could you help? – Aanchal Adhikari Jan 21 at 8:54
  • It would be Soft coding, instead of Account if there was a custom object and you tried deleting it it will allow you to delete that object even if it was referenced in any code via 'Custom_Object__c' format – Pranay Jaiswal Jan 21 at 9:29
  • I implemented the apex trigger provided above but I having an issue. Even when I attach pdf files to a record corresponding to the object where I want to count attachments, the apex trigger is not firing. I'm attaching the files in the notes and attachments section. I googled and found that it might be a bug related to the Atachment sObject. Is is still an issue? – Aanchal Adhikari Jan 22 at 3:56
  • I also have another issue. If I try using the function newInstance to create a new instance of the object related to the parentId, I get an error saying the method doesn't exist. Can I use the newSObject instead? – Aanchal Adhikari Jan 22 at 7:32
3

1) Why Trigger.old is used?

A) Your trigger is written on insert, update,delete, undelete. The Thing is Trigger.new is only available in insert, update, and undelete triggers, so when a file is deleted Trigger.new will be null. You still have to update attachment count on deleted attachment's parent record. Thus here Trigger.old come in picture. It will have parent id of deleted attachment. Check trigger context variable doc here

2)Why attids.add(Trigger.oldMap.get(c.id).parentid) used?

A) Well, there is no need of it, you could directly use attids.add(c.parentid); as you get c from Trigger.old. Trigger.oldMap and Trigger.old have same values. One is in list format and other is in Map.

3) Why testObject__c l on Line 7 used?

A) It's unnecessary

The code is written here works, but its bit confusing and can be optimized further. Also the code is using for loop to count the size which is just overkill of CPU time.

If you use Aggregate Queries, the code will be faster as you are using database to tell you the count rather than counting urself in for loops.

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.