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]);