perf(banking): kill per-transaction dedup N+1 in bank sync (PHP-LARAVEL-3Y) (#621)
## What
Fixes the N+1 flagged by Sentry **PHP-LARAVEL-3Y** in
`App\Jobs\SyncBankingConnectionJob`.
`TransactionSyncService::importTransaction()` ran one `exists()` probe
**per incoming bank transaction** to detect duplicates:
```sql
select exists(
select * from transactions
where account_id = ?
and (dedup_fingerprint = ? or external_transaction_id = ?)
) as exists
```
A sync importing N transactions issued N dedup `SELECT`s. As transaction
volume per sync grows, so does the query count.
## How
Preload the account's existing dedup keys **once** at the start of
`sync()` and match against in-memory sets:
- `loadExistingDedupKeys()` streams (`cursor()`) the two dedup columns
for the account — including soft-deleted rows (`withTrashed()`) — into
two keyed sets. One query instead of N.
- `importTransaction()` checks membership in memory (`isset()`), an
exact mirror of the old `fingerprint OR external_id` predicate.
- Keys from successful inserts are folded back into the sets, so
duplicates **within the same sync** (e.g. the same transaction repeated
across pages) are still caught in memory.
- The `(account_id, dedup_fingerprint)` unique index +
`UniqueConstraintViolationException` catch still backstop concurrent
syncs (`SyncBankingConnectionJob` is already `ShouldBeUnique` per
connection).
## Commits
1. `perf(banking): preload dedup keys to kill per-transaction N+1` — the
core fix + a query-count regression test.
2. `perf(banking): stream dedup key preload with a cursor` — avoids
buffering every historical row into a Collection on top of the sets
(memory guard for very large accounts).
3. `fix(banking): match external ids case-insensitively in dedup` — the
old query compared `external_transaction_id` under the production
`utf8mb4_unicode_ci` collation; the in-memory `isset()` is byte-exact.
With no unique index on `external_transaction_id`, a legacy id stored as
`ABC` vs an incoming `abc` would have silently double-imported.
Normalized with `mb_strtolower()` on both sides + a test.
4. `test(banking): cover intra-run cross-page dedup` — new coverage for
the same transaction appearing on two pages of one sync.
## Testing
- 303/303 in `tests/Feature/OpenBanking/` green; full backend suite
green except one **pre-existing, unrelated** failure on `main`
(`DashboardTest` returns 409 locally — an Inertia asset-version artifact
that resolves once `bun run build` runs in CI).
- New tests: dedup runs as a single preload SELECT regardless of batch
size; case-insensitive external-id dedup; cross-page intra-run dedup.
- `pint` clean; `larastan` clean on the changed file.
## Reviewed by two independent agents (architecture +
product/correctness)
Both rated the change behavior-preserving and shippable. Follow-ups they
surfaced, intentionally **not** bundled here to keep this fix focused
and low-risk:
- **`WiseTransactionSyncService` has the identical latent N+1**
(`:149-158`). It is not currently firing in Sentry (lower volume). Worth
a separate PR — possibly extracting a shared dedup-set helper once both
call sites need it.
- **Residual collation divergence**: accent/width folding from
`utf8mb4_unicode_ci` is not replicated in PHP. Irrelevant for real bank
reference ids (ASCII), accepted.
- The test comment referencing a "cleanup command" that repoints orphan
fingerprinted rows describes a command that does not exist in the repo —
pre-existing, worth correcting separately.
Fixes PHP-LARAVEL-3Y
This commit is contained in:
Víctor Falcón
GitHub
parent
0f8eca50d0
commit
84bad76316
|
|
@ -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<string, true>, 1: array<string, true>} 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.
|
||||
|
|
|
|||
|
|
@ -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]);
|
||||
|
|
|
|||
Loading…
Reference in New Issue