migrate oc8 mount.json external storages to the database #28662

phisch · 2017-08-14T08:02:12Z

Description

This is a follow-up to #28655. This migrates storages defined in data/mount.json to the database.

Related Issue

#28655

How Has This Been Tested?

Has been tested manually with the limited storage options i had (local mount).

Types of changes

Bug fix (non-breaking change which fixes an issue)
New (old?) feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

… external stores, marked files_external to use migrations

PVince81

Looks fine so far

PVince81 · 2017-08-14T08:06:39Z

apps/files_external/appinfo/Migrations/Version20170814051424.php

+    public function run(IOutput $out) {
+
+		/** @var GlobalStoragesService $globalStoragesService */
+    	$globalStoragesService = \OC::$server->query('GlobalStoragesService');


copy-pasted indents failed

PVince81 · 2017-08-14T08:08:26Z

lib/private/Files/External/Service/LegacyStoragesService.php

+	 * @return array list of mount configs
+	 */
+	protected function readLegacyConfig() {
+		$mountConfig = new \OC_Mount_Config();


Please use LegacyUtil instead to not re-add a dependency on this horrible class.

Most of the code was moved from OC_Mount_Config to \OC\Files\External\LegacyUtil

PVince81 · 2017-08-14T08:08:46Z

lib/private/Files/External/Service/LegacyStoragesService.php

+			$storageOptions['priority'] = $backend->getPriority();
+		}
+		$storageConfig->setPriority($storageOptions['priority']);
+		if ($mountType === \OC_Mount_Config::MOUNT_TYPE_USER) {


same for the constants

phisch · 2017-08-14T08:43:42Z

lib/private/Files/External/Service/LegacyStoragesService.php

+		if (isset($storageOptions['authMechanism']) && $storageOptions['authMechanism'] !== 'builtin::builtin') {
+			$authMechanism = $this->backendService->getAuthMechanism($storageOptions['authMechanism']);
+		} else {
+			$authMechanism = $backend->getLegacyAuthMechanism($storageOptions);


@PVince81 the backend doesn't have this method anymore. Why do we set the storage option to null, but throw an exception when there is no legacy auth mechanism? edit: should i un-delete this method, or can we work around this?

Before we introduced the concept of "one storage can have multiple auth mechanisms", every storage could only have a single one. I think the legacy auth value was there to tell which of the multiple ones the migration should default to if none is specified. (I'm not 100% sure of this though)

If specifying null here is problematic, maybe we need to change the logic to make "null" automatically default to the only auth mechanism supported by the backend, if any.

phisch · 2017-08-14T08:46:10Z

lib/private/Files/External/Service/LegacyStoragesService.php

+						// but at this point we don't know the max-id, so use
+						// first group it by config hash
+						$storageOptions['mountpoint'] = $rootMountPath;
+						$configId = \OC_Mount_Config::makeConfigHash($storageOptions);


@PVince81 makeConfigHash doesn't exist anymore, do you know if it is important that this is actually the hashed config, or does it just has to be unique? Can't we use uniqidfor this?

Bring it back and put it into LegacyUtil. It must be a hash of all config values because this is used to deduplicate configurations. Storing duplicate configs with slight variations (like "applicable" field) is the way mount.json used to work...

…roduced makeConfigHash, fixed some minor things

phisch · 2017-08-14T09:52:26Z

lib/private/Files/External/Service/LegacyStoragesService.php

+			$authMechanism = $this->backendService->getAuthMechanism($storageOptions['authMechanism']);
+		} else {
+			$authMechanisms = $this->backendService->getAuthMechanisms();
+			$authMechanism = $authMechanisms[0];


Using the first available AuthMechanism should be fine.

PVince81

Looks better. I still have some doubt about your usage of "null" storage, but am also confused about the difference between "$storageOptions" and setAuthMechanism. Did you find out ?

PVince81 · 2017-08-14T09:53:41Z

apps/files_external/appinfo/Migrations/Version20170814051424.php

+	public function run(IOutput $out) {
+
+		/** @var GlobalStoragesService $globalStoragesService */
+		$globalStoragesService = \OC::$server->query('GlobalStoragesService');


I guess injection in constructor doesn't work in migrations ?

Migration files are required by the MigrationService, so we can't use the DI for them. Making them "container aware" like that should be fine. (even if container aware stinks)

PVince81 · 2017-08-14T09:54:13Z

apps/files_external/appinfo/Migrations/Version20170814051424.php

+
+		/** @var GlobalStoragesService $globalStoragesService */
+		$globalStoragesService = \OC::$server->query('GlobalStoragesService');
+		$legacyStoragesService = new LegacyStoragesService(\OC::$server->getStoragesBackendService());


above you query by string name and here by direct method, consistency would be nice (injection even better)

I do not want to add the LegacyStorageService to the DI container, just so others don't get the idea to use it 😛

PVince81 · 2017-08-14T09:54:47Z

apps/files_external/appinfo/info.xml

 	</types>
 	<ocsid>166048</ocsid>
-
+	<use-migrations>true</use-migrations>


you likely also need to increase the app version to trigger the migrations?

Yeah, forgot about that.

But for anyone who actually needs this migration, the version would already bump. (oc8 update).

PVince81 · 2017-08-14T09:56:05Z

lib/private/Files/External/Service/LegacyStoragesService.php

+		} else {
+			$authMechanisms = $this->backendService->getAuthMechanisms();
+			$authMechanism = $authMechanisms[0];
+			$storageOptions['authMechanism'] = 'null'; // to make error handling easier


wouldn't this make it use the "null" authentication mechanism ?
shouldn't this be replaced to the value of $authMechanism or its alias ?

This is about what the old code was like, just that it gets another $authMechanism here. I really don't know what the idea behind the old code was.

if it works fine then ok

phisch · 2017-08-14T10:25:40Z

No, i did not find out more regarding the $storageOptions and setAuthMechanism. It is complicated to understand what the intention behind that was when the code is not compatible anymore. I think using the first available AuthMechanism should be about the best we can do there.

PVince81 · 2017-08-14T10:33:18Z

Test matrix:

Axis 1: mount type
- system-wide, applicable to all
- system-wide, applicable to some users and groups
- personal mount
Axis 2: storage type
- SFTP (has 2 auth mech in OC 10)
- SMB (has 2 auth mech in OC 10)
- GDrive (1 auth mech)
Axis 3: source version
- upgrade from 8.2.10
- upgrade from 9.0.9

Steps:

Check out version from axis 3
Create several external storages based on axis 1 and 2
Upgrade to this PR
Check that storages still work and make sense

PVince81 · 2017-08-14T12:32:03Z

Tested updating from 8.2.11 with GDrive, SFTP, SFTP + RSA key and different "applicable" values to stable10 daily + this PR: the conversion of the configs worked.

PVince81

👍

lock · 2019-08-02T20:02:09Z

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.

added LegacyStoragesService to access mount.json, added migration for…

b2ea702

… external stores, marked files_external to use migrations

phisch added the 3 - To Review label Aug 14, 2017

phisch added this to the development milestone Aug 14, 2017

phisch self-assigned this Aug 14, 2017

phisch requested review from DeepDiver1975 and PVince81 August 14, 2017 08:02

phisch added 2 commits August 14, 2017 10:06

added and fixed missing/outdated file header

dc4f22d

fixed indentation issues

e1a1487

PVince81 reviewed Aug 14, 2017

View reviewed changes

phisch commented Aug 14, 2017

View reviewed changes

phisch added 2 commits August 14, 2017 11:02

using LegacyUtil to not depend on OC_Mount_Config if possible, re int…

ed1a7e4

…roduced makeConfigHash, fixed some minor things

use the first available auth mechanism as fallback

69d3cf1

phisch commented Aug 14, 2017

View reviewed changes

PVince81 reviewed Aug 14, 2017

View reviewed changes

PVince81 mentioned this pull request Aug 14, 2017

Add initial CHANGELOG.md, preparing 10.0.3 beta #28632

Merged

3 tasks

increased version, so the migrations run

613e0e3

PVince81 approved these changes Aug 14, 2017

View reviewed changes

PVince81 added the backport-request label Aug 14, 2017

PVince81 merged commit 5e85c80 into master Aug 14, 2017

PVince81 deleted the migrate-mounts branch August 14, 2017 13:23

phisch mentioned this pull request Aug 14, 2017

[stable10] migrate mounts #28673

Merged

PVince81 removed the backport-request label Aug 15, 2017

felixboehm mentioned this pull request Aug 17, 2017

Release checklist 10.0.3 beta/RC #28685

Closed

31 tasks

ownclouders added the status/STALE label Dec 20, 2017

lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019

migrate oc8 mount.json external storages to the database #28662

migrate oc8 mount.json external storages to the database #28662

Uh oh!

Conversation

phisch commented Aug 14, 2017

Description

Related Issue

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

PVince81 left a comment

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!

phisch Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phisch Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

phisch commented Aug 14, 2017

Uh oh!

PVince81 commented Aug 14, 2017

Uh oh!

PVince81 commented Aug 14, 2017

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

lock bot commented Aug 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

phisch Aug 14, 2017 •

edited

Loading

phisch Aug 14, 2017 •

edited

Loading