Merge pull request #31843 from nextcloud/dav-plugin-perf

Improve performance dav PROPFIND
This commit is contained in:
Robin Appelman 2022-04-06 16:49:43 +00:00 committed by GitHub
commit 9f34d176de
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 137 additions and 70 deletions

View file

@ -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;
}
}

View file

@ -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;
}
}

View file

@ -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);
}

View file

@ -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) {

View file

@ -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)

View file

@ -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
);

View file

@ -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);

View file

@ -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();

View file

@ -254,7 +254,7 @@ class FileInfo implements \OCP\Files\FileInfo, \ArrayAccess {
*/
public function getType() {
if (!isset($this->data['type'])) {
$this->data['type'] = ($this->getMimetype() === 'httpd/unix-directory') ? self::TYPE_FOLDER : self::TYPE_FILE;
$this->data['type'] = ($this->getMimetype() === self::MIMETYPE_FOLDER) ? self::TYPE_FOLDER : self::TYPE_FILE;
}
return $this->data['type'];
}

View file

@ -97,10 +97,10 @@ class Folder extends Node implements \OCP\Files\Folder {
* @throws \OCP\Files\NotFoundException
*/
public function getDirectoryListing() {
$folderContent = $this->view->getDirectoryContent($this->path);
$folderContent = $this->view->getDirectoryContent($this->path, '', $this->getFileInfo());
return array_map(function (FileInfo $info) {
if ($info->getMimetype() === 'httpd/unix-directory') {
if ($info->getMimetype() === FileInfo::MIMETYPE_FOLDER) {
return new Folder($this->root, $this->view, $info->getPath(), $info);
} else {
return new File($this->root, $this->view, $info->getPath(), $info);

View file

@ -1431,7 +1431,7 @@ class View {
* @param string $mimetype_filter limit returned content to this mimetype or mimepart
* @return FileInfo[]
*/
public function getDirectoryContent($directory, $mimetype_filter = '') {
public function getDirectoryContent($directory, $mimetype_filter = '', \OCP\Files\FileInfo $directoryInfo = null) {
$this->assertPathLength($directory);
if (!Filesystem::isValidPath($directory)) {
return [];
@ -1449,13 +1449,20 @@ class View {
$cache = $storage->getCache($internalPath);
$user = \OC_User::getUser();
$data = $this->getCacheEntry($storage, $internalPath, $directory);
if (!$directoryInfo) {
$data = $this->getCacheEntry($storage, $internalPath, $directory);
if (!$data instanceof ICacheEntry || !isset($data['fileid'])) {
return [];
}
} else {
$data = $directoryInfo;
}
if (!$data instanceof ICacheEntry || !isset($data['fileid']) || !($data->getPermissions() & Constants::PERMISSION_READ)) {
if (!($data->getPermissions() & Constants::PERMISSION_READ)) {
return [];
}
$folderId = $data['fileid'];
$folderId = $data->getId();
$contents = $cache->getFolderContentsById($folderId); //TODO: mimetype_filter
$sharingDisabled = \OCP\Util::isSharingDisabledForUser();

View file

@ -78,6 +78,8 @@ class FolderTest extends NodeTest {
new FileInfo('/bar/foo/asd', null, 'foo/asd', ['fileid' => 2, 'path' => '/bar/foo/asd', 'name' => 'asd', 'size' => 100, 'mtime' => 50, 'mimetype' => 'text/plain'], null),
new FileInfo('/bar/foo/qwerty', null, 'foo/qwerty', ['fileid' => 3, 'path' => '/bar/foo/qwerty', 'name' => 'qwerty', 'size' => 200, 'mtime' => 55, 'mimetype' => 'httpd/unix-directory'], null),
]);
$view->method('getFileInfo')
->willReturn($this->createMock(FileInfo::class));
$node = new Folder($root, $view, '/bar/foo');
$children = $node->getDirectoryListing();