Various database migration fixes (#25209)

* String columns with a length higher then 4000 are converted into a CLOB columns automagically - we have to respect this when migrating

* Adding schema migration tests to prevent unnecessary and non-sense migration steps
Fix Oracle autoincrement and unsigned handling

* Fix sqlite integer type for autoincrement

* Use lower case table names - fixes pg

* Fix postgres with default -1 - this only affect pg 9.4 servers - 9.5 seems to work fine

lib/private/DB/MDB2SchemaManager.php

@@ -84,7 +84,7 @@ class MDB2SchemaManager {
 		} else if ($platform instanceof MySqlPlatform) {
 			return new MySQLMigrator($this->conn, $random, $config, $dispatcher);
 		} else if ($platform instanceof PostgreSqlPlatform) {
-			return new Migrator($this->conn, $random, $config, $dispatcher);
+			return new PostgreSqlMigrator($this->conn, $random, $config, $dispatcher);
 		} else {
 			return new NoCheckMigrator($this->conn, $random, $config, $dispatcher);
 		}

lib/private/DB/Migrator.php

@@ -33,6 +33,8 @@ use \Doctrine\DBAL\Schema\Table;
 use \Doctrine\DBAL\Schema\Schema;
 use \Doctrine\DBAL\Schema\SchemaConfig;
 use \Doctrine\DBAL\Schema\Comparator;
+use Doctrine\DBAL\Types\StringType;
+use Doctrine\DBAL\Types\Type;
 use OCP\IConfig;
 use OCP\Security\ISecureRandom;
 use Symfony\Component\EventDispatcher\EventDispatcher;
@@ -194,7 +196,25 @@ class Migrator {
 		return new Table($newName, $table->getColumns(), $newIndexes, array(), 0, $table->getOptions());
 	}
 
+	/**
+	 * @param Schema $targetSchema
+	 * @param \Doctrine\DBAL\Connection $connection
+	 * @return \Doctrine\DBAL\Schema\SchemaDiff
+	 * @throws DBALException
+	 */
 	protected function getDiff(Schema $targetSchema, \Doctrine\DBAL\Connection $connection) {
+		// adjust varchar columns with a length higher then getVarcharMaxLength to clob
+		foreach ($targetSchema->getTables() as $table) {
+			foreach ($table->getColumns() as $column) {
+				if ($column->getType() instanceof StringType) {
+					if ($column->getLength() > $connection->getDatabasePlatform()->getVarcharMaxLength()) {
+						$column->setType(Type::getType('text'));
+						$column->setLength(null);
+					}
+				}
+			}
+		}
+
 		$filterExpression = $this->getFilterExpression();
 		$this->connection->getConfiguration()->
 		setFilterSchemaAssetsExpression($filterExpression);

lib/private/DB/OracleMigrator.php

@@ -23,6 +23,7 @@
 
 namespace OC\DB;
 
+use Doctrine\DBAL\Schema\ColumnDiff;
 use Doctrine\DBAL\Schema\Schema;
 
 class OracleMigrator extends NoCheckMigrator {
@@ -39,7 +40,12 @@ class OracleMigrator extends NoCheckMigrator {
 			$tableDiff->name = $this->connection->quoteIdentifier($tableDiff->name);
 			foreach ($tableDiff->changedColumns as $column) {
 				$column->oldColumnName = $this->connection->quoteIdentifier($column->oldColumnName);
+				// auto increment is not relevant for oracle and can anyhow not be applied on change
+				$column->changedProperties = array_diff($column->changedProperties, ['autoincrement', 'unsigned']);
 			}
+			$tableDiff->changedColumns = array_filter($tableDiff->changedColumns, function (ColumnDiff $column) {
+				return count($column->changedProperties) > 0;
+			});
 		}
 
 		return $schemaDiff;

lib/private/DB/PostgreSqlMigrator.php

@@ -0,0 +1,55 @@
+<?php
+/**
+ * @author Thomas Müller <thomas.mueller@tmit.eu>
+ *
+ * @copyright Copyright (c) 2016, ownCloud, Inc.
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace OC\DB;
+
+use Doctrine\DBAL\Schema\Schema;
+
+class PostgreSqlMigrator extends Migrator {
+	/**
+	 * @param Schema $targetSchema
+	 * @param \Doctrine\DBAL\Connection $connection
+	 * @return \Doctrine\DBAL\Schema\SchemaDiff
+	 */
+	protected function getDiff(Schema $targetSchema, \Doctrine\DBAL\Connection $connection) {
+		$schemaDiff = parent::getDiff($targetSchema, $connection);
+
+		foreach ($schemaDiff->changedTables as $tableDiff) {
+			// fix default value in brackets - pg 9.4 is returning a negative default value in ()
+			// see https://github.com/doctrine/dbal/issues/2427
+			foreach ($tableDiff->changedColumns as $column) {
+				$column->changedProperties = array_filter($column->changedProperties, function ($changedProperties) use ($column) {
+					if ($changedProperties !== 'default') {
+						return true;
+					}
+					$fromDefault = $column->fromColumn->getDefault();
+					$toDefault = $column->column->getDefault();
+					$fromDefault = trim($fromDefault, "()");
+
+					// by intention usage of !=
+					return $fromDefault != $toDefault;
+				});
+			}
+		}
+
+		return $schemaDiff;
+	}
+}

lib/private/DB/SQLiteMigrator.php

@@ -26,6 +26,8 @@ namespace OC\DB;
 
 use Doctrine\DBAL\DBALException;
 use Doctrine\DBAL\Schema\Schema;
+use Doctrine\DBAL\Types\BigIntType;
+use Doctrine\DBAL\Types\Type;
 
 class SQLiteMigrator extends Migrator {
 
@@ -76,6 +78,15 @@ class SQLiteMigrator extends Migrator {
 		$platform->registerDoctrineTypeMapping('smallint unsigned', 'integer');
 		$platform->registerDoctrineTypeMapping('varchar ', 'string');
 
+		// with sqlite autoincrement columns is of type integer
+		foreach ($targetSchema->getTables() as $table) {
+			foreach ($table->getColumns() as $column) {
+				if ($column->getType() instanceof BigIntType && $column->getAutoincrement()) {
+					$column->setType(Type::getType('integer'));
+				}
+			}
+		}
+
 		return parent::getDiff($targetSchema, $connection);
 	}
 }

tests/lib/DB/DBSchemaTest.php

@@ -8,15 +8,17 @@
 
 namespace Test\DB;
 
+use Doctrine\DBAL\Platforms\SqlitePlatform;
 use OC_DB;
 use OCP\Security\ISecureRandom;
+use Test\TestCase;
 
 /**
  * Class DBSchemaTest
  *
  * @group DB
  */
-class DBSchemaTest extends \Test\TestCase {
+class DBSchemaTest extends TestCase {
 	protected $schema_file = 'static://test_db_scheme';
 	protected $schema_file2 = 'static://test_db_scheme2';
 	protected $table1;
@@ -53,7 +55,6 @@ class DBSchemaTest extends \Test\TestCase {
 	 * @medium
 	 */
 	public function testSchema() {
-		$platform = \OC::$server->getDatabaseConnection()->getDatabasePlatform();
 		$this->doTestSchemaCreating();
 		$this->doTestSchemaChanging();
 		$this->doTestSchemaDumping();
@@ -97,7 +98,7 @@ class DBSchemaTest extends \Test\TestCase {
 	 */
 	public function assertTableNotExist($table) {
 		$platform = \OC::$server->getDatabaseConnection()->getDatabasePlatform();
-		if ($platform instanceof \Doctrine\DBAL\Platforms\SqlitePlatform) {
+		if ($platform instanceof SqlitePlatform) {
 			// sqlite removes the tables after closing the DB
 			$this->assertTrue(true);
 		} else {

tests/lib/DB/SchemaDiffTest.php

@@ -0,0 +1,99 @@
+<?php
+/**
+ * @author Thomas Müller <thomas.mueller@tmit.eu>
+ *
+ * @copyright Copyright (c) 2016, ownCloud, Inc.
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace Test\DB;
+
+use Doctrine\DBAL\Schema\SchemaDiff;
+use OC\DB\MDB2SchemaManager;
+use OC\DB\MDB2SchemaReader;
+use OCP\IConfig;
+use Test\TestCase;
+
+/**
+ * Class MigratorTest
+ *
+ * @group DB
+ *
+ * @package Test\DB
+ */
+class SchemaDiffTest extends TestCase {
+	/** @var \Doctrine\DBAL\Connection $connection */
+	private $connection;
+
+	/** @var MDB2SchemaManager */
+	private $manager;
+
+	/** @var IConfig */
+	private $config;
+
+	/** @var string */
+	private $testPrefix;
+
+	protected function setUp() {
+		parent::setUp();
+
+		$this->config = \OC::$server->getConfig();
+		$this->connection = \OC::$server->getDatabaseConnection();
+		$this->manager = new MDB2SchemaManager($this->connection);
+		$this->testPrefix= strtolower($this->getUniqueID($this->config->getSystemValue('dbtableprefix', 'oc_'), 3));
+	}
+
+	protected function tearDown() {
+		$this->manager->removeDBStructure('static://test_db_scheme');
+		parent::tearDown();
+	}
+
+	/**
+	 * @dataProvider providesSchemaFiles
+	 * @param string $xml
+	 */
+	public function testZeroChangeOnSchemaMigrations($xml) {
+
+		$xml = str_replace( '*dbprefix*', $this->testPrefix, $xml );
+		$schemaFile = 'static://test_db_scheme';
+		file_put_contents($schemaFile, $xml);
+
+		// apply schema
+		$this->manager->createDbFromStructure($schemaFile);
+
+		$schemaReader = new MDB2SchemaReader($this->config, $this->connection->getDatabasePlatform());
+		$endSchema = $schemaReader->loadSchemaFromFile($schemaFile);
+
+		// get the diff
+		/** @var SchemaDiff $diff */
+		$migrator = $this->manager->getMigrator();
+		$diff = $this->invokePrivate($migrator, 'getDiff', [$endSchema, $this->connection]);
+
+		// no sql statement is expected
+		$sqls = $diff->toSql($this->connection->getDatabasePlatform());
+		$this->assertEquals([], $sqls);
+	}
+
+	public function providesSchemaFiles() {
+		return [
+			'explicit test on autoincrement' => [file_get_contents(__DIR__ . '/schemDiffData/autoincrement.xml')],
+			'explicit test on clob' => [file_get_contents(__DIR__ . '/schemDiffData/clob.xml')],
+			'explicit test on unsigned' => [file_get_contents(__DIR__ . '/schemDiffData/unsigned.xml')],
+			'explicit test on default -1' => [file_get_contents(__DIR__ . '/schemDiffData/default-1.xml')],
+			'testing core schema' => [file_get_contents(__DIR__ . '/schemDiffData/core.xml')],
+		];
+	}
+}

tests/lib/DB/schemDiffData/autoincrement.xml

@@ -0,0 +1,16 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<database>
+	<table>
+		<name>*dbprefix*external_config</name>
+		<declaration>
+			<field>
+				<name>config_id</name>
+				<type>integer</type>
+				<default>0</default>
+				<notnull>true</notnull>
+				<autoincrement>1</autoincrement>
+				<length>6</length>
+			</field>
+		</declaration>
+	</table>
+</database>

tests/lib/DB/schemDiffData/clob.xml

@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<database>
+	<table>
+		<name>*dbprefix*external_config</name>
+		<declaration>
+			<field>
+				<name>value</name>
+				<type>text</type>
+				<notnull>true</notnull>
+				<length>100000</length>
+			</field>
+		</declaration>
+	</table>
+</database>

tests/lib/DB/schemDiffData/default-1.xml

@@ -0,0 +1,51 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<database>
+
+	<table>
+
+		<!--
+		Table for storing transactional file locking
+		-->
+		<name>*dbprefix*file_locks</name>
+
+		<declaration>
+
+			<field>
+				<name>id</name>
+				<type>integer</type>
+				<default>0</default>
+				<notnull>true</notnull>
+				<unsigned>true</unsigned>
+				<length>4</length>
+				<autoincrement>1</autoincrement>
+			</field>
+
+			<field>
+				<name>lock</name>
+				<type>integer</type>
+				<default>0</default>
+				<notnull>true</notnull>
+				<length>4</length>
+			</field>
+
+			<field>
+				<name>key</name>
+				<type>text</type>
+				<notnull>true</notnull>
+				<default>(stupid text)</default>
+				<length>64</length>
+			</field>
+
+			<field>
+				<name>ttl</name>
+				<type>integer</type>
+				<default>-1</default>
+				<notnull>true</notnull>
+				<length>4</length>
+			</field>
+
+		</declaration>
+
+	</table>
+
+</database>

tests/lib/DB/schemDiffData/unsigned.xml

@@ -0,0 +1,68 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<database>
+
+	<table>
+
+		<!--
+		Scheduled background jobs.
+		See OC\BackgroundJob\JobList.
+		-->
+		<name>*dbprefix*jobs</name>
+
+		<declaration>
+
+			<field>
+				<name>id</name>
+				<type>integer</type>
+				<default>0</default>
+				<notnull>true</notnull>
+				<autoincrement>1</autoincrement>
+				<unsigned>true</unsigned>
+				<length>4</length>
+			</field>
+
+			<field>
+				<name>class</name>
+				<type>text</type>
+				<default></default>
+				<notnull>true</notnull>
+				<length>255</length>
+			</field>
+
+			<field>
+				<name>argument</name>
+				<type>text</type>
+				<default></default>
+				<notnull>true</notnull>
+				<length>4000</length>
+			</field>
+
+			<field>
+				<!-- timestamp when the job was executed the last time -->
+				<name>last_run</name>
+				<type>integer</type>
+				<default></default>
+				<notnull>false</notnull>
+			</field>
+
+			<field>
+				<!-- timestamp when the job was checked if it needs execution the last time -->
+				<name>last_checked</name>
+				<type>integer</type>
+				<default></default>
+				<notnull>false</notnull>
+			</field>
+
+			<field>
+				<!-- timestamp when the job was reserved the last time, 1 day timeout -->
+				<name>reserved_at</name>
+				<type>integer</type>
+				<default></default>
+				<notnull>false</notnull>
+			</field>
+
+		</declaration>
+
+	</table>
+
+</database>