Apex strange exception handling

Question

I have a problem with my trigger. In Sandbox, everything works fine. When I deploy it to production, I receive multiple exceptions - which is really strange, because I got angry and put the whole code in "try" block. When I put everything in "try" block, it shouldn't be possible not to catch an exception!

This is how my code looks:

trigger UpdateLeadField on Lead (before insert, before update) {

    //debugging in progress
    try{
        if(Trigger.New[0].SomeField__c==NULL)
        {
            for(Lead obj: trigger.new)
                obj.SomeField__c = obj.OwnerId;
            return;
        }
    
        if (Trigger.New[0].OwnerId != Trigger.Old[0].OwnerId) 
        {
            for(Lead obj: trigger.new)
            {
                if(Trigger.New[0].SomeField__c==NULL || [SELECT Name FROM UserRole Where Id = :[SELECT Id, UserRoleId FROM User WHERE Id = :obj.OwnerId][0].UserRoleId].Name == 'Some role')
                obj.SomeField__c = obj.OwnerId;
            }
        }
    }
    catch (Exception e)
    {
        System.debug(e);
    }
}

The main problem seems to be with Web-to-Lead - When deployed, it stops working. The exception:

Apex script unhandled trigger exception by user/organization: 005C00000XXXXXX/00XXXXX0000LXXQ

UpdateLeadField: data changed by trigger for field Some field: id value of incorrect type: 00GC00000XXXXXqMAC

"Some field" is a lookup field.

Answer 1

Exception Cause

The key issue that is causing that exception is that you are trying to assign a Group Id to a User Lookup. You need to make sure it is a User Id first.

if (obj.OwnerId.getSObjectType() == User.sObjectType) obj.SomeField__c = obj.OwnerId;

Why It Wasn't Caught

The line where you assign the OwnerId does not cause an Exception. It causes the transaction to be invalid. Wherever you perform this update from is now throwing a DmlException. If you were doing the following it would be caught:

try
{
    someLead.OwnerId = someGroupId;
    update someLead;
}
catch (DmlException dmx)
{
    // do stuff
}

However the following causes no exception to be thrown:

try
{
    someLead.OwnerId = someGroup.Id; // valid
}
catch (Exception pokemon) // gotta catch 'em all!
{
    // do stuff
}

But as I explain below, getting frustrated and putting everything in a try/catch block is a really bad approach to solving this kind of problem, especially if you don't take an action in your catch block. It might hide it, but it will never solve it and will almost certainly make things worse down the line.

Other Areas For Improvement

You have a few code quality issues. For instance this pattern...

try
{
    // do several complex operations
}
catch (Exception pokemon) // gotta catch 'em all!
{
    // debug
}

is a recipe for headaches and confusion. First of all, you should always know a specific type of Exception you are expecting. In this instance, it should probably be DmlException. Second, you are failing silently, and unit testing against this pattern has a high chance of passing when it should actually fail.

Another issue is that you have a query (or two) in a for loop:

SELECT Name FROM UserRole Where Id = :
[SELECT Id, UserRoleId FROM User WHERE Id = :obj.OwnerId][0].UserRoleId]

You should move these outside the loop, and probably not have nested queries in that way.

Set<Id> ownerIds = new Set<Id>();
for (Lead record : trigger.new) ownerIds.add(record.OwnerId);
Map<Id, User> userMap = new Map<Id, User>([
    SELECT UserRole.Name FROM User
    WHERE Id IN :ownerIds
]);
for (Lead record : trigger.new)
{
    User owner = userMap.get(record.OwnerId);
    if (owner != null && owner.UserRole.Name == 'Some Role')
    {
        // do stuff
    }
}

A third problem with your code is that you are checking conditions on trigger.new[0], then acting on the whole collection. This is going to lead to very inconsistent behavior. You should apply a filter to the whole collection, then act in a certain way on that subset. For example, each record without Somefield__c populated should have X happen, each record where the OwnerId changed should have Y happen.

One last recommendation, move your functionality to static methods in an Apex Class. It is not the best idea to have a bunch of code in your triggers. For one thing, it makes the functionality much more difficult to test. If you had the following it would be much easier to asses the functionality in a unit test:

public with sharing class LeadServices
{
    public static void populateSomeField(List<Lead> leads)
    {
        for (Lead lead : leads) lead.SomeField__c = lead.OwnerId;
    }
    public static List<Lead> filterLeadsToPopulateSomeField
        (List<Lead> newRecords, Map<Id, Lead> oldMap)
    {
        List<Lead> toPopulate = new List<Lead>();
        for (Lead lead : newRecords)
        {
            if (!isOwnedByUser(lead)) continue;
            else if (lead.SomeField__c == null) toPopulate.add(lead);
            else if (lead.OwnerId != oldMap.get(lead.Id).OwnerId) toPopulate.add(lead);
        }
        return toPopulate;
    }
    public static Boolean isOwnedByUser(Lead lead)
    {
        return lead.OwnerId != null &&
            lead.OwnerId.getSObjectType() == User.sObjectType;
    }
}