Skip to content

Commit

Permalink
Merge pull request #10 from carsdotcom/safe-update-includes-custom-sa…
Browse files Browse the repository at this point in the history 
…ving
  • Loading branch information
@bbene
bbene authored Nov 13, 2024
2 parents 3d75b3e + 9a62e46 commit 9f4bbc7
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 39 deletions.
45 changes: 19 additions & 26 deletions app/JsonModel.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand All @@ -475,8 +466,9 @@ public function safeUpdateRecursive(array $attributes, bool $isRootOfChange = tr
}
}
if ($isRootOfChange) {
$this->save();
return $this->save();
}
return true;
}

/**
Expand Down Expand Up @@ -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();
}

Expand Down
74 changes: 61 additions & 13 deletions tests/Unit/JsonModelTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ public function testLinkedJsonModelValidatesBeforeSave(): void
{
[$model, $jsonmodel] = $this->mockLinkedValidatedJsonModel();
SchemaValidator::shouldReceive('validateOrThrow')
->once()
->atLeast()->once()
->with(
$jsonmodel,
$jsonmodel::SCHEMA,
Expand Down Expand Up @@ -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
Expand All @@ -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.
*/
Expand All @@ -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;
Expand Down

0 comments on commit 9f4bbc7

Please sign in to comment.