Bugfix: Fix an issue where belongs_to would trigger recursion

Author: chrispenny

src/Services/CacheProcessingService.php

@@ -58,10 +58,6 @@ private function updateEdge(EdgeUpdateDto $dto): array
                 return [];
             }
 
-            if ($this->alreadyProcessed($edge->getToClassName(), $instance->getField($edge->getRelation().'ID'))) {
-                return [];
-            }
-
             $relatedInstance = $instance->getField($edge->getRelation());
 
             if (!$relatedInstance) {
@@ -78,21 +74,17 @@ private function updateEdge(EdgeUpdateDto $dto): array
         }
 
         if ($relationType === 'has_many' || $relationType === 'many_many' || $relationType === 'belongs_many_many') {
-            return $this->updateInstances($instance->{$edge->getRelation()}(), $dto);
+            return $this->updateInstances($instance->{$edge->getRelation()}());
         }
 
         return [];
     }
 
-    private function updateInstances(DataList $instances, EdgeUpdateDto $dto): array
+    private function updateInstances(DataList $instances): array
     {
         $results = [];
 
         foreach ($instances as $relatedInstance) {
-            if ($this->alreadyProcessed($dto->getEdge()->getToClassName(), $relatedInstance->ID)) {
-                continue;
-            }
-
             $results = array_merge(
                 $results,
                 $this->updateInstance($relatedInstance)
@@ -104,6 +96,10 @@ private function updateInstances(DataList $instances, EdgeUpdateDto $dto): array
 
     private function updateInstance(DataObject $instance): array
     {
+        if ($this->alreadyProcessed($instance->ClassName, $instance->ID)) {
+            return [];
+        }
+
         // Find or create the CacheKey for this instance
         $cacheKey = CacheKey::updateOrCreateKey($instance);
         $processedUpdate = $this->getUpdatesService()->findOrCreateProcessedUpdate($instance->ClassName, $instance->ID);

tests/Mocks/Models/CaredHasOneNonVersionedModel.php

@@ -0,0 +1,31 @@
+<?php
+
+namespace Terraformers\KeysForCache\Tests\Mocks\Models;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\DataObject;
+use SilverStripe\ORM\HasManyList;
+use Terraformers\KeysForCache\Extensions\CacheKeyExtension;
+use Terraformers\KeysForCache\Tests\Mocks\Pages\CaresPage;
+
+/**
+ * This model is referenced by CaresPage as a has_one, meaning that this model has a has_many back to CaresPage
+ *
+ * @property string $Title
+ * @method HasManyList|CaresPage CaresPages()
+ * @mixin CacheKeyExtension
+ */
+class CaredHasOneNonVersionedModel extends DataObject implements TestOnly
+{
+    private static array $db = [
+        'Title' => 'Varchar',
+    ];
+
+    private static array $has_many = [
+        'CaresPages' => CaresPage::class,
+    ];
+
+    private static string $table_name = 'CaredHasOneNonVersionedModel';
+
+    private static bool $has_cache_key = false;
+}

tests/Mocks/Pages/CaresPage.php

@@ -12,6 +12,7 @@
 use Terraformers\KeysForCache\Tests\Mocks\Models\CaredBelongsToModel;
 use Terraformers\KeysForCache\Tests\Mocks\Models\CaredHasManyModel;
 use Terraformers\KeysForCache\Tests\Mocks\Models\CaredHasOneModel;
+use Terraformers\KeysForCache\Tests\Mocks\Models\CaredHasOneNonVersionedModel;
 use Terraformers\KeysForCache\Tests\Mocks\Models\CaredManyManyModel;
 use Terraformers\KeysForCache\Tests\Mocks\Models\CaredThroughModel;
 use Terraformers\KeysForCache\Tests\Mocks\Models\PolymorphicCaredHasManyModel;
@@ -20,9 +21,11 @@
 /**
  * @property int $CaredBelongsToModelID
  * @property int $CaredHasOneModelID
+ * @property int $CaredHasOneNonVersionedModelID
  * @property int $PolymorphicHasOneID
  * @method CaredBelongsToModel CaredBelongsToModel()
  * @method CaredHasOneModel CaredHasOneModel()
+ * @method CaredHasOneNonVersionedModel CaredHasOneNonVersionedModel()
  * @method DataObject PolymorphicHasOne()
  * @method HasManyList|CaredHasManyModel[] CaredHasManyModels()
  * @method HasManyList|CaresPageCaredThroughModel[] CaresPageCaredThroughModels()
@@ -35,6 +38,7 @@ class CaresPage extends Page implements TestOnly
     private static array $has_one = [
         'CaredBelongsToModel' => CaredBelongsToModel::class,
         'CaredHasOneModel' => CaredHasOneModel::class,
+        'CaredHasOneNonVersionedModel' => CaredHasOneNonVersionedModel::class,
         'PolymorphicHasOne' => DataObject::class,
     ];
 
@@ -56,6 +60,7 @@ class CaresPage extends Page implements TestOnly
     private static array $cares = [
         'CaredBelongsToModel',
         'CaredHasOneModel',
+        'CaredHasOneNonVersionedModel',
         'CaredHasManyModels',
         'CaredManyManyModels',
         'CaredThroughModels',

tests/RelationshipGraph/GraphTest.php

@@ -15,6 +15,7 @@
 use Terraformers\KeysForCache\Tests\Mocks\Models\CaredBelongsToModel;
 use Terraformers\KeysForCache\Tests\Mocks\Models\CaredHasManyModel;
 use Terraformers\KeysForCache\Tests\Mocks\Models\CaredHasOneModel;
+use Terraformers\KeysForCache\Tests\Mocks\Models\CaredHasOneNonVersionedModel;
 use Terraformers\KeysForCache\Tests\Mocks\Models\CaredManyManyModel;
 use Terraformers\KeysForCache\Tests\Mocks\Models\PolymorphicCaredHasManyModel;
 use Terraformers\KeysForCache\Tests\Mocks\Models\PolymorphicTouchedHasManyModel;
@@ -167,6 +168,7 @@ public function testGetRelationshipConfig(): void
         $expectPageTwoCares = [
             'CaredBelongsToModel' => CaredBelongsToModel::class,
             'CaredHasOneModel' => CaredHasOneModel::class,
+            'CaredHasOneNonVersionedModel' => CaredHasOneNonVersionedModel::class,
             'CaredHasManyModels' => CaredHasManyModel::class,
             'CaredManyManyModels' => CaredManyManyModel::class,
             'CaredThroughModels' => [

tests/Scenarios/CaresTest.php

@@ -9,6 +9,7 @@
 use Terraformers\KeysForCache\Tests\Mocks\Models\CaredBelongsToModel;
 use Terraformers\KeysForCache\Tests\Mocks\Models\CaredHasManyModel;
 use Terraformers\KeysForCache\Tests\Mocks\Models\CaredHasOneModel;
+use Terraformers\KeysForCache\Tests\Mocks\Models\CaredHasOneNonVersionedModel;
 use Terraformers\KeysForCache\Tests\Mocks\Models\CaredManyManyModel;
 use Terraformers\KeysForCache\Tests\Mocks\Models\CaredThroughModel;
 use Terraformers\KeysForCache\Tests\Mocks\Models\PolymorphicCaredHasManyModel;
@@ -30,6 +31,7 @@ class CaresTest extends SapphireTest
         CaredBelongsToModel::class,
         CaredHasManyModel::class,
         CaredHasOneModel::class,
+        CaredHasOneNonVersionedModel::class,
         CaredManyManyModel::class,
         CaredThroughModel::class,
         PolymorphicCaredHasOneModel::class,
@@ -120,6 +122,34 @@ public function testCaresHasOne(): void
         $this->assertNotEquals($originalKey, $newKey);
     }
 
+    public function testCaresHasOneNonVersioned(): void
+    {
+        // Updates are processed as part of scaffold, so we need to flush before we kick off
+        ProcessedUpdatesService::singleton()->flush();
+
+        $page = $this->objFromFixture(CaresPage::class, 'page1');
+        $model = $this->objFromFixture(CaredHasOneNonVersionedModel::class, 'model1');
+
+        // Check that we're set up correctly
+        $this->assertEquals(CaredHasOneNonVersionedModel::class, $model->ClassName);
+        $this->assertEquals($page->CaredHasOneNonVersionedModelID, $model->ID);
+
+        $originalKey = $page->getCacheKey();
+
+        $this->assertNotNull($originalKey);
+        $this->assertNotEmpty($originalKey);
+
+        // Begin changes
+        $model->forceChange();
+        $model->write();
+
+        $newKey = $page->getCacheKey();
+
+        $this->assertNotNull($newKey);
+        $this->assertNotEmpty($newKey);
+        $this->assertNotEquals($originalKey, $newKey);
+    }
+
     public function testPolymorphicCaresHasOne(): void
     {
         // Updates are processed as part of scaffold, so we need to flush before we kick off

tests/Scenarios/CaresTest.yml

@@ -6,6 +6,10 @@ Terraformers\KeysForCache\Tests\Mocks\Models\CaredHasOneModel:
   model1:
     Title: Has One Model 1
 
+Terraformers\KeysForCache\Tests\Mocks\Models\CaredHasOneNonVersionedModel:
+  model1:
+    Title: Has One Non Versioned Model 1
+
 Terraformers\KeysForCache\Tests\Mocks\Models\CaredHasManyModel:
   model1:
     Title: Has Many Model 1
@@ -31,6 +35,7 @@ Terraformers\KeysForCache\Tests\Mocks\Pages\CaresPage:
     Title: Page 1
     CaredBelongsToModel: =>Terraformers\KeysForCache\Tests\Mocks\Models\CaredBelongsToModel.model1
     CaredHasOneModel: =>Terraformers\KeysForCache\Tests\Mocks\Models\CaredHasOneModel.model1
+    CaredHasOneNonVersionedModel: =>Terraformers\KeysForCache\Tests\Mocks\Models\CaredHasOneNonVersionedModel.model1
     CaredHasManyModels:
       - =>Terraformers\KeysForCache\Tests\Mocks\Models\CaredHasManyModel.model1
     CaredManyManyModels:

tests/Scenarios/GlobalCaresTest.php

@@ -23,13 +23,12 @@ class GlobalCaresTest extends SapphireTest
 
     public function testGlobalCares(): void
     {
+        $siteConfig = SiteConfig::current_site_config();
+        $page = $this->objFromFixture(GlobalCaresPage::class, 'page1');
 
         // Updates are processed as part of scaffold, so we need to flush before we kick off
         ProcessedUpdatesService::singleton()->flush();
 
-        $siteConfig = SiteConfig::current_site_config();
-        $page = $this->objFromFixture(GlobalCaresPage::class, 'page1');
-
         // Check we're set up correctly
         $originalKey = $page->getCacheKey();