Merge pull request aws#756 from jeskew/fix/handle-strange-s3-errors

Treat 200-coded error responses from S3 as retriable errors

src/Api/Parser/Exception/ParserException.php

@@ -0,0 +1,4 @@
+<?php
+namespace Aws\Api\Parser\Exception;
+
+class ParserException extends \RuntimeException {}

src/Api/Parser/JsonRpcParser.php

@@ -11,6 +11,8 @@
  */
 class JsonRpcParser extends AbstractParser
 {
+    use PayloadParserTrait;
+
     private $parser;
 
     /**
@@ -31,7 +33,7 @@ public function __invoke(
 
         return new Result($this->parser->parse(
             $operation->getOutput(),
-            json_decode($response->getBody(), true)
+            $this->parseJson($response->getBody())
         ));
     }
 }

src/Api/Parser/PayloadParserTrait.php

@@ -0,0 +1,51 @@
+<?php
+namespace Aws\Api\Parser;
+
+use Aws\Api\Parser\Exception\ParserException;
+
+trait PayloadParserTrait
+{
+    /**
+     * @param string $json
+     *
+     * @throws ParserException
+     *
+     * @return array
+     */
+    private function parseJson($json)
+    {
+        $jsonPayload = json_decode($json, true);
+
+        if (JSON_ERROR_NONE !== json_last_error()) {
+            throw new ParserException('Error parsing JSON: '
+                . json_last_error_msg());
+        }
+
+        return $jsonPayload;
+    }
+
+    /**
+     * @param string $xml
+     *
+     * @throws ParserException
+     *
+     * @return \SimpleXMLElement
+     */
+    private function parseXml($xml)
+    {
+        $priorSetting = libxml_use_internal_errors(true);
+        try {
+            libxml_clear_errors();
+            $xmlPayload = new \SimpleXMLElement($xml);
+            if ($error = libxml_get_last_error()) {
+                throw new \RuntimeException($error->message);
+            }
+        } catch (\Exception $e) {
+            throw new ParserException("Error parsing XML: {$e->getMessage()}", 0, $e);
+        } finally {
+            libxml_use_internal_errors($priorSetting);
+        }
+
+        return $xmlPayload;
+    }
+}

src/Api/Parser/QueryParser.php

@@ -11,6 +11,8 @@
  */
 class QueryParser extends AbstractParser
 {
+    use PayloadParserTrait;
+
     /** @var XmlParser */
     private $xmlParser;
 
@@ -39,7 +41,7 @@ public function __invoke(
         ResponseInterface $response
     ) {
         $output = $this->api->getOperation($command->getName())->getOutput();
-        $xml = new \SimpleXMLElement($response->getBody());
+        $xml = $this->parseXml($response->getBody());
 
         if ($this->honorResultWrapper && $output['resultWrapper']) {
             $xml = $xml->{$output['resultWrapper']};

src/Api/Parser/RestJsonParser.php

@@ -10,6 +10,8 @@
  */
 class RestJsonParser extends AbstractRestParser
 {
+    use PayloadParserTrait;
+
     /** @var JsonParser */
     private $parser;
 
@@ -28,7 +30,7 @@ protected function payload(
         StructureShape $member,
         array &$result
     ) {
-        $jsonBody = json_decode($response->getBody(), true);
+        $jsonBody = $this->parseJson($response->getBody());
 
         if ($jsonBody) {
             $result += $this->parser->parse($member, $jsonBody);

src/Api/Parser/RestXmlParser.php

@@ -10,6 +10,8 @@
  */
 class RestXmlParser extends AbstractRestParser
 {
+    use PayloadParserTrait;
+
     /** @var XmlParser */
     private $parser;
 
@@ -28,7 +30,7 @@ protected function payload(
         StructureShape $member,
         array &$result
     ) {
-        $xml = new \SimpleXMLElement($response->getBody());
+        $xml = $this->parseXml($response->getBody());
         $result += $this->parser->parse($member, $xml);
     }
 }

src/S3/AmbiguousSuccessParser.php

@@ -0,0 +1,60 @@
+<?php
+namespace Aws\S3;
+
+use Aws\Api\Parser\AbstractParser;
+use Aws\CommandInterface;
+use Aws\Exception\AwsException;
+use Psr\Http\Message\ResponseInterface;
+
+/**
+ * Converts errors returned with a status code of 200 to a retryable error type.
+ *
+ * @internal
+ */
+class AmbiguousSuccessParser extends AbstractParser
+{
+    private static $ambiguousSuccesses = [
+        'UploadPartCopy' => true,
+        'CopyObject' => true,
+        'CompleteMultipartUpload' => true,
+    ];
+
+    /** @var callable */
+    private $parser;
+    /** @var callable */
+    private $errorParser;
+    /** @var string */
+    private $exceptionClass;
+
+    public function __construct(
+        callable $parser,
+        callable $errorParser,
+        $exceptionClass = AwsException::class
+    ) {
+        $this->parser = $parser;
+        $this->errorParser = $errorParser;
+        $this->exceptionClass = $exceptionClass;
+    }
+
+    public function __invoke(
+        CommandInterface $command,
+        ResponseInterface $response
+    ) {
+        if (200 === $response->getStatusCode()
+            && isset(self::$ambiguousSuccesses[$command->getName()])
+        ) {
+            $errorParser = $this->errorParser;
+            $parsed = $errorParser($response);
+            if (isset($parsed['code']) && isset($parsed['message'])) {
+                throw new $this->exceptionClass(
+                    $parsed['message'],
+                    $command,
+                    ['connection_error' => true]
+                );
+            }
+        }
+
+        $fn = $this->parser;
+        return $fn($command, $response);
+    }
+}

src/S3/RetryableMalformedResponseParser.php

@@ -0,0 +1,48 @@
+<?php
+namespace Aws\S3;
+
+use Aws\Api\Parser\AbstractParser;
+use Aws\Api\Parser\Exception\ParserException;
+use Aws\CommandInterface;
+use Aws\Exception\AwsException;
+use Psr\Http\Message\ResponseInterface;
+
+/**
+ * Converts malformed responses to a retryable error type.
+ *
+ * @internal
+ */
+class RetryableMalformedResponseParser extends AbstractParser
+{
+    /** @var callable */
+    private $parser;
+    /** @var string */
+    private $exceptionClass;
+
+    public function __construct(
+        callable $parser,
+        $exceptionClass = AwsException::class
+    ) {
+        $this->parser = $parser;
+        $this->exceptionClass = $exceptionClass;
+    }
+
+    public function __invoke(
+        CommandInterface $command,
+        ResponseInterface $response
+    ) {
+        $fn = $this->parser;
+
+        try {
+            return $fn($command, $response);
+        } catch (ParserException $e) {
+            throw new $this->exceptionClass(
+                "Error parsing response for {$command->getName()}:"
+                    . " AWS parsing error: {$e->getMessage()}",
+                $command,
+                ['connection_error' => true, 'exception' => $e],
+                $e
+            );
+        }
+    }
+}

src/S3/S3Client.php

@@ -494,7 +494,16 @@ public static function _applyRetryConfig($value, $_, HandlerList $list)
     public static function _applyApiProvider($value, array &$args, HandlerList $list)
     {
         ClientResolver::_apply_api_provider($value, $args, $list);
-        $args['parser'] = new GetBucketLocationParser($args['parser']);
+        $args['parser'] = new GetBucketLocationParser(
+            new AmbiguousSuccessParser(
+                new RetryableMalformedResponseParser(
+                    $args['parser'],
+                    $args['exception_class']
+                ),
+                $args['error_parser'],
+                $args['exception_class']
+            )
+        );
     }
 
     /**

tests/DynamoDb/DynamoDbClientTest.php

@@ -79,8 +79,8 @@ public function dataForFormatValueTest()
     public function testValidatesAndRetriesCrc32()
     {
         $queue = [
-            new Response(200, ['x-amz-crc32' => '123'], 'foo'),
-            new Response(200, ['x-amz-crc32' => '2356372769'], 'foo')
+            new Response(200, ['x-amz-crc32' => '123'], '"foo"'),
+            new Response(200, ['x-amz-crc32' => '400595255'], '"foo"')
         ];
 
         $handler = function ($request, $options) use (&$queue) {

tests/S3/AmbiguousSuccessParserTest.php

@@ -0,0 +1,92 @@
+<?php
+namespace Aws\Test\S3;
+
+use Aws\Api\ApiProvider;
+use Aws\CommandInterface;
+use Aws\S3\AmbiguousSuccessParser;
+use Aws\S3\Exception\S3Exception;
+use Psr\Http\Message\ResponseInterface;
+
+class AmbiguousSuccessParserTest extends \PHPUnit_Framework_TestCase
+{
+    private $instance;
+
+    public function setUp()
+    {
+        $parser = function () {};
+        $errorParser = function () {
+            return ['code' => 'InternalError', 'message' => 'Sorry!'];
+        };
+
+        $this->instance = new AmbiguousSuccessParser(
+            $parser,
+            $errorParser,
+            S3Exception::class
+        );
+    }
+
+    /**
+     * @dataProvider opsWithAmbiguousSuccessesProvider
+     * @param string $operation
+     *
+     * @expectedException \Aws\S3\Exception\S3Exception
+     * @expectedExceptionMessage Sorry!
+     */
+    public function testConvertsAmbiguousSuccessesToExceptions($operation)
+    {
+        $command = $this->getMock(CommandInterface::class);
+        $command->expects($this->any())
+            ->method('getName')
+            ->willReturn($operation);
+        $response = $this->getMock(ResponseInterface::class);
+        $response->expects($this->any())
+            ->method('getStatusCode')
+            ->willReturn(200);
+
+        $instance = $this->instance;
+        $instance($command, $response);
+    }
+
+    /**
+     * @dataProvider opsWithoutAmbiguousSuccessesProvider
+     * @param string $operation
+     */
+    public function testIgnoresAmbiguousSuccessesOnUnaffectedOperations($operation)
+    {
+        $command = $this->getMock(CommandInterface::class);
+        $command->expects($this->any())
+            ->method('getName')
+            ->willReturn($operation);
+        $response = $this->getMock(ResponseInterface::class);
+        $response->expects($this->any())
+            ->method('getStatusCode')
+            ->willReturn(200);
+
+        $instance = $this->instance;
+        $instance($command, $response);
+    }
+
+    public function opsWithAmbiguousSuccessesProvider()
+    {
+        return [
+            ['CopyObject'],
+            ['UploadPartCopy'],
+            ['CompleteMultipartUpload'],
+        ];
+    }
+
+    public function opsWithoutAmbiguousSuccessesProvider()
+    {
+        $provider = ApiProvider::defaultProvider();
+        return array_map(
+            function ($op) { return [$op]; },
+            array_diff(
+                array_keys($provider('api', 's3', 'latest')['operations']),
+                array_map(
+                    function (array $args) { return $args[0]; },
+                    $this->opsWithAmbiguousSuccessesProvider()
+                )
+            )
+        );
+    }
+}

tests/S3/RetryableMalformedResponseParserTest.php

@@ -0,0 +1,31 @@
+<?php
+namespace Aws\Test\S3;
+
+
+use Aws\Api\Parser\Exception\ParserException;
+use Aws\CommandInterface;
+use Aws\S3\Exception\S3Exception;
+use Aws\S3\RetryableMalformedResponseParser;
+use Psr\Http\Message\ResponseInterface;
+
+class RetryableMalformedResponseParserTest extends \PHPUnit_Framework_TestCase
+{
+    /**
+     * @expectedException \Aws\S3\Exception\S3Exception
+     * @expectedExceptionMessage Sorry!
+     */
+    public function testConvertsParserExceptionsToRetryableExceptions()
+    {
+        $parser = function () { throw new ParserException('Sorry!'); };
+
+        $instance = new RetryableMalformedResponseParser(
+            $parser,
+            S3Exception::class
+        );
+
+        $instance(
+            $this->getMock(CommandInterface::class),
+            $this->getMock(ResponseInterface::class)
+        );
+    }
+}