fix(automation): avoid rule preview n+1 (#431)
## Sentry issue - PHP-LARAVEL-2F: https://whisper-money.sentry.io/issues/122581787/ ## Root cause - Automation rule match preview eagerly loaded account, bank, category, and labels for every 500-transaction chunk, even for rules that only inspect description fields. - Sentry flagged repeated account/bank eager-load queries across chunks as an N+1 pattern on `/settings/automation-rules/{automationRule}/matches`. ## Fix - Detect variables used by an automation rule and eager load only relationships required for evaluation. - Keep label eager loading only when label-only skip logic needs it. - Avoid lazy loading unused relationships while preserving full data shape for rule evaluation. - Update the Sentry prompt to use the `sentry` CLI workflow. ## Verification - `vendor/bin/pint --dirty --format agent` - `php artisan test --compact tests/Feature/AutomationRuleApplicationTest.php` - `php artisan test --compact tests/Feature/AutomationRuleEvaluationTest.php tests/Feature/AutomationRuleApplicationTest.php`
This commit is contained in:
parent
0d4c68361a
commit
fd67cf7c72
|
|
@ -15,7 +15,8 @@ Goal:
|
||||||
Rules:
|
Rules:
|
||||||
- Protect local work. Start with `git status --short`. If uncommitted changes exist, stop and ask before branching.
|
- Protect local work. Start with `git status --short`. If uncommitted changes exist, stop and ask before branching.
|
||||||
- Never expose secrets. Do not print tokens, `.env`, Sentry auth, DB URLs, or PII.
|
- Never expose secrets. Do not print tokens, `.env`, Sentry auth, DB URLs, or PII.
|
||||||
- Use Sentry MCP tools first: `find_organizations`, `find_projects`, `search_issues`, `get_sentry_resource`, `search_issue_events`, `get_issue_tag_values`, `get_replay_details`, `get_profile_details`, `analyze_issue_with_seer` when root cause unclear.
|
- Use the Sentry CLI (`sentry`) first. Prefer stored OAuth login over env build tokens; if `SENTRY_AUTH_TOKEN` is invalid or too narrow, run CLI commands as `env -u SENTRY_AUTH_TOKEN sentry ...` unless `SENTRY_FORCE_ENV_TOKEN=1` is intentionally set.
|
||||||
|
- Never print raw Sentry JSON that may contain PII. Redact emails, user IDs when summarizing. Keep secrets out of output.
|
||||||
- For Laravel ecosystem changes, use `application-info` and `search-docs` before code changes.
|
- For Laravel ecosystem changes, use `application-info` and `search-docs` before code changes.
|
||||||
- Activate/read relevant project skills when touched: Pest tests, Inertia React, Wayfinder, Tailwind, Fortify, Pennant.
|
- Activate/read relevant project skills when touched: Pest tests, Inertia React, Wayfinder, Tailwind, Fortify, Pennant.
|
||||||
- Every code change needs programmatic verification. Add or update a focused Pest/test when feasible. Run minimum affected tests. Run `vendor/bin/pint --dirty --format agent` after PHP edits.
|
- Every code change needs programmatic verification. Add or update a focused Pest/test when feasible. Run minimum affected tests. Run `vendor/bin/pint --dirty --format agent` after PHP edits.
|
||||||
|
|
@ -23,17 +24,24 @@ Rules:
|
||||||
|
|
||||||
Workflow:
|
Workflow:
|
||||||
1. Identify issue:
|
1. Identify issue:
|
||||||
- If `$ARGUMENTS` is a Sentry URL, fetch it with `get_sentry_resource(url=...)`.
|
- Confirm auth and org/project access with `sentry auth status`, `sentry org list --json`, and `sentry project list <org> --json` when needed.
|
||||||
- If `$ARGUMENTS` is a bare issue id, discover org/project if needed, then fetch issue with `get_sentry_resource(resourceType="issue", resourceId=<id>, organizationSlug=<org>)`.
|
- If `$ARGUMENTS` is a Sentry URL, extract `/issues/<numeric-id>/` or the visible short issue id, then fetch it with `sentry issue view <issue> --json --fields id,shortId,title,culprit,permalink,level,status,substatus,count,userCount,firstSeen,lastSeen,project,metadata,priority,platform,isUnhandled`.
|
||||||
- If no args, find org/project, search unresolved production issues with both frequency and user sorting, compare top results, and pick one with best impact score: high event count, high user count, recent lastSeen, production environment, clear actionable stack.
|
- If `$ARGUMENTS` is a bare issue id or short id, fetch it with `sentry issue view <issue> --json --fields id,shortId,title,culprit,permalink,level,status,substatus,count,userCount,firstSeen,lastSeen,project,metadata,priority,platform,isUnhandled`.
|
||||||
|
- If no args, list unresolved production issues with both frequency and user sorting, then compare top results:
|
||||||
|
- `sentry issue list <org>/<project> --query 'is:unresolved environment:production' --sort freq --limit 25 --json --fields id,shortId,title,culprit,count,userCount,lastSeen,permalink,priority,metadata,project`
|
||||||
|
- `sentry issue list <org>/<project> --query 'is:unresolved environment:production' --sort user --limit 25 --json --fields id,shortId,title,culprit,count,userCount,lastSeen,permalink,priority,metadata,project`
|
||||||
|
- Pick best impact score: high event count, high user count, recent lastSeen, production environment, clear actionable stack.
|
||||||
- Record chosen short issue id and Sentry URL in notes.
|
- Record chosen short issue id and Sentry URL in notes.
|
||||||
2. Branch:
|
2. Branch:
|
||||||
- Derive branch name from Sentry short issue id only, e.g. `WHISPER-MONEY-123`.
|
- Derive branch name from Sentry short issue id only, e.g. `WHISPER-MONEY-123`.
|
||||||
- Run `git switch -c <issue-id>`; if branch exists, `git switch <issue-id>`.
|
- Run `git switch -c <issue-id>`; if branch exists, `git switch <issue-id>`.
|
||||||
3. Investigate:
|
3. Investigate:
|
||||||
- Fetch latest events, stack trace, breadcrumbs, environment, release, tags, URLs/routes, affected users count, and relevant traces/replays/profiles if present.
|
- Fetch latest issue details and spans with `sentry issue view <issue> --spans all --json`.
|
||||||
|
- Fetch recent events with `sentry issue events <issue> --limit 10 --full --json`.
|
||||||
|
- Extract stack trace, breadcrumbs, environment, release, tags, URLs/routes, affected users count, trace/span data, and suspect queries. Redact PII in notes.
|
||||||
|
- For event details if needed, use `sentry event view <event-id> --json` or `sentry api <endpoint> --json`.
|
||||||
- Reproduce locally using tests or focused command. Inspect app logs/browser logs if relevant.
|
- Reproduce locally using tests or focused command. Inspect app logs/browser logs if relevant.
|
||||||
- If root cause not obvious after issue/event data, run Seer analysis.
|
- If root cause not obvious after issue/event data, run Seer with `sentry issue explain <issue> --json` and/or `sentry issue plan <issue> --json`.
|
||||||
4. Fix:
|
4. Fix:
|
||||||
- Read nearby code and conventions first.
|
- Read nearby code and conventions first.
|
||||||
- Implement minimal fix.
|
- Implement minimal fix.
|
||||||
|
|
|
||||||
|
|
@ -177,10 +177,15 @@ class AutomationRuleApplicationController extends Controller
|
||||||
|
|
||||||
$ids = [];
|
$ids = [];
|
||||||
|
|
||||||
|
$eagerLoads = $service->eagerLoadsForRuleEvaluation($rule);
|
||||||
|
if ($onlyUncategorized && $rule->action_category_id === null) {
|
||||||
|
$eagerLoads[] = 'labels';
|
||||||
|
}
|
||||||
|
|
||||||
Transaction::query()
|
Transaction::query()
|
||||||
->where('user_id', $rule->user_id)
|
->where('user_id', $rule->user_id)
|
||||||
->whereNull('description_iv')
|
->whereNull('description_iv')
|
||||||
->with(['account.bank', 'category', 'labels'])
|
->with(array_values(array_unique($eagerLoads)))
|
||||||
->orderByDesc('transaction_date')
|
->orderByDesc('transaction_date')
|
||||||
->orderByDesc('created_at')
|
->orderByDesc('created_at')
|
||||||
->chunkById(500, function ($transactions) use ($rule, $service, $onlyUncategorized, &$ids) {
|
->chunkById(500, function ($transactions) use ($rule, $service, $onlyUncategorized, &$ids) {
|
||||||
|
|
|
||||||
|
|
@ -45,7 +45,7 @@ class AutomationRuleService
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
$transactionData = $this->prepareTransactionData($transaction);
|
$transactionData = $this->prepareTransactionData($transaction, $rule);
|
||||||
|
|
||||||
try {
|
try {
|
||||||
$normalizedRulesJson = $this->normalizeRuleJson($rule->rules_json);
|
$normalizedRulesJson = $this->normalizeRuleJson($rule->rules_json);
|
||||||
|
|
@ -91,15 +91,37 @@ class AutomationRuleService
|
||||||
return empty(array_diff($ruleLabelIds, $transactionLabelIds));
|
return empty(array_diff($ruleLabelIds, $transactionLabelIds));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* @return array<int, string>
|
||||||
|
*/
|
||||||
|
public function eagerLoadsForRuleEvaluation(AutomationRule $rule): array
|
||||||
|
{
|
||||||
|
$variables = $this->ruleVariables($rule);
|
||||||
|
$eagerLoads = [];
|
||||||
|
|
||||||
|
if (in_array('bank_name', $variables, true)) {
|
||||||
|
$eagerLoads[] = 'account.bank';
|
||||||
|
} elseif (in_array('account_name', $variables, true)) {
|
||||||
|
$eagerLoads[] = 'account';
|
||||||
|
}
|
||||||
|
|
||||||
|
if (in_array('category', $variables, true)) {
|
||||||
|
$eagerLoads[] = 'category';
|
||||||
|
}
|
||||||
|
|
||||||
|
return $eagerLoads;
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* @return array{description: string, amount: float, transaction_date: string, bank_name: string, account_name: string, category: string|null, notes: string|null}
|
* @return array{description: string, amount: float, transaction_date: string, bank_name: string, account_name: string, category: string|null, notes: string|null}
|
||||||
*/
|
*/
|
||||||
private function prepareTransactionData(Transaction $transaction): array
|
private function prepareTransactionData(Transaction $transaction, ?AutomationRule $rule = null): array
|
||||||
{
|
{
|
||||||
$transaction->loadMissing(['account.bank', 'category']);
|
$transaction->loadMissing($rule ? $this->eagerLoadsForRuleEvaluation($rule) : ['account.bank', 'category']);
|
||||||
|
|
||||||
$account = $transaction->account;
|
$account = $transaction->relationLoaded('account') ? $transaction->account : null;
|
||||||
$bank = $account?->bank;
|
$bank = $account?->relationLoaded('bank') ? $account->bank : null;
|
||||||
|
$category = $transaction->relationLoaded('category') ? $transaction->category : null;
|
||||||
|
|
||||||
$accountName = '';
|
$accountName = '';
|
||||||
if ($account && ! $account->encrypted) {
|
if ($account && ! $account->encrypted) {
|
||||||
|
|
@ -112,7 +134,7 @@ class AutomationRuleService
|
||||||
'transaction_date' => $transaction->transaction_date->format('Y-m-d'),
|
'transaction_date' => $transaction->transaction_date->format('Y-m-d'),
|
||||||
'bank_name' => mb_strtolower($bank->name ?? ''),
|
'bank_name' => mb_strtolower($bank->name ?? ''),
|
||||||
'account_name' => mb_strtolower($accountName),
|
'account_name' => mb_strtolower($accountName),
|
||||||
'category' => $transaction->category?->name,
|
'category' => $category?->name,
|
||||||
'notes' => $transaction->notes
|
'notes' => $transaction->notes
|
||||||
? $this->normalizeWhitespace(mb_strtolower($transaction->notes))
|
? $this->normalizeWhitespace(mb_strtolower($transaction->notes))
|
||||||
: null,
|
: null,
|
||||||
|
|
@ -141,6 +163,35 @@ class AutomationRuleService
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* @return array<int, string>
|
||||||
|
*/
|
||||||
|
private function ruleVariables(AutomationRule $rule): array
|
||||||
|
{
|
||||||
|
$variables = [];
|
||||||
|
$this->collectRuleVariables($this->normalizeRuleJson($rule->rules_json), $variables);
|
||||||
|
|
||||||
|
return array_values(array_unique($variables));
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* @param array<int, string> $variables
|
||||||
|
*/
|
||||||
|
private function collectRuleVariables(mixed $ruleJson, array &$variables): void
|
||||||
|
{
|
||||||
|
if (! is_array($ruleJson)) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (isset($ruleJson['var']) && is_string($ruleJson['var'])) {
|
||||||
|
$variables[] = $ruleJson['var'];
|
||||||
|
}
|
||||||
|
|
||||||
|
foreach ($ruleJson as $value) {
|
||||||
|
$this->collectRuleVariables($value, $variables);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
private function applyActions(Transaction $transaction, AutomationRule $rule): bool
|
private function applyActions(Transaction $transaction, AutomationRule $rule): bool
|
||||||
{
|
{
|
||||||
$changed = false;
|
$changed = false;
|
||||||
|
|
|
||||||
|
|
@ -11,6 +11,7 @@ use App\Models\Label;
|
||||||
use App\Models\Transaction;
|
use App\Models\Transaction;
|
||||||
use App\Models\User;
|
use App\Models\User;
|
||||||
use Illuminate\Support\Facades\Cache;
|
use Illuminate\Support\Facades\Cache;
|
||||||
|
use Illuminate\Support\Facades\DB;
|
||||||
use Illuminate\Support\Facades\Event;
|
use Illuminate\Support\Facades\Event;
|
||||||
use Illuminate\Support\Facades\Queue;
|
use Illuminate\Support\Facades\Queue;
|
||||||
|
|
||||||
|
|
@ -86,6 +87,36 @@ test('matches endpoint skips already categorized when only_uncategorized is true
|
||||||
$allResponse->assertOk()->assertJsonPath('total', 2);
|
$allResponse->assertOk()->assertJsonPath('total', 2);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('matches endpoint avoids repeated relationship queries for description-only rules', function () {
|
||||||
|
Transaction::factory()->enableBanking()->count(501)->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;
|
||||||
|
});
|
||||||
|
|
||||||
|
$this->actingAs($this->user)
|
||||||
|
->getJson(route('automation-rules.matches', $this->rule))
|
||||||
|
->assertOk()
|
||||||
|
->assertJsonPath('total', 501);
|
||||||
|
|
||||||
|
$accountEagerLoadQueries = collect($queries)
|
||||||
|
->filter(fn (string $query): bool => (str_contains($query, 'from "accounts"') || str_contains($query, 'from `accounts`'))
|
||||||
|
&& (str_contains($query, '"accounts"."id" in') || str_contains($query, '`accounts`.`id` in')));
|
||||||
|
$bankEagerLoadQueries = collect($queries)
|
||||||
|
->filter(fn (string $query): bool => (str_contains($query, 'from "banks"') || str_contains($query, 'from `banks`'))
|
||||||
|
&& (str_contains($query, '"banks"."id" in') || str_contains($query, '`banks`.`id` in')));
|
||||||
|
|
||||||
|
expect($accountEagerLoadQueries)->toHaveCount(1)
|
||||||
|
->and($bankEagerLoadQueries)->toHaveCount(1);
|
||||||
|
});
|
||||||
|
|
||||||
test('matches endpoint deduplicates cached matching transaction ids', function () {
|
test('matches endpoint deduplicates cached matching transaction ids', function () {
|
||||||
$transaction = Transaction::factory()->enableBanking()->create([
|
$transaction = Transaction::factory()->enableBanking()->create([
|
||||||
'user_id' => $this->user->id,
|
'user_id' => $this->user->id,
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue