2

Background & Data structure: please click here to data structure image

Each contact can have multiple tours/trips and each trip will have records in 3 custom objects (with a lookup relationship back to the Tours__c). Once a proof is uploaded as an attachment under Visa/Tickets/Hotel objects, I need to update the Status field on the parent object and only if it's an European Tour with exception of Poland and Albania, for example: when someone uploads their Visa document, the Status field on the Visa__c object needs to be set to 'To be reviewed' and so on. Similarly if an attachment associated with these object is deleted, I need to reset the Status field to a null. The tour destination field is in the Tours object.

Question:
We have a trigger (after insert and after delete) tacked onto the Attachment object that does that currently (using the ParentID field on the Attachment object) and to prepare for potential bulk transactions, I am executing a SOQL query one time each for the 3 objects (Visa, Tickets and Hotel). I have never really worked with SOSL in the past but from what I have read, it would not really make sense here because I already have the IDs of the records that needs to be updated but I am open to corrections. I know that would be the way to avoid potentially making multiple SOQL calls? Any advice/pointer would be greatly appreciated:

Code:
To keep this already long post concise, I have pared the code and inline documentation a bit:

public class AttachmentsTriggerClass {

    private Map<String, List<Id>> parentIds = new Map<String, List<Id>>{};

    //method that is gets called from the trigger   
    public void call(List<Attachment> listOfAttachments, String action) {

            /*some custom business logic happens here */    
            updateRequirementFlags(listOfAttachments, action);             
    }

    public void updateRequirementFlags(List<Attachment> listOfAttachments, String action) {
        List<sObject> rows = new List<sObject>();
        List<sObject> resultset;
        List<Id> idsToQuery;
        String query;
        String condition;

        getParentObjectNames(listOfAttachments);
        for (String objectName: parentIds.keySet()) {

            /* This is where I need advise on, right now I am executing SOQL query in a loop, yes, I know it's forbidden. */    
            resultset  = new List<sObject>();
            idsToQuery = (List<Id>) parentIds.get( objectName );
            if (!idsToQuery.isEmpty()) {
                condition  = ' WHERE ID IN :idsToQuery AND Tour__c != \'\' AND Tour__r.Type__c =\'European\' AND Tour__r.Destination__c NOT IN (\'Poland\',\'Albania\') ';
                query      = 'SELECT ID, Name, Tour__c FROM ' + objectName + condition;

                resultset = Database.Query(query);
                for (sObject row: resultset) {
                    if (action == 'Insert')  {
                        row.put('Status__c', 'To be reviewed');
                    } else if (action == 'Delete') {
                        row.put('Status__c', null);
                    }
                    rows.add(row);
                }   
            }
        }

        if (!rows.isEmpty()) {
            Database.update(rows);
        }
    }

    private void getParentObjectNames(List<Attachment> listOfAttch) {   

        Schema.SObjectType sObj;
        String objectName; 
        List<Id> listOfIds;
        for (Attachment att: (List<Attachment>) listOfAttch) 
    {
                sObj       = att.ParentId.getSObjectType();
                objectName = sObj.getDescribe().getName();
                listOfIds  = new List<Id>();

                if (parentIds.containsKey(objectName)) {
                        listOfIds = parentIds.get( objectName );
                }
                listOfIds.add(att.ParentId);
                parentIds.put(objectName, listOfIds);
        }
    }
}
  • The query bit, as mentioned by David, is perfectly fine. There is room for improvement in the code overall, though. For example, you could use Map<SObjectType, List<Id>> instead of Map<String, List<Id>>, which would shave off some of the lines of code you're currently using, and there's a few unnecessary IF statements, and some of the remaining code could be restructured. Also, you might want to make a "whitelist" of allowed objects, since attachments can be put on virtually any object. – sfdcfox Aug 4 '18 at 2:54
  • @sfdcfox, thank you for sharing your insights, I will rehash the code per your suggestions. Totally agree with your whitelist comment and that is actually already part of the original code, but I think I deleted that line (and some more) from my OP to keep it concise. – StayinHungryNFoolish Aug 6 '18 at 15:11
1

There is absolutely nothing wrong with executing SOQL in a loop that needs to be executed in a loop, and this is exactly one of those times. You need to source data from several sObjects based on specific Ids, and this is the right pattern to do that. SOSL won't achieve your objective, so it's not a concern here.

Using a total of 3 SOQL queries won't break most transactions unless the rest of the code running in that transaction is extremely heavyweight or unbulkified. I don't see any routes to further bulkify this code - a maximum of 3 SOQL and 1 DML is as good as you can get it, and you use guards to avoid wasting queries where not needed.

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.