Interplay between onValidate and onAfterInsert/Update

[cropredy]

By way of example, I've excerpted from the samplecode Domain Opportunities.cls.

Is this by design?

If onValidate() places an error via opp.Type.addError(error(msg,opp,fieldToken)) on some record in the batch, say record 22, then when the onAfterInsert comes along, the Accounts related to all opportunities in the batch including record 22, will go through the update via uow.commitWork().

This seems flawed, - if there is a onValidate() handler error, the affected records should be excluded in the sample code from inclusion in accountIds.

It would be nice if there were an easy way to communicate the erroneous records between handlers in the framework, but the errors(...) method only saves the errors in a list, not a map keyed by the record.ID.

I could, I guess, introduce my own error(...) into a subclass of fflib_SObjectDomain that in turn is extended by Opportunities.cls. My version of error(..) would capture the errorneous record id in a map for inspection by the onAfterInsert and onAfterUpdate handlers.

But I'm wondering if I'm missing something provided by the Domain layer that would obviate this seemingly basic feature.

    public override void onValidate(Map<Id,SObject> existingRecords)
    {
        // Validate changes to Opportunities
        for(Opportunity opp : (List<Opportunity>) Records)
        {
            Opportunity existingOpp = (Opportunity) existingRecords.get(opp.Id);
            if(opp.Type != existingOpp.Type)
            {
                opp.Type.addError( error('You cannot change the Opportunity type once it has been created.', opp, Opportunity.Type) );
            }
        }
    }

    public override void onAfterInsert()
    {
        // Related Accounts
        List<Id> accountIds = new List<Id>();
        for(Opportunity opp : (List<Opportunity>) Records)
            if(opp.AccountId!=null) // ?! what about excluding the opps that had their type changed ?
                accountIds.add(opp.AccountId);

        // Update last Opportunity activity on the related Accounts (via the Accounts domain class)
        fflib_SObjectUnitOfWork uow = new fflib_SObjectUnitOfWork(new Schema.SObjectType[] { Account.SObjectType }); 
        Accounts accounts = new Accounts([select Id, Name from Account where id in :accountIds]);
        accounts.updateOpportunityActivity(uow);
        uow.commitWork();               
    }

[afawcett]

Thanks for the question. Your not missing anything, this is an optimisation that could be considered. If in the default all or nothing error handling case the entire operation will get rolled back eventually, though in the partial update case yes, the opp would get updated. I guess it really depends if thats something you care about tracking all the time. It's not something that has come up tbh, but is a good point none the less.

The error method was actually only intended as an aid to more advanced unit testing. As ApexPages.getMessages is quite basic, and logging the errors raised in the code this way allows for more robust test asserts.

Its true it could be enhanced to support this use case. Though it is static method, and i think you'd really want have something scoped more by the domain object instance (and record set) these two methods operate on. You can track this yourself between these two methods of course via an instance variable. Or create a new base class with such functionality in as you pointed out.

I'd be interested in a other views of those that follow this repo on if adding this into the core library or just leaving as a pattern that can be implemented as a custom base class. Perhaps if in the meantime you could share your custom base class idea here?

[cropredy]

Andy - thanks for the reply. In the allOrNothing = true use case, I realize that the downstream (after insert, after update) work would never committed. But if the downstream work used methods that assumed valid precursor SObjects, you could get NPEs or other badness in some scenarios. And defensive coding seemed counter-productive given that most SFDC apps aren't installed on, say, the Juno mission to Jupiter where maximum redundancy is paramount.

My custom base class looks like this:

public virtual with sharing class ApplicationSObjectDomain extends fflib_SObjectDomain {

    //  Class to hold common methods used by all domain classes where such common methods don't exist in fflib__SObjectDomain.cls.

    public map<ID,SObject> idInErrorMap = new map<ID,SObject> ();

    public ApplicationSObjectDomain(List<SObject> records) {
        super(records);
    }

    public ApplicationSObjectDomain(List<SObject> records, SobjectType sObjectType) {
        super(records, sObjectType);
    }

    public String verror(String message, SObject record)    {
        idInErrorMap.put((ID)record.get('ID'),record);
        return error(message, record);  
    }


    public String verror(String message, SObject record, SObjectField field)    {
        idInErrorMap.put((ID)record.get('ID'),record);
        return error(message, record, field);   
    }

    public Boolean hasError(ID id) {
        return idInErrorMap.containsKey(id);
    }

    public SObject[] validRecords() { // exclude from queries records already noted as failing onValidate
        SObject[] res = new List<SObject>();
        for (SObject sobj : (List<SObject>) Records) {
            ID id = (ID) sobj.get('id');
            if (id == null || !hasError(id)) res.add(sobj);
        }
        return res;
    }
}

And gets used to record errors this way:

opp.Type.addError(verror(msg,opp,fieldToken));

And gets used in the onAfterUpdate (or onAfterInsert) handlers:

 // assume here that update of Opportunity needs to propagate something into OLI not doable by cross-object formula fields ...
 public override void onAfterUpdate(Map<Id,SObject> existingRecordsMap) {     
    OpportunityLineItems olis = new OpportunityLineItems ( OpportunityLineItemsSelector()
                                                           .selectByOppoId(getOppoIds()));
    ...create uow and ask OpportunityLineItems domain class instance to update some fields and register as dirty
   .uow.commitWork();
}

// Handy helper (grabs set of ID fields from list of SObjects
private set<ID> getOppoIds()   {return Util.getIdSetFromField(validRecords(), OpportunityLineItem.OpportunityId);}  // for selectors 

[afawcett]

Thanks for this, this looks great to me. Lets see what others in the community here think over the next week or so.

[cropredy]

An optimization to the validRecords() method:

    public SObject[] validRecords() { // exclude from queries records already noted as failing onValidate
        if (idInErrorMap.isEmpty()) return Records; // if no errors in domain instance, avoid looping to find nothing.  
        SObject[] res = new List<SObject>();
        for (SObject sobj : (List<SObject>) Records) {
            ID id = (ID) sobj.get('id');
            if (id == null || !hasError(id)) res.add(sobj);
        }
        return res;
    }

or , a more complex (arguable) optimization

    public SObject[] validRecords() { // exclude from queries records already noted as failing onValidate
       SObject[] res = new List<Sobject.().addAll(Records); // assume all are valid
       Integer removeCount = 0;
       for (Integer i = Records.size(), i > 0; i--) {  // reverse order, remove inError Records
          if (removeCount == idInError.map.size()) break; // no errors left to find

          ID id = (ID) sobj.get('id');
          if (hasError(id)) {
             res.remove(i);
             removeCount++;
        }
        return res;
    }