New Custom Casting For Eloquent Models Has Excessive set() calls #31778
Replies: 14 comments 21 replies
-
Going to leave this to Taylor to review |
Beta Was this translation helpful? Give feedback.
-
Yeah, probably worth exploring if we can get rid of some of those redundant calls for sure. |
Beta Was this translation helpful? Give feedback.
-
Even though nothing's broken, I'm gonna mark this as a bug so we can take a look at this later on. Appreciating prs in the meantime! |
Beta Was this translation helpful? Give feedback.
-
Suppose we set the attributes of Eloquent Model through the way specified in the test: $model->address = $address = new Address('110 Kingsbrook St.', 'My Childhood House');
$address->lineOne = '117 Spencer St.'; The What if we add a property like $classCastCache['address'] = unserialize(serialize($originalClassCastCache['address'])); And the protected function mergeAttributesFromClassCasts()
{
foreach ($this->classCastCache as $key => $value) {
if ($this->classCastCache[$key] == $this->originalClassCastCache[$key] ) {
continue;
}
$caster = $this->resolveCasterClass($key);
$this->attributes = array_merge(
$this->attributes,
$caster instanceof CastsInboundAttributes
? [$key => $value]
: $this->normalizeCastClassResponse($key, $caster->set($this, $key, $value, $this->attributes))
);
}
} |
Beta Was this translation helpful? Give feedback.
-
@ralphschindler I made a fairly simple change that had a yuge reduction in |
Beta Was this translation helpful? Give feedback.
-
I did some further investigation on this. There are
|
Beta Was this translation helpful? Give feedback.
-
@taylorotwell @ralphschindler I have done some work on the issue regards Excessive Would you mind to check / test this out - linaspasv/framework@06f189123bb17762d9473697201a1c831df77fac (updated - branch is based on master) ? I would love to get some feedback before PR submission. Thank you. My setup: $model = Model::find(1);
$model->options->get(10); // <--- ::get() is executed
// before the update used to execute ::set()
$model->options->get(20); // <--- reading from $classCastCache
// before the update used to execute ::set()
$model->options->put('test', 30); // <---- updating object at $classCastCache
// before the update:
// - 7 ::set() calls when custom cast value has been changed
// - 4 ::set() calls when custom cast value has not been changed
$model->save(); // <--- single set() is executed class Model extends Eloquent
{
protected $table = 'models';
protected $casts = [
'options' => CollectionCast::class
];
} class CollectionCast implements CastsAttributes
{
public function get($model, string $key, $value, array $attributes)
{
return new Collection(json_decode($value, true));
}
public function set($model, string $key, $value, array $attributes)
{
if ($value instanceof Collection) {
$value = $value->toArray();
}
return json_encode($value);
}
} |
Beta Was this translation helpful? Give feedback.
-
Maybe my thought in my previous comment isn't good solution. |
Beta Was this translation helpful? Give feedback.
-
I encountered this problem when trying to use separate classes to write and read some attribute. Reader class cannot be serialized with And exception is thrown when i tried to access model attribute: Maybe this will help someone. My custom cast class looks like: <?php
namespace App\Casts;
use Illuminate\Contracts\Database\Eloquent\CastsAttributes;
use App\Exceptions\ServiceResultCastException;
use App\Tools\Services\Contracts\ServiceResultReaderInterface;
use App\Tools\Services\MyService\ServiceResultReader;
class ServiceResult implements CastsAttributes
{
public function get($model, $key, $value, $attributes)
{
return ServiceResultReader::fromArray(json_decode($value, true));
}
public function set($model, $key, $value, $attributes)
{
if ($value instanceof ServiceResultReaderInterface) {
return [];
}
$result = json_encode($value);
if ($result === false) {
throw new ServiceResultCastException(
"Service result serialization failed"
);
}
return $result;
}
} |
Beta Was this translation helpful? Give feedback.
-
Hi @ralphschindler ! Is there any update on this issue? I also encountered your issue while I was creating a custom cast to simplify file uploads. Thank you for your contribution! 👍 Best regards, |
Beta Was this translation helpful? Give feedback.
-
Any chance we can get a solution to this in the main framework? I was rather confused and surprised why set was called so many times for my custom Caster that implements |
Beta Was this translation helpful? Give feedback.
-
@linaspasv said:
Yes - Taylor is understandably strict about accepting PRs which have any risk of creating backwards compatibility issues i.e. a breaking change within a Laravel version - breaking changes are reserved for new Laravel versions. But if these changes are straight forward and fix an obvious bug with no side effects, then he will accept it. I think the issue would come if you had a setter which did not produce the same result each time (as an example, say a counter that incremented every time there was a change) and where your code is now expecting the result from having been called multiple times. Yes, IMO this is an edge case rather than a common coding paradigm, nevertheless it might create a backwards compatibility issue. It is probably NOT too late to submit this as a (potentially) breaking change for Laravel 11 which is due out in a few weeks, especially if there is a pedigree of the trait working without issues for some time. |
Beta Was this translation helpful? Give feedback.
-
I noticed if add a bool property
The negative effects of doing so, cast object cannot be modified in the following way:
Should use below way instead:
|
Beta Was this translation helpful? Give feedback.
-
This seems to be another uncovered case that the framework will not handle. Can we have your permission to add your trait to our free MIT library laravel-crud-wizard-free? We already incorporated into it some uncovered/corner cases that the framework does not handle. Update: Your performUpdate(Builder $query) is not ok. Proposal inspired by #31778 (reply in thread): use Illuminate\Contracts\Database\Eloquent\CastsInboundAttributes;
use Illuminate\Database\ConnectionInterface;
/**
* If you are using casts, you can use this trait in your models to optimize performance OR DO NOT USE CASTS
*
* Inspired by https://github.com/linaspasv
* https://github.com/laravel/framework/discussions/31778#discussioncomment-7345461
*/
trait ModelCastsOptimizerTrait
{
/**
* Determine if the model or any of the given attribute(s) have been modified.
*
* @param array|string|null $attributes
*/
public function isDirty($attributes = null): bool
{
$attributes = \is_array($attributes ?? \array_keys($this->attributes)) ? $attributes : \func_get_args();
foreach ($attributes as $key) {
if (!\array_key_exists($key, $this->attributes)) {
continue;
}
$this->mergeAttributeFromClassCasts($key);
if (!$this->originalIsEquivalent($key)) {
return true;
}
}
return false;
}
/**
* Get an attribute from the model.
*
* @param string $key
* @return mixed
*/
public function getAttribute($key): mixed
{
if (isset($this->classCastCache[$key])) {
return $this->classCastCache[$key];
}
return parent::getAttribute($key);
}
/**
* Get an attribute from the $attributes array.
*
* @param string $key
*/
protected function getAttributeFromArray($key): mixed
{
$this->mergeAttributeFromClassCasts($key);
return $this->attributes[$key] ?? null;
}
/**
* Sync multiple original attribute with their current values.
*
* @param array|string $attributes
*/
public function syncOriginalAttributes($attributes): static
{
$attributes = \is_array($attributes) ? $attributes : \func_get_args();
foreach ($attributes as $attribute) {
$this->original[$attribute] = $this->getAttributeFromArray($attribute);
}
return $this;
}
/**
* Merge a single cast class attribute back into the model.
*/
protected function mergeAttributeFromClassCasts(string $key): void
{
if (!isset($this->classCastCache[$key])) {
return;
}
$value = $this->classCastCache[$key];
$caster = $this->resolveCasterClass($key);
$this->attributes = \array_merge(
$this->attributes,
$caster instanceof CastsInboundAttributes
? [$key => $value]
: $this->normalizeCastClassResponse($key, $caster->set($this, $key, $value, $this->attributes))
);
}
/**
* Merge the cast class attributes back into the model.
*/
protected function mergeAttributesFromClassCasts(): void
{
foreach (\array_keys($this->classCastCache) as $key) {
$this->mergeAttributeFromClassCasts($key);
}
}
/**
* Save the model to the database.
*
* @param array $options
* @return bool
*/
public function save(array $options = [])
{
$query = $this->newModelQuery();
// If the "saving" event returns false we'll bail out of the save and return
// false, indicating that the save failed. This provides a chance for any
// listeners to cancel save operations if validations fail or whatever.
if ($this->fireModelEvent('saving') === false) {
return false;
}
// If the model already exists in the database we can just update our record
// that is already in this database using the current IDs in this "where"
// clause to only update this model. Otherwise, we'll just insert them.
if ($this->exists) {
$saved = !$this->isDirty() || $this->performUpdate($query);
}
// If the model is brand new, we'll insert it into our database and set the
// ID attribute on the model to the value of the newly inserted row's ID
// which is typically an auto-increment value managed by the database.
else {
$saved = $this->performInsert($query);
if (
'' === (string)$this->getConnectionName()
&& ($connection = $query->getConnection()) instanceof ConnectionInterface
) {
$this->setConnection($connection->getName());
}
}
// If the model is successfully saved, we need to do a few more things once
// that is done. We will call the "saved" method here to run any actions
// we need to happen after a model gets successfully saved right here.
if ($saved) {
$this->finishSave($options);
}
return $saved;
}
/**
* Perform any actions that are necessary after the model is saved.
*/
protected function finishSave(array $options): void
{
$this->fireModelEvent('saved', false);
$wasChanged = ($this->wasRecentlyCreated && [] === $this->original) || $this->changes !== [];
if ($wasChanged && ($options['touch'] ?? true)) {
$this->touchOwners();
}
$this->original = $this->attributes;
}
} |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
n/a
Description:
When attempting to write a CustomCast that will convert between a complex Object and the serialization for the database,
CastAttributes::set()
is called an excessive amount of times. This could be problematic (and unnecessary) if the work to serialize is non-trivial (like converting points into a binary representation.)This is due to excessive calls to
HasAttributes::mergeAttributesFromClassCasts()
that originate from multiple places in bothModel::getAttributes()
andModel::save()
calls:save()
itself: https://github.com/laravel/framework/blob/master/src/Illuminate/Database/Eloquent/Model.php#L670save()
callingperformInsert()
callinggetAttributes()
:https://github.com/laravel/framework/blob/master/src/Illuminate/Database/Eloquent/Model.php#L827
save()
callingfinishSave()
callingisDirty()
callinggetDirty()
callinggetAttributes()
: https://github.com/laravel/framework/blob/master/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L1326save()
callingfinishSave()
callingsyncOriginal()
callinggetAttributes()
: https://github.com/laravel/framework/blob/master/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L1205There is probably more to unpack here, and perhaps custom attributes should only be doing lightweight transformation, but it feels like this could be a good tool if Eloquent could manage to only transform the values to a savable value once before insert/update. Should we continue this discussion?
Steps To Reproduce:
7.x...ralphschindler:custom-cast-excessive-calls(branch now based off master: master...ralphschindler:custom-cast-excessive-calls)
Beta Was this translation helpful? Give feedback.
All reactions