Custom Save on New/Edit page - Is this valid code?

Question

In my controller extension I have a custom save method that

• works for New and Edit scenarios
• also saves modified child records

The below code works in all unit tests but starts to show strange behavior in customer orgs (duplicate values, workflow errors).

private void save() {
    // Note: Provide ACID behavior
    Savepoint toBeforeState = Database.setSavepoint();

    try {
        upsert record;
        saveChildrenOfTypeA();
        saveChildrenOfTypeB();
    }
    catch(Exception ex) {
        ApexPages.addMessages(ex);
        Database.rollback(toBeforeState);

        // Note: See http://salesforce.stackexchange.com/questions/57020/data-not-available-when-using-database-rollback-in-custom-save-action
        removeInvalidIds();

        throw ex;
    }
}

It's just a feeling, but from browsing the web, I think maybe

1. The use of upsert instead of insert and update
2. The try/catch with the database rollback
3. The re-throwing of the exception

is wrong.

What do you think?!

Answer 1

I think something like this might make it easier to debug where your problem lies.

private void save() {
    // Note: Provide ACID behavior
    Savepoint toBeforeState = Database.setSavepoint();

    string failedIn = '';
    string error = '';
    try {
        failedIn = 'Parent';
        upsert record;
        failedIn = 'TypeA';
        saveChildrenOfTypeA();
        failedIn = 'TypeB';
        saveChildrenOfTypeB();
        failedIn = null;
    }
    catch(Exception ex) {
        error = ex.getMessage();
    }

    if(failedIn != null)
    {
        ApexPages.addMessages('Failed In:' +failedIn+' Error:'+error);
        Database.rollback(toBeforeState);

        // Note: See http://salesforce.stackexchange.com/questions/57020/data-not-available-when-using-database-rollback-in-custom-save-action
        removeInvalidIds();
    }
}

That way you can make sure that the problem is not the try catch or the fact that you are throwing an exception.

Good luck!

Luis Luciani

Edit:

Sorry for the confusion. Let me try to explain myself.

You need both features. The try/catch will prevent the execution from stopping and throwing an ugly error as soon as one is encountered. The savepoint and rollback you need so that you do not leave any half processed changes on the DB. If you were not using the try/catch, then you would not need the savepoints, because the platform will handle the rollback for you, even if the first change succeeded and a subsequent one failed.

In conclusion, seems like you have 2 options. Get rid of the try/catch and the savepoints, or keep both and do some debugging to determine why you are ending up with dupes (In which case I would start with the code provided above).

Hope that helps.

Answer 2

I believe this is the type of thing that you need to do which I was trying to explain in comments and apparently not doing a very good job of doing. The database.upsert(list<sObject>,true) requires that all records be either inserted or updated in order for there to be success.

Whether or not you choose to get the record Id's is entirely your choice. As you can see, at any point at which you've failed to successfully upsert all your records, you automatically do a rollback. You can add an Apex.Page Messages at that point if you desire to.

Using this method, there's no need for a try-catch block. The method assumes the use of lists and would need to be modified if you're using individual records.

Note: Edited to correct locations of 1st and 3rd Database.rollbacks.

private void save() {
    // Note: Provide ACID behavior
    Savepoint toBeforeState = Database.setSavepoint();    

    if(record.IsEmpty() == false)
    {
        list<Database.UpsertResult>uResults = Database.upsert(record, true);   

        // Iterate through each returned result
        for (Database.uResults ur : urList) 
        {
            if (ur.isSuccess()) 
            {
               // Operation was successful, so get the ID of the record that was processed
               System.debug('Successfully inserted record. record ID: ' + ur.getId());
            }else{
               // Operation failed, so get all errors                
               for(Database.Error err : ur.getErrors()) 
               {
                  System.debug('The following error has occurred.');                    
                  System.debug(err.getStatusCode() + ': ' + err.getMessage());
                  System.debug('record fields that affected this error: ' + err.getFields());
               }
                Database.rollback(toBeforeState); 
            }
        }  
    }   


    if(ChildrenOfTypeA.IsEmpty() == false)
    {
        list<Database.upsertResult>uResultsA = Database.upsert(ChildrenOfTypeA, true);    
        // Iterate through each returned result
        for (Database.uResultA urA : urListA) 
        {
            if (urA.isSuccess()) {
               // Operation was successful, so get the ID of the record that was processed
               System.debug('Successfully inserted record. record ID: ' + urA.getId());
            }else {
                // Operation failed, so get all errors                
                for(Database.Error err : urA.getErrors()) 
                {
                    System.debug('The following error has occurred.');                    
                    System.debug(err.getStatusCode() + ': ' + err.getMessage());
                    System.debug('record fields that affected this error: ' + err.getFields());
                }
                Database.rollback(toBeforeState);
            }
        }
     }

    if(ChildrenOfTypeB.IsEmpty() == false)
    {
    list<Database.upsertResult>uResultsB = Database.upsert(ChildrenOfTypeB, true);

       // Iterate through each returned result
       for (Database.uResultA urB : urListB) 
       {
           if (urB.isSuccess()) 
           {
               // Operation was successful, so get the ID of the record that was processed
               System.debug('Successfully inserted record. record ID: ' + urB.getId());
           }else {
               // Operation failed, so get all errors                
               for(Database.Error err : urB.getErrors()) 
               {
                   System.debug('The following error has occurred.');                    
                   System.debug(err.getStatusCode() + ': ' + err.getMessage());
                   System.debug('record fields that affected this error: ' + err.getFields());
               }
               Database.rollback(toBeforeState);
           }
       }
   }
}

Answer 3

I think problems will arise from rethrowing the exception that you just catched and converted to an PageMessage.