RomanNum3ral
/
whisper-money
mirror of https://github.com/whisper-money/whisper-money.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix(banking): dedup EnableBanking transactions by deterministic fingerprint (#390) 

## 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.
This commit is contained in:
Víctor Falcón 2026-05-13 11:30:11 +01:00 committed by GitHub
parent d140b4fd4c
commit d9204bb3d6
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 356 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
app/Models/Transaction.php
View File

@ -47,6 +47,7 @@ class Transaction extends Model
'notes_iv',
'source',
'external_transaction_id',
'dedup_fingerprint',
'raw_data',
];

65
app/Services/Banking/TransactionFingerprint.php Normal file
View File

@ -0,0 +1,65 @@
<?php
namespace App\Services\Banking;
/**
* Builds a deterministic fingerprint for an EnableBanking transaction
* payload so we can dedup even when the upstream bank omits a stable
* id (transaction_id / entry_reference).
*
* Shared between the live sync path and the cleanup command so they
* stay in lock-step.
*/
class TransactionFingerprint
{
/**
* @param array<string, mixed> $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<int, string>|string $remittance
*/
private static function remittance(array|string $remittance): string
{
if (is_string($remittance)) {
return $remittance;
}
return implode('|', $remittance);
}
/**
* @param array<int, mixed> $parts
*/
private static function hash(array $parts): string
{
return 'fp_'.hash('sha256', implode("\x1f", array_map('strval', $parts)));
}
}

70
app/Services/Banking/TransactionSyncService.php
View File

@ -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;
}

24
database/migrations/2026_05_13_085027_add_dedup_fingerprint_to_transactions_table.php Normal file
View File

@ -0,0 +1,24 @@
<?php
use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;
return new class extends Migration
{
public function up(): void
{
Schema::table('transactions', function (Blueprint $table) {
$table->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');
});
}
};

113
tests/Feature/OpenBanking/TransactionSyncServiceTest.php
View File

@ -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();
});

105
tests/Unit/Services/Banking/TransactionFingerprintTest.php Normal file
View File

@ -0,0 +1,105 @@
<?php
use App\Services\Banking\TransactionFingerprint;
test('external transaction id is the canonical fingerprint when present', function () {
$payload = baseEnableBankingPayload([
'transaction_id' => '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<string, mixed> $overrides
* @return array<string, mixed>
*/
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);
}