🐛 [#711] Don't reuse cache for differently configured class finder instances (#712)

Author: andrew-demb

src/Discovery/Cache/HardClassFinderComputedCache.php

@@ -8,12 +8,13 @@
 use ReflectionClass;
 use TheCodingMachine\GraphQLite\Discovery\ClassFinder;
 
+use function sprintf;
+
 class HardClassFinderComputedCache implements ClassFinderComputedCache
 {
     public function __construct(
         private readonly CacheInterface $cache,
-    )
-    {
+    ) {
     }
 
     /**
@@ -32,6 +33,7 @@ public function compute(
         callable $reduce,
     ): mixed
     {
+        $key = sprintf('%s.%s', $key, $classFinder->hash());
         $result = $this->cache->get($key);
 
         if ($result !== null) {

src/Discovery/Cache/SnapshotClassFinderComputedCache.php

@@ -9,6 +9,8 @@
 use TheCodingMachine\GraphQLite\Cache\FilesSnapshot;
 use TheCodingMachine\GraphQLite\Discovery\ClassFinder;
 
+use function sprintf;
+
 /**
  * Provides cache for a {@see ClassFinder} based on a {@see filemtime()}.
  *
@@ -49,6 +51,7 @@ public function compute(
         callable $reduce,
     ): mixed
     {
+        $key = sprintf('%s.%s', $key, $classFinder->hash());
         $entries = $this->entries($classFinder, $key . '.entries', $map);
 
         return $reduce($entries);

src/Discovery/ClassFinder.php

@@ -11,4 +11,9 @@
 interface ClassFinder extends IteratorAggregate
 {
     public function withPathFilter(callable $filter): self;
+
+    /**
+     * Path filter does not affect the hash.
+     */
+    public function hash(): string;
 }

src/Discovery/KcsClassFinder.php

@@ -12,8 +12,8 @@ class KcsClassFinder implements ClassFinder
 {
     public function __construct(
         private FinderInterface $finder,
-    )
-    {
+        private readonly string $hash,
+    ) {
     }
 
     public function withPathFilter(callable $filter): ClassFinder
@@ -29,4 +29,9 @@ public function getIterator(): Traversable
     {
         return $this->finder->getIterator();
     }
+
+    public function hash(): string
+    {
+        return $this->hash;
+    }
 }

src/Discovery/StaticClassFinder.php

@@ -7,11 +7,16 @@
 use ReflectionClass;
 use Traversable;
 
+use function md5;
+use function serialize;
+
 class StaticClassFinder implements ClassFinder
 {
     /** @var (callable(string): bool)|null  */
     private mixed $pathFilter = null;
 
+    private string|null $hash = null;
+
     /** @param list<class-string> $classes */
     public function __construct(
         private readonly array $classes,
@@ -41,4 +46,9 @@ public function getIterator(): Traversable
             yield $class => $classReflection;
         }
     }
+
+    public function hash(): string
+    {
+        return $this->hash ??= md5(serialize($this->classes));
+    }
 }

src/SchemaFactory.php

@@ -70,6 +70,7 @@
 
 use function array_reverse;
 use function class_exists;
+use function implode;
 use function md5;
 use function substr;
 use function trigger_error;
@@ -565,6 +566,8 @@ private function createClassFinder(): ClassFinder
             $finder = $finder->inNamespace($namespace);
         }
 
-        return new KcsClassFinder($finder);
+        $hash = md5(implode(',', $this->namespaces));
+
+        return new KcsClassFinder($finder, $hash);
     }
 }

tests/AbstractQueryProvider.php

@@ -29,8 +29,8 @@
 use TheCodingMachine\GraphQLite\Discovery\Cache\ClassFinderComputedCache;
 use TheCodingMachine\GraphQLite\Discovery\Cache\HardClassFinderComputedCache;
 use TheCodingMachine\GraphQLite\Discovery\ClassFinder;
-use TheCodingMachine\GraphQLite\Discovery\StaticClassFinder;
 use TheCodingMachine\GraphQLite\Discovery\KcsClassFinder;
+use TheCodingMachine\GraphQLite\Discovery\StaticClassFinder;
 use TheCodingMachine\GraphQLite\Fixtures\Mocks\MockResolvableInputObjectType;
 use TheCodingMachine\GraphQLite\Fixtures\TestObject;
 use TheCodingMachine\GraphQLite\Fixtures\TestObject2;
@@ -67,6 +67,9 @@
 use TheCodingMachine\GraphQLite\Types\ResolvableMutableInputInterface;
 use TheCodingMachine\GraphQLite\Types\TypeResolver;
 
+use function implode;
+use function md5;
+
 abstract class AbstractQueryProvider extends TestCase
 {
     private $testObjectType;
@@ -481,7 +484,9 @@ protected function getClassFinder(array|string $namespaces): ClassFinder
 
         $finder = $finder->withFileFinder(new CachedFileFinder(new DefaultFileFinder(), $arrayAdapter));
 
-        return new KcsClassFinder($finder);
+        $hash = md5(implode(',', (array) $namespaces));
+
+        return new KcsClassFinder($finder, $hash);
     }
 
     protected function getClassFinderComputedCache(): ClassFinderComputedCache

tests/Discovery/Cache/HardClassFinderComputedCacheTest.php

@@ -1,9 +1,12 @@
 <?php
 
+declare(strict_types=1);
+
 namespace TheCodingMachine\GraphQLite\Discovery\Cache;
 
 use PHPUnit\Framework\Attributes\CoversClass;
 use PHPUnit\Framework\TestCase;
+use ReflectionClass;
 use Symfony\Component\Cache\Adapter\ArrayAdapter;
 use Symfony\Component\Cache\Psr16Cache;
 use TheCodingMachine\GraphQLite\Discovery\StaticClassFinder;
@@ -12,10 +15,12 @@
 use TheCodingMachine\GraphQLite\Fixtures\Types\FooType;
 use TheCodingMachine\GraphQLite\Loggers\ExceptionLogger;
 
+use function array_values;
+
 #[CoversClass(HardClassFinderComputedCache::class)]
 class HardClassFinderComputedCacheTest extends TestCase
 {
-    public function testCachesResultOfReduceFunction(): void
+    public function testNotReusedCacheBecauseDifferentList(): void
     {
         $arrayAdapter = new ArrayAdapter();
         $arrayAdapter->setLogger(new ExceptionLogger());
@@ -30,8 +35,46 @@ public function testCachesResultOfReduceFunction(): void
                 TestType::class,
             ]),
             'key',
-            fn (\ReflectionClass $reflection) => $reflection->getShortName(),
-            fn (array $entries) => [array_values($entries)],
+            static fn (ReflectionClass $reflection) => $reflection->getShortName(),
+            static fn (array $entries) => [array_values($entries)],
+        );
+
+        $this->assertSame([
+            'FooType',
+            'FooExtendType',
+            'TestType',
+        ], $result);
+
+        // Class finder have different class list - result should not be reused from the cache.
+        // This is necessary to avoid caching issues when there're multiple class finders shares the same cache pool.
+        [$result] = $classFinderComputedCache->compute(
+            new StaticClassFinder([FooType::class]),
+            'key',
+            static fn (ReflectionClass $reflection) => $reflection->getShortName(),
+            static fn (array $entries) => [array_values($entries)],
+        );
+
+        $this->assertSame(['FooType'], $result);
+    }
+
+    public function testReusedCacheBecauseSameList(): void
+    {
+        $arrayAdapter = new ArrayAdapter();
+        $arrayAdapter->setLogger(new ExceptionLogger());
+        $cache = new Psr16Cache($arrayAdapter);
+
+        $classFinderComputedCache = new HardClassFinderComputedCache($cache);
+
+        $classList = [
+            FooType::class,
+            FooExtendType::class,
+            TestType::class,
+        ];
+        [$result] = $classFinderComputedCache->compute(
+            new StaticClassFinder($classList),
+            'key',
+            static fn (ReflectionClass $reflection) => $reflection->getShortName(),
+            static fn (array $entries) => [array_values($entries)],
         );
 
         $this->assertSame([
@@ -40,13 +83,13 @@ public function testCachesResultOfReduceFunction(): void
             'TestType',
         ], $result);
 
-        // Even though the class finder and both functions have changed - the result should still be cached.
+        // Class finder have the same class list - even both functions have changed - the result should be cached.
         // This is useful in production, where code and file structure doesn't change.
         [$result] = $classFinderComputedCache->compute(
-            new StaticClassFinder([]),
+            new StaticClassFinder($classList),
             'key',
-            fn (\ReflectionClass $reflection) => self::fail('Should not be called.'),
-            fn (array $entries) => self::fail('Should not be called.'),
+            static fn (ReflectionClass $reflection) => self::fail('Should not be called.'),
+            static fn (array $entries) => self::fail('Should not be called.'),
         );
 
         $this->assertSame([
@@ -55,4 +98,4 @@ public function testCachesResultOfReduceFunction(): void
             'TestType',
         ], $result);
     }
-}
+}

tests/Discovery/Cache/SnapshotClassFinderComputedCacheTest.php

@@ -1,41 +1,46 @@
 <?php
 
+declare(strict_types=1);
+
 namespace TheCodingMachine\GraphQLite\Discovery\Cache;
 
 use PHPUnit\Framework\Attributes\CoversClass;
 use PHPUnit\Framework\TestCase;
+use ReflectionClass;
 use Symfony\Component\Cache\Adapter\ArrayAdapter;
 use Symfony\Component\Cache\Psr16Cache;
 use TheCodingMachine\GraphQLite\Discovery\StaticClassFinder;
 use TheCodingMachine\GraphQLite\Fixtures\TestType;
-use TheCodingMachine\GraphQLite\Fixtures\Types\EnumType;
 use TheCodingMachine\GraphQLite\Fixtures\Types\FooExtendType;
 use TheCodingMachine\GraphQLite\Fixtures\Types\FooType;
 use TheCodingMachine\GraphQLite\Loggers\ExceptionLogger;
 
-use function Safe\touch;
+use function array_values;
+use function clearstatcache;
 use function Safe\filemtime;
+use function Safe\touch;
 
 #[CoversClass(SnapshotClassFinderComputedCache::class)]
 class SnapshotClassFinderComputedCacheTest extends TestCase
 {
-    public function testCachesIndividualEntries(): void
+    public function testCachesIndividualEntriesSameList(): void
     {
         $arrayAdapter = new ArrayAdapter();
         $arrayAdapter->setLogger(new ExceptionLogger());
         $cache = new Psr16Cache($arrayAdapter);
 
         $classFinderComputedCache = new SnapshotClassFinderComputedCache($cache);
 
+        $classList = [
+            FooType::class,
+            FooExtendType::class,
+            TestType::class,
+        ];
         [$result] = $classFinderComputedCache->compute(
-            new StaticClassFinder([
-                FooType::class,
-                FooExtendType::class,
-                TestType::class,
-            ]),
+            new StaticClassFinder($classList),
             'key',
-            fn (\ReflectionClass $reflection) => $reflection->getShortName(),
-            fn (array $entries) => [array_values($entries)],
+            static fn (ReflectionClass $reflection) => $reflection->getShortName(),
+            static fn (array $entries) => [array_values($entries)],
         );
 
         $this->assertSame([
@@ -45,14 +50,10 @@ public function testCachesIndividualEntries(): void
         ], $result);
 
         [$result] = $classFinderComputedCache->compute(
-            new StaticClassFinder([
-                FooType::class,
-                FooExtendType::class,
-                TestType::class,
-            ]),
+            new StaticClassFinder($classList),
             'key',
-            fn (\ReflectionClass $reflection) => self::fail('Should not be called.'),
-            fn (array $entries) => [array_values($entries)],
+            static fn (ReflectionClass $reflection) => self::fail('Should not be called.'),
+            static fn (array $entries) => [array_values($entries)],
         );
 
         $this->assertSame([
@@ -61,23 +62,19 @@ public function testCachesIndividualEntries(): void
             'TestType',
         ], $result);
 
-        $this->touch((new \ReflectionClass(FooType::class))->getFileName());
+        $this->touch((new ReflectionClass(FooType::class))->getFileName());
 
         [$result] = $classFinderComputedCache->compute(
-            new StaticClassFinder([
-                FooType::class,
-                TestType::class,
-                EnumType::class,
-            ]),
+            new StaticClassFinder($classList),
             'key',
-            fn (\ReflectionClass $reflection) => $reflection->getShortName() . ' Modified',
-            fn (array $entries) => [array_values($entries)],
+            static fn (ReflectionClass $reflection) => $reflection->getShortName() . ' Modified',
+            static fn (array $entries) => [array_values($entries)],
         );
 
         $this->assertSame([
             'FooType Modified',
+            'FooExtendType',
             'TestType',
-            'EnumType Modified',
         ], $result);
     }
 
@@ -86,4 +83,4 @@ private function touch(string $fileName): void
         touch($fileName, filemtime($fileName) + 1);
         clearstatcache();
     }
-}
+}

tests/Discovery/KcsClassFinderTest.php

@@ -20,9 +20,10 @@ class KcsClassFinderTest extends TestCase
 {
     public function testYieldsGivenClasses(): void
     {
+        $namespaces = 'TheCodingMachine\GraphQLite\Fixtures\Types';
         $finder = new KcsClassFinder(
-            (new ComposerFinder())
-                ->inNamespace('TheCodingMachine\GraphQLite\Fixtures\Types')
+            (new ComposerFinder())->inNamespace($namespaces),
+            md5($namespaces)
         );
 
         $finderWithPath = $finder->withPathFilter(fn (string $path) => str_contains($path, 'FooExtendType.php'));
@@ -50,4 +51,4 @@ private function assertFoundClasses(array $expectedClasses, ClassFinder $classFi
         $this->assertContainsOnlyInstancesOf(\ReflectionClass::class, $result);
         $this->assertEqualsCanonicalizing($expectedClasses, array_keys($result));
     }
-}
+}