From dfd16a1e5c760922dd3ff823df58ad37aa985ff1 Mon Sep 17 00:00:00 2001 From: james simone Date: Wed, 17 Mar 2021 09:23:58 -0600 Subject: [PATCH] Final updates for #46 - fixed recommencement bug with RollupRelationshipFieldFinder, and added test proving it works --- rollup/main/default/classes/Rollup.cls | 31 +++++++++----- .../classes/RollupFullBatchRecalculator.cls | 6 ++- .../classes/RollupRelationshipFieldFinder.cls | 11 +++-- .../RollupRelationshipFieldFinderTests.cls | 1 - rollup/main/default/classes/RollupTests.cls | 41 ++++++++++++++++++- .../RollupControl.Org_Defaults.md-meta.xml | 4 -- .../MaximumRollupRetries__c.field-meta.xml | 15 ------- 7 files changed, 72 insertions(+), 37 deletions(-) delete mode 100644 rollup/main/default/objects/RollupControl__mdt/fields/MaximumRollupRetries__c.field-meta.xml diff --git a/rollup/main/default/classes/Rollup.cls b/rollup/main/default/classes/Rollup.cls index 46982bfc..270fe775 100644 --- a/rollup/main/default/classes/Rollup.cls +++ b/rollup/main/default/classes/Rollup.cls @@ -1376,7 +1376,8 @@ global without sharing virtual class Rollup implements Database.Batchable objIds = new Set(); // get everything that doesn't have a null Id - a pretty trick - Integer amountOfCalcItems = String.isBlank(potentialGrandparentRelationshipFieldPath) ? getCountFromDb(countQuery, objIds, queryWrapper.recordIds) : 0; + Set recordIds = queryWrapper.recordIds; // also used below, bound to the "queryString" variable + Integer amountOfCalcItems = getCountFromDb(countQuery, objIds, recordIds); // emptyRollup used to call "getMaxQueryRows" below, as well as the default rollupControl.BatchChunkSize__c Rollup emptyRollup = new Rollup(RollupInvocationPoint.FROM_LWC); @@ -1395,7 +1396,6 @@ global without sharing virtual class Rollup implements Database.Batchable(queryFields), 'Id', '!=', queryWrapper.getQuery()); if (amountOfCalcItems < emptyRollup.getMaxQueryRows()) { - Set recordIds = queryWrapper.recordIds; List calculationItems = Database.query(queryString); Rollup thisRollup = getRollup( new List{ rollupInfo }, @@ -1410,7 +1410,7 @@ global without sharing virtual class Rollup implements Database.Batchable> calcItemsByLookupField = grandparentRollups.containsKey(rollup.lookupObj) - ? grandparentRollups.get(rollup.lookupObj).getParentLookupToRecords() - : this.getCalcItemsByLookupField(rollup, this.lookupObjectToUniqueFieldNames.get(rollup.lookupObj)); + if (grandparentRollups.containsKey(rollup.lookupObj) && rollup.traversal == null) { + rollup.traversal = grandparentRollups.get(rollup.lookupObj); + } + + Map> calcItemsByLookupField = this.getCalcItemsByLookupField(rollup, this.lookupObjectToUniqueFieldNames.get(rollup.lookupObj)); // some rollups may not finish retrieving all parent rows the first time around - and that's ok! we can keep // trying until all necessary records have been retrieved if (rollup.traversal != null && rollup.traversal.getIsFinished() == false) { @@ -2003,10 +2005,12 @@ global without sharing virtual class Rollup implements Database.Batchable stackDepth; + isDeferralAllowed = Test.isRunningTest() == false && this.rollupControl.MaxRollupRetries__c > stackDepth; + this.rollups.clear(); this.rollups.addAll(this.deferredRollups); this.deferredRollups.clear(); + if (System.isBatch()) { Database.executeBatch(this, this.rollupControl.BatchChunkSize__c.intValue()); } else { @@ -2131,16 +2135,21 @@ global without sharing virtual class Rollup implements Database.Batchable> queryCountsToLookupIds = new Map>(); for (Rollup rollup : this.rollups) { rollup.isFullRecalc = this.isFullRecalc; if (targetType == null) { targetType = rollup.lookupObj; - } else if (rollup.lookupObj != targetType || String.isNotBlank(rollup.metadata?.GrandparentRelationshipFieldPath__c)) { + } else if (rollup.lookupObj != targetType) { hasMoreThanOneTarget = true; + } else if (String.isNotBlank(rollup.metadata?.GrandparentRelationshipFieldPath__c)) { + // getting the count for grandparent (or greater) relationships will be handled further + // downstream; for our purposes, it isn't useful to try to get all of the records while + // we're still in a sync context + continue; } if (hasMoreThanOneTarget) { diff --git a/rollup/main/default/classes/RollupFullBatchRecalculator.cls b/rollup/main/default/classes/RollupFullBatchRecalculator.cls index 97daf8de..5fd4e862 100644 --- a/rollup/main/default/classes/RollupFullBatchRecalculator.cls +++ b/rollup/main/default/classes/RollupFullBatchRecalculator.cls @@ -2,15 +2,19 @@ public class RollupFullBatchRecalculator extends Rollup { private final String queryString; private final Rollup__mdt rollupInfo; private final SObjectType calcItemType; + private final Set recordIds; - public RollupFullBatchRecalculator(String queryString, RollupInvocationPoint invokePoint, Rollup__mdt rollupInfo, SObjectType calcItemType) { + public RollupFullBatchRecalculator(String queryString, RollupInvocationPoint invokePoint, Rollup__mdt rollupInfo, SObjectType calcItemType, Set recordIds) { super(invokePoint); this.queryString = queryString; this.rollupInfo = rollupInfo; + this.recordIds = recordIds; } public override Database.QueryLocator start(Database.BatchableContext bc) { Set objIds = new Set(); // necessary; there's a bind variable in the query string + // note - if the optional where clause was appended to the passed in query string, this.recordIds is also + // used as a bind variable return Database.getQueryLocator(this.queryString); } diff --git a/rollup/main/default/classes/RollupRelationshipFieldFinder.cls b/rollup/main/default/classes/RollupRelationshipFieldFinder.cls index 8de5754f..8668a94b 100644 --- a/rollup/main/default/classes/RollupRelationshipFieldFinder.cls +++ b/rollup/main/default/classes/RollupRelationshipFieldFinder.cls @@ -16,6 +16,7 @@ public without sharing class RollupRelationshipFieldFinder { private List records; private List relationshipParts; private Boolean isFirstRun = true; + private String currentRelationshipName; public RollupRelationshipFieldFinder( RollupControl__mdt rollupControl, @@ -32,9 +33,7 @@ public without sharing class RollupRelationshipFieldFinder { this.uniqueFinalFieldNames = uniqueFinalFieldNames; if (this.relationshipParts.size() == 1) { - String fieldName = this.relationshipParts[0]; - this.relationshipParts.set(0, ultimateParent.getDescribe().getName()); - this.relationshipParts.add(fieldName); + this.relationshipParts.add(0, ultimateParent.getDescribe().getName()); } this.originalParts = new List(this.relationshipParts); } @@ -141,6 +140,10 @@ public without sharing class RollupRelationshipFieldFinder { Limits.getLimitQueryRows() / 4 < Limits.getQueryRows() || Limits.getLimitHeapSize() / 2 < Limits.getHeapSize() ) { + // we pop fields off of the list while recursively iterating + // which means we need to re-add the last field used if we are stopping + // due to limits + this.relationshipParts.add(0, this.currentRelationshipName); return this.traversal; } @@ -198,7 +201,7 @@ public without sharing class RollupRelationshipFieldFinder { private Traversal recurseThroughObjectChain(List records, SObjectType baseSObjectType) { // cache the latest records through in case we need to continue later this.recommencementRecords = records; - String currentRelationshipName = this.relationshipParts.remove(0); + this.currentRelationshipName = this.relationshipParts.remove(0); Map fieldMap = baseSObjectType.getDescribe().fields.getMap(); SObjectField field = this.getField(fieldMap, currentRelationshipName); diff --git a/rollup/main/default/classes/RollupRelationshipFieldFinderTests.cls b/rollup/main/default/classes/RollupRelationshipFieldFinderTests.cls index 892d8edd..f1bdda6a 100644 --- a/rollup/main/default/classes/RollupRelationshipFieldFinderTests.cls +++ b/rollup/main/default/classes/RollupRelationshipFieldFinderTests.cls @@ -80,7 +80,6 @@ private class RollupRelationshipFieldFinderTests { Account intermediateTwo = new Account(Name = 'Intermediate 2'); insert new List{ intermediateOne, intermediateTwo }; - List updatedAccounts = [SELECT Id, OwnerId, Name FROM Account]; if (updatedAccounts.size() == 2) { // don't run the rest of the test if the org has some kind of ownership assignment going on that would invalidate diff --git a/rollup/main/default/classes/RollupTests.cls b/rollup/main/default/classes/RollupTests.cls index 6fdf4e83..78fff57f 100644 --- a/rollup/main/default/classes/RollupTests.cls +++ b/rollup/main/default/classes/RollupTests.cls @@ -2985,6 +2985,45 @@ private class RollupTests { System.assertEquals(opps[0].Name + ', ' + opps[1].Name, updatedUser.AboutMe, 'Grandparent rollup should have worked!'); } + @isTest + static void shouldDeferGrandparentRollupSafelyTillAllParentRecordsAreRetrieved() { + Account acc = [SELECT Id, OwnerId FROM Account]; + List opps = new List{ + new Opportunity(AccountId = acc.Id, StageName = 'Prospecting', Name = 'One', CloseDate = System.today()), + new Opportunity(AccountId = acc.Id, StageName = 'Prospecting', Name = 'Two', CloseDate = System.today()) + }; + insert opps; + + DMLMock mock = new DMLMock(); + Rollup.defaultControl = new RollupControl__mdt(MaxQueryRows__c = 2, BatchChunkSize__c = 1, MaxRollupRetries__c = 100); + // start as synchronous rollup to allow for one deferral + Rollup.specificControl = new RollupControl__mdt(ShouldRunAs__c = 'Synchronous Rollup', MaxQueryRows__c = 2); + Rollup.DML = mock; + Rollup.shouldRun = true; + Rollup.records = opps; + Rollup.rollupMetadata = new List{ + new Rollup__mdt( + CalcItem__c = 'Opportunity', + RollupFieldOnCalcItem__c = 'Name', + LookupFieldOnCalcItem__c = 'AccountId', + LookupObject__c = 'User', + LookupFieldOnLookupObject__c = 'Id', + RollupFieldOnLookupObject__c = 'AboutMe', + RollupOperation__c = 'CONCAT', + GrandparentRelationshipFieldPath__c = 'Account.Owner.AboutMe' + ) + }; + Rollup.apexContext = TriggerOperation.AFTER_INSERT; + + Test.startTest(); + Rollup.runFromTrigger(); + Test.stopTest(); + + System.assertEquals(1, mock.Records.size(), 'Grandparent record should have been found!'); + User updatedUser = (User) mock.Records[0]; + System.assertEquals(opps[0].Name + ', ' + opps[1].Name, updatedUser.AboutMe, 'Grandparent rollup should have worked!'); + } + //** Helpers */ private static DMLMock loadAccountIdMock(List records) { @@ -3034,4 +3073,4 @@ private class RollupTests { String result = String.valueOf(startingNumber++); return sObjectType.getDescribe().getKeyPrefix() + '0'.repeat(12 - result.length()) + result; } -} \ No newline at end of file +} diff --git a/rollup/main/default/customMetadata/RollupControl.Org_Defaults.md-meta.xml b/rollup/main/default/customMetadata/RollupControl.Org_Defaults.md-meta.xml index bed9f269..52619e06 100644 --- a/rollup/main/default/customMetadata/RollupControl.Org_Defaults.md-meta.xml +++ b/rollup/main/default/customMetadata/RollupControl.Org_Defaults.md-meta.xml @@ -22,10 +22,6 @@ MaxRollupRetries__c 100.0 - - MaximumRollupRetries__c - - ShouldAbortRun__c false diff --git a/rollup/main/default/objects/RollupControl__mdt/fields/MaximumRollupRetries__c.field-meta.xml b/rollup/main/default/objects/RollupControl__mdt/fields/MaximumRollupRetries__c.field-meta.xml deleted file mode 100644 index bc48dff1..00000000 --- a/rollup/main/default/objects/RollupControl__mdt/fields/MaximumRollupRetries__c.field-meta.xml +++ /dev/null @@ -1,15 +0,0 @@ - - - MaximumRollupRetries__c - 100 - Use in conjunction with Max Query Rows. This determines the maximum possible rollup jobs (either batched or queued) that can be spawned from a single overall rollup operation. Default is 100. - false - DeveloperControlled - Use in conjunction with Max Query Rows. This determines the maximum possible rollup jobs (either batched or queued) that can be spawned from a single overall rollup operation. Default is 100. - - 18 - false - 0 - Number - false -