diff --git a/README.md b/README.md index fa479de..c84c910 100644 --- a/README.md +++ b/README.md @@ -5,12 +5,12 @@ ## Deployment - + Deploy to Salesforce - + Deploy to Salesforce Sandbox diff --git a/core/classes/FlowRoundRobinAssigner.cls b/core/classes/FlowRoundRobinAssigner.cls index 90c29b4..3e3d953 100644 --- a/core/classes/FlowRoundRobinAssigner.cls +++ b/core/classes/FlowRoundRobinAssigner.cls @@ -1,8 +1,6 @@ global without sharing class FlowRoundRobinAssigner { @TestVisible private static RoundRobinAssigner.IAssignmentRepo stubAssignmentRepo; - @TestVisible - private static Boolean hasBeenUpdated = false; private static final Set PROCESSED_RECORD_IDS = new Set(); private static final FlowRoundRobinAssigner SELF = new FlowRoundRobinAssigner(); @@ -26,16 +24,23 @@ global without sharing class FlowRoundRobinAssigner { @InvocableMethod(category='Round Robin' label='Round robin records') global static void assign(List flowInputs) { - if (hasBeenUpdated == false) { - for (FlowInput input : flowInputs) { - if (input.recordsToRoundRobin?.isEmpty() != false && input.recordToRoundRobin != null) { - input.recordsToRoundRobin = new List{ input.recordToRoundRobin }; - } - if (input.recordsToRoundRobin.isEmpty() == false) { - SELF.trackAssignedIds(input); - SELF.roundRobin(input); - } + FlowInput bulkifiedInput; + for (FlowInput input : flowInputs) { + if (bulkifiedInput == null) { + bulkifiedInput = input; + } + if (input.recordsToRoundRobin?.isEmpty() != false && input.recordToRoundRobin != null) { + input.recordsToRoundRobin = new List{ input.recordToRoundRobin }; } + bulkifiedInput.recordsToRoundRobin.addAll(input.recordsToRoundRobin); + } + + if (bulkifiedInput?.recordsToRoundRobin.isEmpty() == false) { + SELF.trackAssignedIds(bulkifiedInput); + SELF.roundRobin(bulkifiedInput); + } + if (bulkifiedInput?.updateRecords == true) { + update bulkifiedInput.recordsToRoundRobin; } } @@ -44,10 +49,6 @@ global without sharing class FlowRoundRobinAssigner { RoundRobinAssigner.IAssignmentRepo assignmentRepo = this.getAssignmentRepo(input); RoundRobinAssigner.Details assignmentDetails = this.getAssignmentDetails(input); new RoundRobinAssigner(assignmentRepo, assignmentDetails).assignOwners(input.recordsToRoundRobin); - if (input.updateRecords) { - update input.recordsToRoundRobin; - hasBeenUpdated = true; - } } private void validateInput(FlowInput input) { diff --git a/core/classes/FlowRoundRobinAssignerTests.cls b/core/classes/FlowRoundRobinAssignerTests.cls index 3c41b53..b33cbad 100644 --- a/core/classes/FlowRoundRobinAssignerTests.cls +++ b/core/classes/FlowRoundRobinAssignerTests.cls @@ -60,7 +60,6 @@ private class FlowRoundRobinAssignerTests { FlowRoundRobinAssigner.assign(new List{ input }); - System.assertEquals(true, FlowRoundRobinAssigner.hasBeenUpdated); System.assertEquals(UserInfo.getUserId(), cpa.OwnerId); } diff --git a/core/classes/QueryAssigner.cls b/core/classes/QueryAssigner.cls index 925150c..9b348f3 100644 --- a/core/classes/QueryAssigner.cls +++ b/core/classes/QueryAssigner.cls @@ -1,9 +1,17 @@ public without sharing class QueryAssigner implements RoundRobinAssigner.IAssignmentRepo { + private static final Map> QUERY_TO_RECORDS = new Map>(); private final List validAssignmentIds; public QueryAssigner(String query, String assignmentFieldName) { Set assignmentIds = new Set(); - List matchingRecords = Database.query(query); + List matchingRecords; + if (QUERY_TO_RECORDS.containsKey(query)) { + matchingRecords = QUERY_TO_RECORDS.get(query); + } else { + matchingRecords = Database.query(query); + QUERY_TO_RECORDS.put(query, matchingRecords); + } + for (SObject matchingRecord : matchingRecords) { assignmentIds.add((Id) matchingRecord.get(assignmentFieldName)); } diff --git a/core/classes/RoundRobinRepository.cls b/core/classes/RoundRobinRepository.cls index 75d54e2..72b801e 100644 --- a/core/classes/RoundRobinRepository.cls +++ b/core/classes/RoundRobinRepository.cls @@ -1,7 +1,6 @@ public without sharing class RoundRobinRepository extends AbstractCacheRepo { private static Map CACHED_ASSIGNMENTS; - @SuppressWarnings('PMD.ApexCRUDViolation') public void accept(IThreadSafeCacheVisitor visitor, List records) { RoundRobin__c currentAssignment = this.getCurrentAssignment(visitor.getVisitKey()); visitor.visitRecords(records, currentAssignment); @@ -47,39 +46,56 @@ public without sharing class RoundRobinRepository extends AbstractCacheRepo { @SuppressWarnings('PMD.ApexCRUDViolation') private Boolean commitUpdatedAssignment(RoundRobin__c assignment) { - Boolean wasCommitSuccessful = true; Map currentCache = this.getCachedAssignments(); if ( currentCache.containsKey(assignment.Name) && currentCache.get(assignment.Name).LastUpdated__c > CACHED_ASSIGNMENTS.get(assignment.Name).LastUpdated__c ) { - assignment = currentCache.get(assignment.Name); - wasCommitSuccessful = false; - } else { - assignment.LastUpdated__c = System.now(); - /** - * integration tests with after save Flows have shown something unfortunate: - * though the second (recursive) call to the assigner is spawned in a second transaction - * the RoundRobin__c.getAll() still doesn't contain the Id of the inserted record (for the times where the assignment - * is being run for the first time). - * That means that we can't just call "upsert", and instead have to do this goofy - * song and dance to ensure the Id is appended correctly - */ - if (assignment.Id == null) { - List existingAssignments = [SELECT Id FROM RoundRobin__c WHERE Name = :assignment.Name]; - if (existingAssignments.isEmpty() == false) { - assignment.Id = existingAssignments[0].Id; - } + return false; + } + assignment.LastUpdated__c = System.now(); + /** + * integration tests with after save Flows have shown something unfortunate: + * though the second (recursive) call to the assigner is spawned in a second transaction + * the RoundRobin__c.getAll() call still doesn't contain the Id of the inserted record (for the times where the assignment + * is being run for the first time). + * That means that we can't just call "upsert", and instead have to do this goofy + * song and dance to ensure the Id is appended correctly + */ + if (assignment.Id == null) { + List existingAssignments = [SELECT Id FROM RoundRobin__c WHERE Name = :assignment.Name]; + if (existingAssignments.isEmpty() == false) { + assignment.Id = existingAssignments[0].Id; } - if (assignment.Id != null) { - update assignment; - } else { - insert assignment; + } + if (assignment.Id != null) { + try { + /** + * if two separate threads are trying to round robin at the same time, the LastUpdated__c check above + * isn't enough to ensure write-safety, and unfortunately FOR UPDATE is the only mutex Apex offers + * as a write-safe guarantee. One downside (among many) is that FOR UPDATE frequently throws; another is + * that another locking thread can release early - let's protect against both those eventualities + */ + RoundRobin__c lockedAssignment = [SELECT Id, Name, LastUpdated__c FROM RoundRobin__c WHERE Id = :assignment.Id FOR UPDATE]; + if (lockedAssignment.LastUpdated__c >= assignment.LastUpdated__c) { + // lock was released early, but the existing Index__c now almost certainly has stale values in it + // re-round robin to get the now-correct values + return false; + } + lockedAssignment.Index__c = assignment.Index__c; + lockedAssignment.LastUpdated__c = assignment.LastUpdated__c; + update lockedAssignment; + // purely for the map assignment, below + assignment = lockedAssignment; + } catch (DmlException ex) { + return false; } + } else { + insert assignment; } CACHED_ASSIGNMENTS.put(assignment.Name, assignment); - return wasCommitSuccessful; + return true; } private Map getCachedAssignments() { diff --git a/integration-tests/classes/RoundRobinFlowIntegrationTests.cls b/integration-tests/classes/RoundRobinFlowIntegrationTests.cls index 1d4f83b..3702687 100644 --- a/integration-tests/classes/RoundRobinFlowIntegrationTests.cls +++ b/integration-tests/classes/RoundRobinFlowIntegrationTests.cls @@ -14,10 +14,16 @@ private class RoundRobinFlowIntegrationTests { roundRobinUser.LastName = 'roundRobinUser'; insert roundRobinUser; - Lead lead = new Lead(LastName = 'Test Assignment', Company = 'Test'); - insert lead; + List leads = new List(); + // stress test! + for (Integer index = 0; index < 200; index++) { + leads.add(new Lead(LastName = 'Assignment ' + index, Company = 'Round Robin')); + } + insert leads; - lead = [SELECT Id, OwnerId FROM Lead]; - System.assertEquals(roundRobinUser.Id, lead.OwnerId); + leads = [SELECT Id, OwnerId FROM Lead]; + for (Lead lead : leads) { + System.assertEquals(roundRobinUser.Id, lead.OwnerId); + } } } diff --git a/package.json b/package.json index e7182bc..3d9af25 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "salesforce-round-robin", - "version": "0.1.1", + "version": "0.1.2", "description": "Round robin records in Salesforce (SFDC) using Flow or Apex. Performant, fair, fast assignment with configurable user pools", "repository": { "type": "git", diff --git a/sfdx-project.json b/sfdx-project.json index 0195c0f..6e1962f 100644 --- a/sfdx-project.json +++ b/sfdx-project.json @@ -5,8 +5,8 @@ "definitionFile": "config/project-scratch-def.json", "package": "salesforce-round-robin", "path": "core", - "versionName": "Reverts mutex due to bulkification issues with Flow, adds more safety for recursive Flow transactions", - "versionNumber": "0.1.1.0", + "versionName": "Better flow bulkification for updates, reinstated mutex", + "versionNumber": "0.1.2.0", "versionDescription": "Invocable & Apex-ready Round Robin Assigner for Salesforce Flow, Process Builder, Apex and more", "releaseNotesUrl": "https://github.com/jamessimone/salesforce-round-robin/releases/latest" }, @@ -21,6 +21,7 @@ "salesforce-round-robin": "0Ho6g000000GnClCAK", "salesforce-round-robin@0.0.4-0": "04t6g000008SjZEAA0", "salesforce-round-robin@0.1.0-0": "04t6g000008SjpyAAC", - "salesforce-round-robin@0.1.1-0": "04t6g000008fjhFAAQ" + "salesforce-round-robin@0.1.1-0": "04t6g000008fjhFAAQ", + "salesforce-round-robin@0.1.2-0": "04t6g000008fjhyAAA" } } \ No newline at end of file