diff --git a/app/JsonModel.php b/app/JsonModel.php index 06361a8..19df9f7 100644 --- 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,30 +421,12 @@ 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); } /** @@ -451,22 +434,30 @@ public function safeUpdate(array $attributes): void * 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 index be29ad9..4879557 100644 --- 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;