From 3bc87aa2d8b5fe87d7b96569642a54cd01c43630 Mon Sep 17 00:00:00 2001 From: romanetar Date: Fri, 23 Oct 2026 14:55:11 +0200 Subject: [PATCH 1/3] fix(member): route group checks through the request-scoped membership cache isAdmin, isSummitAdmin, isRegistrationAdmin, isTester, isTrackChairAdmin and isOnGroup ran one Group_Members query per call through getGroupByCode(), the most executed statement on prod-12. They now resolve membership through belongsToGroup(), which answers from $groupMembershipCache after the first lookup per group code. getGroupByCode() consults and fills the same cache so a known negative answer costs no query. belongsToGroup() no longer caches the result when the query fails, so a transient error cannot stick as "not a member" for the request. --- app/Models/Foundation/Main/Member.php | 36 ++-- .../Models/MemberGroupMembershipCacheTest.php | 174 ++++++++++++++++++ .../MemberGroupMembershipFailureTest.php | 80 ++++++++ 3 files changed, 271 insertions(+), 19 deletions(-) create mode 100644 tests/Unit/Models/MemberGroupMembershipCacheTest.php create mode 100644 tests/Unit/Models/MemberGroupMembershipFailureTest.php diff --git a/app/Models/Foundation/Main/Member.php b/app/Models/Foundation/Main/Member.php index 6ae4a1cd5..ae2894dac 100644 --- a/app/Models/Foundation/Main/Member.php +++ b/app/Models/Foundation/Main/Member.php @@ -897,14 +897,12 @@ public function isAdmin($skip_external = false): bool { // admin or super admin Log::debug(sprintf("Member::isAdmin id %s email %s", $this->id, $this->email)); - $superAdminGroup = $this->getGroupByCode(IGroup::SuperAdmins); - if (!is_null($superAdminGroup)) { + if ($this->belongsToGroup(IGroup::SuperAdmins)) { Log::debug(sprintf("Member::isAdmin has Super Admin Group On DB")); return true; } - $adminGroup = $this->getGroupByCode(IGroup::Administrators); - if (!is_null($adminGroup)) { + if ($this->belongsToGroup(IGroup::Administrators)) { Log::debug(sprintf("Member::isAdmin has Admin Group On DB")); return true; } @@ -923,8 +921,7 @@ public function isAdmin($skip_external = false): bool public function isSummitAdmin(): bool { - $summitAdminGroup = $this->getGroupByCode(IGroup::SummitAdministrators); - if (!is_null($summitAdminGroup)) + if ($this->belongsToGroup(IGroup::SummitAdministrators)) return true; if ($this->isOnExternalGroup(IGroup::SummitAdministrators)) return true; @@ -933,8 +930,7 @@ public function isSummitAdmin(): bool public function isRegistrationAdmin(): bool { - $summitAdminGroup = $this->getGroupByCode(IGroup::SummitRegistrationAdmins); - if (!is_null($summitAdminGroup)) + if ($this->belongsToGroup(IGroup::SummitRegistrationAdmins)) return true; if ($this->isOnExternalGroup(IGroup::SummitRegistrationAdmins)) return true; @@ -944,8 +940,7 @@ public function isRegistrationAdmin(): bool public function isTester(): bool { - $summitAdminGroup = $this->getGroupByCode(IGroup::Testers); - if (!is_null($summitAdminGroup)) + if ($this->belongsToGroup(IGroup::Testers)) return true; if ($this->isOnExternalGroup(IGroup::Testers)) return true; @@ -954,8 +949,7 @@ public function isTester(): bool public function isTrackChairAdmin(): bool { - $summitAdminGroup = $this->getGroupByCode(IGroup::TrackChairsAdmins); - if (!is_null($summitAdminGroup)) + if ($this->belongsToGroup(IGroup::TrackChairsAdmins)) return true; if ($this->isOnExternalGroup(IGroup::TrackChairsAdmins)) return true; @@ -1018,8 +1012,7 @@ public function isOnGroup(string $code, $skip_external = false) Log::debug(sprintf("Member::isOnGroup member %s group code %s isAdmin true", $this->id, $code)); return true; } - $group = $this->getGroupByCode($code); - if (!is_null($group)) { + if ($this->belongsToGroup($code)) { Log::debug(sprintf("Member::isOnGroup member %s group code %s belongs to group", $this->id, $code)); return true; } @@ -1036,10 +1029,15 @@ public function isOnGroup(string $code, $skip_external = false) */ public function getGroupByCode(string $code): ?Group { + $code = trim($code); + // a known negative answer needs no query + if (array_key_exists($code, $this->groupMembershipCache) && !$this->groupMembershipCache[$code]) { + return null; + } $criteria = Criteria::create(); - $criteria->where(Criteria::expr()->eq('code', trim($code))); + $criteria->where(Criteria::expr()->eq('code', $code)); $res = $this->groups->matching($criteria)->first(); - return $res === false ? null : $res; + return ($this->groupMembershipCache[$code] = ($res !== false)) ? $res : null; } /** @@ -1081,10 +1079,10 @@ public function belongsToGroup(string $code): bool $res = $res->fetchFirstColumn(); return $this->groupMembershipCache[$code] = (intval($res[0]) > 0); } catch (\Exception $ex) { - + // do not cache the failure: a transient error must not stick as "not a member" for the whole request + Log::warning($ex); } - return $this->groupMembershipCache[$code] = false; - + return false; } /** diff --git a/tests/Unit/Models/MemberGroupMembershipCacheTest.php b/tests/Unit/Models/MemberGroupMembershipCacheTest.php new file mode 100644 index 000000000..f0c956500 --- /dev/null +++ b/tests/Unit/Models/MemberGroupMembershipCacheTest.php @@ -0,0 +1,174 @@ +clear(); + } + + protected function tearDown(): void + { + self::clearMemberTestData(); + parent::tearDown(); + } + + private function loadMember(int $id): Member + { + return self::$em->find(Member::class, $id); + } + + /** + * Runs $fn and returns how many membership queries (Group_Members) it issued. + */ + private function countMembershipQueries(callable $fn): int + { + $captured_sql = []; + $logger = new class($captured_sql) implements SQLLogger { + private array $sink; + + public function __construct(array &$sink) + { + $this->sink = &$sink; + } + + public function startQuery($sql, ?array $params = null, ?array $types = null) + { + $this->sink[] = $sql; + } + + public function stopQuery() + { + } + }; + + $connection = Registry::getManager(SilverstripeBaseModel::EntityManager)->getConnection(); + $previous_logger = $connection->getConfiguration()->getSQLLogger(); + $connection->getConfiguration()->setSQLLogger($logger); + + try { + $fn(); + } finally { + $connection->getConfiguration()->setSQLLogger($previous_logger); + } + + return count(array_filter($captured_sql, fn($sql) => stripos($sql, 'Group_Members') !== false)); + } + + public function testRepeatedIsAdminOnNonAdminIssuesAtMostTwoQueries(): void + { + $member = $this->loadMember(self::$member->getId()); + + $queries = $this->countMembershipQueries(function () use ($member) { + for ($i = 0; $i < 10; $i++) { + $this->assertFalse($member->isAdmin(true)); + } + }); + + // SuperAdmins + Administrators, once each + $this->assertSame(2, $queries); + } + + public function testIsOnGroupIssuesAtMostThreeQueries(): void + { + $member = $this->loadMember(self::$member->getId()); + + $queries = $this->countMembershipQueries(function () use ($member) { + for ($i = 0; $i < 10; $i++) { + $this->assertTrue($member->isOnGroup(IGroup::TrackChairs, true)); + } + }); + + // SuperAdmins + Administrators + TrackChairs + $this->assertSame(3, $queries); + } + + public function testIsOnGroupForAdminShortCircuitsWithoutSecondLookup(): void + { + self::setMemberDefaultGroup(IGroup::Administrators); + self::$em->clear(); + $member = $this->loadMember(self::$member->getId()); + + $queries = $this->countMembershipQueries(function () use ($member) { + $this->assertTrue($member->isOnGroup(IGroup::Testers, true)); + }); + + // SuperAdmins (miss) + Administrators (hit); the requested group is never looked up + $this->assertSame(2, $queries); + } + + public function testMemberInGroupAndNotInGroup(): void + { + $member = $this->loadMember(self::$member->getId()); + $summit_admin = $this->loadMember(self::$member2->getId()); + + $this->assertFalse($member->isSummitAdmin()); + $this->assertFalse($member->isRegistrationAdmin()); + $this->assertFalse($member->isTester()); + $this->assertFalse($member->isTrackChairAdmin()); + $this->assertTrue($summit_admin->isSummitAdmin()); + $this->assertFalse($summit_admin->isAdmin(true)); + } + + public function testGroupByCodeAnswersNegativeFromCache(): void + { + $member = $this->loadMember(self::$member->getId()); + $this->assertFalse($member->belongsToGroup(IGroup::Testers)); + + $queries = $this->countMembershipQueries(function () use ($member) { + $this->assertNull($member->getGroupByCode(IGroup::Testers)); + }); + + $this->assertSame(0, $queries); + } + + public function testGroupByCodeFillsTheCache(): void + { + $member = $this->loadMember(self::$member->getId()); + + $group = $member->getGroupByCode(IGroup::TrackChairs); + $this->assertNotNull($group); + $this->assertSame(IGroup::TrackChairs, $group->getCode()); + + $queries = $this->countMembershipQueries(function () use ($member) { + $this->assertTrue($member->belongsToGroup(IGroup::TrackChairs)); + }); + + $this->assertSame(0, $queries); + } +} diff --git a/tests/Unit/Models/MemberGroupMembershipFailureTest.php b/tests/Unit/Models/MemberGroupMembershipFailureTest.php new file mode 100644 index 000000000..48c72f42e --- /dev/null +++ b/tests/Unit/Models/MemberGroupMembershipFailureTest.php @@ -0,0 +1,80 @@ +shouldReceive('fetchFirstColumn')->andReturn([$count]); + + $stmt = Mockery::mock(Statement::class); + $stmt->shouldReceive('executeQuery')->andReturn($result); + + return $stmt; + } + + public function testFailedQueryIsNotCached(): void + { + $member = Mockery::mock(Member::class)->makePartial()->shouldAllowMockingProtectedMethods(); + $calls = 0; + $member->shouldReceive('prepareRawSQL') + ->twice() + ->andReturnUsing(function () use (&$calls) { + if (++$calls === 1) { + throw new \Exception('connection lost'); + } + return $this->membershipStatement(1); + }); + + // the failing lookup answers "not a member"... + $this->assertFalse($member->belongsToGroup(IGroup::Administrators)); + // ...but is not remembered: the next check runs the query again and sees the real answer + $this->assertTrue($member->belongsToGroup(IGroup::Administrators)); + } + + public function testSuccessfulQueryIsStillCached(): void + { + $member = Mockery::mock(Member::class)->makePartial()->shouldAllowMockingProtectedMethods(); + $member->shouldReceive('prepareRawSQL') + ->once() + ->andReturn($this->membershipStatement(0)); + + $this->assertFalse($member->belongsToGroup(IGroup::Administrators)); + $this->assertFalse($member->belongsToGroup(IGroup::Administrators)); + } +} From d7e9b99864dcd9f99691a22fe30b302b89820528 Mon Sep 17 00:00:00 2001 From: romanetar Date: Fri, 23 Oct 2026 14:58:43 +0200 Subject: [PATCH 2/3] test(member): re-attach fixtures before swapping the member group in the admin short-circuit test --- tests/Unit/Models/MemberGroupMembershipCacheTest.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/Unit/Models/MemberGroupMembershipCacheTest.php b/tests/Unit/Models/MemberGroupMembershipCacheTest.php index f0c956500..4f9e7fcb2 100644 --- a/tests/Unit/Models/MemberGroupMembershipCacheTest.php +++ b/tests/Unit/Models/MemberGroupMembershipCacheTest.php @@ -15,6 +15,7 @@ use App\Models\Foundation\Main\IGroup; use Doctrine\DBAL\Logging\SQLLogger; use LaravelDoctrine\ORM\Facades\Registry; +use models\main\Group; use models\main\Member; use models\utils\SilverstripeBaseModel; use Tests\InsertMemberTestData; @@ -120,6 +121,9 @@ public function testIsOnGroupIssuesAtMostThreeQueries(): void public function testIsOnGroupForAdminShortCircuitsWithoutSecondLookup(): void { + // setUp cleared the unit of work: re-attach the fixtures before swapping the member's group + self::$group = self::$em->find(Group::class, self::$group->getId()); + self::$member = self::$em->find(Member::class, self::$member->getId()); self::setMemberDefaultGroup(IGroup::Administrators); self::$em->clear(); $member = $this->loadMember(self::$member->getId()); From fc7ec7b90671f561115f1adfc9d8b7858c450cde Mon Sep 17 00:00:00 2001 From: romanetar Date: Mon, 26 Oct 2026 15:07:53 +0100 Subject: [PATCH 3/3] fix(member): keep unflushed group changes visible to the membership cache add2Group and removeFromGroup now record the change in the request-scoped membership cache instead of clearing it. belongsToGroup() reads the DB, so a membership added but not yet flushed was invisible to isOnGroup(), which made TrackChairService::add fail with 'does not belong to group track-chairs'. --- app/Models/Foundation/Main/Member.php | 8 ++++---- .../Models/MemberGroupMembershipCacheTest.php | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/app/Models/Foundation/Main/Member.php b/app/Models/Foundation/Main/Member.php index ae2894dac..d4ad8fd69 100644 --- a/app/Models/Foundation/Main/Member.php +++ b/app/Models/Foundation/Main/Member.php @@ -2067,9 +2067,9 @@ public function add2Group(Group $group) { if ($this->groups->contains($group)) return; $this->groups->add($group); - // belongsToGroup() memoizes per instance against the DB; a mutation makes - // that memo stale (the fresh row/removal becomes visible after flush). - $this->groupMembershipCache = []; + // belongsToGroup() reads the DB, so an unflushed membership would be invisible to it + // (e.g. TrackChairService adds the group and then checks isOnGroup in the same request). + $this->groupMembershipCache[trim($group->getCode())] = true; } public function removeFromGroup(Group $group) @@ -2077,7 +2077,7 @@ public function removeFromGroup(Group $group) if (!$this->groups->contains($group)) return; $this->groups->removeElement($group); //$group->removeMember($this); - $this->groupMembershipCache = []; + $this->groupMembershipCache[trim($group->getCode())] = false; } /** diff --git a/tests/Unit/Models/MemberGroupMembershipCacheTest.php b/tests/Unit/Models/MemberGroupMembershipCacheTest.php index 4f9e7fcb2..19eaf46f4 100644 --- a/tests/Unit/Models/MemberGroupMembershipCacheTest.php +++ b/tests/Unit/Models/MemberGroupMembershipCacheTest.php @@ -175,4 +175,22 @@ public function testGroupByCodeFillsTheCache(): void $this->assertSame(0, $queries); } + + public function testUnflushedGroupMutationsAreVisibleToGroupChecks(): void + { + $member = $this->loadMember(self::$member->getId()); + $this->assertFalse($member->belongsToGroup(IGroup::Testers)); + + $group = new Group(); + $group->setCode(IGroup::Testers); + $group->setTitle(IGroup::Testers); + self::$em->persist($group); + + // not flushed: the raw SQL lookup alone could not see this membership + $member->add2Group($group); + $this->assertTrue($member->isOnGroup(IGroup::Testers, true)); + + $member->removeFromGroup($group); + $this->assertFalse($member->belongsToGroup(IGroup::Testers)); + } }