Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: reduce sql queries in Share::getUsersSharingFile() #41111

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: reduce sql queries in Share::getUsersSharingFile() #41111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 17 additions & 0 deletions lib/private/Diagnostics/Query.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,23 @@ public function getDuration() {
return $this->end - $this->start;
}

public function getFullSql(): string {
$s = preg_replace('/\s\s+/', ' ', $this->sql);
foreach ($this->params as $i => $param) {
$param = json_encode($param);
# NOTE: this might not result in a perfect sql query because ? must not be a placeholder
# nevertheless for analysis purpose this is a good enough approach
$pos = strpos($s, '?');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be problematic if the query contains ? which aren't placeholders.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed - but I consider this a minor issue for the time being as this is for pure debugging purpose.
I did not find a better way. 🤷

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The json-serialized data looks good to me, just return the query and the parameters separately.
Anyway, I think we should add a TODO / FIXME at least so we're aware of this issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The json-serialized data looks good to me, just return the query and the parameters separately.

this has a very bad DX as it requires brain power to interpolate the strings. Does not really help to analyse the sql chaos we have ....

if ($pos !== false) {
$s = substr_replace($s, $param, $pos, \strlen('?'));
} else {
$s = str_replace(":$i", $param, $s);
}
}

return $s;
}

public function jsonSerialize() {
return [
'query' => $this->sql,
Expand Down
224 changes: 97 additions & 127 deletions lib/private/Share/Share.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
use Doctrine\DBAL\Connection;
use OC\Files\Filesystem;
use OC\Group\Group;
use OC\User\NoUserException;
use OCA\FederatedFileSharing\DiscoveryManager;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IUser;
Expand Down Expand Up @@ -151,7 +152,7 @@ public static function getUsersSharingFile($path, $ownerUser, $includeOwner = fa
if ($userObject === null) {
$msg = "Backends provided no user object for $ownerUser";
\OC::$server->getLogger()->error($msg, ['app' => __CLASS__]);
throw new \OC\User\NoUserException($msg);
throw new NoUserException($msg);
}

$ownerUser = $userObject->getUID();
Expand All @@ -160,7 +161,7 @@ public static function getUsersSharingFile($path, $ownerUser, $includeOwner = fa
$shares = $sharePaths = $fileTargets = [];
$publicShare = false;
$remoteShare = false;
$source = -1;
$sources = [];
$cache = $mountPath = false;

$view = new \OC\Files\View('/' . $ownerUser . '/files');
Expand All @@ -172,8 +173,10 @@ public static function getUsersSharingFile($path, $ownerUser, $includeOwner = fa
$meta = $view->getFileInfo(\dirname($path));
}

$source = -1;
if ($meta !== false) {
$source = $meta['fileid'];
$sources[]= $source;
$cache = new \OC\Files\Cache\Cache($meta['storage']);

$mountPath = $meta->getMountPoint()->getMountPoint();
Expand All @@ -182,127 +185,96 @@ public static function getUsersSharingFile($path, $ownerUser, $includeOwner = fa
}
}

$connection = \OC::$server->getDatabaseConnection();

// let's get the parent for the next round
$paths = [];
$rejected = [];
# TODO: performance consideration - build list of all parent folders up front - don't loop
while ($source !== -1) {
// Fetch all shares with another user
if (!$returnUserPaths) {
$query = \OC_DB::prepare(
'SELECT `parent`, `share_with`, `file_source`, `file_target`, `accepted`
FROM
`*PREFIX*share`
WHERE
`item_source` = ? AND `share_type` = ? AND `item_type` IN (\'file\', \'folder\')'
);
$result = $query->execute([$source, self::SHARE_TYPE_USER]);
} else {
$query = \OC_DB::prepare(
'SELECT `parent`, `share_with`, `file_source`, `file_target`, `accepted`
FROM
`*PREFIX*share`
WHERE
`item_source` = ? AND `share_type` IN (?, ?) AND `item_type` IN (\'file\', \'folder\')'
);
$result = $query->execute([$source, self::SHARE_TYPE_USER, self::$shareTypeGroupUserUnique]);
}

if (\OCP\DB::isError($result)) {
\OCP\Util::writeLog('OCP\Share', \OC_DB::getErrorMessage(), \OCP\Util::ERROR);
} else {
while ($row = $result->fetchRow()) {
if ((int)($row['accepted']) === Constants::STATE_REJECTED) {
$rejected[]= $row['parent'];
continue;
}
$shares[] = $row['share_with'];
if ($returnUserPaths) {
$fileTargets[(int) $row['file_source']][$row['share_with']] = $row;
}
}
}

// We also need to take group shares into account
$qb = $connection->getQueryBuilder();
$qb->select('share_with', 'file_source', 'file_target')
->from('share')
->where($qb->expr()->eq('item_source', $qb->createNamedParameter($source)))
->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(self::SHARE_TYPE_GROUP)))
->andWhere($qb->expr()->in('item_type', $qb->createNamedParameter(['file', 'folder'], Connection::PARAM_STR_ARRAY)));

if (!empty($rejected)) {
$qb->andWhere($qb->expr()->notIn('id', $qb->createNamedParameter($rejected, Connection::PARAM_INT_ARRAY)));
}
$result = $qb->execute();

while ($row = $result->fetch()) {
$usersInGroup = self::usersInGroup($row['share_with']);
$shares = \array_merge($shares, $usersInGroup);
if ($returnUserPaths) {
foreach ($usersInGroup as $user) {
if (!isset($fileTargets[(int) $row['file_source']][$user])) {
// When the user already has an entry for this file source
// the file is either shared directly with him as well, or
// he has an exception entry (because of naming conflict).
$fileTargets[(int) $row['file_source']][$user] = $row;
}
}
if ($recursive === true) {
while ($source !== -1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we'll have to assume that there will always be a "source == 1" entry. I think the request will crash with an "out of memory" error in case of an infinite loop, but I don't know if there is anything we could do to pinpoint where the problem is if that happens.

$meta = $cache->get((int)$source);
if ($meta !== false) {
$paths[$source] = $meta['path'];
$source = (int)$meta['parent'];
$sources[]= $source;
} else {
\OC::$server->getLogger()->debug("Share::getUsersSharingFile: could not find $source in file cache.");
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should log something in this case. If we don't get the parent it means that part of the tree structure is broken, which seems serious to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 means we hit the root of the tree and we can stop.

Logic is basically unchanged it was extracted to prevent executing sql in the loop.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we hit this break if $cache->get((int)$source) === false.
I mean, assuming we have a working tree, we'll leave the loop "naturally" because the $source will be -1 eventually. However, we enter this break if we don't get the cache info from the source file; if this happens, we might have a broken tree because we can't reach the root of the storage, so I think we should at least log something.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will have a second look ... as stated above: this code is working ™️ this way for quite a long time this way .....
Nevertheless you might be right ..... thx

}
}
}

//check for public link shares
if (!$publicShare) {
$query = \OC_DB::prepare(
'
SELECT `share_with`
FROM `*PREFIX*share`
WHERE `item_source` = ? AND `share_type` = ? AND `item_type` IN (\'file\', \'folder\')',
1
);
$connection = \OC::$server->getDatabaseConnection();

$result = $query->execute([$source, self::SHARE_TYPE_LINK]);
$rejected = [];
// Fetch all shares with another user
$qb = $connection->getQueryBuilder();
$qb->select('parent', 'share_with', 'file_source', 'file_target', 'accepted')
->from('share')
->where($qb->expr()->in('item_source', $qb->createNamedParameter($sources, Connection::PARAM_INT_ARRAY)))
->andWhere($qb->expr()->in('item_type', $qb->createNamedParameter(['file', 'folder'], Connection::PARAM_STR_ARRAY)));

if (\OCP\DB::isError($result)) {
\OCP\Util::writeLog('OCP\Share', \OC_DB::getErrorMessage(), \OCP\Util::ERROR);
} else {
if ($result->fetchRow()) {
$publicShare = true;
}
}
if ($returnUserPaths) {
$qb->andWhere($qb->expr()->in('share_type', $qb->createNamedParameter([self::SHARE_TYPE_USER, self::$shareTypeGroupUserUnique], Connection::PARAM_INT_ARRAY)));
} else {
$qb->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(self::SHARE_TYPE_USER)));
}
$result = $qb->execute();
while ($row = $result->fetch()) {
if ((int)($row['accepted']) === Constants::STATE_REJECTED) {
$rejected[]= $row['parent'];
continue;
}
$shares[] = $row['share_with'];
if ($returnUserPaths) {
$fileTargets[(int) $row['file_source']][$row['share_with']] = $row;
}
}
$result->free();

//check for remote share
if (!$remoteShare) {
$query = \OC_DB::prepare(
'
SELECT `share_with`
FROM `*PREFIX*share`
WHERE `item_source` = ? AND `share_type` = ? AND `item_type` IN (\'file\', \'folder\')',
1
);
// We also need to take group shares into account
$qb = $connection->getQueryBuilder();
$qb->select('share_with', 'file_source', 'file_target')
->from('share')
->where($qb->expr()->in('item_source', $qb->createNamedParameter($sources, Connection::PARAM_INT_ARRAY)))
->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(self::SHARE_TYPE_GROUP)))
->andWhere($qb->expr()->in('item_type', $qb->createNamedParameter(['file', 'folder'], Connection::PARAM_STR_ARRAY)));

$result = $query->execute([$source, self::SHARE_TYPE_REMOTE]);
if (!empty($rejected)) {
$qb->andWhere($qb->expr()->notIn('id', $qb->createNamedParameter($rejected, Connection::PARAM_INT_ARRAY)));
}
$result = $qb->execute();

if (\OCP\DB::isError($result)) {
\OCP\Util::writeLog('OCP\Share', \OC_DB::getErrorMessage(), \OCP\Util::ERROR);
} else {
if ($result->fetchRow()) {
$remoteShare = true;
while ($row = $result->fetch()) {
$usersInGroup = self::usersInGroup($row['share_with']);
$shares = \array_merge($shares, $usersInGroup);
if ($returnUserPaths) {
foreach ($usersInGroup as $user) {
if (!isset($fileTargets[(int) $row['file_source']][$user])) {
// When the user already has an entry for this file source
// the file is either shared directly with him as well, or
// he has an exception entry (because of naming conflict).
$fileTargets[(int) $row['file_source']][$user] = $row;
}
}
}
}
$result->free();

// let's get the parent for the next round
$meta = $cache->get((int)$source);
if ($recursive === true && $meta !== false) {
$paths[$source] = $meta['path'];
$source = (int)$meta['parent'];
} else {
$source = -1;
//check for public link shares
$qb = $connection->getQueryBuilder();
$qb->select('share_with', 'share_type')
->from('share')
->where($qb->expr()->in('item_source', $qb->createNamedParameter($sources, Connection::PARAM_INT_ARRAY)))
->andWhere($qb->expr()->in('share_type', $qb->createNamedParameter([3, 6], Connection::PARAM_INT_ARRAY)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should use constants here.

->andWhere($qb->expr()->in('item_type', $qb->createNamedParameter(['file', 'folder'], Connection::PARAM_STR_ARRAY)));
$result = $qb->execute();
while ($row = $result->fetch()) {
if ((int)($row['share_type']) === self::SHARE_TYPE_LINK) {
$publicShare = true;
}
if ((int)($row['share_type']) === self::SHARE_TYPE_REMOTE) {
$remoteShare = true;
}
}
$result->free();

// Include owner in list of users, if requested
if ($includeOwner) {
Expand All @@ -314,30 +286,28 @@ public static function getUsersSharingFile($path, $ownerUser, $includeOwner = fa
$fileTargetIDs = \array_unique($fileTargetIDs);

if (!empty($fileTargetIDs)) {
$query = \OC_DB::prepare(
'SELECT `fileid`, `path`
FROM `*PREFIX*filecache`
WHERE `fileid` IN (' . \implode(',', $fileTargetIDs) . ')'
);
$result = $query->execute();
$qb = $connection->getQueryBuilder();
$qb->select('fileid', 'path')
->from('filecache')
->where($qb->expr()->in('fileid', $qb->createNamedParameter($fileTargetIDs, Connection::PARAM_INT_ARRAY)));

if (\OCP\DB::isError($result)) {
\OCP\Util::writeLog('OCP\Share', \OC_DB::getErrorMessage(), \OCP\Util::ERROR);
} else {
while ($row = $result->fetchRow()) {
foreach ($fileTargets[$row['fileid']] as $uid => $shareData) {
if ($mountPath !== false) {
$sharedPath = $shareData['file_target'];
$sharedPath .= \substr($path, \strlen($mountPath) + \strlen($paths[$row['fileid']]));
$sharePaths[$uid] = $sharedPath;
} else {
$sharedPath = $shareData['file_target'];
$sharedPath .= \substr($path, \strlen($row['path']) -5);
$sharePaths[$uid] = $sharedPath;
}
$result = $qb->execute();

while ($row = $result->fetch()) {
foreach ($fileTargets[$row['fileid']] as $uid => $shareData) {
if ($mountPath !== false) {
$sharedPath = $shareData['file_target'];
# TODO: this produces spooky results
$sharedPath .= \substr($path, \strlen($mountPath) + \strlen($paths[$row['fileid']]));
$sharePaths[$uid] = $sharedPath;
} else {
$sharedPath = $shareData['file_target'];
$sharedPath .= \substr($path, \strlen($row['path']) -5);
$sharePaths[$uid] = $sharedPath;
}
}
}
$result->free();
}

if ($includeOwner) {
Expand Down
2 changes: 1 addition & 1 deletion lib/public/IDBConnection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public function getQueryBuilder();
* @param string $sql the sql query with ? placeholder for params
* @param int $limit the maximum number of rows
* @param int $offset from which row we want to start
* @return \Doctrine\DBAL\Driver\Statement The prepared statement.
* @return \Doctrine\DBAL\Statement The prepared statement.
* @since 6.0.0
*/
public function prepare($sql, $limit=null, $offset=null);
Expand Down