Move files external to core #25422

PVince81 · 2016-07-08T14:22:03Z

There's more to come.

mention-bot · 2016-07-08T14:22:04Z

@PVince81, thanks for your PR! By analyzing the annotation information on this pull request, we identified @Xenopathic, @icewind1991 and @LukasReschke to be potential reviewers

PVince81 · 2016-07-08T15:37:50Z

Fixed some tests.
Moved BackendService to core as \OCP\Files\External\IStoragesBackendService implemented as \OC\Files\External\StoragesBackendService.

Looking good so far but there is more to go to have core being able to work without files_external enabled.

PVince81 · 2016-08-19T16:01:35Z

get rid of StorageMigrator

PVince81 · 2016-08-19T16:03:31Z

get rid of LegacyStoragesService: at this stage everybody already migrated their config

PVince81 · 2016-08-19T16:05:42Z

need alias for backend identifiers because they contain "Files_External" in them

PVince81 · 2016-08-19T16:30:17Z

Moved storage services classes to core, still WIP/incomplete. Not tested.
Stay tuned...

DeepDiver1975 · 2016-08-20T13:42:37Z

Conflicting

PVince81 · 2016-09-12T17:47:10Z

Rebased, added more fixes for the tests etc. Seems to work so far, few tests will fail.

I think we should also move the commands and the UI to core.
The UI would go under "settings/".

PVince81 · 2016-09-14T09:55:42Z

I'd say the first main goal is to make external apps like "files_external_ftp" work even when files_external is disabled. Once this works, it's already a big step forward even if a lot of things still need to be moved like the UI and CLI commands.

PVince81 · 2016-10-02T07:04:14Z

Some people disable files _external even though they have storages configured, to disable them.
When migrating, we don't want those to suddenly pop up again.
So I suggest to first implement the feature "disable single storage" to avoid a loss of functionality and update surprises.
Then the migration code will detect whether files_external was disabled and if yes will disable all storages.

disable all ext storages if files_external was disabled during update, requires External Storage: add option to disable a storage temporarly #11518

PVince81 · 2016-10-05T15:52:25Z

find out what setLegacyAuthMechanism* methods are for and see if we can get rid of them
=> can't get rid of it. It seems to be related to old storages like "SMB_OC" that need to be mapped to an auth mechanism class to be able to work as it did before.
We'll only be able to get rid of these once we provide a per-storage-type config migration that properly converts the settings to the new formats.

PVince81 · 2016-10-05T15:59:55Z

Good news! It is now possible to run this FTP branch owncloud/files_external_ftp#9 with this PR here and have the "files_external" app disabled !

You'll still need to enable "files_external" first to configure the storage but then can disable it again. The storage will stay available.

There is still a lot of work to do to make sure everything still works.

And also review whether each class I picked for the public or private namespace makes sense.
I had to add a few aliases in the server container for DI to work properly.

PVince81 · 2016-10-05T16:25:23Z

1) Test\Files\External\Service\GlobalStoragesServiceTest::testUpdateStorage with data set #0 (array('mountpoint', 'identifier:\Test\Files\Extern...ackend', 'identifier:\Auth\Mechanism', array('value1', 'value2', 'testPassword'), array(), array(), 15))
18:15:43 TypeError: Argument 1 passed to Mock_AuthMechanism_86ad8386::wrapStorage() must implement interface OCP\Files\Storage, null given, called in /var/lib/jenkins/workspace/owncloud-core_core_PR-25422-NVXOMA4CC3FS3H34SQTLTEWMW4QUKUB4WWGYUPAFM4P2XYM3IARA/lib/private/Files/External/Service/StoragesService.php on line 535
18:15:43 
18:15:43 /var/lib/jenkins/workspace/owncloud-core_core_PR-25422-NVXOMA4CC3FS3H34SQTLTEWMW4QUKUB4WWGYUPAFM4P2XYM3IARA/lib/private/Files/External/Service/StoragesService.php:535
18:15:43 /var/lib/jenkins/workspace/owncloud-core_core_PR-25422-NVXOMA4CC3FS3H34SQTLTEWMW4QUKUB4WWGYUPAFM4P2XYM3IARA/lib/private/Files/External/Service/StoragesService.php:451
18:15:43 /var/lib/jenkins/workspace/owncloud-core_core_PR-25422-NVXOMA4CC3FS3H34SQTLTEWMW4QUKUB4WWGYUPAFM4P2XYM3IARA/tests/lib/Files/External/Service/GlobalStoragesServiceTest.php:180
18:15:43 
18:15:43 2) Test\Files\External\Service\GlobalStoragesServiceTest::testUpdateStorage with data set #1 (array('mountpoint', 'identifier:\Test\Files\Extern...ackend', 'identifier:\Auth\Mechanism', array('value1', 'value2', 'testPassword'), array('user1', 'user2'), array(), 15))
18:15:43 TypeError: Argument 1 passed to Mock_AuthMechanism_86ad8386::wrapStorage() must implement interface OCP\Files\Storage, null given, called in /var/lib/jenkins/workspace/owncloud-core_core_PR-25422-NVXOMA4CC3FS3H34SQTLTEWMW4QUKUB4WWGYUPAFM4P2XYM3IARA/lib/private/Files/External/Service/StoragesService.php on line 535
18:15:43

Weird, I don't get these locally... Maybe related to PHP version.

PVince81 · 2016-10-05T18:07:17Z

Aha! Indeed I see these errors with PHP 7.0.11

PVince81 · 2016-10-05T19:09:40Z

get rid of OC_MountConfig references in core:

lib/private/Files/External/Service/LegacyStoragesService.php:82:                if ($mountType === \OC_Mount_Config::MOUNT_TYPE_USER) {
lib/private/Files/External/Service/LegacyStoragesService.php:88:                } else if ($mountType === \OC_Mount_Config::MOUNT_TYPE_GROUP) {
lib/private/Files/External/Service/LegacyStoragesService.php:153:                                       $storageOptions['options'] = \OC_Mount_Config::decryptPasswords($storageOptions['opti
...

PVince81 · 2016-10-05T19:10:16Z

oh, and fix encryption:

apps/encryption/lib/Migration.php:323:          return \OC_Mount_Config::getSystemMountPoints();
lib/private/Encryption/File.php:88:                     $mounts = \OC_Mount_Config::getSystemMountPoints();
lib/private/Encryption/Util.php:290:                    $mounts = \OC_Mount_Config::getSystemMountPoints();

PVince81 · 2016-10-06T07:52:33Z

raise ticket for future: ability to register mount providers / storage backends in info.xml

PVince81 · 2016-10-06T08:11:48Z

@DeepDiver1975 @jvillafanez should we get rid of the ability to import old style "mount.json" using the "Import" command ? This would make it possible to clear a lot of legacy cruft and reduce testing overhead.

The only use case I can think of is admins who provision their OC using mount.json files automatically and haven't taken the time to convert these configs to the new json export format.

As for existing mount.json files inside the install, these should already be migrated to the DB format.

PVince81 · 2016-10-06T09:01:25Z

PVince81 · 2016-10-06T09:08:03Z

add shims for password/null/builtin auth mechanisms in case old apps are using it ? => no
move Password auth mechanism to public namespace to allow storages using it as legacy auth ? => no

=> no more legacy auth needed

PVince81 · 2016-10-06T09:21:43Z

BUG: order of auth mechanisms is now different. "Username and password" should be at the top in the dropdown. (this is due to registration order...)

PVince81 · 2016-10-06T11:23:35Z

some performance concerns: after merging this every setupFS call will have to read the ext storage tables even if there is nothing there. So even people who don't use ext storage would get a perf decrease. Possible long-term solution here in the fstab discussion: #26190

PVince81 · 2016-10-06T11:47:16Z

Failure possibly due to array expectation order:

13:32:03 1) Test\Files\External\Service\DBConfigServiceTest::testAddApplicable
13:32:03 Failed asserting that two arrays are equal.
13:32:03 --- Expected
13:32:03 +++ Actual
13:32:03 @@ @@
13:32:03  Array (
13:32:03      0 => Array (
13:32:03 -        'type' => 3
13:32:03 -        'value' => 'test'
13:32:03 -        'mount_id' => 2
13:32:03 +        'type' => 1
13:32:03 +        'value' => null
13:32:03 +        'mount_id' => '2'
13:32:03      )
13:32:03      1 => Array (...)
13:32:03      2 => Array (
13:32:03 -        'type' => 1
13:32:03 -        'value' => null
13:32:03 -        'mount_id' => 2
13:32:03 +        'type' => 3
13:32:03 +        'value' => 'test'
13:32:03 +        'mount_id' => '2'
13:32:03      )
13:32:03  )
13:32:03 
13:32:03 /var/lib/jenkins/workspace/owncloud-core_core_PR-25422-NVXOMA4CC3FS3H34SQTLTEWMW4QUKUB4WWGYUPAFM4P2XYM3IARA/tests/lib/Files/External/Service/DBConfigServiceTest.php:96
13:32:03

butonic · 2016-10-14T06:23:40Z

will fix #22641

PVince81 · 2016-11-03T11:22:16Z

Raised #26538 to handle missing backends more gracefully.

PVince81 · 2016-11-08T09:33:46Z

This is tricky... I tried enabling the WND app and making it work without any modifications to the app.
This is not possible with this PR because we need to introduce proper interfaces and also the WND app is using classes from OCA\Files_External that it's not supposed to use.

And the WND app also provides some additional auth providers which need to be adjusted to use IStorageConfig instead of the old object to be compatible. I guess it's ok to ask storage and auth providers to adjust their implementations to this PR's change.

DeepDiver1975 · 2016-11-08T09:36:59Z

And the WND app also provides some additional auth providers which need to be adjusted to use IStorageConfig instead of the old object to be compatible. I guess it's ok to ask storage and auth providers to adjust their implementations to this PR's change.

absolutly

PVince81 · 2016-11-08T10:18:03Z

Tested updating from OC 8.0 with SFTP_Key, SMB_OC, Dropbox and others up to this branch, all worked fine. This means that getting rid of the legacy stuff did not break those, which is great news.

PVince81 · 2016-11-08T14:50:05Z

What sound does it make when squashing 21 commits ?

Not sure, but that's what I did here. Hopefully a single commit will make all the renames more apparent.

PVince81 · 2016-11-08T16:10:03Z

Went through my test steps and it's looking good so far !

Apart from this little bug #25422 (comment) I think this could be merged.

However once this is merge we should work on making it fail gracefully in case a third party ext storage app gets disabled during update, this would be handled through an approach like this: #26539

jvillafanez · 2016-11-11T08:56:54Z

We should make the Backend class abstract with an abstract method "initializeBackend". Something like:

abstract class Backend implements \JsonSerializable {
    public function __construct(IL10N $a) {
        $this->initializeBackend($a);
    }
    abstract protected function initializeBackend(IL10N $a);
}

This way we can force the Backend class to not be instanciated (it doesn't make any sense), and also point out to the subclasses what is the method that they should implement.

If the subclasses choose to redefine the constructor, they should call the parent constructor first (which is common practice anyway)

jvillafanez · 2016-11-11T08:57:15Z

apps/files_external/lib/Lib/Backend/SFTP_Key.php

@@ -42,7 +40,6 @@ public function __construct(IL10N $l, RSA $legacyAuth, SFTP $sftpBackend) {
 					->setFlag(DefinitionParameter::FLAG_OPTIONAL),
 			])
 			->addAuthScheme(AuthMechanism::SCHEME_PUBLICKEY)
-			->setLegacyAuthMechanism($legacyAuth)
 			->deprecateTo($sftpBackend)


I'm not finding this method... 😕

It's in IdentifierTrait

PVince81 · 2016-11-11T10:20:23Z

This way we can force the Backend class to not be instanciated (it doesn't make any sense), and also point out to the subclasses what is the method that they should implement.

While I agree with the base idea there are some issues:

backends might need dependency injection in their constructors. So I'd say the init method should have no arguments. It's the backend's responsibility to get the auto-DI from the constructor and store the instances. => no big issue
Instantiation is needed to get access to the param definition. Not sure if always needed in a regular usage where the admin page is not shown, would need to check.
When instantiating StorageConfig we are storing an instance of Backend quite early. Ideally I'd like to change this to only store the backend id and only instantiate the backend (and also auth mechanisms) at the last moment. But I'd say out of scope for this PR.

@jvillafanez what do you think ? Is what I mentionned in 3) also what you hoped by making the init lazy ?

PVince81 · 2016-11-11T10:24:24Z

Ref from the old code: https://github.com/owncloud/core/blob/v9.1.2/apps/files_external/lib/Service/StoragesService.php#L295

jvillafanez · 2016-11-15T07:57:12Z

For me it doesn't make any sense that we can create instances of the Backend class when we have specific implementations. That's the reason to make the Backend class abstract.

I didn't think about lazy initialization... I completely agree we should go that way. The only we should take care is that we'll need to make this completely transparent to the implementations: if we consider that the subclasses must implement an "initBackend" method for example, checking if the backend is initialized or "deinitialize" the backend should be done transparently in the parent class.

PVince81 · 2016-11-15T08:11:27Z

@jvillafanez ok, now I understand. I'm fine making the Backend class abstract. Not sure if initBackend is needed here because subclasses can still have their own constructor and we're not trying to enforce any specific signature.

jvillafanez · 2016-11-15T10:32:41Z

I don't think it's worthy to initialize lazily the backend. Too much trouble for a small benefit in this case since most of the things the backend does are assigments. Let's just make the class abstract.

PVince81 · 2016-11-21T11:07:38Z

Here, they're abstract now: e33028c

Let's hope the tests still pass, if they do, merge ? @jvillafanez @DeepDiver1975

DeepDiver1975 · 2016-11-21T13:04:30Z

👍

lock · 2019-08-04T08:02:48Z

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.

PVince81 added the 2 - Developing label Jul 8, 2016

PVince81 added this to the 9.2 milestone Jul 8, 2016

PVince81 mentioned this pull request Jul 8, 2016

Move files_external backend + config logic to core #18160

Closed

DeepDiver1975 mentioned this pull request Jul 18, 2016

Make sure files_external is loaded before accessing \OC_Mount_Config … #25334

Closed

PVince81 force-pushed the move-files_external-to-core branch from aee765f to 815d018 Compare September 12, 2016 17:46

PVince81 mentioned this pull request Sep 14, 2016

Use PSR-4 and public API owncloud/files_external_ftp#9

Merged

DeepDiver1975 force-pushed the move-files_external-to-core branch from 815d018 to e3c278c Compare September 27, 2016 13:32

PVince81 force-pushed the move-files_external-to-core branch from e3c278c to 4502170 Compare October 5, 2016 15:57

PVince81 mentioned this pull request Oct 5, 2016

Add SFTP tests to CI #26283

Closed

PVince81 force-pushed the move-files_external-to-core branch from 4a3ac48 to 1812d87 Compare October 6, 2016 11:59

PVince81 force-pushed the move-files_external-to-core branch from e975083 to ca7bf05 Compare November 8, 2016 09:25

PVince81 force-pushed the move-files_external-to-core branch from b8fe1d9 to 9f66e45 Compare November 8, 2016 14:49

Move ext storage registration APIs to core

269f6f6

PVince81 force-pushed the move-files_external-to-core branch from 122a276 to 269f6f6 Compare November 8, 2016 20:38

PVince81 added 3 - To Review and removed 2 - Developing labels Nov 8, 2016

jvillafanez reviewed Nov 11, 2016

View reviewed changes

Make files external AuthMechanism and Backend abstract

e33028c

DeepDiver1975 merged commit 8c8fe55 into master Nov 21, 2016

DeepDiver1975 deleted the move-files_external-to-core branch November 21, 2016 13:04

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

Move files external to core #25422

Move files external to core #25422

Conversation

PVince81 commented Jul 8, 2016

mention-bot commented Jul 8, 2016

PVince81 commented Jul 8, 2016

PVince81 commented Aug 19, 2016 • edited Loading

PVince81 commented Aug 19, 2016 • edited Loading

PVince81 commented Aug 19, 2016

PVince81 commented Aug 19, 2016

DeepDiver1975 commented Aug 20, 2016

PVince81 commented Sep 12, 2016

PVince81 commented Sep 14, 2016

PVince81 commented Oct 2, 2016 • edited Loading

PVince81 commented Oct 5, 2016 • edited Loading

PVince81 commented Oct 5, 2016

PVince81 commented Oct 5, 2016

PVince81 commented Oct 5, 2016

PVince81 commented Oct 5, 2016 • edited Loading

PVince81 commented Oct 5, 2016 • edited Loading

PVince81 commented Oct 6, 2016

PVince81 commented Oct 6, 2016

PVince81 commented Oct 6, 2016 • edited Loading

PVince81 commented Oct 6, 2016 • edited Loading

PVince81 commented Oct 6, 2016

PVince81 commented Oct 6, 2016

PVince81 commented Oct 6, 2016

butonic commented Oct 14, 2016

PVince81 commented Nov 3, 2016

PVince81 commented Nov 8, 2016

DeepDiver1975 commented Nov 8, 2016

PVince81 commented Nov 8, 2016

PVince81 commented Nov 8, 2016

PVince81 commented Nov 8, 2016

jvillafanez commented Nov 11, 2016

jvillafanez Nov 11, 2016

Choose a reason for hiding this comment

PVince81 Nov 11, 2016

Choose a reason for hiding this comment

PVince81 commented Nov 11, 2016

PVince81 commented Nov 11, 2016

jvillafanez commented Nov 15, 2016

PVince81 commented Nov 15, 2016

jvillafanez commented Nov 15, 2016

PVince81 commented Nov 21, 2016

DeepDiver1975 commented Nov 21, 2016

lock bot commented Aug 4, 2019

PVince81 commented Aug 19, 2016 •

edited

Loading

PVince81 commented Aug 19, 2016 •

edited

Loading

PVince81 commented Oct 2, 2016 •

edited

Loading

PVince81 commented Oct 5, 2016 •

edited

Loading

PVince81 commented Oct 5, 2016 •

edited

Loading

PVince81 commented Oct 5, 2016 •

edited

Loading

PVince81 commented Oct 6, 2016 •

edited

Loading

PVince81 commented Oct 6, 2016 •

edited

Loading