Improve delay in accessing WebDAV folders on external storage

[luka-nextcloud]

• Resolves: #

Summary

Issue: loading time while open an external directory increases with the number of children (files, folders).
Root cause: profind function has been called for each child to fetch metadata/permissions and it seem unneccessary.
Update:

• Caching the profind response of children while calling function opendir. Then, we don't need to call profind function for each child anymore.
• Optimize propfind caching to prevent duplicate call

Before: calling of profind function for each child increases loading time; propfind duplicate call for parent dir

After: calling profind function reduced since profind response has been cached while calling opendir; no duplicate propfind call

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)

[juliusknorr]

Maybe we can use a separate cache like $this->propfindCache for that to not mix up with existing behavior of the statCache.

[luka-nextcloud]

@juliushaertl I don't think we really need a separate cache since function profind also uses stateCache to cache the profind response.

[juliusknorr]

Yes though it represents just the existance of a file as a boolean and I think it would be wise not to mix different value types here and rather have a clean separation.

It seems that the usage of the cache actualy got lost with your recent changes

[luka-nextcloud]

@juliushaertl Please check again, thanks.

[juliusknorr]

Since you're at it, maybe you can also look into saving the remaining duplicate request that is still left:

[luka-nextcloud]

@juliushaertl I've updated PR & description, please check.

[juliusknorr]

Can you double check that? When I was testing it the propfind was still duplicate as the second one didn't read the cached response from anywhere.

I think it would make sense to reuse the internal profind method in https://github.com/nextcloud/server/pull/38418/files#diff-53ee4c908f9ae3a27e45d03c57ac5ed83c5916d7260561a03f279880671f6155R240 instead of directly calling the $this->client->propFind

[luka-nextcloud]

Can you double check that? When I was testing it the propfind was still duplicate as the second one didn't read the cached response from anywhere.

You are right, I updated the screenshot again.

I think it would make sense to reuse the internal profind method in https://github.com/nextcloud/server/pull/38418/files#diff-53ee4c908f9ae3a27e45d03c57ac5ed83c5916d7260561a03f279880671f6155R240 instead of directly calling the $this->client->propFind

As I know, we have to use $this->client->propFind() since the target item (file, folder) is an external dir. The internal propFind is only for internal item (file,folder)

[juliusknorr]

Tested and works

[juliusknorr]

I've pushed a small fixup commit to address an encoding issue with files with spaces, but otherwise this seemed good.

Note that I'll revoke my initial question for reusing the first propfind as i think this one is intentional only done on the root without children (depth=0) as it would also get requested when listing the parent folder of the external storage mount. In this case it might be wise to only request relevant information.

In case we want to optimize that this could easily be done through the following patch

diff --git a/lib/private/Files/Storage/DAV.php b/lib/private/Files/Storage/DAV.php
index 267f32a0e5b..cf031ac8c00 100644
--- a/lib/private/Files/Storage/DAV.php
+++ b/lib/private/Files/Storage/DAV.php
@@ -314,9 +314,11 @@ class DAV extends Common {
                                                '{DAV:}getetag',
                                                '{DAV:}quota-available-bytes',
                                        ]
-                               );
+                               , 1);
                                $this->statCache->set($path, $response);
-                               $this->propfindCache->set($path, $response);
+                               foreach ($response as $file => $responseData) {
+                                       $this->propfindCache->set($file, $responseData);
+                               }
                        } catch (ClientHttpException $e) {
                                if ($e->getHttpStatus() === 404 || $e->getHttpStatus() === 405) {
                                        $this->statCache->clear($path . '/');

[icewind1991]

I'm a bit hesitant (but not fully against) adding extra caching layers.

The issue shown in the OP should also be solvable by implementing a more optimized version of IStorage::getDirectoryContent for Dav.

The default implementation that is currently inherited is doing the "stat every file in the folder" that seems to be causing the issue. Changing it to "load the folder once and the metadata for each item" should give the same speedup in that case.

[come-nc]

I'm a bit hesitant (but not fully against) adding extra caching layers.

The issue shown in the OP should also be solvable by implementing a more optimized version of IStorage::getDirectoryContent for Dav.

The default implementation that is currently inherited is doing the "stat every file in the folder" that seems to be causing the issue. Changing it to "load the folder once and the metadata for each item" should give the same speedup in that case.

But the caching may help with other operations as well no?

[juliusknorr]

I think the caching here makes sense for all cases here also if it is not just about iterating over the directory content. It especially helps as the previous code called a propfind on any call that would obtain file metadata like the getPermission which is likely to be called in a few other places, so I think caching in an ArrayCache for the scope of the current request is still the most sane approach here.

[Alexandero89]

Don´t know if its because of your commits, but i was testing your PR and checked the logs.
I get some strange absolute urls there.
Example log:
{"reqId":"jXHhrkauwfkRmocMZ0C5","level":0,"time":"2023-06-19T11:03:05+00:00","remoteAddr":"","user":"--","app":"dav","method":"","url":"--","message":"sending dav PROPFIND request to external storage: http://localhosthttps://www.mycloud.com/remote.php/webdav/Backups/Android/Download/DCIM/OpenCamera/IMG_20210313_100632.jpg","userAgent":"--","version":"26.0.2.1","data":{"app":"dav"}}

Should be: "https://www.mycloud.com/remote.php/webdav/Backups/Android/Download/DCIM/OpenCamera/IMG_20210313_100632.jpg"

Is: "http://localhosthttps://www.mycloud.com/remote.php/webdav/Backups/Android/Download/DCIM/OpenCamera/IMG_20210313_100632.jpg"

So "$request->getAbsoluteUrl()" is returning something strange.

[icewind1991]

There is also a different propfind cache already which this is duplicating

[icewind1991]

#38945 implements getDirectoryContent and ensures that the existing statcache is better used

[juliusknorr]

Let's close this then as #38945 was merged