Apex Trigger error when updating child record

Question

I'm very new to triggers.

The following trigger updates a custom field in a custom object (Events__c) with the sum of all "TotalPrice" from a related object (OpportunityLineItem).

I'm getting the following error:

Error:Apex trigger OppTrigger caused an unexpected exception, contact your administrator: OppTrigger: execution of AfterUpdate caused by: System.DmlException: Update failed. First exception on row 0; first error: MISSING_ARGUMENT, Id not specified in an update call: []: Class.OpportunityHandler.aggregateResults: line 73, column 1

Here is the Trigger:

trigger OppTrigger on OpportunityLineItem (after delete,after update,
after insert, after undelete) {
    if(trigger.isInsert)
    {
        OpportunityHandler oppHandler =new OpportunityHandler();
        oppHandler.oppInsert(Trigger.New); 
    }
    else if(trigger.isDelete)
    {
        OpportunityHandler oppHandler =new OpportunityHandler();
        oppHandler.oppDelete(Trigger.Old);
    }
    else if(trigger.isUnDelete)
    {
        OpportunityHandler oppHandler =new OpportunityHandler();
        oppHandler.oppundelete(Trigger.New);
    }
    else if(trigger.isUpdate)
    {
        OpportunityHandler oppHandler =new OpportunityHandler();
        oppHandler.oppUpdate(Trigger.New,Trigger.Oldmap);
    }

}

Here is the Class:

public class OpportunityHandler
{
    list<Events__c> oppList = new list<Events__c>();
    set<id> oppIDs = new set<id>();


    public void oppInsert(List<OpportunityLineItem> oppLists)
    {
        for(OpportunityLineItem opp : oppLists)
        {
            if(opp.Events__c!=null)
            {
                oppIDs.add(opp.Events__c);
            }
        }
        aggregateResults();
    }

    public void oppUpdate(List<OpportunityLineItem> oppLists,Map<Id,OpportunityLineItem> oppOldmap)
    {
        for(OpportunityLineItem opp : oppLists)
        {
            if(opp.Events__c!=null)
            {
                oppIDs.add(opp.Events__c);
                if(oppOldmap.get(opp.id).Events__c!=opp.Events__c)
                {
                    oppIDs.add(oppOldmap.get(opp.id).Events__c);
                }
            }

        }
        aggregateResults();

    }

    public void oppDelete(List<OpportunityLineItem> oppLists)
    {
        for(OpportunityLineItem opp : oppLists)
        {
            if(opp.Events__c!=null)
            {
                oppIDs.add(opp.Events__c);
            }
        }
        aggregateResults();
    }
    public void oppundelete(List<OpportunityLineItem> oppLists)
    {
        for(OpportunityLineItem opp : oppLists)
        {
            if(opp.Events__c!=null)
            {
                oppIDs.add(opp.Events__c);
            }
        }
        aggregateResults();
    }

    public void aggregateResults()
    {
        AggregateResult[] groupedResults = [SELECT SUM(TotalPrice), Events__c FROM OpportunityLineItem where Events__c IN :oppIDs GROUP BY Events__c];
        for(AggregateResult ar:groupedResults) 
        {
            Id oppId = (ID)ar.get('Events__c');
            Double sum = (DOUBLE)ar.get('expr0');
            Events__c opp1 = new Events__c(Id=oppId);
            opp1.Confirmed__c= sum;
            oppList.add(opp1);
        }
            if(oppList.size()>0 && oppList!=null)
            {
                update oppList;
            }
    }
}

Answer 1

Your code looks mostly fine to me. I think the only way you could get this error is if you somehow had a null in your oppIds set.

I do see that you're doing null checks before adding an id into your set, but there's one line where you're not doing a null check.

In the following code:

public void oppUpdate(List<OpportunityLineItem> oppLists,Map<Id,OpportunityLineItem> oppOldmap)
{
    for(OpportunityLineItem opp : oppLists)
    {
        if(opp.Events__c!=null)
        {
            // opp.Events__c is null checked
            oppIDs.add(opp.Events__c);
            if(oppOldmap.get(opp.id).Events__c!=opp.Events__c)
            {
                // but oppOldMap's Events__c is _not_ null checked
                oppIDs.add(oppOldmap.get(opp.id).Events__c);
            }
        }

    }
    aggregateResults();

}

The easy way around this is to take advantage of the remove() method of the Set class. Instead of checking for nulls in a loop, just add everything, and then remove the null (if one is in the set).

public void oppUpdate(List<OpportunityLineItem> oppLists,Map<Id,OpportunityLineItem> oppOldmap)
{
    for(OpportunityLineItem opp : oppLists)
    {
        oppIDs.add(opp.Events__c);
        oppIDs.add(oppOldmap.get(opp.id).Events__c);
    }

    // We can just remove nulls (well, max of 1 null since it's a set) after the loop
    // This is marginally faster than evaluating IFs in a loop, but this change isn't work it
    //   for that speedup.
    // The real gain here is the fewer lines of code and increased readability/reliability
    oppIDs.remove(null);

    aggregateResults();
}

You can apply the same pattern to your other methods too.