Fix decoration of cache adapters inheriting parent service (#525)

* Fix decoration of cache adapters inheriting parent service * Fix CR issues
getsentry · Jun 18, 2021 · 551b0b6 · 551b0b6
1 parent 5f5fe82
commit 551b0b6
Show file tree

Hide file tree

Showing 7 changed files with 166 additions and 31 deletions.
diff --git 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
@@ -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\\<TCacheAdapter of Symfony\\\\Component\\\\Cache\\\\Adapter\\\\AdapterInterface,TDecoratedCacheAdapter of Symfony\\\\Component\\\\Cache\\\\Adapter\\\\AdapterInterface\\>\\:\\: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\\<TCacheAdapter of Symfony\\\\Component\\\\Cache\\\\Adapter\\\\AdapterInterface,TDecoratedCacheAdapter of Symfony\\\\Component\\\\Cache\\\\Adapter\\\\AdapterInterface\\>\\:\\: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\\<TCacheAdapter of Symfony\\\\Component\\\\Cache\\\\Adapter\\\\AdapterInterface,TDecoratedCacheAdapter of Symfony\\\\Component\\\\Cache\\\\Adapter\\\\AdapterInterface\\>\\:\\: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
@@ -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
@@ -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<string, Definition> $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<mixed>
+     */
+    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
@@ -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<AdapterInterface>
+     * @return class-string<TDecoratedCacheAdapter>
      */
     abstract protected static function getAdapterClassFqcn(): string;
 }

diff --git 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<TraceableCacheAdapter>
+ * @phpstan-extends AbstractTraceableCacheAdapterTest<TraceableCacheAdapter, AdapterInterface>
  */
 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
@@ -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<TraceableTagAwareCacheAdapter>
+ * @phpstan-extends AbstractTraceableCacheAdapterTest<TraceableTagAwareCacheAdapter, TagAwareAdapterInterface>
  */
 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);
     }