fix(member): route group checks through the request-scoped membership cache

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;
     }
 
     /**
@@ -2069,17 +2067,17 @@ 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)
     {
         if (!$this->groups->contains($group)) return;
         $this->groups->removeElement($group);
         //$group->removeMember($this);
-        $this->groupMembershipCache = [];
+        $this->groupMembershipCache[trim($group->getCode())] = false;
     }
 
     /**

tests/Unit/Models/MemberGroupMembershipCacheTest.php

@@ -0,0 +1,196 @@
+<?php namespace Tests\Unit\Models;
+/**
+ * Copyright 2026 OpenStack Foundation
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+
+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;
+use Tests\TestCase;
+
+/**
+ * Class MemberGroupMembershipCacheTest
+ *
+ * Member::isAdmin(), isSummitAdmin(), isRegistrationAdmin(), isTester(), isTrackChairAdmin() and
+ * isOnGroup() answer from the request-scoped $groupMembershipCache after the first lookup per
+ * group code, instead of running one membership query per call. The queries are counted straight
+ * off the connection, the only way to tell "queried once" from "queried N times, same result".
+ *
+ * @package Tests\Unit\Models
+ */
+final class MemberGroupMembershipCacheTest extends TestCase
+{
+    use InsertMemberTestData;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        // self::$member is on TrackChairs (not an admin), self::$member2 on SummitAdministrators
+        self::insertMemberTestData(IGroup::TrackChairs);
+        self::$em->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
+    {
+        // 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());
+
+        $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);
+    }
+
+    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));
+    }
+}

tests/Unit/Models/MemberGroupMembershipFailureTest.php

@@ -0,0 +1,80 @@
+<?php namespace Tests\Unit\Models;
+/**
+ * Copyright 2026 OpenStack Foundation
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+
+use App\Models\Foundation\Main\IGroup;
+use Doctrine\DBAL\Result;
+use Doctrine\DBAL\Statement;
+use Mockery;
+use models\main\Member;
+use Tests\TestCase;
+
+/**
+ * Class MemberGroupMembershipFailureTest
+ *
+ * Member::belongsToGroup() must not cache the answer when the membership query fails: the lookup
+ * answers "not a member" (fail closed) but a transient error cannot stick for the rest of the
+ * request, so the next check runs the query again. No database needed: the raw SQL preparation
+ * is stubbed on a partial mock.
+ *
+ * @package Tests\Unit\Models
+ */
+final class MemberGroupMembershipFailureTest extends TestCase
+{
+    protected function tearDown(): void
+    {
+        Mockery::close();
+        parent::tearDown();
+    }
+
+    private function membershipStatement(int $count): Statement
+    {
+        $result = Mockery::mock(Result::class);
+        $result->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));
+    }
+}