From fc215d0980ec3cfa21ac64119bcec614cc1323bb Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 4 Feb 2016 12:51:23 +0100 Subject: [PATCH] Make the share object lazy Share providers can now just pass in a fileId. And the node will only be created once needed. --- lib/private/server.php | 3 +- lib/private/share20/defaultshareprovider.php | 5 +- lib/private/share20/manager.php | 17 ++-- lib/private/share20/share.php | 50 ++++++++-- lib/public/share/ishare.php | 16 +++- .../lib/share20/defaultshareprovidertest.php | 6 +- tests/lib/share20/managertest.php | 93 ++++++++++--------- 7 files changed, 116 insertions(+), 74 deletions(-) diff --git a/lib/private/server.php b/lib/private/server.php index 69406c2203..55ac64a0c2 100644 --- a/lib/private/server.php +++ b/lib/private/server.php @@ -618,7 +618,8 @@ class Server extends ServerContainer implements IServerContainer { $c->getGroupManager(), $c->getL10N('core'), $factory, - $c->getUserManager() + $c->getUserManager(), + $c->getRootFolder() ); return $manager; diff --git a/lib/private/share20/defaultshareprovider.php b/lib/private/share20/defaultshareprovider.php index 38d1dae316..48164a5d41 100644 --- a/lib/private/share20/defaultshareprovider.php +++ b/lib/private/share20/defaultshareprovider.php @@ -711,7 +711,7 @@ class DefaultShareProvider implements IShareProvider { * @throws InvalidShare */ private function createShare($data) { - $share = new Share(); + $share = new Share($this->rootFolder); $share->setId((int)$data['id']) ->setShareType((int)$data['share_type']) ->setPermissions((int)$data['permissions']) @@ -744,8 +744,7 @@ class DefaultShareProvider implements IShareProvider { $share->setShareOwner($data['uid_owner']); } - $path = $this->getNode($share->getShareOwner(), (int)$data['file_source']); - $share->setNode($path); + $share->setNodeId((int)$data['file_source']); if ($data['expiration'] !== null) { $expiration = \DateTime::createFromFormat('Y-m-d H:i:s', $data['expiration']); diff --git a/lib/private/share20/manager.php b/lib/private/share20/manager.php index db42f76d7a..3db76d92ac 100644 --- a/lib/private/share20/manager.php +++ b/lib/private/share20/manager.php @@ -21,6 +21,7 @@ namespace OC\Share20; +use OCP\Files\IRootFolder; use OCP\IUserManager; use OCP\Share\IManager; use OCP\Share\IProviderFactory; @@ -45,30 +46,24 @@ class Manager implements IManager { /** @var IProviderFactory */ private $factory; - /** @var ILogger */ private $logger; - /** @var IConfig */ private $config; - /** @var ISecureRandom */ private $secureRandom; - /** @var IHasher */ private $hasher; - /** @var IMountManager */ private $mountManager; - /** @var IGroupManager */ private $groupManager; - /** @var IL10N */ private $l; - /** @var IUserManager */ private $userManager; + /** @var IRootFolder */ + private $rootFolder; /** * Manager constructor. @@ -92,7 +87,8 @@ class Manager implements IManager { IGroupManager $groupManager, IL10N $l, IProviderFactory $factory, - IUserManager $userManager + IUserManager $userManager, + IRootFolder $rootFolder ) { $this->logger = $logger; $this->config = $config; @@ -103,6 +99,7 @@ class Manager implements IManager { $this->l = $l; $this->factory = $factory; $this->userManager = $userManager; + $this->rootFolder = $rootFolder; } /** @@ -888,7 +885,7 @@ class Manager implements IManager { * @return \OCP\Share\IShare; */ public function newShare() { - return new \OC\Share20\Share(); + return new \OC\Share20\Share($this->rootFolder); } /** diff --git a/lib/private/share20/share.php b/lib/private/share20/share.php index db91ad4a91..5b7945b1da 100644 --- a/lib/private/share20/share.php +++ b/lib/private/share20/share.php @@ -20,7 +20,9 @@ */ namespace OC\Share20; +use OCP\Files\IRootFolder; use OCP\Files\Node; +use OCP\Files\NotFoundException; use OCP\IUser; use OCP\IGroup; @@ -31,14 +33,16 @@ class Share implements \OCP\Share\IShare { /** @var string */ private $providerId; /** @var Node */ - private $path; + private $node; + /** @var int */ + private $fileId; /** @var int */ private $shareType; - /** @var IUser|IGroup */ + /** @var string */ private $sharedWith; - /** @var IUser */ + /** @var string */ private $sharedBy; - /** @var IUser */ + /** @var string */ private $shareOwner; /** @var int */ private $permissions; @@ -57,6 +61,13 @@ class Share implements \OCP\Share\IShare { /** @var bool */ private $mailSend; + /** @var IRootFolder */ + private $rootFolder; + + public function __construct(IRootFolder $rootFolder) { + $this->rootFolder = $rootFolder; + } + /** * @inheritdoc */ @@ -90,8 +101,8 @@ class Share implements \OCP\Share\IShare { /** * @inheritdoc */ - public function setNode(Node $path) { - $this->path = $path; + public function setNode(Node $node) { + $this->node = $node; return $this; } @@ -99,7 +110,32 @@ class Share implements \OCP\Share\IShare { * @inheritdoc */ public function getNode() { - return $this->path; + if ($this->node === null) { + + if ($this->shareOwner === null || $this->fileId === null) { + throw new NotFoundException(); + } + + $userFolder = $this->rootFolder->getUserFolder($this->shareOwner); + + $nodes = $userFolder->getById($this->fileId); + if (empty($nodes)) { + throw new NotFoundException(); + } + + $this->node = $nodes[0]; + } + + return $this->node; + } + + /** + * @inheritdoc + */ + public function setNodeId($fileId) { + $this->node = null; + $this->fileId = $fileId; + return $this; } /** diff --git a/lib/public/share/ishare.php b/lib/public/share/ishare.php index 5a82436c72..074f411758 100644 --- a/lib/public/share/ishare.php +++ b/lib/public/share/ishare.php @@ -24,8 +24,7 @@ namespace OCP\Share; use OCP\Files\File; use OCP\Files\Folder; use OCP\Files\Node; -use OCP\IUser; -use OCP\IGroup; +use OCP\Files\NotFoundException; /** * Interface IShare @@ -55,20 +54,29 @@ interface IShare { /** * Set the node of the file/folder that is shared * - * @param File|Folder $path + * @param Node $node * @return \OCP\Share\IShare The modified object * @since 9.0.0 */ - public function setNode(Node $path); + public function setNode(Node $node); /** * Get the node of the file/folder that is shared * * @return File|Folder * @since 9.0.0 + * @throws NotFoundException */ public function getNode(); + /** + * Set file id for lazy evaluation of the node + * @param int $fileId + * @return \OCP\Share\IShare The modified object + * @since 9.0.0 + */ + public function setNodeId($fileId); + /** * Set the shareType * diff --git a/tests/lib/share20/defaultshareprovidertest.php b/tests/lib/share20/defaultshareprovidertest.php index 19f21fba7b..1f3fa9af32 100644 --- a/tests/lib/share20/defaultshareprovidertest.php +++ b/tests/lib/share20/defaultshareprovidertest.php @@ -561,7 +561,7 @@ class DefaultShareProviderTest extends \Test\TestCase { } public function testCreateUserShare() { - $share = new \OC\Share20\Share(); + $share = new \OC\Share20\Share($this->rootFolder); $shareOwner = $this->getMock('OCP\IUser'); $shareOwner->method('getUID')->WillReturn('shareOwner'); @@ -609,7 +609,7 @@ class DefaultShareProviderTest extends \Test\TestCase { } public function testCreateGroupShare() { - $share = new \OC\Share20\Share(); + $share = new \OC\Share20\Share($this->rootFolder); $shareOwner = $this->getMock('\OCP\IUser'); $shareOwner->method('getUID')->willReturn('shareOwner'); @@ -657,7 +657,7 @@ class DefaultShareProviderTest extends \Test\TestCase { } public function testCreateLinkShare() { - $share = new \OC\Share20\Share(); + $share = new \OC\Share20\Share($this->rootFolder); $shareOwner = $this->getMock('\OCP\IUser'); $shareOwner->method('getUID')->willReturn('shareOwner'); diff --git a/tests/lib/share20/managertest.php b/tests/lib/share20/managertest.php index aa286cc471..01e5283fcf 100644 --- a/tests/lib/share20/managertest.php +++ b/tests/lib/share20/managertest.php @@ -20,6 +20,7 @@ */ namespace Test\Share20; +use OCP\Files\IRootFolder; use OCP\IUserManager; use OCP\Share\IProviderFactory; use OCP\Share\IShare; @@ -35,7 +36,6 @@ use OCP\Security\ISecureRandom; use OCP\Security\IHasher; use OCP\Files\Mount\IMountManager; use OCP\IGroupManager; -use Sabre\VObject\Property\VCard\DateTime; /** * Class ManagerTest @@ -47,36 +47,28 @@ class ManagerTest extends \Test\TestCase { /** @var Manager */ protected $manager; - /** @var ILogger */ protected $logger; - /** @var IConfig */ protected $config; - /** @var ISecureRandom */ protected $secureRandom; - /** @var IHasher */ protected $hasher; - /** @var IShareProvider | \PHPUnit_Framework_MockObject_MockObject */ protected $defaultProvider; - /** @var IMountManager */ protected $mountManager; - /** @var IGroupManager */ protected $groupManager; - /** @var IL10N */ protected $l; - /** @var DummyFactory */ protected $factory; - /** @var IUserManager */ protected $userManager; + /** @var IRootFolder | \PHPUnit_Framework_MockObject_MockObject */ + protected $rootFolder; public function setUp() { @@ -87,6 +79,7 @@ class ManagerTest extends \Test\TestCase { $this->mountManager = $this->getMock('\OCP\Files\Mount\IMountManager'); $this->groupManager = $this->getMock('\OCP\IGroupManager'); $this->userManager = $this->getMock('\OCP\IUserManager'); + $this->rootFolder = $this->getMock('\OCP\Files\IRootFolder'); $this->l = $this->getMock('\OCP\IL10N'); $this->l->method('t') @@ -105,7 +98,8 @@ class ManagerTest extends \Test\TestCase { $this->groupManager, $this->l, $this->factory, - $this->userManager + $this->userManager, + $this->rootFolder ); $this->defaultProvider = $this->getMockBuilder('\OC\Share20\DefaultShareProvider') @@ -131,7 +125,8 @@ class ManagerTest extends \Test\TestCase { $this->groupManager, $this->l, $this->factory, - $this->userManager + $this->userManager, + $this->rootFolder ]); } @@ -792,7 +787,7 @@ class ManagerTest extends \Test\TestCase { * @expectedExceptionMessage Only sharing with group members is allowed */ public function testUserCreateChecksShareWithGroupMembersOnlyDifferentGroups() { - $share = new \OC\Share20\Share(); + $share = $this->manager->newShare(); $sharedBy = $this->getMock('\OCP\IUser'); $sharedWith = $this->getMock('\OCP\IUser'); @@ -822,7 +817,7 @@ class ManagerTest extends \Test\TestCase { } public function testUserCreateChecksShareWithGroupMembersOnlySharedGroup() { - $share = new \OC\Share20\Share(); + $share = $this->manager->newShare(); $sharedBy = $this->getMock('\OCP\IUser'); $sharedWith = $this->getMock('\OCP\IUser'); @@ -904,8 +899,7 @@ class ManagerTest extends \Test\TestCase { ->setProviderId('foo') ->setId('bar'); - $share2 = new \OC\Share20\Share(); - $owner2 = $this->getMock('\OCP\IUser'); + $share2 = $this->manager->newShare(); $share2->setShareType(\OCP\Share::SHARE_TYPE_GROUP) ->setShareOwner('shareOwner2') ->setProviderId('foo') @@ -928,7 +922,7 @@ class ManagerTest extends \Test\TestCase { } public function testUserCreateChecksIdenticalPathNotSharedWithUser() { - $share = new \OC\Share20\Share(); + $share = $this->manager->newShare(); $sharedWith = $this->getMock('\OCP\IUser'); $path = $this->getMock('\OCP\Files\Node'); $share->setSharedWith('sharedWith') @@ -939,7 +933,7 @@ class ManagerTest extends \Test\TestCase { $this->userManager->method('get')->with('sharedWith')->willReturn($sharedWith); - $share2 = new \OC\Share20\Share(); + $share2 = $this->manager->newShare(); $share2->setShareType(\OCP\Share::SHARE_TYPE_GROUP) ->setShareOwner('shareOwner2') ->setProviderId('foo') @@ -967,7 +961,7 @@ class ManagerTest extends \Test\TestCase { * @expectedExceptionMessage Only sharing within your own groups is allowed */ public function testGroupCreateChecksShareWithGroupMembersOnlyNotInGroup() { - $share = new \OC\Share20\Share(); + $share = $this->manager->newShare(); $user = $this->getMock('\OCP\IUser'); $group = $this->getMock('\OCP\IGroup'); @@ -988,7 +982,7 @@ class ManagerTest extends \Test\TestCase { } public function testGroupCreateChecksShareWithGroupMembersOnlyInGroup() { - $share = new \OC\Share20\Share(); + $share = $this->manager->newShare(); $user = $this->getMock('\OCP\IUser'); $group = $this->getMock('\OCP\IGroup'); @@ -1028,7 +1022,7 @@ class ManagerTest extends \Test\TestCase { ->setProviderId('foo') ->setId('bar'); - $share2 = new \OC\Share20\Share(); + $share2 = $this->manager->newShare(); $share2->setSharedWith('sharedWith') ->setProviderId('foo') ->setId('baz'); @@ -1041,14 +1035,14 @@ class ManagerTest extends \Test\TestCase { } public function testGroupCreateChecksPathAlreadySharedWithDifferentGroup() { - $share = new \OC\Share20\Share(); + $share = $this->manager->newShare(); $share->setSharedWith('sharedWith'); $path = $this->getMock('\OCP\Files\Node'); $share->setNode($path); - $share2 = new \OC\Share20\Share(); + $share2 = $this->manager->newShare(); $share2->setSharedWith('sharedWith2'); $this->defaultProvider->method('getSharesByPath') @@ -1063,7 +1057,7 @@ class ManagerTest extends \Test\TestCase { * @expectedExceptionMessage Link sharing not allowed */ public function testLinkCreateChecksNoLinkSharesAllowed() { - $share = new \OC\Share20\Share(); + $share = $this->manager->newShare(); $this->config ->method('getAppValue') @@ -1079,7 +1073,7 @@ class ManagerTest extends \Test\TestCase { * @expectedExceptionMessage Link shares can't have reshare permissions */ public function testLinkCreateChecksSharePermissions() { - $share = new \OC\Share20\Share(); + $share = $this->manager->newShare(); $share->setPermissions(\OCP\Constants::PERMISSION_SHARE); @@ -1097,7 +1091,7 @@ class ManagerTest extends \Test\TestCase { * @expectedExceptionMessage Link shares can't have delete permissions */ public function testLinkCreateChecksDeletePermissions() { - $share = new \OC\Share20\Share(); + $share = $this->manager->newShare(); $share->setPermissions(\OCP\Constants::PERMISSION_DELETE); @@ -1115,7 +1109,7 @@ class ManagerTest extends \Test\TestCase { * @expectedExceptionMessage Public upload not allowed */ public function testLinkCreateChecksNoPublicUpload() { - $share = new \OC\Share20\Share(); + $share = $this->manager->newShare(); $share->setPermissions(\OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE); @@ -1130,7 +1124,7 @@ class ManagerTest extends \Test\TestCase { } public function testLinkCreateChecksPublicUpload() { - $share = new \OC\Share20\Share(); + $share = $this->manager->newShare(); $share->setPermissions(\OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE); @@ -1145,7 +1139,7 @@ class ManagerTest extends \Test\TestCase { } public function testLinkCreateChecksReadOnly() { - $share = new \OC\Share20\Share(); + $share = $this->manager->newShare(); $share->setPermissions(\OCP\Constants::PERMISSION_READ); @@ -1316,7 +1310,7 @@ class ManagerTest extends \Test\TestCase { ->getMock(); $manager->expects($this->once())->method('canShare')->willReturn(false); - $share = new \OC\Share20\Share(); + $share = $this->manager->newShare(); $manager->createShare($share); } @@ -1610,7 +1604,8 @@ class ManagerTest extends \Test\TestCase { $this->groupManager, $this->l, $factory, - $this->userManager + $this->userManager, + $this->rootFolder ); $share = $this->getMock('\OCP\Share\IShare'); @@ -1695,7 +1690,7 @@ class ManagerTest extends \Test\TestCase { ->getMock(); $manager->expects($this->once())->method('canShare')->willReturn(false); - $share = new \OC\Share20\Share(); + $share = $this->manager->newShare(); $manager->updateShare($share); } @@ -1711,13 +1706,13 @@ class ManagerTest extends \Test\TestCase { ]) ->getMock(); - $originalShare = new \OC\Share20\Share(); + $originalShare = $this->manager->newShare(); $originalShare->setShareType(\OCP\Share::SHARE_TYPE_GROUP); $manager->expects($this->once())->method('canShare')->willReturn(true); $manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare); - $share = new \OC\Share20\Share(); + $share = $this->manager->newShare(); $share->setProviderId('foo') ->setId('42') ->setShareType(\OCP\Share::SHARE_TYPE_USER); @@ -1737,14 +1732,14 @@ class ManagerTest extends \Test\TestCase { ]) ->getMock(); - $originalShare = new \OC\Share20\Share(); + $originalShare = $this->manager->newShare(); $originalShare->setShareType(\OCP\Share::SHARE_TYPE_GROUP) ->setSharedWith('origGroup'); $manager->expects($this->once())->method('canShare')->willReturn(true); $manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare); - $share = new \OC\Share20\Share(); + $share = $this->manager->newShare(); $share->setProviderId('foo') ->setId('42') ->setShareType(\OCP\Share::SHARE_TYPE_GROUP) @@ -1765,14 +1760,14 @@ class ManagerTest extends \Test\TestCase { ]) ->getMock(); - $originalShare = new \OC\Share20\Share(); + $originalShare = $this->manager->newShare(); $originalShare->setShareType(\OCP\Share::SHARE_TYPE_USER) ->setSharedWith('sharedWith'); $manager->expects($this->once())->method('canShare')->willReturn(true); $manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare); - $share = new \OC\Share20\Share(); + $share = $this->manager->newShare(); $share->setProviderId('foo') ->setId('42') ->setShareType(\OCP\Share::SHARE_TYPE_USER) @@ -1793,19 +1788,22 @@ class ManagerTest extends \Test\TestCase { ]) ->getMock(); - $originalShare = new \OC\Share20\Share(); + $originalShare = $this->manager->newShare(); $originalShare->setShareType(\OCP\Share::SHARE_TYPE_USER) ->setSharedWith('origUser'); $manager->expects($this->once())->method('canShare')->willReturn(true); $manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare); - $share = new \OC\Share20\Share(); + $node = $this->getMock('\OCP\Files\File'); + + $share = $this->manager->newShare(); $share->setProviderId('foo') ->setId('42') ->setShareType(\OCP\Share::SHARE_TYPE_USER) ->setSharedWith('origUser') - ->setShareOwner('newUser'); + ->setShareOwner('newUser') + ->setNode($node); $this->defaultProvider->expects($this->once()) ->method('update') @@ -1831,19 +1829,22 @@ class ManagerTest extends \Test\TestCase { ]) ->getMock(); - $originalShare = new \OC\Share20\Share(); + $originalShare = $this->manager->newShare(); $originalShare->setShareType(\OCP\Share::SHARE_TYPE_GROUP) ->setSharedWith('origUser'); $manager->expects($this->once())->method('canShare')->willReturn(true); $manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare); - $share = new \OC\Share20\Share(); + $node = $this->getMock('\OCP\Files\File'); + + $share = $this->manager->newShare(); $share->setProviderId('foo') ->setId('42') ->setShareType(\OCP\Share::SHARE_TYPE_GROUP) ->setSharedWith('origUser') - ->setShareOwner('owner'); + ->setShareOwner('owner') + ->setNode($node); $this->defaultProvider->expects($this->once()) ->method('update') @@ -1871,7 +1872,7 @@ class ManagerTest extends \Test\TestCase { ]) ->getMock(); - $originalShare = new \OC\Share20\Share(); + $originalShare = $this->manager->newShare(); $originalShare->setShareType(\OCP\Share::SHARE_TYPE_LINK); $tomorrow = new \DateTime();