Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add method to storage backends to get directory content with metadata #20232

Merged
merged 1 commit into from
Apr 20, 2020

Conversation

icewind1991
Copy link
Member

Currently you need to use opendir and then call getMetadata for
every file, which adds overhead because most storage backends already
get the metadata when doing the opendir.

While storagebackends can (and do) use caching to relief this problem,
this adds cache invalidation dificulties and only a limited number of
items are generally cached (to prevent memory usage exploding when
scanning large storages)

With this new methods storage backends can use the child metadata they
got from listing the folder to return metadata without having to keep
seperate caches.

Signed-off-by: Robin Appelman robin@icewind.nl

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Mar 30, 2020
@icewind1991 icewind1991 added this to the Nextcloud 19 milestone Mar 30, 2020
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't say if the change semantics are sound, but the code looks fine

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘

@rullzer
Copy link
Member

rullzer commented Apr 3, 2020

Conflicts... please rebase

This was referenced Apr 4, 2020
@icewind1991 icewind1991 force-pushed the storage-getdirectorycontent branch 2 times, most recently from 0256b7b to 3c97369 Compare April 10, 2020 14:54
@skjnldsv
Copy link
Member

PHP Fatal error: Declaration of OCA\Files_Sharing\Scanner::scanFile($file, $reuseExisting = 0, $parentId = -1, $cacheData = NULL, $lock = true) must be compatible with OC\Files\Cache\Scanner::scanFile($file, $reuseExisting = 0, $parentId = -1, $cacheData = NULL, $lock = true, $data = NULL) in /drone/src/apps/files_sharing/lib/Scanner.php on line 0

Lots of failures :)

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish 2. developing Work in progress and removed 3. to review Waiting for reviews 4. to release Ready to be released and/or waiting for tests to finish labels Apr 11, 2020
@icewind1991 icewind1991 force-pushed the storage-getdirectorycontent branch 3 times, most recently from 827b3fe to f497ff6 Compare April 15, 2020 16:53
This was referenced Apr 15, 2020
@icewind1991 icewind1991 force-pushed the storage-getdirectorycontent branch 5 times, most recently from 7ea5676 to f31daf8 Compare April 20, 2020 12:51
Currently you need to use `opendir` and then call `getMetadata` for
every file, which adds overhead because most storage backends already
get the metadata when doing the `opendir`.

While storagebackends can (and do) use caching to relief this problem,
this adds cache invalidation dificulties and only a limited number of
items are generally cached (to prevent memory usage exploding when
scanning large storages)

With this new methods storage backends can use the child metadata they
got from listing the folder to return metadata without having to keep
seperate caches.

Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991 icewind1991 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 20, 2020
@icewind1991
Copy link
Member Author

ci seems happy now

@blizzz blizzz merged commit 7b49368 into master Apr 20, 2020
@blizzz blizzz deleted the storage-getdirectorycontent branch April 20, 2020 22:23
@nickvergessen
Copy link
Member

Breaks Talk integration tests (as it is askign on the root node whether it is shared, and that triggers:

 "Exception": "OCP\\Files\\ForbiddenException",
  "Message": "Invalid path",
  "Code": 0,
  "Trace": [
    {
      "file": "/home/nickv/Nextcloud/19/server/lib/private/Files/Storage/Local.php",
      "line": 159,
      "function": "getSourcePath",
      "class": "OC\\Files\\Storage\\Local",
      "type": "->",
      "args": [
        "/.htaccess"
      ]
    },

Full log + stack:

{"Exception":"OCP\\Files\\ForbiddenException","Message":"Invalid path","Code":0,"Trace":[{"file":"\/home\/nickv\/Nextcloud\/19\/server\/lib\/private\/Files\/Storage\/Local.php","line":159,"function":"getSourcePath","class":"OC\\Files\\Storage\\Local","type":"->","args":["\/.htaccess"]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/lib\/private\/Files\/Storage\/Common.php","line":879,"function":"getMetaData","class":"OC\\Files\\Storage\\Local","type":"->","args":["\/.htaccess"]},{"function":"getDirectoryContent","class":"OC\\Files\\Storage\\Common","type":"->","args":[""]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/lib\/private\/Files\/Cache\/Scanner.php","line":408,"function":"iterator_to_array","args":[{"__class__":"Generator"}]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/lib\/private\/Files\/Cache\/Scanner.php","line":388,"function":"handleChildren","class":"OC\\Files\\Cache\\Scanner","type":"->","args":["",false,3,16,true,0]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/lib\/private\/Files\/Cache\/Scanner.php","line":340,"function":"scanChildren","class":"OC\\Files\\Cache\\Scanner","type":"->","args":["",false,3,16,true]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/lib\/private\/Files\/View.php","line":1338,"function":"scan","class":"OC\\Files\\Cache\\Scanner","type":"->","args":["",false]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/lib\/private\/Files\/View.php","line":1382,"function":"getCacheEntry","class":"OC\\Files\\View","type":"->","args":[{"cache":{"__class__":"OC\\Files\\Cache\\Cache"},"scanner":{"__class__":"OC\\Files\\Cache\\Scanner"},"watcher":null,"propagator":null,"updater":null,"__class__":"OCA\\Files_Trashbin\\Storage"},"",""]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/lib\/private\/Files\/Node\/Node.php","line":100,"function":"getFileInfo","class":"OC\\Files\\View","type":"->","args":["\/"]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/lib\/private\/Files\/Node\/Node.php","line":342,"function":"getFileInfo","class":"OC\\Files\\Node\\Node","type":"->","args":[]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/appsbabies\/spreed\/lib\/Files\/Util.php","line":206,"function":"isShared","class":"OC\\Files\\Node\\Node","type":"->","args":[]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/appsbabies\/spreed\/lib\/Files\/Util.php","line":133,"function":"getAnyDirectShareOfNodeAccessibleByUser","class":"OCA\\Talk\\Files\\Util","type":"->","args":[{"__class__":"OC\\Files\\Node\\Root"},"participant1"]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/appsbabies\/spreed\/lib\/Files\/Listener.php","line":117,"function":"getAnyPublicShareOfFileOwnedByUserOrAnyDirectShareOfFileAccessibleByUser","class":"OCA\\Talk\\Files\\Util","type":"->","args":["967","participant1"]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/appsbabies\/spreed\/lib\/Files\/Listener.php","line":69,"function":"preventUsersWithoutAccessToTheFileFromJoining","class":"OCA\\Talk\\Files\\Listener","type":"->","args":[{"__class__":"OCA\\Talk\\Room"},"participant1"]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/3rdparty\/symfony\/event-dispatcher\/EventDispatcher.php","line":251,"function":"OCA\\Talk\\Files\\{closure}","class":"OCA\\Talk\\Files\\Listener","type":"::","args":["*** sensitive parameters replaced ***"]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/3rdparty\/symfony\/event-dispatcher\/EventDispatcher.php","line":73,"function":"callListeners","class":"Symfony\\Component\\EventDispatcher\\EventDispatcher","type":"->","args":[[{"__class__":"Closure"},{"__class__":"Closure"}],"*** sensitive parameter replaced ***","*** sensitive parameter replaced ***"]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/lib\/private\/EventDispatcher\/EventDispatcher.php","line":86,"function":"dispatch","class":"Symfony\\Component\\EventDispatcher\\EventDispatcher","type":"->","args":["*** sensitive parameter replaced ***","*** sensitive parameter replaced ***"]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/appsbabies\/spreed\/lib\/Room.php","line":770,"function":"dispatch","class":"OC\\EventDispatcher\\EventDispatcher","type":"->","args":["*** sensitive parameter replaced ***","*** sensitive parameter replaced ***"]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/appsbabies\/spreed\/lib\/Controller\/RoomController.php","line":1155,"function":"joinRoom","class":"OCA\\Talk\\Room","type":"->","args":[{"__class__":"OC\\User\\User"},"",true]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/lib\/private\/AppFramework\/Http\/Dispatcher.php","line":169,"function":"joinRoom","class":"OCA\\Talk\\Controller\\RoomController","type":"->","args":["a3pvmsv9",""]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/lib\/private\/AppFramework\/Http\/Dispatcher.php","line":99,"function":"executeController","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->","args":[{"__class__":"OCA\\Talk\\Controller\\RoomController"},"joinRoom"]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/lib\/private\/AppFramework\/App.php","line":136,"function":"dispatch","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->","args":[{"__class__":"OCA\\Talk\\Controller\\RoomController"},"joinRoom"]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/lib\/private\/AppFramework\/Routing\/RouteActionHandler.php","line":47,"function":"main","class":"OC\\AppFramework\\App","type":"::","args":["OCA\\Talk\\Controller\\RoomController","joinRoom",{"__class__":"OC\\AppFramework\\DependencyInjection\\DIContainer"},{"apiVersion":"v1","token":"a3pvmsv9","_route":"ocs.spreed.Room.joinRoom"}]},{"function":"__invoke","class":"OC\\AppFramework\\Routing\\RouteActionHandler","type":"->","args":[{"apiVersion":"v1","token":"a3pvmsv9","_route":"ocs.spreed.Room.joinRoom"}]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/lib\/private\/Route\/Router.php","line":298,"function":"call_user_func","args":[{"__class__":"OC\\AppFramework\\Routing\\RouteActionHandler"},{"apiVersion":"v1","token":"a3pvmsv9","_route":"ocs.spreed.Room.joinRoom"}]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/ocs\/v1.php","line":82,"function":"match","class":"OC\\Route\\Router","type":"->","args":["\/ocsapp\/apps\/spreed\/api\/v1\/room\/a3pvmsv9\/participants\/active"]},{"file":"\/home\/nickv\/Nextcloud\/19\/server\/ocs\/v2.php","line":24,"args":["\/home\/nickv\/Nextcloud\/19\/server\/ocs\/v1.php"],"function":"require_once"}],"File":"\/home\/nickv\/Nextcloud\/19\/server\/lib\/private\/Files\/Storage\/Local.php","Line":435,"CustomMessage":"--"}

@nickvergessen
Copy link
Member

To see it run ./run.sh features/conversation/files.feature:223 from inside apps/spreed/tests/integration/

@icewind1991
Copy link
Member Author

I'm having some issues running the tests but I guess #20599 might fix it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants