Skip to content

Commit f3923fa

Browse files
romanetarromanetar
authored
fix(sponsor-sync): use SELECT result to detect row existence in addSponsorPermission, not UPDATE affected-row count (#529)
Signed-off-by: romanetar <[email protected]>
1 parent eb85f34 commit f3923fa

2 files changed

Lines changed: 100 additions & 7 deletions

File tree

‎app/Models/Foundation/Main/Member.php‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3468,18 +3468,25 @@ public function getIndividualMemberJoinDate(): ?\DateTime
34683468
* concurrent jobs for the same (member, sponsor, slug) serialize here and
34693469
* the second job always reads the post-first-job value, preventing duplicates.
34703470
*
3471-
* Returns the number of rows matched by the WHERE clause (0 when the
3472-
* Sponsor_Users row does not yet exist, 1 when it does).
3471+
* Returns 0 when the Sponsor_Users row does not exist, 1 when it does
3472+
* (regardless of whether the slug was newly added or was already present).
34733473
*/
34743474
public function addSponsorPermission(int $sponsor_id, string $group_slug): int
34753475
{
3476-
// Lock the row before the read-modify-write so concurrent transactions
3477-
// serialize and the IF(JSON_CONTAINS) in the UPDATE sees the committed state.
3478-
$this->prepareRawSQL(
3479-
'SELECT Permissions FROM Sponsor_Users WHERE SponsorID = :sponsor_id AND MemberID = :member_id FOR UPDATE',
3476+
// Lock the row and detect whether it exists in a single query.
3477+
// Using the SELECT result (not the UPDATE's affected-row count) to determine
3478+
// existence avoids the false-0 that MySQL returns when the UPDATE is a no-op
3479+
// because the slug was already in Permissions (IF branch returns same value).
3480+
$res = $this->prepareRawSQL(
3481+
'SELECT 1 FROM Sponsor_Users WHERE SponsorID = :sponsor_id AND MemberID = :member_id FOR UPDATE',
34803482
['sponsor_id' => $sponsor_id, 'member_id' => $this->getId()]
34813483
)->executeQuery();
34823484

3485+
if ($res->fetchOne() === false) {
3486+
return 0; // Row does not exist; caller must create it first.
3487+
}
3488+
3489+
// Row exists: ensure the permission is present (idempotent).
34833490
$sql = <<<SQL
34843491
UPDATE Sponsor_Users
34853492
SET Permissions = IF(
@@ -3489,11 +3496,13 @@ public function addSponsorPermission(int $sponsor_id, string $group_slug): int
34893496
)
34903497
WHERE SponsorID = :sponsor_id AND MemberID = :member_id
34913498
SQL;
3492-
return $this->prepareRawSQL($sql, [
3499+
$this->prepareRawSQL($sql, [
34933500
'group_slug' => $group_slug,
34943501
'sponsor_id' => $sponsor_id,
34953502
'member_id' => $this->getId(),
34963503
])->executeStatement();
3504+
3505+
return 1; // Row existed; permission is now guaranteed to be present.
34973506
}
34983507

34993508
/**

‎tests/Unit/Entities/SponsorPermissionTrackingTest.php‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -413,4 +413,88 @@ public function testAddUserAllowsMemberInMultipleSponsorsForSameSummit(): void
413413
$memberIds = array_map(fn($m) => $m->getId(), $sponsor1->getMembers()->toArray());
414414
$this->assertContains(self::$member->getId(), $memberIds);
415415
}
416+
417+
// -------------------------------------------------------------------------
418+
// 9. Member::addSponsorPermission — retry after eager row creation
419+
// -------------------------------------------------------------------------
420+
421+
public function testAddSponsorPermissionReturnsOneWhenPermissionAlreadyPresent(): void
422+
{
423+
$sponsor_id = self::$sponsors[0]->getId();
424+
$member_id = self::$member->getId();
425+
426+
// Seed Permissions with the slug that will be re-added — this produces the
427+
// "no rows changed" MySQL response that previously caused the false 0 return.
428+
$this->setPermissions($sponsor_id, $member_id, IGroup::Sponsors);
429+
self::$em->clear();
430+
431+
$member = self::$member_repository->find($member_id);
432+
$result = $member->addSponsorPermission($sponsor_id, IGroup::Sponsors);
433+
434+
$this->assertSame(
435+
1,
436+
$result,
437+
'addSponsorPermission must return 1 when the row exists, even if the slug was already present ' .
438+
'(old code returned 0 here, triggering eager creation and ultimately a RuntimeException).'
439+
);
440+
}
441+
442+
/**
443+
* End-to-end simulation of the eager-creation retry path in
444+
* SponsorUserSyncService::addSponsorUserToGroup.
445+
*
446+
* Sequence:
447+
* 1. No Sponsor_Users row → addSponsorPermission returns 0.
448+
* 2. The service creates the row via Sponsor::addUser (eager creation).
449+
* 3. Retry → addSponsorPermission returns 1 and writes the permission.
450+
*
451+
* Before the fix, step 3 returned 0 when the initial call also returned 0 because
452+
* the row already existed with the permission set, causing a RuntimeException.
453+
* This test ensures the retry succeeds whenever the row is present after creation.
454+
*/
455+
public function testAddSponsorPermissionRetrySucceedsAfterEagerRowCreation(): void
456+
{
457+
$sponsor_id = self::$sponsors[0]->getId();
458+
$member_id = self::$member->getId();
459+
460+
// Remove the existing Sponsor_Users entry to simulate the race-condition scenario
461+
// where the group event arrives before the membership event.
462+
self::$em->getConnection()->executeStatement(
463+
'DELETE FROM Sponsor_Users WHERE SponsorID = ? AND MemberID = ?',
464+
[$sponsor_id, $member_id]
465+
);
466+
self::$em->clear();
467+
468+
$member = self::$member_repository->find($member_id);
469+
470+
// Step 1 — first call: row does not exist → must return 0.
471+
$firstResult = $member->addSponsorPermission($sponsor_id, IGroup::Sponsors);
472+
$this->assertSame(0, $firstResult, 'First call must return 0 when no Sponsor_Users row exists.');
473+
474+
// Step 2 — eager creation: SponsorUserSyncService calls Sponsor::addUser to
475+
// insert the row, then flushes so the INSERT is visible within the transaction.
476+
self::$em->clear();
477+
$sponsor = self::$em->find(Sponsor::class, $sponsor_id);
478+
$member = self::$member_repository->find($member_id);
479+
$sponsor->addUser($member);
480+
self::$em->flush();
481+
self::$em->clear();
482+
483+
// Step 3 — retry: row now exists → must return 1 and write the permission.
484+
$member = self::$member_repository->find($member_id);
485+
$retryResult = $member->addSponsorPermission($sponsor_id, IGroup::Sponsors);
486+
$this->assertSame(1, $retryResult, 'Retry must return 1 after the Sponsor_Users row has been created.');
487+
488+
$raw = self::$em->getConnection()->executeQuery(
489+
'SELECT Permissions FROM Sponsor_Users WHERE SponsorID = ? AND MemberID = ?',
490+
[$sponsor_id, $member_id]
491+
)->fetchOne();
492+
493+
$permissions = json_decode($raw, true) ?? [];
494+
$this->assertContains(
495+
IGroup::Sponsors,
496+
$permissions,
497+
'The Sponsors slug must be present in Permissions after a successful retry.'
498+
);
499+
}
416500
}

0 commit comments

Comments
 (0)