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

[ext storage] Rely on numeric_id, store config in DB, improve config format #11261

Closed
PVince81 opened this issue Sep 23, 2014 · 35 comments
Closed

Comments

@PVince81
Copy link
Contributor

Code like this is just unreadable and difficult to extend: https://github.com/owncloud/core/blob/master/apps/files_external/lib/config.php#L503

The external storage mount points are stored with multi-dimensional arrays:

$mounts[$mountType][$applicableUsers][$mountPointName][$extraField]

This needs a lot of code gymnastics to get to the right information and it also increases the risk of misunderstanding and introducing bugs this way.

I suggest to provide a new class to contain that data, like:

class MountPoint {
    private $type; // personal or system
    private $applicable; // array of users/groups to which the mount point applies
    private $mountPath; // path to the mount point in the user's file system
    private $priority;
    // ...
}

Hopefully in OC8...

@icewind1991 @Xenopathic

@PVince81
Copy link
Contributor Author

This is especially important if external storage is going to play a more important role in the future.

@MTRichards @craigpg @karlitschek

CC @fernandoruiz @jmaciasportela

@PVince81
Copy link
Contributor Author

This is just half of the pain.
The other half is the way how the config is stored in mount.json.
If you create a SFTP storage for two users, the config looks like this:

    "user": {
        "user1": {
            "\/$user\/files\/sftp": {
                "class": "\\OC\\Files\\Storage\\SFTP",
                "options": {
                    "host": "localhost",
                    "user": "vincent",
                    "password": "",
                    "root": "\/home\/vincent\/temp\/sftpmount",
                    "password_encrypted": "MTAxMmZiYmMxNGRmYjM3ZH8hV2NliG6z0wi7lCxy+tY="
                },
                "priority": 100
            }
        },
        "root": {
            "\/$user\/files\/sftp": {
                "class": "\\OC\\Files\\Storage\\SFTP",
                "options": {
                    "host": "localhost",
                    "user": "vincent",
                    "password": "",
                    "root": "\/home\/vincent\/temp\/sftpmount",
                    "password_encrypted": "Y2JmMTU3ZWJmNjczZWQzZRvQv08Iz9oXgGVQPaowiRo="
                },
                "priority": 100
            }
        }
    }

The config is stored multiple times for every user/group.
There is logic to magically merge the config to show as a single row in the UI... it merges the configs if all the parameters (including password) are the same. Else they will appear as separate entries.

@karlitschek
Copy link
Contributor

yes. agreed

@MTRichards MTRichards added this to the backlog milestone Sep 23, 2014
@RobinMcCorkell
Copy link
Member

The multi-stacked arrays are very difficult to handle, I agree. I do have a suggestion for the JSON output though - instead of storing it as 'applicable type -> applicable -> storage -> class/options...', store it as 'storage -> class/options/applicable...'. For example:

  "\/$user\/files\/sftp": {
      "class": "\\OC\\Files\\Storage\\SFTP",
      "options": {
          "host": "localhost",
          "user": "vincent",
          "password": "",
          "root": "\/home\/vincent\/temp\/sftpmount",
          "password_encrypted": "MTAxMmZiYmMxNGRmYjM3ZH8hV2NliG6z0wi7lCxy+tY="
      },
      "priority": 100,
      "applicable": {
          "user": [
              "user1",
              "root"
          ]
      }
  }

The only drawback is that finding applicable mount points becomes harder - the code needs to iterate through every storage and check if it is applicable.

Alternatively, what about storing this in the database? oc_files_external_storages to define the storages, oc_files_external_params to define parameters of each storage, and oc_files_external_map to map the storages to their applicable users/groups:

oc_files_external_storages
==========================
id | class
-------------------------------
0  | \\OC\\Files\\Storage\\SFTP


oc_files_external_params
========================
id | storage_id | name               | value
---------------------------------------------------------------------------------
0  | 0          | host               | localhost
1  | 0          | user               | vincent
2  | 0          | password           | NULL
3  | 0          | root               | \/home\/vincent\/temp\/sftpmount
4  | 0          | password_encrypted | MTAxMmZiYmMxNGRmYjM3ZH8hV2NliG6z0wi7lCxy+tY=

oc_files_external_map
=====================
id | storage_id | type   | value
---------------------------------------
0  | 0          | user   | user1
1  | 0          | user   | root

Or something to that effect

@PVince81
Copy link
Contributor Author

Storing in the database might make more sense, yes. It was already mentioned several times in discussions. I also like that you added the storage column which can be mapped to the matching oc_storages table instead of doing the usual "generate the storage id by concatenning settings" magic to find the matching entry.

@PVince81 PVince81 modified the milestones: 2014-sprint-08-next, backlog Oct 28, 2014
@PVince81
Copy link
Contributor Author

Related: #11830 about the unstable storage string id.

@PVince81
Copy link
Contributor Author

I think oc_files_external_storage could simply be the existing oc_storages table, but extended with a type column.

We'd simply use the numeric_id from oc_storages to map to the configs.

@PVince81
Copy link
Contributor Author

Updated proposal:

oc_storages

id storage_type scope
0 external system
1 external user
2 home user

oc_files_external_params

id storage_id name value
0 0 type sftp
1 0 host localhost
2 0 user vincent
3 0 password NULL
4 0 root /home/vincent/temp/sftpmount
5 0 password_encrypted MTAxMmZiYmMxNGRmYjM3ZH8hV2NliG6z0wi7lCxy+tY=
6 1 type smb
7 1 host localhost
8 1 user vincent
9 1 password NULL
10 1 root /home/vincent/temp/sftpmount
11 1 password_encrypted MTAxMmZiYmMxNGRmYjM3ZH8hV2NliG6z0wi7lCxy+tY=

oc_files_external_applicable

id storage_id type value
0 0 user user1
1 0 user root
2 0 group group1
3 1 user user2

@icewind1991
Copy link
Contributor

Looks good, cant think of any use case missed by that.

This could also be used to handle shares and cleanup that mess

@PVince81
Copy link
Contributor Author

Thanks for the feedback, glad you like it 😉

I'm still having some doubts about the "scope" column, but I guess that's something that we can tweak on the way while working on it.

Also I hope that admins won't mind the removal of mount.json, in case some admins were using it for other things or relying on it for automatically setting up instances. If needed we could add a ./occ files_external:addStorage command that accepts a json file (mount.json) to be able to automatically add storages.

@icewind1991
Copy link
Contributor

The scope column could be replaced by adding something like a "personal" value to the type column in applicable but I'm not sure if that's any better.

@PVince81
Copy link
Contributor Author

This change will also require to spend time fixing the JS part.
Today I discovered that it's using loops to delete every applicable user from the storage, passing the whole configuration every time: https://github.com/owncloud/core/blob/master/apps/files_external/js/settings.js#L136

The introduction of the storage id will make this much easier and cleaner to handle.

@MTRichards
Copy link
Contributor

Also I hope that admins won't mind the removal of mount.json, in case some admins were using it for other things or relying on it for automatically setting up instances. If needed we could add a ./occ files_external:addStorage command that accepts a json file (mount.json) to be able to automatically add storages.

There are cases where admins do use mount.json for automated provisioning, so it would be great to either preserve it, or allow people to make a mount.json and then make a call to occ to import it - so that it can all be automated.

@PVince81
Copy link
Contributor Author

Good point.

I'll start with a list:

  • use database for ext storage config as specified previously
  • fix JS side to work with storage ids
  • migration step that takes existing mount.json files (system scope and user scope) and convert them to the new format
  • provide ./occ call to import old mount.json files using the same migration code
  • get rid of string storage id and mount storages using only numeric id

@icewind1991
Copy link
Contributor

Besides having a console command to import mount.json files, having some commands to manage external storages directly would be usefull

@PVince81
Copy link
Contributor Author

True. The import command is mandatory only to make sure there is no loss of feature.
Other operations like removing, updating storages could be added later on, separately.

@MTRichards
Copy link
Contributor

Besides having a console command to import mount.json files, having some commands to manage external storages directly would be usefull

Yes!

Just as an example, I know of a case where a script is run to scan for new users, and then a mount.json is automatically added for each user automatically. So the import should think about admin centrally mounted mount.json files, as well as user mounted files.

And then, in another case, custom mount.json files too (talk to @ogoffart and @guruz about that one).

@PVince81
Copy link
Contributor Author

@MTRichards raised this separate feature as #11886

@MTRichards
Copy link
Contributor

@MTRichards raised this separate feature as #11886

Thanks.

@PVince81
Copy link
Contributor Author

@icewind1991 do you intend to continue working on this ?
Or should we try to hammer the new config options into the existing config ?

I'm teared between fixing the DB first (which isn't trivial) or implementing the options first (which will be duplicated X times in the crappy mount.json format)

@PVince81
Copy link
Contributor Author

Anyway, there is also UI works that need to be done: fixing the UI side to work with the new storage_id instead of passing everything. I made this ticket for that: #12216

The ticket here would rather be for the backend side / storage format.

@PVince81
Copy link
Contributor Author

This is the ticket for moving mount.json to the DB. As discussed with @DeepDiver1975, moving to 8.2.

From what I heard @Xenopathic will work on it soon, so assigning him.

@PVince81
Copy link
Contributor Author

I hope we can totally get rid of the storage id string in the process as it will solve many issues.

@PVince81
Copy link
Contributor Author

@icewind1991 do you think it's possible in any way to make mounting the storages depend on the numeric id instead of storage string id ? How complex would the change be ?

@icewind1991
Copy link
Contributor

I think the main usage of the string id atm is the storage<-> cache mapping, currently a storage calculates it's string id, looks up the numeric id and uses that for cache operations.

We would need to adjust every point a where a mount is created and inject the numeric storage id (non trivial for non-files_external since we dont keep track of numeric storage id's for things like home storages)

@RobinMcCorkell
Copy link
Member

We should try and get rid of getId() on the storage implementation itself, it's awkward to have to construct the storage to get its ID. That leads to easy-to-break code such as https://github.com/owncloud/core/blob/master/apps/files_external/lib/config.php#L971-L980. Especially with session-based credentials, some storages will reject being constructed entirely, so we can never correctly get the ID of such storages.

@RobinMcCorkell RobinMcCorkell modified the milestones: 9.0-next, 8.2-current Sep 10, 2015
@PVince81
Copy link
Contributor Author

PVince81 commented Nov 6, 2015

Moving the the database as a first step (independent from old storage ids, for now): #20354

@PVince81
Copy link
Contributor Author

Also the new oc_mounts table will make it possible to do reverse lookups: #15166

@PVince81
Copy link
Contributor Author

9.0 code has the storages in the database and a mounts table, yay!
This is done.

@lock
Copy link

lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants