Moved fetch retries out of connectors into Porter.

Bilge · Bilge · commit 6c4dcd94cc2b · 2016-12-08T00:09:26.000Z
diff --git a/.travis.yml b/.travis.yml
@@ -21,7 +21,7 @@ script:
 
   # Remove mapper and run all non-Mapper tests again.
   - composer --dev --no-update-with-dependencies remove scriptfusion/mapper
-  - '! composer info | awk ''{print $1}'' | grep ^scriptfusion/mapper'
+  - '! composer info | grep ^scriptfusion/mapper\\b'
   - bin/test --exclude-group Mapper
 
 after_success:
diff --git a/composer.json b/composer.json
@@ -11,6 +11,7 @@
   "require": {
     "php": "^5.5|^7",
     "scriptfusion/static-class": "^1",
+    "scriptfusion/retry": "^1.1",
     "scriptfusion/retry-error-handlers": "^1",
     "eloquent/enumeration": "^5",
     "psr/cache": "^1",
diff --git a/src/Net/Http/HttpConnector.php b/src/Net/Http/HttpConnector.php
@@ -4,7 +4,6 @@
 use ScriptFUSION\Porter\Connector\CachingConnector;
 use ScriptFUSION\Porter\Net\UrlBuilder;
 use ScriptFUSION\Porter\Options\EncapsulatedOptions;
-use ScriptFUSION\Retry\ErrorHandler\ExponentialBackoffErrorHandler;
 
 /**
  * Fetches data from an HTTP server via the PHP wrapper.
@@ -14,8 +13,6 @@
  */
 class HttpConnector extends CachingConnector
 {
-    const DEFAULT_TRIES = 5;
-
     /** @var HttpOptions */
     private $options;
 
@@ -25,9 +22,6 @@ class HttpConnector extends CachingConnector
     /** @var string */
     private $baseUrl;
 
-    /** @var int */
-    private $tries = self::DEFAULT_TRIES;
-
     public function __construct(HttpOptions $options = null)
     {
         parent::__construct();
@@ -53,34 +47,32 @@ public function fetchFreshData($source, EncapsulatedOptions $options = null)
             throw new \InvalidArgumentException('Options must be an instance of HttpOptions.');
         }
 
-        return \ScriptFUSION\Retry\retry($this->getTries(), function () use ($source, $options) {
-            if (false === $response = @file_get_contents(
-                $this->getOrCreateUrlBuilder()->buildUrl(
-                    $source,
-                    $options ? $options->getQueryParameters() : [],
-                    $this->getBaseUrl()
+        if (false === $response = @file_get_contents(
+            $this->getOrCreateUrlBuilder()->buildUrl(
+                $source,
+                $options ? $options->getQueryParameters() : [],
+                $this->getBaseUrl()
+            ),
+            false,
+            stream_context_create([
+                'http' => ['ignore_errors' => true] + array_merge(
+                    $this->options->extractHttpContextOptions(),
+                    $options ? $options->extractHttpContextOptions() : []
                 ),
-                false,
-                stream_context_create([
-                    'http' => ['ignore_errors' => true] + array_merge(
-                        $this->options->extractHttpContextOptions(),
-                        $options ? $options->extractHttpContextOptions() : []
-                    ),
-                ])
-            )) {
-                $error = error_get_last();
-                throw new HttpConnectionException($error['message'], $error['type']);
-            }
+            ])
+        )) {
+            $error = error_get_last();
+            throw new HttpConnectionException($error['message'], $error['type']);
+        }
 
-            $code = explode(' ', $http_response_header[0], 3)[1];
-            if ($code < 200 || $code >= 400) {
-                throw new HttpServerException(
-                    "HTTP server responded with error: \"$http_response_header[0]\".\n\n$response"
-                );
-            }
+        $code = explode(' ', $http_response_header[0], 3)[1];
+        if ($code < 200 || $code >= 400) {
+            throw new HttpServerException(
+                "HTTP server responded with error: \"$http_response_header[0]\".\n\n$response"
+            );
+        }
 
-            return $response;
-        }, new ExponentialBackoffErrorHandler);
+        return $response;
     }
 
     private function getOrCreateUrlBuilder()
@@ -107,28 +99,4 @@ public function setBaseUrl($baseUrl)
 
         return $this;
     }
-
-    /**
-     * Gets the maximum number of fetch attempts
-     *
-     * @return int
-     */
-    public function getTries()
-    {
-        return $this->tries;
-    }
-
-    /**
-     * Sets the maximum number of fetch attempts.
-     *
-     * @param int $tries Maximum fetch attempts.
-     *
-     * @return $this
-     */
-    public function setTries($tries)
-    {
-        $this->tries = max(1, $tries|0);
-
-        return $this;
-    }
 }
diff --git a/src/Net/Soap/SoapConnector.php b/src/Net/Soap/SoapConnector.php
@@ -4,7 +4,6 @@
 use ScriptFUSION\Porter\Connector\CachingConnector;
 use ScriptFUSION\Porter\Options\EncapsulatedOptions;
 use ScriptFUSION\Porter\Type\ObjectType;
-use ScriptFUSION\Retry\ErrorHandler\ExponentialBackoffErrorHandler;
 
 /**
  * Fetches data from a SOAP service.
@@ -33,11 +32,7 @@ public function fetchFreshData($source, EncapsulatedOptions $options = null)
 
         $params = array_merge($this->options->getParameters(), $options ? $options->getParameters() : []);
 
-        return ObjectType::toArray(
-            \ScriptFUSION\Retry\retry(5, function () use ($source, $params) {
-                return $this->getOrCreateClient()->$source($params);
-            }, new ExponentialBackoffErrorHandler)
-        );
+        return ObjectType::toArray($this->getOrCreateClient()->$source($params));
     }
 
     private function getOrCreateClient()
diff --git a/src/Porter.php b/src/Porter.php
@@ -20,12 +20,15 @@
 use ScriptFUSION\Porter\Provider\ProviderFactory;
 use ScriptFUSION\Porter\Provider\Resource\ProviderResource;
 use ScriptFUSION\Porter\Specification\ImportSpecification;
+use ScriptFUSION\Retry\ErrorHandler\ExponentialBackoffErrorHandler;
 
 /**
  * Imports data according to an ImportSpecification.
  */
 class Porter
 {
+    const DEFAULT_FETCH_ATTEMPTS = 5;
+
     /**
      * @var Provider[]
      */
@@ -46,6 +49,11 @@ class Porter
      */
     private $defaultCacheAdvice;
 
+    /**
+     * @var int
+     */
+    private $maxFetchAttempts = self::DEFAULT_FETCH_ATTEMPTS;
+
     public function __construct()
     {
         $this->defaultCacheAdvice = CacheAdvice::SHOULD_NOT_CACHE();
@@ -127,7 +135,13 @@ private function fetch(ProviderResource $resource, CacheAdvice $cacheAdvice = nu
         $provider = $this->getProvider($resource->getProviderClassName(), $resource->getProviderTag());
         $this->applyCacheAdvice($provider, $cacheAdvice ?: $this->defaultCacheAdvice);
 
-        return $provider->fetch($resource);
+        return \ScriptFUSION\Retry\retry(
+            $this->maxFetchAttempts,
+            function () use ($provider, $resource) {
+                return $provider->fetch($resource);
+            },
+            new ExponentialBackoffErrorHandler
+        );
     }
 
     private function filter(ProviderRecords $records, callable $predicate, $context)
@@ -289,4 +303,28 @@ public function setMapper(CollectionMapper $mapper)
 
         return $this;
     }
+
+    /**
+     * Gets the maximum number of fetch attempts per import.
+     *
+     * @return int
+     */
+    public function getMaxFetchAttempts()
+    {
+        return $this->maxFetchAttempts;
+    }
+
+    /**
+     * Sets the maximum number of fetch attempts per import.
+     *
+     * @param int $attempts Maximum fetch attempts.
+     *
+     * @return $this
+     */
+    public function setMaxFetchAttempts($attempts)
+    {
+        $this->maxFetchAttempts = max(1, $attempts|0);
+
+        return $this;
+    }
 }
diff --git a/test/Functional/Porter/Net/Http/HttpConnectorTest.php b/test/Functional/Porter/Net/Http/HttpConnectorTest.php
@@ -6,7 +6,7 @@
 use ScriptFUSION\Porter\Net\Http\HttpConnector;
 use ScriptFUSION\Porter\Net\Http\HttpOptions;
 use ScriptFUSION\Porter\Net\Http\HttpServerException;
-use ScriptFUSION\Retry\FailingTooHardException;
+use ScriptFUSION\Retry\ErrorHandler\ExponentialBackoffErrorHandler;
 use Symfony\Component\Process\Process;
 
 final class HttpConnectorTest extends \PHPUnit_Framework_TestCase
@@ -41,46 +41,22 @@ public function testConnectionToLocalWebserver()
 
     public function testConnectionTimeout()
     {
-        try {
-            $this->fetch($this->connector->setTries(1));
-        } catch (FailingTooHardException $exception) {
-            self::assertInstanceOf(HttpConnectionException::class, $exception->getPrevious());
-
-            return;
-        }
+        $this->setExpectedException(HttpConnectionException::class);
 
-        self::fail('Expected exception was not thrown.');
+        $this->fetch();
     }
 
     public function testErrorResponse()
     {
         $server = $this->startServer('404');
+
+        $this->setExpectedExceptionRegExp(HttpServerException::class, '[^foo\z]m');
+
         try {
             $this->fetch();
-        } catch (FailingTooHardException $exception) {
-            self::assertInstanceOf(HttpServerException::class, $exception->getPrevious(), $exception->getMessage());
-            self::assertStringEndsWith('foo', $exception->getPrevious()->getMessage());
-
-            return;
         } finally {
             $this->stopServer($server);
         }
-
-        self::fail('Expected exception was not thrown.');
-    }
-
-    public function testOneTry()
-    {
-        $this->setExpectedException(FailingTooHardException::class, '1');
-
-        $this->fetch($this->connector->setTries(1));
-    }
-
-    public function testDefaultTries()
-    {
-        $this->setExpectedException(FailingTooHardException::class, (string)HttpConnector::DEFAULT_TRIES);
-
-        $this->fetch();
     }
 
     /**
@@ -91,11 +67,30 @@ public function testDefaultTries()
     private function startServer($script)
     {
         $server = (
-            // Prevent forking on some Unix systems.
-            new Process(sprintf('%sphp -S %s %s.php', file_exists('/bin/sh') ? 'exec ' : '', self::HOST, $script))
+            new Process(sprintf(
+                '%sphp -S %s %s.php',
+                // Prevent forking on some Unix systems.
+                file_exists('/bin/sh') ? 'exec ' : '',
+                self::HOST,
+                $script
+            ))
         )->setWorkingDirectory(self::$dir);
         $server->start();
 
+        // Wait for server to spawn.
+        \ScriptFUSION\Retry\retry(5, function () {
+            $this->fetch();
+        }, function (\Exception $exception) {
+            static $handler;
+            $handler = $handler ?: new ExponentialBackoffErrorHandler;
+
+            if (!$exception instanceof HttpConnectionException) {
+                return false;
+            }
+
+            return $handler();
+        });
+
         return $server;
     }
 
diff --git a/test/Integration/Porter/PorterTest.php b/test/Integration/Porter/PorterTest.php
@@ -22,6 +22,7 @@
 use ScriptFUSION\Porter\ProviderNotFoundException;
 use ScriptFUSION\Porter\Specification\ImportSpecification;
 use ScriptFUSION\Porter\Specification\StaticDataImportSpecification;
+use ScriptFUSION\Retry\FailingTooHardException;
 use ScriptFUSIONTest\MockFactory;
 
 final class PorterTest extends \PHPUnit_Framework_TestCase
@@ -210,6 +211,22 @@ public function testImportTaggedResource()
         self::assertSame($output, $records->current());
     }
 
+    public function testOneTry()
+    {
+        $this->setExpectedException(FailingTooHardException::class, '1');
+
+        $this->provider->shouldReceive('fetch')->once()->andThrow(\Exception::class);
+        $this->porter->setMaxFetchAttempts(1)->import($this->specification);
+    }
+
+    public function testDefaultTries()
+    {
+        $this->setExpectedException(FailingTooHardException::class, (string)Porter::DEFAULT_FETCH_ATTEMPTS);
+
+        $this->provider->shouldReceive('fetch')->times(Porter::DEFAULT_FETCH_ATTEMPTS)->andThrow(\Exception::class);
+        $this->porter->import($this->specification);
+    }
+
     #endregion
 
     #region Import one
