From d081a1a5adf007b383b0140c3ac469249d79878a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 20 Apr 2017 15:55:30 +0200 Subject: [PATCH 1/4] quote index columns on oracle, handle all index changes, minor phpdoc cleanup --- lib/private/DB/OracleMigrator.php | 112 +++++++++++++++++++++++------- 1 file changed, 85 insertions(+), 27 deletions(-) diff --git a/lib/private/DB/OracleMigrator.php b/lib/private/DB/OracleMigrator.php index 2735529b5e..b01ec78684 100644 --- a/lib/private/DB/OracleMigrator.php +++ b/lib/private/DB/OracleMigrator.php @@ -32,6 +32,55 @@ use Doctrine\DBAL\Schema\Schema; use Doctrine\DBAL\Schema\Table; class OracleMigrator extends NoCheckMigrator { + + /** + * Quote a column's name but changing the name requires recreating + * the column instance and copying over all properties. + * + * @param Column $column old column + * @return Column new column instance with new name + */ + protected function quoteColumn(Column $column) { + $newColumn = new Column( + $this->connection->quoteIdentifier($column->getName()), + $column->getType() + ); + $newColumn->setAutoincrement($column->getAutoincrement()); + $newColumn->setColumnDefinition($column->getColumnDefinition()); + $newColumn->setComment($column->getComment()); + $newColumn->setDefault($column->getDefault()); + $newColumn->setFixed($column->getFixed()); + $newColumn->setLength($column->getLength()); + $newColumn->setNotnull($column->getNotnull()); + $newColumn->setPrecision($column->getPrecision()); + $newColumn->setScale($column->getScale()); + $newColumn->setUnsigned($column->getUnsigned()); + $newColumn->setPlatformOptions($column->getPlatformOptions()); + $newColumn->setCustomSchemaOptions($column->getPlatformOptions()); + return $newColumn; + } + + /** + * Quote an index's name but changing the name requires recreating + * the index instance and copying over all properties. + * + * @param Index $index old index + * @return Index new index instance with new name + */ + protected function quoteIndex($index) { + return new Index( + //TODO migrate existing uppercase indexes, then $this->connection->quoteIdentifier($index->getName()), + $index->getName(), + array_map(function($columnName) { + return $this->connection->quoteIdentifier($columnName); + }, $index->getColumns()), + $index->isUnique(), + $index->isPrimary(), + $index->getFlags(), + $index->getOptions() + ); + } + /** * @param Schema $targetSchema * @param \Doctrine\DBAL\Connection $connection @@ -46,35 +95,10 @@ class OracleMigrator extends NoCheckMigrator { return new Table( $this->connection->quoteIdentifier($table->getName()), array_map(function(Column $column) { - $newColumn = new Column( - $this->connection->quoteIdentifier($column->getName()), - $column->getType() - ); - $newColumn->setAutoincrement($column->getAutoincrement()); - $newColumn->setColumnDefinition($column->getColumnDefinition()); - $newColumn->setComment($column->getComment()); - $newColumn->setDefault($column->getDefault()); - $newColumn->setFixed($column->getFixed()); - $newColumn->setLength($column->getLength()); - $newColumn->setNotnull($column->getNotnull()); - $newColumn->setPrecision($column->getPrecision()); - $newColumn->setScale($column->getScale()); - $newColumn->setUnsigned($column->getUnsigned()); - $newColumn->setPlatformOptions($column->getPlatformOptions()); - $newColumn->setCustomSchemaOptions($column->getPlatformOptions()); - return $newColumn; + return $this->quoteColumn($column); }, $table->getColumns()), array_map(function(Index $index) { - return new Index( - $this->connection->quoteIdentifier($index->getName()), - array_map(function($columnName) { - return $this->connection->quoteIdentifier($columnName); - }, $index->getColumns()), - $index->isUnique(), - $index->isPrimary(), - $index->getFlags(), - $index->getOptions() - ); + return $this->quoteIndex($index); }, $table->getIndexes()), $table->getForeignKeys(), 0, @@ -95,14 +119,48 @@ class OracleMigrator extends NoCheckMigrator { foreach ($schemaDiff->changedTables as $tableDiff) { $tableDiff->name = $this->connection->quoteIdentifier($tableDiff->name); + + $tableDiff->addedColumns = array_map(function(Column $column) { + return $this->quoteColumn($column); + }, $tableDiff->addedColumns); + 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']); } + // remove columns that no longer have changed (because autoincrement and unsigned are not supported) $tableDiff->changedColumns = array_filter($tableDiff->changedColumns, function (ColumnDiff $column) { return count($column->changedProperties) > 0; }); + + $tableDiff->removedColumns = array_map(function(Column $column) { + return $this->quoteColumn($column); + }, $tableDiff->removedColumns); + + $tableDiff->renamedColumns = array_map(function(Column $column) { + return $this->quoteColumn($column); + }, $tableDiff->renamedColumns); + + $tableDiff->addedIndexes = array_map(function(Index $index) { + return $this->quoteIndex($index); + }, $tableDiff->addedIndexes); + + $tableDiff->changedIndexes = array_map(function(Index $index) { + return $this->quoteIndex($index); + }, $tableDiff->changedIndexes); + + $tableDiff->removedIndexes = array_map(function(Index $index) { + return $this->quoteIndex($index); + }, $tableDiff->removedIndexes); + + $tableDiff->renamedIndexes = array_map(function(Index $index) { + return $this->quoteIndex($index); + }, $tableDiff->renamedIndexes); + + // TODO handle $tableDiff->addedForeignKeys + // TODO handle $tableDiff->changedForeignKeys + // TODO handle $tableDiff->removedForeignKeys } return $schemaDiff; From 6a51c1bc4fd0f2496103f901fef3b550688c2364 Mon Sep 17 00:00:00 2001 From: Piotr Mrowczynski Date: Mon, 2 Oct 2017 12:32:21 +0200 Subject: [PATCH 2/4] Add foreign key support to OC --- lib/private/DB/OracleMigrator.php | 40 +++++++++++++++++++++++++++---- tests/lib/DB/MigratorTest.php | 39 ++++++++++++++++++++++++++++-- 2 files changed, 73 insertions(+), 6 deletions(-) diff --git a/lib/private/DB/OracleMigrator.php b/lib/private/DB/OracleMigrator.php index b01ec78684..4541f9cb72 100644 --- a/lib/private/DB/OracleMigrator.php +++ b/lib/private/DB/OracleMigrator.php @@ -30,6 +30,7 @@ use Doctrine\DBAL\Schema\ColumnDiff; use Doctrine\DBAL\Schema\Index; use Doctrine\DBAL\Schema\Schema; use Doctrine\DBAL\Schema\Table; +use Doctrine\DBAL\Schema\ForeignKeyConstraint; class OracleMigrator extends NoCheckMigrator { @@ -81,6 +82,27 @@ class OracleMigrator extends NoCheckMigrator { ); } + /** + * Quote an ForeignKeyConstraint's name but changing the name requires recreating + * the ForeignKeyConstraint instance and copying over all properties. + * + * @param ForeignKeyConstraint $fkc old fkc + * @return ForeignKeyConstraint new fkc instance with new name + */ + protected function quoteForeignKeyConstraint($fkc) { + return new ForeignKeyConstraint( + array_map(function($columnName) { + return $this->connection->quoteIdentifier($columnName); + }, $fkc->getLocalColumns()), + $this->connection->quoteIdentifier($fkc->getForeignTableName()), + array_map(function($columnName) { + return $this->connection->quoteIdentifier($columnName); + }, $fkc->getForeignColumns()), + $fkc->getName(), + $fkc->getOptions() + ); + } + /** * @param Schema $targetSchema * @param \Doctrine\DBAL\Connection $connection @@ -100,7 +122,9 @@ class OracleMigrator extends NoCheckMigrator { array_map(function(Index $index) { return $this->quoteIndex($index); }, $table->getIndexes()), - $table->getForeignKeys(), + array_map(function(ForeignKeyConstraint $fck) { + return $this->quoteForeignKeyConstraint($fck); + }, $table->getForeignKeys()), 0, $table->getOptions() ); @@ -158,9 +182,17 @@ class OracleMigrator extends NoCheckMigrator { return $this->quoteIndex($index); }, $tableDiff->renamedIndexes); - // TODO handle $tableDiff->addedForeignKeys - // TODO handle $tableDiff->changedForeignKeys - // TODO handle $tableDiff->removedForeignKeys + $tableDiff->addedForeignKeys = array_map(function(ForeignKeyConstraint $fkc) { + return $this->quoteForeignKeyConstraint($fkc); + }, $tableDiff->addedForeignKeys); + + $tableDiff->changedForeignKeys = array_map(function(ForeignKeyConstraint $fkc) { + return $this->quoteForeignKeyConstraint($fkc); + }, $tableDiff->changedForeignKeys); + + $tableDiff->removedForeignKeys = array_map(function(ForeignKeyConstraint $fkc) { + return $this->quoteForeignKeyConstraint($fkc); + }, $tableDiff->removedForeignKeys); } return $schemaDiff; diff --git a/tests/lib/DB/MigratorTest.php b/tests/lib/DB/MigratorTest.php index e4f45c4bb8..ea718240c5 100644 --- a/tests/lib/DB/MigratorTest.php +++ b/tests/lib/DB/MigratorTest.php @@ -41,6 +41,9 @@ class MigratorTest extends \Test\TestCase { /** @var string */ private $tableName; + /** @var string */ + private $tableNameTmp; + protected function setUp() { parent::setUp(); @@ -50,11 +53,23 @@ class MigratorTest extends \Test\TestCase { $this->markTestSkipped('DB migration tests are not supported on OCI'); } $this->manager = new \OC\DB\MDB2SchemaManager($this->connection); - $this->tableName = strtolower($this->getUniqueID($this->config->getSystemValue('dbtableprefix', 'oc_') . 'test_')); + $this->tableName = $this->getUniqueTableName(); + $this->tableNameTmp = $this->getUniqueTableName(); + } + + private function getUniqueTableName() { + return strtolower($this->getUniqueID($this->config->getSystemValue('dbtableprefix', 'oc_') . 'test_')); } protected function tearDown() { - $this->connection->exec('DROP TABLE ' . $this->tableName); + // Try to delete if exists (IF EXISTS NOT SUPPORTED IN ORACLE) + try { + $this->connection->exec('DROP TABLE ' . $this->connection->quoteIdentifier($this->tableNameTmp)); + } catch (\Doctrine\DBAL\DBALException $e) {} + + try { + $this->connection->exec('DROP TABLE ' . $this->connection->quoteIdentifier($this->tableName)); + } catch (\Doctrine\DBAL\DBALException $e) {} parent::tearDown(); } @@ -200,4 +215,24 @@ class MigratorTest extends \Test\TestCase { $this->assertTrue(true); } + + public function testAddingForeignKey() { + $startSchema = new Schema([], [], $this->getSchemaConfig()); + $table = $startSchema->createTable($this->tableName); + $table->addColumn('id', 'integer', ['autoincrement' => true]); + $table->addColumn('name', 'string'); + $table->setPrimaryKey(['id']); + + $fkName = "fkc"; + $tableFk = $startSchema->createTable($this->tableNameTmp); + $tableFk->addColumn('fk_id', 'integer'); + $tableFk->addColumn('name', 'string'); + $tableFk->addForeignKeyConstraint($this->tableName, array('fk_id'), array('id'), array(), $fkName); + + $migrator = $this->manager->getMigrator(); + $migrator->migrate($startSchema); + + + $this->assertTrue($startSchema->getTable($this->tableNameTmp)->hasForeignKey($fkName)); + } } From 9f16468789e3e9a15ba1e42ced605a1ceb7b37d8 Mon Sep 17 00:00:00 2001 From: Victor Dubiniuk Date: Mon, 16 Oct 2017 23:43:24 +0300 Subject: [PATCH 3/4] Die NoCheckMigrator --- lib/private/DB/MDB2SchemaManager.php | 2 +- lib/private/DB/NoCheckMigrator.php | 39 ---------------------------- lib/private/DB/OracleMigrator.php | 2 +- 3 files changed, 2 insertions(+), 41 deletions(-) delete mode 100644 lib/private/DB/NoCheckMigrator.php diff --git a/lib/private/DB/MDB2SchemaManager.php b/lib/private/DB/MDB2SchemaManager.php index 89b0d15321..ad3f93a264 100644 --- a/lib/private/DB/MDB2SchemaManager.php +++ b/lib/private/DB/MDB2SchemaManager.php @@ -89,7 +89,7 @@ class MDB2SchemaManager { } else if ($platform instanceof PostgreSqlPlatform) { return new PostgreSqlMigrator($this->conn, $random, $config, $dispatcher); } else { - return new NoCheckMigrator($this->conn, $random, $config, $dispatcher); + return new Migrator($this->conn, $random, $config, $dispatcher); } } diff --git a/lib/private/DB/NoCheckMigrator.php b/lib/private/DB/NoCheckMigrator.php deleted file mode 100644 index 723653511b..0000000000 --- a/lib/private/DB/NoCheckMigrator.php +++ /dev/null @@ -1,39 +0,0 @@ - - * @author Robin Appelman - * - * @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 - * - */ - -namespace OC\DB; - -use Doctrine\DBAL\Schema\Schema; - -/** - * migrator for database platforms that don't support the upgrade check - * - * @package OC\DB - */ -class NoCheckMigrator extends Migrator { - /** - * @param \Doctrine\DBAL\Schema\Schema $targetSchema - * @throws \OC\DB\MigrationException - */ - public function checkMigrate(Schema $targetSchema) {} -} diff --git a/lib/private/DB/OracleMigrator.php b/lib/private/DB/OracleMigrator.php index 4541f9cb72..f5e06b50d9 100644 --- a/lib/private/DB/OracleMigrator.php +++ b/lib/private/DB/OracleMigrator.php @@ -32,7 +32,7 @@ use Doctrine\DBAL\Schema\Schema; use Doctrine\DBAL\Schema\Table; use Doctrine\DBAL\Schema\ForeignKeyConstraint; -class OracleMigrator extends NoCheckMigrator { +class OracleMigrator extends Migrator { /** * Quote a column's name but changing the name requires recreating From 218e2357c84e37db33510224ab6e91534841eba5 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 30 Oct 2017 18:59:52 +0100 Subject: [PATCH 4/4] Rebuild autoloader Signed-off-by: Joas Schilling --- lib/composer/composer/autoload_classmap.php | 1 - lib/composer/composer/autoload_static.php | 1 - 2 files changed, 2 deletions(-) diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 709ac7c50e..5c55c83b05 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -529,7 +529,6 @@ return array( 'OC\\DB\\Migrator' => $baseDir . '/lib/private/DB/Migrator.php', 'OC\\DB\\MySQLMigrator' => $baseDir . '/lib/private/DB/MySQLMigrator.php', 'OC\\DB\\MySqlTools' => $baseDir . '/lib/private/DB/MySqlTools.php', - 'OC\\DB\\NoCheckMigrator' => $baseDir . '/lib/private/DB/NoCheckMigrator.php', 'OC\\DB\\OCPostgreSqlPlatform' => $baseDir . '/lib/private/DB/OCPostgreSqlPlatform.php', 'OC\\DB\\OCSqlitePlatform' => $baseDir . '/lib/private/DB/OCSqlitePlatform.php', 'OC\\DB\\OracleConnection' => $baseDir . '/lib/private/DB/OracleConnection.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 7ba734aac8..23fd2d4280 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -559,7 +559,6 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\DB\\Migrator' => __DIR__ . '/../../..' . '/lib/private/DB/Migrator.php', 'OC\\DB\\MySQLMigrator' => __DIR__ . '/../../..' . '/lib/private/DB/MySQLMigrator.php', 'OC\\DB\\MySqlTools' => __DIR__ . '/../../..' . '/lib/private/DB/MySqlTools.php', - 'OC\\DB\\NoCheckMigrator' => __DIR__ . '/../../..' . '/lib/private/DB/NoCheckMigrator.php', 'OC\\DB\\OCPostgreSqlPlatform' => __DIR__ . '/../../..' . '/lib/private/DB/OCPostgreSqlPlatform.php', 'OC\\DB\\OCSqlitePlatform' => __DIR__ . '/../../..' . '/lib/private/DB/OCSqlitePlatform.php', 'OC\\DB\\OracleConnection' => __DIR__ . '/../../..' . '/lib/private/DB/OracleConnection.php',