From 406a0dca3bc93090271a03fa3ba4cb082067aa91 Mon Sep 17 00:00:00 2001 From: Andrii Dembitskyi Date: Wed, 20 Nov 2024 06:32:15 +0200 Subject: [PATCH] :bug: [#711] Don't reuse cache for differently configured class finder instances (#712) --- .../Cache/HardClassFinderComputedCache.php | 6 +- .../SnapshotClassFinderComputedCache.php | 3 + src/Discovery/ClassFinder.php | 5 ++ src/Discovery/KcsClassFinder.php | 9 ++- src/Discovery/StaticClassFinder.php | 10 +++ src/SchemaFactory.php | 5 +- tests/AbstractQueryProvider.php | 9 ++- .../HardClassFinderComputedCacheTest.php | 59 +++++++++++++-- .../SnapshotClassFinderComputedCacheTest.php | 51 ++++++------- tests/Discovery/KcsClassFinderTest.php | 7 +- .../TestCustomMapperController.php | 21 ++++++ .../Types/TestCustomMapperObject.php | 18 +++++ .../Controllers/TestLegacyController.php | 17 +++++ .../Types/TestLegacyObject.php | 18 +++++ tests/GlobControllerQueryProviderTest.php | 13 +++- tests/Integration/IntegrationTestCase.php | 15 +++- tests/Mappers/ClassFinderTypeMapperTest.php | 73 ++++++++++++++++++- 17 files changed, 285 insertions(+), 54 deletions(-) create mode 100644 tests/Fixtures/ClassFinderCustomTypeMapper/Controllers/TestCustomMapperController.php create mode 100644 tests/Fixtures/ClassFinderCustomTypeMapper/Types/TestCustomMapperObject.php create mode 100644 tests/Fixtures/ClassFinderTypeMapper/Controllers/TestLegacyController.php create mode 100644 tests/Fixtures/ClassFinderTypeMapper/Types/TestLegacyObject.php diff --git a/src/Discovery/Cache/HardClassFinderComputedCache.php b/src/Discovery/Cache/HardClassFinderComputedCache.php index 472aa5a58..70d303a1f 100644 --- a/src/Discovery/Cache/HardClassFinderComputedCache.php +++ b/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) { diff --git a/src/Discovery/Cache/SnapshotClassFinderComputedCache.php b/src/Discovery/Cache/SnapshotClassFinderComputedCache.php index 3e4b9d230..46a80b48d 100644 --- a/src/Discovery/Cache/SnapshotClassFinderComputedCache.php +++ b/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); diff --git a/src/Discovery/ClassFinder.php b/src/Discovery/ClassFinder.php index 67d2cf3b1..92a7cbe42 100644 --- a/src/Discovery/ClassFinder.php +++ b/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; } diff --git a/src/Discovery/KcsClassFinder.php b/src/Discovery/KcsClassFinder.php index d54616b8e..c6939c357 100644 --- a/src/Discovery/KcsClassFinder.php +++ b/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; + } } diff --git a/src/Discovery/StaticClassFinder.php b/src/Discovery/StaticClassFinder.php index 7eedb617b..d04298aa7 100644 --- a/src/Discovery/StaticClassFinder.php +++ b/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 $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)); + } } diff --git a/src/SchemaFactory.php b/src/SchemaFactory.php index 3991341b3..53b43948b 100644 --- a/src/SchemaFactory.php +++ b/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); } } diff --git a/tests/AbstractQueryProvider.php b/tests/AbstractQueryProvider.php index 8e8c09fe7..88ae390f4 100644 --- a/tests/AbstractQueryProvider.php +++ b/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 diff --git a/tests/Discovery/Cache/HardClassFinderComputedCacheTest.php b/tests/Discovery/Cache/HardClassFinderComputedCacheTest.php index 407550a0b..6040d4aab 100644 --- a/tests/Discovery/Cache/HardClassFinderComputedCacheTest.php +++ b/tests/Discovery/Cache/HardClassFinderComputedCacheTest.php @@ -1,9 +1,12 @@ 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); } -} \ No newline at end of file +} diff --git a/tests/Discovery/Cache/SnapshotClassFinderComputedCacheTest.php b/tests/Discovery/Cache/SnapshotClassFinderComputedCacheTest.php index bbf50389d..51be822af 100644 --- a/tests/Discovery/Cache/SnapshotClassFinderComputedCacheTest.php +++ b/tests/Discovery/Cache/SnapshotClassFinderComputedCacheTest.php @@ -1,25 +1,29 @@ setLogger(new ExceptionLogger()); @@ -27,15 +31,16 @@ public function testCachesIndividualEntries(): void $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(); } -} \ No newline at end of file +} diff --git a/tests/Discovery/KcsClassFinderTest.php b/tests/Discovery/KcsClassFinderTest.php index 589308236..5698b0c0c 100644 --- a/tests/Discovery/KcsClassFinderTest.php +++ b/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)); } -} \ No newline at end of file +} diff --git a/tests/Fixtures/ClassFinderCustomTypeMapper/Controllers/TestCustomMapperController.php b/tests/Fixtures/ClassFinderCustomTypeMapper/Controllers/TestCustomMapperController.php new file mode 100644 index 000000000..9aaa339fe --- /dev/null +++ b/tests/Fixtures/ClassFinderCustomTypeMapper/Controllers/TestCustomMapperController.php @@ -0,0 +1,21 @@ +inNamespace('TheCodingMachine\\GraphQLite\\Fixtures'); - $finder->filter(static fn (ReflectionClass $class) => $class->getNamespaceName() === 'TheCodingMachine\\GraphQLite\\Fixtures'); // Fix for recursive:false + $finder->inNamespace($namespace); + $finder->filter(static fn (ReflectionClass $class) => $class->getNamespaceName() === $namespace); // Fix for recursive:false + $hash = md5($namespace); + $globControllerQueryProvider = new GlobControllerQueryProvider( $this->getFieldsBuilder(), $container, $this->getAnnotationReader(), - new KcsClassFinder($finder), - new HardClassFinderComputedCache(new Psr16Cache(new NullAdapter())) + new KcsClassFinder($finder, $hash), + new HardClassFinderComputedCache(new Psr16Cache(new NullAdapter())), ); $queries = $globControllerQueryProvider->getQueries(); diff --git a/tests/Integration/IntegrationTestCase.php b/tests/Integration/IntegrationTestCase.php index 200247f56..1b1aa796b 100644 --- a/tests/Integration/IntegrationTestCase.php +++ b/tests/Integration/IntegrationTestCase.php @@ -95,12 +95,19 @@ public function createContainer(array $overloadedServices = []): ContainerInterf return new Schema($container->get(QueryProviderInterface::class), $container->get(RecursiveTypeMapperInterface::class), $container->get(TypeResolver::class), $container->get(RootTypeMapperInterface::class)); }, ClassFinder::class => function () { + $namespaces = [ + 'TheCodingMachine\\GraphQLite\\Fixtures\\Integration\\Types', + 'TheCodingMachine\\GraphQLite\\Fixtures\\Integration\\Models', + 'TheCodingMachine\\GraphQLite\\Fixtures\\Integration\\Controllers', + ]; + + $hash = md5(implode(',', $namespaces)); $composerFinder = new ComposerFinder(); - $composerFinder->inNamespace('TheCodingMachine\\GraphQLite\\Fixtures\\Integration\\Types'); - $composerFinder->inNamespace('TheCodingMachine\\GraphQLite\\Fixtures\\Integration\\Models'); - $composerFinder->inNamespace('TheCodingMachine\\GraphQLite\\Fixtures\\Integration\\Controllers'); + foreach ($namespaces as $namespace) { + $composerFinder->inNamespace($namespace); + } - return new KcsClassFinder($composerFinder); + return new KcsClassFinder($composerFinder, $hash); }, ClassFinderComputedCache::class => function () { return new HardClassFinderComputedCache( diff --git a/tests/Mappers/ClassFinderTypeMapperTest.php b/tests/Mappers/ClassFinderTypeMapperTest.php index 1657c271c..37f412a8e 100644 --- a/tests/Mappers/ClassFinderTypeMapperTest.php +++ b/tests/Mappers/ClassFinderTypeMapperTest.php @@ -4,19 +4,23 @@ namespace TheCodingMachine\GraphQLite\Mappers; +use GraphQL\Error\DebugFlag; +use GraphQL\GraphQL; use GraphQL\Type\Definition\ObjectType; use GraphQL\Type\Definition\Type; use stdClass; use Symfony\Component\Cache\Adapter\ArrayAdapter; -use Symfony\Component\Cache\Adapter\NullAdapter; use Symfony\Component\Cache\Psr16Cache; use TheCodingMachine\GraphQLite\AbstractQueryProvider; use TheCodingMachine\GraphQLite\AnnotationReader; use TheCodingMachine\GraphQLite\Annotations\Exceptions\ClassNotFoundException; +use TheCodingMachine\GraphQLite\Containers\BasicAutoWiringContainer; +use TheCodingMachine\GraphQLite\Containers\EmptyContainer; use TheCodingMachine\GraphQLite\Containers\LazyContainer; use TheCodingMachine\GraphQLite\FailedResolvingInputType; use TheCodingMachine\GraphQLite\Fixtures\BadExtendType\BadExtendType; use TheCodingMachine\GraphQLite\Fixtures\BadExtendType2\BadExtendType2; +use TheCodingMachine\GraphQLite\Fixtures\ClassFinderCustomTypeMapper\Types\TestCustomMapperObject; use TheCodingMachine\GraphQLite\Fixtures\InheritedInputTypes\ChildTestFactory; use TheCodingMachine\GraphQLite\Fixtures\Integration\Types\FilterDecorator; use TheCodingMachine\GraphQLite\Fixtures\Mocks\MockResolvableInputObjectType; @@ -28,7 +32,9 @@ use TheCodingMachine\GraphQLite\Fixtures\Types\FooType; use TheCodingMachine\GraphQLite\Fixtures\Types\TestFactory; use TheCodingMachine\GraphQLite\GraphQLRuntimeException; +use TheCodingMachine\GraphQLite\Loggers\ExceptionLogger; use TheCodingMachine\GraphQLite\NamingStrategy; +use TheCodingMachine\GraphQLite\SchemaFactory; use TheCodingMachine\GraphQLite\Types\MutableObjectType; class ClassFinderTypeMapperTest extends AbstractQueryProvider @@ -401,4 +407,69 @@ public function testNonInstantiableInput(): void $this->expectExceptionMessage("Class 'TheCodingMachine\GraphQLite\Fixtures\NonInstantiableInput\AbstractFoo' annotated with @Input must be instantiable."); $mapper->mapClassToInputType(AbstractFoo::class); } + + public function testEndToEnd(): void + { + $container = new LazyContainer([ + FooType::class => static function () { + return new FooType(); + }, + FooExtendType::class => static function () { + return new FooExtendType(); + }, + ]); + + $typeGenerator = $this->getTypeGenerator(); + $inputTypeGenerator = $this->getInputTypeGenerator(); + + $classFinderComputedCache = $this->getClassFinderComputedCache(); + + // todo + $arrayAdapter = new ArrayAdapter(); + $arrayAdapter->setLogger(new ExceptionLogger()); + $schemaFactory = new SchemaFactory(new Psr16Cache($arrayAdapter), new BasicAutoWiringContainer(new EmptyContainer())); + $schemaFactory->addNamespace('TheCodingMachine\GraphQLite\Fixtures\ClassFinderTypeMapper\Types'); + $schemaFactory->addNamespace('TheCodingMachine\GraphQLite\Fixtures\ClassFinderTypeMapper\Controllers'); + $schemaFactory->addNamespace('TheCodingMachine\GraphQLite\Fixtures\ClassFinderCustomTypeMapper\Controllers'); + + $classFinder = $this->getClassFinder([ +// 'TheCodingMachine\GraphQLite\Fixtures\ClassFinderCustomTypeMapper\Controllers', + 'TheCodingMachine\GraphQLite\Fixtures\ClassFinderCustomTypeMapper\Types', + ]); + $mapper = new ClassFinderTypeMapper($classFinder, $typeGenerator, $inputTypeGenerator, $this->getInputTypeUtils(), $container, new AnnotationReader(), new NamingStrategy(), $this->getTypeMapper(), $classFinderComputedCache); + + // Register the class finder type mapper in your application using the SchemaFactory instance +// $schemaFactory->addTypeMapper($mapper); + + // ---- + $schemaFactory->addTypeMapperFactory(new StaticClassListTypeMapperFactory([TestCustomMapperObject::class])); + + $schema = $schemaFactory->createSchema(); + + $schema->validate(); + + $queryString = ' + query { + legacyObject { + foo + } + customMapperObject { + foo + } + } + '; + + $result = GraphQL::executeQuery( + $schema, + $queryString, + ); + + $this->assertSame([ + 'data' => [ + 'legacyObject' => ['foo' => 42], + 'customMapperObject' => ['foo' => 42], + ], +// 'legacyObject' => ['foo' => 42], + ], $result->toArray(DebugFlag::RETHROW_INTERNAL_EXCEPTIONS)); + } }