Merge pull request #32 from mathroc/refactoring/unwrap-connections

avoid having nested Doctrine\DBAL\Connection-s
daryl-software · Dec 2, 2015 · e136876 · e136876
2 parents e5120b3 + 9455fc1
commit e136876
Show file tree

Hide file tree

Showing 15 changed files with 102 additions and 166 deletions.
diff --git a/composer.json b/composer.json
@@ -13,15 +13,9 @@
         }
     },
     "require": {
-        "doctrine/dbal": "dev-avoid-over-nesting-exceptions as 2.5.x-dev"
+        "doctrine/dbal": "^2.5.2"
     },
     "require-dev": {
         "behat/behat": "~3.0"
-    },
-    "repositories": [
-        {
-            "type": "vcs",
-            "url": "https://github.com/mathroc/dbal"
-        }
-    ]
+    }
 }
diff --git a/features/bootstrap/FeatureContext.php b/features/bootstrap/FeatureContext.php
@@ -1,7 +1,8 @@
 <?php
 
-use Doctrine\DBAL\Exception\TableNotFoundException;
 use Doctrine\DBAL\DriverManager;
+use Doctrine\DBAL\Exception\TableNotFoundException;
+use Ez\DbLinker\Driver\Connection\RetryConnection;
 
 trait FeatureContext
 {
@@ -135,7 +136,7 @@ public function requestsAreForcedOnMasterFor($connectionName)
     {
         $connection = $this->getWrappedConnection($connectionName);
         if ($connection instanceof Ez\DbLinker\Driver\Connection\RetryConnection) {
-            $connection = $connection->wrappedConnection()->getWrappedConnection();
+            $connection = $connection->wrappedConnection();
         }
         $connection->connectToMaster();
     }
@@ -315,7 +316,7 @@ public function databaseHasGoneAwayOn($connectionName)
      */
     public function retryLimitShouldBe($connectionName, $n)
     {
-        $retryLimit = $this->getWrappedConnection($connectionName)->retryStrategy()->retryLimit();
+        $retryLimit = $this->connections[$connectionName]['params']['retryStrategy']->retryLimit();
         assert($retryLimit === (int) $n, "Retry limit is $retryLimit, $n expected.");
     }
 
@@ -384,7 +385,7 @@ public function shouldHaveSlave($connectionName, $n)
     {
         $connection = $this->getWrappedConnection($connectionName);
         if ($connection instanceof \Ez\DbLinker\Driver\Connection\RetryConnection) {
-            $connection = $connection->wrappedConnection()->getWrappedConnection();
+            $connection = $connection->wrappedConnection();
         }
         $slaveCount = $connection->slaves()->count();
         assert($slaveCount === (int)$n, "Slaves count is $slaveCount, $n expected.");
@@ -446,26 +447,14 @@ public function theLastStatementSucceeded()
         }
     }
 
-    /**
-     * @Then the last statement error should be :errorName
-     */
-    public function theLastStatementExpectedErrorCodeShouldBe($errorName)
-    {
-        $errorCode = $this->errorCode(
-            $this->lastStatementError ?:
-            $this->statement->connection->getWrappedConnection()->retryStrategy()->lastError()
-        );
-        $this->errorCodeMatchesErrorName($errorCode, $errorName);
-    }
-
     /**
      * @Then the last error should be :errorName on :connectionName
      */
     public function theLastErrorShouldBeOn($errorName, $connectionName)
     {
         $errorCode = $this->errorCode(
             $this->connections[$connectionName]['last-error'] ?:
-            $this->getWrappedConnection($connectionName)->retryStrategy()->lastError()
+            $retryStrategy = $this->connections[$connectionName]['params']['retryStrategy']->lastError()
         );
         $this->errorCodeMatchesErrorName($errorCode, $errorName);
     }
@@ -481,7 +470,7 @@ private function errorCodeMatchesErrorName($errorCode, $errorName)
     }
 
     abstract protected function errorToCode($errorName);
-    abstract protected function errorCode($exception);
+    abstract protected function errorCode(Exception $exception);
 
     private function getWrappedConnection($connectionName)
     {
@@ -507,15 +496,12 @@ abstract protected function masterParams($username = null, $password = '');
      */
     public function tableCanBeCreatedAutomaticallyOn($tableName, $connectionName)
     {
-        $connection = $this->getWrappedConnection($connectionName);
-        $connection->retryStrategy()->addHandler(function (
-            Doctrine\DBAL\DBALException $exception,
-            Ez\DbLinker\Driver\Connection\RetryConnection $connection
+        $retryStrategy = $this->connections[$connectionName]['params']['retryStrategy'];
+        $retryStrategy->addHandler(function (
+            Exception $exception,
+            RetryConnection $connection
         ) use ($tableName) {
-            if (
-                $exception instanceof TableNotFoundException &&
-                strpos($exception->getMessage(), $tableName) !== false
-            ) {
+            if (strpos($exception->getMessage(), $tableName) !== false) {
                 $connection->exec("CREATE TABLE {$tableName} (id INT)");
                 return true;
             }

diff --git a/features/bootstrap/MySQLContext.php b/features/bootstrap/MySQLContext.php
@@ -2,6 +2,11 @@
 
 use Doctrine\DBAL\DriverManager;
 
+use Doctrine\DBAL\DBALException;
+use Doctrine\DBAL\Driver\DriverException as DDriverException;
+use Doctrine\DBAL\Exception\DriverException as EDriverException;
+use Ez\DbLinker\Driver\Connection\RetryConnection;
+
 trait MySQLContext
 {
     private function masterParams($username = null, $password = '') {
@@ -71,9 +76,14 @@ private function retryStrategy($n)
         return new MysqlRetryStrategy($n);
     }
 
-    private function errorCode($exception)
+    private function errorCode(Exception $exception)
     {
-        return $exception->getErrorCode();
+        if ($exception instanceof DBALException) {
+            $exception = $exception->getPrevious();
+        }
+        if ($exception instanceof DDriverException || $exception instanceof EDriverException) {
+            return $exception->getErrorCode();
+        }
     }
 
     private function retryDriverClass()
@@ -116,15 +126,13 @@ class MysqlRetryStrategy extends Ez\DbLinker\RetryStrategy\MysqlRetryStrategy
     private $handlers = [];
 
     public function shouldRetry(
-        Doctrine\DBAL\DBALException $exception,
-        Ez\DbLinker\Driver\Connection\RetryConnection $connection,
-        $method,
-        Array $arguments
+        Exception $exception,
+        RetryConnection $connection
     ) {
         $this->lastError = $exception;
         return array_reduce($this->handlers, function($retry, Closure $handler) use ($exception, $connection) {
             return $retry || $handler($exception, $connection);
-        }, false) || parent::shouldRetry($exception, $connection, $method, $arguments);
+        }, false) || parent::shouldRetry($exception, $connection);
     }
 
     public function lastError()

diff --git a/features/bootstrap/PostgreSQLContext.php b/features/bootstrap/PostgreSQLContext.php
@@ -63,7 +63,7 @@ private function retryStrategy($n)
     }
 
 
-    private function errorCode($exception)
+    private function errorCode(Exception $exception)
     {
         if(preg_match("/SQLSTATE\[(?<errorCode>[A-Z0-9]*)\]/", $exception->getMessage(), $matches)) {
             return $matches["errorCode"];
@@ -113,15 +113,13 @@ class PostgreSQLRetryStrategy extends Ez\DbLinker\RetryStrategy\PostgreSQLRetryS
     private $handlers = [];
 
     public function shouldRetry(
-        Doctrine\DBAL\DBALException $exception,
-        Ez\DbLinker\Driver\Connection\RetryConnection $connection,
-        $method,
-        Array $arguments
+        Exception $exception,
+        RetryConnection $connection
     ) {
         $this->lastError = $exception;
         return array_reduce($this->handlers, function($retry, Closure $handler) use ($exception, $connection) {
             return $retry || $handler($exception, $connection);
-        }, false) || parent::shouldRetry($exception, $connection, $method, $arguments);
+        }, false) || parent::shouldRetry($exception, $connection);
     }
 
     public function lastError()

diff --git a/features/retry.feature b/features/retry.feature
@@ -83,4 +83,4 @@ Feature: Retry
          When I prepare a statement "SELECT * FROM not_here_yet" on "conn"
           And I execute this statement
          Then the last statement succeeded
-          And the last statement error should be "NO_SUCH_TABLE"
+          And the last error should be "NO_SUCH_TABLE" on "conn"
diff --git a/src/Driver/Connection/CallAndRetry.php b/src/Driver/Connection/CallAndRetry.php
@@ -2,46 +2,28 @@
 
 namespace Ez\DbLinker\Driver\Connection;
 
-use Doctrine\DBAL\DBALException;
+use Exception;
+use Ez\DbLinker\RetryStrategy;
 
 trait CallAndRetry
 {
     /**
-     * call $method woth $arguments and retry if necessary
-     * @param  string $method    method name
-     * @param  array  $arguments [description]
+     * call $callable and retry if necessary
      */
-    private function callAndRetry($method, array $arguments)
+    private function callAndRetry(callable $callable, RetryStrategy $strategy, RetryConnection $connection)
     {
         do {
             try {
-                return @call_user_func_array([$this->wrappedObject(), $method], $arguments);
-            } catch (DBALException $exception) {
-                if (!$this->retryStrategy()->shouldRetry(
+                return @$callable();
+            } catch (Exception $exception) {
+                if (!$strategy->shouldRetry(
                     $exception,
-                    $this->retryConnection(),
-                    $method,
-                    $arguments
+                    $connection
                 )) {
                     // stop trying
                     throw $exception;
                 }
             }
         } while (true);
     }
-
-    /**
-     * @return mixed
-     */
-    abstract protected function wrappedObject();
-
-    /**
-     * @return Ez\DbLinker\RetryConnection
-     */
-    abstract protected function retryConnection();
-
-    /**
-     * @return Ez\DbLinker\RetryStrategy
-     */
-    abstract protected function retryStrategy();
 }
diff --git a/src/Driver/Connection/MasterSlavesConnection.php b/src/Driver/Connection/MasterSlavesConnection.php
@@ -11,8 +11,9 @@ class MasterSlavesConnection implements Connection
     private $master;
     private $slavesWeights;
     private $connection;
+    private $currentConnectionFactory;
 
-    public function __construct(Connection $master, SplObjectStorage $slavesWeights)
+    public function __construct($master, SplObjectStorage $slavesWeights)
     {
         $this->master = $master;
         $this->checkSlavesWeights($slavesWeights);
@@ -22,37 +23,37 @@ public function __construct(Connection $master, SplObjectStorage $slavesWeights)
     private function checkSlavesWeights(SplObjectStorage $slavesWeights)
     {
         foreach ($slavesWeights as $slave) {
-            $weight = $slavesWeights[$slave];
-            if (!$slave instanceof Connection) {
-                throw new Exception(
-                    'All objects attached to $slavesWeights must implements Doctrine\DBAL\Driver\Connection'
-                );
-            }
-            if ((int)$weight < 0) {
+            if ((int)$slavesWeights[$slave] < 0) {
                 throw new Exception('Slave weight must be >= 0');
             }
         }
     }
 
     public function connectToMaster()
     {
-        $this->connection = $this->master;
+        $this->currentConnectionFactory = $this->master;
+        $this->connection = null;
     }
 
     public function connectToSlave()
     {
+        $this->currentConnectionFactory = null;
         $this->connection = null;
     }
 
     public function isConnectedToMaster()
     {
-        return $this->connection === $this->master;
+        return $this->currentConnectionFactory === $this->master;
     }
 
     private function connection()
     {
         if ($this->connection === null) {
-            $this->connection = $this->chooseASlave();
+            if ($this->currentConnectionFactory === null) {
+                $this->currentConnectionFactory = $this->chooseASlave() ?: $this->master;
+            }
+            $factory = $this->currentConnectionFactory;
+            $this->connection = $factory();
         }
         return $this->connection;
     }
@@ -66,7 +67,7 @@ private function chooseASlave()
     {
         $totalSlavesWeight = $this->totalSlavesWeight();
         if ($totalSlavesWeight < 1) {
-            return $this->master;
+            return null;
         }
         $weightTarget = mt_rand(1, $totalSlavesWeight);
         foreach ($this->slavesWeights as $slave) {
@@ -88,7 +89,8 @@ private function totalSlavesWeight()
 
     public function disableCurrentSlave()
     {
-        $this->slavesWeights->detach($this->connection);
+        $this->slavesWeights->detach($this->currentConnectionFactory);
+        $this->currentConnectionFactory = null;
         $this->connection = null;
     }