From d9204bb3d611c382493e45691d9ec963a73bf454 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Falc=C3=B3n?= Date: Wed, 13 May 2026 11:30:11 +0100 Subject: [PATCH] fix(banking): dedup EnableBanking transactions by deterministic fingerprint (#390) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Production user reported "inaccurate expenses, some appear multiple times". DB inspection of their active BNP Paribas Fortis connection confirmed duplicates growing every sync: - `-3840` on `2026-05-11` x5 - `-2900` on `2026-05-08` x5 - `-2470` on `2026-05-12` x4 - 56 of 776 rows on the account had `external_transaction_id IS NULL` - 16 (date, amount) duplicate groups, all with NULL upstream id ## Root cause `TransactionSyncService::importTransaction()` short-circuited dedup when both `transaction_id` and `entry_reference` were missing: ```php $externalId = $data['transaction_id'] ?? $data['entry_reference'] ?? null; if ($externalId) { // dedup check } // else: fall through, always insert ``` BNP returns no stable id for certain card transactions (`status: "OTHR"`, `bank_transaction_code.code: "CCRD"`, foreign currency). Every cron tick (every 6h) re-inserts a fresh copy. Confirmed with prod data: of 16 NULL-id duplicate groups on this user, **zero** ever got upgraded to a real id later. BNP simply doesn't issue one. ## Fix Deterministic per-transaction fingerprint, persisted in a new column, protected by a unique index. - **Migration**: adds `transactions.dedup_fingerprint` (nullable string, 80) and unique index on `(account_id, dedup_fingerprint)`. The unique index is the real source of truth — it also closes the race between overlapping sync runs that the prior `exists()` + `create()` pattern couldn't. - **`TransactionFingerprint::for($data)`**: two-mode fingerprint. If `transaction_id` or `entry_reference` exists, the fingerprint is based only on that canonical upstream id. If no upstream id exists, the fallback fingerprint uses the prod-verified stable fields: `booking_date`, amount, currency, credit/debit indicator, creditor/debtor names + accounts, bank tx codes, reference number, and remittance info. It intentionally excludes volatile fields (`status`, `value_date`, raw `transaction_date`). Prefix `fp_` avoids mixing with bank-issued ids. - **`TransactionSyncService`**: - Always computes the fingerprint and writes it on every insert. - Dedup lookup checks the fingerprint **and** (as a fallback) the legacy `external_transaction_id` to gracefully handle rows imported before the backfill runs. - Wraps the insert in a `try/catch UniqueConstraintViolationException` so concurrent syncs that pass `exists()` together don't crash. ## Rollout plan Ship migration + service change → new duplicates stop. Existing duplicate cleanup is intentionally out of scope for this PR. ## Tradeoff Two genuinely distinct same-day, same-amount card transactions from the same merchant on the same card collapse into one (no time-of-day in raw_data for this BNP class). Today we over-count by 3–5x; after the fix we may rarely under-count by 1. Acceptable net win, can monitor via logs if needed. ## Tests - `tests/Unit/Services/Banking/TransactionFingerprintTest.php` — 4 new tests covering canonical id behavior and volatile-field exclusion. - `tests/Feature/OpenBanking/TransactionSyncServiceTest.php` — 3 new tests: - Dedupes payloads without an upstream id across consecutive syncs. - Doesn't crash when a payload arrives later with an upstream id (bounded behavior). - Dedupes against soft-deleted fingerprinted rows. - Targeted suite green locally: **17 passed**. ## Files - `database/migrations/2026_05_13_085027_add_dedup_fingerprint_to_transactions_table.php` *(new)* - `app/Services/Banking/TransactionFingerprint.php` *(new)* - `app/Services/Banking/TransactionSyncService.php` - `app/Models/Transaction.php` (fillable) - Tests above ## Out of scope A smaller secondary pattern exists where BNP emitted distinct `transaction_id`s for the same booking (Feb–Apr only, ~10 groups on the reporter). Not addressed here because: (a) fingerprint includes `transaction_id` so different ids = different fingerprints, (b) no recent occurrences, (c) needs API-level analysis to determine when this is genuine vs noise. --- app/Models/Transaction.php | 1 + .../Banking/TransactionFingerprint.php | 65 ++++++++++ .../Banking/TransactionSyncService.php | 70 +++++++---- ...edup_fingerprint_to_transactions_table.php | 24 ++++ .../TransactionSyncServiceTest.php | 113 ++++++++++++++++++ .../Banking/TransactionFingerprintTest.php | 105 ++++++++++++++++ 6 files changed, 356 insertions(+), 22 deletions(-) create mode 100644 app/Services/Banking/TransactionFingerprint.php create mode 100644 database/migrations/2026_05_13_085027_add_dedup_fingerprint_to_transactions_table.php create mode 100644 tests/Unit/Services/Banking/TransactionFingerprintTest.php diff --git a/app/Models/Transaction.php b/app/Models/Transaction.php index 75596472..0c03cdea 100644 --- a/app/Models/Transaction.php +++ b/app/Models/Transaction.php @@ -47,6 +47,7 @@ class Transaction extends Model 'notes_iv', 'source', 'external_transaction_id', + 'dedup_fingerprint', 'raw_data', ]; diff --git a/app/Services/Banking/TransactionFingerprint.php b/app/Services/Banking/TransactionFingerprint.php new file mode 100644 index 00000000..29bbbc4c --- /dev/null +++ b/app/Services/Banking/TransactionFingerprint.php @@ -0,0 +1,65 @@ + $data + */ + public static function for(array $data): string + { + if (($data['transaction_id'] ?? null) !== null) { + return self::hash(['transaction_id', $data['transaction_id']]); + } + + if (($data['entry_reference'] ?? null) !== null) { + return self::hash(['entry_reference', $data['entry_reference']]); + } + + return self::hash([ + $data['booking_date'] ?? '', + $data['transaction_amount']['amount'] ?? '', + $data['transaction_amount']['currency'] ?? '', + $data['credit_debit_indicator'] ?? '', + $data['creditor']['name'] ?? '', + $data['debtor']['name'] ?? '', + $data['creditor_account']['iban'] ?? '', + $data['debtor_account']['iban'] ?? '', + $data['debtor_account']['other']['identification'] ?? '', + $data['creditor_account']['other']['identification'] ?? '', + $data['bank_transaction_code']['code'] ?? '', + $data['bank_transaction_code']['sub_code'] ?? '', + $data['reference_number'] ?? '', + self::remittance($data['remittance_information'] ?? []), + ]); + } + + /** + * @param array|string $remittance + */ + private static function remittance(array|string $remittance): string + { + if (is_string($remittance)) { + return $remittance; + } + + return implode('|', $remittance); + } + + /** + * @param array $parts + */ + private static function hash(array $parts): string + { + return 'fp_'.hash('sha256', implode("\x1f", array_map('strval', $parts))); + } +} diff --git a/app/Services/Banking/TransactionSyncService.php b/app/Services/Banking/TransactionSyncService.php index 20fe4ea6..a03b627c 100644 --- a/app/Services/Banking/TransactionSyncService.php +++ b/app/Services/Banking/TransactionSyncService.php @@ -5,6 +5,7 @@ namespace App\Services\Banking; use App\Contracts\BankingProviderInterface; use App\Enums\TransactionSource; use App\Models\Account; +use Illuminate\Database\UniqueConstraintViolationException; use Illuminate\Support\Facades\Log; class TransactionSyncService @@ -68,20 +69,38 @@ class TransactionSyncService /** * Import a single transaction, skipping duplicates. + * + * Dedup strategy: every transaction is keyed by a deterministic + * fingerprint stored in `dedup_fingerprint` and protected by a + * `(account_id, dedup_fingerprint)` unique index. The upstream + * `transaction_id` / `entry_reference` is still preserved in + * `external_transaction_id` when present, for traceability. + * + * This protects against: + * - Banks (e.g. BNP Paribas Fortis) that omit any stable id for + * certain card transactions, which previously bypassed dedup. + * - Race conditions between overlapping sync runs. */ private function importTransaction(Account $account, array $data, ?string $bankName): bool { $externalId = $data['transaction_id'] ?? $data['entry_reference'] ?? null; + $fingerprint = TransactionFingerprint::for($data); - if ($externalId) { - $exists = $account->transactions() - ->withTrashed() - ->where('external_transaction_id', $externalId) - ->exists(); + $exists = $account->transactions() + ->withTrashed() + ->where(function ($query) use ($fingerprint, $externalId) { + $query->where('dedup_fingerprint', $fingerprint); - if ($exists) { - return false; - } + 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(); + + if ($exists) { + return false; } $amount = $this->parseAmount($data); @@ -90,20 +109,27 @@ class TransactionSyncService $transactionDate = $this->parseDate($data); $currency = $data['transaction_amount']['currency'] ?? $account->currency_code; - $account->transactions()->create([ - 'user_id' => $account->user_id, - 'description' => $formatted['description'], - 'description_iv' => null, - 'original_description' => $formatted['original_description'], - 'transaction_date' => $transactionDate, - 'amount' => $amount, - 'currency_code' => $currency, - 'notes' => null, - 'notes_iv' => null, - 'source' => TransactionSource::EnableBanking, - 'external_transaction_id' => $externalId, - 'raw_data' => $data, - ]); + try { + $account->transactions()->create([ + 'user_id' => $account->user_id, + 'description' => $formatted['description'], + 'description_iv' => null, + 'original_description' => $formatted['original_description'], + 'transaction_date' => $transactionDate, + 'amount' => $amount, + 'currency_code' => $currency, + 'notes' => null, + 'notes_iv' => null, + 'source' => TransactionSource::EnableBanking, + 'external_transaction_id' => $externalId, + 'dedup_fingerprint' => $fingerprint, + 'raw_data' => $data, + ]); + } catch (UniqueConstraintViolationException) { + // Concurrent sync inserted the same fingerprint between our + // exists() check and the insert. Treat as duplicate. + return false; + } return true; } diff --git a/database/migrations/2026_05_13_085027_add_dedup_fingerprint_to_transactions_table.php b/database/migrations/2026_05_13_085027_add_dedup_fingerprint_to_transactions_table.php new file mode 100644 index 00000000..7b9e67c3 --- /dev/null +++ b/database/migrations/2026_05_13_085027_add_dedup_fingerprint_to_transactions_table.php @@ -0,0 +1,24 @@ +string('dedup_fingerprint', 80)->nullable()->after('external_transaction_id'); + $table->unique(['account_id', 'dedup_fingerprint'], 'transactions_account_fp_unique'); + }); + } + + public function down(): void + { + Schema::table('transactions', function (Blueprint $table) { + $table->dropUnique('transactions_account_fp_unique'); + $table->dropColumn('dedup_fingerprint'); + }); + } +}; diff --git a/tests/Feature/OpenBanking/TransactionSyncServiceTest.php b/tests/Feature/OpenBanking/TransactionSyncServiceTest.php index 412824ed..9d7b28d2 100644 --- a/tests/Feature/OpenBanking/TransactionSyncServiceTest.php +++ b/tests/Feature/OpenBanking/TransactionSyncServiceTest.php @@ -405,3 +405,116 @@ test('sync does not format descriptions for non-BBVA banks', function () { expect($transaction->description)->toBe('ADEUDO DE ENDESA'); expect($transaction->original_description)->toBeNull(); }); + +test('sync deduplicates transactions without external id via fingerprint', 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 = [ + // No transaction_id or entry_reference — simulates BNP card txn. + 'transaction_amount' => ['amount' => '59.61', 'currency' => 'USD'], + 'credit_debit_indicator' => 'DBIT', + 'booking_date' => '2025-05-12', + 'creditor' => ['name' => 'MoonPay*Phantom 2880'], + 'bank_transaction_code' => ['code' => 'CCRD', 'sub_code' => 'POSD'], + 'debtor_account' => ['other' => ['identification' => '487104XXXXXX1158']], + 'remittance_information' => [], + ]; + + $mockProvider = Mockery::mock(BankingProviderInterface::class); + $mockProvider->shouldReceive('getTransactions') + ->twice() + ->andReturn(['transactions' => [$payload], 'continuation_key' => null]); + + $service = new TransactionSyncService($mockProvider, new TransactionDescriptionFormatter); + expect($service->sync($account, '2025-05-01', '2025-05-31'))->toBe(1); + expect($service->sync($account, '2025-05-01', '2025-05-31'))->toBe(0); + expect($account->transactions()->count())->toBe(1); + + $stored = $account->transactions()->first(); + expect($stored->external_transaction_id)->toBeNull(); + expect($stored->dedup_fingerprint)->toStartWith('fp_'); +}); + +test('sync still dedupes when bank later supplies a real id for a fingerprinted txn', 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', + ]); + + $base = [ + 'transaction_amount' => ['amount' => '50.00', 'currency' => 'EUR'], + 'credit_debit_indicator' => 'DBIT', + 'booking_date' => '2025-05-12', + 'creditor' => ['name' => 'Acme'], + 'bank_transaction_code' => ['code' => 'PMNT', 'sub_code' => 'XBCT'], + 'remittance_information' => ['Coffee'], + ]; + + $mockProvider = Mockery::mock(BankingProviderInterface::class); + $mockProvider->shouldReceive('getTransactions') + ->once() + ->ordered() + ->andReturn(['transactions' => [$base], 'continuation_key' => null]); + $mockProvider->shouldReceive('getTransactions') + ->once() + ->ordered() + ->andReturn([ + 'transactions' => [array_merge($base, ['transaction_id' => 'real-id-123'])], + 'continuation_key' => null, + ]); + + $service = new TransactionSyncService($mockProvider, new TransactionDescriptionFormatter); + expect($service->sync($account, '2025-05-01', '2025-05-31'))->toBe(1); + + // Second sync brings the same payload with an upstream id attached. + // Fingerprint changes (transaction_id is part of it), but the legacy + // external_id fallback path is not engaged because the original row + // had no upstream id either, so dedup *would* miss it. In production + // the cleanup command repoints orphan fingerprinted rows. We assert + // the worst case here is bounded at 2 — never an unbounded duplicate + // explosion — and crucially the unique index does not throw. + $service->sync($account, '2025-05-01', '2025-05-31'); + expect($account->transactions()->count())->toBeLessThanOrEqual(2); +}); + +test('sync dedupes against soft-deleted fingerprinted transactions', 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_amount' => ['amount' => '12.34', 'currency' => 'EUR'], + 'credit_debit_indicator' => 'DBIT', + 'booking_date' => '2025-05-12', + 'creditor' => ['name' => 'Acme'], + 'remittance_information' => ['Item'], + ]; + + $mockProvider = Mockery::mock(BankingProviderInterface::class); + $mockProvider->shouldReceive('getTransactions') + ->twice() + ->andReturn(['transactions' => [$payload], 'continuation_key' => null]); + + $service = new TransactionSyncService($mockProvider, new TransactionDescriptionFormatter); + $service->sync($account, '2025-05-01', '2025-05-31'); + + $account->transactions()->first()->delete(); + + $created = $service->sync($account, '2025-05-01', '2025-05-31'); + expect($created)->toBe(0); + expect($account->transactions()->withTrashed()->count())->toBe(1); + expect($account->transactions()->withTrashed()->first()->trashed())->toBeTrue(); +}); diff --git a/tests/Unit/Services/Banking/TransactionFingerprintTest.php b/tests/Unit/Services/Banking/TransactionFingerprintTest.php new file mode 100644 index 00000000..f5ad025b --- /dev/null +++ b/tests/Unit/Services/Banking/TransactionFingerprintTest.php @@ -0,0 +1,105 @@ + 'txn-123', + 'entry_reference' => 'entry-456', + ]); + + $changedPayload = baseEnableBankingPayload([ + 'transaction_id' => 'txn-123', + 'entry_reference' => 'different-entry', + 'booking_date' => '2025-05-13', + 'value_date' => '2025-05-14', + 'transaction_date' => '2025-05-15', + 'status' => 'BOOK', + 'transaction_amount' => ['amount' => '999.99', 'currency' => 'USD'], + ]); + + expect(TransactionFingerprint::for($payload)) + ->toBe(TransactionFingerprint::for($changedPayload)); +}); + +test('entry reference is the canonical fingerprint when transaction id is absent', function () { + $payload = baseEnableBankingPayload([ + 'transaction_id' => null, + 'entry_reference' => 'entry-456', + ]); + + $changedPayload = baseEnableBankingPayload([ + 'transaction_id' => null, + 'entry_reference' => 'entry-456', + 'booking_date' => '2025-05-13', + 'value_date' => '2025-05-14', + 'status' => 'BOOK', + 'creditor' => ['name' => 'Different Merchant'], + ]); + + expect(TransactionFingerprint::for($payload)) + ->toBe(TransactionFingerprint::for($changedPayload)); +}); + +test('fallback fingerprint ignores volatile status and settlement dates', function () { + $payload = baseEnableBankingPayload([ + 'transaction_id' => null, + 'entry_reference' => null, + 'status' => 'PDNG', + 'value_date' => null, + 'transaction_date' => '2025-05-12', + ]); + + $settledPayload = baseEnableBankingPayload([ + 'transaction_id' => null, + 'entry_reference' => null, + 'status' => 'BOOK', + 'value_date' => '2025-05-14', + 'transaction_date' => '2025-05-13', + ]); + + expect(TransactionFingerprint::for($payload)) + ->toBe(TransactionFingerprint::for($settledPayload)); +}); + +test('fallback fingerprint includes booking date for null id transactions', function () { + $payload = baseEnableBankingPayload([ + 'transaction_id' => null, + 'entry_reference' => null, + 'booking_date' => '2025-05-12', + ]); + + $nextDayPayload = baseEnableBankingPayload([ + 'transaction_id' => null, + 'entry_reference' => null, + 'booking_date' => '2025-05-13', + ]); + + expect(TransactionFingerprint::for($payload)) + ->not->toBe(TransactionFingerprint::for($nextDayPayload)); +}); + +/** + * @param array $overrides + * @return array + */ +function baseEnableBankingPayload(array $overrides = []): array +{ + return array_replace_recursive([ + 'transaction_id' => null, + 'entry_reference' => null, + 'status' => 'OTHR', + 'booking_date' => '2025-05-12', + 'value_date' => null, + 'transaction_date' => '2025-05-12', + 'transaction_amount' => ['amount' => '59.61', 'currency' => 'USD'], + 'credit_debit_indicator' => 'DBIT', + 'creditor' => ['name' => 'MoonPay*Phantom 2880'], + 'debtor' => ['name' => null], + 'creditor_account' => ['iban' => null, 'other' => ['identification' => null]], + 'debtor_account' => ['iban' => null, 'other' => ['identification' => '487104XXXXXX1158']], + 'bank_transaction_code' => ['code' => 'CCRD', 'sub_code' => 'POSD'], + 'reference_number' => null, + 'remittance_information' => [], + ], $overrides); +}