Throw exceptions for error status codes

Author: ddeboer

src/Exception/ProxyResponseException.php

@@ -11,27 +11,26 @@
 
 namespace FOS\HttpCache\Exception;
 
-use Http\Adapter\Exception\HttpAdapterException;
+use Psr\Http\Message\ResponseInterface;
 
 /**
  * Wrapping an error response from the caching proxy.
  */
 class ProxyResponseException extends \RuntimeException implements HttpCacheExceptionInterface
 {
     /**
-     * @param HttpAdapterException $adapterException HTTP adapter exception.
+     * @param ResponseInterface $response HTTP response
      *
      * @return ProxyResponseException
      */
-    public static function proxyResponse(HttpAdapterException $adapterException)
+    public static function proxyResponse(ResponseInterface $response)
     {
         $message = sprintf(
-            '%s error response "%s" from caching proxy at %s',
-            $adapterException->getResponse()->getStatusCode(),
-            $adapterException->getResponse()->getReasonPhrase(),
-            $adapterException->getRequest()->getHeaderLine('Host')
+            '%s error response "%s" from caching proxy',
+            $response->getStatusCode(),
+            $response->getReasonPhrase()
         );
 
-        return new ProxyResponseException($message, 0, $adapterException);
+        return new ProxyResponseException($message, 0);
     }
 }

src/ProxyClient/AbstractProxyClient.php

@@ -19,6 +19,7 @@
 use Http\Adapter\Exception\MultiHttpAdapterException;
 use Http\Adapter\HttpAdapter;
 use Http\Discovery\HttpAdapterDiscovery;
+use Psr\Http\Message\ResponseInterface;
 
 /**
  * Abstract caching proxy client
@@ -77,27 +78,33 @@ public function flush()
         $this->queue->clear();
 
         try {
-            $this->httpAdapter->sendRequests($queue->all());
+            $responses = $this->httpAdapter->sendRequests($queue->all());
         } catch (MultiHttpAdapterException $e) {
+            // Handle all networking errors: php-http only throws an exception
+            // if no response is available.
             $collection = new ExceptionCollection();
             foreach ($e->getExceptions() as $exception) {
-                // A workaround for php-http currently lacking differentiation
-                // between client, server and networking errors.
+                // php-http only throws an exception if no response is available
                 if (!$exception->getResponse()) {
                     // Assume networking error if no response was returned.
                     $collection->add(
                         ProxyUnreachableException::proxyUnreachable($exception)
                     );
-                } else {
-                    $collection->add(
-                        ProxyResponseException::proxyResponse($exception)
-                    );
                 }
             }
 
+            foreach ($this->handleErrorResponses($e->getResponses()) as $exception) {
+                $collection->add($exception);
+            }
+
             throw $collection;
         }
 
+        $exceptions = $this->handleErrorResponses($responses);
+        if (count($exceptions) > 0) {
+            throw new ExceptionCollection($exceptions);
+        }
+
         return count($queue);
     }
 
@@ -123,4 +130,24 @@ protected function initQueue(array $servers, $baseUri)
     {
         $this->queue = new RequestQueue($servers, $baseUri);
     }
+
+    /**
+     * @param ResponseInterface[] $responses
+     *
+     * @return ProxyResponseException[]
+     */
+    private function handleErrorResponses(array $responses)
+    {
+        $exceptions = [];
+
+        foreach ($responses as $response) {
+            if ($response->getStatusCode() >= 400
+                && $response->getStatusCode() < 600
+            ) {
+                $exceptions[] = ProxyResponseException::proxyResponse($response);
+            }
+        }
+
+        return $exceptions;
+    }
 }

src/Test/HttpClient/MockHttpAdapter.php

@@ -5,7 +5,9 @@
 use Http\Adapter\Exception\MultiHttpAdapterException;
 use Http\Adapter\HttpAdapter;
 use GuzzleHttp\Psr7\Response;
+use Http\Discovery\MessageFactoryDiscovery;
 use Psr\Http\Message\RequestInterface;
+use Psr\Http\Message\ResponseInterface;
 
 /**
  * HTTP adapter mock
@@ -17,6 +19,7 @@
 class MockHttpAdapter implements HttpAdapter
 {
     private $requests = [];
+    private $responses = [];
     private $exception;
 
     /**
@@ -30,7 +33,12 @@ public function sendRequest(RequestInterface $request, array $options = [])
             throw $this->exception;
         }
 
-        return new Response();
+        if (count($this->responses) > 0) {
+            return array_shift($this->responses);
+        }
+
+        return MessageFactoryDiscovery::find()->createResponse();
+
     }
 
     /**
@@ -61,6 +69,11 @@ public function setException(\Exception $exception)
         $this->exception = $exception;
     }
 
+    public function addResponse(ResponseInterface $response)
+    {
+        $this->responses[] = $response;
+    }
+
     /**
      * {@inheritdoc}
      *

tests/Functional/Varnish/VarnishProxyClientTest.php

@@ -121,10 +121,10 @@ public function testRefresh()
         $this->getProxyClient()->refresh('/cache.php')->flush();
         usleep(1000);
         $refreshed = $this->getResponse('/cache.php');
-        
+
         $originalTimestamp = (float)(string) $response->getBody();
         $refreshedTimestamp = (float)(string) $refreshed->getBody();
-        
+
         $this->assertGreaterThan($originalTimestamp, $refreshedTimestamp);
     }
 

tests/Unit/CacheInvalidatorTest.php

@@ -197,10 +197,7 @@ public function testProxyClientExceptionsAreLogged()
             ->shouldReceive('getStatusCode')->andReturn(403)
             ->shouldReceive('getReasonPhrase')->andReturn('Forbidden')
             ->getMock();
-        $adapterException = new HttpAdapterException('Forbidden');
-        $adapterException->setRequest($failedRequest);
-        $adapterException->setResponse($response);
-        $responseException = ProxyResponseException::proxyResponse($adapterException);
+        $responseException = ProxyResponseException::proxyResponse($response);
 
         $exceptions = new ExceptionCollection();
         $exceptions->add($unreachableException)->add($responseException);
@@ -221,7 +218,7 @@ public function testProxyClientExceptionsAreLogged()
             ->shouldReceive('log')->once()
             ->with(
                 'critical',
-                '403 error response "Forbidden" from caching proxy at 127.0.0.1',
+                '403 error response "Forbidden" from caching proxy',
                 ['exception' => $responseException]
             )
             ->getMock();

tests/Unit/ProxyClient/AbstractProxyClientTest.php

@@ -15,6 +15,7 @@
 use FOS\HttpCache\ProxyClient\Varnish;
 use FOS\HttpCache\Test\HttpClient\MockHttpAdapter;
 use Http\Adapter\Exception\HttpAdapterException;
+use Http\Discovery\MessageFactoryDiscovery;
 use Psr\Http\Message\RequestInterface;
 use \Mockery;
 
@@ -31,7 +32,7 @@ class AbstractProxyClientTest extends \PHPUnit_Framework_TestCase
     /**
      * @dataProvider exceptionProvider
      *
-     * @param \Exception $exception The exception that curl should throw.
+     * @param \Exception $exception Exception thrown by HTTP adapter.
      * @param string     $type      The returned exception class to be expected.
      * @param string     $message   Optional exception message to match against.
      */
@@ -74,30 +75,36 @@ public function exceptionProvider()
         $unreachableException = new HttpAdapterException();
         $unreachableException->setRequest($request);
 
-        // Client exception (with response)
-        $response = \Mockery::mock('\Psr\Http\Message\ResponseInterface')
-            ->shouldReceive('getStatusCode')->andReturn(500)
-            ->shouldReceive('getReasonPhrase')->andReturn('Uh-oh!')
-            ->getMock()
-        ;
-        $responseException = new HttpAdapterException();
-        $responseException->setRequest($request);
-        $responseException->setResponse($response);
-
         return [
             [
                 $unreachableException,
                 '\FOS\HttpCache\Exception\ProxyUnreachableException',
                 'bla.com'
-            ],
-            [
-                $responseException,
-                '\FOS\HttpCache\Exception\ProxyResponseException',
-                'bla.com'
-            ],
+            ]
         ];
     }
 
+    public function testErrorResponsesAreConvertedToExceptions()
+    {
+        $response = MessageFactoryDiscovery::find()->createResponse(
+            405,
+            'Not allowed'
+        );
+        $this->client->addResponse($response);
+
+        $varnish = new Varnish(['127.0.0.1:123'], 'my_hostname.dev', $this->client);
+        try {
+            $varnish->purge('/')->flush();
+            $this->fail('Should have aborted with an exception');
+        } catch (ExceptionCollection $exceptions) {
+            $this->assertCount(1, $exceptions);
+            $this->assertEquals(
+                '405 error response "Not allowed" from caching proxy',
+                $exceptions->getFirst()->getMessage()
+            );
+        }
+    }
+
     /**
      * @expectedException \FOS\HttpCache\Exception\MissingHostException
      * @expectedExceptionMessage cannot be invalidated without a host
@@ -170,23 +177,23 @@ public function testFlushEmpty()
 
     public function testFlushCountSuccess()
     {
-        $self = $this;
         $httpAdapter = \Mockery::mock('\Http\Adapter\HttpAdapter')
             ->shouldReceive('sendRequests')
             ->once()
             ->with(
                 \Mockery::on(
-                    function ($requests) use ($self) {
+                    function ($requests) {
                         /** @type RequestInterface[] $requests */
-                        $self->assertCount(4, $requests);
+                        $this->assertCount(4, $requests);
                         foreach ($requests as $request) {
-                            $self->assertEquals('PURGE', $request->getMethod());
+                            $this->assertEquals('PURGE', $request->getMethod());
                         }
 
                         return true;
                     }
                 )
             )
+            ->andReturn([])
             ->getMock();
 
         $varnish = new Varnish(['127.0.0.1', '127.0.0.2'], 'fos.lo', $httpAdapter);
@@ -202,23 +209,23 @@ function ($requests) use ($self) {
 
     public function testEliminateDuplicates()
     {
-        $self = $this;
         $client = \Mockery::mock('\Http\Adapter\HttpAdapter')
             ->shouldReceive('sendRequests')
             ->once()
             ->with(
                 \Mockery::on(
-                    function ($requests) use ($self) {
+                    function ($requests) {
                         /** @type RequestInterface[] $requests */
-                        $self->assertCount(4, $requests);
+                        $this->assertCount(4, $requests);
                         foreach ($requests as $request) {
-                            $self->assertEquals('PURGE', $request->getMethod());
+                            $this->assertEquals('PURGE', $request->getMethod());
                         }
 
                         return true;
                     }
                 )
             )
+            ->andReturn([])
             ->getMock();
 
         $varnish = new Varnish(array('127.0.0.1', '127.0.0.2'), 'fos.lo', $client);