fix(banking): stop duplicating EnableBanking transactions with positional entry_reference (#669)
## 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.
This commit is contained in:
parent
dada23cd84
commit
cd918523e8
|
|
@ -5,10 +5,7 @@ namespace App\Services\Banking;
|
||||||
/**
|
/**
|
||||||
* Builds a deterministic fingerprint for an EnableBanking transaction
|
* Builds a deterministic fingerprint for an EnableBanking transaction
|
||||||
* payload so we can dedup even when the upstream bank omits a stable
|
* payload so we can dedup even when the upstream bank omits a stable
|
||||||
* id (transaction_id / entry_reference).
|
* id (transaction_id / entry_reference), consumed by TransactionSyncService.
|
||||||
*
|
|
||||||
* Shared between the live sync path and the cleanup command so they
|
|
||||||
* stay in lock-step.
|
|
||||||
*/
|
*/
|
||||||
class TransactionFingerprint
|
class TransactionFingerprint
|
||||||
{
|
{
|
||||||
|
|
@ -21,8 +18,24 @@ class TransactionFingerprint
|
||||||
return self::hash(['transaction_id', $data['transaction_id']]);
|
return self::hash(['transaction_id', $data['transaction_id']]);
|
||||||
}
|
}
|
||||||
|
|
||||||
if (($data['entry_reference'] ?? null) !== null) {
|
$entryReference = $data['entry_reference'] ?? null;
|
||||||
return self::hash(['entry_reference', $data['entry_reference']]);
|
|
||||||
|
// 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([
|
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<int, string>|string $remittance
|
* @param array<int, string>|string $remittance
|
||||||
*/
|
*/
|
||||||
|
|
|
||||||
|
|
@ -41,6 +41,41 @@ test('entry reference is the canonical fingerprint when transaction id is absent
|
||||||
->toBe(TransactionFingerprint::for($changedPayload));
|
->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 () {
|
test('fallback fingerprint ignores volatile status and settlement dates', function () {
|
||||||
$payload = baseEnableBankingPayload([
|
$payload = baseEnableBankingPayload([
|
||||||
'transaction_id' => null,
|
'transaction_id' => null,
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue