Merge pull request #5 from jamessimone/v1.0.3-updates

v1.0.3 - last of the initial launch feedback

Author: jamessimone

README.md

@@ -203,7 +203,7 @@ public static Rollup sumFromTrigger(
   SObjectType lookupSobjectType
 )
 
-//for using as the "one line of code" and CMDT-driven rollups
+// for using as the "one line of code" and CMDT-driven rollups
 public static void runFromTrigger()
 ```
 
@@ -243,7 +243,37 @@ Rollup.sumFromTrigger(
 ).runCalc();
 ```
 
-It's that simple. Note that in order for custom apex solutions that don't use the `batch` static method on `Rollup` to properly start, the `runCalc()` method must also be called. That is, if you only have one rollup operation per object, you'll _always_ need to call `runCalc()` when invoking `Rollup` from a trigger.
+It's that simple. Note that in order for custom Apex solutions that don't use the `batch` static method on `Rollup` to properly start, the `runCalc()` method must also be called. That is, if you only have one rollup operation per object, you'll _always_ need to call `runCalc()` when invoking `Rollup` from a trigger.
+
+Another note for when the use of an `Evaluator` class might be necessary -- let's say that you have some slight lookup skew caused by a fallback object in a lookup relationship. This fallback object has thousands of objects tied to it, and updates to it are frequently painful / slow. If you didn't need the rollup for the fallback, you could implement an `Evaluator` to exclude it from being processed:
+
+```java
+// again using the example of Opportunities
+trigger OpportunityTrigger on Opportunity(before update, after update, before insert, after insert, before delete) {
+
+  Rollup.sumFromTrigger(
+    Opportunity.Amount
+    Opportunity.AccountId,
+    Account.Id,
+    Account.AnnualRevenue,
+    Account.SObjectType,
+    new FallbackAccountExcluder()
+  ).runCalc();
+
+  public class FallbackAccountExcluder implements Rollup.Evaluator {
+    public Boolean matches(Object calcItem) {
+      if((calcItem instanceof Opportunity) == false) {
+        return false;
+      }
+
+      Opportunity opp = (Opportunity) calcItem;
+      // there are so many ways you could avoid hard-coding the Id here:
+      // custom settings, custom metadata, labels, and platform cache, to name a few
+      return opp.AccountId == 'your fallback Account Id' ? false : true;
+    }
+  }
+}
+```
 
 ## Special Considerations
 
@@ -253,6 +283,16 @@ While pains have been taken to create a solution that's truly one-sized-fits-all
 
 Picklists are a loaded topic in Salesforce. They're not only dropdowns, but the order is supposed to matter! MIN/MAXING on a picklist is supposed to return the deepest possible entry in the picklist (for MAX), or the closest to the top of the picklist (for MIN). If you've studied the aggregate function documentation thoroughly in the Salesforce Developer Docs, this will comes as no surprise - but because the ranking system for picklist differs from the ranking system for other pieces of text, I thought to call it out specifically.
 
+### Recalculations
+
+One of the reasons that `Rollup` can boast of superior performance is that, for many operations, it can perform all of the rolling-up necessary without performing much in the way of queries. There are, as always, exceptions to that rule. "Recalculations" are triggered when certain rollup operations encounter something of interest:
+
+- a MIN operation might find that one of the calculation items supplied to it previously _was_ the minimum value, but is no longer the minimum on an update
+- a MAX operation might find that one of the calculation items supplied to it previously _was_ the _maxmimum_ value, but is no longer the max on an update
+- ... pretty much any operation involving AVERAGE
+
+In these instances, `Rollup` _does_ requery the calculation object; it also does another loop through the calculation items supplied to it in search of _all_ the values necessary to find the true rollup value. This provides context, more than anything -- the rollup operation should still be lightning fast.
+
 ## Commit History & Contributions
 
 This repository comes after the result of [dozens of commits](https://github.com/jamessimone/apex-mocks-stress-test/commits/rollup) on my working repository. You can view the full history of the evolution of `Rollup` there.

package.json

@@ -1,6 +1,6 @@
 {
   "name": "apex-rollup",
-  "version": "1.0.2",
+  "version": "1.0.3",
   "description": "Fast, configurable, elastically scaling custom rollup solution. Apex Invocable action, one-liner Apex trigger/CMDT-driven logic, and scheduled Apex-ready.",
   "repository": {
     "type": "git",

rollup/main/default/classes/Rollup.cls

@@ -762,9 +762,6 @@ global without sharing virtual class Rollup implements Database.Batchable<SObjec
       return new RollupAsyncProcessor(RollupInvocationPoint.FROM_TRIGGER);
     }
 
-    List<SObject> calcItems = getTriggerRecords();
-    SObjectType sObjectType = calcItems.getSObjectType();
-
     String rollupContext;
     Boolean shouldReturn = false;
     Map<Id, SObject> oldCalcItems;
@@ -791,9 +788,10 @@ global without sharing virtual class Rollup implements Database.Batchable<SObjec
       rollupInfo.RollupType__c = rollupContext + rollupInfo.RollupType__c;
     }
 
+    List<SObject> calcItems = getTriggerRecords();
     return shouldReturn
       ? new RollupAsyncProcessor(RollupInvocationPoint.FROM_TRIGGER)
-      : getRollup(rollupMetadata, sObjectType, calcItems, oldCalcItems, eval, RollupInvocationPoint.FROM_TRIGGER);
+      : getRollup(rollupMetadata, calcItems.getSObjectType(), calcItems, oldCalcItems, eval, RollupInvocationPoint.FROM_TRIGGER);
   }
 
   /** end public-facing section, begin private static helpers */
@@ -831,6 +829,9 @@ global without sharing virtual class Rollup implements Database.Batchable<SObjec
       return oldRecordsMap;
     }
 
+    // normally, you could use a shortcut to initialize a Set<Id> like this
+    // by calling new Map<Id, SObject>(currentRecords).keyset() -
+    // but that code path fails if there are null Ids in the list
     Set<Id> currentRecordIds = new Set<Id>();
     for (SObject currentRecord : currentRecords) {
       if (currentRecord.Id != null) {
@@ -846,13 +847,15 @@ global without sharing virtual class Rollup implements Database.Batchable<SObjec
       SObject existingRecordOrDefault = currentRecord.Id != null && existingOldRecordsMap.containsKey(currentRecord.Id)
         ? existingOldRecordsMap.get(currentRecord.Id)
         : (SObject) Type.forName(sObjectDescribe.getName()).newInstance();
+      existingRecordOrDefault.Id = currentRecord.Id;
       Map<String, SObjectField> fieldTokensForObject = sObjectDescribe.fields.getMap();
       SObjectField fieldToken = fieldTokensForObject.get(rollupFieldOnCalcItem);
       if (existingRecordOrDefault.get(rollupFieldOnCalcItem) != null || fieldToken.getDescribe().isUpdateable() == false) {
         continue;
       } else {
         existingRecordOrDefault.put(rollupFieldOnCalcItem, FieldInitializer.getDefaultValue(fieldToken));
       }
+      existingOldRecordsMap.put(currentRecord.Id, existingRecordOrDefault);
     }
     return existingOldRecordsMap;
   }
@@ -1440,16 +1443,23 @@ global without sharing virtual class Rollup implements Database.Batchable<SObjec
         }
 
         if (this.shouldShortCircuit && this.isFirstTimeThrough) {
-          // if, at any point, short circuiting is enabled, we need to loop back around
-          // one last time to ensure no other items in memory will contribute to the final rollup
+          /**
+           * an example of short circuiting - halfway through the list during a MIN operation,
+           * Rollup encounters a calcItem whose previous value equals the current min, but the new value
+           * is greater than the min. This triggers a full recalc, since it is at once both possible
+           * for items outside the list of calcItems to contain the new min, as WELL as for an item
+           * WITHIN the existing calcItems to be the new min. This means we have to go back and re-process
+           * the records that were already iterated on to ensure that the current operation successfully accounts
+           * for everything in-memory and in the database
+           */
           this.isFirstTimeThrough = false;
-          index = 0;
+          index = 0; // resets the for-loop
         }
       }
-      this.returnVal = this.setReturnValue();
+      this.setReturnValue();
     }
 
-    // all of these are no-ops by default
+    // all of these are no-ops by default; child classes opt-in to the rollup types applicable
     public virtual void handleCountDistinct(Op op, SObject calcItem, SObjectField operationField) {
     }
     public virtual void handleUpdateCountDistinct(Op op, SObject calcItem, SObjectField operationField, Map<Id, SObject> oldCalcItems) {
@@ -1474,9 +1484,7 @@ global without sharing virtual class Rollup implements Database.Batchable<SObjec
     }
     protected virtual void handleShortCircuit(Op op, SObject calcItem, SObjectField operationField) {
     }
-
-    protected virtual Object setReturnValue() {
-      return this.returnVal;
+    protected virtual void setReturnValue() {
     }
 
     protected virtual Object calculateNewAggregateValue(Set<Id> excludedItems, Op operation, SObjectField operationField, SObjectType sObjectType) {
@@ -1509,8 +1517,8 @@ global without sharing virtual class Rollup implements Database.Batchable<SObjec
       }
     }
 
-    protected override Object setReturnValue() {
-      return this.distinctValues.size();
+    protected override void setReturnValue() {
+      this.returnVal = this.distinctValues.size();
     }
 
     protected override void handleShortCircuit(Op op, SObject calcItem, SObjectField operationField) {
@@ -1662,8 +1670,8 @@ global without sharing virtual class Rollup implements Database.Batchable<SObjec
       }
     }
 
-    protected override Object setReturnValue() {
-      return this.returnDecimal;
+    protected override void setReturnValue() {
+      this.returnVal = this.returnDecimal;
     }
 
     protected virtual override Object calculateNewAggregateValue(Set<Id> excludedItems, Op operation, SObjectField operationField, SObjectType sObjectType) {
@@ -1804,10 +1812,10 @@ global without sharing virtual class Rollup implements Database.Batchable<SObjec
       this.stringVal = (String) this.returnVal;
     }
 
-    protected override Object setReturnValue() {
+    protected override void setReturnValue() {
       String possibleReturnValue = this.stringVal.normalizeSpace();
       String withoutEndingComma = possibleReturnValue.endsWith(',') ? possibleReturnValue.substring(0, possibleReturnValue.length() - 1) : possibleReturnValue;
-      return (withoutEndingComma.startsWith(',') ? withoutEndingComma.substring(1, withoutEndingComma.length()) : withoutEndingComma).trim();
+      this.returnVal = (withoutEndingComma.startsWith(',') ? withoutEndingComma.substring(1, withoutEndingComma.length()) : withoutEndingComma).trim();
     }
 
     protected override void handleShortCircuit(Op op, SObject calcItem, SObjectField operationField) {

rollup/main/default/classes/RollupTests.cls

@@ -1584,6 +1584,28 @@ private class RollupTests {
     System.assertEquals(750, updatedAcc.AnnualRevenue, 'SUM AFTER_UPDATE from flow should match diff for Amount');
   }
 
+  @isTest
+  static void shouldCorrectlyInitializeDefaultsWhenOldRecordsDontExistFromFlow() {
+    List<Opportunity> opps = new List<Opportunity>{
+      new Opportunity(Amount = 1000, Id = generateId(Opportunity.SObjectType)),
+      new Opportunity(Amount = 1000, Id = generateId(Opportunity.SObjectType))
+    };
+    DMLMock mock = getMock(opps);
+    opps[1].AccountId = null;
+
+    Test.startTest();
+    List<Rollup.FlowOutput> flowOutputs = Rollup.performRollup(prepareFlowTest(opps, 'UPDATE', 'SUM'));
+    Test.stopTest();
+
+    System.assertEquals(1, flowOutputs.size(), 'Flow ouputs were not provided');
+    System.assertEquals('SUCCESS', flowOutputs[0].message);
+    System.assertEquals(true, flowOutputs[0].isSuccess);
+
+    System.assertEquals(1, mock.Records.size(), 'SUM AFTER_UPDATE from flow did not update accounts');
+    Account updatedAcc = (Account) mock.Records[0];
+    System.assertEquals(1000, updatedAcc.AnnualRevenue, 'SUM AFTER_UPDATE from flow should match diff for Amount');
+  }
+
   @isTest
   static void shouldBeInvokedSuccessfullyBeforeDeleteFromFlow() {
     List<Opportunity> opps = new List<Opportunity>{ new Opportunity(Amount = 1000) };