From 84bad76316425616899f03157fa8246144635288 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Falc=C3=B3n?= Date: Thu, 2 Jul 2026 15:34:29 +0200 Subject: [PATCH] perf(banking): kill per-transaction dedup N+1 in bank sync (PHP-LARAVEL-3Y) (#621) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What Fixes the N+1 flagged by Sentry **PHP-LARAVEL-3Y** in `App\Jobs\SyncBankingConnectionJob`. `TransactionSyncService::importTransaction()` ran one `exists()` probe **per incoming bank transaction** to detect duplicates: ```sql select exists( select * from transactions where account_id = ? and (dedup_fingerprint = ? or external_transaction_id = ?) ) as exists ``` A sync importing N transactions issued N dedup `SELECT`s. As transaction volume per sync grows, so does the query count. ## How Preload the account's existing dedup keys **once** at the start of `sync()` and match against in-memory sets: - `loadExistingDedupKeys()` streams (`cursor()`) the two dedup columns for the account — including soft-deleted rows (`withTrashed()`) — into two keyed sets. One query instead of N. - `importTransaction()` checks membership in memory (`isset()`), an exact mirror of the old `fingerprint OR external_id` predicate. - Keys from successful inserts are folded back into the sets, so duplicates **within the same sync** (e.g. the same transaction repeated across pages) are still caught in memory. - The `(account_id, dedup_fingerprint)` unique index + `UniqueConstraintViolationException` catch still backstop concurrent syncs (`SyncBankingConnectionJob` is already `ShouldBeUnique` per connection). ## Commits 1. `perf(banking): preload dedup keys to kill per-transaction N+1` — the core fix + a query-count regression test. 2. `perf(banking): stream dedup key preload with a cursor` — avoids buffering every historical row into a Collection on top of the sets (memory guard for very large accounts). 3. `fix(banking): match external ids case-insensitively in dedup` — the old query compared `external_transaction_id` under the production `utf8mb4_unicode_ci` collation; the in-memory `isset()` is byte-exact. With no unique index on `external_transaction_id`, a legacy id stored as `ABC` vs an incoming `abc` would have silently double-imported. Normalized with `mb_strtolower()` on both sides + a test. 4. `test(banking): cover intra-run cross-page dedup` — new coverage for the same transaction appearing on two pages of one sync. ## Testing - 303/303 in `tests/Feature/OpenBanking/` green; full backend suite green except one **pre-existing, unrelated** failure on `main` (`DashboardTest` returns 409 locally — an Inertia asset-version artifact that resolves once `bun run build` runs in CI). - New tests: dedup runs as a single preload SELECT regardless of batch size; case-insensitive external-id dedup; cross-page intra-run dedup. - `pint` clean; `larastan` clean on the changed file. ## Reviewed by two independent agents (architecture + product/correctness) Both rated the change behavior-preserving and shippable. Follow-ups they surfaced, intentionally **not** bundled here to keep this fix focused and low-risk: - **`WiseTransactionSyncService` has the identical latent N+1** (`:149-158`). It is not currently firing in Sentry (lower volume). Worth a separate PR — possibly extracting a shared dedup-set helper once both call sites need it. - **Residual collation divergence**: accent/width folding from `utf8mb4_unicode_ci` is not replicated in PHP. Irrelevant for real bank reference ids (ASCII), accepted. - The test comment referencing a "cleanup command" that repoints orphan fingerprinted rows describes a command that does not exist in the repo — pre-existing, worth correcting separately. Fixes PHP-LARAVEL-3Y --- .../Banking/TransactionSyncService.php | 83 ++++++++++-- .../TransactionSyncServiceTest.php | 121 ++++++++++++++++++ 2 files changed, 190 insertions(+), 14 deletions(-) diff --git a/app/Services/Banking/TransactionSyncService.php b/app/Services/Banking/TransactionSyncService.php index 95c6a005..c6292ea1 100644 --- a/app/Services/Banking/TransactionSyncService.php +++ b/app/Services/Banking/TransactionSyncService.php @@ -31,6 +31,15 @@ class TransactionSyncService $dailyBalances = []; $bankName = $account->bank?->name; + // Preload the account's existing dedup keys once. Without this every + // incoming transaction ran its own exists() probe (the N+1 in + // PHP-LARAVEL-3Y). Keys inserted during this run are folded back into + // the sets so duplicates within the same sync are still caught in + // memory, and the unique index still backstops concurrent syncs. + // ponytail: loads every key for the account; if one account's history + // ever dwarfs its sync window, narrow this to the incoming batch's keys. + [$knownFingerprints, $knownExternalIds] = $this->loadExistingDedupKeys($account); + do { $result = $this->provider->getTransactions( $account->external_account_id, @@ -41,7 +50,7 @@ class TransactionSyncService ); foreach ($result['transactions'] as $transaction) { - if ($this->importTransaction($account, $transaction, $bankName)) { + if ($this->importTransaction($account, $transaction, $bankName, $knownFingerprints, $knownExternalIds)) { $created++; } @@ -81,23 +90,16 @@ class TransactionSyncService * certain card transactions, which previously bypassed dedup. * - Race conditions between overlapping sync runs. */ - private function importTransaction(Account $account, array $data, ?string $bankName): bool + private function importTransaction(Account $account, array $data, ?string $bankName, array &$knownFingerprints, array &$knownExternalIds): bool { $externalId = $data['transaction_id'] ?? $data['entry_reference'] ?? null; $fingerprint = TransactionFingerprint::for($data); - $exists = $account->transactions() - ->withTrashed() - ->where(function ($query) use ($fingerprint, $externalId) { - $query->where('dedup_fingerprint', $fingerprint); - - if ($externalId !== null) { - // Also match legacy rows imported before the fingerprint - // column existed, which are keyed solely on the upstream id. - $query->orWhere('external_transaction_id', $externalId); - } - }) - ->exists(); + // Mirror of the previous exists() probe against the preloaded sets: + // match on the fingerprint, or — for legacy rows keyed solely on the + // upstream id before the fingerprint column existed — the external id. + $exists = isset($knownFingerprints[$fingerprint]) + || ($externalId !== null && isset($knownExternalIds[$this->dedupExternalIdKey($externalId)])); if ($exists) { return false; @@ -133,9 +135,62 @@ class TransactionSyncService return false; } + $knownFingerprints[$fingerprint] = true; + + if ($externalId !== null) { + $knownExternalIds[$this->dedupExternalIdKey($externalId)] = true; + } + return true; } + /** + * Normalize an external transaction id for dedup lookups so matching stays + * case-insensitive, mirroring the production `utf8mb4_unicode_ci` collation + * the old `where external_transaction_id = ?` probe relied on. Without this + * a legacy id stored as `ABC` would no longer dedup an incoming `abc`, and + * since there is no unique index on `external_transaction_id` that would + * silently double-import the transaction. (Accent/width folding is not + * replicated; bank reference ids are ASCII in practice.) + */ + private function dedupExternalIdKey(string $externalId): string + { + return mb_strtolower($externalId); + } + + /** + * Preload the account's existing dedup keys, including soft-deleted rows, + * so duplicate detection runs against in-memory sets instead of one + * exists() query per incoming transaction. + * + * @return array{0: array, 1: array} fingerprints keyed set, external ids keyed set + */ + private function loadExistingDedupKeys(Account $account): array + { + $knownFingerprints = []; + $knownExternalIds = []; + + // cursor() streams rows so peak memory is the two sets, not an extra + // buffered Collection of every historical row on top of them. + $rows = $account->transactions() + ->withTrashed() + ->toBase() + ->select(['dedup_fingerprint', 'external_transaction_id']) + ->cursor(); + + foreach ($rows as $row) { + if ($row->dedup_fingerprint !== null) { + $knownFingerprints[$row->dedup_fingerprint] = true; + } + + if ($row->external_transaction_id !== null) { + $knownExternalIds[$this->dedupExternalIdKey($row->external_transaction_id)] = true; + } + } + + return [$knownFingerprints, $knownExternalIds]; + } + /** * Parse amount from EnableBanking transaction data. * Returns amount in cents (bigint). Debits are negative. diff --git a/tests/Feature/OpenBanking/TransactionSyncServiceTest.php b/tests/Feature/OpenBanking/TransactionSyncServiceTest.php index 48d138ad..721272f7 100644 --- a/tests/Feature/OpenBanking/TransactionSyncServiceTest.php +++ b/tests/Feature/OpenBanking/TransactionSyncServiceTest.php @@ -9,6 +9,7 @@ use App\Models\Transaction; use App\Models\User; use App\Services\Banking\TransactionDescriptionFormatter; use App\Services\Banking\TransactionSyncService; +use Illuminate\Support\Facades\DB; test('sync creates transactions from provider data', function () { $user = User::factory()->onboarded()->create(); @@ -521,6 +522,126 @@ test('sync still dedupes when bank later supplies a real id for a fingerprinted expect($account->transactions()->count())->toBeLessThanOrEqual(2); }); +test('sync dedupes external ids case-insensitively like the production collation', function () { + $user = User::factory()->onboarded()->create(); + $connection = BankingConnection::factory()->create(['user_id' => $user->id]); + $account = Account::factory()->connected()->create([ + 'user_id' => $user->id, + 'banking_connection_id' => $connection->id, + 'external_account_id' => 'ext-123', + ]); + + // Legacy row keyed only on the upstream id (no fingerprint), stored uppercase. + Transaction::factory()->enableBanking()->create([ + 'user_id' => $user->id, + 'account_id' => $account->id, + 'external_transaction_id' => 'ABC-001', + ]); + + $mockProvider = Mockery::mock(BankingProviderInterface::class); + $mockProvider->shouldReceive('getTransactions') + ->once() + ->andReturn([ + 'transactions' => [ + [ + // Same id, different case — utf8mb4_unicode_ci treats these as equal. + 'transaction_id' => 'abc-001', + 'transaction_amount' => ['amount' => '50.00', 'currency' => 'EUR'], + 'credit_debit_indicator' => 'DBIT', + 'booking_date' => '2025-01-15', + 'remittance_information' => ['Duplicate with different case'], + ], + ], + 'continuation_key' => null, + ]); + + $service = new TransactionSyncService($mockProvider, new TransactionDescriptionFormatter); + $created = $service->sync($account, '2025-01-01', '2025-01-31'); + + expect($created)->toBe(0); + expect($account->transactions()->count())->toBe(1); +}); + +test('sync dedupes a transaction that repeats across pages in one run', function () { + $user = User::factory()->onboarded()->create(); + $connection = BankingConnection::factory()->create(['user_id' => $user->id]); + $account = Account::factory()->connected()->create([ + 'user_id' => $user->id, + 'banking_connection_id' => $connection->id, + 'external_account_id' => 'ext-123', + ]); + + $payload = [ + 'transaction_id' => 'txn-dup', + 'transaction_amount' => ['amount' => '50.00', 'currency' => 'EUR'], + 'credit_debit_indicator' => 'DBIT', + 'booking_date' => '2025-01-15', + 'remittance_information' => ['Repeated across pages'], + ]; + + $mockProvider = Mockery::mock(BankingProviderInterface::class); + $mockProvider->shouldReceive('getTransactions') + ->once() + ->ordered() + ->andReturn(['transactions' => [$payload], 'continuation_key' => 'page2']); + $mockProvider->shouldReceive('getTransactions') + ->once() + ->ordered() + ->andReturn(['transactions' => [$payload], 'continuation_key' => null]); + + $service = new TransactionSyncService($mockProvider, new TransactionDescriptionFormatter); + $created = $service->sync($account, '2025-01-01', '2025-01-31'); + + // The second occurrence is folded into the in-memory set from the first + // insert, so it is deduped without relying on the unique-index backstop. + expect($created)->toBe(1); + expect($account->transactions()->count())->toBe(1); +}); + +test('sync checks for duplicates once per run regardless of batch size', function () { + $user = User::factory()->onboarded()->create(); + $connection = BankingConnection::factory()->create(['user_id' => $user->id]); + $account = Account::factory()->connected()->create([ + 'user_id' => $user->id, + 'banking_connection_id' => $connection->id, + 'external_account_id' => 'ext-123', + ]); + + $transactions = collect(range(1, 6))->map(fn (int $i) => [ + 'transaction_id' => "txn-{$i}", + 'transaction_amount' => ['amount' => '10.00', 'currency' => 'EUR'], + 'credit_debit_indicator' => 'DBIT', + 'booking_date' => '2025-01-15', + 'remittance_information' => ["Purchase {$i}"], + ])->all(); + + $mockProvider = Mockery::mock(BankingProviderInterface::class); + $mockProvider->shouldReceive('getTransactions') + ->once() + ->andReturn(['transactions' => $transactions, 'continuation_key' => null]); + + $service = new TransactionSyncService($mockProvider, new TransactionDescriptionFormatter); + + DB::enableQueryLog(); + $created = $service->sync($account, '2025-01-01', '2025-01-31'); + $queries = collect(DB::getQueryLog()); + DB::disableQueryLog(); + + expect($created)->toBe(6); + + // The dedup lookup is a single preload SELECT, not one exists() per row. + $dedupSelects = $queries->filter(fn (array $q): bool => str_contains($q['query'], 'dedup_fingerprint') + && str_starts_with(strtolower(ltrim($q['query'])), 'select') + ); + expect($dedupSelects)->toHaveCount(1); + + // The old per-row `select exists(... dedup_fingerprint ...)` probe is gone. + $dedupExistsProbes = $queries->filter(fn (array $q): bool => str_contains(strtolower($q['query']), 'exists(') + && str_contains($q['query'], 'dedup_fingerprint') + ); + expect($dedupExistsProbes)->toHaveCount(0); +}); + test('sync dedupes against soft-deleted fingerprinted transactions', function () { $user = User::factory()->onboarded()->create(); $connection = BankingConnection::factory()->create(['user_id' => $user->id]);