Introducing AppData #1306

rullzer · 2016-09-07T14:43:07Z

Follow up of #1039
Implementing #136

This PR introduces an AppData folder. Basically the file variant of app tables. Sometimes you need to store data but where to put it. Or we want a more efficient way to store the data.

For example:

Avatars are now stored directly in the user directory data/userid/avatar.jpg etc. It might make sense to have 1 place for all the avatars. data/<appdata>/avatars/<userid>/avatar.jpg. It could be that your user mounts are on slow (but huge) storage and you want the avatars to be serverd blazing fast.
Previews: In order to share previews properly we should store them by fileid in a generic place. Not in the user folder. That will break preview sharing for shared storage etc.
Themeing: Right now it put themed files under data/ a better place would of course be data/<appdata>/theming/icons/<icon>.svg etc.

Right now this uses the very limited simpleFS (see #1039). And we use the instanceid as appdatafolder. Which means:

An IAppData is chrooted to data/<instanceid>/<appid> this means it can't escape
A simpleFS is mapped on top of that
The root of the simplefs can only contain folder
A folder in a simpleFS can only contain files

This is limiting. I know. But giving more freedom is always easier than reducing it. Plus I think this should be fine for most apps to start with.

Right now this is just mapped on our normal fs. But those abstraction allow this to be improved in the future.

Todo:

AppData folder should be appdata_instanceid
Make sure users with that name fails hard
Mark all as experimental so we can still change for NC12
Rename classes (File/Folder/Root clash way to much)
Make lazy
Tests
Proper DI like L10N

Avatar releated todo:

Fix tests
Migration script

icewind1991 · 2016-09-07T15:40:16Z

lib/private/Files/AppData/Factory.php

+
+			/** @var string $instanceId */
+			try {
+				$appDataFolder = $this->rootFolder->get($instanceId);


imo it would be nices to have something like appdata_$instanceid as the folder name, makes it clear to the admin what that weirdly named folder is

Also can we add some magic to ensure that a user with that name doesn't get that folder? :)

Yes was actually also thinking about that.
Of course we can do that

Well it would be kind of hard to do it for ldap etc for example. But I'm sure we can do it somehwere when we create the users folder

@icewind1991 should we just test for this in the initMountPoints function? that if the userId is equal to appdata_<instanceid> throw exception?

copySkeleton seems like the best place

mmattel · 2016-09-08T16:31:53Z

That will break preview sharing for shared storage etc.

Why not putting all the previews into the database?
Space consumption is more or less the same. Eases access control (access by user or group identification).

rullzer · 2016-09-08T17:20:26Z

@mmattel no that is way to much data. We also don't store all the files just in the db.

rullzer · 2016-09-12T17:49:57Z

Review time!

CC: @icewind1991 @nickvergessen @MorrisJobke @LukasReschke

LukasReschke · 2016-09-12T19:19:56Z

lib/private/Files/AppData/AppData.php

+				try {
+					$appDataFolder = $this->rootFolder->newFolder($name);
+				} catch (NotPermittedException $e) {
+					// Log


/me sees no logging here and below ;)

rullzer · 2016-09-13T14:38:13Z

Skip if avatars are disabled

nickvergessen · 2016-09-13T15:03:57Z

lib/private/Repair.php

 				\OC::$server->getUserManager(),
 				\OC::$server->getGroupManager()
 			),
+			new MoveAvatars(


Dont register, when the config is disabled?

I'd rather not introduce logic in that function. To not complicate things.

* Introduce simpleFS * Introduce IAppData * Introduce AppData Factory to get your AppData folder * Update FileDisplayResponse * AppData implements a ISimpleRoot but lazy. So only if an apps starts to access data will stuff get initialized Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

* Fix AvatarTest Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

* Skip move avatar if avatars disabled Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

codecov-io · 2016-10-05T09:18:20Z

Current coverage is 30.64% (diff: 52.43%)

Merging #1306 into master will decrease coverage by 25.27%

@@             master      #1306   diff @@
==========================================
  Files          1064       1087      +23   
  Lines         60691      60456     -235   
  Methods        6812       6879      +67   
  Messages          0          0            
  Branches          0          0            
==========================================
- Hits          33937      18525   -15412   
- Misses        26754      41931   +15177   
  Partials          0          0

Diff Coverage	File Path
0%	lib/private/legacy/util.php
0%	new lib/private/Repair/NC11/MoveAvatarBackgroundJob.php
0%	lib/private/Server.php
0%	lib/private/Repair.php
0%	lib/composer/composer/autoload_classmap.php
0%	new lib/private/Repair/NC11/MoveAvatars.php
••••• 50%	lib/private/AvatarManager.php
••••••• 75%	new lib/private/Files/AppData/AppData.php
•••••••••• 100%	lib/private/Avatar.php
•••••••••• 100%	new lib/private/Files/SimpleFS/SimpleFolder.php

Review all 16 files changed

Powered by Codecov. Last update 8dafcab...316db0a

rullzer · 2016-10-05T09:50:07Z

Fixed. Review time!

LukasReschke · 2016-10-05T09:55:43Z

lib/public/Files/SimpleFS/ISimpleRoot.php

+	 * @return ISimpleFolder
+	 * @since 9.2.0
+	 */
+	public function newFolder($name);


Missing annotations that this also can return an exception?

https://github.com/nextcloud/server/pull/1306/files#diff-c4d23a2caf57c34cf6a6f376daa5ac98R95

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

MorrisJobke · 2016-10-05T21:49:11Z

👍

LukasReschke · 2016-10-06T07:25:02Z

LGTM

rullzer added enhancement 2. developing Work in progress labels Sep 7, 2016

rullzer added this to the Nextcloud 11.0 milestone Sep 7, 2016

icewind1991 reviewed Sep 7, 2016
View reviewed changes

rullzer force-pushed the simplefs branch from d746250 to 9352665 Compare September 8, 2016 13:21

LukasReschke mentioned this pull request Sep 9, 2016

[WIP] Use new appstore endpoint #1335

Closed

9 tasks

rullzer force-pushed the simplefs branch 4 times, most recently from 3defcd2 to 015a8f4 Compare September 12, 2016 13:57

rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 12, 2016

LukasReschke reviewed Sep 12, 2016
View reviewed changes

MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 13, 2016

rullzer force-pushed the simplefs branch 2 times, most recently from 6ff6af3 to 85e6797 Compare September 13, 2016 12:44

nickvergessen reviewed Sep 13, 2016
View reviewed changes

rullzer added 13 commits October 5, 2016 11:00

Add Tests

ac38a3a

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

avatar to appdata

6807cb6

* Fix AvatarTest Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Avatar migration step

92dc9e6

* Skip move avatar if avatars disabled Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Add for proper DI

3260f69

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Kill users with the reserved name on login

f23390e

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Update autoloader

a961354

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Deprecate old app folder

851769a

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Use a backgroundjob to move avatars

735abbc

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Update autoloader

537af9b

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Fix repair

2578a81

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

DI fails for bg job

a7be37d

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Fix avatar tests

7512683

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer force-pushed the simplefs branch from f17b2cd to 7512683 Compare October 5, 2016 09:00

LukasReschke approved these changes Oct 5, 2016

View reviewed changes

add proper exception documentation for ISimpleFS interface

316db0a

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

MorrisJobke force-pushed the simplefs branch from b36d883 to 316db0a Compare October 5, 2016 21:47

MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 5, 2016

LukasReschke merged commit ea9b1cc into master Oct 6, 2016

LukasReschke deleted the simplefs branch October 6, 2016 07:25

This was referenced Oct 6, 2016

Have something like apps-data for data of applications #136

Closed

AppData documentation nextcloud/documentation#200

Closed

juliusknorr mentioned this pull request Oct 12, 2016

Move background image and logo to AppData #1725

Closed

tflidd mentioned this pull request May 29, 2022

Clarify apps need to be replicated during restores and migrations nextcloud/documentation#8142

Open

joshtrichards mentioned this pull request Jul 9, 2023

apps and categories in apps page are not displayed if oc_data/appdata_XXXXXX isn't writeable by webserver #2706

Closed

Uh oh!

Introducing AppData #1306

Introducing AppData #1306

Uh oh!

Conversation

rullzer commented Sep 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Todo:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmattel commented Sep 8, 2016

Uh oh!

rullzer commented Sep 8, 2016

Uh oh!

rullzer commented Sep 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rullzer commented Sep 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Oct 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 30.64% (diff: 52.43%)

Uh oh!

rullzer commented Oct 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MorrisJobke commented Oct 5, 2016

Uh oh!

LukasReschke commented Oct 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

rullzer commented Sep 7, 2016 •

edited

Loading

rullzer commented Sep 13, 2016 •

edited

Loading

codecov-io commented Oct 5, 2016 •

edited

Loading