Bypass accessor when using increment() or decrement() methods

[grimmdude]

Hi Everyone,

I noticed that incrementing/decrementing a database column with increment()/decrement() causes any custom accessor on that column attribute to be executed. In my case this is undesirable, so I ended up using an update() query instead.

It seems to me that accessors should never be executed when incrementing/decrementing a column, but maybe I'm missing a use case? Curious your thoughts.

I believe this is where the accessor is being fired:

framework/src/Illuminate/Database/Eloquent/Model.php

Lines 977 to 979 in b7c6287

	$this->{$column} = $this->isClassDeviable($column)
	? $this->deviateClassCastableAttribute($method, $column, $amount)
	: $this->{$column} + ($method === 'increment' ? $amount : $amount * -1);

PR submitted: #52701

[ghost]

$this->{$column} is generating that. as it is a 'get/read'.
It could be replaced by
$this->getAttributes()[$column] ?? null

[rodrigopedra]

It seems to me that accessors should never be executed when incrementing/decrementing a column

Why not? Can you elaborate?

If someone has an accessor to a JSON field stored in DB, then reading from the accessor would be mandatory.

[grimmdude]

Maybe I'm misunderstanding how increment() works then because I would expect it to operate purely as a database query, not based on the value of the instantiated model.

[rodrigopedra]

I guess it used to be like that some time ago. But if the increased usage of Attribute and casts, it was changed to take those into account.

In some sense, if it was a pure database operation, the assignment would have to be changed, as it is now it would trigger any mutator on the field.

Thus, as it would trigger a field's mutator, reading the value from its accessor makes more sense.

[rodrigopedra]

If you want to use a pure database approach, you can use the underlying Illuminate\Database\Query\Builder@increment() method instead:

User::query()->toBase()->increment('login_count');

framework/src/Illuminate/Database/Query/Builder.php

Lines 3924 to 3931 in a2c66ad

	public function increment($column, $amount = 1, array $extra = [])
	{
	if (! is_numeric($amount)) {
	throw new InvalidArgumentException('Non-numeric value passed to increment method.');
	}
	return $this->incrementEach([$column => $amount], $extra);
	}

[grimmdude]

Got it, thank you for that explanation. Basing off of model values has the potential for a race condition unless I'm mistaken. I will definitely start using that underlying increment() method instead for the pure database approach.

[Sophist-UK]

If there is a potential for a race condition then this should be tested and if necessary fixed.

@grimmdude Perhaps you could explain (in simple terms that dunces like me can understand) why you think that this might create a race condition?

[grimmdude]

I feel like I still must be misunderstanding, but if it's just incrementing based on model values then here's a very contrived race condition example:

// SELECT sent_emails FROM users where id = 1
// > 1

$u1 = User::find(1);
$u2 = User::find(1);

$u1->sent_emails = $u1->sent_emails + 1;
$u1->save();
// SELECT sent_emails FROM users where id = 1
// > 2

$u2->sent_emails = $u2->sent_emails + 1;
$u2->save();
// SELECT sent_emails FROM users where id = 1
// > 2 (should be 3)

You probably would never do this in a script, but it's possible to happen when processing a high volume of async jobs for instance.

I'm confused though because this is not how I understood increment() to work, and everywhere I read seems to imply this is not how it works. But if it's relying on model accessors to update the value then it certainly seems like this is the method it's using.

[Sophist-UK]

Yes - this is a race condition, however this is always an issue in Laravel - for example I have two users running the following at the same time:

$u = User::find(1);
$u->sent_emails = $u1->sent_emails + 1;
$u->save();

You normally have to code this inside a transaction to avoid this.