From a98b038300fd4c4d64f4f9cb3268ba365382772a Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 18 Feb 2016 15:36:38 +0100 Subject: [PATCH 1/5] Query the cache when checking if a node exists --- lib/private/files/node/root.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/private/files/node/root.php b/lib/private/files/node/root.php index 0be7ee2c49..b5306eed8b 100644 --- a/lib/private/files/node/root.php +++ b/lib/private/files/node/root.php @@ -176,8 +176,9 @@ class Root extends Folder implements IRootFolder { $path = $this->normalizePath($path); if ($this->isValidPath($path)) { $fullPath = $this->getFullPath($path); - if ($this->view->file_exists($fullPath)) { - return $this->createNode($fullPath); + $fileInfo = $this->view->getFileInfo($fullPath); + if ($fileInfo) { + return $this->createNode($fullPath, $fileInfo); } else { throw new NotFoundException($path); } From 5e6c905a145d288127a08062a7a95e7577f5f4e2 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 18 Feb 2016 15:41:10 +0100 Subject: [PATCH 2/5] pass the fileinfo to the node if available --- lib/private/files/node/folder.php | 4 ++-- tests/lib/files/node/folder.php | 8 ++++---- tests/lib/files/node/root.php | 15 ++++----------- 3 files changed, 10 insertions(+), 17 deletions(-) diff --git a/lib/private/files/node/folder.php b/lib/private/files/node/folder.php index e8c49cd2c0..c376bfb188 100644 --- a/lib/private/files/node/folder.php +++ b/lib/private/files/node/folder.php @@ -100,9 +100,9 @@ class Folder extends Node implements \OCP\Files\Folder { $isDir = $info['mimetype'] === 'httpd/unix-directory'; } if ($isDir) { - return new Folder($this->root, $this->view, $path); + return new Folder($this->root, $this->view, $path, $info); } else { - return new File($this->root, $this->view, $path); + return new File($this->root, $this->view, $path, $info); } } diff --git a/tests/lib/files/node/folder.php b/tests/lib/files/node/folder.php index 09bf32561e..06ca35f7f1 100644 --- a/tests/lib/files/node/folder.php +++ b/tests/lib/files/node/folder.php @@ -601,8 +601,8 @@ class Folder extends \Test\TestCase { $cache = $this->getMock('\OC\Files\Cache\Cache', array(), array('')); $view->expects($this->once()) - ->method('file_exists') - ->will($this->returnValue(true)); + ->method('getFileInfo') + ->will($this->returnValue(new FileInfo('/bar/foo/qwerty', null, 'qwerty', [], null))); $storage->expects($this->once()) ->method('getCache') @@ -683,8 +683,8 @@ class Folder extends \Test\TestCase { $cache = $this->getMock('\OC\Files\Cache\Cache', array(), array('')); $view->expects($this->any()) - ->method('file_exists') - ->will($this->returnValue(true)); + ->method('getFileInfo') + ->will($this->returnValue(new FileInfo('/bar/foo/qwerty', null, 'qwerty', [], null))); $storage->expects($this->any()) ->method('getCache') diff --git a/tests/lib/files/node/root.php b/tests/lib/files/node/root.php index 4b1aea1da4..9c77959869 100644 --- a/tests/lib/files/node/root.php +++ b/tests/lib/files/node/root.php @@ -12,6 +12,9 @@ use OC\Files\FileInfo; use OCP\Files\NotPermittedException; use OC\Files\Mount\Manager; +/** + * @group DB + */ class Root extends \Test\TestCase { private $user; @@ -41,16 +44,6 @@ class Root extends \Test\TestCase { ->with('/bar/foo') ->will($this->returnValue($this->getFileInfo(array('fileid' => 10, 'path' => 'bar/foo', 'name', 'mimetype' => 'text/plain')))); - $view->expects($this->once()) - ->method('is_dir') - ->with('/bar/foo') - ->will($this->returnValue(false)); - - $view->expects($this->once()) - ->method('file_exists') - ->with('/bar/foo') - ->will($this->returnValue(true)); - $root->mount($storage, ''); $node = $root->get('/bar/foo'); $this->assertEquals(10, $node->getId()); @@ -73,7 +66,7 @@ class Root extends \Test\TestCase { $root = new \OC\Files\Node\Root($manager, $view, $this->user); $view->expects($this->once()) - ->method('file_exists') + ->method('getFileInfo') ->with('/bar/foo') ->will($this->returnValue(false)); From 6031ae1ad41cb39ac8783b4a95f98574c28c92f1 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 19 Feb 2016 14:27:55 +0100 Subject: [PATCH 3/5] improve reuse in getUserFolder --- lib/private/files/node/root.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/private/files/node/root.php b/lib/private/files/node/root.php index b5306eed8b..69c98368df 100644 --- a/lib/private/files/node/root.php +++ b/lib/private/files/node/root.php @@ -337,18 +337,18 @@ class Root extends Folder implements IRootFolder { $dir = '/' . $userId; $folder = null; - if (!$this->nodeExists($dir)) { - $folder = $this->newFolder($dir); - } else { + try { $folder = $this->get($dir); + } catch (NotFoundException $e) { + $folder = $this->newFolder($dir); } $dir = '/files'; - if (!$folder->nodeExists($dir)) { + try { + $folder = $folder->get($dir); + } catch (NotFoundException $e) { $folder = $folder->newFolder($dir); \OC_Util::copySkeleton($userId, $folder); - } else { - $folder = $folder->get($dir); } return $folder; From d0dd76bb8ac904cff425d4c32f2033951e3a2ba5 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 19 Feb 2016 14:29:01 +0100 Subject: [PATCH 4/5] set watch policy in test --- apps/files_trashbin/tests/trashbin.php | 33 +++++++++++++++++++------- apps/files_versions/tests/versions.php | 15 ++++++++++++ 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/apps/files_trashbin/tests/trashbin.php b/apps/files_trashbin/tests/trashbin.php index 930f8c2bf3..8d616b6e8e 100644 --- a/apps/files_trashbin/tests/trashbin.php +++ b/apps/files_trashbin/tests/trashbin.php @@ -86,11 +86,12 @@ class Test_Trashbin extends \Test\TestCase { } - public static function tearDownAfterClass() { // cleanup test user $user = \OC::$server->getUserManager()->get(self::TEST_TRASHBIN_USER1); - if ($user !== null) { $user->delete(); } + if ($user !== null) { + $user->delete(); + } \OC::$server->getConfig()->setSystemValue('trashbin_retention_obligation', self::$rememberRetentionObligation); @@ -109,6 +110,18 @@ class Test_Trashbin extends \Test\TestCase { parent::setUp(); \OC::$server->getAppManager()->enableApp('files_trashbin'); + $config = \OC::$server->getConfig(); + $mockConfig = $this->getMock('\OCP\IConfig'); + $mockConfig->expects($this->any()) + ->method('getSystemValue') + ->will($this->returnCallback(function ($key, $default) use ($config) { + if ($key === 'filesystem_check_changes') { + return \OC\Files\Cache\Watcher::CHECK_ONCE; + } else { + return $config->getSystemValue($key, $default); + } + })); + $this->overwriteService('AllConfig', $mockConfig); $this->trashRoot1 = '/' . self::TEST_TRASHBIN_USER1 . '/files_trashbin'; $this->trashRoot2 = '/' . self::TEST_TRASHBIN_USER2 . '/files_trashbin'; @@ -117,6 +130,7 @@ class Test_Trashbin extends \Test\TestCase { } protected function tearDown() { + $this->restoreService('AllConfig'); // disable trashbin to be able to properly clean up \OC::$server->getAppManager()->disableApp('files_trashbin'); @@ -138,8 +152,8 @@ class Test_Trashbin extends \Test\TestCase { public function testExpireOldFiles() { $currentTime = time(); - $expireAt = $currentTime - 2*24*60*60; - $expiredDate = $currentTime - 3*24*60*60; + $expireAt = $currentTime - 2 * 24 * 60 * 60; + $expiredDate = $currentTime - 3 * 24 * 60 * 60; // create some files \OC\Files\Filesystem::file_put_contents('file1.txt', 'file1'); @@ -187,7 +201,7 @@ class Test_Trashbin extends \Test\TestCase { $currentTime = time(); $folder = "trashTest-" . $currentTime . '/'; - $expiredDate = $currentTime - 3*24*60*60; + $expiredDate = $currentTime - 3 * 24 * 60 * 60; // create some files \OC\Files\Filesystem::mkdir($folder); @@ -250,6 +264,7 @@ class Test_Trashbin extends \Test\TestCase { /** * verify that the array contains the expected results + * * @param OCP\Files\FileInfo[] $result * @param string[] $expected */ @@ -265,7 +280,7 @@ class Test_Trashbin extends \Test\TestCase { } if (!$found) { // if we didn't found the expected file, something went wrong - $this->assertTrue(false, "can't find expected file '" . $expectedFile . "' in trash bin"); + $this->assertTrue(false, "can't find expected file '" . $expectedFile . "' in trash bin"); } } } @@ -281,7 +296,7 @@ class Test_Trashbin extends \Test\TestCase { // modify every second file $counter = ($counter + 1) % 2; if ($counter === 1) { - $source = $trashRoot . '/files/' . $file['name'].'.d'.$file['mtime']; + $source = $trashRoot . '/files/' . $file['name'] . '.d' . $file['mtime']; $target = \OC\Files\Filesystem::normalizePath($trashRoot . '/files/' . $file['name'] . '.d' . $expireDate); $this->rootView->rename($source, $target); $file['mtime'] = $expireDate; @@ -445,7 +460,7 @@ class Test_Trashbin extends \Test\TestCase { $trashedFile = $filesInTrash[0]; $this->assertTrue( - OCA\Files_Trashbin\Trashbin::restore( + OCA\Files_Trashbin\Trashbin::restore( 'folder.d' . $trashedFile->getMtime() . '/file1.txt', 'file1.txt', $trashedFile->getMtime() @@ -639,7 +654,7 @@ class Test_Trashbin extends \Test\TestCase { if ($create) { try { \OC::$server->getUserManager()->createUser($user, $user); - } catch(\Exception $e) { // catch username is already being used from previous aborted runs + } catch (\Exception $e) { // catch username is already being used from previous aborted runs } } diff --git a/apps/files_versions/tests/versions.php b/apps/files_versions/tests/versions.php index e82e65bf3a..f6658e092b 100644 --- a/apps/files_versions/tests/versions.php +++ b/apps/files_versions/tests/versions.php @@ -74,6 +74,19 @@ class Test_Files_Versioning extends \Test\TestCase { protected function setUp() { parent::setUp(); + $config = \OC::$server->getConfig(); + $mockConfig = $this->getMock('\OCP\IConfig'); + $mockConfig->expects($this->any()) + ->method('getSystemValue') + ->will($this->returnCallback(function ($key, $default) use ($config) { + if ($key === 'filesystem_check_changes') { + return \OC\Files\Cache\Watcher::CHECK_ONCE; + } else { + return $config->getSystemValue($key, $default); + } + })); + $this->overwriteService('AllConfig', $mockConfig); + // clear hooks \OC_Hook::clear(); \OC::registerShareHooks(); @@ -87,6 +100,8 @@ class Test_Files_Versioning extends \Test\TestCase { } protected function tearDown() { + $this->restoreService('AllConfig'); + if ($this->rootView) { $this->rootView->deleteAll(self::TEST_VERSIONS_USER . '/files/'); $this->rootView->deleteAll(self::TEST_VERSIONS_USER2 . '/files/'); From 0b0b3253bb75f5fffb39201da39139d4f26d8a22 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 21 Mar 2016 14:20:33 +0100 Subject: [PATCH 5/5] properly use fileinfo objects --- lib/private/files/node/folder.php | 28 +++++------ tests/lib/files/node/folder.php | 81 ++++++++++++++++++++++++------- 2 files changed, 75 insertions(+), 34 deletions(-) diff --git a/lib/private/files/node/folder.php b/lib/private/files/node/folder.php index c376bfb188..f4d7dae20a 100644 --- a/lib/private/files/node/folder.php +++ b/lib/private/files/node/folder.php @@ -90,14 +90,14 @@ class Folder extends Node implements \OCP\Files\Folder { /** * @param string $path - * @param array $info + * @param FileInfo $info * @return File|Folder */ - protected function createNode($path, $info = array()) { - if (!isset($info['mimetype'])) { + protected function createNode($path, FileInfo $info = null) { + if (is_null($info)) { $isDir = $this->view->is_dir($path); } else { - $isDir = $info['mimetype'] === 'httpd/unix-directory'; + $isDir = $info->getType() === FileInfo::TYPE_FOLDER; } if ($isDir) { return new Folder($this->root, $this->view, $path, $info); @@ -211,10 +211,9 @@ class Folder extends Node implements \OCP\Files\Folder { private function searchCommon($method, $args) { $files = array(); $rootLength = strlen($this->path); - /** - * @var \OC\Files\Storage\Storage $storage - */ - list($storage, $internalPath) = $this->view->resolvePath($this->path); + $mount = $this->root->getMount($this->path); + $storage = $mount->getStorage(); + $internalPath = $mount->getInternalPath($this->path); $internalPath = rtrim($internalPath, '/'); if ($internalPath !== '') { $internalPath = $internalPath . '/'; @@ -229,7 +228,7 @@ class Folder extends Node implements \OCP\Files\Folder { $result['internalPath'] = $result['path']; $result['path'] = substr($result['path'], $internalRootLength); $result['storage'] = $storage; - $files[] = $result; + $files[] = new \OC\Files\FileInfo($this->path . '/' . $result['path'], $storage, $result['internalPath'], $result, $mount); } } @@ -245,17 +244,14 @@ class Folder extends Node implements \OCP\Files\Folder { $result['internalPath'] = $result['path']; $result['path'] = $relativeMountPoint . $result['path']; $result['storage'] = $storage; - $files[] = $result; + $files[] = new \OC\Files\FileInfo($this->path . '/' . $result['path'], $storage, $result['internalPath'], $result, $mount); } } } - $result = array(); - foreach ($files as $file) { - $result[] = $this->createNode($this->normalizePath($this->path . '/' . $file['path']), $file); - } - - return $result; + return array_map(function(FileInfo $file) { + return $this->createNode($file->getPath(), $file); + }, $files); } /** diff --git a/tests/lib/files/node/folder.php b/tests/lib/files/node/folder.php index 06ca35f7f1..f6d488d659 100644 --- a/tests/lib/files/node/folder.php +++ b/tests/lib/files/node/folder.php @@ -376,6 +376,14 @@ class Folder extends \Test\TestCase { ->method('getCache') ->will($this->returnValue($cache)); + $mount = $this->getMock('\OCP\Files\Mount\IMountPoint'); + $mount->expects($this->once()) + ->method('getStorage') + ->will($this->returnValue($storage)); + $mount->expects($this->once()) + ->method('getInternalPath') + ->will($this->returnValue('foo')); + $cache->expects($this->once()) ->method('search') ->with('%qw%') @@ -388,9 +396,10 @@ class Folder extends \Test\TestCase { ->with('/bar/foo') ->will($this->returnValue(array())); - $view->expects($this->once()) - ->method('resolvePath') - ->will($this->returnValue(array($storage, 'foo'))); + $root->expects($this->once()) + ->method('getMount') + ->with('/bar/foo') + ->will($this->returnValue($mount)); $node = new \OC\Files\Node\Folder($root, $view, '/bar/foo'); $result = $node->search('qw'); @@ -404,13 +413,21 @@ class Folder extends \Test\TestCase { * @var \OC\Files\View | \PHPUnit_Framework_MockObject_MockObject $view */ $view = $this->getMock('\OC\Files\View'); - $root = $this->getMock('\OC\Files\Node\Root', array('getUser', 'getMountsIn'), array($manager, $view, $this->user)); + $root = $this->getMock('\OC\Files\Node\Root', array('getUser', 'getMountsIn', 'getMount'), array($manager, $view, $this->user)); $root->expects($this->any()) ->method('getUser') ->will($this->returnValue($this->user)); $storage = $this->getMock('\OC\Files\Storage\Storage'); $cache = $this->getMock('\OC\Files\Cache\Cache', array(), array('')); + $mount = $this->getMock('\OCP\Files\Mount\IMountPoint'); + $mount->expects($this->once()) + ->method('getStorage') + ->will($this->returnValue($storage)); + $mount->expects($this->once()) + ->method('getInternalPath') + ->will($this->returnValue('files')); + $storage->expects($this->once()) ->method('getCache') ->will($this->returnValue($cache)); @@ -428,9 +445,10 @@ class Folder extends \Test\TestCase { ->with('') ->will($this->returnValue(array())); - $view->expects($this->once()) - ->method('resolvePath') - ->will($this->returnValue(array($storage, 'files'))); + $root->expects($this->once()) + ->method('getMount') + ->with('') + ->will($this->returnValue($mount)); $result = $root->search('qw'); $this->assertEquals(1, count($result)); @@ -450,6 +468,14 @@ class Folder extends \Test\TestCase { $storage = $this->getMock('\OC\Files\Storage\Storage'); $cache = $this->getMock('\OC\Files\Cache\Cache', array(), array('')); + $mount = $this->getMock('\OCP\Files\Mount\IMountPoint'); + $mount->expects($this->once()) + ->method('getStorage') + ->will($this->returnValue($storage)); + $mount->expects($this->once()) + ->method('getInternalPath') + ->will($this->returnValue('')); + $storage->expects($this->once()) ->method('getCache') ->will($this->returnValue($cache)); @@ -466,9 +492,10 @@ class Folder extends \Test\TestCase { ->with('/bar') ->will($this->returnValue(array())); - $view->expects($this->once()) - ->method('resolvePath') - ->will($this->returnValue(array($storage, ''))); + $root->expects($this->once()) + ->method('getMount') + ->with('/bar') + ->will($this->returnValue($mount)); $node = new \OC\Files\Node\Folder($root, $view, '/bar'); $result = $node->search('qw'); @@ -489,6 +516,14 @@ class Folder extends \Test\TestCase { $storage = $this->getMock('\OC\Files\Storage\Storage'); $cache = $this->getMock('\OC\Files\Cache\Cache', array(), array('')); + $mount = $this->getMock('\OCP\Files\Mount\IMountPoint'); + $mount->expects($this->once()) + ->method('getStorage') + ->will($this->returnValue($storage)); + $mount->expects($this->once()) + ->method('getInternalPath') + ->will($this->returnValue('foo')); + $storage->expects($this->once()) ->method('getCache') ->will($this->returnValue($cache)); @@ -505,9 +540,10 @@ class Folder extends \Test\TestCase { ->with('/bar/foo') ->will($this->returnValue(array())); - $view->expects($this->once()) - ->method('resolvePath') - ->will($this->returnValue(array($storage, 'foo'))); + $root->expects($this->once()) + ->method('getMount') + ->with('/bar/foo') + ->will($this->returnValue($mount)); $node = new \OC\Files\Node\Folder($root, $view, '/bar/foo'); $result = $node->searchByTag('tag1', 'user1'); @@ -531,6 +567,14 @@ class Folder extends \Test\TestCase { $subStorage = $this->getMock('\OC\Files\Storage\Storage'); $subMount = $this->getMock('\OC\Files\Mount\MountPoint', array(), array(null, '')); + $mount = $this->getMock('\OCP\Files\Mount\IMountPoint'); + $mount->expects($this->once()) + ->method('getStorage') + ->will($this->returnValue($storage)); + $mount->expects($this->once()) + ->method('getInternalPath') + ->will($this->returnValue('foo')); + $subMount->expects($this->once()) ->method('getStorage') ->will($this->returnValue($subStorage)); @@ -566,9 +610,10 @@ class Folder extends \Test\TestCase { ->with('/bar/foo') ->will($this->returnValue(array($subMount))); - $view->expects($this->once()) - ->method('resolvePath') - ->will($this->returnValue(array($storage, 'foo'))); + $root->expects($this->once()) + ->method('getMount') + ->with('/bar/foo') + ->will($this->returnValue($mount)); $node = new \OC\Files\Node\Folder($root, $view, '/bar/foo'); @@ -602,7 +647,7 @@ class Folder extends \Test\TestCase { $view->expects($this->once()) ->method('getFileInfo') - ->will($this->returnValue(new FileInfo('/bar/foo/qwerty', null, 'qwerty', [], null))); + ->will($this->returnValue(new FileInfo('/bar/foo/qwerty', null, 'qwerty', ['mimetype' => 'text/plain'], null))); $storage->expects($this->once()) ->method('getCache') @@ -684,7 +729,7 @@ class Folder extends \Test\TestCase { $view->expects($this->any()) ->method('getFileInfo') - ->will($this->returnValue(new FileInfo('/bar/foo/qwerty', null, 'qwerty', [], null))); + ->will($this->returnValue(new FileInfo('/bar/foo/qwerty', null, 'qwerty', ['mimetype' => 'plain'], null))); $storage->expects($this->any()) ->method('getCache')