ownCloud migrations by DeepDiver1975 · Pull Request #27041 · owncloud/core

DeepDiver1975 · 2017-01-26T11:39:56Z

Description

Own migration mechanism - doctrine/migration cannot be used due to license issues

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.

PVince81 · 2017-01-27T13:57:14Z

upgradeSchema ? (unless you remove down)

down is off the plate - only up - I'm open regarding the naming of this method

PVince81 · 2017-01-30T08:43:13Z

@DeepDiver1975 ready for review or still developing ?

DeepDiver1975 · 2017-01-30T08:50:03Z

@DeepDiver1975 ready for review or still developing ?

Unit testing is to be added - but generally speaking this is ready to be reviewed

PVince81

Look good so far, see my comments for some point needing discussion.

From what I see there is a lot of complexity there so it's very important to have good unit tests here.

PVince81 · 2017-01-30T08:52:01Z

-		$this->addArgument('app', InputArgument::REQUIRED, 'Name of the app this migration command shall work on');
+		$this
+			->setName('migrations:execute')
+			->setDescription('Execute a single migration version up or down manually.')


should we limit this to "up" for now to avoid false hopes from admins who might try "down" out ?

copy paste issue - we only support up

PVince81 · 2017-01-30T08:55:38Z

+/**
+ * Auto-generated migration step: Please modify to your needs!
+ */
+class Version<version> implements ISqlMigration {


Three different migration types ? Is that really needed ? ("keep it simple")

What is the best practice for an app that needs to do Schema and non-Schema migrations (ex: moving stuff inside the data folder). To make it atomic in some way I think a dev would rather write a single generic migration that does all at once: SQL changes, data folder changes, etc all within the run() function.

What do you think ? (to be documented)

So basically I see these three because of the different scenarios

change the schema

in some cases changing the schema is not enough - we need to explicitly apply sql statements

last but not least: I want to allow us to basically write repair steps as migrations as well

In this case a single type could be enough, and receive the SQL connection and the Schema injected in the run() method. This way it is possible to do all three within a single one.

Having interfaces for each type sounds a bit over-engineered. There is also the risk of people implementing all three interfaces on a single migration. Would everything still work fine ?

But maybe you had other good reasons in mind ?

Doing all in run by the implementor of the step requires him/her to do all on his/her own.

Schema migration:

function run($connection) { $fromSchema = $connection->createSchema(); // now do all the schema manipultation try { $connection->migrateToSchema($toSchema); } catch ($ex){ } }

I want to take away this from the implementor - all which is to be implemented: changes to the schema - done.

Furthermore - the migration interfaces are designed to execute any operations after the method returns back to core - which allows us to gain more control over the execution.

This have been the ideas while designing these interfaces.

Ok, fine by me.

I could imagine complex situations where it is not possible:

do a DB update

do a FS update

do another DB update based on the FS changes

But for such complex changes one can simply use the generic runner.

Well - do 3 steps which are executed one after the other

PVince81 · 2017-01-30T08:56:43Z

+		$numNewMigrations = count(array_diff(array_keys($availableMigrations), $executedMigrations));
+
+		$infos = [
+			'App'								=> $configuration->getApp(),


wild indent

PVince81 · 2017-01-30T08:57:39Z

 		return addcslashes($param, '\\_%');
 	}
+
+	public function createSchema() {


PHPDoc, especially considering the MDB2 part

should be on interface level - will take care

PVince81 · 2017-01-30T08:58:44Z

+		// set default table creation options
+		$connectionParams['defaultTableOptions'] = [
+			'collate' => 'utf8_bin',
+			'tablePrefix' => $connectionParams['tablePrefix']


Have we missed this all these years ? Could this save a bit of work from the custom query builder ?

No idea - we shall have a look ...

PVince81 · 2017-01-30T09:07:15Z

+		// read known migrations
+		$toBeExecuted = $migrationConfiguration->getMigrationsToExecute($to);
+		foreach ($toBeExecuted as $version => $class) {
+			$this->executeStep($migrationConfiguration, $version, $class);


how / where are exceptions handled in case an exception is thrown within a step ?
does PHPDoc need an additional @throws clause to clarify this ?

when you add unit tests please also make sure to cover exception handling/forwarding.

PVince81 · 2017-01-30T09:07:48Z

-		$migrationConfiguration->setOutputWriter(new OutputWriter(function ($message){
-			\OCP\Util::writeLog('migrations', $message, \OCP\Util::INFO);
-		}));
+	public function executeStep(MigrationConfiguration $migrationConfiguration, $version, $class = null) {


PVince81 · 2017-01-30T09:08:15Z

@@ -42,14 +42,10 @@

 class Migrator {


Hmmm, what's a Migrator compared to MigrationService and MigrationConfiguration ? Add PHPDoc to clarify

Migrator: old
MigrService and MigConfig: new

@deprecated ?

the migrators are still used for anything which is xml related - I'd keep them for now

PVince81 · 2017-01-30T09:09:06Z

+use OCP\ILogger;
+use OCP\Migration\IOutput;
+
+class SimpleOutput implements IOutput {


What is this and why do we need this specifically for migrations ? Add answer in PHPDoc

PVince81 · 2017-01-30T09:09:58Z

+	 * Create the schema of the connected database
+	 *
+	 * @return Schema
+	 * @since 10.0.0


10.0

We don't add APIs in minor/patch versions

CLAassistant · 2017-01-30T23:28:01Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ PVince81
✅ DeepDiver1975
❌ scrutinizer-auto-fixer

DeepDiver1975 · 2017-01-31T14:23:01Z

Live examples:
https://github.com/owncloud/activity/pull/546/files#diff-04138d72adbf7df60b3bdbd7ec0021f8
https://github.com/owncloud/activity/pull/546/files#diff-2c2d25a12549ca7b22726e57d5fd5152
https://github.com/owncloud/activity/pull/546/files#diff-00ddc4adb2379843e153b3f0b7495bce

PVince81

A few more minor comments.

Else it looks good.

PVince81 · 2017-01-30T16:13:26Z

+
+	}
+
+	private function generateMigration(MigrationService $ms, $version, $kind) {


I know it's a private function, but I miss PHPDoc to explain the format of $version and $kind which are not self-explanatory.

PVince81 · 2017-01-30T16:15:05Z

+
+		$infos = $this->getMigrationsInfos($ms);
+		foreach ($infos as $key => $value) {
+			$output->writeln("    <comment>>></comment> $key: " . str_repeat(' ', 50 - strlen($key)) . $value);


uh, what's this weird string <comment>>></comment> ? Purpose ? Explain in a PHP comment

Ohhhhhh got it

PVince81 · 2017-01-30T16:15:52Z

+		}
+	}
+
+	public function getMigrationsInfos(MigrationService $ms) {


PHPDoc: @return array associative array of human readable info name as key and the actual information as value

PVince81 · 2017-01-30T16:16:28Z

+
+	public function changeSchema(Schema $schema, array $options) {
+		$prefix = $options['tablePrefix'];
+		if (!$schema->hasTable("${prefix}external_mounts")) {


Do we still need the prefix ? I thought it was set on the connection already ?

This is not possible at the moment because the schema class is disconnected to the connection.
The schema as generated from the database holds the prefix already - so when comparing we need to add it.

I'm not yet that happy about this - we might need to subclass Schema or introduce ISchema.
I will do this in a follow up PR - this PR is at it's limits

PVince81 · 2017-01-30T16:16:47Z

+	/**
+	 * @param Schema $schema
+	 */
+	public function down(Schema $schema) {


PVince81 · 2017-01-30T16:17:46Z

@@ -42,14 +42,10 @@

 class Migrator {


@deprecated ?

PVince81 · 2017-01-31T16:14:00Z

Please check my comments then 👍

DeepDiver1975 · 2017-01-31T16:18:15Z

Shall we kill ITransactionalStep for now? Just to keep it simpler? @PVince81

PVince81 · 2017-01-31T16:22:40Z

Shall we kill ITransactionalStep for now? Just to keep it simpler?

I'd say yes, let's remove it.

In general my intuitive feeling when writing migrations was that the migration itself is always within a transaction. So every migration run would have its own transaction. And maybe the top level that runs all migrations also has a parent transaction.

Implement occ commands migrations:status, migrations:execute, migrations:generate and migrations:migrate

PVince81 · 2017-02-01T14:31:27Z

🎉

lock · 2019-08-04T00:02:15Z

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.

DeepDiver1975 added the 2 - Developing label Jan 26, 2017

DeepDiver1975 added this to the 10.0 milestone Jan 26, 2017

DeepDiver1975 self-assigned this Jan 26, 2017

DeepDiver1975 force-pushed the owncloud-migrations branch 3 times, most recently from 85e2980 to ef4213d Compare January 26, 2017 14:15

PVince81 reviewed Jan 27, 2017

View reviewed changes

DeepDiver1975 force-pushed the owncloud-migrations branch from 049f648 to 7a6792e Compare January 27, 2017 14:19

DeepDiver1975 added 3 - To Review and removed 2 - Developing labels Jan 30, 2017

DeepDiver1975 changed the title ~~[WIP] Owncloud migrations~~ ownCloud migrations Jan 30, 2017

PVince81 reviewed Jan 30, 2017

View reviewed changes

PVince81 mentioned this pull request Jan 30, 2017

External storage tables are now created in a migration #26923

Closed

13 tasks

DeepDiver1975 force-pushed the owncloud-migrations branch 2 times, most recently from d7fd46f to 5623297 Compare January 30, 2017 22:14

PVince81 reviewed Jan 31, 2017

View reviewed changes

DeepDiver1975 force-pushed the owncloud-migrations branch from 6544dbf to 5d74e42 Compare January 31, 2017 16:36

DeepDiver1975 and others added 6 commits February 1, 2017 13:30

Kill doctrine/migrations

3775381

External storage tables are now created in a migration

9c4dde7

The default collation is set in the connection for schema operations

4ad984a

Adding schema methods to IDbConnection

491a7b1

Introduce own migration mechanism

3415c0e

Implement occ commands migrations:status, migrations:execute, migrations:generate and migrations:migrate

Implement Version20170111103310 as ISchemaMigration

9cc249c

Fix OracleMigrator to set proper quoted names

6c52ff9

DeepDiver1975 force-pushed the owncloud-migrations branch from 5d74e42 to 6c52ff9 Compare February 1, 2017 12:30

DeepDiver1975 merged commit 97e8f39 into master Feb 1, 2017

DeepDiver1975 deleted the owncloud-migrations branch February 1, 2017 14:12

This was referenced Feb 1, 2017

Doctrine migration wrong default collation #26925

Closed

Upgrade from 9.1.3 to master 💣 #26907

Closed

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


		}

		private function generateMigration(MigrationService $ms, $version, $kind) {

Uh oh!

Conversation

DeepDiver1975 commented Jan 26, 2017

Description

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Jan 30, 2017

Uh oh!

DeepDiver1975 commented Jan 30, 2017

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!

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!

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!

CLAassistant commented Jan 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeepDiver1975 commented Jan 31, 2017

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!

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!

CLAassistant commented Jan 30, 2017 •

edited

Loading