Merge pull request #67 from demiankatz/fix-off-by-one

Fix off-by-one error in S3\Stream.
Close #64

CHANGELOG.md

@@ -24,4 +24,6 @@ All notable changes to this project will be documented in this file, in reverse
   missing use statement
 - [#49](https://github.com/zendframework/ZendService_Amazon/pull/49) fixed date
   format mismatch with signature in S3
+- [#67](https://github.com/zendframework/ZendService_Amazon/pull/67) fixed
+  of-by-one bug in S3 stream that was truncating and corrupting data
 

library/ZendService/Amazon/S3/Stream.php

@@ -172,7 +172,7 @@ public function stream_read($count)
         }
 
         $range_start = $this->_position;
-        $range_end = $this->_position+$count;
+        $range_end = $this->_position+$count-1;
 
         // Only fetch more data from S3 if we haven't fetched any data yet (postion=0)
         // OR, the range end position is greater than the size of the current object

tests/ZendService/Amazon/S3/OfflineStreamTest.php

@@ -0,0 +1,174 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link      http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2014 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license   http://framework.zend.com/license/new-bsd New BSD License
+ * @package   Zend_Service
+ */
+
+namespace ZendServiceTest\Amazon\S3;
+
+use Zend\Http\Response;
+use ZendService\Amazon\S3;
+
+/**
+ * @category   Zend
+ * @package    Zend_Service_Amazon_S3
+ * @subpackage UnitTests
+ * @group      Zend_Service
+ * @group      Zend_Service_Amazon
+ * @group      Zend_Service_Amazon_S3
+ */
+class OfflineStreamTest extends \PHPUnit_Framework_TestCase
+{
+    /**
+     * Setup a fake S3 object with a fake document inside it. Return the document
+     * so we can set expectations appropriately.
+     *
+     * @param int $fileSizeTimes256 Data size in multiples of 256 bytes.
+     * @param $rangeConstraint PHPUnit constraint for Range header value
+     *
+     * @return string
+     */
+    protected function setUpS3($fileSizeTimes256 = 10, $rangeConstraint = null)
+    {
+        // Mock the S3 client:
+        $s3 = $this->getMockBuilder('ZendService\Amazon\S3\S3')
+            ->disableOriginalConstructor()
+            ->setMethods(['getInfo', '_makeRequest', '_validBucketName'])
+            ->getMock();
+
+        // Simulate a 500-byte file
+        $file = str_repeat(
+            pack('C*', ...range(0, 255)) . pack('C*', ...range(255, 0, -1)),
+            $fileSizeTimes256
+        );
+        $fileSize = strlen($file);
+        $s3->expects($this->once())->method('getInfo')
+            ->will($this->returnValue(['size' => $fileSize]));
+
+        $callback = function ($a, $b, $c, $headers) use ($file, $rangeConstraint) {
+            if ($rangeConstraint) {
+                $rangeConstraint = $this->logicalAnd(
+                    $this->matches('bytes=%d-%d'),
+                    $rangeConstraint
+                );
+            } else {
+                $rangeConstraint = $this->matches('bytes=%d-%d');
+            }
+            $this->assertThat(
+                $headers['Range'],
+                $rangeConstraint
+            );
+            $range = explode('-', substr($headers['Range'], 6));
+            $startPos = (int) $range[0];
+            $endPos = (int) $range[1];
+            $this->assertGreaterThanOrEqual(
+                (int)$startPos,
+                (int)$endPos,
+                'Range header end position must be greater than or equal to start position'
+            );
+            $length =  $endPos - $startPos + 1;
+
+            // Simulate an appropriately-sized response to the current request
+            $response = new Response();
+            $response->setStatusCode(Response::STATUS_CODE_206);
+            $chunk = substr($file, (int) $startPos, $length);
+            $response->setContent($chunk);
+            return $response;
+        };
+
+
+        $s3->expects($this->any())->method('_makeRequest')
+            ->with(
+                $this->equalTo('GET'),
+                $this->anything(),
+                $this->equalTo(null),
+                $this->arrayHasKey('Range')
+            )->will($this->returnCallback($callback));
+
+        // Register the mock client so it is called for all test:// URLs:
+        $s3->registerAsClient('test');
+
+        return $file;
+    }
+
+    /**
+     * Test that the stream reader works in combination with fread().
+     *
+     * @return void
+     */
+    public function testFread()
+    {
+        // Set up the test document:
+        $document = $this->setUpS3(20);
+
+        // Run the test:
+        $stream = new S3\Stream();
+        stream_wrapper_register('test', get_class($stream));
+        $handle = fopen('test://foo/bar', 'r');
+        // Read document in two 500-byte chunks, make sure it is intact:
+        $final = '';
+        while ($chunk = fgets($handle)) {
+            $final .= $chunk;
+        }
+        fclose($handle);
+        // don't use equals, we don't want binary data output into terminal
+        $this->assertTrue(
+            $document === $final,
+            'Resulting data does not match original'
+        );
+    }
+
+    /**
+     * Test stream reading to make sure that appropriate byte ranges are requested
+     * in HTTP headers and that chunks are reassembled correctly. Obviously, being
+     * a mock-based test, this cannot confirm that Amazon responds appropriately
+     * to the HTTP request.
+     *
+     * @return void
+     */
+    public function testStreamRead()
+    {
+        // Set up the test document:
+        $document = $this->setUpS3(20);
+
+        // Run the test:
+        $stream = new S3\Stream();
+        $stream->stream_open('test://foo/bar', 'r', 0, null);
+        // Read document in 500-byte chunks, make sure it is intact:
+        $final = '';
+        while ($chunk = $stream->stream_read(500)) {
+            $final .= $chunk;
+        }
+        // don't use equals, we don't want binary data output into terminal
+        $this->assertTrue(
+            $document === $final,
+            'Resulting data does not match original'
+        );
+    }
+
+    /**
+     * Test correct range constraint on first chunk.
+     *
+     * @return void
+     */
+    public function testRangeConstraint()
+    {
+        // Set up the test document:
+        $document = $this->setUpS3(20, $this->equalTo('bytes=0-499'));
+
+        // Run the test:
+        $stream = new S3\Stream();
+        $stream->stream_open('test://foo/bar', 'r', 0, null);
+        // Read first 500-byte chunk:
+        $chunk = $stream->stream_read(500);
+        // don't use equals, we don't want binary data output into terminal
+        $this->assertTrue(
+            substr($document, 0, 500) === $chunk,
+            'Resulting data does not match original'
+        );
+    }
+}