Skip to content

Commit

Permalink
Various enhancements related to aws#604
Browse files Browse the repository at this point in the history 
This commit makes a few enhancements to the SDK based on aws#604:

1. Context parameters like cache and client are now removed from the
   parameters that are sent when executing commands in the S3 stream
   wrapper. When these parameters were present, they resulted in
   lengthy debug messages with the TraceMiddleware, and even caused
   stream resources to enter an invalid state after dumping them.
2. Updated TraceMiddleware to better provide debug information about
   exceptions. These debug messages previously var_dump'd the exception,
   causing a massive dump to the debug output. This is now handled by
   extracting specific pieces of information from exceptions and
   formatting in a more readable way.
3. Added a try/catch to the signature version 4 signer such that if a
   stream claims it's readable, but creating the hash fails for some
   reason, and CouldNotCreateChecksumException will still be thrown,
   which will provide more context as to why the checksum failed to
   create.
  • Loading branch information
@mtdowling
mtdowling committed Jun 3, 2015
1 parent c194ec8 commit 162e8f8
Show file tree
Hide file tree
Showing 6 changed files with 100 additions and 26 deletions.
4 changes: 2 additions & 2 deletions src/Exception/CouldNotCreateChecksumException.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

class CouldNotCreateChecksumException extends \RuntimeException
{
public function __construct($algorithm)
public function __construct($algorithm, \Exception $previous = null)
{
$prefix = $algorithm === 'md5' ? "An" : "A";
parent::__construct("{$prefix} {$algorithm} checksum could not be "
Expand All @@ -14,6 +14,6 @@ public function __construct($algorithm)
. "stream in a GuzzleHttp\\Stream\\CachingStream object. You "
. "should be careful though and remember that the CachingStream "
. "utilizes PHP temp streams. This means that the stream will be "
. "temporarily stored on the local disk.");
. "temporarily stored on the local disk.", 0, $previous);
}
}
27 changes: 17 additions & 10 deletions src/S3/StreamWrapper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ public function stream_flush()
if ($this->body->isSeekable()) {
$this->body->seek(0);
}
$params = $this->getOptions();
$params = $this->getOptions(true);
$params['Body'] = $this->body;

// Attempt to guess the ContentType of the upload based on the
Expand Down Expand Up @@ -502,7 +502,7 @@ public function rename($path_from, $path_to)
return $this->boolCall(function () use ($partsFrom, $partsTo) {
// Copy the object and allow overriding default parameters if
// desired, but by default copy metadata
$this->getClient()->copyObject($this->getOptions() + [
$this->getClient()->copyObject($this->getOptions(true) + [
'Bucket' => $partsTo['Bucket'],
'Key' => $partsTo['Key'],
'MetadataDirective' => 'COPY',
Expand All @@ -513,7 +513,7 @@ public function rename($path_from, $path_to)
$this->getClient()->deleteObject([
'Bucket' => $partsFrom['Bucket'],
'Key' => $partsFrom['Key']
] + $this->getOptions());
] + $this->getOptions(true));
return true;
});
}
Expand Down Expand Up @@ -547,7 +547,7 @@ private function validate($path, $mode)
$this->getClient()->doesObjectExist(
$this->getOption('Bucket'),
$this->getOption('Key'),
$this->getOptions()
$this->getOptions(true)
)
) {
$errors[] = "{$path} already exists on Amazon S3";
Expand All @@ -559,9 +559,12 @@ private function validate($path, $mode)
/**
* Get the stream context options available to the current stream
*
* @param bool $removeContextData Set to true to remove contextual kvp's
* like 'client' from the result.
*
* @return array
*/
private function getOptions()
private function getOptions($removeContextData = false)
{
// Context is not set when doing things like stat
if ($this->context === null) {
Expand All @@ -573,8 +576,13 @@ private function getOptions()

$default = stream_context_get_options(stream_context_get_default());
$default = isset($default['s3']) ? $default['s3'] : [];
$result = $this->params + $options + $default;

if ($removeContextData) {
unset($result['client'], $result['seekable'], $result['cache']);
}

return $this->params + $options + $default;
return $result;
}

/**
Expand Down Expand Up @@ -628,16 +636,15 @@ private function getBucketKey($path)
*/
private function withPath($path)
{
$params = $this->getOptions();
unset($params['seekable'], $params['client']);
$params = $this->getOptions(true);

return $this->getBucketKey($path) + $params;
}

private function openReadStream()
{
$client = $this->getClient();
$command = $client->getCommand('GetObject', $this->getOptions());
$command = $client->getCommand('GetObject', $this->getOptions(true));
$command['@http']['stream'] = true;
$result = $client->execute($command);
$this->body = $result['Body'];
Expand All @@ -661,7 +668,7 @@ private function openAppendStream()
try {
// Get the body of the object and seek to the end of the stream
$client = $this->getClient();
$this->body = $client->getObject($this->getOptions())['Body'];
$this->body = $client->getObject($this->getOptions(true))['Body'];
$this->body->seek(0, SEEK_END);
return true;
} catch (S3Exception $e) {
Expand Down
6 changes: 5 additions & 1 deletion src/Signature/SignatureV4.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,11 @@ protected function getPayload(RequestInterface $request)
throw new CouldNotCreateChecksumException('sha256');
}

return Psr7\hash($request->getBody(), 'sha256');
try {
return Psr7\hash($request->getBody(), 'sha256');
} catch (\Exception $e) {
throw new CouldNotCreateChecksumException('sha256', $e);
}
}

protected function getPresignedPayload(RequestInterface $request)
Expand Down
38 changes: 25 additions & 13 deletions src/TraceMiddleware.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,22 +160,32 @@ private function resultArray($value)

private function exceptionArray($e)
{
if (!($e instanceof AwsException)) {
if (!($e instanceof \Exception)) {
return $e;
}

return [
$result = [
'instance' => spl_object_hash($e),
'class' => get_class($e),
'message' => $e->getMessage(),
'type' => $e->getAwsErrorType(),
'code' => $e->getAwsErrorCode(),
'requestId' => $e->getAwsRequestId(),
'statusCode' => $e->getStatusCode(),
'result' => $this->resultArray($e->getResult()),
'request' => $this->requestArray($e->getRequest()),
'response' => $this->responseArray($e->getResponse())
'file' => $e->getFile(),
'line' => $e->getLine(),
'trace' => $e->getTraceAsString(),
];

if ($e instanceof AwsException) {
$result += [
'type' => $e->getAwsErrorType(),
'code' => $e->getAwsErrorCode(),
'requestId' => $e->getAwsRequestId(),
'statusCode' => $e->getStatusCode(),
'result' => $this->resultArray($e->getResult()),
'request' => $this->requestArray($e->getRequest()),
'response' => $this->responseArray($e->getResponse()),
];
}

return $result;
}

private function compareArray($a, $b, $path, array &$diff)
Expand Down Expand Up @@ -207,11 +217,13 @@ private function str($value)
{
if (is_scalar($value)) {
return (string) $value;
} else {
ob_start();
var_dump($value);
return ob_get_clean();
} elseif ($value instanceof \Exception) {
$value = $this->exceptionArray($value);
}

ob_start();
var_dump($value);
return ob_get_clean();
}

private function streamStr(StreamInterface $body)
Expand Down
15 changes: 15 additions & 0 deletions tests/Signature/SignatureV4Test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,21 @@ public function testEnsuresContentSha256CanBeCalculated()
$signature->signRequest($request, $credentials);
}

/**
* @expectedException \Aws\Exception\CouldNotCreateChecksumException
*/
public function testEnsuresContentSha256CanBeCalculatedWhenSeekFails()
{
list($request, $credentials, $signature) = $this->getFixtures();
$stream = Psr7\FnStream::decorate(Psr7\stream_for('foo'), [
'seek' => function () {
throw new \Exception('Could not seek');
}
]);
$request = $request->withBody($stream);
$signature->signRequest($request, $credentials);
}

public function testProvider()
{
return [
Expand Down
36 changes: 36 additions & 0 deletions tests/TraceMiddlewareTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,38 @@ public function testEmitsDebugInfo()
}

public function testTracksExceptions()
{
$str = '';
$logfn = function ($value) use (&$str) { $str .= $value; };
$list = new HandlerList();
$list->setHandler(function ($cmd, $req) {
return \GuzzleHttp\Promise\promise_for(new Result());
});
$list->appendInit(function ($handler) {
return function ($cmd, $req = null) use ($handler) {
$req = $req->withHeader('foo', 'bar');
return $handler($cmd, $req);
};
});
$list->appendValidate(function ($handler) {
return function ($cmd, $req = null) use ($handler) {
return new RejectedPromise(new \Exception('Oh no!'));
};
});
$list->interpose(new TraceMiddleware(['logfn' => $logfn]));
$handler = $list->resolve();
$command = new Command('foo');
$request = new Request('GET', 'http://foo.com');
$handler($command, $request);
Promise\queue()->run();
$this->assertContains('error was set to array', $str);
$this->assertContains('trace', $str);
$this->assertContains('class', $str);
$this->assertContains('message', $str);
$this->assertContains('string(6) "Oh no!"', $str);
}

public function testTracksAwsSpecificExceptions()
{
$str = '';
$logfn = function ($value) use (&$str) { $str .= $value; };
Expand Down Expand Up @@ -85,6 +117,10 @@ public function testTracksExceptions()
$handler($command, $request);
Promise\queue()->run();
$this->assertContains('error was set to array', $str);
$this->assertContains('trace', $str);
$this->assertContains('class', $str);
$this->assertContains('message', $str);
$this->assertContains('string(5) "error"', $str);
}

public function testScrubsAuthStrings()
Expand Down

0 comments on commit 162e8f8

Please sign in to comment.