Skip to content

Commit

Permalink
fix: fix is_dir() and scandir() behavior in stream wrapper (#2496)
Browse files Browse the repository at this point in the history 
* fix: fix is_dir() and scandir() handling in stream wrapper

* fix: don't yield empty results

* Update Storage/tests/Unit/StreamWrapperTest.php

Co-Authored-By: David Supplee <dwsupplee@gmail.com>
  • Loading branch information
@jdpedrie @dwsupplee
jdpedrie and dwsupplee committed Dec 11, 2019
1 parent b2c946f commit 67f0029
Show file tree
Hide file tree
Showing 9 changed files with 161 additions and 108 deletions.
124 changes: 68 additions & 56 deletions Storage/src/StreamWrapper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

namespace Google\Cloud\Storage;

use Google\Cloud\Core\Exception\NotFoundException;
use Google\Cloud\Core\Exception\ServiceException;
use Google\Cloud\Storage\Bucket;
use GuzzleHttp\Psr7\CachingStream;
Expand Down Expand Up @@ -310,10 +309,11 @@ public function dir_opendir($path, $options)
*/
public function dir_readdir()
{
$object = $this->directoryIterator->current();
if ($object) {
$name = $this->directoryIterator->current();
if ($name) {
$this->directoryIterator->next();
return $object->name();

return $name;
}

return false;
Expand All @@ -327,10 +327,33 @@ public function dir_readdir()
public function dir_rewinddir()
{
try {
$this->directoryIterator = $this->bucket->objects([
$iterator = $this->bucket->objects([
'prefix' => $this->file,
'fields' => 'items/name,nextPageToken'
]);

// The delimiter options do not give us what we need, so instead we
// list all results matching the given prefix, enumerate the
// iterator, filter and transform results, and yield a fresh
// generator containing only the directory listing.
$this->directoryIterator = call_user_func(function () use ($iterator) {
$yielded = [];
foreach ($iterator as $object) {
$name = $object->name();
$name = substr($object->name(), strlen($this->makeDirectory($this->file)));
$parts = explode('/', $name);

// since the service call returns nested results and we only
// want to yield results directly within the requested directory,
// check if we've already yielded this value.
if ($parts[0] === "" || in_array($parts[0], $yielded)) {
continue;
}

$yielded[] = $parts[0];
yield $name => $parts[0];
}
});
} catch (ServiceException $e) {
return false;
}
Expand Down Expand Up @@ -386,27 +409,32 @@ public function mkdir($path, $mode, $options)
*/
public function rename($from, $to)
{
$url = (array) parse_url($to) + [
$this->openPath($from);
$destination = (array) parse_url($to) + [
'path' => '',
'host' => ''
];

$destinationBucket = $url['host'];
$destinationPath = substr($url['path'], 1);
$destinationBucket = $destination['host'];
$destinationPath = substr($destination['path'], 1);

$this->dir_opendir($from, 0);
foreach ($this->directoryIterator as $file) {
$name = $file->name();
$newPath = str_replace($this->file, $destinationPath, $name);
// loop through to rename file and children, if given path is a directory.
$objects = $this->bucket->objects([
'prefix' => $this->file
]);

$obj = $this->bucket->object($name);
foreach ($objects as $obj) {
$oldName = $obj->name();
$newPath = str_replace($this->file, $destinationPath, $oldName);
try {
$obj->rename($newPath, ['destinationBucket' => $destinationBucket]);
$obj->rename($newPath, [
'destinationBucket' => $destinationBucket
]);
} catch (ServiceException $e) {
// If any rename calls fail, abort and return false
return false;
}
}

return true;
}

Expand Down Expand Up @@ -481,8 +509,9 @@ public function url_stat($path, $flags)
$client = $this->openPath($path);

// if directory
if ($this->isDirectory($this->file)) {
return $this->urlStatDirectory();
$dir = $this->getDirectoryInfo($this->file);
if ($dir) {
return $this->urlStatDirectory($dir);
}

return $this->urlStatFile();
Expand Down Expand Up @@ -528,47 +557,18 @@ private function makeDirectory($path)
*
* @return array|bool
*/
private function urlStatDirectory()
private function urlStatDirectory(StorageObject $object)
{
$stats = [];
// 1. try to look up the directory as a file
try {
$this->object = $this->bucket->object($this->file);
$info = $this->object->info();

// equivalent to 40777 and 40444 in octal
$stats['mode'] = $this->bucket->isWritable()
? self::DIRECTORY_WRITABLE_MODE
: self::DIRECTORY_READABLE_MODE;
$this->statsFromFileInfo($info, $stats);

return $this->makeStatArray($stats);
} catch (NotFoundException $e) {
} catch (ServiceException $e) {
return false;
}

// 2. try list files in that directory
try {
$objects = $this->bucket->objects([
'prefix' => $this->file,
]);

if (!$objects->current()) {
// can't list objects or doesn't exist
return false;
}
} catch (ServiceException $e) {
return false;
}
$info = $object->info();

// equivalent to 40777 and 40444 in octal
$mode = $this->bucket->isWritable()
$stats['mode'] = $this->bucket->isWritable()
? self::DIRECTORY_WRITABLE_MODE
: self::DIRECTORY_READABLE_MODE;
return $this->makeStatArray([
'mode' => $mode
]);
$this->statsFromFileInfo($info, $stats);

return $this->makeStatArray($stats);
}

/**
Expand Down Expand Up @@ -619,14 +619,26 @@ private function statsFromFileInfo(array &$info, array &$stats)
}

/**
* Return whether we think the provided path is a directory or not
* Get the given path as a directory.
*
* In list objects calls, directories are returned with a trailing slash. By
* providing the given path with a trailing slash as a list prefix, we can
* check whether the given path exists as a directory.
*
* If the path does not exist or is not a directory, return null.
*
* @param string $path
* @return bool
* @return StorageObject|null
*/
private function isDirectory($path)
private function getDirectoryInfo($path)
{
return substr($path, -1) == '/';
$scan = $this->bucket->objects([
'prefix' => $this->makeDirectory($path),
'resultLimit' => 1,
'fields' => 'items/name,items/size,items/updated,items/timeCreated,nextPageToken'
]);

return $scan->current();
}

/**
Expand Down
25 changes: 18 additions & 7 deletions Storage/tests/System/StreamWrapper/DirectoryTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@

/**
* @group storage
* @group streamWrapper
* @group storage-stream-wrapper
* @group storage-stream-wrapper-directory
*/
class DirectoryTest extends StreamWrapperTestCase
{
Expand All @@ -31,6 +32,7 @@ public static function setUpBeforeClass()
self::$bucket->upload('somedata', ['name' => 'some_folder/1.txt']);
self::$bucket->upload('somedata', ['name' => 'some_folder/2.txt']);
self::$bucket->upload('somedata', ['name' => 'some_folder/3.txt']);
self::$bucket->upload('somedata', ['name' => 'some_folder/nest/3.txt']);
}

public function testMkDir()
Expand All @@ -41,6 +43,14 @@ public function testMkDir()
$this->assertTrue(is_dir($dir . '/'));
}

public function testIsDir()
{
$dir = self::generateUrl('test_directory');
$this->assertTrue(mkdir($dir));
$this->assertFileExists($dir);
$this->assertTrue(is_dir($dir));
}

public function testRmDir()
{
$dir = self::generateUrl('test_directory/');
Expand All @@ -63,20 +73,21 @@ public function testListDirectory()
{
$dir = self::generateUrl('some_folder');
$fd = opendir($dir);
$this->assertEquals('some_folder/1.txt', readdir($fd));
$this->assertEquals('some_folder/2.txt', readdir($fd));
$this->assertEquals('1.txt', readdir($fd));
$this->assertEquals('2.txt', readdir($fd));
rewinddir($fd);
$this->assertEquals('some_folder/1.txt', readdir($fd));
$this->assertEquals('1.txt', readdir($fd));
closedir($fd);
}

public function testScanDirectory()
{
$dir = self::generateUrl('some_folder');
$expected = [
'some_folder/1.txt',
'some_folder/2.txt',
'some_folder/3.txt',
'1.txt',
'2.txt',
'3.txt',
'nest',
];
$this->assertEquals($expected, scandir($dir));
$this->assertEquals(array_reverse($expected), scandir($dir, SCANDIR_SORT_DESCENDING));
Expand Down
3 changes: 2 additions & 1 deletion Storage/tests/System/StreamWrapper/ImageTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@

/**
* @group storage
* @group streamWrapper
* @group storage-stream-wrapper
* @group storage-stream-wrapper-image
*/
class ImageTest extends StreamWrapperTestCase
{
Expand Down
3 changes: 2 additions & 1 deletion Storage/tests/System/StreamWrapper/ReadTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@

/**
* @group storage
* @group streamWrapper
* @group storage-stream-wrapper
* @group storage-stream-wrapper-read
*/
class ReadTest extends StreamWrapperTestCase
{
Expand Down
3 changes: 2 additions & 1 deletion Storage/tests/System/StreamWrapper/RenameTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@

/**
* @group storage
* @group streamWrapper
* @group storage-stream-wrapper
* @group storage-stream-wrapper-rename
*/
class RenameTest extends StreamWrapperTestCase
{
Expand Down
2 changes: 1 addition & 1 deletion Storage/tests/System/StreamWrapper/StreamWrapperTestCase.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

/**
* @group storage
* @group streamWrapper
* @group storage-stream-wrapper
*/
class StreamWrapperTestCase extends StorageTestCase
{
Expand Down
3 changes: 2 additions & 1 deletion Storage/tests/System/StreamWrapper/UrlStatTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@

/**
* @group storage
* @group streamWrapper
* @group storage-stream-wrapper
* @group storage-stream-wrapper-urlstat
*/
class UrlStatTest extends StreamWrapperTestCase
{
Expand Down
3 changes: 2 additions & 1 deletion Storage/tests/System/StreamWrapper/WriteTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@

/**
* @group storage
* @group streamWrapper
* @group storage-stream-wrapper
* @group storage-stream-wrapper-write
*/
class WriteTest extends StreamWrapperTestCase
{
Expand Down
Loading

0 comments on commit 67f0029

Please sign in to comment.