safeUpdateRecursive should back out any changes that cause saving han…

…dler to exception or return false
carsdotcom · Nov 11, 2024 · 9a62e46 · 9a62e46
1 parent 3d75b3e
commit 9a62e46
Show file tree

Hide file tree

Showing 2 changed files with 80 additions and 39 deletions.
diff --git a/app/JsonModel.php b/app/JsonModel.php
@@ -19,6 +19,7 @@
 
 use ArrayAccess;
 use Carsdotcom\JsonSchemaValidation\Exceptions\JsonSchemaValidationException;
+use Carsdotcom\JsonSchemaValidation\Helpers\FriendlyClassName;
 use Carsdotcom\JsonSchemaValidation\Traits\ValidatesWithJsonSchema;
 use Carsdotcom\LaravelJsonModel\Contracts\CanCascadeEvents;
 use Carsdotcom\JsonSchemaValidation\Contracts\CanValidate;
@@ -420,53 +421,43 @@ public function updateRecursive(array $attributes, bool $isRootOfChange = true)
     }
 
     /**
-     * Given a set of changes that *might* contain some invalid data,
-     * take the good parts and throw out the rest.
-     * Assumes that $this was valid before the changes, and that each key could be an independent change,
-     * so if you have validation states where two attributes have to agree, choose `update` instead.
-     * @param array $attributes
-     * @return void
+     * @deprecated It is always safer to use safeUpdateRecursive, so this non-recursive variant is deprecated
+     * "If I am only for myself, what am I?"
      */
     public function safeUpdate(array $attributes): void
     {
-        foreach ($attributes as $key => $updatedAttribute) {
-            $wasSet = isset($this->{$key});
-            $previousValue = $this->{$key}; // __get will fill null even if it wasn't null
-            $this->{$key} = $updatedAttribute;
-            try {
-                $this->validateOrThrow();
-            } catch (JsonSchemaValidationException) {
-                if ($wasSet) {
-                    $this->{$key} = $previousValue;
-                } else {
-                    unset($this->{$key});
-                }
-            }
-        }
-        $this->save();
+        $this->safeUpdateRecursive($attributes);
     }
 
     /**
      * Given a set of changes that *might* contain some invalid data,
      * take the good parts and throw out the rest.
      * Assumes that $this was valid before the changes, and that each key could be an independent change,
      * so if you have validation states where two attributes have to agree, choose `update` instead.
+     * @return bool was save successful?
      */
-    public function safeUpdateRecursive(array $attributes, bool $isRootOfChange = true): void
+    public function safeUpdateRecursive(array $attributes, bool $isRootOfChange = true, array &$caughtExceptions = null): bool
     {
+        if ($caughtExceptions === null) {
+            $caughtExceptions = [];
+        }
         foreach ($attributes as $key => $updatedAttribute) {
             $wasSet = isset($this->{$key});
             $previousValue = $this->{$key}; // __get will fill null even if it wasn't null
 
             if ($this->{$key} instanceof JsonModel) {
-                $this->{$key}->safeUpdateRecursive($updatedAttribute, false);
+                $this->{$key}->safeUpdateRecursive($updatedAttribute, false, $caughtExceptions);
             } else {
                 $this->{$key} = $updatedAttribute;
             }
 
             try {
-                $this->validateOrThrow();
-            } catch (JsonSchemaValidationException) {
+                $canSave = $this->preSave();
+                if (!$canSave) {
+                    throw new \DomainException("A saving handler on " . (new FriendlyClassName())($this) . " returned false but provided no reason.");
+                }
+            } catch (\Throwable $e) {
+                $caughtExceptions[] = $e;
                 if ($wasSet) {
                     $this->{$key} = $previousValue;
                 } else {
@@ -475,8 +466,9 @@ public function safeUpdateRecursive(array $attributes, bool $isRootOfChange = tr
             }
         }
         if ($isRootOfChange) {
-            $this->save();
+            return $this->save();
         }
+        return true;
     }
 
     /**
@@ -587,6 +579,7 @@ public function preSave(): bool
         if ($this->fireModelEvent('saving') === false) {
             return false;
         }
+        $this->validateOrThrow(); // We validate after the handlers, because the handlers could clean up data, validate never does
         return $this->cascadePreSave();
     }
 

diff --git a/tests/Unit/JsonModelTest.php b/tests/Unit/JsonModelTest.php
@@ -386,7 +386,7 @@ public function testLinkedJsonModelValidatesBeforeSave(): void
     {
         [$model, $jsonmodel] = $this->mockLinkedValidatedJsonModel();
         SchemaValidator::shouldReceive('validateOrThrow')
-            ->once()
+            ->atLeast()->once()
             ->with(
                 $jsonmodel,
                 $jsonmodel::SCHEMA,
@@ -808,17 +808,12 @@ public function testSafeUpdate(): void
             'first_name' => 'Jeremy',
         ];
 
-        $model = mock(Model::class)->makePartial();
+        $model = $this->makeMockModel();
         $jsonModel = new class ($model, 'data') extends EventedJsonModel {
             const SCHEMA = 'person.json';
         };
         $jsonModel->mobile_phone = '8885551111';
 
-        $model
-            ->shouldReceive('save')
-            ->once()
-            ->andReturn(true);
-
         $jsonModel->safeUpdate($mixedChanges);
         self::assertFalse(isset($jsonModel->email)); // attribute is invalid, revert to unset
         self::assertSame('8885551111', $jsonModel->mobile_phone); // attribute is invalid, revert to previous value
@@ -835,21 +830,59 @@ public function testSafeUpdateRecursive(): void
             ],
         ];
 
-        $model = mock(Model::class)->makePartial();
+        $model = $this->makeMockModel();
         $jsonModel = new Person($model, 'data');
         $jsonModel->address->country = 'CA';
 
-        $model
-            ->shouldReceive('save')
-            ->once()
-            ->andReturn(true);
-
         $jsonModel->safeUpdateRecursive($mixedChanges);
         self::assertFalse(isset($jsonModel->email), 'attribute is invalid, should revert to unset');
         self::assertSame('CA', $jsonModel->address->country, 'attribute is invalid, revert to previous value');
         self::assertSame('Jeremy', $jsonModel->first_name, 'attribute is valid, set');
     }
+
+    public function makeMockModel(): Model
+    {
+        $model = mock(Model::class)->makePartial();
+        $model
+            ->shouldReceive('save')
+            ->andReturn(true);
+        return $model;
+    }
+
+    public function testSafeUpdateRecursiveIncludesSavingHandlers(): void
+    {
+        $model = $this->makeMockModel();
+        $jsonModel = new CustomSavingHandler($model, 'data');
+        $caughtExceptions = [];
+        $jsonModel->safeUpdateRecursive(['shirt' => 'red', 'bestCaptain' => 'Solo'], caughtExceptions: $caughtExceptions);
+        self::assertCanonicallySame(['shirt' => 'red'], $jsonModel);
+        self::assertCanonicallySame(["Sorry, Solo is not a valid choice for Best Star Trek Captain."], array_map(fn ($e) => $e->getMessage(), $caughtExceptions));
+
+        // can mix schema and custom handler problems
+        $caughtExceptions = [];
+        $jsonModel->safeUpdateRecursive(['shirt' => 'orange', 'bestCaptain' => 'Starbuck', 'tribbles' => 14], caughtExceptions: $caughtExceptions);
+        self::assertCanonicallySame([
+            'shirt' => 'red', // orange is invalid in the schema, reverted
+            'tribbles' => 14
+        ], $jsonModel);
+        self::assertCanonicallySame([
+            "The properties must match schema: shirt\nThe data should match one item from enum",
+            "Sorry, Starbuck is not a valid choice for Best Star Trek Captain."
+        ], array_map(fn ($e) => ($e instanceof JsonSchemaValidationException) ? $e->errorsAsMultilineString() : $e->getMessage(), $caughtExceptions));
+
+        // False saving handler is also a problem.
+        $caughtExceptions = [];
+        $jsonModel->safeUpdateRecursive(['tribbles' => 101], caughtExceptions: $caughtExceptions);
+        self::assertCanonicallySame([
+            'shirt' => 'red',
+            'tribbles' => 14, // excess tribbles caused saving to return false, that gets reverted but no message
+        ], $jsonModel);
+        self::assertCanonicallySame([
+            "A saving handler on Custom Saving Handler returned false but provided no reason."
+        ], array_map(fn ($e) => ($e instanceof JsonSchemaValidationException) ? $e->errorsAsMultilineString() : $e->getMessage(), $caughtExceptions));
+    }
 }
+
 /**
  * These are classes and models needed to test inheritance, etc.
  */
@@ -863,6 +896,21 @@ class UpstreamModel extends JsonModel
     ];
 }
 
+class CustomSavingHandler extends JsonModel
+{
+    public const SCHEMA = '{"properties":{"shirt":{"enum":["red", "gold"]}}}';
+    protected static function boot()
+    {
+        parent::boot();
+        static::saving(function (self $model) {
+            if ($model->isDirty('bestCaptain') && $model->bestCaptain !== 'Saru' ) {
+                throw new DomainException("Sorry, {$model->bestCaptain} is not a valid choice for Best Star Trek Captain.");
+            }
+            return $model->tribbles < 99;
+        });
+    }
+}
+
 class DownstreamModel extends JsonModel
 {
     use HasJsonModelAttributes;