DAV endpoint for system tags #20786

PVince81 · 2015-11-27T12:04:02Z

REQUIRES: Implement systemtag managers and mapper #20650
~~TODO: add endpoint to retrieve all files/folders tagged by a specific tag?~~
TODO: PUT doesn't work, needs discussion
~~TODO: allow MOVE for rename in "by-name", only if target name doesn't exist~~

BELOW is obsolete, jump to #20786 (comment)

@DeepDiver1975 @nickvergessen

Currently the URLs look like this:
- http://root:admin@localhost/owncloud/remote.php/dav/systemtags/$userId/by-id/$tagId
- http://root:admin@localhost/owncloud/remote.php/dav/systemtags/$userId/by-name/$tagName_$userVisible_$userAssignable
by-id is used to list tags by id for direct access when the id is known
by-id cannot be used to PUT because we cannot know the id beforehand, so read-only
by-name is used mostly to create new tags using a known name compound. For example "tag1" with user visible true and user assignable false will have the name "tag1_1_0". This is needed to have unique names and also because PUT expects us to know the URL beforehand (no generated ids here)
DELETE works in both endpoints

Questions:

are these endpoints acceptable ?
regarding creating new tags:
- body is empty as it's not a file
- Sabre reports the exception "PUT only allowed for files"
- it is not clear how to pass additional properties to a PUT call, maybe headers (although, not relevant for this task as we don't have any additional properties)
- should we use POST instead of PUT ? There is a RFC proposal here for POST http://greenbytes.de/tech/webdav/rfc5995.html

@evert mind helping with the PUT questions ? Are there any examples how to use non-files as sabre resources ?

mention-bot · 2015-11-27T12:04:04Z

By analyzing the blame information on this pull request, we identified @LukasReschke, @icewind1991 and @DeepDiver1975 to be potential reviewers

PVince81 · 2015-11-27T12:05:27Z

Note: cadaver doesn't work on these endpoints, maybe because there is no mtime/size/etc

PVince81 · 2015-11-27T12:09:05Z

Additional info: tag id is auto-generated and the tag info is a tuple of "tag name", "user visible" and "user assignable". So it's possible to have several tags with the same name but different flags. This is why I used the compound names.

PVince81 · 2015-11-27T12:10:42Z

To test, create a file "propfind_tags.txt":

<?xml version="1.0"?>
<a:propfind xmlns:a="DAV:" xmlns:oc="http://owncloud.org/ns">
        <a:prop><oc:id/></a:prop>
        <a:prop><oc:display-name/></a:prop>
        <a:prop><oc:user-visible/></a:prop>
        <a:prop><oc:user-assignable/></a:prop>
</a:propfind>

Then with curl:

curl -X PROPFIND -H "Content-Type: text/xml" --data-binary "@propfind_tags.txt" http://root:admin@localhost/owncloud/remote.php/dav/systemtags/root/by-id/ | xmllint --format -

Might need to manually create some tags in the DB, for example:

insert into oc_systemtag values (NULL, 'readonly', 1, 0);

DeepDiver1975 · 2015-11-27T12:22:15Z

apps/dav/lib/systemtag/systemtagnode.php

+			$this->tagManager->deleteTags($this->tag->getId());
+		} catch (TagNotFoundException $e) {
+			// can happen if concurrent deletion occurred
+			throw new NotFound('Tag with id ' . $this->tag->getId() . ' not found', 0, $e);


delete operation is idempotent - deleting a not existing element has the same effect as deleting an existing element: it is gone after the request.

so - no excpetion shall be thrown

This is a misunderstanding of what idempotent means.

http://evertpot.com/idempotence-in-http/

DeepDiver1975 · 2015-11-27T12:24:52Z

I'll look into the creating topic - PUT has to work - caldav and carddav also use put - principals as well

PVince81 · 2015-11-27T12:53:08Z

PUT is fixable I think. It was more of a semantic question.
If you say that PUT can be used with address books and co, then it is the correct way even though they would all have an empty body.

DeepDiver1975 · 2015-11-27T13:02:45Z

PUT for me works with cadaver if I create a local file names test_1_1 🙈

Regarding the properties on creation: we can submit them in the request body - as in file content

DeepDiver1975 · 2015-11-27T13:07:36Z

so - system tags are not related to the user - as a result having the principal in the url doesn't make sense

nickvergessen · 2015-11-27T13:19:00Z

apps/dav/lib/systemtag/systemtagnodebyname.php

+	 */
+	public function getName() {
+		return $this->tag->getName() . '_' .
+			($this->tag->isUserVisible() ? '1' : '0') . '_' .


visibility makes no sense, if it's not visible it should not be exposed via webdav

admins should see them in order to allow modifications

DeepDiver1975 · 2015-11-27T13:20:52Z

furthermore: since tags hold not content by itself - we could model them as collections as well ...

PVince81 · 2015-11-27T13:33:08Z

system tags are not related to the user

Yes that's what I thought. However at some point the admins must be able to see the ones that are not user visible. Is it acceptable to have the endpoints deliver different results based on who is logged in then ?

DeepDiver1975 · 2015-11-27T13:47:07Z

Yes that's what I thought. However at some point the admins must be able to see the ones that are not user visible. Is it acceptable to have the endpoints deliver different results based on who is logged in then ?

URL are unique identifiers to the resource since tags are not user related the user has no value in the url. With respect to accessibility: an user invisible tag does exist - but is simply not accessible by a user.

What might be an approach to think about: encode the visibility in the url and the assignability as well

/dav/system-tags/visible/assignable/test
/dav/system-tags/invisible/assignable/test
/dav/system-tags/visible/not-assignable/test
/dav/system-tags/invisible/not-assignable/test

PVince81 · 2015-11-27T14:40:50Z

Or to have less endpoints, have an "admin" endpoint and "user" endpoint:

/dav/system-tags/admin/by-id/$tagId
/dav/system-tags/admin/by-name/$tagName_$assignable
/dav/system-tags/user/by-id/$tagId
/dav/system-tags/user/by-name/$tagName_$assignable

Admin tags would be the ones that are not user visible.
But then it splits the existing structure in a weird way.

DeepDiver1975 · 2015-11-27T15:07:35Z

Or to have less endpoints, have an "admin" endpoint and "user" endpoint:

this would generate two differen URLs to the same resource - not nice

evert · 2015-11-27T22:16:19Z

Hey guys. Very cool commit! While I don't know exactly what system tags are, it's really awesome to see people using advance sabre stuff ;)

The POST add-member stuff is something I'm definitely interested in having in sabredav. If you guys want this, I would be happy to include it in a minor 3.0.x release as I don't think this would introduce any sort of BC issues.

What's the issue with PUT exactly? You can't PUT on a collection, unless that collection also implements the IFile interface. But is that really what you want? PUT must either create or modify the actual resource you're targetting in the url.

evert · 2015-11-27T22:17:35Z

To be a bit more precise: What does your PUT request contain in its request body, and what should its effect be?

PVince81 · 2015-11-30T09:52:05Z

@evert a system-wide (aka global) tag is the tuple "tag name", "user visible", "user assignable".
In the database every tag also has an autoincrement id.

What is not clear is how to design the endpoints to allow for creation of tags using PUT.
My initial idea was to have an endpoint "by-name" (actually by-tuple) where one can do a PUT /tags/by-name/${tagName}_${userVisible}_${userAssignable}. The PUT would have an empty body because there is no additional information. The tag "name" is actually a compound name using the components of the tuple.

In other places in the code we need to work using the autoincrement $tagId, so I thought about having an additional read-only endpoint "by-id" to access tags by id. But as @DeepDiver1975 pointed out, a resource URL should actually be unique.

What do you think ? Does that make sense ?

PVince81 · 2015-11-30T15:20:49Z

Also, how to do pagination / server-side filtering ?

PVince81 · 2015-11-30T15:39:42Z

How to filter by file id ? Maybe an endpoint "/dav/system-tags/admin/files/$fileid/" and the collection returns a list of tags assigned to this file ?
The other alternative would be "SEARCH" on "/dav/files/" instead with a "oc:systemtag" property, but that is unexplored territory...

DeepDiver1975 · 2015-11-30T15:45:42Z

In other places in the code we need to work using the autoincrement $tagId, so I thought about having an additional read-only endpoint "by-id" to access tags by id. But as @DeepDiver1975 pointed out, a resource URL should actually be unique.

going with to different endpoint "by-id" and "by-name" is fine for my understanding - my comment about uniqueness was related to the user component in the url.

Also, how to do pagination / server-side filtering ?

We can define REPORTs for that - a well used mechanism in the area of dav.

PVince81 · 2015-11-30T16:49:43Z

Another problem I see is with assigning tags to files and concurrency:

A simple way to assign tags "one", "two" to a file "file1.txt" is PROPPATCH the file with <oc:systemtags><oc:systemtag>one</oc:systemtag><oc:systemtag>two</oc:systemtag></oc:systemtags>. This is an absolute operation and replaces ALL tags for the file. If the user has two browser tabs open with the tags "existingtag" and adds a tag "newtag" in one tab, then the second tab "newtag2". Due to the absolute nature of this operation, the second tab will save the set as "existingtag", "newtags2" and the user will lose the value "newtag" added in the first tab.

Another solution would be to have an endpoint that represents the tag list as a collection for a file: "/dav/system-tags/files/$fileid/$tagId". Then assigning/unassigning tags is a matter of PUT/DELETE tag ids into the collection. Or MOVE/COPY ? We're not creating new tags, just using existing ones.

Or is there a standard way to make PROPPATCH add sub-properties instead of replacing the whole set ?

DeepDiver1975 · 2015-11-30T17:06:14Z

Due to the absolute nature of this operation, the second tab will save the set as "existingtag", "newtags2" and the user will lose the value "newtag" added in the first tab.

This will be protected by the If-Match header and the etag ... valid question: do we update the etag of a file as soon as tags got added/removed? This will trigger sync on the clients.

Not what we want to have .....

PVince81 · 2015-11-30T17:09:09Z

@DeepDiver1975 regarding etag + metadata, related tickets: #16589 #20474

PVince81 · 2015-12-04T09:58:38Z

Please review @nickvergessen @blizzz @DeepDiver1975 @icewind1991

blizzz · 2015-12-04T11:06:51Z

apps/dav/lib/systemtag/systemtagnode.php

+use OCP\SystemTag\TagNotFoundException;
+use OCP\SystemTag\TagAlreadyExistsException;
+
+class SystemTagNode implements \Sabre\DAV\INode {


class description?

PVince81 · 2015-12-04T16:31:51Z

@blizzz adressed all the comments and added the precious "s" (which broke all the tests, took a while to fix them 😉)

blizzz · 2015-12-04T19:48:16Z

@blizzz adressed all the comments and added the precious "s" (which broke all the tests, took a while to fix them 😉)

vim should have a plugin that shows you all usages of a method and/or to savely rename it. Or, use ack(-grep) for finding those calls :)

blizzz · 2015-12-04T19:57:57Z

OK, i am happy 👍

PVince81 · 2015-12-07T08:45:45Z

vim should have a plugin that shows you all usages of a method and/or to savely rename it. Or, use ack(-grep) for finding those calls :)

I did, but some of the mocks had the method name hard-coded in a string. So a regular grep helped here 😉

icewind1991 · 2015-12-08T12:48:42Z

Did basic testing, code looks good 👍

DAV endpoint for system tags

PVince81 added the 2 - Developing label Nov 27, 2015

PVince81 added this to the 9.0-current milestone Nov 27, 2015

DeepDiver1975 added the in progress label Nov 27, 2015

PVince81 added the feature:tags label Nov 27, 2015

PVince81 mentioned this pull request Nov 27, 2015

System-wide tagging backend #20258

Closed

6 tasks

DeepDiver1975 reviewed Nov 27, 2015
View reviewed changes

DeepDiver1975 force-pushed the systemtags-dav branch from b7244b2 to 4d1cd6a Compare November 27, 2015 12:37

nickvergessen reviewed Nov 27, 2015
View reviewed changes

PVince81 added 3 - To Review and removed 2 - Developing labels Dec 3, 2015

PVince81 changed the title ~~[WIP] DAV endpoint for system tags~~ DAV endpoint for system tags Dec 3, 2015

blizzz reviewed Dec 4, 2015
View reviewed changes

Fixed system tags DAV and API and docs

316b907

icewind1991 added 4 - To release and removed 3 - To Review labels Dec 8, 2015

DeepDiver1975 added a commit that referenced this pull request Dec 8, 2015

Merge pull request #20786 from owncloud/systemtags-dav

85409b6

DAV endpoint for system tags

DeepDiver1975 merged commit 85409b6 into master Dec 8, 2015

DeepDiver1975 deleted the systemtags-dav branch December 8, 2015 12:51

PVince81 mentioned this pull request Dec 8, 2015

[research] DAV tags experiment #12867

Closed

PVince81 mentioned this pull request Feb 2, 2016

Devel docs for Webdav endpoints for system tags owncloud-archive/documentation#2120

Closed

PVince81 mentioned this pull request May 15, 2017

Document 'tagging' API owncloud-archive/documentation#2984

Closed

voroyam mentioned this pull request Nov 19, 2018

WebDAV Issue Collection owncloud/docs#268

Closed

4 tasks

lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAV endpoint for system tags #20786

DAV endpoint for system tags #20786

PVince81 commented Nov 27, 2015

mention-bot commented Nov 27, 2015

PVince81 commented Nov 27, 2015

PVince81 commented Nov 27, 2015

PVince81 commented Nov 27, 2015

DeepDiver1975 Nov 27, 2015

evert Nov 27, 2015

DeepDiver1975 commented Nov 27, 2015

PVince81 commented Nov 27, 2015

DeepDiver1975 commented Nov 27, 2015

DeepDiver1975 commented Nov 27, 2015

nickvergessen Nov 27, 2015

DeepDiver1975 Nov 27, 2015

nickvergessen Nov 27, 2015

DeepDiver1975 commented Nov 27, 2015

PVince81 commented Nov 27, 2015

DeepDiver1975 commented Nov 27, 2015

PVince81 commented Nov 27, 2015

DeepDiver1975 commented Nov 27, 2015

evert commented Nov 27, 2015

evert commented Nov 27, 2015

PVince81 commented Nov 30, 2015

PVince81 commented Nov 30, 2015

PVince81 commented Nov 30, 2015

DeepDiver1975 commented Nov 30, 2015

PVince81 commented Nov 30, 2015

DeepDiver1975 commented Nov 30, 2015

PVince81 commented Nov 30, 2015

PVince81 commented Dec 4, 2015

blizzz Dec 4, 2015

PVince81 commented Dec 4, 2015

blizzz commented Dec 4, 2015

blizzz commented Dec 4, 2015

PVince81 commented Dec 7, 2015

icewind1991 commented Dec 8, 2015

DAV endpoint for system tags #20786

DAV endpoint for system tags #20786

Conversation

PVince81 commented Nov 27, 2015

mention-bot commented Nov 27, 2015

PVince81 commented Nov 27, 2015

PVince81 commented Nov 27, 2015

PVince81 commented Nov 27, 2015

DeepDiver1975 Nov 27, 2015

Choose a reason for hiding this comment

evert Nov 27, 2015

Choose a reason for hiding this comment

DeepDiver1975 commented Nov 27, 2015

PVince81 commented Nov 27, 2015

DeepDiver1975 commented Nov 27, 2015

DeepDiver1975 commented Nov 27, 2015

nickvergessen Nov 27, 2015

Choose a reason for hiding this comment

DeepDiver1975 Nov 27, 2015

Choose a reason for hiding this comment

nickvergessen Nov 27, 2015

Choose a reason for hiding this comment

DeepDiver1975 commented Nov 27, 2015

PVince81 commented Nov 27, 2015

DeepDiver1975 commented Nov 27, 2015

PVince81 commented Nov 27, 2015

DeepDiver1975 commented Nov 27, 2015

evert commented Nov 27, 2015

evert commented Nov 27, 2015

PVince81 commented Nov 30, 2015

PVince81 commented Nov 30, 2015

PVince81 commented Nov 30, 2015

DeepDiver1975 commented Nov 30, 2015

PVince81 commented Nov 30, 2015

DeepDiver1975 commented Nov 30, 2015

PVince81 commented Nov 30, 2015

PVince81 commented Dec 4, 2015

blizzz Dec 4, 2015

Choose a reason for hiding this comment

PVince81 commented Dec 4, 2015

blizzz commented Dec 4, 2015

blizzz commented Dec 4, 2015

PVince81 commented Dec 7, 2015

icewind1991 commented Dec 8, 2015