diff options
author | Robin Appelman <robin@icewind.nl> | 2022-04-06 16:49:43 +0000 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-04-06 16:49:43 +0000 |
commit | 9f34d176de8a72f155aa8a5e7c238684da430ca7 (patch) | |
tree | 82345b7956b6965c6088264e3591f2cb182d1a17 /apps | |
parent | b88dc5ff2e32952e03d88d421627ed0d60486aa6 (diff) | |
parent | 5e69f98c16b551327edec2788156b699ee1e38d5 (diff) | |
download | nextcloud-server-9f34d176de8a72f155aa8a5e7c238684da430ca7.tar.gz nextcloud-server-9f34d176de8a72f155aa8a5e7c238684da430ca7.zip |
Merge pull request #31843 from nextcloud/dav-plugin-perf
Improve performance dav PROPFIND
Diffstat (limited to 'apps')
-rw-r--r-- | apps/dav/lib/Connector/Sabre/Directory.php | 13 | ||||
-rw-r--r-- | apps/dav/lib/Connector/Sabre/File.php | 4 | ||||
-rw-r--r-- | apps/dav/lib/Connector/Sabre/Node.php | 25 | ||||
-rw-r--r-- | apps/dav/lib/Connector/Sabre/SharesPlugin.php | 15 | ||||
-rw-r--r-- | apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php | 36 | ||||
-rw-r--r-- | apps/dav/tests/unit/Connector/Sabre/FileTest.php | 67 | ||||
-rw-r--r-- | apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php | 9 | ||||
-rw-r--r-- | apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php | 15 |
8 files changed, 121 insertions, 63 deletions
diff --git a/apps/dav/lib/Connector/Sabre/Directory.php b/apps/dav/lib/Connector/Sabre/Directory.php index bd92b3b22a4..8b616b0cb8a 100644 --- a/apps/dav/lib/Connector/Sabre/Directory.php +++ b/apps/dav/lib/Connector/Sabre/Directory.php @@ -38,6 +38,7 @@ use OCA\DAV\Connector\Sabre\Exception\FileLocked; use OCA\DAV\Connector\Sabre\Exception\Forbidden; use OCA\DAV\Connector\Sabre\Exception\InvalidPath; use OCP\Files\FileInfo; +use OCP\Files\Folder; use OCP\Files\ForbiddenException; use OCP\Files\InvalidPathException; use OCP\Files\NotPermittedException; @@ -144,7 +145,9 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node implements \Sabre\DAV\ICol $info = $this->fileView->getFileInfo($this->path . '/' . $name); if (!$info) { // use a dummy FileInfo which is acceptable here since it will be refreshed after the put is complete - $info = new \OC\Files\FileInfo($path, null, null, [], null); + $info = new \OC\Files\FileInfo($path, null, null, [ + 'type' => FileInfo::TYPE_FILE + ], null); } $node = new \OCA\DAV\Connector\Sabre\File($this->fileView, $info); @@ -233,7 +236,7 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node implements \Sabre\DAV\ICol throw new \Sabre\DAV\Exception\NotFound('File with name ' . $path . ' could not be located'); } - if ($info['mimetype'] === 'httpd/unix-directory') { + if ($info->getMimeType() === FileInfo::MIMETYPE_FOLDER) { $node = new \OCA\DAV\Connector\Sabre\Directory($this->fileView, $info, $this->tree, $this->shareManager); } else { $node = new \OCA\DAV\Connector\Sabre\File($this->fileView, $info, $this->shareManager); @@ -261,7 +264,7 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node implements \Sabre\DAV\ICol // the caller believe that the collection itself does not exist throw new Forbidden('No read permissions'); } - $folderContent = $this->fileView->getDirectoryContent($this->path); + $folderContent = $this->getNode()->getDirectoryListing(); } catch (LockedException $e) { throw new Locked(); } @@ -474,4 +477,8 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node implements \Sabre\DAV\ICol return false; } + + public function getNode(): Folder { + return $this->node; + } } diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 70dffbaaaed..a46ca372be7 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -753,4 +753,8 @@ class File extends Node implements IFile { public function hash(string $type) { return $this->fileView->hash($type, $this->path); } + + public function getNode(): \OCP\Files\File { + return $this->node; + } } diff --git a/apps/dav/lib/Connector/Sabre/Node.php b/apps/dav/lib/Connector/Sabre/Node.php index 79b4db0e327..e4517068f42 100644 --- a/apps/dav/lib/Connector/Sabre/Node.php +++ b/apps/dav/lib/Connector/Sabre/Node.php @@ -36,9 +36,12 @@ namespace OCA\DAV\Connector\Sabre; use OC\Files\Mount\MoveableMount; +use OC\Files\Node\File; +use OC\Files\Node\Folder; use OC\Files\View; use OCA\DAV\Connector\Sabre\Exception\InvalidPath; use OCP\Files\FileInfo; +use OCP\Files\IRootFolder; use OCP\Files\StorageNotAvailableException; use OCP\Share\IShare; use OCP\Share\Exceptions\ShareNotFound; @@ -75,6 +78,8 @@ abstract class Node implements \Sabre\DAV\INode { */ protected $shareManager; + protected \OCP\Files\Node $node; + /** * Sets up the node, expects a full path name * @@ -91,10 +96,26 @@ abstract class Node implements \Sabre\DAV\INode { } else { $this->shareManager = \OC::$server->getShareManager(); } + if ($info instanceof Folder || $info instanceof File) { + $this->node = $info; + } else { + $root = \OC::$server->get(IRootFolder::class); + if ($info->getType() === FileInfo::TYPE_FOLDER) { + $this->node = new Folder($root, $view, $this->path, $info); + } else { + $this->node = new File($root, $view, $this->path, $info); + } + } } protected function refreshInfo() { $this->info = $this->fileView->getFileInfo($this->path); + $root = \OC::$server->get(IRootFolder::class); + if ($this->info->getType() === FileInfo::TYPE_FOLDER) { + $this->node = new Folder($root, $this->fileView, $this->path, $this->info); + } else { + $this->node = new File($root, $this->fileView, $this->path, $this->info); + } } /** @@ -403,6 +424,10 @@ abstract class Node implements \Sabre\DAV\INode { return $this->info; } + public function getNode(): \OCP\Files\Node { + return $this->node; + } + protected function sanitizeMtime($mtimeFromRequest) { return MtimeSanitizer::sanitizeMtime($mtimeFromRequest); } diff --git a/apps/dav/lib/Connector/Sabre/SharesPlugin.php b/apps/dav/lib/Connector/Sabre/SharesPlugin.php index d482aa4b510..57c91e05a8c 100644 --- a/apps/dav/lib/Connector/Sabre/SharesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/SharesPlugin.php @@ -165,7 +165,7 @@ class SharesPlugin extends \Sabre\DAV\ServerPlugin { // if we already cached the folder this file is in we know there are no shares for this file if (array_search($parentPath, $this->cachedFolders) === false) { try { - $node = $this->userFolder->get($sabreNode->getPath()); + $node = $sabreNode->getNode(); } catch (NotFoundException $e) { return []; } @@ -201,18 +201,7 @@ class SharesPlugin extends \Sabre\DAV\ServerPlugin { !is_null($propFind->getStatus(self::SHAREES_PROPERTYNAME)) ) ) { - try { - $folderNode = $this->userFolder->get($sabreNode->getPath()); - } catch (NotFoundException $e) { - // If the folder can't be properly found just return - return; - } - - if (!($folderNode instanceof Folder)) { - // Safety check - return; - } - + $folderNode = $sabreNode->getNode(); $this->cachedFolders[] = $sabreNode->getPath(); $childShares = $this->getSharesFolder($folderNode); foreach ($childShares as $id => $shares) { diff --git a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php index c88d2302bec..e8297c2ac66 100644 --- a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php @@ -29,6 +29,7 @@ namespace OCA\DAV\Tests\Unit\Connector\Sabre; use OC\Files\FileInfo; +use OC\Files\Node\Node; use OC\Files\Storage\Wrapper\Quota; use OCA\DAV\Connector\Sabre\Directory; use OCP\Files\ForbiddenException; @@ -82,9 +83,12 @@ class DirectoryTest extends \Test\TestCase { $this->view = $this->createMock('OC\Files\View'); $this->info = $this->createMock('OC\Files\FileInfo'); - $this->info->expects($this->any()) - ->method('isReadable') + $this->info->method('isReadable') ->willReturn(true); + $this->info->method('getType') + ->willReturn(Node::TYPE_FOLDER); + $this->info->method('getName') + ->willReturn("folder"); } private function getDir($path = '/') { @@ -186,17 +190,17 @@ class DirectoryTest extends \Test\TestCase { $info2 = $this->getMockBuilder(FileInfo::class) ->disableOriginalConstructor() ->getMock(); - $info1->expects($this->any()) - ->method('getName') + $info1->method('getName') ->willReturn('first'); - $info1->expects($this->any()) - ->method('getEtag') + $info1->method('getPath') + ->willReturn('folder/first'); + $info1->method('getEtag') ->willReturn('abc'); - $info2->expects($this->any()) - ->method('getName') + $info2->method('getName') ->willReturn('second'); - $info2->expects($this->any()) - ->method('getEtag') + $info2->method('getPath') + ->willReturn('folder/second'); + $info2->method('getEtag') ->willReturn('def'); $this->view->expects($this->once()) @@ -403,8 +407,12 @@ class DirectoryTest extends \Test\TestCase { private function moveTest($source, $destination, $updatables, $deletables) { $view = new TestViewDirectory($updatables, $deletables); - $sourceInfo = new FileInfo($source, null, null, [], null); - $targetInfo = new FileInfo(dirname($destination), null, null, [], null); + $sourceInfo = new FileInfo($source, null, null, [ + 'type' => FileInfo::TYPE_FOLDER, + ], null); + $targetInfo = new FileInfo(dirname($destination), null, null, [ + 'type' => FileInfo::TYPE_FOLDER, + ], null); $sourceNode = new Directory($view, $sourceInfo); $targetNode = $this->getMockBuilder(Directory::class) @@ -429,8 +437,8 @@ class DirectoryTest extends \Test\TestCase { $view = new TestViewDirectory($updatables, $deletables); - $sourceInfo = new FileInfo($source, null, null, [], null); - $targetInfo = new FileInfo(dirname($destination), null, null, [], null); + $sourceInfo = new FileInfo($source, null, null, ['type' => FileInfo::TYPE_FOLDER], null); + $targetInfo = new FileInfo(dirname($destination), null, null, ['type' => FileInfo::TYPE_FOLDER], null); $sourceNode = new Directory($view, $sourceInfo); $targetNode = $this->getMockBuilder(Directory::class) diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index d12a86f6e8d..9870a62845c 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -37,6 +37,7 @@ use OC\Files\Storage\Wrapper\PermissionsMask; use OC\Files\View; use OCA\DAV\Connector\Sabre\File; use OCP\Constants; +use OCP\Files\FileInfo; use OCP\Files\ForbiddenException; use OCP\Files\Storage; use OCP\IConfig; @@ -211,7 +212,8 @@ class FileTest extends TestCase { ->willReturnArgument(0); $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [ - 'permissions' => \OCP\Constants::PERMISSION_ALL + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -272,7 +274,8 @@ class FileTest extends TestCase { $_SERVER['HTTP_OC_CHUNKED'] = true; $info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-0', $this->getMockStorage(), null, [ - 'permissions' => \OCP\Constants::PERMISSION_ALL + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -282,7 +285,8 @@ class FileTest extends TestCase { $file->releaseLock(ILockingProvider::LOCK_SHARED); $info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-1', $this->getMockStorage(), null, [ - 'permissions' => \OCP\Constants::PERMISSION_ALL + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -326,7 +330,10 @@ class FileTest extends TestCase { $viewRoot . '/' . ltrim($path, '/'), $this->getMockStorage(), null, - ['permissions' => \OCP\Constants::PERMISSION_ALL], + [ + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, + ], null ); @@ -690,7 +697,8 @@ class FileTest extends TestCase { $_SERVER['REQUEST_METHOD'] = 'PUT'; $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [ - 'permissions' => \OCP\Constants::PERMISSION_ALL + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -723,7 +731,8 @@ class FileTest extends TestCase { $view->lockFile('/test.txt', ILockingProvider::LOCK_EXCLUSIVE); $info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt', $this->getMockStorage(), null, [ - 'permissions' => \OCP\Constants::PERMISSION_ALL + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -758,7 +767,8 @@ class FileTest extends TestCase { $_SERVER['HTTP_OC_CHUNKED'] = true; $info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-0', $this->getMockStorage(), null, [ - 'permissions' => \OCP\Constants::PERMISSION_ALL + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); $file->acquireLock(ILockingProvider::LOCK_SHARED); @@ -766,7 +776,8 @@ class FileTest extends TestCase { $file->releaseLock(ILockingProvider::LOCK_SHARED); $info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-1', $this->getMockStorage(), null, [ - 'permissions' => \OCP\Constants::PERMISSION_ALL + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -797,7 +808,8 @@ class FileTest extends TestCase { ->willReturnArgument(0); $info = new \OC\Files\FileInfo('/*', $this->getMockStorage(), null, [ - 'permissions' => \OCP\Constants::PERMISSION_ALL + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -836,7 +848,8 @@ class FileTest extends TestCase { ->willReturnArgument(0); $info = new \OC\Files\FileInfo('/*', $this->getMockStorage(), null, [ - 'permissions' => \OCP\Constants::PERMISSION_ALL + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); $file->setName('/super*star.txt'); @@ -863,7 +876,8 @@ class FileTest extends TestCase { $_SERVER['REQUEST_METHOD'] = 'PUT'; $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [ - 'permissions' => \OCP\Constants::PERMISSION_ALL + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -897,7 +911,8 @@ class FileTest extends TestCase { ->willReturn(true); $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [ - 'permissions' => \OCP\Constants::PERMISSION_ALL + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -915,7 +930,8 @@ class FileTest extends TestCase { ->getMock(); $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [ - 'permissions' => 0 + 'permissions' => 0, + 'type' => FileInfo::TYPE_FOLDER, ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -938,7 +954,8 @@ class FileTest extends TestCase { ->willReturn(false); $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [ - 'permissions' => \OCP\Constants::PERMISSION_ALL + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -961,7 +978,8 @@ class FileTest extends TestCase { ->willThrowException(new ForbiddenException('', true)); $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [ - 'permissions' => \OCP\Constants::PERMISSION_ALL + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -997,7 +1015,10 @@ class FileTest extends TestCase { '/' . $this->user . '/files/' . $path, $this->getMockStorage(), null, - ['permissions' => \OCP\Constants::PERMISSION_ALL], + [ + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, + ], null ); @@ -1129,7 +1150,8 @@ class FileTest extends TestCase { ->willReturn(false); $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [ - 'permissions' => \OCP\Constants::PERMISSION_ALL + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -1149,7 +1171,8 @@ class FileTest extends TestCase { ->willThrowException(new ForbiddenException('', true)); $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [ - 'permissions' => \OCP\Constants::PERMISSION_ALL + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -1168,7 +1191,8 @@ class FileTest extends TestCase { ->method('fopen'); $info = new \OC\Files\FileInfo('/test.txt', $this->getMockStorage(), null, [ - 'permissions' => \OCP\Constants::PERMISSION_CREATE // no read perm + 'permissions' => \OCP\Constants::PERMISSION_CREATE, // no read perm + 'type' => FileInfo::TYPE_FOLDER, ], null); $file = new \OCA\DAV\Connector\Sabre\File($view, $info); @@ -1215,7 +1239,10 @@ class FileTest extends TestCase { '/' . $this->user . '/files/' . $path, $this->getMockStorage(), null, - ['permissions' => \OCP\Constants::PERMISSION_ALL], + [ + 'permissions' => \OCP\Constants::PERMISSION_ALL, + 'type' => FileInfo::TYPE_FOLDER, + ], null ); diff --git a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php index 7416cf7a3f7..5f516cec113 100644 --- a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php @@ -167,17 +167,14 @@ class ObjectTreeTest extends \Test\TestCase { $fileInfo = $this->getMockBuilder(FileInfo::class) ->disableOriginalConstructor() ->getMock(); - $fileInfo->expects($this->once()) - ->method('getType') + $fileInfo->method('getType') ->willReturn($type); - $fileInfo->expects($this->once()) - ->method('getName') + $fileInfo->method('getName') ->willReturn($outputFileName); $fileInfo->method('getStorage') ->willReturn($this->createMock(\OC\Files\Storage\Common::class)); - $view->expects($this->once()) - ->method('getFileInfo') + $view->method('getFileInfo') ->with($fileInfoQueryPath) ->willReturn($fileInfo); diff --git a/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php index f528310e54c..a81226ccb5e 100644 --- a/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php @@ -111,9 +111,7 @@ class SharesPluginTest extends \Test\TestCase { ->disableOriginalConstructor() ->getMock(); - $this->userFolder->expects($this->once()) - ->method('get') - ->with('/subdir') + $sabreNode->method('getNode') ->willReturn($node); $this->shareManager->expects($this->any()) @@ -180,16 +178,19 @@ class SharesPluginTest extends \Test\TestCase { $node = $this->createMock(Folder::class); $node->method('getId') ->willReturn(123); - $node1 = $this->createMock(File::class); + $node1 = $this->createMock(\OC\Files\Node\File::class); $node1->method('getId') ->willReturn(111); - $node2 = $this->createMock(File::class); + $node2 = $this->createMock(\OC\Files\Node\File::class); $node2->method('getId') ->willReturn(222); - $this->userFolder->method('get') - ->with('/subdir') + $sabreNode->method('getNode') ->willReturn($node); + $sabreNode1->method('getNode') + ->willReturn($node1); + $sabreNode2->method('getNode') + ->willReturn($node2); $dummyShares = array_map(function ($type) { $share = $this->getMockBuilder(IShare::class)->getMock(); |