Incrementing in a concurrent environment could generate mismatched data between model and db

[marius-mcp]

This is mind blowing...
#52694 (reply in thread)

The definition of incrementing is atomic and laravel made it different....

Again. This is incredible....

Update.

Now I see that only the property from the model is incremented in an non atomic way and that the query executed is like it should be.

If 2 concurrent processes update same row by incrementing a column the data from the model could be out of sync.

row: col:1

process 1: reads it, sees 1. Increments it => after increment the number will be 2.
process 2: reads it in the same time, sees 1. Increments it after the above increment => after increment the number will be 2 but in db it is 3.

[marius-mcp]

This is the query builder increment

     * Increment a column's value by a given amount.
     *
     * @param  string  $column
     * @param  float|int  $amount
     * @param  array  $extra
     * @return int
     *
     * @throws \InvalidArgumentException
     */
    public function increment($column, $amount = 1, array $extra = [])
    {
        if (! is_numeric($amount)) {
            throw new InvalidArgumentException('Non-numeric value passed to increment method.');
        }

        $wrapped = $this->grammar->wrap($column);

        $columns = array_merge([$column => $this->raw("$wrapped + $amount")], $extra);

        return $this->update($columns);
    }

But surprise. It is called when the model does not exist (INCREDIBLE AGAIN because it will increment all rows).
And it is called for the actual increment.

It is called from Model:

protected function incrementOrDecrement($column, $amount, $extra, $method)
    {
        $query = $this->newQueryWithoutRelationships();

        if (! $this->exists) {
            return $query->{$method}($column, $amount, $extra); // here will update all rows...
        }

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

        $this->forceFill($extra);

        if ($this->fireModelEvent('updating') === false) {
            return false;
        }

        return tap($this->setKeysForSaveQuery($query)->{$method}($column, $amount, $extra), function () use ($column) { // here it will increment
            $this->syncChanges();

            $this->fireModelEvent('updated', false);

            $this->syncOriginalAttribute($column);
        });
    }

[Sophist-UK]

@marius-mcp So it's all good and this discussion should be considered closed?

[marius-mcp]

Also a transaction is not needed for increment.

[Sophist-UK]

Nope, nope, nope.

If you want increment to be atomic you need to code it as a DB Query so that it updates immediately. An increment on a model should not increment the database until explicitly saved. (That is the difference IMO between using a model approach and using a DB Query approach.

What I want to see is consistency of approach rather than some adherence to a technical definition of what "increment" should or shouldn't do.

[marius-mcp]

I understand your point but still, I would not use a transaction.

[marius-mcp]

@Sophist-UK The decision of putting the atomic increment/decrement on a model was made a long time ago.

Now we have this code that:

• increments in model for the updating event
• then increments atomic on db
• then does not sync the incremented column

Changing these as you say would mean a breaking change.

Leaving it as it is would maybe bite some users that use the incremented value from the model after the update, instead of reading it from db again.

[Sophist-UK]

PHP has dealt with breaking changes for decades in order to transform from the mess it was in v3/v4 into the capable language we see today. You just need to have a long-term warning process that you follow whereby development environments get deprecation warnings pop up, but production environments don't.

IMO this needs to be dealt with by a combination of documentation warnings now, and deprecation warnings in Laravel 12 followed by these methods being removed in (say) Laravel 13 or Laravel 14.

[marius-mcp]

After reviewing the code my conclusion is that a documentation update is required. A code change to refresh the model wouldn't be the best option because it would be needed only sometimes.

The model's increment and decrement functions are protected but they can be called via __call magic method, so people will use them.

It is important to note that after increment or decrement, the model's value can be out of sync and if that value is to be used from the model after the increment/decrement, the model should be refreshed prior to being used.

Also this isClassDeviable:

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

will have no effect on the query being executed.
using $this->{$column} = will result in setAttribute being called anyway, which checks for casts , mutators etc.

Also incrementing a json column will generate an sql like json_col = json_col + 1 @rodrigopedra, so, increment should not be used. (#52694 (comment))

[Sophist-UK]

If the functions are protected so that they don't get called, then the documentation should be updated to tell people NOT to use them because of the inconsistencies that will be created, and perhaps they should be deprecated entirely.

[marius-mcp]

@rodrigopedra Found this interesting article about atomic json columns incrementing https://www.bennadel.com/blog/3935-atomically-incrementing-json-column-values-in-mysql-5-7-32-and-lucee-cfml-5-3-7-47.htm

@Sophist-UK I agree but still, it is a breaking change.

[macropay-solutions]

Elegant solution for the current situation:
macropay-solutions/laravel-crud-wizard-free@93c8e94

Update + macropay-solutions/laravel-crud-wizard-free@dc47398
inspired by #52786

@marius-mcp

Basically it will refresh only the incremented columns during the model's lifetime, only on first access (after the increment was done). Side effect will be that the issue of adding or subtracting floats or decimals with + and - will be avoided (some numbers can't be stored in binary as in 10 base, and they end up being stored like .99999999...). This is another issue of laravel/lumen increment by the way.

For testing this code can be used coupled with printing the sqls executed:

        $a = [];
        $incrementColumn = ''; // fill the column's name
        $l = Model::query()->firstOrFail();
        $a[] = \array_merge(['array initial'], $l->toArray());

        $l->increment($incrementColumn, '111.11');
        $a[] = \array_merge(['array i1'], $l->toArray());
        $a[] = \array_merge(['dirty i1'], $l->getDirty());
        $a[] = \array_merge(['original i1'], $l->getOriginal());

        var_dump($l->{$incrementColumn});
        var_dump($l->{$incrementColumn});
        $a[] = \array_merge(['array ia1'], $l->toArray());
        $a[] = \array_merge(['dirty ia1'], $l->getDirty());
        $a[] = \array_merge(['original ia1'], $l->getOriginal());

        $l->increment($incrementColumn, '222.22');
        $a[] = \array_merge(['array i2'], $l->toArray());
        $a[] = \array_merge(['dirty i2'], $l->getDirty());
        $a[] = \array_merge(['original i2'], $l->getOriginal());
        var_dump($l->{$incrementColumn});
        var_dump($l->{$incrementColumn});
        $a[] = \array_merge(['array ia2'], $l->toArray());
        $a[] = \array_merge(['dirty ia2'], $l->getDirty());
        $a[] = \array_merge(['original ia2'], $l->getOriginal());


        echo '<table border="1">';

        $i = 0;
        foreach ($a as $val) {
            if ($i === 0) {
                echo '<tr>';
                foreach ($val as $col => $v) {
                    echo "<td>$col</td>";
                }
                echo '</tr>';
            }
            $i++;
            echo '<tr>';
            foreach ($val as $col => $v) {
                echo "<td>$v</td>";
            }
            echo "</tr>";
        }
        echo '</table>';
        die;

[marius-mcp]

Thank you.

[marius-mcp]

Found a new issue related to this:

#52786

[macropay-solutions]

We issued a patch inspired by this and updated the answer.