Cache the dependent/suggester counts in Redis

.env.test

@@ -4,6 +4,7 @@ APP_SECRET='$ecretf0rt3st'
 DATABASE_URL="mysql://root@127.0.0.1:3306/packagist?serverVersion=8.4.7"
 MAILER_DSN=null://null
 REDIS_URL=redis://localhost/14
+REDIS_CACHE_URL=redis://localhost/15
 APP_MAILER_FROM_EMAIL=packagist@example.org
 APP_HOSTNAME=packagist.wip
 DEFAULT_URI=http://packagist.wip

phpstan-baseline.neon

@@ -54,12 +54,6 @@ parameters:
 			count: 1
 			path: src/Entity/AuditRecordRepository.php
 
-		-
-			message: '#^Method App\\Entity\\PackageRepository\:\:getSuggestCount\(\) should return int\<0, max\> but returns int\.$#'
-			identifier: return.type
-			count: 1
-			path: src/Entity/PackageRepository.php
-
 		-
 			message: '#^Query error\: Unknown column ''d\.total'' in ''order clause'' \(1054\)\.$#'
 			identifier: dba.syntaxError

src/Entity/PackageRepository.php

@@ -20,6 +20,7 @@
 use Doctrine\ORM\Query;
 use Doctrine\ORM\QueryBuilder;
 use Doctrine\Persistence\ManagerRegistry;
+use Predis\Client;
 
 /**
  * @author Jordi Boggiano <j.boggiano@seld.be>
@@ -32,8 +33,10 @@ class PackageRepository extends ServiceEntityRepository
     // @phpstan-ignore classConstant.unused
     private const LISTING_WITH_AUTO_UPDATE_WARNINGS_FIELDS = 'id, name, description, type, gitHubStars, frozen, language, abandoned, replacementPackage, autoUpdated, repository';
 
-    public function __construct(ManagerRegistry $registry)
-    {
+    public function __construct(
+        ManagerRegistry $registry,
+        private Client $redisCache,
+    ) {
         parent::__construct($registry, Package::class);
     }
 
@@ -627,14 +630,16 @@ public function getReadmeContentsByPackageIds(array $ids): array
      */
     public function getDependentCount(string $name, ?int $type = null): int
     {
-        $sql = 'SELECT COUNT(*) count FROM dependent WHERE packageName = :name';
-        $args = ['name' => $name];
-        if (null !== $type) {
-            $sql .= ' AND type = :type';
-            $args['type'] = $type;
-        }
+        return $this->getCachedCount('dep-count:'.strtolower($name).':'.($type ?? 'all'), function () use ($name, $type): int {
+            $sql = 'SELECT COUNT(*) count FROM dependent WHERE packageName = :name';
+            $args = ['name' => $name];
+            if (null !== $type) {
+                $sql .= ' AND type = :type';
+                $args['type'] = $type;
+            }
 
-        return max(0, (int) $this->getEntityManager()->getConnection()->fetchOne($sql, $args));
+            return (int) $this->getEntityManager()->getConnection()->fetchOne($sql, $args);
+        });
     }
 
     /**
@@ -709,10 +714,37 @@ public function getDefaultBranchRequireFor(array $requirers, string $requiree):
      */
     public function getSuggestCount(string $name): int
     {
-        $sql = 'SELECT COUNT(*) count FROM suggester WHERE packageName = :name';
-        $args = ['name' => $name];
+        return $this->getCachedCount('sug-count:'.strtolower($name), function () use ($name): int {
+            $sql = 'SELECT COUNT(*) count FROM suggester WHERE packageName = :name';
+
+            return (int) $this->getEntityManager()->getConnection()->fetchOne($sql, ['name' => $name]);
+        });
+    }
+
+    /**
+     * Both counts are rendered as tab labels on every package page view, where the COUNT(*) is a
+     * large index scan for widely-required packages like psr/log. Being an hour out of date on a
+     * badge is harmless, so this is TTL-only with no explicit invalidation.
+     *
+     * Keys are lowercased because the packageName columns use a case-insensitive collation, so
+     * differently-cased requests must not get separate entries.
+     *
+     * @param callable(): int $compute
+     *
+     * @return int<0, max>
+     */
+    private function getCachedCount(string $cacheKey, callable $compute): int
+    {
+        $cached = $this->redisCache->get($cacheKey);
+        if ($cached !== null) {
+            return max(0, (int) $cached);
+        }
+
+        $count = max(0, $compute());
+        // random variance spreads out the refresh of the most-requested packages
+        $this->redisCache->setex($cacheKey, 3600 + random_int(0, 600), (string) $count);
 
-        return (int) $this->getEntityManager()->getConnection()->fetchOne($sql, $args);
+        return $count;
     }
 
     /**

tests/Entity/PackageRepositoryTest.php

@@ -12,10 +12,13 @@
 
 namespace App\Tests\Entity;
 
+use App\Entity\Dependent;
 use App\Entity\Package;
 use App\Entity\PackageFreezeReason;
 use App\Entity\PackageRepository;
+use App\Entity\Suggester;
 use App\Tests\IntegrationTestCase;
+use Predis\Client;
 
 class PackageRepositoryTest extends IntegrationTestCase
 {
@@ -197,4 +200,63 @@ public function testGetFilteredQueryBuilderExcludesSuppressedByDefault(): void
         self::assertContains('vendor/spam', $withFrozen);
         self::assertContains('vendor/malware', $withFrozen);
     }
+
+    public function testGetDependentCountIsCachedPerType(): void
+    {
+        $requirer = self::createPackage('test/requirer', 'https://example.org/requirer');
+        $devRequirer = self::createPackage('test/dev-requirer', 'https://example.org/dev-requirer');
+        $this->store($requirer, $devRequirer);
+        $this->store(
+            new Dependent($requirer, 'test/required', Dependent::TYPE_REQUIRE),
+            new Dependent($devRequirer, 'test/required', Dependent::TYPE_REQUIRE_DEV),
+        );
+
+        self::assertSame(2, $this->packageRepository->getDependentCount('test/required'));
+        self::assertSame(1, $this->packageRepository->getDependentCount('test/required', Dependent::TYPE_REQUIRE));
+        self::assertSame(1, $this->packageRepository->getDependentCount('test/required', Dependent::TYPE_REQUIRE_DEV));
+
+        // each variant gets its own key, so the type filter cannot be served from the unfiltered count
+        self::assertSame('2', $this->redisCache()->get('dep-count:test/required:all'));
+        self::assertSame('1', $this->redisCache()->get('dep-count:test/required:'.Dependent::TYPE_REQUIRE));
+        self::assertSame('1', $this->redisCache()->get('dep-count:test/required:'.Dependent::TYPE_REQUIRE_DEV));
+    }
+
+    public function testGetDependentCountReadsTheCacheAndIgnoresNameCasing(): void
+    {
+        $this->redisCache()->set('dep-count:test/required:all', '42');
+
+        self::assertSame(42, $this->packageRepository->getDependentCount('test/required'));
+        // packageName uses a case-insensitive collation, so casing must not produce a second entry
+        self::assertSame(42, $this->packageRepository->getDependentCount('Test/Required'));
+    }
+
+    public function testGetSuggestCountIsCached(): void
+    {
+        $suggester = self::createPackage('test/suggester', 'https://example.org/suggester');
+        $this->store($suggester);
+        $this->store(new Suggester($suggester, 'test/suggested'));
+
+        self::assertSame(1, $this->packageRepository->getSuggestCount('test/suggested'));
+        self::assertSame('1', $this->redisCache()->get('sug-count:test/suggested'));
+
+        $this->redisCache()->set('sug-count:test/suggested', '7');
+        self::assertSame(7, $this->packageRepository->getSuggestCount('test/suggested'));
+    }
+
+    public function testCountsCacheZeroSoUnknownPackagesDoNotRequeryEveryPageView(): void
+    {
+        self::assertSame(0, $this->packageRepository->getDependentCount('test/nothing-requires-this'));
+        self::assertSame(0, $this->packageRepository->getSuggestCount('test/nothing-requires-this'));
+
+        self::assertSame('0', $this->redisCache()->get('dep-count:test/nothing-requires-this:all'));
+        self::assertSame('0', $this->redisCache()->get('sug-count:test/nothing-requires-this'));
+    }
+
+    private function redisCache(): Client
+    {
+        $client = static::getContainer()->get('snc_redis.cache');
+        self::assertInstanceOf(Client::class, $client);
+
+        return $client;
+    }
 }

tests/IntegrationTestCase.php

@@ -31,6 +31,11 @@ protected function setUp(): void
         $this->client = self::createClient();
         $this->client->disableReboot(); // prevent reboot to keep the transaction
 
+        // The DB is rolled back per test but Redis is not, so cached values keyed by package name
+        // would leak into later tests that reuse a name. The cache client has its own DB in the
+        // test env (REDIS_CACHE_URL) so this cannot clear state the default client owns.
+        static::getContainer()->get('snc_redis.cache')->flushdb();
+
         static::getService(Connection::class)->beginTransaction();
 
         parent::setUp();