Implement stream wrapper for gs:// #323

chingor13 · 2017-01-24T21:26:37Z

This implementation creates 2 new classes:

Uploader\StreamableUploader - a special case of ResumableUploader where we do not know the full length of the document we're trying to upload. It uses an in memory buffer to collect data to the point where we can write a chunk to the server using the resumable upload API.
Storage\StreamWrapper - implements the callbacks necessary for a stream_wrapper. It delegates read operations to a GuzzleHttp stream and write operations to the StreamableUploader.

Note: this will be squashed when it's ready.

Add StreamWrapper registration instructions to README. Add ability to register/unregister the StreamWrapper. first stab at a StreamingUploader class Refactor uploader Add bucket->getStreamableUploader() Can pass storage options to the uploader and downloader streams via the stream context move register functions into the Storage namespace, autoloaded by composer add StreamingUploaderTest cannot lazy load because we need to know if the file open succeeded rename StreamingUploader to StreamableUploader to match convention Add test for StreamableUploader to test batching Add documentation for Bucket#getStreamableUploader with example. Adding tests for StreamWrapper with the most common use cases. Fix README with the correct usage

The method names need to be underscored instead of camel case because they are the callback methods needed to implement a stream wrapper.

jdpedrie

Nice work so far!

src/Storage/functions.php

+    $protocol = $protocol ?: 'gs';
+    if (!in_array($protocol, stream_get_wrappers())) {
+        stream_wrapper_register($protocol, 'Google\Cloud\Storage\StreamWrapper')
+            or die("Failed to register '$protocol://' protocol");


src/Storage/functions.php

+{
+    $protocol = $protocol ?: 'gs';
+    if (!in_array($protocol, stream_get_wrappers())) {
+        stream_wrapper_register($protocol, 'Google\Cloud\Storage\StreamWrapper')


src/Storage/functions.php

@@ -0,0 +1,17 @@
+<?php


src/Storage/StreamWrapper.php

+        $this->file = substr($url['path'], 1);
+        $this->mode = $mode;
+
+        $client = $this->getOption('client') ?: new StorageClient();


src/Upload/StreamableUploader.php

+{
+    const DEFAULT_WRITE_CHUNK_SIZE = 262144;
+
+    public function __construct(...$args)


…p 5.5

…to register

Add a test for overriding the StorageClient in a stream wrapper via the context.

src/Storage/StorageClient.php

+     *        'gs'.
+     * @throws \RuntimeException
+     */
+    public static function registerStreamWrapper(string $protocol = null)


src/Storage/StorageClient.php

+     */
+    public static function getDefaultClient()
+    {
+        return self::$defaultClient;


src/Upload/StreamableUploader.php

+        try {
+            $response = $this->requestWrapper->send($request, $this->requestOptions);
+        } catch (GoogleException $ex) {
+            throw new GoogleException(


src/Upload/StreamableUploader.php

+
+        try {
+            $response = $this->requestWrapper->send($request, $this->requestOptions);
+        } catch (GoogleException $ex) {


…iable.

Enables handling streams with that StorageClient instance as the default StorageClient.

…treaming upload fails

…am handler callback functions.

bshaffer

I would prefer we not use static methods here - The storage client should be instantiated first, so that authentication can be set normally, i.e.

$storage = new StorageClient([
    'projectId' => $projectId,
    // any other configuration options
]);
$storage->registerStreamWrapper();

src/Storage/StorageClient.php

@@ -217,4 +217,32 @@ public function createBucket($name, array $options = [])
        $response = $this->connection->insertBucket($options + ['name' => $name, 'project' => $this->projectId]);
        return new Bucket($this->connection, $name, $response);
    }
+
+    /**
+     * Register a this StorageClient as the handler for stream reading/writing.


README.md

+require 'vendor/autoload.php';
+
+use Google\Cloud\Storage\StorageClient;
+StorageClient::registerStreamWrapper();


In hhvm, you cannot get options from a null context.

dwsupplee

Everything is coming along nicely :). Great work 👍

src/Storage/StorageClient.php

+     *        'gs'.
+     * @throws \RuntimeException
+     */
+    public function registerAsStreamWrapper(string $protocol = null)


src/Storage/StorageClient.php

+     */
+    public function registerAsStreamWrapper(string $protocol = null)
+    {
+        if (StreamWrapper::register($protocol)) {


src/Storage/StreamWrapper.php

+        $protocol = $protocol ?: 'gs';
+        if (!in_array($protocol, stream_get_wrappers())) {
+            if (!stream_wrapper_register($protocol, StreamWrapper::class)) {
+                throw new RuntimeException("Failed to register '$protocol://' protocol");


src/Storage/StreamWrapper.php

+     *        'gs'.
+     * @throws \RuntimeException
+     */
+    public static function register(string $protocol = null)


src/Storage/StreamWrapper.php

+            );
+        } elseif ($this->isReadable()) {
+            try {
+                $this->stream = $this->bucket->object($this->file)->downloadAsStream($this->getOptions());


src/Upload/StreamableUploader.php

+ * Uploader that is a special case of the ResumableUploader where we can write
+ * the file contents in a streaming manner.
+ */
+class StreamableUploader extends ResumableUploader


src/Upload/StreamableUploader.php

+     */
+    public function __construct()
+    {
+        call_user_func_array(array($this, 'parent::__construct'), func_get_args());


src/Upload/StreamableUploader.php

+     */
+    private function resetBuffer($data = "")
+    {
+        $this->buffer = new BufferStream($this->chunkSize);


src/Upload/StreamableUploader.php

+     * Write some partial data. If there's enough data to send a chunk,
+     * then we will send a chunk. Any remaining data is sent on close.
+     *
+     * @param mixed $data The data being written. Can be a string or stream.


src/Storage/StreamWrapper.php

+    // Must be public according to the PHP documentation
+    public $context;
+
+    // a GuzzleHttp\Psr7\StreamInterface instance


…m wrapper

jdpedrie

Looks good! Just a couple non-code things noted. David should still review before merge. He has deeper knowledge of Storage than I do.

src/Storage/StorageClient.php

+    /**
+     * Registers this StorageClient as the handler for stream reading/writing.
+     *
+     * @param string $protocol The name of the protocol to use. Defaults to


src/Storage/StreamWrapper.php

+
+    private $protocol;
+    private $bucket;
+    private $file;


src/Storage/StreamWrapper.php

+        return true;
+    }
+
+    private function returnError($message, $flags)


jdpedrie · 2017-02-16T15:47:13Z

One other thing... rather than all these @codingStandardsIgnoreStart and @codingStandardsIgnoreEnd annotations, let's just modify the ruleset to exclude that rule from being applied to StreamWrapper.php.

./ruleset.xml

<?xml version="1.0"?>
<ruleset name="Google-Cloud-PHP-PSR2">
  <rule ref="PSR2" />
  <rule ref="Generic.Files.LineLength">
    <exclude-pattern>src/*/V[0-9]+</exclude-pattern>
  </rule>
  <rule ref="PSR1.Methods.CamelCapsMethodName">
    <exclude-pattern>src/Storage/StreamWrapper.php</exclude-pattern>
  </rule>
  <file>src</file>
</ruleset>

Ignore the camel case method name rule for StreamWrapper as the required callback method names are snake cased.

tmatsuo · 2017-02-17T19:34:13Z

@jdpedrie Thanks!

@dwsupplee Any chance you can take a look? I'd like to publish our WordPress plugin before Next ;)

dwsupplee · 2017-02-17T20:25:09Z

Absolutely! Sorry for the delay. I'll do a final once over today :).

dwsupplee

Thanks for all your hard work on this so far 👊 . I tested the code against quite a few scenarios lats night and it held up very well.

Outside of the comments, LGTM!

src/Storage/Bucket.php

+     * Example:
+     * ```
+     * $uploader = $bucket->getStreamableUploader(
+     *     "initial contents",


src/Storage/Bucket.php

+     *     ['name' => 'data.txt']
+     * );
+     *
+     * $uploader->write('some line');


src/Storage/Bucket.php

+     *           applied using md5 hashing functionality. If true and the
+     *           calculated hash does not match that of the upstream server the
+     *           upload will be rejected.
+     *     @type int $chunkSize If provided the upload will be done in chunks.


src/Storage/Bucket.php

+     *           for best performance it is recommended to pass in a cached
+     *           version of the already calculated SHA.
+     * }
+     * @return StreamableUploader


src/Storage/Bucket.php

+     * @param  string $file Optional file to try to write.
+     * @return boolean
+     */
+    public function isWritable($file = null)


src/Storage/WriteStream.php

+     * @param  string|StreamInterface $data Data to write
+     * @return int The number of bytes written
+     */
+    public function write($data)


src/Upload/StreamableUploader.php

+    /**
+     * Triggers the upload process.
+     *
+     * @param bool $remainder If true, send the all the remaining data and close


tests/system/Storage/StreamWrapper/DirectoryTest.php

+
+/**
+ * @group storage
+ * @group streamWrapper


src/Storage/StreamWrapper.php

+     */
+    public function dir_rewinddir()
+    {
+        $this->directoryGenerator = $this->bucket->objects([


src/Storage/StreamWrapper.php

+    /**
+     * Callback handler for reading an entry from a directory handle.
+     *
+     * @return string


The mode int is extrapolated into one of publicRead, projectPrivate, or private. We will create a bucket if no path is given (i.e. gs://bucket-name-only/') or if the STREAM_MKDIR_RECURSIVE flag is provided.

Any other ServiceException will be re-thrown. Also added some unit tests to account for this behavior.

chingor13 · 2017-02-22T01:05:32Z

The travis runs appear to be borked and I don't have access to kick the jobs.

dwsupplee · 2017-02-22T02:13:20Z

I cancelled the queued builds except for the latest. That should help speed things up, but we may be at the mercy of other builds running in the org.

dwsupplee · 2017-02-22T17:43:00Z

@chingor13 is there anything else you would like to touch on before we merge?

chingor13 · 2017-02-22T17:57:35Z

@dwsupplee I think it's good to go :)

dwsupplee · 2017-02-22T18:15:17Z

Awesome! Thanks so much for this.

jdpedrie · 2017-02-22T18:38:35Z

Great work guys!

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 24, 2017

chingor13 added 3 commits January 24, 2017 13:42

Fix php style guideline errors

5bff096

Fix the remaining php coding standard errors.

40ae470

The method names need to be underscored instead of camel case because they are the callback methods needed to implement a stream wrapper.

Fix php 5.6 errors

a9a5d08

jdpedrie suggested changes Jan 24, 2017

View reviewed changes

chingor13 added 6 commits January 24, 2017 14:31

Fix travis errors

9f109ce

Remove variadic from StreamableUploader. It is not compatible with ph…

e596ca9

…p 5.5

Throw a RuntimeException instead of dying if the StreamWrapper fails …

1e8f14c

…to register

Fix throwing of RuntimeException

9e41cc3

Move Storage namespaced functions to static methods on StorageClient.

4dc989c

Use static functions on StorageClient to get/set the default client.

f8a3f19

Add a test for overriding the StorageClient in a stream wrapper via the context.

jdpedrie reviewed Jan 25, 2017

View reviewed changes

chingor13 added 8 commits January 25, 2017 11:52

StreamWrapper will maintain its default StorageClient as a static var…

45a4641

…iable.

Implement StorageClient#registerAsStreamWrapper().

77d0d6e

Enables handling streams with that StorageClient instance as the default StorageClient.

Add documentation do StreamWrapper public functions

09a4ffe

Catch ServiceExceptions instead of all GoogleExceptions

18be73d

Pass the original exception to the re-raised GoogleException when a s…

41fcb84

…treaming upload fails

Fix stream wrapper checking array key for options

2456d51

check array_key_exists on reading options

435cc1e

move codingStandardsIgnoreStart before the documentation for the stre…

ba65eec

…am handler callback functions.

bshaffer reviewed Jan 25, 2017

View reviewed changes

chingor13 added 3 commits January 25, 2017 14:15

Fix documentation on registering a StorageClient as a stream wrapper.

819e8b9

Fix building of docs for StreamWrapper class

0c6944c

Fix StreamWrapper loading of options from stream context in hhvm.

d86565b

In hhvm, you cannot get options from a null context.

dwsupplee reviewed Jan 26, 2017

View reviewed changes

chingor13 added 4 commits January 26, 2017 12:35

Register the StorageClient with the protocol when registering a strea…

52da349

…m wrapper

Fix documentation for StreamWrapper getClient() and register()

0d7c111

Fix RuntimeException namespace

afa9463

Remove getOption() which is no longer needed

54331aa

tmatsuo approved these changes Feb 16, 2017

View reviewed changes

jdpedrie approved these changes Feb 16, 2017

View reviewed changes

chingor13 added 3 commits February 16, 2017 10:05

Documentation updates. Group private functions together.

67f19f0

Add custom phpcs ruleset.

9d2cfd4

Ignore the camel case method name rule for StreamWrapper as the required callback method names are snake cased.

Merge remote-tracking branch 'upstream/master' into gcs_stream_wrapper

c1b5371

dwsupplee reviewed Feb 18, 2017

View reviewed changes

chingor13 added 10 commits February 21, 2017 09:48

Updating docs for comments

2fb8bb6

ReadStream documentation updates

2b78220

Handle Service exceptions when listing directories

7591391

rmdir now will attempt to delete the bucket if the path is '/'

f66c80a

Fix redefinition of method

201d95f

Put the rename function back which was accidentally stomped

c2e8375

StreamWrapper can create the bucket and respect the specified mode.

4826581

The mode int is extrapolated into one of publicRead, projectPrivate, or private. We will create a bucket if no path is given (i.e. gs://bucket-name-only/') or if the STREAM_MKDIR_RECURSIVE flag is provided.

Handle service exceptions when renaming files/directories

1b2654c

Can specify the chunkSize of the streaming upload via stream context.

7bd960c

Bucket->isWritable will now expect a 403 access denied.

937541e

Any other ServiceException will be re-thrown. Also added some unit tests to account for this behavior.

dwsupplee merged commit 4ac28cf into googleapis:master Feb 22, 2017

dwsupplee mentioned this pull request Feb 22, 2017

Support GCS stream wrapper #23

Closed

jdpedrie mentioned this pull request Feb 22, 2017

Prepare v0.21.0 #345

Merged

chingor13 deleted the gcs_stream_wrapper branch February 22, 2017 22:08

dwsupplee mentioned this pull request Jul 19, 2018

Checking gs bucket readability, fails when bucket is not writeable. #1182

Open

Implement stream wrapper for gs:// #323

Implement stream wrapper for gs:// #323

Conversation

chingor13 commented Jan 24, 2017 • edited Loading

jdpedrie left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

bshaffer left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

dwsupplee left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

jdpedrie left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

jdpedrie commented Feb 16, 2017

tmatsuo commented Feb 17, 2017

dwsupplee commented Feb 17, 2017

dwsupplee left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

chingor13 commented Feb 22, 2017

dwsupplee commented Feb 22, 2017

dwsupplee commented Feb 22, 2017 • edited Loading

chingor13 commented Feb 22, 2017

dwsupplee commented Feb 22, 2017

jdpedrie commented Feb 22, 2017

chingor13 commented Jan 24, 2017 •

edited

Loading

dwsupplee commented Feb 22, 2017 •

edited

Loading