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

Reduce number of queries to read shares in a folder #34918

Closed
wants to merge 8 commits into from
Closed

Reduce number of queries to read shares in a folder #34918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
10bc136
Reduce number of queries to read shares
starypatyk Nov 1, 2022
1d3070b
An attempt to fix testGetShares unit test
starypatyk Nov 1, 2022
641968d
Experimental change - disable uid check
starypatyk Nov 3, 2022
7d57fbf
Fixes after review
starypatyk Nov 6, 2022
0aca9f1
Seems to run as expected with uid check disabled
starypatyk Nov 6, 2022
093e47e
Additional fixes after review (removed references)
starypatyk Nov 7, 2022
6054d48
Revert: Experimental change - disable uid check
starypatyk May 2, 2023
c7f5d11
Revert "Revert: Experimental change - disable uid check"
starypatyk Nov 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 5 additions & 49 deletions apps/files_sharing/lib/Controller/ShareAPIController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,13 +915,12 @@ private function getSharesInDir(Node $folder): array {
throw new OCSBadRequestException($this->l->t('Not a directory'));
}

$nodes = $folder->getDirectoryListing();

/** @var \OCP\Share\IShare[] $shares */
$shares = array_reduce($nodes, function ($carry, $node) {
$carry = array_merge($carry, $this->getAllShares($node, true));
come-nc marked this conversation as resolved.
Show resolved Hide resolved
return $carry;
}, []);
$shares = [];
$sharesInFolder = $this->shareManager->getSharesInFolder($this->currentUser, $folder, true);
foreach ($sharesInFolder as $key => $value) {
$shares = array_merge($shares, $value);
}

// filter out duplicate shares
$known = [];
Expand Down Expand Up @@ -1999,49 +1998,6 @@ private function shareProviderResharingRights(string $userId, IShare $share, $no
return false;
}

/**
* Get all the shares for the current user
*
* @param Node|null $path
* @param boolean $reshares
* @return IShare[]
*/
private function getAllShares(?Node $path = null, bool $reshares = false) {
// Get all shares
$userShares = $this->shareManager->getSharesBy($this->currentUser, IShare::TYPE_USER, $path, $reshares, -1, 0);
$groupShares = $this->shareManager->getSharesBy($this->currentUser, IShare::TYPE_GROUP, $path, $reshares, -1, 0);
$linkShares = $this->shareManager->getSharesBy($this->currentUser, IShare::TYPE_LINK, $path, $reshares, -1, 0);

// EMAIL SHARES
$mailShares = $this->shareManager->getSharesBy($this->currentUser, IShare::TYPE_EMAIL, $path, $reshares, -1, 0);

// CIRCLE SHARES
$circleShares = $this->shareManager->getSharesBy($this->currentUser, IShare::TYPE_CIRCLE, $path, $reshares, -1, 0);

// TALK SHARES
$roomShares = $this->shareManager->getSharesBy($this->currentUser, IShare::TYPE_ROOM, $path, $reshares, -1, 0);

// DECK SHARES
$deckShares = $this->shareManager->getSharesBy($this->currentUser, IShare::TYPE_DECK, $path, $reshares, -1, 0);

// SCIENCEMESH SHARES
$sciencemeshShares = $this->shareManager->getSharesBy($this->currentUser, IShare::TYPE_SCIENCEMESH, $path, $reshares, -1, 0);

// FEDERATION
if ($this->shareManager->outgoingServer2ServerSharesAllowed()) {
$federatedShares = $this->shareManager->getSharesBy($this->currentUser, IShare::TYPE_REMOTE, $path, $reshares, -1, 0);
} else {
$federatedShares = [];
}
if ($this->shareManager->outgoingServer2ServerGroupSharesAllowed()) {
$federatedGroupShares = $this->shareManager->getSharesBy($this->currentUser, IShare::TYPE_REMOTE_GROUP, $path, $reshares, -1, 0);
} else {
$federatedGroupShares = [];
}

return array_merge($userShares, $groupShares, $linkShares, $mailShares, $circleShares, $roomShares, $deckShares, $sciencemeshShares, $federatedShares, $federatedGroupShares);
}


/**
* merging already formatted shares.
Expand Down
16 changes: 16 additions & 0 deletions apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1458,6 +1458,22 @@ function ($user, $shareType, $node) use ($shares) {
->method('outgoingServer2ServerGroupSharesAllowed')
->willReturn($extraShareTypes[ISHARE::TYPE_REMOTE_GROUP] ?? false);

$sharesWithoutTypes = [];
foreach ($shares as $file => $fileShares) {
$sharesWithoutTypes[$file] = [];
foreach ($fileShares as $shareType => $shareTypeShares) {
if ($shareType === ISHARE::TYPE_REMOTE or $shareType === ISHARE::TYPE_REMOTE_GROUP) {
if ($extraShareTypes[$shareType] ?? false) {
$sharesWithoutTypes[$file] = array_merge($sharesWithoutTypes[$file], $shareTypeShares);
}
} else {
$sharesWithoutTypes[$file] = array_merge($sharesWithoutTypes[$file], $shareTypeShares);
}
}
}
$this->shareManager
->method('getSharesInFolder')->willReturn($sharesWithoutTypes);

$this->groupManager
->method('isInGroup')
->willReturnCallback(
Expand Down
7 changes: 0 additions & 7 deletions lib/private/Share20/DefaultShareProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,13 +674,6 @@ public function getSharesInFolder($userId, Folder $node, $reshares, $shallow = t
*/
if ($reshares === false) {
$qb->andWhere($qb->expr()->eq('uid_initiator', $qb->createNamedParameter($userId)));
} else {
$qb->andWhere(
$qb->expr()->orX(
$qb->expr()->eq('uid_owner', $qb->createNamedParameter($userId)),
$qb->expr()->eq('uid_initiator', $qb->createNamedParameter($userId))
)
);
Comment on lines -677 to -683
Copy link
Member

Choose a reason for hiding this comment

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

Removing this means we don't filter by $userId at all anymore with $reshares = true which is almost certainly incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am also concerned about this change.

I have reverted it to check once again and I see that three integration tests fail consistently - in exactly the same way as when I started the PR.

--- Failed scenarios:

    /drone/src/build/integration/sharing_features/sharing-v1-part2.feature:306
    /drone/src/build/integration/sharing_features/sharing-v1-part2.feature:381
    /drone/src/build/integration/sharing_features/sharing-v1-part2.feature:406

Please see my comments above - esp. this one and this one.

I would welcome any suggestion how to fix this in a better way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @icewind1991,

I have looked once again into this issue. I think I have a better understanding now.

One of the failing tests describes the following scenario:

server/build/integration/sharing_features/sharing-v1-part2.feature

Lines 306 to 318 in 96f0118

Scenario: getting all shares including subfiles in a directory by a resharer
Given user "user0" exists
And user "user1" exists
And user "user2" exists
And user "user3" exists
And file "PARENT" of user "user0" is shared with user "user1"
And user "user1" accepts last share
And file "PARENT (2)/CHILD" of user "user1" is shared with user "user2"
And user "user2" accepts last share
And file "CHILD" of user "user2" is shared with user "user3"
When As an "user1"
And sending "GET" to "/apps/files_sharing/api/v1/shares?subfiles=true&path=PARENT (2)"
Then the list of returned shares has 2 shares

This scenario expects two shares to be returned for user1: user1 -> user2 and user2 -> user3. Obviously in the second share user1 is neither owner nor initiator - but the share should be returned anyway.

I have recreated data for this scenario in my test environment and tried to repeat the issue from the UI. I have not been able to repeat this in the UI, but I have noticed a discrepancy between the following two API calls:

  1. Retrieve shares for the /PARENT (2)/CHILD folder (used in the UI)
/ocs/v2.php/apps/files_sharing/api/v1/shares?path=%2FPARENT%20\(2\)%2FCHILD&reshares=true
  1. Retrieve shares for the /PARENT (2) folder and its subfolders/files (used in the test)
/ocs/v2.php/apps/files_sharing/api/v1/shares?path=%2FPARENT%20\(2\)&reshares=true&subfiles=true

In the first case the shares are retrieved by DefaultShareProvider::getSharesBy‎ that contains the following fragment to handle the reshares mode:

server/lib/private/Share20/DefaultShareProvider.php

Lines 760 to 771 in 96f0118

if ($reshares === false) {
$qb->andWhere($qb->expr()->eq('uid_initiator', $qb->createNamedParameter($userId)));
} else {
if ($node === null) {
$qb->andWhere(
$qb->expr()->orX(
$qb->expr()->eq('uid_owner', $qb->createNamedParameter($userId)),
$qb->expr()->eq('uid_initiator', $qb->createNamedParameter($userId))
)
);
}
}

Here the userId parameter is ignored in reshares mode when the node parameter is specified.

Before my changes the second API call was handled by invoking getSharesBy individually for each item in the folder.

In this PR I want to reuse better handling already available in DefaultShareProvider::getSharesInFolder.

So, to achieve consistency with previous behaviour - I think that the userId check should be removed from the getSharesInFolder method. The folder node must be provided, the code always adds an additional filter to retrieve only shares relevant to files/subfolders inside the provided folder.

server/lib/private/Share20/DefaultShareProvider.php

Lines 696 to 718 in 96f0118

$childMountNodes = array_filter($node->getDirectoryListing(), function (Node $node): bool {
return $node->getInternalPath() === '';
});
$childMountRootIds = array_map(function (Node $node): int {
return $node->getId();
}, $childMountNodes);
$qb->innerJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid'));
if ($shallow) {
$qb->andWhere(
$qb->expr()->orX(
$qb->expr()->eq('f.parent', $qb->createNamedParameter($node->getId())),
$qb->expr()->in('f.fileid', $qb->createParameter('chunk'))
)
);
} else {
$qb->andWhere(
$qb->expr()->orX(
$qb->expr()->like('f.path', $qb->createNamedParameter($this->dbConn->escapeLikeParameter($node->getInternalPath()) . '/%')),
$qb->expr()->in('f.fileid', $qb->createParameter('chunk'))
)
);
}

Is my understanding correct? Or have I overlooked something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @icewind1991,

Trying once again to move this PR forward. I rebased the changes yesterday and once again removed the uid check in reshares mode. Please see my explanation in the comment above.

This is the last major item still open on the list of performance issues in the Android app that I started over a year ago.

Would be great to get this merged.

}

// todo? maybe get these from the oc_mounts table
Expand Down
Loading