New Custom Casting For Eloquent Models Has Excessive set() calls #31778
Replies: 14 comments 19 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.
-
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