Name the mechanism that actually rebuilds the row, and log what the delete removed
The migration's own docblock justified an irreversible delete with a mechanism
that cannot fire: after the dedupe the deleted row's Price id is always still
claimed, by the survivor, so AdoptStripePrice's `claimed` callback always skips
it. The rebuild is real, but it goes through a fresh createPrice() under the
deleted row's own tuple-specific idempotency key, not through adoption. A
comment whose whole job is to justify deleting production rows has to name the
mechanism that runs, not the one that structurally cannot — this is the fourth
false justification this branch has carried, and the incident the whole
feature exists for started the same way.
The delete itself left no trace: down() restores the index, not the rows, and
two rows on one Price necessarily have different tuples, so at most one can
match what the Price actually charges — after the one real run there would be
nothing to check the survivor against. Added a Log::warning per duplicate
group naming the shared Price, the row kept, and every row thrown away.
Test: read back stripe_price_id and amount_cents on the surviving and
unrelated rows instead of only checking they still exist, closing the "kept
the right row but damaged it" gap. Added a second duplicate group of three
rows sharing one Price so the migration's foreach runs more than once and a
single iteration deletes more than one row. Re-verified the "lowest id
survives" assertion is load-bearing by flipping min('id') to max('id') during
this fix and confirming the test fails there, then reverting.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
feature/betriebsmodus^2
nexxo
parent
2c9e9ea398
commit
cb9781d2e7
|
|
@ -3,6 +3,7 @@
|
|||
use Illuminate\Database\Migrations\Migration;
|
||||
use Illuminate\Database\Schema\Blueprint;
|
||||
use Illuminate\Support\Facades\DB;
|
||||
use Illuminate\Support\Facades\Log;
|
||||
use Illuminate\Support\Facades\Schema;
|
||||
|
||||
/**
|
||||
|
|
@ -18,10 +19,22 @@ use Illuminate\Support\Facades\Schema;
|
|||
*
|
||||
* Duplicates are DELETED rather than archived. An archived row goes on claiming
|
||||
* the id, and a unique index knows nothing about `archived_at`. Deleting is safe
|
||||
* because the row rebuilds itself: the next ensure() finds the Price at Stripe
|
||||
* through the adoption step and writes the row again. `subscription_addons`
|
||||
* holds the Stripe id as text, not as a foreign key, so no booking loses its
|
||||
* price.
|
||||
* because the row rebuilds itself — but not through adoption: the survivor keeps
|
||||
* the shared Price, so AdoptStripePrice's `claimed` callback reports it taken no
|
||||
* matter which row asks, and the deleted row's own tuple — necessarily a
|
||||
* DIFFERENT one, since the composite unique on (addon_key, reverse_charge,
|
||||
* amount_cents, currency, interval) from 2026_07_31_200000 forbids two rows
|
||||
* sharing a tuple — mints a fresh Price of its own instead, under its own
|
||||
* tuple-specific idempotency key (AddonPrices::ensure() falling through to
|
||||
* createPrice()). `subscription_addons` holds the Stripe id as text, not as a
|
||||
* foreign key, so no booking loses its price either way.
|
||||
*
|
||||
* The `Log::warning` below is the only record this ever gets: `down()` restores
|
||||
* the index, not the rows it deleted. It names the shared Price, the row kept,
|
||||
* and every row thrown away, so an operator reading the log after the one real
|
||||
* run this migration gets can check the survivor's `amount_cents` against what
|
||||
* Stripe actually charges for that Price — the lowest id is written first, not
|
||||
* necessarily right, and nothing else will say what was discarded.
|
||||
*/
|
||||
return new class extends Migration
|
||||
{
|
||||
|
|
@ -36,9 +49,35 @@ return new class extends Migration
|
|||
->pluck('stripe_price_id');
|
||||
|
||||
foreach ($shared as $priceId) {
|
||||
$rows = DB::table('stripe_addon_prices')->where('stripe_price_id', $priceId)->get();
|
||||
|
||||
// The lowest id stays — the row that was written first, which is the
|
||||
// one anything already billing is likeliest to have been reading.
|
||||
$keep = DB::table('stripe_addon_prices')->where('stripe_price_id', $priceId)->min('id');
|
||||
$keep = (int) $rows->min('id');
|
||||
$kept = $rows->first(fn ($row) => (int) $row->id === $keep);
|
||||
$discarded = $rows->reject(fn ($row) => (int) $row->id === $keep);
|
||||
|
||||
// The only forensic trace this delete ever leaves — see the class
|
||||
// docblock for why that has to be enough.
|
||||
Log::warning('stripe: kept one row of several claiming one price, deleted the rest', [
|
||||
'stripe_price_id' => $priceId,
|
||||
'kept' => [
|
||||
'id' => $kept->id,
|
||||
'addon_key' => $kept->addon_key,
|
||||
'reverse_charge' => (bool) $kept->reverse_charge,
|
||||
'amount_cents' => $kept->amount_cents,
|
||||
'currency' => $kept->currency,
|
||||
'interval' => $kept->interval,
|
||||
],
|
||||
'deleted' => $discarded->map(fn ($row) => [
|
||||
'id' => $row->id,
|
||||
'addon_key' => $row->addon_key,
|
||||
'reverse_charge' => (bool) $row->reverse_charge,
|
||||
'amount_cents' => $row->amount_cents,
|
||||
'currency' => $row->currency,
|
||||
'interval' => $row->interval,
|
||||
])->values()->all(),
|
||||
]);
|
||||
|
||||
DB::table('stripe_addon_prices')
|
||||
->where('stripe_price_id', $priceId)
|
||||
|
|
|
|||
|
|
@ -471,7 +471,7 @@ it('lets no two module rows claim one stripe price', function () {
|
|||
->toThrow(UniqueConstraintViolationException::class);
|
||||
});
|
||||
|
||||
it('deletes the newer of two rows already sharing one stripe price when the migration runs', function () {
|
||||
it('keeps only the lowest-id row in every group of duplicates, and leaves the rest of the table alone', function () {
|
||||
// This is the one branch of 2026_07_31_210000_one_row_per_stripe_price that
|
||||
// deletes rows from a live table, and it cannot be reached through the
|
||||
// ordinary migrated schema: RefreshDatabase already ran this migration, so
|
||||
|
|
@ -482,7 +482,7 @@ it('deletes the newer of two rows already sharing one stripe price when the migr
|
|||
// delete branch instead of finding nothing to do.
|
||||
Schema::table('stripe_addon_prices', fn (Blueprint $table) => $table->dropUnique('stripe_addon_prices_price_unique'));
|
||||
|
||||
// Two rows sharing one Price, differing in `reverse_charge` so the
|
||||
// Group one: two rows sharing a Price, differing in `reverse_charge` so the
|
||||
// pre-existing COMPOSITE unique index (addon_key, reverse_charge,
|
||||
// amount_cents, currency, interval) does not itself refuse the insert —
|
||||
// the VAT-rate-zero case Block D warned about, where only the shared
|
||||
|
|
@ -501,7 +501,34 @@ it('deletes the newer of two rows already sharing one stripe price when the migr
|
|||
'created_at' => now(), 'updated_at' => now(),
|
||||
]);
|
||||
|
||||
// On its own Price entirely, and must survive untouched.
|
||||
// Group two: THREE rows sharing a second, different Price — its own tuple
|
||||
// dimension varied per row (interval, then currency) so the composite index
|
||||
// never fires here either. One group of two proves the loop runs; this one
|
||||
// proves it runs more than once AND that a single iteration can delete more
|
||||
// than one row.
|
||||
$kept2 = DB::table('stripe_addon_prices')->insertGetId([
|
||||
'addon_key' => 'priority_support', 'reverse_charge' => false,
|
||||
'amount_cents' => 3900, 'net_cents' => 3900, 'currency' => 'EUR', 'interval' => 'month',
|
||||
'stripe_product_id' => 'prod_support', 'stripe_price_id' => 'price_shared_two',
|
||||
'created_at' => now(), 'updated_at' => now(),
|
||||
]);
|
||||
|
||||
$duplicate2a = DB::table('stripe_addon_prices')->insertGetId([
|
||||
'addon_key' => 'priority_support', 'reverse_charge' => false,
|
||||
'amount_cents' => 3900, 'net_cents' => 3900, 'currency' => 'EUR', 'interval' => 'year',
|
||||
'stripe_product_id' => 'prod_support', 'stripe_price_id' => 'price_shared_two',
|
||||
'created_at' => now(), 'updated_at' => now(),
|
||||
]);
|
||||
|
||||
$duplicate2b = DB::table('stripe_addon_prices')->insertGetId([
|
||||
'addon_key' => 'priority_support', 'reverse_charge' => false,
|
||||
'amount_cents' => 3900, 'net_cents' => 3900, 'currency' => 'USD', 'interval' => 'month',
|
||||
'stripe_product_id' => 'prod_support', 'stripe_price_id' => 'price_shared_two',
|
||||
'created_at' => now(), 'updated_at' => now(),
|
||||
]);
|
||||
|
||||
// On its own Price entirely — not part of any duplicate group — and must
|
||||
// survive completely untouched.
|
||||
$unrelated = DB::table('stripe_addon_prices')->insertGetId([
|
||||
'addon_key' => 'extra_storage', 'reverse_charge' => false,
|
||||
'amount_cents' => 1000, 'net_cents' => 1000, 'currency' => 'EUR', 'interval' => 'month',
|
||||
|
|
@ -514,13 +541,35 @@ it('deletes the newer of two rows already sharing one stripe price when the migr
|
|||
// migration's up() a second time in this same process.
|
||||
(require database_path('migrations/2026_07_31_210000_one_row_per_stripe_price.php'))->up();
|
||||
|
||||
// The LOWER id survives — the row that was written first, which is the one
|
||||
// anything already billing is likeliest to have been reading. Were that
|
||||
// ordering silently inverted, the surviving row could be one nothing ever
|
||||
// pointed at.
|
||||
expect(DB::table('stripe_addon_prices')->where('id', $kept)->exists())->toBeTrue()
|
||||
$keptRow = DB::table('stripe_addon_prices')->where('id', $kept)->first();
|
||||
$kept2Row = DB::table('stripe_addon_prices')->where('id', $kept2)->first();
|
||||
$unrelatedRow = DB::table('stripe_addon_prices')->where('id', $unrelated)->first();
|
||||
|
||||
// The LOWER id survives in each group — the row that was written first,
|
||||
// which is the one anything already billing is likeliest to have been
|
||||
// reading. Were that ordering silently inverted, the surviving row could be
|
||||
// one nothing ever pointed at. Read back, not merely checked for existence:
|
||||
// a survivor that exists but was mangled in the process — the wrong
|
||||
// stripe_price_id, or an amount_cents that no longer matches what the
|
||||
// shared Price actually charges — would be exactly the "kept the right row
|
||||
// but damaged it" failure existence alone cannot catch.
|
||||
expect($keptRow)->not->toBeNull()
|
||||
->and($keptRow->stripe_price_id)->toBe('price_shared')
|
||||
->and((int) $keptRow->amount_cents)->toBe(2900)
|
||||
->and($kept2Row)->not->toBeNull()
|
||||
->and($kept2Row->stripe_price_id)->toBe('price_shared_two')
|
||||
->and((int) $kept2Row->amount_cents)->toBe(3900)
|
||||
->and($unrelatedRow)->not->toBeNull()
|
||||
->and($unrelatedRow->stripe_price_id)->toBe('price_untouched')
|
||||
->and((int) $unrelatedRow->amount_cents)->toBe(1000)
|
||||
// Every duplicate in both groups is gone — the higher id from group
|
||||
// one, and BOTH higher ids from group two's three-way tie.
|
||||
->and(DB::table('stripe_addon_prices')->where('id', $duplicate)->exists())->toBeFalse()
|
||||
->and(DB::table('stripe_addon_prices')->where('id', $unrelated)->exists())->toBeTrue();
|
||||
->and(DB::table('stripe_addon_prices')->where('id', $duplicate2a)->exists())->toBeFalse()
|
||||
->and(DB::table('stripe_addon_prices')->where('id', $duplicate2b)->exists())->toBeFalse()
|
||||
// Six rows went in, three survive: nothing beyond the two kept
|
||||
// duplicates and the untouched row is left standing.
|
||||
->and(DB::table('stripe_addon_prices')->count())->toBe(3);
|
||||
|
||||
// And the index is back in force: a further row on a third, otherwise
|
||||
// distinct tuple, but the SAME Price id, is refused by stripe_price_id
|
||||
|
|
|
|||
Loading…
Reference in New Issue