Refactor Selector to Query Once #1360

aheber · 2023-06-17T18:59:57Z

Significant refactor of the selector layer to query the config records from both sources once and cache them. Cache's must be separate by constructor's different security scopes to avoid leaking data.

Critical Changes

Huge changes to how Rollup Configs are retrieved. Primarily to keep SOQL counts low in the event of recursion or chained rollups where one rollup update may cascade into another rollup. Not a big issue for most people but we do get reports of DLRS running over it's query limits from time to time.

This is a high risk change because this impacts nearly every aspect of the application. Will need extra QA on this including UI-based testing because this powers the config editing interfaces as well as the rollup engine.

Issues Closed

#1256

Cache built per-constructor combo

aheber · 2023-06-17T19:01:03Z

dlrs/main/classes/RollupService.cls

@@ -1235,9 +1235,7 @@ global with sharing class RollupService {
        }
        Map<String, RollupSummary> shadowRollupsByMdtId = new Map<String, RollupSummary>();
        for (
-          RollupSummary shadowRollup : new RollupSummariesSelector.CustomObjectSelector(


This change allows accessing the data cache instead of re-querying the records. Added a new public method to the class to allow requesting records that aren't active.

aheber · 2023-06-17T19:02:31Z

dlrs/main/classes/RollupServiceTest3.cls

@@ -113,6 +113,9 @@ private with sharing class RollupServiceTest3 {
    rollupSummary.CalculationMode__c = 'Scheduled';
    insert rollupSummary;

+    // burn the rollup cache so it can see the record


Rollup config's were changed after the cache is build the first time. Clearing the cache allows the new rollups to be seen.

aheber · 2023-06-17T19:03:44Z

dlrs/main/classes/RollupSummariesSelector.cls

@@ -34,6 +34,11 @@ public class RollupSummariesSelector {
  @TestVisible
  private CustomMetadataSelector m_customMetadataSelector;

+  @TestVisible
+  private static Map<String, List<RollupSummary>> summariesCacheByKey = new Map<String, List<RollupSummary>>();


static cache for holding the queried data between instances.

aheber · 2023-06-17T19:04:10Z

dlrs/main/classes/RollupSummariesSelector.cls

+  @TestVisible
+  private static Map<String, List<RollupSummary>> summariesCacheByKey = new Map<String, List<RollupSummary>>();
+
+  private List<RollupSummary> allSummaries;


Per-instance data that we can access to fill requests.

aheber · 2023-06-17T19:04:34Z

dlrs/main/classes/RollupSummariesSelector.cls

@@ -49,16 +54,42 @@ public class RollupSummariesSelector {
      enforceSecurity,
      forEdit
    );
+
+    String cacheKey = String.format(


The data cache key is the security/usage parameters provided in the constructor.

Which results in something like false|true - for readability's sake, what do you think about String.valueOf(cacheKeyEnum), cacheKeyEnum being a readable representation of the four states

SelectedForReadWithSecurity SelectedForEditWithSecurity SelectedForReadWithoutSecurity SelectedForEditWithoutSecurity

I thought about that. I didn't make reading the cache key a priority because it was never exposed anywhere. I didn't think anyone would need it for troubleshooting or comprehending the code.

I wouldn't want to change the constructor interface so the caller passed in an Enum, I think that would be a very large change. Converting the current passed in variables to an enum would just be a multi-step if chain that didn't feel like was adding value.
I think if we wanted to build a string based on constructor variables that could make sense, rather that deriving one from the string values directly. That could be done easily with two ternary statements.

I'll put through a change so we can see if it feels better.

One additional thought on this. Maybe the concern is more about tests that need to pre-fill caches, in which case they need to know the cache key so a human will have to read/set the data.
What if we provided a @TestVisible method using the recommended Enum to set the cache value. It would mean we'd have to go ahead and add an internal method with the if chain that translates the constructor arguments to an enum, I'd probably also expose an additional constructor override that allows you to pass the enum directly and we'd slowly convert the codebase to use that. This would be a much longer play but the test classes that need to set the cache directly might really appreciate it down the road.

I'd probably provide a public method to clear the cache as well, no parameters though, just burn all the caches.

I'm also thinking about the ideal future where test classes can set RollupSummary config state without inserting records, this will be necessary when we eventually want to deprecate the Custom Object which is still used heavily in our tests.

aheber · 2023-06-17T19:05:16Z

dlrs/main/classes/RollupSummariesSelector.cls

-    records = m_customObjectSelector.selectById(idSet);
-    records.addAll(m_customMetadataSelector.selectById(idSet));
-    records.addAll(m_customMetadataSelector.selectByDeveloperName(idSet));
+    for (RollupSummary rs : this.allSummaries) {


Outer methods now pull from the local instance's memory.

aheber · 2023-06-17T19:06:44Z

dlrs/main/classes/RollupSummariesSelector.cls

+  /**
+   * Returns active lookup rollup summary definitions for the given rollup unique names
+   **/
+  public List<RollupSummary> selectByUniqueName(Set<String> uniqueNames) {


Added to enable mapping the stub mdt records for scheduled items in RollupService.

aheber · 2023-06-17T19:07:21Z

dlrs/main/classes/RollupSummariesSelector.cls

@@ -167,99 +219,6 @@ public class RollupSummariesSelector {
      return LookupRollupSummary__c.sObjectType;
    }

-    public List<RollupSummary> selectById(Set<String> idSet) {


Now that we're not running queries for specific use cases we are able to remove nearly all of these per-type queries.

aheber · 2023-06-17T19:08:15Z

dlrs/main/classes/RollupSummariesSelector.cls

-      }
-
-      rollupSummaryComparables.sort();
+  private static List<RollupSummary> sortSummaries(


sortSummaries moved to the outer class scope so it can be access but the new logic to pull from in memory data.

Szandor72

Thank you, Anthony. Much appreciated.

Szandor72 · 2023-06-18T09:02:29Z

dlrs/main/classes/RollupServiceTest.cls

@@ -807,8 +807,7 @@ private with sharing class RollupServiceTest {
    // Assert rollup
    System.assertEquals(
      0,
-      [SELECT AnnualRevenue FROM Account WHERE Id = :account.Id]
-      .AnnualRevenue
+      [SELECT AnnualRevenue FROM Account WHERE Id = :account.Id].AnnualRevenue


these seem auto formatting changes? How did these happen?

My computer runs prettier on each save. I don't know why these lines differed on this execution of prettier.

Szandor72 · 2023-06-18T10:01:58Z

dlrs/main/classes/RollupServiceTest5.cls

@@ -133,6 +133,8 @@ private class RollupServiceTest5 {
    rollupSummaryOpp.CalculationMode__c = 'Scheduled';
    insert rollupSummaryOpp;

+    RollupSummariesSelector.summariesCacheByKey = new Map<String, List<RollupSummary>>();


I am surprised to find the necessity to reset the @testvisible cache here but not in other test methods.

The first time you cause a rollup to be executed during a test the cache is built. For a few tests we run once then insert a new Rollup record and cause the rollups to be executed again. In those cases the cached data doesn't hold the new config records and thus we have to invalidate the cache before proceeding.
This scenarios should never be an issue in production execution because we shouldn't be modifying rollup configs as a product of a transaction that executes a rollup. (At least I've never heard of it happening)
If we thought it might be a concern. We already have a trigger on the custom objects, we could add to that trigger logic to invalidate the cache after insert/update.

I didn't do anything about it because the custom object is a relic that should be deprecated and removed rather than reinforced and nobody could update the CMDT real-time as part of a triggered action.

Szandor72 · 2023-06-18T10:09:24Z

dlrs/main/classes/RollupSummariesSelector.cls

@@ -49,16 +54,42 @@ public class RollupSummariesSelector {
      enforceSecurity,
      forEdit
    );
+
+    String cacheKey = String.format(


Which results in something like false|true - for readability's sake, what do you think about String.valueOf(cacheKeyEnum), cacheKeyEnum being a readable representation of the four states

SelectedForReadWithSecurity SelectedForEditWithSecurity SelectedForReadWithoutSecurity SelectedForEditWithoutSecurity

Szandor72 · 2023-06-18T10:15:04Z

dlrs/main/classes/RollupSummariesSelector.cls

+    return false;
+  }
+
+  private Boolean containsIgnoreCase(Set<String> collection, String val) {


with api 58 ( see your other PR) set implements iterable

I think that would be a great way to do it. Just take a param of iterable string and not need the overrides. I expect the 58 to be merged first then I can modify this PR before it is merged. Thanks!

aheber · 2023-06-20T01:51:53Z

dlrs/main/classes/RollupServiceTest3.cls

@@ -243,6 +246,9 @@ private with sharing class RollupServiceTest3 {
    rollupSummary.CalculationMode__c = 'Scheduled';
    insert rollupSummary;

+    // burn the cache


Examples of clearing the cache mid-transaction.

aheber · 2023-06-20T01:52:54Z

dlrs/main/classes/RollupSummariesSelectorTest.cls

-    RollupSummariesSelector rollupSelector = new RollupSummariesSelector(false, false);
-    rollupSelector.m_customMetadataSelector = mock;
+    // fill cache
+    RollupSummariesSelector.setRollupCache(


New test-only means of settings the cache without having to know how the cache key is constructed.

aheber · 2023-06-20T01:54:08Z

dlrs/main/classes/RollupSummariesSelector.cls

+
+  private List<RollupSummary> allSummaries;
+
+  public static void clearRollupCache() {


New method to clear the existing cache of data, if needed.

aheber · 2023-06-20T01:55:13Z

dlrs/main/classes/RollupSummariesSelector.cls

+  }
+
+  @TestVisible
+  private static void setRollupCache(


Useful data mocking if needed. Will be very useful when we want to refactor out all the tests that add Custom Objects to use Custom Metadata while being able to ignore what might already exist in the org.

…tion

sfenton3 · 2023-06-25T20:18:21Z

@aheber It looks like this test failed when you merged main branch into this branch.

aheber · 2023-06-25T20:19:28Z

Thanks, I'll get it cleaned up today.

sfenton3 · 2023-06-26T05:28:31Z

After - QA Changes:
Reduced the number of SOQL queries used by 4 (same as number of child records being rolled up)

QA - Reason	QA Picture
1. Create Rollup Definition with Self referential parent hierarchy
2. View Rollup Hierarchy on account
3. Change AmountTest on lowest member of hierarchy
4. View Number of SOQL queries called
5.
6.
7.
8.

Before - QA Changes:

QA - Reason	QA Picture
1. Create Rollup Definition with Self referential parent hierarchy
2. View Rollup Hierarchy on account
3. Change AmountTest on lowest member of hierarchy
4. View Number of SOQL queries called
5.
6.
7.
8.

Leveraging new ability for Set to implement Iterable in v58.0

Szandor72 · 2023-07-04T14:43:28Z

dlrs/main/classes/RollupSummariesSelector.cls

    }
  }

+  private static String getCacheKey(boolean enforceSecurity, boolean forEdit) {
+    return String.format(


Szandor72 · 2023-07-04T14:45:41Z

dlrs/main/classes/RollupSummariesSelector.cls

-  private Boolean containsIgnoreCase(List<String> collection, String val) {
-    if (val == null) {
-      return collection.contains(val);
+  private Boolean containsIgnoreCase(Iterable<String> collection, String val) {


Elegant. but s, i, val are a bit crude :)

sfenton3 · 2023-08-09T00:25:43Z

looks like this is stuck on automated tests.

aheber · 2023-08-09T14:40:45Z

@sfenton3 this has passing tests again.

sfenton3 · 2023-08-12T18:48:29Z

@aheber All set, thanks!

Refactor Selector to Query Once

4c6cb8e

Cache built per-constructor combo

aheber requested review from Szandor72 and sfenton3 June 17, 2023 18:59

aheber commented Jun 17, 2023

View reviewed changes

Correct property name

a2480ab

Szandor72 reviewed Jun 18, 2023

View reviewed changes

Update cache management

da7dc12

aheber commented Jun 20, 2023

View reviewed changes

aheber added this to In Progress in Dev Project Jun 20, 2023

sfenton3 moved this from In Progress to Dev Done in Dev Project Jun 22, 2023

sfenton3 moved this from Dev Done to QE In Progress in Dev Project Jun 22, 2023

Merge branch 'main' into feature/1256-cache-rollup-config-per-transac…

87b59e7

…tion

repair lines mangled by merge

1b73251

github-actions bot and others added 3 commits June 27, 2023 17:08

Merge 0da57bf into feature/1256-cache-rollup-config-per-transaction

afbbdd0

Conosolidate methods for iterable collections

b02df5d

Leveraging new ability for Set to implement Iterable in v58.0

add null handling

afbfbf5

Szandor72 reviewed Jul 4, 2023

View reviewed changes

Merge 3bbe7b8 into feature/1256-cache-rollup-config-per-transaction

a3a5198

JimBTek added this to the 2.19 milestone Jul 18, 2023

Merge a67843c into feature/1256-cache-rollup-config-per-transaction

429e333

sfenton3 previously approved these changes Aug 9, 2023

View reviewed changes

Update to trigger Actions

89fe066

aheber dismissed sfenton3’s stale review via 89fe066 August 9, 2023 14:23

sfenton3 self-requested a review August 12, 2023 18:47

sfenton3 approved these changes Aug 12, 2023

View reviewed changes

sfenton3 merged commit 02752a7 into main Aug 12, 2023
3 checks passed

Dev Project automation moved this from QE In Progress to Dev Done Aug 12, 2023

sfenton3 deleted the feature/1256-cache-rollup-config-per-transaction branch August 12, 2023 18:48

sfenton3 moved this from Dev Done to Ready in Dev Project Aug 12, 2023

JimBTek moved this from Ready to Previously Released in Dev Project Oct 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Selector to Query Once #1360

Refactor Selector to Query Once #1360

aheber commented Jun 17, 2023

aheber Jun 17, 2023

aheber Jun 17, 2023

aheber Jun 17, 2023

aheber Jun 17, 2023

aheber Jun 17, 2023

Szandor72 Jun 18, 2023

aheber Jun 18, 2023

aheber Jun 18, 2023

aheber Jun 17, 2023

aheber Jun 17, 2023

aheber Jun 17, 2023

aheber Jun 17, 2023

Szandor72 left a comment

Szandor72 Jun 18, 2023

aheber Jun 18, 2023

Szandor72 Jun 18, 2023

aheber Jun 18, 2023

Szandor72 Jun 18, 2023

Szandor72 Jun 18, 2023

aheber Jun 18, 2023

aheber Jun 20, 2023

aheber Jun 20, 2023

aheber Jun 20, 2023

aheber Jun 20, 2023

sfenton3 commented Jun 25, 2023

aheber commented Jun 25, 2023

sfenton3 commented Jun 26, 2023 •

edited

Szandor72 Jul 4, 2023

Szandor72 Jul 4, 2023

sfenton3 commented Aug 9, 2023

aheber commented Aug 9, 2023

sfenton3 commented Aug 12, 2023


		private List<RollupSummary> allSummaries;

		public static void clearRollupCache() {

Refactor Selector to Query Once #1360

Refactor Selector to Query Once #1360

Conversation

aheber commented Jun 17, 2023

Critical Changes

Issues Closed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Szandor72 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfenton3 commented Jun 25, 2023

aheber commented Jun 25, 2023

sfenton3 commented Jun 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfenton3 commented Aug 9, 2023

aheber commented Aug 9, 2023

sfenton3 commented Aug 12, 2023

sfenton3 commented Jun 26, 2023 •

edited