use configurable keys for keeping static connections (#296)

Author: dmaicher

README.md

@@ -108,6 +108,51 @@ dama_doctrine_test:
         connection_a: true
 ```
 
+#### Controlling how connections are kept statically in the current php process
+
+By default, every configured doctrine DBAL connection will have its own driver connection that is managed in the current php process.
+In case you need to customize this behavior you can choose different "connection keys" that are used to select driver connections.
+
+Example for 2 connections that will re-use the same driver connection instance:
+
+```yaml
+doctrine:
+    dbal:
+        connections:
+            default:
+                url: '%database.url1%'
+
+            foo:
+                url: '%database.url2%'
+
+dama_doctrine_test:
+    connection_keys:
+        # assigning the same key will result in the same internal driver connection being re-used for both DBAL connections
+        default: custom_key
+        foo: custom_key
+```
+
+**Since v8.1.0**: For connections with read/write replicas the bundle will use the **same** underlying driver connection by default for the primary and also for replicas. This addresses an [issue](https://github.com/dmaicher/doctrine-test-bundle/issues/289) where inconsistencies happened when reading/writing to different connections. This can also be customized as follows:
+
+```yaml
+doctrine:
+    dbal:
+        connections:
+            default:
+                url: '%database.url%'
+                replicas:
+                    replica_one:
+                        url: '%database.url_replica%'
+
+dama_doctrine_test:
+    connection_keys:
+        # assigning different keys will result in separate internal driver connection being used for primary and replica
+        default:
+            primary: custom_key_primary
+            replicas:
+                replica_one: custom_key_primary_replica
+```
+
 ### Example
 
 An example usage can be seen within the functional tests included in this bundle: https://github.com/dmaicher/doctrine-test-bundle/tree/master/tests

composer.json

@@ -19,7 +19,6 @@
     ],
     "require": {
         "php": "^7.4 || ^8.0",
-        "ext-json": "*",
         "doctrine/dbal": "^3.3 || ^4.0",
         "doctrine/doctrine-bundle": "^2.2.2",
         "psr/cache": "^1.0 || ^2.0 || ^3.0",

makefile

@@ -8,7 +8,8 @@ test_phpunit_legacy: tests/Functional/app/parameters.yml
 	vendor/bin/phpunit -c tests/phpunit.9.xml tests/
 
 phpstan:
-	vendor/bin/phpstan analyse -c phpstan.neon -a vendor/autoload.php -l 5 src tests
+	vendor/bin/phpstan analyse -c phpstan.neon -a vendor/autoload.php -l 8 src
+	vendor/bin/phpstan analyse -c phpstan.neon -a vendor/autoload.php -l 5 tests
 
 behat:
 	vendor/bin/behat -c tests/behat.yml -fprogress

src/DAMA/DoctrineTestBundle/DependencyInjection/Configuration.php

@@ -8,6 +8,7 @@
 class Configuration implements ConfigurationInterface
 {
     public const ENABLE_STATIC_CONNECTION = 'enable_static_connection';
+    public const CONNECTION_KEYS = 'connection_keys';
     public const STATIC_META_CACHE = 'enable_static_meta_data_cache';
     public const STATIC_QUERY_CACHE = 'enable_static_query_cache';
 
@@ -45,9 +46,57 @@ public function getConfigTreeBuilder(): TreeBuilder
                 ->end()
                 ->booleanNode(self::STATIC_META_CACHE)->defaultTrue()->end()
                 ->booleanNode(self::STATIC_QUERY_CACHE)->defaultTrue()->end()
+                ->arrayNode(self::CONNECTION_KEYS)
+                    ->normalizeKeys(false)
+                    ->variablePrototype()
+                    ->end()
+                    ->validate()
+                    ->ifTrue(function ($value) {
+                        if ($value === null) {
+                            return false;
+                        }
+
+                        if (!is_array($value)) {
+                            return true;
+                        }
+
+                        foreach ($value as $k => $v) {
+                            if (!is_string($k) || !(is_string($v) || is_array($v))) {
+                                return true;
+                            }
+
+                            if (!is_array($v)) {
+                                continue;
+                            }
+
+                            if (count($v) !== 2
+                                || !is_string($v['primary'] ?? null)
+                                || !is_array($v['replicas'] ?? null)
+                                || !$this->isAssocStringArray($v['replicas'])
+                            ) {
+                                return true;
+                            }
+                        }
+
+                        return false;
+                    })
+                    ->thenInvalid('Must be array<string, string|array{primary: string, replicas: array<string, string>}>')
+                    ->end()
+                ->end()
             ->end()
         ;
 
         return $treeBuilder;
     }
+
+    private function isAssocStringArray(array $value): bool
+    {
+        foreach ($value as $k => $v) {
+            if (!is_string($k) || !is_string($v)) {
+                return false;
+            }
+        }
+
+        return true;
+    }
 }

src/DAMA/DoctrineTestBundle/DependencyInjection/DAMADoctrineTestExtension.php

@@ -24,5 +24,9 @@ public function load(array $configs, ContainerBuilder $container): void
             'dama.'.Configuration::ENABLE_STATIC_CONNECTION,
             $config[Configuration::ENABLE_STATIC_CONNECTION]
         );
+        $container->setParameter(
+            'dama.'.Configuration::CONNECTION_KEYS,
+            $config[Configuration::CONNECTION_KEYS] ?? [],
+        );
     }
 }

src/DAMA/DoctrineTestBundle/DependencyInjection/DoctrineTestCompilerPass.php

@@ -17,7 +17,6 @@ class DoctrineTestCompilerPass implements CompilerPassInterface
 {
     public function process(ContainerBuilder $container): void
     {
-        $transactionalBehaviorEnabledConnections = $this->getTransactionEnabledConnectionNames($container);
         $container->register('dama.doctrine.dbal.middleware', Middleware::class);
         $cacheNames = [];
 
@@ -29,11 +28,15 @@ public function process(ContainerBuilder $container): void
             $cacheNames[] = 'doctrine.orm.%s_query_cache';
         }
 
-        $connectionNames = array_keys($container->getParameter('doctrine.connections'));
+        /** @var array<string, mixed> $connections */
+        $connections = $container->getParameter('doctrine.connections');
+        $connectionNames = array_keys($connections);
+        $transactionalBehaviorEnabledConnections = $this->getTransactionEnabledConnectionNames($container, $connectionNames);
+        $connectionKeys = $this->getConnectionKeys($container, $connectionNames);
 
         foreach ($connectionNames as $name) {
             if (in_array($name, $transactionalBehaviorEnabledConnections, true)) {
-                $this->modifyConnectionService($container, $name);
+                $this->modifyConnectionService($container, $connectionKeys[$name] ?? null, $name);
             }
 
             foreach ($cacheNames as $cacheName) {
@@ -56,19 +59,25 @@ public function process(ContainerBuilder $container): void
         $container->getParameterBag()->remove('dama.'.Configuration::ENABLE_STATIC_CONNECTION);
         $container->getParameterBag()->remove('dama.'.Configuration::STATIC_META_CACHE);
         $container->getParameterBag()->remove('dama.'.Configuration::STATIC_QUERY_CACHE);
+        $container->getParameterBag()->remove('dama.'.Configuration::CONNECTION_KEYS);
     }
 
-    private function modifyConnectionService(ContainerBuilder $container, string $name): void
+    /**
+     * @param string|array{primary: string, replicas: array<string, string>}|null $connectionKey
+     */
+    private function modifyConnectionService(ContainerBuilder $container, $connectionKey, string $name): void
     {
         $connectionDefinition = $container->getDefinition(sprintf('doctrine.dbal.%s_connection', $name));
 
         if (!$this->hasSavepointsEnabled($connectionDefinition)) {
             throw new \LogicException(sprintf('This bundle relies on savepoints for nested database transactions. You need to enable "use_savepoints" on the Doctrine DBAL config for connection "%s".', $name));
         }
 
+        /** @var array<string, mixed> $connectionOptions */
+        $connectionOptions = $connectionDefinition->getArgument(0);
         $connectionDefinition->replaceArgument(
             0,
-            $this->getModifiedConnectionOptions($connectionDefinition->getArgument(0), $name),
+            $this->getModifiedConnectionOptions($connectionOptions, $connectionKey, $name),
         );
 
         $connectionConfig = $container->getDefinition(sprintf('doctrine.dbal.%s_connection.configuration', $name));
@@ -90,27 +99,44 @@ private function modifyConnectionService(ContainerBuilder $container, string $na
         $connectionConfig->setMethodCalls($methodCalls);
     }
 
-    private function getModifiedConnectionOptions(array $options, string $name): array
-    {
-        $connectionOptions = array_merge($options, [
-            'dama.keep_static' => true,
-            'dama.connection_name' => $name,
-        ]);
-
-        if (isset($connectionOptions['primary'])) {
-            $connectionOptions['primary'] = array_merge($connectionOptions['primary'], [
-                'dama.keep_static' => true,
-                'dama.connection_name' => $name,
-            ]);
-        }
-
-        if (isset($connectionOptions['replica']) && is_array($connectionOptions['replica'])) {
-            foreach ($connectionOptions['replica'] as $replicaName => &$replica) {
-                $replica = array_merge($replica, [
-                    'dama.keep_static' => true,
-                    'dama.connection_name' => sprintf('%s.%s', $name, $replicaName),
-                ]);
+    /**
+     * @param array<string, mixed>                                                $connectionOptions
+     * @param string|array{primary: string, replicas: array<string, string>}|null $connectionKey
+     *
+     * @return array<string, mixed>
+     */
+    private function getModifiedConnectionOptions(
+        array $connectionOptions,
+        $connectionKey,
+        string $name
+    ): array {
+        if (!isset($connectionOptions['primary'])) {
+            if (is_array($connectionKey)) {
+                throw new \InvalidArgumentException(sprintf('Connection key for connection "%s" must be a string', $name));
             }
+
+            $connectionOptions['dama.connection_key'] = $connectionKey ?? $name;
+
+            return $connectionOptions;
+        }
+
+        $connectionOptions['dama.connection_key'] = $connectionKey['primary'] ?? $connectionKey ?? $name;
+        $connectionOptions['primary']['dama.connection_key'] = $connectionOptions['dama.connection_key'];
+
+        if (!is_array($connectionOptions['replica'] ?? null)) {
+            return $connectionOptions;
+        }
+
+        $replicaKeys = [];
+        if (isset($connectionKey['replicas'])) {
+            /** @var array<string> $definedReplicaNames */
+            $definedReplicaNames = array_keys($connectionOptions['replica']);
+            $this->validateConnectionNames(array_keys($connectionKey['replicas']), $definedReplicaNames);
+            $replicaKeys = $connectionKey['replicas'];
+        }
+
+        foreach ($connectionOptions['replica'] as $replicaName => &$replicaOptions) {
+            $replicaOptions['dama.connection_key'] = $replicaKeys[$replicaName] ?? $connectionOptions['dama.connection_key'];
         }
 
         return $connectionOptions;
@@ -123,11 +149,12 @@ private function registerStaticCache(
     ): void {
         $cache = new Definition();
         $namespace = sha1($cacheServiceId);
+        $originalServiceClass = $originalCacheServiceDefinition->getClass();
 
-        if (is_a($originalCacheServiceDefinition->getClass(), CacheItemPoolInterface::class, true)) {
+        if ($originalServiceClass !== null && is_a($originalServiceClass, CacheItemPoolInterface::class, true)) {
             $cache->setClass(Psr6StaticArrayCache::class);
             $cache->setArgument(0, $namespace); // make sure we have no key collisions
-        } elseif (is_a($originalCacheServiceDefinition->getClass(), Cache::class, true)) {
+        } elseif ($originalServiceClass !== null && is_a($originalServiceClass, Cache::class, true)) {
             throw new \InvalidArgumentException(sprintf('Configuring "%s" caches is not supported anymore. Upgrade to PSR-6 caches instead.', Cache::class));
         } else {
             throw new \InvalidArgumentException(sprintf('Unsupported cache class "%s" found on service "%s".', $originalCacheServiceDefinition->getClass(), $cacheServiceId));
@@ -140,14 +167,15 @@ private function registerStaticCache(
     }
 
     /**
+     * @param string[] $connectionNames
+     *
      * @return string[]
      */
-    private function getTransactionEnabledConnectionNames(ContainerBuilder $container): array
+    private function getTransactionEnabledConnectionNames(ContainerBuilder $container, array $connectionNames): array
     {
-        /** @var bool|array $enableStaticConnectionsConfig */
+        /** @var bool|array<string, bool> $enableStaticConnectionsConfig */
         $enableStaticConnectionsConfig = $container->getParameter('dama.'.Configuration::ENABLE_STATIC_CONNECTION);
 
-        $connectionNames = array_keys($container->getParameter('doctrine.connections'));
         if (is_array($enableStaticConnectionsConfig)) {
             $this->validateConnectionNames(array_keys($enableStaticConnectionsConfig), $connectionNames);
         }
@@ -193,4 +221,18 @@ private function hasSavepointsEnabled(Definition $connectionDefinition): bool
 
         return false;
     }
+
+    /**
+     * @param string[] $connectionNames
+     *
+     * @return array<string, string|array{primary: string, replicas: array<string, string>}>
+     */
+    private function getConnectionKeys(ContainerBuilder $container, array $connectionNames): array
+    {
+        /** @var array<string, string> $connectionKeys */
+        $connectionKeys = $container->getParameter('dama.'.Configuration::CONNECTION_KEYS);
+        $this->validateConnectionNames(array_keys($connectionKeys), $connectionNames);
+
+        return $connectionKeys;
+    }
 }

src/DAMA/DoctrineTestBundle/Doctrine/Cache/Psr6StaticArrayCache.php

@@ -39,6 +39,9 @@ public function getItem($key): CacheItemInterface
         return $this->adapter->getItem($key);
     }
 
+    /**
+     * @return iterable<CacheItemInterface>
+     */
     public function getItems(array $keys = []): iterable
     {
         return $this->adapter->getItems($keys);

src/DAMA/DoctrineTestBundle/Doctrine/DBAL/StaticDriver.php

@@ -10,7 +10,7 @@
 class StaticDriver extends Driver\Middleware\AbstractDriverMiddleware
 {
     /**
-     * @var Connection[]
+     * @var array<string, Connection>
      */
     private static $connections = [];
 
@@ -21,14 +21,12 @@ class StaticDriver extends Driver\Middleware\AbstractDriverMiddleware
 
     public function connect(array $params): Connection
     {
-        if (!self::isKeepStaticConnections()
-            || !isset($params['dama.keep_static'])
-            || !$params['dama.keep_static']
-        ) {
+        if (!self::isKeepStaticConnections() || !isset($params['dama.connection_key'])) {
             return parent::connect($params);
         }
 
-        $key = sha1(json_encode($params));
+        /** @var string $key */
+        $key = $params['dama.connection_key'];
 
         if (!isset(self::$connections[$key])) {
             self::$connections[$key] = parent::connect($params);