From cd918523e84e38d56dfcfe0a80f906a764c9e668 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Falc=C3=B3n?= Date: Sun, 12 Jul 2026 18:25:13 +0200 Subject: [PATCH] fix(banking): stop duplicating EnableBanking transactions with positional entry_reference (#669) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem A user reported the same bank transactions appearing twice. Investigation in prod showed this is a systematic dedup bug in the EnableBanking sync affecting a few hundred rows across several users/accounts. ## Root cause The affected banks don't return a real `transaction_id`; their only id is a **positional** `entry_reference` of the form `{booking_date}.{index}` (e.g. `YYYY-MM-DD.0`). That field is **null the day a transaction first appears** and only **populated on a later sync**. `TransactionFingerprint::for()` preferred `entry_reference` when present and fell back to a content hash when absent. So the same transaction produced two different fingerprints: | | First sync (same day) | Later sync | |---|---|---| | `entry_reference` | `null` | `YYYY-MM-DD.0` | | `dedup_fingerprint` | content-based | id-based | | `external_transaction_id` | `null` | `YYYY-MM-DD.0` | Neither the fingerprint nor the `external_transaction_id` dedup path matched across the two syncs → duplicate row. The one-shot historical first sync has no duplicates; the duplicates start with the daily incremental syncs. ## Fix Treat a positional (`^\d{4}-\d{2}-\d{2}\.\d+$`) `entry_reference` as "no stable id" and fall through to the content hash, which is identical on both syncs. Genuine `transaction_id` and non-positional `entry_reference` keep keying exactly as before. ## Trade-off (accepted, follow-up tracked) The positional index is also the only field that distinguishes two genuinely-distinct same-day transactions with byte-identical content. Falling to the content hash collapses them to one fingerprint, so only the first is kept — a rare silent under-count. We accept it here over the systematic duplication it fixes; **existing rows are unaffected** (their distinct positional value is still stored in `external_transaction_id` and caught by the fallback dedup path on re-sync). Fixing both cases needs occurrence-aware dedup in the consumer (a schema change) — left as a follow-up and documented in the code. ## Tests - Regression: a positional `entry_reference` matches the same transaction seen earlier without one. - Boundary: non-positional references still key on `entry_reference`. - `php artisan test tests/Unit/Services/Banking/TransactionFingerprintTest.php` → 6 passed. `pint` clean. ## Data cleanup (separate, after deploy) The already-duplicated rows still need a one-off cleanup (soft-delete the later copy per group, keeping the content-fingerprint original). It must run **after** this fix ships, otherwise the next daily sync re-creates them. Not included in this PR. --- .../Banking/TransactionFingerprint.php | 30 ++++++++++++---- .../Banking/TransactionFingerprintTest.php | 35 +++++++++++++++++++ 2 files changed, 59 insertions(+), 6 deletions(-) diff --git a/app/Services/Banking/TransactionFingerprint.php b/app/Services/Banking/TransactionFingerprint.php index 29bbbc4c..ec63275d 100644 --- a/app/Services/Banking/TransactionFingerprint.php +++ b/app/Services/Banking/TransactionFingerprint.php @@ -5,10 +5,7 @@ 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. + * id (transaction_id / entry_reference), consumed by TransactionSyncService. */ class TransactionFingerprint { @@ -21,8 +18,24 @@ class TransactionFingerprint return self::hash(['transaction_id', $data['transaction_id']]); } - if (($data['entry_reference'] ?? null) !== null) { - return self::hash(['entry_reference', $data['entry_reference']]); + $entryReference = $data['entry_reference'] ?? null; + + // Some ASPSPs emit a positional `{booking_date}.{index}` entry_reference + // that is absent the day a transaction first appears and only populated + // on a later sync. Keying on it fingerprints the same transaction + // differently across syncs, so it slips past dedup and imports twice. + // Treat that positional form as "no stable id" and fall through to the + // content hash, which is identical on both syncs. + // + // Trade-off: the index is also the only field that would tell apart two + // genuinely distinct same-day transactions with byte-identical content + // (e.g. two identical tolls). Dropping it collapses them to one + // fingerprint, so only the first is kept. We accept that here — a rare + // silent under-count over the systematic duplication it fixes. Fixing + // both needs occurrence-aware dedup in the consumer (a schema change), + // tracked as a follow-up. + if ($entryReference !== null && ! self::isPositionalReference($entryReference)) { + return self::hash(['entry_reference', $entryReference]); } return self::hash([ @@ -43,6 +56,11 @@ class TransactionFingerprint ]); } + private static function isPositionalReference(string $reference): bool + { + return preg_match('/^\d{4}-\d{2}-\d{2}\.\d+$/D', $reference) === 1; + } + /** * @param array|string $remittance */ diff --git a/tests/Unit/Services/Banking/TransactionFingerprintTest.php b/tests/Unit/Services/Banking/TransactionFingerprintTest.php index f5ad025b..8a0345e8 100644 --- a/tests/Unit/Services/Banking/TransactionFingerprintTest.php +++ b/tests/Unit/Services/Banking/TransactionFingerprintTest.php @@ -41,6 +41,41 @@ test('entry reference is the canonical fingerprint when transaction id is absent ->toBe(TransactionFingerprint::for($changedPayload)); }); +test('positional entry reference is ignored so it matches the same transaction seen without one', function () { + $firstSync = baseEnableBankingPayload([ + 'transaction_id' => null, + 'entry_reference' => null, + ]); + + $laterSync = baseEnableBankingPayload([ + 'transaction_id' => null, + 'entry_reference' => '2025-05-12.0', + ]); + + expect(TransactionFingerprint::for($firstSync)) + ->toBe(TransactionFingerprint::for($laterSync)); +}); + +test('a non-positional entry reference is still the canonical fingerprint', function () { + // Only the exact `{date}.{index}` shape is treated as positional; anything + // else keeps keying on entry_reference so real bank references are honoured. + foreach (['2025-05-12', '2025-05-12.0.1', 'REF-2025-05-12.0', '2025-05-12.'] as $reference) { + $withReference = baseEnableBankingPayload([ + 'transaction_id' => null, + 'entry_reference' => $reference, + ]); + + $sameReferenceOtherContent = baseEnableBankingPayload([ + 'transaction_id' => null, + 'entry_reference' => $reference, + 'creditor' => ['name' => 'Different Merchant'], + ]); + + expect(TransactionFingerprint::for($withReference)) + ->toBe(TransactionFingerprint::for($sameReferenceOtherContent)); + } +}); + test('fallback fingerprint ignores volatile status and settlement dates', function () { $payload = baseEnableBankingPayload([ 'transaction_id' => null,