fix: batch automation rule application (#435)
## Sentry - Issue: PHP-LARAVEL-2C - URL: https://whisper-money.sentry.io/issues/122336983/ ## Root cause Applying an automation rule to multiple transactions used per-transaction `saveQuietly()` and `syncWithoutDetaching()`. Sentry detected repeated transaction updates and pivot lookups/inserts on `/settings/automation-rules/{automationRule}/apply`. ## Fix - Added batched automation rule application for transaction collections. - Bulk updates category changes in one query. - Bulk inserts missing label pivots with `insertOrIgnore`. - Reused batching from sync controller path and queued job chunks. ## Verification - `vendor/bin/pint --dirty --format agent` - `php artisan test --compact tests/Feature/AutomationRuleApplicationTest.php`
This commit is contained in:
parent
d5d22b9d57
commit
606093d311
|
|
@ -96,8 +96,6 @@ class AutomationRuleApplicationController extends Controller
|
|||
}
|
||||
|
||||
if ($total <= self::SYNC_THRESHOLD) {
|
||||
$changed = 0;
|
||||
|
||||
$transactions = Transaction::query()
|
||||
->where('user_id', $automationRule->user_id)
|
||||
->whereIn('id', $matchingIds)
|
||||
|
|
@ -105,11 +103,7 @@ class AutomationRuleApplicationController extends Controller
|
|||
->with(['account.bank', 'category', 'labels'])
|
||||
->get();
|
||||
|
||||
foreach ($transactions as $transaction) {
|
||||
if ($service->applyRuleActions($transaction, $automationRule)) {
|
||||
$changed++;
|
||||
}
|
||||
}
|
||||
$changed = $service->applyRuleActionsToTransactions($transactions, $automationRule);
|
||||
|
||||
$applied = $transactions->count();
|
||||
|
||||
|
|
|
|||
|
|
@ -46,14 +46,11 @@ class ApplySingleAutomationRuleJob implements ShouldQueue
|
|||
->whereNull('description_iv')
|
||||
->with(['account.bank', 'category', 'labels'])
|
||||
->chunkById(100, function ($transactions) use ($service, $rule, $total, &$processed, &$applied, &$changed) {
|
||||
foreach ($transactions as $transaction) {
|
||||
if ($service->applyRuleActions($transaction, $rule)) {
|
||||
$changed++;
|
||||
}
|
||||
$applied++;
|
||||
$processed++;
|
||||
$this->updateProgress(status: 'processing', processed: $processed, total: $total, applied: $applied, updated: $changed);
|
||||
}
|
||||
$changed += $service->applyRuleActionsToTransactions($transactions, $rule);
|
||||
$applied += $transactions->count();
|
||||
$processed += $transactions->count();
|
||||
|
||||
$this->updateProgress(status: 'processing', processed: $processed, total: $total, applied: $applied, updated: $changed);
|
||||
});
|
||||
|
||||
$this->updateProgress(status: 'done', processed: $processed, total: $total, applied: $applied, updated: $changed);
|
||||
|
|
|
|||
|
|
@ -3,8 +3,11 @@
|
|||
namespace App\Services;
|
||||
|
||||
use App\Models\AutomationRule;
|
||||
use App\Models\LabelTransaction;
|
||||
use App\Models\Transaction;
|
||||
use Illuminate\Database\Eloquent\Collection as EloquentCollection;
|
||||
use Illuminate\Support\Collection;
|
||||
use Illuminate\Support\Str;
|
||||
use JWadhams\JsonLogic;
|
||||
|
||||
class AutomationRuleService
|
||||
|
|
@ -57,14 +60,82 @@ class AutomationRuleService
|
|||
}
|
||||
|
||||
/**
|
||||
* Apply a single rule's actions to the transaction, ignoring rule
|
||||
* priority and other rules. The transaction must already match the rule.
|
||||
*
|
||||
* Returns true if any attribute of the transaction was changed.
|
||||
* @param EloquentCollection<int, Transaction> $transactions
|
||||
*/
|
||||
public function applyRuleActions(Transaction $transaction, AutomationRule $rule): bool
|
||||
public function applyRuleActionsToTransactions(EloquentCollection $transactions, AutomationRule $rule): int
|
||||
{
|
||||
return $this->applyActions($transaction, $rule);
|
||||
if ($transactions->isEmpty()) {
|
||||
return 0;
|
||||
}
|
||||
|
||||
$rule->loadMissing('labels');
|
||||
$transactions->loadMissing('labels');
|
||||
|
||||
$changedTransactionIds = [];
|
||||
|
||||
if ($rule->action_category_id !== null) {
|
||||
$categoryTransactionIds = $transactions
|
||||
->filter(fn (Transaction $transaction): bool => $transaction->category_id !== $rule->action_category_id)
|
||||
->pluck('id')
|
||||
->all();
|
||||
|
||||
if ($categoryTransactionIds !== []) {
|
||||
Transaction::query()
|
||||
->whereIn('id', $categoryTransactionIds)
|
||||
->update([
|
||||
'category_id' => $rule->action_category_id,
|
||||
'updated_at' => now(),
|
||||
]);
|
||||
|
||||
foreach ($categoryTransactionIds as $transactionId) {
|
||||
$changedTransactionIds[$transactionId] = true;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if ($rule->action_note && $rule->action_note_iv === null) {
|
||||
foreach ($transactions as $transaction) {
|
||||
$existingNotes = $transaction->notes ?? '';
|
||||
|
||||
if ($this->noteAlreadyPresent($existingNotes, $rule->action_note)) {
|
||||
continue;
|
||||
}
|
||||
|
||||
$transaction->notes = $existingNotes
|
||||
? $existingNotes."\n".$rule->action_note
|
||||
: $rule->action_note;
|
||||
$transaction->saveQuietly();
|
||||
$changedTransactionIds[$transaction->id] = true;
|
||||
}
|
||||
}
|
||||
|
||||
$labelIds = $rule->labels->pluck('id')->all();
|
||||
if ($labelIds !== []) {
|
||||
$now = now();
|
||||
$labelTransactionRows = [];
|
||||
|
||||
foreach ($transactions as $transaction) {
|
||||
$transactionLabelIds = $transaction->labels->pluck('id')->all();
|
||||
$missingLabelIds = array_diff($labelIds, $transactionLabelIds);
|
||||
|
||||
foreach ($missingLabelIds as $labelId) {
|
||||
$labelTransactionRows[] = [
|
||||
'id' => (string) Str::uuid(),
|
||||
'label_id' => $labelId,
|
||||
'transaction_id' => $transaction->id,
|
||||
'created_at' => $now,
|
||||
'updated_at' => $now,
|
||||
];
|
||||
$changedTransactionIds[$transaction->id] = true;
|
||||
}
|
||||
}
|
||||
|
||||
if ($labelTransactionRows !== []) {
|
||||
LabelTransaction::query()->insertOrIgnore($labelTransactionRows);
|
||||
}
|
||||
}
|
||||
|
||||
return count($changedTransactionIds);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -189,6 +189,52 @@ test('apply endpoint runs synchronously when matches are below threshold', funct
|
|||
)->toBe(3);
|
||||
});
|
||||
|
||||
test('apply endpoint batches category and label writes', function () {
|
||||
Queue::fake();
|
||||
|
||||
$label = Label::factory()->create(['user_id' => $this->user->id]);
|
||||
$this->rule->labels()->attach($label);
|
||||
|
||||
Transaction::factory()->enableBanking()->count(5)->create([
|
||||
'user_id' => $this->user->id,
|
||||
'account_id' => $this->account->id,
|
||||
'category_id' => null,
|
||||
'description' => 'Grocery Store',
|
||||
'amount' => -1000,
|
||||
]);
|
||||
|
||||
$queries = [];
|
||||
DB::listen(function ($query) use (&$queries): void {
|
||||
$queries[] = $query->sql;
|
||||
});
|
||||
|
||||
$response = $this->actingAs($this->user)
|
||||
->postJson(route('automation-rules.apply', $this->rule), [
|
||||
'only_uncategorized' => true,
|
||||
]);
|
||||
|
||||
$transactionUpdateQueries = collect($queries)
|
||||
->filter(fn (string $query): bool => str_contains($query, 'update "transactions" set')
|
||||
|| str_contains($query, 'update `transactions` set'));
|
||||
$perTransactionPivotLookupQueries = collect($queries)
|
||||
->filter(fn (string $query): bool => (str_contains($query, 'from "label_transaction"')
|
||||
|| str_contains($query, 'from `label_transaction`'))
|
||||
&& (str_contains($query, '"label_transaction"."transaction_id" =')
|
||||
|| str_contains($query, '`label_transaction`.`transaction_id` =')));
|
||||
|
||||
$response->assertOk()
|
||||
->assertJsonPath('status', 'done')
|
||||
->assertJsonPath('applied', 5)
|
||||
->assertJsonPath('updated', 5)
|
||||
->assertJsonPath('total', 5);
|
||||
|
||||
expect($transactionUpdateQueries)->toHaveCount(1)
|
||||
->and($perTransactionPivotLookupQueries)->toHaveCount(0);
|
||||
|
||||
Queue::assertNothingPushed();
|
||||
$this->assertDatabaseCount('label_transaction', 5);
|
||||
});
|
||||
|
||||
test('apply endpoint queues a job when matches exceed threshold', function () {
|
||||
Queue::fake();
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue