From 9de737764bc92cbcbeaf52d28b9970b869c4aeb5 Mon Sep 17 00:00:00 2001 From: romanetar Date: Wed, 15 Apr 2026 18:49:34 +0200 Subject: [PATCH] fix(sponsor-sync): use SELECT result to detect row existence in addSponsorPermission, not UPDATE affected-row count Signed-off-by: romanetar --- app/Models/Foundation/Main/Member.php | 23 +++-- .../SponsorPermissionTrackingTest.php | 84 +++++++++++++++++++ 2 files changed, 100 insertions(+), 7 deletions(-) diff --git a/app/Models/Foundation/Main/Member.php b/app/Models/Foundation/Main/Member.php index 76c57f37c..a03425a0a 100644 --- a/app/Models/Foundation/Main/Member.php +++ b/app/Models/Foundation/Main/Member.php @@ -3468,18 +3468,25 @@ public function getIndividualMemberJoinDate(): ?\DateTime * concurrent jobs for the same (member, sponsor, slug) serialize here and * the second job always reads the post-first-job value, preventing duplicates. * - * Returns the number of rows matched by the WHERE clause (0 when the - * Sponsor_Users row does not yet exist, 1 when it does). + * Returns 0 when the Sponsor_Users row does not exist, 1 when it does + * (regardless of whether the slug was newly added or was already present). */ public function addSponsorPermission(int $sponsor_id, string $group_slug): int { - // Lock the row before the read-modify-write so concurrent transactions - // serialize and the IF(JSON_CONTAINS) in the UPDATE sees the committed state. - $this->prepareRawSQL( - 'SELECT Permissions FROM Sponsor_Users WHERE SponsorID = :sponsor_id AND MemberID = :member_id FOR UPDATE', + // Lock the row and detect whether it exists in a single query. + // Using the SELECT result (not the UPDATE's affected-row count) to determine + // existence avoids the false-0 that MySQL returns when the UPDATE is a no-op + // because the slug was already in Permissions (IF branch returns same value). + $res = $this->prepareRawSQL( + 'SELECT 1 FROM Sponsor_Users WHERE SponsorID = :sponsor_id AND MemberID = :member_id FOR UPDATE', ['sponsor_id' => $sponsor_id, 'member_id' => $this->getId()] )->executeQuery(); + if ($res->fetchOne() === false) { + return 0; // Row does not exist; caller must create it first. + } + + // Row exists: ensure the permission is present (idempotent). $sql = <<prepareRawSQL($sql, [ + $this->prepareRawSQL($sql, [ 'group_slug' => $group_slug, 'sponsor_id' => $sponsor_id, 'member_id' => $this->getId(), ])->executeStatement(); + + return 1; // Row existed; permission is now guaranteed to be present. } /** diff --git a/tests/Unit/Entities/SponsorPermissionTrackingTest.php b/tests/Unit/Entities/SponsorPermissionTrackingTest.php index 55074b05c..a8127da50 100644 --- a/tests/Unit/Entities/SponsorPermissionTrackingTest.php +++ b/tests/Unit/Entities/SponsorPermissionTrackingTest.php @@ -413,4 +413,88 @@ public function testAddUserAllowsMemberInMultipleSponsorsForSameSummit(): void $memberIds = array_map(fn($m) => $m->getId(), $sponsor1->getMembers()->toArray()); $this->assertContains(self::$member->getId(), $memberIds); } + + // ------------------------------------------------------------------------- + // 9. Member::addSponsorPermission — retry after eager row creation + // ------------------------------------------------------------------------- + + public function testAddSponsorPermissionReturnsOneWhenPermissionAlreadyPresent(): void + { + $sponsor_id = self::$sponsors[0]->getId(); + $member_id = self::$member->getId(); + + // Seed Permissions with the slug that will be re-added — this produces the + // "no rows changed" MySQL response that previously caused the false 0 return. + $this->setPermissions($sponsor_id, $member_id, IGroup::Sponsors); + self::$em->clear(); + + $member = self::$member_repository->find($member_id); + $result = $member->addSponsorPermission($sponsor_id, IGroup::Sponsors); + + $this->assertSame( + 1, + $result, + 'addSponsorPermission must return 1 when the row exists, even if the slug was already present ' . + '(old code returned 0 here, triggering eager creation and ultimately a RuntimeException).' + ); + } + + /** + * End-to-end simulation of the eager-creation retry path in + * SponsorUserSyncService::addSponsorUserToGroup. + * + * Sequence: + * 1. No Sponsor_Users row → addSponsorPermission returns 0. + * 2. The service creates the row via Sponsor::addUser (eager creation). + * 3. Retry → addSponsorPermission returns 1 and writes the permission. + * + * Before the fix, step 3 returned 0 when the initial call also returned 0 because + * the row already existed with the permission set, causing a RuntimeException. + * This test ensures the retry succeeds whenever the row is present after creation. + */ + public function testAddSponsorPermissionRetrySucceedsAfterEagerRowCreation(): void + { + $sponsor_id = self::$sponsors[0]->getId(); + $member_id = self::$member->getId(); + + // Remove the existing Sponsor_Users entry to simulate the race-condition scenario + // where the group event arrives before the membership event. + self::$em->getConnection()->executeStatement( + 'DELETE FROM Sponsor_Users WHERE SponsorID = ? AND MemberID = ?', + [$sponsor_id, $member_id] + ); + self::$em->clear(); + + $member = self::$member_repository->find($member_id); + + // Step 1 — first call: row does not exist → must return 0. + $firstResult = $member->addSponsorPermission($sponsor_id, IGroup::Sponsors); + $this->assertSame(0, $firstResult, 'First call must return 0 when no Sponsor_Users row exists.'); + + // Step 2 — eager creation: SponsorUserSyncService calls Sponsor::addUser to + // insert the row, then flushes so the INSERT is visible within the transaction. + self::$em->clear(); + $sponsor = self::$em->find(Sponsor::class, $sponsor_id); + $member = self::$member_repository->find($member_id); + $sponsor->addUser($member); + self::$em->flush(); + self::$em->clear(); + + // Step 3 — retry: row now exists → must return 1 and write the permission. + $member = self::$member_repository->find($member_id); + $retryResult = $member->addSponsorPermission($sponsor_id, IGroup::Sponsors); + $this->assertSame(1, $retryResult, 'Retry must return 1 after the Sponsor_Users row has been created.'); + + $raw = self::$em->getConnection()->executeQuery( + 'SELECT Permissions FROM Sponsor_Users WHERE SponsorID = ? AND MemberID = ?', + [$sponsor_id, $member_id] + )->fetchOne(); + + $permissions = json_decode($raw, true) ?? []; + $this->assertContains( + IGroup::Sponsors, + $permissions, + 'The Sponsors slug must be present in Permissions after a successful retry.' + ); + } }