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 23, 2017
1 parent 82ee84e commit 20bc46b
Show file tree
Hide file tree
Showing 9 changed files with 508 additions and 38 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, $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
111 changes: 111 additions & 0 deletions lib/private/Share20/DefaultShareProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* @author phisch <[email protected]>
* @author Roeland Jago Douma <[email protected]>
* @author Vincent Petry <[email protected]>
* @author Piotr Mrowczynski <[email protected]>
*
* @copyright Copyright (c) 2017, ownCloud GmbH
* @license AGPL-3.0
Expand Down Expand Up @@ -788,6 +789,116 @@ public function getSharedWith($userId, $shareType, $node, $limit, $offset) {
return $shares;
}


/**
* @inheritdoc
*/
public function getAllSharedWith($userId, $node) {
// 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');

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

// Check if user is member of some groups and chunk them
$sharedWithGroup = array_map(function(IGroup $group) { return $group->getGID(); }, $allGroups);
$sharedWithGroupChunks = array_chunk($sharedWithGroup, 100);

// Check how many group chunks do we need
$sharedWithGroupChunksNo = count($sharedWithGroupChunks);

// Check that if there are no groups for users, we still query for a shared with user
$chunkNoRequired = $sharedWithGroupChunksNo > 0 ? $sharedWithGroupChunksNo : 1;

for ($chunkNo = 0; $chunkNo < $chunkNoRequired; $chunkNo++) {
// Query for user/group shares
$qb = $this->dbConn->getQueryBuilder();
$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'));

// Apply predicate on item_type
$qb->where($qb->expr()->orX(
$qb->expr()->eq('item_type', $qb->createNamedParameter('file')),
$qb->expr()->eq('item_type', $qb->createNamedParameter('folder'))
));

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

// Handle chunking logic
if ($chunkNo === 0 && $sharedWithGroupChunksNo > 0) {
// There are groups to be checked and this is first chunk
// In the first chunk, check for shared with user along with groups
$groups = $sharedWithGroupChunks[$chunkNo];
$qb->andWhere($qb->expr()->orX(
$qb->expr()->andX(
$qb->expr()->eq('share_type', $qb->createNamedParameter(\OCP\Share::SHARE_TYPE_GROUP)),
$qb->expr()->in('share_with', $qb->createNamedParameter(
$groups,
IQueryBuilder::PARAM_STR_ARRAY
))
),
$qb->expr()->andX(
$qb->expr()->eq('share_type', $qb->createNamedParameter(\OCP\Share::SHARE_TYPE_USER)),
$qb->expr()->eq('share_with', $qb->createNamedParameter($userId))
)
));
} else if ($sharedWithGroupChunksNo > 0) {
// There are groups to be checked and this is consecutive chunk
$groups = $sharedWithGroupChunks[$chunkNo];
$qb->andWhere(
$qb->expr()->andX(
$qb->expr()->eq('share_type', $qb->createNamedParameter(\OCP\Share::SHARE_TYPE_GROUP)),
$qb->expr()->in('share_with', $qb->createNamedParameter(
$groups,
IQueryBuilder::PARAM_STR_ARRAY
))
)
);
} else {
// There are groups to be checked
$qb->andWhere(
$qb->expr()->andX(
$qb->expr()->eq('share_type', $qb->createNamedParameter(\OCP\Share::SHARE_TYPE_USER)),
$qb->expr()->eq('share_with', $qb->createNamedParameter($userId))
)
);
}

$cursor = $qb->execute();

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

// Resolve shares
// TODO: will be optimised by https://github.com/owncloud/core/pull/27434
$resolveGroupShares = [];
foreach($groupShares as $groupShare) {
$resolveGroupShares[] = $this->resolveGroupShare($groupShare, $userId);
}

// Merge and return
$resolvedShares = array_merge($shares, $resolveGroupShares);
return $resolvedShares;
}

/**
* Get a share by token
*
Expand Down
54 changes: 42 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 \OC\Share\Constants[] $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,27 @@ 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();

// Aggregate all required $shareTypes by mapping provider to supported shareTypes
$providerIdMap = $this->shareTypeToProviderMap($shareTypes);
foreach ($providerIdMap as $providerId => $shareTypeArray) {
// Get provider from cache
$provider = $this->factory->getProvider($providerId);

// Obtain all shares for all the supported provider types
$queriedShares = $provider->getAllSharedWith($userId, $node);
$shares = array_merge($shares, $queriedShares);
}

return $shares;
}

/**
* @inheritdoc
*/
Expand Down
19 changes: 16 additions & 3 deletions lib/public/Share/IManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public function moveShare(IShare $share, $recipientId);
* Get all shares shared by (initiated) by the provided user for specific node IDs.
*
* @param string $userId
* @param int[] $shareTypes
* @param \OC\Share\Constants[] $shareTypes
* @param int[] $nodeIDs
* @param bool $reshares
* @return IShare[]
Expand All @@ -102,7 +102,7 @@ public function getAllSharesBy($userId, $shareTypes, $nodeIDs, $reshares = false
* Get shares shared by (initiated) by the provided user.
*
* @param string $userId
* @param int $shareType
* @param \OC\Share\Constants $shareType
* @param Node|null $path
* @param bool $reshares
* @param int $limit The maximum number of returned results, -1 for all results
Expand All @@ -112,12 +112,25 @@ 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 \OC\Share\Constants[] $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
*
* @param string $userId
* @param int $shareType
* @param \OC\Share\Constants $shareType
* @param Node|null $node
* @param int $limit The maximum number of shares returned, -1 for all
* @param int $offset
Expand Down
23 changes: 18 additions & 5 deletions lib/public/Share/IShareProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public function move(\OCP\Share\IShare $share, $recipient);
* Get all shares by the given user
*
* @param string $userId
* @param int $shareType
* @param \OC\Share\Constants $shareType
* @param Node|null $node
* @param bool $reshares Also get the shares where $user is the owner instead of just the shares where $user is the initiator
* @param int $limit The maximum number of shares to be returned, -1 for all shares
Expand All @@ -108,7 +108,7 @@ public function getSharesBy($userId, $shareType, $node, $reshares, $limit, $offs
* Get all shares by the given user for specified shareTypes array
*
* @param string $userId
* @param int[] $shareTypes
* @param \OC\Share\Constants[] $shareTypes
* @param Node[] $nodeIDs
* @param bool $reshares - Also get the shares where $user is the owner instead of just the shares where $user is the initiator
* @return \OCP\Share\IShare[]
Expand Down Expand Up @@ -136,11 +136,24 @@ public function getShareById($id, $recipientId = null);
*/
public function getSharesByPath(Node $path);


/**
* Get shared with the given user for shares of all supported share types for this share provider,
* with file_source predicate specified ($node is Node) or
* without ($node is null and scan over file_source is performed).
*
* @param string $userId get shares where this user is the recipient
* @param Node|null $node
* @return \OCP\Share\IShare[]
* @since 10.0.0
*/
public function getAllSharedWith($userId, $node);

/**
* Get shared with the given user
* Get shared with the given user specifying share type predicate for this specific share provider
*
* @param string $userId get shares where this user is the recipient
* @param int $shareType
* @param \OC\Share\Constants $shareType
* @param Node|null $node
* @param int $limit The max number of entries returned, -1 for all
* @param int $offset
Expand All @@ -164,7 +177,7 @@ public function getShareByToken($token);
* So clean up the relevant shares.
*
* @param string $uid
* @param int $shareType
* @param \OC\Share\Constants $shareType
* @since 9.1.0
*/
public function userDeleted($uid, $shareType);
Expand Down
Loading

0 comments on commit 20bc46b

Please sign in to comment.