feat: Use file cache query builder for searching for favorites #39303

juliushaertl · 2023-07-11T09:08:21Z

When trying to REPORT files that are favorites we still used the old code path to aggregate them, this moves this over to the filecache search query builder methods.

Request to trigger:

blackfire curl -X REPORT \
  --url http://admin:admin@nextcloud.local/remote.php/dav/files/admin/MyGroupfolder4 \
  --header 'Content-Type: application/xml' \
  --data '<?xml version="1.0"?>
<oc:filter-files  xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns" xmlns:nc="http://nextcloud.org/ns">
         <oc:filter-rules>
                 <oc:favorite>1</oc:favorite>
         </oc:filter-rules>
        <d:prop>
            <d:resourcetype />
        </d:prop>
 </oc:filter-files> '

TODO

...

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

apps/dav/lib/Connector/Sabre/FilesReportPlugin.php

+		}
+
+		if ($favoriteFilter !== null) {
+			$tmpNodes = $this->userFolder->searchByTag(ITags::TAG_FAVORITE, $this->userSession->getUser()->getUID());


blizzz

The favorites logic in processFilterRulesForFileIDs() should be removed, also.

All the comments are no-brainers and I understand due to the fact this being an early draft. The approach is fine!

apps/dav/lib/Connector/Sabre/FilesReportPlugin.php

blizzz · 2023-07-11T10:26:36Z

lib/private/Files/Node/Folder.php

@@ -295,8 +295,8 @@ public function searchByMime($mimetype) {
 	 * @param string $userId owner of the tags
 	 * @return Node[]
 	 */
-	public function searchByTag($tag, $userId) {
-		$query = $this->queryFromOperator(new SearchComparison(ISearchComparison::COMPARE_EQUAL, 'tagname', $tag), $userId);
+	public function searchByTag($tag, $userId, int $limit = 0, int $offset = 0) {


I suppose the interface needs to be extended as well? Though I don't see a psalm warning.
LazyFolder should look the same, alone for consistency.

Adjusted, would need another adjustment in groupfolders again once merged.

For potential backports this is a bit more tricky but we may just use an ugly direct call on the private class implementation of a separate method then to avoid the interface adjustment.

The foundational PRs were also backported and there the Folder changes were indeed made private. Since the signature is different, it's likely taking more to make at least tooling accept that state.

apps/dav/lib/Connector/Sabre/FilesReportPlugin.php

juliushaertl · 2023-07-12T14:27:56Z

Ready for review now

blizzz · 2023-07-12T17:11:05Z

lib/private/Files/Node/NonExistingFolder.php

@@ -150,7 +150,7 @@ public function searchByMime($mimetype) {
 		throw new NotFoundException();
 	}

-	public function searchByTag($tag, $userId) {
+	public function searchByTag($tag, $userId, int $limit = 0, int $offset = 0) {


That class is a good finding, i shall add searchBySystemTag here also – will be a separate PR.

marcelklehr · 2023-07-13T13:06:51Z

apps/dav/lib/Connector/Sabre/FilesReportPlugin.php

+		}
+
+		if ($this->hasFilterFavorites($filterRules)) {
+			$tmpNodes = $this->userFolder->searchByTag(ITags::TAG_FAVORITE, $this->userSession->getUser()->getUID(), $limit ?? 0, $offset ?? 0);


Isn't this this thing again where we slice multiple times and then intersect the results? Does this give proper results? I forgot.

limit = 3 offset = 2 a = [0,3,6,9,12,15,18,21,24] b = [0,2,4,6,8,10,12,14,16,18,20,22,24] slice(a) // 6,9,12 slice(b) // 4,6,8 intersect(slice(a), slice(b)) // 6 intersect(a,b) // 0,6,12,18,24 slice(intersect(a,b)) // 12,18,24

So, it can give too little results. Sometimes even none when some would be expected.

Good point, will see what could be done about that

Since both the searchBySystemTag and searchByTag end up doing a search with their specific search queries anyway. You can instead build a single search query that performs all the filtering in one go.

and you can probably also include the filtering for the fileids from processFilterRulesForFileIDs in the same query to remove the need for the findNodesByFileIds later down

You can instead build a single search query that performs all the filtering in one go.

Search stuff is not in OCP, though. Would be awesome if it was.

using it in the dav app is fine since it's releases are coupled with core

Signed-off-by: Julius Härtl <jus@bitgrid.net>

github-advanced-security bot found potential problems Jul 11, 2023

View reviewed changes

blizzz reviewed Jul 11, 2023

View reviewed changes

juliushaertl force-pushed the enh/favorite-search branch from d438f85 to 2ea465b Compare July 11, 2023 12:54

github-advanced-security bot found potential problems Jul 11, 2023

View reviewed changes

apps/dav/lib/Connector/Sabre/FilesReportPlugin.php Fixed Show fixed Hide fixed

apps/dav/lib/Connector/Sabre/FilesReportPlugin.php Fixed Show fixed Hide fixed

apps/dav/lib/Connector/Sabre/FilesReportPlugin.php Fixed Show fixed Hide fixed

juliushaertl force-pushed the enh/favorite-search branch 3 times, most recently from 674aa3c to f075ff3 Compare July 12, 2023 11:52

juliushaertl marked this pull request as ready for review July 12, 2023 14:26

juliushaertl requested review from blizzz, marcelklehr and icewind1991 July 12, 2023 14:26

juliushaertl added 3. to review Waiting for reviews performance 🚀 labels Jul 12, 2023

juliushaertl added this to the Nextcloud 28 milestone Jul 12, 2023

blizzz approved these changes Jul 12, 2023

View reviewed changes

marcelklehr reviewed Jul 13, 2023

View reviewed changes

skjnldsv mentioned this pull request Nov 1, 2023

chore: 28.0.0 beta 1 #41228

Merged

This was referenced Nov 6, 2023

28.0.0 beta 2 #41314

Merged

28.0.0 beta 3 #41379

Merged

blizzz mentioned this pull request Nov 14, 2023

28.0.0 beta 4 #41477

Merged

juliushaertl modified the milestones: Nextcloud 28, Nextcloud 29 Nov 16, 2023

juliushaertl added 2 commits February 27, 2024 14:26

feat: Use file cache query builder for searching for favorites

b230e54

Signed-off-by: Julius Härtl <jus@bitgrid.net>

feat: Pass limit/offset for searchByTag

dfd21de

Signed-off-by: Julius Härtl <jus@bitgrid.net>

AndyScherzinger force-pushed the enh/favorite-search branch from f075ff3 to dfd21de Compare February 27, 2024 13:26

This was referenced Mar 12, 2024

29.0.0 beta 2 #44152

Merged

29.0.0 beta 3 #44186

Merged

29.0.0 beta 4 #44275

Merged

Altahrim mentioned this pull request Mar 20, 2024

29.0.0 beta 5 #44361

Merged

Altahrim mentioned this pull request Mar 25, 2024

29.0.0 beta 6 #44449

Merged

skjnldsv mentioned this pull request Mar 28, 2024

29.0.0 RC1 #44552

Merged

81 tasks

skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024

skjnldsv added 2. developing Work in progress stale Ticket or PR with no recent activity and removed 3. to review Waiting for reviews labels Jul 27, 2024

This was referenced Jul 30, 2024

30.0.0 beta 2 #46870

Merged

30.0.0 beta 3 #46946

Merged

This was referenced Aug 5, 2024

30.0.0 beta 4 #47028

Merged

30.0.0 beta 5 #47100

Merged

skjnldsv mentioned this pull request Aug 13, 2024

30.0.0 RC1 #47190

Merged

juliushaertl modified the milestones: Nextcloud 30, Nextcloud 31 Aug 13, 2024

skjnldsv closed this Aug 14, 2024

skjnldsv removed this from the Nextcloud 31 milestone Aug 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use file cache query builder for searching for favorites #39303

feat: Use file cache query builder for searching for favorites #39303

juliushaertl commented Jul 11, 2023 •

edited

Loading

blizzz left a comment

blizzz Jul 11, 2023

juliushaertl Jul 12, 2023

blizzz Jul 12, 2023

juliushaertl commented Jul 12, 2023

blizzz Jul 12, 2023

marcelklehr Jul 13, 2023

marcelklehr Jul 13, 2023

marcelklehr Jul 13, 2023

juliushaertl Jul 13, 2023

icewind1991 Jul 17, 2023

icewind1991 Jul 17, 2023

marcelklehr Jul 17, 2023

icewind1991 Jul 17, 2023

feat: Use file cache query builder for searching for favorites #39303

feat: Use file cache query builder for searching for favorites #39303

Conversation

juliushaertl commented Jul 11, 2023 • edited Loading

TODO

Checklist

blizzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliushaertl commented Jul 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliushaertl commented Jul 11, 2023 •

edited

Loading