From fed86e8382acb84b81e75e43fa9318700d5502ae Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Sun, 16 Feb 2020 01:10:39 +0100 Subject: [PATCH 1/4] better tests for SimpleFolder test behavior, not implementation Signed-off-by: Robin Appelman --- tests/lib/Files/SimpleFS/SimpleFolderTest.php | 119 +++++++----------- 1 file changed, 48 insertions(+), 71 deletions(-) diff --git a/tests/lib/Files/SimpleFS/SimpleFolderTest.php b/tests/lib/Files/SimpleFS/SimpleFolderTest.php index 9dcca32090f61..b902cac77cc2b 100644 --- a/tests/lib/Files/SimpleFS/SimpleFolderTest.php +++ b/tests/lib/Files/SimpleFS/SimpleFolderTest.php @@ -24,116 +24,93 @@ namespace Test\File\SimpleFS; use OC\Files\SimpleFS\SimpleFolder; +use OC\Files\Storage\Temporary; +use OC\Files\View; use OCP\Files\File; use OCP\Files\Folder; use OCP\Files\Node; use OCP\Files\NotFoundException; use OCP\Files\SimpleFS\ISimpleFile; +use Test\Traits\MountProviderTrait; +use Test\Traits\UserTrait; -class SimpleFolderTest extends \Test\TestCase { - /** @var Folder|\PHPUnit_Framework_MockObject_MockObject */ +/** + * @group DB + */ +class SimpleFolderTest extends \Test\TestCase { + use MountProviderTrait; + use UserTrait; + + /** @var Folder */ private $folder; + /** @var Folder */ + private $parentFolder; + /** @var SimpleFolder */ private $simpleFolder; + private $storage; + protected function setUp(): void { parent::setUp(); - $this->folder = $this->createMock(Folder::class); + $this->storage = new Temporary([]); + $this->createUser('simple', 'simple'); + $this->registerMount('simple', $this->storage, '/simple/files'); + $this->loginAsUser('simple'); + + $this->parentFolder = \OC::$server->getUserFolder('simple'); + + $this->folder = $this->parentFolder->newFolder('test'); $this->simpleFolder = new SimpleFolder($this->folder); } public function testGetName() { - $this->folder->expects($this->once()) - ->method('getName') - ->willReturn('myname'); - - $this->assertEquals('myname', $this->simpleFolder->getName()); + $this->assertEquals('test', $this->simpleFolder->getName()); } public function testDelete() { - $this->folder->expects($this->once()) - ->method('delete'); - + $this->assertTrue($this->parentFolder->nodeExists('test')); $this->simpleFolder->delete(); + $this->assertFalse($this->parentFolder->nodeExists('test')); } - public function dataFileExists() { - return [ - [true], - [false], - ]; - } + public function testFileExists() { + $this->folder->newFile('exists'); - /** - * @dataProvider dataFileExists - * @param bool $exists - */ - public function testFileExists($exists) { - $this->folder->expects($this->once()) - ->method('nodeExists') - ->with($this->equalTo('file')) - ->willReturn($exists); - - $this->assertEquals($exists, $this->simpleFolder->fileExists('file')); + $this->assertFalse($this->simpleFolder->fileExists('not-exists')); + $this->assertTrue($this->simpleFolder->fileExists('exists')); } - public function dataGetFile() { - return [ - [File::class, false], - [Folder::class, true], - [Node::class, true], - ]; - } + public function testGetFile() { + $this->folder->newFile('exists'); - /** - * @dataProvider dataGetFile - * @param string $class - * @param bool $exception - */ - public function testGetFile($class, $exception) { - $node = $this->createMock($class); - - $this->folder->expects($this->once()) - ->method('get') - ->with($this->equalTo('file')) - ->willReturn($node); - - try { - $result = $this->simpleFolder->getFile('file'); - $this->assertFalse($exception); - $this->assertInstanceOf(ISimpleFile::class, $result); - } catch (NotFoundException $e) { - $this->assertTrue($exception); - } + $result = $this->simpleFolder->getFile('exists'); + $this->assertInstanceOf(ISimpleFile::class, $result); + + $this->expectException(NotFoundException::class); + $this->simpleFolder->getFile('not-exists'); } public function testNewFile() { - $file = $this->createMock(File::class); - - $this->folder->expects($this->once()) - ->method('newFile') - ->with($this->equalTo('file')) - ->willReturn($file); - $result = $this->simpleFolder->newFile('file'); $this->assertInstanceOf(ISimpleFile::class, $result); + $this->assertFalse($this->folder->nodeExists('file')); + $result->putContent('bar'); + + $this->assertTrue($this->folder->nodeExists('file')); + $this->assertEquals('bar', $result->getContent()); } public function testGetDirectoryListing() { - $file = $this->createMock(File::class); - $folder = $this->createMock(Folder::class); - $node = $this->createMock(Node::class); - - $this->folder->expects($this->once()) - ->method('getDirectoryListing') - ->willReturn([$file, $folder, $node]); + $this->folder->newFile('file1'); + $this->folder->newFile('file2'); $result = $this->simpleFolder->getDirectoryListing(); - - $this->assertCount(1, $result); + $this->assertCount(2, $result); $this->assertInstanceOf(ISimpleFile::class, $result[0]); + $this->assertInstanceOf(ISimpleFile::class, $result[1]); } } From 5ca1929e8cbc9beb32783a769e5a127527c1d93e Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Sun, 16 Feb 2020 01:14:52 +0100 Subject: [PATCH 2/4] Create SimpleFile only when writing the content instead of first creating an empty file and then writing the content. This solves the overhead of creating an empty file with the common pattern: ```php $file = $simpleFilder->newFile('foo.txt'); $file->putContent('bar.txt'); ``` roughly halving the number of storage and database operations that need to be done when creating a `SimpleFile`. This is not automatically done with `File` because that has a more complex api which I'm more hesitant to touch. Instead the `Folder::newFile` api has been extended to accept the content for the new file. In my local testing, the overhead of first creating an empty file took about 20% of the time for preview generation Signed-off-by: Robin Appelman --- lib/private/Files/Node/Folder.php | 10 +- lib/private/Files/Node/LazyRoot.php | 2 +- lib/private/Files/Node/NonExistingFolder.php | 2 +- lib/private/Files/SimpleFS/NewSimpleFile.php | 221 +++++++++++++++++++ lib/private/Files/SimpleFS/SimpleFolder.php | 5 +- lib/public/Files/Folder.php | 3 +- 6 files changed, 235 insertions(+), 8 deletions(-) create mode 100644 lib/private/Files/SimpleFS/NewSimpleFile.php diff --git a/lib/private/Files/Node/Folder.php b/lib/private/Files/Node/Folder.php index 1267c81866834..727b08e9335f7 100644 --- a/lib/private/Files/Node/Folder.php +++ b/lib/private/Files/Node/Folder.php @@ -173,15 +173,21 @@ public function newFolder($path) { /** * @param string $path + * @param string | resource | null $content * @return \OC\Files\Node\File * @throws \OCP\Files\NotPermittedException */ - public function newFile($path) { + public function newFile($path, $content = null) { if ($this->checkPermissions(\OCP\Constants::PERMISSION_CREATE)) { $fullPath = $this->getFullPath($path); $nonExisting = new NonExistingFile($this->root, $this->view, $fullPath); $this->sendHooks(['preWrite', 'preCreate'], [$nonExisting]); - if (!$this->view->touch($fullPath)) { + if ($content !== null) { + $result = $this->view->file_put_contents($fullPath, $content); + } else { + $result = $this->view->touch($fullPath); + } + if (!$result) { throw new NotPermittedException('Could not create path'); } $node = new File($this->root, $this->view, $fullPath); diff --git a/lib/private/Files/Node/LazyRoot.php b/lib/private/Files/Node/LazyRoot.php index 76868cfa5cdb3..8076c3b4f6abe 100644 --- a/lib/private/Files/Node/LazyRoot.php +++ b/lib/private/Files/Node/LazyRoot.php @@ -394,7 +394,7 @@ public function newFolder($path) { /** * @inheritDoc */ - public function newFile($path) { + public function newFile($path, $content = null) { return $this->__call(__FUNCTION__, func_get_args()); } diff --git a/lib/private/Files/Node/NonExistingFolder.php b/lib/private/Files/Node/NonExistingFolder.php index 30470740495ca..65af837da43bd 100644 --- a/lib/private/Files/Node/NonExistingFolder.php +++ b/lib/private/Files/Node/NonExistingFolder.php @@ -139,7 +139,7 @@ public function newFolder($path) { throw new NotFoundException(); } - public function newFile($path) { + public function newFile($path, $content = null) { throw new NotFoundException(); } diff --git a/lib/private/Files/SimpleFS/NewSimpleFile.php b/lib/private/Files/SimpleFS/NewSimpleFile.php new file mode 100644 index 0000000000000..02dde1d2f883d --- /dev/null +++ b/lib/private/Files/SimpleFS/NewSimpleFile.php @@ -0,0 +1,221 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OC\Files\SimpleFS; + +use Icewind\Streams\CallbackWrapper; +use OCP\Files\File; +use OCP\Files\Folder; +use OCP\Files\NotFoundException; +use OCP\Files\NotPermittedException; +use OCP\Files\SimpleFS\ISimpleFile; + +class NewSimpleFile implements ISimpleFile { + private $parentFolder; + private $name; + /** @var File|null */ + private $file = null; + + /** + * File constructor. + * + * @param File $file + */ + public function __construct(Folder $parentFolder, string $name) { + $this->parentFolder = $parentFolder; + $this->name = $name; + } + + /** + * Get the name + * + * @return string + */ + public function getName() { + return $this->name; + } + + /** + * Get the size in bytes + * + * @return int + */ + public function getSize() { + if ($this->file) { + return $this->file->getSize(); + } else { + return 0; + } + } + + /** + * Get the ETag + * + * @return string + */ + public function getETag() { + if ($this->file) { + return $this->file->getEtag(); + } else { + return ''; + } + } + + /** + * Get the last modification time + * + * @return int + */ + public function getMTime() { + if ($this->file) { + return $this->file->getMTime(); + } else { + return time(); + } + } + + /** + * Get the content + * + * @return string + * @throws NotFoundException + * @throws NotPermittedException + */ + public function getContent() { + if ($this->file) { + $result = $this->file->getContent(); + + if ($result === false) { + $this->checkFile(); + } + + return $result; + } else { + return ''; + } + } + + /** + * Overwrite the file + * + * @param string|resource $data + * @throws NotPermittedException + * @throws NotFoundException + */ + public function putContent($data) { + try { + if ($this->file) { + return $this->file->putContent($data); + } else { + $this->file = $this->parentFolder->newFile($this->name, $data); + } + } catch (NotFoundException $e) { + $this->checkFile(); + } + } + + /** + * Sometimes there are some issues with the AppData. Most of them are from + * user error. But we should handle them gracefull anyway. + * + * If for some reason the current file can't be found. We remove it. + * Then traverse up and check all folders if they exists. This so that the + * next request will have a valid appdata structure again. + * + * @throws NotFoundException + */ + private function checkFile() { + $cur = $this->file; + + while ($cur->stat() === false) { + $parent = $cur->getParent(); + try { + $cur->delete(); + } catch (NotFoundException $e) { + // Just continue then + } + $cur = $parent; + } + + if ($cur !== $this->file) { + throw new NotFoundException('File does not exist'); + } + } + + + /** + * Delete the file + * + * @throws NotPermittedException + */ + public function delete() { + if ($this->file) { + $this->file->delete(); + } + } + + /** + * Get the MimeType + * + * @return string + */ + public function getMimeType() { + if ($this->file) { + return $this->file->getMimeType(); + } else { + return 'text/plain'; + } + } + + /** + * Open the file as stream for reading, resulting resource can be operated as stream like the result from php's own fopen + * + * @return resource + * @throws \OCP\Files\NotPermittedException + * @since 14.0.0 + */ + public function read() { + if ($this->file) { + return $this->file->fopen('r'); + } else { + return fopen('php://temp', 'r'); + } + } + + /** + * Open the file as stream for writing, resulting resource can be operated as stream like the result from php's own fopen + * + * @return resource + * @throws \OCP\Files\NotPermittedException + * @since 14.0.0 + */ + public function write() { + if ($this->file) { + return $this->file->fopen('w'); + } else { + $source = fopen('php://temp', 'w+'); + return CallbackWrapper::wrap($source, null, null, null, null, function () use ($source) { + rewind($source); + $this->putContent($source); + }); + } + } +} diff --git a/lib/private/Files/SimpleFS/SimpleFolder.php b/lib/private/Files/SimpleFS/SimpleFolder.php index 833aa7d76cfd9..76f6a198e2520 100644 --- a/lib/private/Files/SimpleFS/SimpleFolder.php +++ b/lib/private/Files/SimpleFS/SimpleFolder.php @@ -81,8 +81,7 @@ public function getFile($name) { } public function newFile($name) { - $file = $this->folder->newFile($name); - - return new SimpleFile($file); + // delay creating the file until it's written to + return new NewSimpleFile($this->folder, $name); } } diff --git a/lib/public/Files/Folder.php b/lib/public/Files/Folder.php index 99d331850cfee..e7286ea028c32 100644 --- a/lib/public/Files/Folder.php +++ b/lib/public/Files/Folder.php @@ -109,11 +109,12 @@ public function newFolder($path); * Create a new file * * @param string $path relative path of the new file + * @param string | resource | null $content content for the new file, since 19.0.0 * @return \OCP\Files\File * @throws \OCP\Files\NotPermittedException * @since 6.0.0 */ - public function newFile($path); + public function newFile($path, $content = null); /** * search for files with the name matching $query From 63608ef461eef289cfc97d1eb5ff63aac3848bea Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 18 Feb 2020 18:02:58 +0100 Subject: [PATCH 3/4] allow writing content directly when creating new SimpleFile Signed-off-by: Robin Appelman --- lib/private/Files/SimpleFS/NewSimpleFile.php | 2 +- lib/private/Files/SimpleFS/SimpleFolder.php | 11 ++++++++--- lib/public/Files/Folder.php | 2 +- lib/public/Files/SimpleFS/ISimpleFolder.php | 3 ++- 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/lib/private/Files/SimpleFS/NewSimpleFile.php b/lib/private/Files/SimpleFS/NewSimpleFile.php index 02dde1d2f883d..2c74a16fa9221 100644 --- a/lib/private/Files/SimpleFS/NewSimpleFile.php +++ b/lib/private/Files/SimpleFS/NewSimpleFile.php @@ -123,7 +123,7 @@ public function getContent() { public function putContent($data) { try { if ($this->file) { - return $this->file->putContent($data); + $this->file->putContent($data); } else { $this->file = $this->parentFolder->newFile($this->name, $data); } diff --git a/lib/private/Files/SimpleFS/SimpleFolder.php b/lib/private/Files/SimpleFS/SimpleFolder.php index 76f6a198e2520..a4ebc6b4e539d 100644 --- a/lib/private/Files/SimpleFS/SimpleFolder.php +++ b/lib/private/Files/SimpleFS/SimpleFolder.php @@ -80,8 +80,13 @@ public function getFile($name) { return new SimpleFile($file); } - public function newFile($name) { - // delay creating the file until it's written to - return new NewSimpleFile($this->folder, $name); + public function newFile($name, $content = null) { + if ($content === null) { + // delay creating the file until it's written to + return new NewSimpleFile($this->folder, $name); + } else { + $file = $this->folder->newFile($name, $content); + return new SimpleFile($file); + } } } diff --git a/lib/public/Files/Folder.php b/lib/public/Files/Folder.php index e7286ea028c32..a78a9ca8f78ad 100644 --- a/lib/public/Files/Folder.php +++ b/lib/public/Files/Folder.php @@ -109,7 +109,7 @@ public function newFolder($path); * Create a new file * * @param string $path relative path of the new file - * @param string | resource | null $content content for the new file, since 19.0.0 + * @param string|resource|null $content content for the new file, since 19.0.0 * @return \OCP\Files\File * @throws \OCP\Files\NotPermittedException * @since 6.0.0 diff --git a/lib/public/Files/SimpleFS/ISimpleFolder.php b/lib/public/Files/SimpleFS/ISimpleFolder.php index e3ef2bb48cd0e..22f8c90849b11 100644 --- a/lib/public/Files/SimpleFS/ISimpleFolder.php +++ b/lib/public/Files/SimpleFS/ISimpleFolder.php @@ -64,11 +64,12 @@ public function getFile($name); * Creates a new file with $name in the folder * * @param string $name + * @param string|resource|null $content @since 19.0.0 * @return ISimpleFile * @throws NotPermittedException * @since 11.0.0 */ - public function newFile($name); + public function newFile($name, $content = null); /** * Remove the folder and all the files in it From 245125d81b60eb29a6923878dd49c0fa0497321a Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 28 Feb 2020 12:55:35 +0100 Subject: [PATCH 4/4] Bump autoloader Signed-off-by: Roeland Jago Douma --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + 2 files changed, 2 insertions(+) diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 39c82330ab6b5..35cf377783144 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -985,6 +985,7 @@ 'OC\\Files\\Search\\SearchComparison' => $baseDir . '/lib/private/Files/Search/SearchComparison.php', 'OC\\Files\\Search\\SearchOrder' => $baseDir . '/lib/private/Files/Search/SearchOrder.php', 'OC\\Files\\Search\\SearchQuery' => $baseDir . '/lib/private/Files/Search/SearchQuery.php', + 'OC\\Files\\SimpleFS\\NewSimpleFile' => $baseDir . '/lib/private/Files/SimpleFS/NewSimpleFile.php', 'OC\\Files\\SimpleFS\\SimpleFile' => $baseDir . '/lib/private/Files/SimpleFS/SimpleFile.php', 'OC\\Files\\SimpleFS\\SimpleFolder' => $baseDir . '/lib/private/Files/SimpleFS/SimpleFolder.php', 'OC\\Files\\Storage\\Common' => $baseDir . '/lib/private/Files/Storage/Common.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index dd3b61ddf8795..95024ad3cbcf1 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1014,6 +1014,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Files\\Search\\SearchComparison' => __DIR__ . '/../../..' . '/lib/private/Files/Search/SearchComparison.php', 'OC\\Files\\Search\\SearchOrder' => __DIR__ . '/../../..' . '/lib/private/Files/Search/SearchOrder.php', 'OC\\Files\\Search\\SearchQuery' => __DIR__ . '/../../..' . '/lib/private/Files/Search/SearchQuery.php', + 'OC\\Files\\SimpleFS\\NewSimpleFile' => __DIR__ . '/../../..' . '/lib/private/Files/SimpleFS/NewSimpleFile.php', 'OC\\Files\\SimpleFS\\SimpleFile' => __DIR__ . '/../../..' . '/lib/private/Files/SimpleFS/SimpleFile.php', 'OC\\Files\\SimpleFS\\SimpleFolder' => __DIR__ . '/../../..' . '/lib/private/Files/SimpleFS/SimpleFolder.php', 'OC\\Files\\Storage\\Common' => __DIR__ . '/../../..' . '/lib/private/Files/Storage/Common.php',