Fix/dav #2608

CarlSchwan · 2022-06-21T11:45:44Z

Resolves: [Bug]: PROPFIND on dav returns null instead of 404 server#32514
Target version: master and stable24

Summary

Clients expects this behavior:
If rich-workspaces is enabled and readme.md is empty or does not exist, it must return "".
If disabled -> null.
If readme.md has content -> show content

This revert to it and also make it possible in a separate commit to not have to add 'Depth: 1' in the curl request to get the workspace information

tobiasKaminsky · 2022-06-21T11:49:19Z

@max-nextcloud can we then also have a test, so this regression cannot happen again? :)

CarlSchwan · 2022-06-21T14:46:58Z

@vinicius73 why was master merged in this branch?

vinicius73 · 2022-06-21T15:36:33Z

@vinicius73 why was master merged in this branch?

This branch has master as target, we can't merge it properly without update it.
Last commits in master update some dependencies, it can affect tests.

CarlSchwan · 2022-06-21T17:20:34Z

we should do rebasing instead then instead of creating merge commits, always a bit cleaner

tobiasKaminsky · 2022-06-22T05:33:35Z

👍 "add folder info" works again.
(though i now cannot use text within Android app, but either this is a problem on my side or a different bug)

tobiasKaminsky · 2022-06-22T06:50:55Z

One problem I now see is:

have folder 1, inside subfolder 2
go into folder 1, retrieve content of it: subfolder 2 does not show rich workspaces
go into folder 2, see that it has content -> is updated and shown in Android
go back to folder 1, refresh -> subfolder 2 has no content thus it is deleted

--> if requested, the folder and its children should always have the rich workspace property properly set

max-nextcloud · 2022-06-22T06:53:11Z

The CI failures are indicating that names other than 'Readme.md' - i.e. 'README.md' or 'Anleitung.md' do not work anymore for the rich workspaces in the web ui.

This used to be server side logic in the /workspace text endpoint. We stopped using that endpoint and rely purely on PROPFIND results since NC 24 to speed up rendering of rich workspaces in the files view.

I'm not a big fan of supporting translated or all caps readme file names - but it's what we did in the past so breaking it does not seem like a good idea either.

max-nextcloud · 2022-06-22T07:03:05Z

also make it possible in a separate commit to not have to add 'Depth: 1' in the curl request to get the workspace information

@CarlSchwan Sounds like you've been using curl to test this... Could you share your curl commands so i can base the tests on it?

max-nextcloud · 2022-06-22T12:34:19Z

Added tests and fixed issues that the reverted commit had fixed.

The workspace property is still not set for nested folders (see #2608 (comment)). I think in order to bring that back we need to revert be562a4.

CarlSchwan · 2022-06-22T13:13:03Z

curl 'http://nextcloud.local/remote.php/dav/files/admin/' -v -X PROPFIND -u admin:admin --data '<?xml version="1.0"?>
<d:propfind  xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns" xmlns:nc="http://nextcloud.org/ns" xmlns:ocs="http://open-collaboration-services.org/ns">
  <d:prop>
    <d:getlastmodified />
    <d:getetag />
    <d:resourcetype />
    <oc:fileid />
    <nc:file-metadata-size /><oc:permissions />
    <oc:size />
    <d:getcontentlength />
    <d:quota-available-bytes />
    <nc:rich-workspace /> <nc:rich-workspace-file /> <nc:has-preview />
    <nc:mount-type />
    <nc:is-encrypted />
    <ocs:share-permissions />
    <oc:tags />
    <oc:favorite />
    <oc:owner-id />
    <oc:owner-display-name />
    <oc:share-types />
    <oc:comments-unread />
  </d:prop>
</d:propfind>' | xmllint --format -

max-nextcloud · 2022-06-22T13:31:52Z

@tobiasKaminsky I think this should fix both issues now. Also added regression tests

max-nextcloud · 2022-06-22T13:36:51Z

lib/DAV/WorkspacePlugin.php

-		if ($propFind->getDepth() === $this->server->getHTTPDepth()) {
+		$propFind->handle(self::WORKSPACE_PROPERTY, function () use ($node) {


I'm not entirely sure about removing this condition. #2608 (comment) sounds like workspace needs to be set for nested folders and we rely on it being set in our new tests for depth=0.

My understanding is that this will only be evaluated anyway if the property is requested. Would like to hear @juliushaertl opinion on this before a merge though.

tobiasKaminsky · 2022-06-23T06:28:01Z

@tobiasKaminsky I think this should fix both issues now. Also added regression tests

Looks good 👍👍👍

tobiasKaminsky · 2022-06-29T05:42:00Z

I found a strange bug?
On Android:

create subfolder 1
go into it
add folder content / rich workspaces

Use Insomnia/cURL to show rich-workspaces property of /1/ folder -> see nothing
Force refresh folder in Android -> still nothings
Go to web in /1/ folder, see rich workspaces -> then it is also in Insomnia / Android

szaimen · 2022-07-04T15:06:40Z

What is missing here?

max-nextcloud · 2022-07-04T20:29:06Z

This PR reverts a change that was introduces as a performance fix. We need to evaluate the impact of undoing that fix.

(See #2608 (review) for the code in question)

szaimen · 2022-07-04T20:32:25Z

This PR reverts a change that was introduces as a performance fix. We need to evaluate the impact of undoing that fix.

👍 I hope this will be possible before the first RC of 24.0.3...

max-nextcloud · 2022-07-05T05:33:14Z

So i profiled this some.

Profiling Command

for n in {1..10} ; do curl -w "%{time_total}\n" -o /dev/null -s 'http://nextcloud.local/remote.php/dav/files/admin/' -X PROPFIND -u admin:admin --data '<?xml version="1.0"?>
<d:propfind  xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns" xmlns:nc="http://nextcloud.org/ns" xmlns:ocs="http://open-collaboration-services.org/ns">
  <d:prop>
    <d:getlastmodified />
    <d:getetag />
    <d:resourcetype />
    <oc:fileid />
    <nc:file-metadata-size /><oc:permissions />
    <oc:size />
    <d:getcontentlength />
    <d:quota-available-bytes />
    <nc:rich-workspace /> <nc:rich-workspace-file /> <nc:has-preview />
    <nc:mount-type />
    <nc:is-encrypted />
    <ocs:share-permissions />
    <oc:tags />
    <oc:favorite />
    <oc:owner-id />
    <oc:owner-display-name />
    <oc:share-types />
    <oc:comments-unread />
  </d:prop>
</d:propfind>'; done | LC_ALL=C awk '{ total += $1; count++ } END { print total/count }'

22 Folders inside root folder of admin:

fix/dav: 0.542629
master: 0.414488

222 Folders inside root folder of admin:

fix/dav: 2.3157
master: 0.763323
fix/dav (Readme.md only): 1.5852

juliusknorr · 2022-07-05T08:48:34Z

@tobiasKaminsky Can you maybe clarify on how the android client is requesting the workspaces especially in regards to the Depth value? From #994 we had issues in the past with large folder structures that is why the response was limited to only return the workspace on the requested root node.

tobiasKaminsky · 2022-07-06T05:12:48Z

@juliushaertl initial bug fix works, what we discussed yesterday!
Also works on iOS.

But this one still happens:

I found a strange bug? On Android:
* create subfolder 1

* go into it

* add folder content / rich workspaces
Use Insomnia/cURL to show rich-workspaces property of /1/ folder -> see nothing Force refresh folder in Android -> still nothings Go to web in /1/ folder, see rich workspaces -> then it is also in Insomnia / Android

juliusknorr · 2022-07-06T07:59:46Z

But this one still happens:

@tobiasKaminsky I could reproduce but seems related to the files_lock app. Do you have that one enabled in your test instance? I'd consider that a separate bug then.

tobiasKaminsky · 2022-07-06T08:10:32Z

@tobiasKaminsky I could reproduce but seems related to the files_lock app. Do you have that one enabled in your test instance? I'd consider that a separate bug then.

I have, yes.
Disabling "fixes" it.
Shall I create a new bug in files_lock, or will you directly create a fix 🤡?

juliusknorr · 2022-07-06T08:50:38Z

Shall I create a new bug in files_lock, or will you directly create a fix 🤡?

Should be fixed on master already with nextcloud/files_lock#75 but I have a follow up patch in the pipeline that I worked on with @max-nextcloud now.

This reverts commit 3e40b7b. Signed-off-by: Carl Schwan <[email protected]>

Otherwise if the Depth header is ommited, the workspace information won't be display at all. Signed-off-by: Carl Schwan <[email protected]>

Regression tests for nextcloud/server#32514 (comment) Signed-off-by: Max <[email protected]>

These fixes were lost when reverting 3e40b7b. Signed-off-by: Max <[email protected]>

Signed-off-by: Max <[email protected]>

Signed-off-by: Julius Härtl <[email protected]>

max-nextcloud · 2022-07-06T09:06:28Z

I profiled this and it actually improves performance. With 222 folders i get:

now: 1.72988
before: 2.3157

This is still fairly slow and also applies when loading the files list in the browser. On the other hand this is the expected behavior by the client. We will either need to add some caching or make more specific requests on the client (one depth 0 PROPFIND with rich-workspace property and a depth 1 PROPFIND without.

I approved this and will merge as it fixes the bug in question.

max-nextcloud · 2022-07-06T09:07:27Z

/backport to stable24

CarlSchwan added 3. to review bug: regression labels Jun 21, 2022

CarlSchwan added this to the Nextcloud 25 milestone Jun 21, 2022

CarlSchwan requested review from juliusknorr and max-nextcloud June 21, 2022 11:45

CarlSchwan self-assigned this Jun 21, 2022

max-nextcloud force-pushed the fix/dav branch 3 times, most recently from 02da266 to 35bc87d Compare June 22, 2022 12:20

max-nextcloud force-pushed the fix/dav branch 2 times, most recently from 21f21f4 to 1bbef41 Compare June 22, 2022 12:57

max-nextcloud force-pushed the fix/dav branch 2 times, most recently from 2a369ea to c24534a Compare June 22, 2022 13:26

max-nextcloud reviewed Jun 22, 2022

View reviewed changes

juliusknorr force-pushed the fix/dav branch from 20b2493 to 7f40ef6 Compare July 5, 2022 09:20

szaimen mentioned this pull request Jul 5, 2022

24.0.3 RC1 nextcloud/server#33124

Merged

18 tasks

max-nextcloud self-requested a review July 6, 2022 08:49

max-nextcloud approved these changes Jul 6, 2022

View reviewed changes

CarlSchwan and others added 6 commits July 6, 2022 10:51

Revert "fix: DAV only return workspace properties if they are set"

82b1234

This reverts commit 3e40b7b. Signed-off-by: Carl Schwan <[email protected]>

Assume that the default depth is 1 in DAV

4f5a37d

Otherwise if the Depth header is ommited, the workspace information won't be display at all. Signed-off-by: Carl Schwan <[email protected]>

test: propfind extension for workspaces

63bbd4a

Regression tests for nextcloud/server#32514 (comment) Signed-off-by: Max <[email protected]>

fix: bring back the share owner handling and workspace file id

741d952

These fixes were lost when reverting 3e40b7b. Signed-off-by: Max <[email protected]>

fix: always handle workspace properties

bfe8a13

Signed-off-by: Max <[email protected]>

Avoid duplicate queries and limit depth by default to 0

940e1b5

Signed-off-by: Julius Härtl <[email protected]>

max-nextcloud force-pushed the fix/dav branch from 960e1a7 to 940e1b5 Compare July 6, 2022 08:52

backportbot-nextcloud bot added the backport-request label Jul 6, 2022

max-nextcloud merged commit d59b701 into master Jul 6, 2022

delete-merged-branch bot deleted the fix/dav branch July 6, 2022 09:07

backportbot-nextcloud bot mentioned this pull request Jul 6, 2022

[stable24] Fix/dav #2676

Merged

1 task

backportbot-nextcloud bot removed the backport-request label Jul 6, 2022

skjnldsv mentioned this pull request Aug 11, 2022

25.0.0 beta 1 nextcloud/server#33505

Merged

joshtrichards mentioned this pull request Jan 10, 2024

[Bug]: Trouble accessing data for certain users: "Exception thrown: OCP\Files\GenericFileException" when Rich Workspaces is enabled w/ external storage and ldap nextcloud/server#42663

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/dav #2608

Fix/dav #2608

CarlSchwan commented Jun 21, 2022

tobiasKaminsky commented Jun 21, 2022

CarlSchwan commented Jun 21, 2022

vinicius73 commented Jun 21, 2022

CarlSchwan commented Jun 21, 2022

tobiasKaminsky commented Jun 22, 2022

tobiasKaminsky commented Jun 22, 2022

max-nextcloud commented Jun 22, 2022 •

edited

Loading

max-nextcloud commented Jun 22, 2022

max-nextcloud commented Jun 22, 2022

CarlSchwan commented Jun 22, 2022

max-nextcloud commented Jun 22, 2022

max-nextcloud Jun 22, 2022

tobiasKaminsky commented Jun 23, 2022

tobiasKaminsky commented Jun 29, 2022

szaimen commented Jul 4, 2022

max-nextcloud commented Jul 4, 2022 •

edited

Loading

szaimen commented Jul 4, 2022 •

edited

Loading

max-nextcloud commented Jul 5, 2022 •

edited

Loading

juliusknorr commented Jul 5, 2022

tobiasKaminsky commented Jul 6, 2022

juliusknorr commented Jul 6, 2022

tobiasKaminsky commented Jul 6, 2022

juliusknorr commented Jul 6, 2022

max-nextcloud commented Jul 6, 2022

max-nextcloud commented Jul 6, 2022

		if ($propFind->getDepth() === $this->server->getHTTPDepth()) {
		$propFind->handle(self::WORKSPACE_PROPERTY, function () use ($node) {

Fix/dav #2608

Fix/dav #2608

Conversation

CarlSchwan commented Jun 21, 2022

Summary

tobiasKaminsky commented Jun 21, 2022

CarlSchwan commented Jun 21, 2022

vinicius73 commented Jun 21, 2022

CarlSchwan commented Jun 21, 2022

tobiasKaminsky commented Jun 22, 2022

tobiasKaminsky commented Jun 22, 2022

max-nextcloud commented Jun 22, 2022 • edited Loading

max-nextcloud commented Jun 22, 2022

max-nextcloud commented Jun 22, 2022

CarlSchwan commented Jun 22, 2022

max-nextcloud commented Jun 22, 2022

max-nextcloud Jun 22, 2022

Choose a reason for hiding this comment

tobiasKaminsky commented Jun 23, 2022

tobiasKaminsky commented Jun 29, 2022

szaimen commented Jul 4, 2022

max-nextcloud commented Jul 4, 2022 • edited Loading

szaimen commented Jul 4, 2022 • edited Loading

max-nextcloud commented Jul 5, 2022 • edited Loading

22 Folders inside root folder of admin:

222 Folders inside root folder of admin:

juliusknorr commented Jul 5, 2022

tobiasKaminsky commented Jul 6, 2022

juliusknorr commented Jul 6, 2022

tobiasKaminsky commented Jul 6, 2022

juliusknorr commented Jul 6, 2022

max-nextcloud commented Jul 6, 2022

max-nextcloud commented Jul 6, 2022

max-nextcloud commented Jun 22, 2022 •

edited

Loading

max-nextcloud commented Jul 4, 2022 •

edited

Loading

szaimen commented Jul 4, 2022 •

edited

Loading

max-nextcloud commented Jul 5, 2022 •

edited

Loading