Skip to content

Commit

Permalink
Merge pull request #14 from carsdotcom/safe-attribute-mutator
Browse files Browse the repository at this point in the history
fix: Attribute mutators could throw exceptions, that also should be safe
  • Loading branch information
bbene authored Dec 4, 2024
2 parents 78d9eb6 + d562e91 commit 8a7dac4
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 6 deletions.
12 changes: 6 additions & 6 deletions app/JsonModel.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
Expand Down
36 changes: 36 additions & 0 deletions tests/Unit/JsonModelTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}

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

0 comments on commit 8a7dac4

Please sign in to comment.