diff --git a/CHANGELOG.md b/CHANGELOG.md index 4981f916..b43d3a61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Unreleased +- Fix decoration of cache adapters inheriting parent service (#525) - Fix extraction of the username of the logged-in user in Symfony 5.3 (#518) ## 4.1.3 (2021-05-31) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 13171eb5..4306d9d9 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -226,9 +226,19 @@ parameters: path: tests/Tracing/Cache/AbstractTraceableCacheAdapterTest.php - - message: "#^Parameter \\#2 \\$decoratedAdapter of class Sentry\\\\SentryBundle\\\\Tracing\\\\Cache\\\\TraceableTagAwareCacheAdapter constructor expects Symfony\\\\Component\\\\Cache\\\\Adapter\\\\TagAwareAdapterInterface, Symfony\\\\Component\\\\Cache\\\\Adapter\\\\AdapterInterface given\\.$#" + message: "#^Parameter \\#1 \\$decoratedAdapter of method Sentry\\\\SentryBundle\\\\Tests\\\\Tracing\\\\Cache\\\\AbstractTraceableCacheAdapterTest\\\\:\\:createCacheAdapter\\(\\) expects TDecoratedCacheAdapter of Symfony\\\\Component\\\\Cache\\\\Adapter\\\\AdapterInterface, PHPUnit\\\\Framework\\\\MockObject\\\\MockObject&Sentry\\\\SentryBundle\\\\Tests\\\\Tracing\\\\Cache\\\\CacheInterface given\\.$#" + count: 2 + path: tests/Tracing/Cache/AbstractTraceableCacheAdapterTest.php + + - + message: "#^Parameter \\#1 \\$decoratedAdapter of method Sentry\\\\SentryBundle\\\\Tests\\\\Tracing\\\\Cache\\\\AbstractTraceableCacheAdapterTest\\\\:\\:createCacheAdapter\\(\\) expects TDecoratedCacheAdapter of Symfony\\\\Component\\\\Cache\\\\Adapter\\\\AdapterInterface, PHPUnit\\\\Framework\\\\MockObject\\\\MockObject&Sentry\\\\SentryBundle\\\\Tests\\\\Tracing\\\\Cache\\\\PruneableCacheAdapterInterface given\\.$#" count: 1 - path: tests/Tracing/Cache/TraceableTagAwareCacheAdapterTest.php + path: tests/Tracing/Cache/AbstractTraceableCacheAdapterTest.php + + - + message: "#^Parameter \\#1 \\$decoratedAdapter of method Sentry\\\\SentryBundle\\\\Tests\\\\Tracing\\\\Cache\\\\AbstractTraceableCacheAdapterTest\\\\:\\:createCacheAdapter\\(\\) expects TDecoratedCacheAdapter of Symfony\\\\Component\\\\Cache\\\\Adapter\\\\AdapterInterface, PHPUnit\\\\Framework\\\\MockObject\\\\MockObject&Sentry\\\\SentryBundle\\\\Tests\\\\Tracing\\\\Cache\\\\ResettableCacheAdapterInterface given\\.$#" + count: 1 + path: tests/Tracing/Cache/AbstractTraceableCacheAdapterTest.php - message: "#^Parameter \\#1 \\$driverException of class Doctrine\\\\DBAL\\\\Exception\\\\DriverException constructor expects Doctrine\\\\DBAL\\\\Driver\\\\Exception, string given\\.$#" diff --git a/src/DependencyInjection/Compiler/CacheTracingPass.php b/src/DependencyInjection/Compiler/CacheTracingPass.php index 4454b2f1..e4f123c3 100644 --- a/src/DependencyInjection/Compiler/CacheTracingPass.php +++ b/src/DependencyInjection/Compiler/CacheTracingPass.php @@ -8,6 +8,7 @@ use Symfony\Component\DependencyInjection\ChildDefinition; use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface; use Symfony\Component\DependencyInjection\ContainerBuilder; +use Symfony\Component\DependencyInjection\Definition; use Symfony\Component\DependencyInjection\Reference; final class CacheTracingPass implements CompilerPassInterface @@ -28,7 +29,13 @@ public function process(ContainerBuilder $container): void continue; } - if (is_subclass_of($cachePoolDefinition->getClass(), TagAwareAdapterInterface::class)) { + $definitionClass = $this->resolveDefinitionClass($container, $cachePoolDefinition); + + if (null === $definitionClass) { + continue; + } + + if (is_subclass_of($definitionClass, TagAwareAdapterInterface::class)) { $traceableCachePoolDefinition = new ChildDefinition('sentry.tracing.traceable_tag_aware_cache_adapter'); } else { $traceableCachePoolDefinition = new ChildDefinition('sentry.tracing.traceable_cache_adapter'); @@ -40,4 +47,16 @@ public function process(ContainerBuilder $container): void $container->setDefinition($serviceId . '.traceable', $traceableCachePoolDefinition); } } + + private function resolveDefinitionClass(ContainerBuilder $container, Definition $definition): ?string + { + $class = $definition->getClass(); + + while (null === $class && $definition instanceof ChildDefinition) { + $definition = $container->findDefinition($definition->getParent()); + $class = $definition->getClass(); + } + + return $class; + } } diff --git a/tests/DependencyInjection/Compiler/CacheTracingPassTest.php b/tests/DependencyInjection/Compiler/CacheTracingPassTest.php index 354fe841..5b380936 100644 --- a/tests/DependencyInjection/Compiler/CacheTracingPassTest.php +++ b/tests/DependencyInjection/Compiler/CacheTracingPassTest.php @@ -11,46 +11,120 @@ use Sentry\State\HubInterface; use Symfony\Component\Cache\Adapter\AdapterInterface; use Symfony\Component\Cache\Adapter\TagAwareAdapterInterface; +use Symfony\Component\DependencyInjection\ChildDefinition; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\DependencyInjection\Definition; use Symfony\Component\DependencyInjection\Reference; final class CacheTracingPassTest extends TestCase { - public function testProcess(): void + /** + * @dataProvider processDataProvider + * + * @param array $definitions + */ + public function testProcess(array $definitions, string $expectedDefinitionClass, string $expectedInnerDefinitionClass): void + { + $container = $this->createContainerBuilder(true); + $container->addDefinitions($definitions); + $container->compile(); + + $cacheTraceableDefinition = $container->findDefinition('app.cache'); + + $this->assertSame($expectedDefinitionClass, $cacheTraceableDefinition->getClass()); + $this->assertInstanceOf(Definition::class, $cacheTraceableDefinition->getArgument(1)); + $this->assertSame($expectedInnerDefinitionClass, $cacheTraceableDefinition->getArgument(1)->getClass()); + } + + /** + * @return \Generator + */ + public function processDataProvider(): \Generator { $cacheAdapter = $this->createMock(AdapterInterface::class); $tagAwareCacheAdapter = $this->createMock(TagAwareAdapterInterface::class); - $container = $this->createContainerBuilder(true); - $container->register('app.cache.foo', \get_class($tagAwareCacheAdapter)) - ->setPublic(true) - ->addTag('cache.pool'); + yield 'Cache pool adapter service' => [ + [ + 'app.cache' => (new Definition(\get_class($cacheAdapter))) + ->setPublic(true) + ->addTag('cache.pool'), + ], + TraceableCacheAdapter::class, + \get_class($cacheAdapter), + ]; + + yield 'Tag-aware cache adapter service' => [ + [ + 'app.cache' => (new Definition(\get_class($tagAwareCacheAdapter))) + ->setPublic(true) + ->addTag('cache.pool'), + ], + TraceableTagAwareCacheAdapter::class, + \get_class($tagAwareCacheAdapter), + ]; + + yield 'Cache pool adapter service inheriting parent service' => [ + [ + 'app.cache.parent' => new Definition(\get_class($cacheAdapter)), + 'app.cache' => (new ChildDefinition('app.cache.parent')) + ->setPublic(true) + ->addTag('cache.pool'), + ], + TraceableCacheAdapter::class, + \get_class($cacheAdapter), + ]; + + yield 'Tag-aware cache pool adapter service inheriting parent service and overriding class' => [ + [ + 'app.cache.parent' => new Definition(\get_class($cacheAdapter)), + 'app.cache' => (new ChildDefinition('app.cache.parent')) + ->setClass(\get_class($tagAwareCacheAdapter)) + ->setPublic(true) + ->addTag('cache.pool'), + ], + TraceableTagAwareCacheAdapter::class, + \get_class($tagAwareCacheAdapter), + ]; + + yield 'Tag-aware cache pool adapter service inheriting multiple parent services' => [ + [ + 'app.cache.parent_1' => new Definition(\get_class($cacheAdapter)), + 'app.cache.parent_2' => (new ChildDefinition('app.cache.parent_1')) + ->setClass(\get_class($tagAwareCacheAdapter)), + 'app.cache' => (new ChildDefinition('app.cache.parent_2')) + ->setPublic(true) + ->addTag('cache.pool'), + ], + TraceableTagAwareCacheAdapter::class, + \get_class($tagAwareCacheAdapter), + ]; + + yield 'Tag-aware cache pool adapter service inheriting parent service' => [ + [ + 'app.cache.parent' => new Definition(\get_class($tagAwareCacheAdapter)), + 'app.cache' => (new ChildDefinition('app.cache.parent')) + ->setPublic(true) + ->addTag('cache.pool'), + ], + TraceableTagAwareCacheAdapter::class, + \get_class($tagAwareCacheAdapter), + ]; + } - $container->register('app.cache.bar', \get_class($cacheAdapter)) - ->setPublic(true) - ->addTag('cache.pool'); + public function testProcessDoesNothingIfCachePoolServiceDefinitionIsAbstract(): void + { + $cacheAdapter = $this->createMock(AdapterInterface::class); + $container = $this->createContainerBuilder(true); - $container->register('app.cache.baz') + $container->register('app.cache', \get_class($cacheAdapter)) ->setPublic(true) ->setAbstract(true) ->addTag('cache.pool'); $container->compile(); - $cacheTraceableDefinition = $container->findDefinition('app.cache.foo'); - - $this->assertSame(TraceableTagAwareCacheAdapter::class, $cacheTraceableDefinition->getClass()); - $this->assertInstanceOf(Definition::class, $cacheTraceableDefinition->getArgument(1)); - $this->assertSame(\get_class($tagAwareCacheAdapter), $cacheTraceableDefinition->getArgument(1)->getClass()); - - $cacheTraceableDefinition = $container->findDefinition('app.cache.bar'); - - $this->assertSame(TraceableCacheAdapter::class, $cacheTraceableDefinition->getClass()); - $this->assertInstanceOf(Definition::class, $cacheTraceableDefinition->getArgument(1)); - $this->assertSame(\get_class($cacheAdapter), $cacheTraceableDefinition->getArgument(1)->getClass()); - - $this->assertFalse($container->hasDefinition('app.cache.baz')); + $this->assertFalse($container->hasDefinition('app.cache')); } public function testProcessDoesNothingIfConditionsForEnablingTracingAreMissing(): void diff --git a/tests/Tracing/Cache/AbstractTraceableCacheAdapterTest.php b/tests/Tracing/Cache/AbstractTraceableCacheAdapterTest.php index b1ce91b5..1cbc3cef 100644 --- a/tests/Tracing/Cache/AbstractTraceableCacheAdapterTest.php +++ b/tests/Tracing/Cache/AbstractTraceableCacheAdapterTest.php @@ -18,6 +18,7 @@ /** * @phpstan-template TCacheAdapter of AdapterInterface + * @phpstan-template TDecoratedCacheAdapter of AdapterInterface */ abstract class AbstractTraceableCacheAdapterTest extends TestCase { @@ -410,12 +411,14 @@ private static function isCachePackageInstalled(): bool } /** + * @phpstan-param TDecoratedCacheAdapter $decoratedAdapter + * * @phpstan-return TCacheAdapter */ - abstract protected function createCacheAdapter(AdapterInterface $decoratedAdapter): AdapterInterface; + abstract protected function createCacheAdapter(AdapterInterface $decoratedAdapter); /** - * @return class-string + * @return class-string */ abstract protected static function getAdapterClassFqcn(): string; } diff --git a/tests/Tracing/Cache/TraceableCacheAdapterTest.php b/tests/Tracing/Cache/TraceableCacheAdapterTest.php index 87a46931..1536adfd 100644 --- a/tests/Tracing/Cache/TraceableCacheAdapterTest.php +++ b/tests/Tracing/Cache/TraceableCacheAdapterTest.php @@ -8,14 +8,14 @@ use Symfony\Component\Cache\Adapter\AdapterInterface; /** - * @phpstan-extends AbstractTraceableCacheAdapterTest + * @phpstan-extends AbstractTraceableCacheAdapterTest */ final class TraceableCacheAdapterTest extends AbstractTraceableCacheAdapterTest { /** * {@inheritdoc} */ - protected function createCacheAdapter(AdapterInterface $decoratedAdapter): AdapterInterface + protected function createCacheAdapter(AdapterInterface $decoratedAdapter): TraceableCacheAdapter { return new TraceableCacheAdapter($this->hub, $decoratedAdapter); } diff --git a/tests/Tracing/Cache/TraceableTagAwareCacheAdapterTest.php b/tests/Tracing/Cache/TraceableTagAwareCacheAdapterTest.php index 539e422c..f5091973 100644 --- a/tests/Tracing/Cache/TraceableTagAwareCacheAdapterTest.php +++ b/tests/Tracing/Cache/TraceableTagAwareCacheAdapterTest.php @@ -5,18 +5,46 @@ namespace Sentry\SentryBundle\Tests\Tracing\Cache; use Sentry\SentryBundle\Tracing\Cache\TraceableTagAwareCacheAdapter; +use Sentry\Tracing\Transaction; +use Sentry\Tracing\TransactionContext; use Symfony\Component\Cache\Adapter\AdapterInterface; use Symfony\Component\Cache\Adapter\TagAwareAdapterInterface; /** - * @phpstan-extends AbstractTraceableCacheAdapterTest + * @phpstan-extends AbstractTraceableCacheAdapterTest */ final class TraceableTagAwareCacheAdapterTest extends AbstractTraceableCacheAdapterTest { + public function testInvalidateTags(): void + { + $transaction = new Transaction(new TransactionContext(), $this->hub); + $transaction->initSpanRecorder(); + + $this->hub->expects($this->once()) + ->method('getSpan') + ->willReturn($transaction); + + $decoratedAdapter = $this->createMock(self::getAdapterClassFqcn()); + $decoratedAdapter->expects($this->once()) + ->method('invalidateTags') + ->with(['foo']) + ->willReturn(true); + + $adapter = $this->createCacheAdapter($decoratedAdapter); + + $this->assertTrue($adapter->invalidateTags(['foo'])); + + $spans = $transaction->getSpanRecorder()->getSpans(); + + $this->assertCount(2, $spans); + $this->assertSame('cache.invalidate_tags', $spans[1]->getOp()); + $this->assertNotNull($spans[1]->getEndTimestamp()); + } + /** * {@inheritdoc} */ - protected function createCacheAdapter(AdapterInterface $decoratedAdapter): AdapterInterface + protected function createCacheAdapter(AdapterInterface $decoratedAdapter): TraceableTagAwareCacheAdapter { return new TraceableTagAwareCacheAdapter($this->hub, $decoratedAdapter); }