-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
fix: reduce sql queries in Share::getUsersSharingFile() #41111
base: master
Are you sure you want to change the base?
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
2f5953e
to
c327327
Compare
c327327
to
403d025
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we taken into account the error handling? I see some if (\OCP\DB::isError($result)) {
code removed, but not the matching code.
$s = preg_replace('/\s\s+/', ' ', $this->sql); | ||
foreach ($this->params as $i => $param) { | ||
$param = json_encode($param); | ||
$pos = strpos($s, '?'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 🤷
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ....
@@ -182,125 +185,94 @@ public static function getUsersSharingFile($path, $ownerUser, $includeOwner = fa | |||
} | |||
} | |||
|
|||
// let's get the parent for the next round | |||
if ($recursive === true) { | |||
while ($source !== -1) { |
There was a problem hiding this comment.
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.
$source = (int)$meta['parent']; | ||
$sources[]= $source; | ||
} else { | ||
break; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
$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))) |
There was a problem hiding this comment.
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.
lib/private/Share/Share.php
Outdated
->andWhere($qb->expr()->in('share_type', $qb->createNamedParameter([3, 6], Connection::PARAM_INT_ARRAY))) | ||
->andWhere($qb->expr()->in('item_type', $qb->createNamedParameter(['file', 'folder'], Connection::PARAM_STR_ARRAY))); | ||
$result = $qb->execute(); | ||
$rows = $result->fetchAllAssociative(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if the query is limited enough to fetch all rows at once. We could clarify with a comment that the limiting factor is that we'll search a few item sources, so we expect 10-20 entries at most (which should be fine).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was doing my calcs based on the fact that this can be 2 entries per folder depth - but this is wrong.
There can be by far more shares.
Will adapt this. THX for the hint 🙏
To my understanding in case of errors we will receive exception from dbal. These checks are pointless from my understanding. But feel free to prove me wrong. |
403d025
to
39bb864
Compare
Kudos, SonarCloud Quality Gate passed! |
So the idea is to let the DBAL exception propagate upwards I guess. I'm ok with that, although I don't know how ownCloud will behave. |
this was already the case for the old code from my understanding. |
39bb864
to
a5e3026
Compare
That's what you get when adding unit tests to legacy code 🙈
|
master is also generating the path without the leading slash .... why ever .... |
a5e3026
to
5d72d79
Compare
5d72d79
to
ccf3b98
Compare
Description
Save the 🪐 - save CPU, network, memory, energy 👯
Number of queries of
Share::getUsersSharingFile()
has been reduced from 17+ to 8Motivation and Context
Be smart on sql queries
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: