Skip to content

Commit

Permalink
Optimize join for sharedWith by moving logic to DB
Browse files Browse the repository at this point in the history
  • Loading branch information
mrow4a committed Mar 21, 2017
1 parent 53c0129 commit 12b8b8b
Show file tree
Hide file tree
Showing 9 changed files with 275 additions and 30 deletions.
8 changes: 8 additions & 0 deletions apps/federatedfilesharing/lib/FederatedShareProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,14 @@ public function getSharesByPath(Node $path) {
return $shares;
}


/**
* @inheritdoc
*/
public function getAllSharedWith($userId, $shareTypes, $node) {
$this->getSharedWith($userId, [self::SHARE_TYPE_REMOTE], $node, -1, 0);
}

/**
* @inheritdoc
*/
Expand Down
5 changes: 3 additions & 2 deletions apps/files_sharing/lib/MountProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,9 @@ public function __construct(IConfig $config, IManager $shareManager, ILogger $lo
* @return \OCP\Files\Mount\IMountPoint[]
*/
public function getMountsForUser(IUser $user, IStorageFactory $storageFactory) {
$shares = $this->shareManager->getSharedWith($user->getUID(), \OCP\Share::SHARE_TYPE_USER, null, -1);
$shares = array_merge($shares, $this->shareManager->getSharedWith($user->getUID(), \OCP\Share::SHARE_TYPE_GROUP, null, -1));
$requiredShareTypes = [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP];
$shares = $this->shareManager->getAllSharedWith($user->getUID(), $requiredShareTypes, null);

// filter out excluded shares and group shares that includes self
$shares = array_filter($shares, function (\OCP\Share\IShare $share) use ($user) {
return $share->getPermissions() > 0 && $share->getShareOwner() !== $user->getUID();
Expand Down
35 changes: 19 additions & 16 deletions apps/files_sharing/tests/MountProviderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -112,18 +112,19 @@ public function testExcludeShares() {
$this->makeMockShare(5, 100, 'user1', '/share4', 31),
];

$userGroupUserShares = array_merge($userShares, $groupShares);

$this->user->expects($this->any())
->method('getUID')
->will($this->returnValue('user1'));

$this->shareManager->expects($this->at(0))
->method('getSharedWith')
->with('user1', \OCP\Share::SHARE_TYPE_USER)
->will($this->returnValue($userShares));
$this->shareManager->expects($this->at(1))
->method('getSharedWith')
->with('user1', \OCP\Share::SHARE_TYPE_GROUP, null, -1)
->will($this->returnValue($groupShares));
$requiredShareTypes = [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP];
$this->shareManager->expects($this->once())
->method('getAllSharedWith')
->with('user1', $requiredShareTypes, null)
->will($this->returnValue($userGroupUserShares));
$this->shareManager->expects($this->never())
->method('getSharedWith');
$this->shareManager->expects($this->any())
->method('newShare')
->will($this->returnCallback(function() use ($rootFolder, $userManager) {
Expand Down Expand Up @@ -311,15 +312,17 @@ public function testMergeShares($userShares, $groupShares, $expectedShares, $mov
$this->user->expects($this->any())
->method('getUID')
->will($this->returnValue('user1'));

$userGroupUserShares = array_merge($userShares, $groupShares);
$requiredShareTypes = [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP];
$this->shareManager->expects($this->once())
->method('getAllSharedWith')
->with('user1', $requiredShareTypes, null)
->will($this->returnValue($userGroupUserShares));

$this->shareManager->expects($this->never())
->method('getSharedWith');

$this->shareManager->expects($this->at(0))
->method('getSharedWith')
->with('user1', \OCP\Share::SHARE_TYPE_USER)
->will($this->returnValue($userShares));
$this->shareManager->expects($this->at(1))
->method('getSharedWith')
->with('user1', \OCP\Share::SHARE_TYPE_GROUP, null, -1)
->will($this->returnValue($groupShares));
$this->shareManager->expects($this->any())
->method('newShare')
->will($this->returnCallback(function() use ($rootFolder, $userManager) {
Expand Down
63 changes: 63 additions & 0 deletions lib/private/Share20/DefaultShareProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -780,6 +780,69 @@ public function getSharedWith($userId, $shareType, $node, $limit, $offset) {
return $shares;
}


/**
* @inheritdoc
*/
public function getAllSharedWith($userId, $shareTypes, $node) {
$qb = $this->dbConn->getQueryBuilder();
// Create array of sharedWith objects (target user -> $userId or group of which user is a member
$user = $this->userManager->get($userId);
$allGroups = $this->groupManager->getUserGroups($user, 'sharing');

$sharedWithUserOrGroup = array_map(function(IGroup $group) { return $group->getGID(); }, $allGroups);
$sharedWithUserOrGroup[] = $userId;

/** @var Share[] $shares */
$shares = [];

//Get shares directly with this user
$sharedWithUserOrGroupChunks = array_chunk($sharedWithUserOrGroup, 100);
foreach ($sharedWithUserOrGroupChunks as $sharedWithUserOrGroupChunk) {
$qb->select('s.*', 'f.fileid', 'f.path')
->selectAlias('st.id', 'storage_string_id')
->from('share', 's')
->leftJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid'))
->leftJoin('f', 'storages', 'st', $qb->expr()->eq('f.storage', 'st.numeric_id'));

$qb->where($qb->expr()->in(
'share_type',
$qb->createNamedParameter($shareTypes, IQueryBuilder::PARAM_INT_ARRAY)
)
);

$qb->andWhere($qb->expr()->in('share_with', $qb->createNamedParameter(
$sharedWithUserOrGroupChunk,
IQueryBuilder::PARAM_STR_ARRAY
)));

// Filter by node if provided
if ($node !== null) {
$qb->andWhere($qb->expr()->eq('file_source', $qb->createNamedParameter($node->getId())));
}

$qb->andWhere($qb->expr()->orX(
$qb->expr()->eq('item_type', $qb->createNamedParameter('file')),
$qb->expr()->eq('item_type', $qb->createNamedParameter('folder'))
));

$cursor = $qb->execute();

while($data = $cursor->fetch()) {
if ($this->isAccessibleResult($data)) {
$share = $this->createShare($data);
if ($share->getShareType() === \OCP\Share::SHARE_TYPE_GROUP){
$shares[] = $this->resolveGroupShare($share, $userId);
} else {
$shares[] = $share;
}
}
}
$cursor->closeCursor();
}
return $shares;
}

/**
* Get a share by token
*
Expand Down
53 changes: 41 additions & 12 deletions lib/private/Share20/Manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,26 @@ public function __construct(
$this->sharingDisabledForUsersCache = new CappedMemoryCache();
}

/**
* @param int[] $shareTypes
* @return int[] $providerIdMap e.g. { "ocinternal" => { 0, 1 }[2] }[1]
*/
private function shareTypeToProviderMap($shareTypes) {
$providerIdMap = array();
foreach ($shareTypes as $shareType) {
// Get provider and its ID, at this point provider is cached at IProviderFactory instance
$provider = $this->factory->getProviderForType($shareType);
$providerId = $provider->identifier();

// Create a key -> multi value map
if (!isset($providerIdMap[$providerId])) {
$providerIdMap[$providerId] = array();
}
array_push($providerIdMap[$providerId], $shareType);
}
return $providerIdMap;
}

/**
* Convert from a full share id to a tuple (providerId, shareId)
*
Expand Down Expand Up @@ -876,7 +896,6 @@ public function moveShare(\OCP\Share\IShare $share, $recipientId) {
$provider->move($share, $recipientId);
}


/**
* @inheritdoc
*/
Expand All @@ -887,18 +906,8 @@ public function getAllSharesBy($userId, $shareTypes, $nodeIDs, $reshares = false
}
// This will ensure that if there are multiple share providers for the same share type, we will execute it in batches
$shares = array();
$providerIdMap = array();
foreach ($shareTypes as $shareType) {
// Get provider and its ID, at this point provider is cached at IProviderFactory instance
$provider = $this->factory->getProviderForType($shareType);
$providerId = $provider->identifier();

// Create a key -> multi value map
if (!isset($providerIdMap[$providerId])) {
$providerIdMap[$providerId] = array();
}
array_push($providerIdMap[$providerId], $shareType);
}
$providerIdMap = $this->shareTypeToProviderMap($shareTypes);

$today = new \DateTime();
foreach ($providerIdMap as $providerId => $shareTypeArray) {
Expand Down Expand Up @@ -1003,6 +1012,26 @@ public function getSharedWith($userId, $shareType, $node = null, $limit = 50, $o
return $provider->getSharedWith($userId, $shareType, $node, $limit, $offset);
}


/**
* @inheritdoc
*/
public function getAllSharedWith($userId, $shareTypes, $node = null) {
$shares = array();
$providerIdMap = $this->shareTypeToProviderMap($shareTypes);

foreach ($providerIdMap as $providerId => $shareTypeArray) {
// Get provider from cache
$provider = $this->factory->getProvider($providerId);

// Obtain shares for this type
$queriedShares = $provider->getAllSharedWith($userId, $shareTypeArray, $node);
$shares = array_merge($shares, $queriedShares);
}

return $shares;
}

/**
* @inheritdoc
*/
Expand Down
13 changes: 13 additions & 0 deletions lib/public/Share/IManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,19 @@ public function getAllSharesBy($userId, $shareTypes, $nodeIDs, $reshares = false
*/
public function getSharesBy($userId, $shareType, $path = null, $reshares = false, $limit = 50, $offset = 0);


/**
* Get shares shared with $userId for specified share types.
* Filter by $node if provided
*
* @param string $userId
* @param int[] $shareTypes
* @param Node|null $node
* @return IShare[]
* @since 10.0.0
*/
public function getAllSharedWith($userId, $shareTypes, $node = null);

/**
* Get shares shared with $user.
* Filter by $node if provided
Expand Down
12 changes: 12 additions & 0 deletions lib/public/Share/IShareProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,18 @@ public function getShareById($id, $recipientId = null);
*/
public function getSharesByPath(Node $path);


/**
* Get shared with the given user for shares of all available types for this share provider within all nodes
*
* @param string $userId get shares where this user is the recipient
* @param int[] $shareTypes
* @param Node|null $node
* @return \OCP\Share\IShare[]
* @since 10.0.0
*/
public function getAllSharedWith($userId, $shareTypes, $node);

/**
* Get shared with the given user
*
Expand Down
81 changes: 81 additions & 0 deletions tests/lib/Share20/DefaultShareProviderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1125,6 +1125,87 @@ public function testGetSharedWithGroupWithNode($storageStringId, $fileName1, $fi
$this->assertEquals(Share::SHARE_TYPE_GROUP, $share->getShareType());
}


/**
* @dataProvider storageAndFileNameProvider
*/
public function testGetAllShared($storageStringId, $fileName1, $fileName2) {
$storageId = $this->createTestStorageEntry($storageStringId);
$fileId1 = $this->createTestFileEntry($fileName1, $storageId);
$fileId2 = $this->createTestFileEntry($fileName2, $storageId);
$id0 = $this->addShareToDB(Share::SHARE_TYPE_USER, 'user0', 'user1', 'user1',
'file', $fileId1, 'myTarget', 31, null, null, null);
$id1 = $this->addShareToDB(Share::SHARE_TYPE_GROUP, 'group0', 'user1', 'user1',
'file', $fileId2, 'myTarget', 31, null, null, null);
$id2 = $this->addShareToDB(Share::SHARE_TYPE_USER, 'user0', 'user1', 'user1',
'file', $fileId2, 'myTarget', 31, null, null, null);

$user0 = $this->createMock(IUser::class);
$user0->method('getUID')->willReturn('user0');
$user1 = $this->createMock(IUser::class);
$user1->method('getUID')->willReturn('user1');

$this->userManager->method('get')->willReturnMap([
['user0', $user0],
['user1', $user1],
]);

$group0 = $this->createMock(IGroup::class);
$group0->method('getGID')->willReturn('group0');

$this->groupManager->method('get')->with('group0')->willReturn($group0);
$this->groupManager->method('getUserGroups')->with($user0)->willReturn([$group0]);

$this->rootFolder->method('getUserFolder')->with('user1')->will($this->returnSelf());
$node1 = $this->createMock(File::class);
$node1->method('getId')->willReturn($fileId1);
$node2 = $this->createMock(File::class);
$node2->method('getId')->willReturn($fileId2);
$this->rootFolder->method('getById')->willReturnCallback(function ($id) use ($node1, $node2) {
if($node1->getId() === $id){
return [$node1];
}
return [$node2];
});

// Check targeting specific node with $node1
$recShares = $this->provider->getAllSharedWith('user0', [Share::SHARE_TYPE_GROUP, Share::SHARE_TYPE_USER], $node1);
$this->assertCount(1, $recShares);
$share = $recShares[0];
$this->assertEquals($id0, $share->getId());
$this->assertSame('user0', $share->getSharedWith());
$this->assertSame('user1', $share->getShareOwner());
$this->assertSame('user1', $share->getSharedBy());
$this->assertEquals(Share::SHARE_TYPE_USER, $share->getShareType());
$shareNode = $share->getNode();
$this->assertSame($node1, $shareNode);

// Check targeting specific node with $node2
$recShares = $this->provider->getAllSharedWith('user0', [Share::SHARE_TYPE_GROUP, Share::SHARE_TYPE_USER], $node2);
$this->assertCount(2, $recShares);
foreach($recShares as $share) {
if ($share->getShareType() === Share::SHARE_TYPE_USER) {
$this->assertEquals($id2, $share->getId());
$this->assertSame('user0', $share->getSharedWith());
$this->assertSame('user1', $share->getShareOwner());
$this->assertSame('user1', $share->getSharedBy());
$shareNode = $share->getNode();
$this->assertSame($node2, $shareNode);
} else {
$this->assertEquals($id1, $share->getId());
$this->assertSame('group0', $share->getSharedWith());
$this->assertSame('user1', $share->getShareOwner());
$this->assertSame('user1', $share->getSharedBy());
$shareNode = $share->getNode();
$this->assertSame($node2, $shareNode);
}
}

// Check targeting all nodes with null
$recShares = $this->provider->getAllSharedWith('user0', [Share::SHARE_TYPE_GROUP, Share::SHARE_TYPE_USER], null);
$this->assertCount(3, $recShares);
}

public function shareTypesProvider() {
return [
[Share::SHARE_TYPE_USER, false],
Expand Down
Loading

0 comments on commit 12b8b8b

Please sign in to comment.