fix: Attribute mutators could throw exceptions, that also should be safe

carsdotcom · Dec 3, 2024 · d562e91 · d562e91
1 parent 78d9eb6
commit d562e91
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 6 deletions.
diff --git a/app/JsonModel.php b/app/JsonModel.php
@@ -445,13 +445,13 @@ public function safeUpdateRecursive(array $attributes, bool $isRootOfChange = tr
             $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, $caughtExceptions);
-            } else {
-                $this->{$key} = $updatedAttribute;
-            }
-
             try {
+                if ($this->{$key} instanceof JsonModel) {
+                    $this->{$key}->safeUpdateRecursive($updatedAttribute, false, $caughtExceptions);
+                } else {
+                    $this->{$key} = $updatedAttribute;
+                }
+
                 $canSave = $this->preSave();
                 if (!$canSave) {
                     throw new \DomainException("A saving handler on " . (new FriendlyClassName())($this) . " returned false but provided no reason.");

diff --git a/tests/Unit/JsonModelTest.php b/tests/Unit/JsonModelTest.php
@@ -881,6 +881,28 @@ public function testSafeUpdateRecursiveIncludesSavingHandlers(): void
             "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));
     }
+
+    public function testSafeUpdateRecursiveIncludesAttributeMutators(): void
+    {
+        $model = $this->makeMockModel();
+        $jsonModel = new CustomAttributeSetter($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 attribute mutator 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));
+    }
 }
 
 /**
@@ -911,6 +933,20 @@ protected static function boot()
     }
 }
 
+class CustomAttributeSetter extends JsonModel
+{
+    public const SCHEMA = '{"properties":{"shirt":{"enum":["red", "gold"]}}}';
+    public function setBestCaptainAttribute($value): void
+    {
+        if($value === 'Saru' ) {
+            $this->attributes['bestCaptain'] = $value;
+        } else {
+            throw new DomainException("Sorry, {$value} is not a valid choice for Best Star Trek Captain.");
+        }
+    }
+}
+
+
 class DownstreamModel extends JsonModel
 {
     use HasJsonModelAttributes;