External storage tables are now created in a migration #26923

Open
wants to merge 2 commits into
from

Projects

None yet

4 participants

@PVince81
Collaborator
PVince81 commented Jan 11, 2017 edited

Description

Reimplement the old files_external DB structure as a Doctrine migration.

Related Issue

Fixes #26907

Motivation and Context

Because starting with 10.0 db_structure.xml is not used any more on updates and we need those tables.

How Has This Been Tested?

  • TEST: setup this branch from scratch: tables properly created
  • TEST: setup OC 9.1 without files_external, migrate to this branch: tables properly created
  • TEST: setup OC 9.1 with files_external enabled and a configured ext storage, migrate to this branch: existing tables were kept with contents

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New 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.

TODOs:

  • ⚠️ make sure the table structure is exactly the same as it was when created from db_structure
@PVince81 PVince81 added this to the 10.0 milestone Jan 11, 2017
@PVince81 PVince81 self-assigned this Jan 11, 2017
@mention-bot

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tanghus, @bantu and @butonic to be potential reviewers.

@PVince81
Collaborator
PVince81 commented Jan 11, 2017 edited
  • BUG: wrong integer type
  • BUG: wrong collation

left is 9.1, right is this branch:

441,442c464,465
<   `applicable_id` bigint(20) NOT NULL AUTO_INCREMENT,
<   `mount_id` bigint(20) NOT NULL,
---
>   `applicable_id` int(11) NOT NULL AUTO_INCREMENT,
>   `mount_id` int(11) NOT NULL,
444c467
<   `value` varchar(64) COLLATE utf8_bin DEFAULT NULL,
---
>   `value` varchar(64) COLLATE utf8_unicode_ci NOT NULL,
449c472
< ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin;
---
> ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;
@PVince81
Collaborator

Added explicit collation and explicit bigint(20) values.
I hope this is correct...

I think I shouldn't need to explicitly specify the collation, see #26925

Also I'm a bit surprised that the original schema had 256 chars for "mountpoint" specified, but the resulting schema in Mysql only had 128 chars defined. That's why I also set it to 128 here in the migration...

Basically all I tried to do is to prevent any differences between the 9.1 schema and the current one by diffing the schema dumps.

@DeepDiver1975 @butonic please review

@PVince81
Collaborator
    Test\Files\External\Service\DBConfigServiceTest.testAddApplicable
    Test\Files\External\Service\DBConfigServiceTest.testRemoveApplicableGlobal
    Test\Files\External\Service\DBConfigServiceTest.testGetAdminMountsForGlobal
    Test\Files\External\Service\GlobalStoragesServiceTest::testAddStorage.testAddStorage with data set #0
    Test\Files\External\Service\GlobalStoragesServiceTest::testUpdateStorage.testUpdateStorage with data set #0
    Test\Files\External\Service\GlobalStoragesServiceTest::testUpdateStorage.testUpdateStorage with data set #1
    Test\Files\External\Service\GlobalStoragesServiceTest::testUpdateStorage.testUpdateStorage with data set #2
    Test\Files\External\Service\GlobalStoragesServiceTest::testUpdateStorage.testUpdateStorage with data set #3
    Test\Files\External\Service\GlobalStoragesServiceTest::testHooksAddStorage.testHooksAddStorage with data set #0
    Test\Files\External\Service\GlobalStoragesServiceTest::testHooksUpdateStorage.testHooksUpdateStorage with data set #0
    Test\Files\External\Service\GlobalStoragesServiceTest::testHooksUpdateStorage.testHooksUpdateStorage with data set #3
    Test\Files\External\Service\GlobalStoragesServiceTest::testHooksDeleteStorage.testHooksDeleteStorage with data set #1
    Test\Files\External\Service\GlobalStoragesServiceTest::testDeleteStorage.testDeleteStorage with data set #0
    Test\Files\External\Service\GlobalStoragesServiceTest::testDeleteStorage.testDeleteStorage with data set #1
    Test\Files\External\Service\GlobalStoragesServiceTest.testGetStoragesBackendNotVisible
    Test\Files\External\Service\GlobalStoragesServiceTest.testGetStoragesAuthMechanismNotVisible
    Test\Files\External\Service\GlobalStoragesServiceTest.testUpdateStorageMountPoint
    Test\Files\External\Service\UserGlobalStoragesServiceTest::testGetStorageWithApplicable.testGetStorageWithApplicable with data set #0
    Test\Files\External\Service\UserGlobalStoragesServiceTest::testDeleteStorage.testDeleteStorage with data set #0
    Test\Files\External\Service\UserGlobalStoragesServiceTest::testDeleteStorage.testDeleteStorage with data set #1
    Test\Files\External\Service\UserGlobalStoragesServiceTest::testGetUniqueStorages.testGetUniqueStorages with data set #0
    Test\Files\External\Service\UserGlobalStoragesServiceTest::testGetUniqueStorages.testGetUniqueStorages with data set #1
    Test\Files\External\Service\UserGlobalStoragesServiceTest::testGetUniqueStorages.testGetUniqueStorages with data set #2
    Test\Files\External\Service\UserGlobalStoragesServiceTest::testGetUniqueStorages.testGetUniqueStorages with data set #3
    Test\Files\External\Service\UserGlobalStoragesServiceTest::testGetUniqueStorages.testGetUniqueStorages with data set #8
    Test\Files\External\Service\UserGlobalStoragesServiceTest::testGetUniqueStorages.testGetUniqueStorages with data set #9
    Test\Files\External\Service\UserGlobalStoragesServiceTest.testUpdateStorage

Looks like I messed up some indices:

Integrity constraint violation: 19 NOT NULL constraint failed: oc_external_applicable.value
@PVince81
Collaborator

Either Oracle had a hiccup or the migrations stuff doesn't work properly with Oracle:

19:32:57   [Doctrine\DBAL\Exception\TableNotFoundException]                                                                                                                                                                                                                                                                              
19:32:57   An exception occurred while executing 'SELECT m."mount_id", "mount_point", "storage_backend", "auth_backend", "priority", m."type" FROM "oc_external_mounts" m INNER JOIN "oc_external_applicable" a ON m."mount_id" = a."mount_id" WHERE (a."type" = ?) AND (a."value" = ?) AND (m."type" = 1)' with params [3, "admin23"]:  
19:32:57   ORA-00942: table or view does not exist
@PVince81 PVince81 External storage tables are now created in a migration
294cca5
@PVince81
Collaborator

Rebased + squashed. Let's hope the Oracle tests work now.

+ * @param Schema $schema
+ */
+ public function up(Schema $schema) {
+ $prefix = $this->connection->getPrefix();
@VicDeo
VicDeo Jan 17, 2017 Member

@PVince81 $this->connection is an instance of \Doctrine\DBAL\Connection and has no method getPrefix

@PVince81
PVince81 Jan 18, 2017 Collaborator

No, it's the ownCloud connection. Else this PR would not work at all at setup time. Try it 😉

@VicDeo
VicDeo Jan 18, 2017 Member

@PVince81 I was kicked in the a** by @DeepDiver1975 several times for exactly that kind of magic

@DeepDiver1975
DeepDiver1975 Jan 18, 2017 Member

I'm very aware of this - thanks for fighting me with my own weapons 🔪 .... maybe we need to come up with our own AbstractMigration class to allow proper handling of things like this .... let's keep this out of this pr for now.

@DeepDiver1975 DeepDiver1975 The default collation is set in the connection for schema operations
8e6f512
@PVince81
Collaborator

👍 for @DeepDiver1975's fix

@PVince81
Collaborator

@DeepDiver1975 @VicDeo is my commit acceptable then and can we merge it ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment