Processed Updates Service minor performance improvement (#75)

* Processed Updates Service minor performance improvement * Make sure records are in DB before processing
silverstripe-terraformers · Oct 2, 2023 · d28ce60 · d28ce60
1 parent d3ce4d6
commit d28ce60
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 16 deletions.
diff --git a/src/Services/CacheProcessingService.php b/src/Services/CacheProcessingService.php
@@ -22,6 +22,13 @@ abstract protected function shouldPublishUpdates(): bool;
 
     public function processChange(DataObject $instance): void
     {
+        // Can't process a record that hasn't been saved to the Database. This would only happen if a developer
+        // specifically calls processChange() in their code. All module hooks for this method are triggered *after*
+        // write() type events
+        if (!$instance->isInDB()) {
+            return;
+        }
+
         $className = $instance->getClassName();
 
         // This record has already been processed in full. It is possible for multiple write() actions to be performed
@@ -163,13 +170,13 @@ private function alreadyProcessed(string $className, int $id): bool
             return false;
         }
 
-        // We are in a "Draft" context, so we don't care whether or not the ProcessedUpdateDTO has been published or
-        // not. Its existence means that it has been processed
+        // We are in a "Draft" context, so we don't care whether the ProcessedUpdateDTO has been published or not. Its
+        // existence means that it has been processed
         if (!$this->shouldPublishUpdates()) {
             return true;
         }
 
-        // We are in a "Live" context, so we need to return whether or not this ProcessedUpdateDTO has been published
+        // We are in a "Live" context, so we need to return whether this ProcessedUpdateDTO has been published
         return $processedUpdate->isPublished();
     }
 

diff --git a/src/Services/ProcessedUpdatesService.php b/src/Services/ProcessedUpdatesService.php
@@ -24,21 +24,15 @@ public function getProcessedUpdates(): array
 
     public function addProcessedUpdate(ProcessedUpdateDto $processedUpdate): void
     {
-        $this->processedUpdates[] = $processedUpdate;
+        $key = $this->getProcessedUpdateKey($processedUpdate->getClassName(), $processedUpdate->getId());
+        $this->processedUpdates[$key] = $processedUpdate;
     }
 
     public function findProcessedUpdate(string $className, int $id): ?ProcessedUpdateDto
     {
-        foreach ($this->processedUpdates as $processedUpdate) {
-            $classNameMatches = $processedUpdate->getClassName() === $className;
-            $idMatches = $processedUpdate->getId() === $id;
+        $key = $this->getProcessedUpdateKey($className, $id);
 
-            if ($idMatches && $classNameMatches) {
-                return $processedUpdate;
-            }
-        }
-
-        return null;
+        return $this->processedUpdates[$key] ?? null;
     }
 
     public function findOrCreateProcessedUpdate(string $className, int $id): ProcessedUpdateDto
@@ -50,9 +44,14 @@ public function findOrCreateProcessedUpdate(string $className, int $id): Process
         }
 
         $processedUpdate = new ProcessedUpdateDto($className, $id);
-        $this->processedUpdates[] = $processedUpdate;
+        $this->addProcessedUpdate($processedUpdate);
 
         return $processedUpdate;
     }
 
+    private function getProcessedUpdateKey(string $className, int $id): string
+    {
+        return sprintf('%s-%s', $className, $id);
+    }
+
 }
diff --git a/tests/Services/ProcessedUpdatesServiceTest.php b/tests/Services/ProcessedUpdatesServiceTest.php
@@ -16,11 +16,11 @@ public function testAddProcessedUpdate(): void
         $this->assertCount(0, $service->getProcessedUpdates());
 
         $service->addProcessedUpdate(new ProcessedUpdateDto(Page::class, 99));
-        // There are no checks for duplication between DTOs
+        // Existing DTOs would be overridden if an identical record is added
         $service->addProcessedUpdate(new ProcessedUpdateDto(Page::class, 98));
         $service->addProcessedUpdate(new ProcessedUpdateDto(Page::class, 98));
 
-        $this->assertCount(3, $service->getProcessedUpdates());
+        $this->assertCount(2, $service->getProcessedUpdates());
     }
 
     public function testFindProcessedUpdate(): void