SystemTags endpoint to return tags used by a user with meta data #37961

blizzz · 2023-04-27T20:34:27Z

Summary

Target case is photos app: when visiting the tags category, all systemtags of the whole cloud are retrieved. In subsequent steps the next tag is requested until the browser view is filled with tag tiles (i.e. previews are requested just as well).

With this approach, we incorporate the dav search and look for user related tags that are used by them, and already returns the statistics (number of files tagged with the respective tag) as well as a file id for the purpose to load the preview. This defaults to the file with the highest id.

Call:

curl -s -u 'user:password' \
  'https://my.nc.srv/remote.php/dav/systemtags-assigned' \
  -X PROPFIND -H 'Accept: text/plain' \
  -H 'Accept-Language: en-US,en;q=0.5'  -H 'Depth: 1' \
  -H 'Content-Type: text/plain;charset=UTF-8' \
  --data @/home/doe/request-systemtag-props.xml

With request-systemtag-props.xml:

<?xml version="1.0" encoding="UTF-8"?>
<d:propfind xmlns:d="DAV:">
        <d:prop xmlns:oc="http://owncloud.org/ns" xmlns:nc="http://nextcloud.org/ns">
                <oc:id/>
                <oc:display-name/>
                <oc:user-visible/>
                <oc:user-assignable/>
                <oc:can-assign/>
                <nc:files-assigned/>
                <nc:reference-fileid/>
        </d:prop>
</d:propfind>

Example output:

  …
  <d:response>
    <d:href>/master/remote.php/dav/systemtags/84</d:href>
    <d:propstat>
      <d:prop>
        <oc:id>84</oc:id>
        <oc:display-name>Computer</oc:display-name>
        <oc:user-visible>true</oc:user-visible>
        <oc:user-assignable>true</oc:user-assignable>
        <oc:can-assign>true</oc:can-assign>
        <nc:files-assigned>42</nc:files-assigned>
        <nc:reference-fileid>924022</nc:reference-fileid>
      </d:prop>
      <d:status>HTTP/1.1 200 OK</d:status>
    </d:propstat>
  </d:response>
  <d:response>
    <d:href>/remote.php/dav/systemtags/97</d:href>
    <d:propstat>
      <d:prop>
        <oc:id>97</oc:id>
        <oc:display-name>Bear</oc:display-name>
        <oc:user-visible>true</oc:user-visible>
        <oc:user-assignable>true</oc:user-assignable>
        <oc:can-assign>true</oc:can-assign>
        <nc:files-assigned>1</nc:files-assigned>
        <nc:reference-fileid>923422</nc:reference-fileid>
      </d:prop>
      <d:status>HTTP/1.1 200 OK</d:status>
    </d:propstat>
  </d:response>
  …

Update 23/05/04

Added was support to retrieve tags for a specific media type. This works with the URI. So, while the systemtags-current root provides tag information across all files, querying /remote.php/dav/systemtags-current/image will return tag information only for all image-typed files.

In the first commit, the image-type condition was hard coded.

/Update

Update 23/05/04

The endpoint was renamed from systemtags-current to systemtags-assigned. The information at the top (before the updates) are adjusted.

/Update

TODO

Performance testing with a serious data set
Reconsider pragmatic architectual choices (quick PoC)

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/SystemTag/SystemTagPlugin.php

apps/dav/lib/SystemTag/SystemTagsInUseCollection.php

lib/private/Files/Node/Folder.php

apps/dav/lib/SystemTag/SystemTagPlugin.php

blizzz · 2023-05-04T10:11:51Z

Added support for media types and updated the information in the summary.

Compared to the first commit, it's a little breaking, for filtering image files was hardcoded. To achive the same results run the query against /remote.php/dav/systemtags-current/image, otherwise tags relevant to any file type are returned.

marcelklehr · 2023-05-04T10:13:33Z

Can we call the endpoint something more meaningful than systemtags-current?

AndyScherzinger

fine by me except other peoples comments of course. And would also love to have this in for 27 even if we might optimize further in the future or have a complete new solution for certain things regarding tags.

Also, given the time I think Beta2 would be fine.

blizzz · 2023-05-04T10:31:08Z

Can we call the endpoint something more meaningful than systemtags-current?

We can, but do you have suggestions? Tbh, I am not super happy about it either, but was so far the best I could think of:

"-current" says it is about tags that are in use at this time

And i disregarded following ideas, because

"-used" could be appropriate, but it has a touch of the past, or could be misinterpreted as something trashed
"-recent" could suggest that valid, but older tags would not be considered
"-active" could be misleading (what is inactive?)
"-user" could be misleading to be user-only tags

blizzz · 2023-05-04T10:46:21Z

fine by me except other peoples comments of course. And would also love to have this in for 27 even if we might optimize further in the future or have a complete new solution for certain things regarding tags.

Also, given the time I think Beta2 would be fine.

Beta2 is fine, just one thing that is missing is also that we extend Folder with public methods, and that should be reflected in the PHP API. We should refrain from touching \OCP\Files\Folder as it has at least 8 implementations, which it would break. So we'll have to add a public interface, but when we do it, we should be sure about it.

Thus said I am not one hundred percent convinced our additions should stay in the Folder implementation, however there is some of the logic we need. We could have a SytemTag specific collection that could wrap it and so have little changes there, but would have a hard dependency on the Folder implementation (if only on few specific parts).

Or we expose what is consolidated here as getCachesAndMountpointsForSearch as PHP API, that can be then consumed by new SystemTag specific classes and have better separation.

Would love to have @icewind1991's opinion on it.

marcelklehr · 2023-05-04T10:51:06Z

We can, but do you have suggestions?

How about

systemtags-in-use
systemtags-assigned

blizzz · 2023-05-04T13:14:33Z

We can, but do you have suggestions?

How about
* `systemtags-in-use`

* `systemtags-assigned`

systemtags-assigned sounds good to me, and yes, clearer than current.

icewind1991

Overall approach seems fine.

Not a big fan of adding it as a public method to folder but not sure if there is a better option

apps/dav/lib/SystemTag/SystemTagsInUseCollection.php

lib/private/Files/Node/Folder.php

icewind1991 · 2023-05-04T14:51:29Z

lib/private/Files/Node/Folder.php

+		// Currently query has to have exactly one search condition. If no media type is provided,
+		// we fall back to the presence of a systemtag.
+		if (empty($mediaType)) {
+			$query = $this->queryFromOperator(new SearchComparison(ISearchComparison::COMPARE_LIKE, 'systemtag', '%'), null, $limit, $offset);


does adding this operator do anything? LIKE '%' seems to be always true to me

It's a shortcut to get a query to pass to $searchHelper->findUsedTagsInCaches(). Certainly this can be solved more elegantly. It was born out of the need to have something ready to bring Magenta back to a working state/environment.

I suspect that this might interfere with picking an index so it would be better to have another way to flag that the query should join on tags.

The behaviour appears same, however (checking with Postgrest) at some we'll have an Index Scan using oc_systemtag_pkey on oc_systemtag systemtag (cost=0.14..13.54 rows=69 width=20) where the poential cost is slighlty lower when the comparison is not included. 13.54 instead of 13.72. May not be representative.

Resolved with latest commit

blizzz · 2023-05-04T19:13:59Z

Not a big fan of adding it as a public method to folder but not sure if there is a better option

@icewind1991 yes feeling the same. One way to reduce the footprint would be to expose getCachesAndMountpointsForSearch only (new interface like ISearchAwareFolder or something like that), then the rest of the logic can be moved to classes withing the SystemTags namespace.

blizzz · 2023-05-05T19:27:33Z

Not a big fan of adding it as a public method to folder but not sure if there is a better option

@icewind1991 yes feeling the same. One way to reduce the footprint would be to expose getCachesAndMountpointsForSearch only (new interface like ISearchAwareFolder or something like that), then the rest of the logic can be moved to classes withing the SystemTags namespace.

But of course that leaves the changes within OC\Files\Cache\. I have repressed it somehow, and from the hip not a quick solution for it. I've got a commit ready to do the former that I can push, if it makes a big difference.

Or we indeed keep it provide as it is for now, just to have it merged. And have time to rethink internal structure and the PHP API, and be it for 28. I am a bit concerned though that the follow up would compete with other important tasks.

apps/dav/lib/SystemTag/SystemTagPlugin.php

Target case is photos app: when visiting the tags category, all systemtags of the whole cloud are retrieved. In subequent steps the next tag is requested until the browser view is filled with tag tiles (i.e. previews are requested just as well). With this approach, we incorpoate the dav search and look for user related tags that are used by them, and already returns the statistics (number of files tagged with the respective tag) as well as a file id for the purpose to load the preview. This defaults to the file with the highest id. Call: curl -s -u 'user:password' \ 'https://my.nc.srv/remote.php/dav/systemtags-current' \ -X PROPFIND -H 'Accept: text/plain' \ -H 'Accept-Language: en-US,en;q=0.5' -H 'Depth: 1' \ -H 'Content-Type: text/plain;charset=UTF-8' \ --data @/home/doe/request-systemtag-props.xml With request-systemtag-props.xml: <?xml version="1.0" encoding="UTF-8"?> <d:propfind xmlns:d="DAV:"> <d:prop xmlns:oc="http://owncloud.org/ns" xmlns:nc="http://nextcloud.org/ns"> <oc:id/> <oc:display-name/> <oc:user-visible/> <oc:user-assignable/> <oc:can-assign/> <nc:files-assigned/> <nc:reference-fileid/> </d:prop> </d:propfind> Example output: … <d:response> <d:href>/master/remote.php/dav/systemtags/84</d:href> <d:propstat> <d:prop> <oc:id>84</oc:id> <oc:display-name>Computer</oc:display-name> <oc:user-visible>true</oc:user-visible> <oc:user-assignable>true</oc:user-assignable> <oc:can-assign>true</oc:can-assign> <nc:files-assigned>42</nc:files-assigned> <nc:reference-fileid>924022</nc:reference-fileid> </d:prop> <d:status>HTTP/1.1 200 OK</d:status> </d:propstat> </d:response> <d:response> <d:href>/remote.php/dav/systemtags/97</d:href> <d:propstat> <d:prop> <oc:id>97</oc:id> <oc:display-name>Bear</oc:display-name> <oc:user-visible>true</oc:user-visible> <oc:user-assignable>true</oc:user-assignable> <oc:can-assign>true</oc:can-assign> <nc:files-assigned>1</nc:files-assigned> <nc:reference-fileid>923422</nc:reference-fileid> </d:prop> <d:status>HTTP/1.1 200 OK</d:status> </d:propstat> </d:response> … Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

- only the media part of the mime type can be search, but not the full mime type. It can be added, should it become necessary. - thus fixes previously hardcoded selector for image/ types - also fixes a return type hint - adds a return type hint Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

- DI SystemTagManager - add some comments and doc - catch NoUserException - add return type hints Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

- adds OC\SystemTag\SystemTagsInFilesDetector where the search logic is moved to Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

blizzz · 2023-05-09T22:01:21Z

@icewind1991 moved it to the search helper as suggested

- search constraints are now fully in control of SystemTagsInFilesDetector::detectAssignedSystemTagsIn(), avoids duplication of a WHERE statement Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

blizzz · 2023-05-11T08:13:26Z

failing drone CI tests unrelated

blizzz · 2023-05-11T08:15:28Z

Will need manual backports to 26 and 25

… although 26 might work, trying.

blizzz · 2023-05-11T08:17:17Z

/backport to stable26

AndyScherzinger · 2023-05-11T08:19:07Z

/backport to stable25

backportbot-nextcloud · 2023-05-11T10:08:10Z

The backport to stable26 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable26
git pull origin/stable26

# Create the new backport branch
git checkout -b fix/foo-stable26

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable26

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

backportbot-nextcloud · 2023-05-11T10:08:15Z

The backport to stable25 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable25
git pull origin/stable25

# Create the new backport branch
git checkout -b fix/foo-stable25

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable25

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

blizzz · 2023-05-16T11:17:39Z

backports created manually

blizzz added bug 2. developing Work in progress feature: dav feature: tags labels Apr 27, 2023

blizzz added this to the Nextcloud 27 milestone Apr 27, 2023

blizzz requested review from nickvergessen, marcelklehr, icewind1991 and AndyScherzinger April 27, 2023 20:34

github-advanced-security bot found potential problems Apr 27, 2023

View reviewed changes

blizzz force-pushed the poc/noid/systemtags-perf branch 2 times, most recently from 543a891 to c174172 Compare April 28, 2023 08:19

This was referenced Apr 28, 2023

Implement systemtags through webdav #37609

Merged

use more efficient tag retrieval on DAV report request #37969

Merged

[stable25] SystemTags optimizations #38015

Closed

Adapt to SystemTags optimizations on server nextcloud/photos#1770

Merged

skjnldsv mentioned this pull request May 3, 2023

27.0.0 beta 1 #38038

Merged

AndyScherzinger approved these changes May 4, 2023

View reviewed changes

marcelklehr approved these changes May 4, 2023

View reviewed changes

icewind1991 requested changes May 4, 2023

View reviewed changes

This was referenced May 9, 2023

27.0.0 beta 2 #38137

Closed

27.0.0 beta 2 #38139

Merged

blizzz force-pushed the poc/noid/systemtags-perf branch from e37f853 to ab3bba2 Compare May 9, 2023 19:00

github-advanced-security bot found potential problems May 9, 2023

View reviewed changes

apps/dav/lib/SystemTag/SystemTagPlugin.php Fixed Show fixed Hide fixed

blizzz added 5 commits May 9, 2023 23:51

fix: improve naming of new systemtags endpoint to systemtags-assigned

b07c5df

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

chore: polish SystemTagsInUseCollection

7f3725c

- DI SystemTagManager - add some comments and doc - catch NoUserException - add return type hints Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

refactor: remove SystemTag logic from Folder into QuerySearchHelper

dbfd2f9

- adds OC\SystemTag\SystemTagsInFilesDetector where the search logic is moved to Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

blizzz force-pushed the poc/noid/systemtags-perf branch from ab3bba2 to dbfd2f9 Compare May 9, 2023 21:59

blizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 9, 2023

blizzz requested a review from icewind1991 May 9, 2023 22:00

blizzz changed the title ~~PoC: SystemTags endpoint to return tags used by a user with meta data~~ SystemTags endpoint to return tags used by a user with meta data May 9, 2023

skjnldsv mentioned this pull request May 10, 2023

feat(changelog): group PR list by author in markdown nextcloud/github_helper#79

Merged

15 tasks

refactor: remove where specification from SELECT getter

df662f5

- search constraints are now fully in control of SystemTagsInFilesDetector::detectAssignedSystemTagsIn(), avoids duplication of a WHERE statement Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

blizzz force-pushed the poc/noid/systemtags-perf branch from 1891d1e to df662f5 Compare May 10, 2023 16:48

blizzz added the backport-request label May 11, 2023

blizzz merged commit b6c034a into master May 11, 2023

blizzz deleted the poc/noid/systemtags-perf branch May 11, 2023 08:16

This was referenced May 16, 2023

[stable26] SystemTags endpoint to return tags used by a user with meta data #38307

Merged

[stable25] SystemTags endpoint to return tags used by a user with meta data #38309

Merged

blizzz removed the backport-request label May 16, 2023

juliusknorr mentioned this pull request Jun 29, 2023

/tags view performance nextcloud/photos#1728

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SystemTags endpoint to return tags used by a user with meta data #37961

SystemTags endpoint to return tags used by a user with meta data #37961

blizzz commented Apr 27, 2023 •

edited

Loading

blizzz commented May 4, 2023

marcelklehr commented May 4, 2023

AndyScherzinger left a comment

blizzz commented May 4, 2023

blizzz commented May 4, 2023

marcelklehr commented May 4, 2023

blizzz commented May 4, 2023

icewind1991 left a comment

icewind1991 May 4, 2023

blizzz May 4, 2023

icewind1991 May 9, 2023 •

edited

Loading

blizzz May 10, 2023 •

edited

Loading

blizzz May 10, 2023

blizzz commented May 4, 2023

blizzz commented May 5, 2023

blizzz commented May 9, 2023

blizzz commented May 11, 2023

blizzz commented May 11, 2023 •

edited

Loading

blizzz commented May 11, 2023

AndyScherzinger commented May 11, 2023

backportbot-nextcloud bot commented May 11, 2023

backportbot-nextcloud bot commented May 11, 2023

blizzz commented May 16, 2023

SystemTags endpoint to return tags used by a user with meta data #37961

SystemTags endpoint to return tags used by a user with meta data #37961

Conversation

blizzz commented Apr 27, 2023 • edited Loading

Summary

TODO

Checklist

blizzz commented May 4, 2023

marcelklehr commented May 4, 2023

AndyScherzinger left a comment

Choose a reason for hiding this comment

blizzz commented May 4, 2023

blizzz commented May 4, 2023

marcelklehr commented May 4, 2023

blizzz commented May 4, 2023

icewind1991 left a comment

Choose a reason for hiding this comment

icewind1991 May 4, 2023

Choose a reason for hiding this comment

blizzz May 4, 2023

Choose a reason for hiding this comment

icewind1991 May 9, 2023 • edited Loading

Choose a reason for hiding this comment

blizzz May 10, 2023 • edited Loading

Choose a reason for hiding this comment

blizzz May 10, 2023

Choose a reason for hiding this comment

blizzz commented May 4, 2023

blizzz commented May 5, 2023

blizzz commented May 9, 2023

blizzz commented May 11, 2023

blizzz commented May 11, 2023 • edited Loading

blizzz commented May 11, 2023

AndyScherzinger commented May 11, 2023

backportbot-nextcloud bot commented May 11, 2023

backportbot-nextcloud bot commented May 11, 2023

blizzz commented May 16, 2023

blizzz commented Apr 27, 2023 •

edited

Loading

icewind1991 May 9, 2023 •

edited

Loading

blizzz May 10, 2023 •

edited

Loading

blizzz commented May 11, 2023 •

edited

Loading