kirkbushell / eloquence

A drop-in library for certain database functionality in Laravel, that allows for extra features that may never make it into the main project.
MIT License
537 stars 60 forks source link

Add transaction to Cacheable increment and decrement #95

Closed Ravaelles closed 3 years ago

Ravaelles commented 3 years ago

It may happen that rapid incrementing/decrementing of count cache produces invalid values. Using a transaction to prevent it.

kirkbushell commented 3 years ago

@Ravaelles this won't do anything, as the increment query is already a single query. If you're looking to manage the race condition between select/update/increment, that will need to be done further up the chain.

Ravaelles commented 3 years ago

Yeah, minutes after I've created this PR I came to the same conclusion - it won't change anything as the record has already been fetched. I've mistaken Eloquent's increment function with mongodb's increment operation, which is pretty much relative +1 update of an attribute and would do exactly what I want here.

Ravaelles commented 3 years ago

@kirkbushell I initially thought Eloquence was operating on memory loaded Eloquent models to do increment/decrement, so the moment ->increment or ->decrement were called it might be possible to work with an outdated model, losing some operations if run in parallel.

But that is not the case! For both operations the config passed looks like this (lacks an actual model):

[
  "model" => "AwardForce\Modules\Fields\Models\Tab"
  "table" => "tabs"
  "field" => "field_count"
  "key" => "id"
  "foreignKey" => "tab_id"
]

and the resulting SQL query is value-agnostic e.g.

update `tabs` set `field_count` = `field_count` + 1 where `id` = 1380

Correct me if I'm wrong, but for Tabs <-> Fields it should be safe. Or at least I don't see any race condition here. What you say?