From f265657bc676272476f814d66c783560f139db02 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 6 Aug 2018 18:25:09 +0200 Subject: [PATCH 1/3] Only check changed items Signed-off-by: Joas Schilling --- lib/private/DB/MigrationService.php | 31 +++++++++++++++++------------ 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/lib/private/DB/MigrationService.php b/lib/private/DB/MigrationService.php index f584cb351d..62072accbc 100644 --- a/lib/private/DB/MigrationService.php +++ b/lib/private/DB/MigrationService.php @@ -25,11 +25,11 @@ namespace OC\DB; use Doctrine\DBAL\Platforms\OraclePlatform; use Doctrine\DBAL\Platforms\PostgreSqlPlatform; -use Doctrine\DBAL\Schema\Column; use Doctrine\DBAL\Schema\Index; use Doctrine\DBAL\Schema\Schema; use Doctrine\DBAL\Schema\SchemaException; use Doctrine\DBAL\Schema\Sequence; +use Doctrine\DBAL\Schema\Table; use OC\IntegrityCheck\Helpers\AppLocator; use OC\Migration\SimpleOutput; use OCP\AppFramework\App; @@ -456,9 +456,9 @@ class MigrationService { }, ['tablePrefix' => $this->connection->getPrefix()]); if ($toSchema instanceof SchemaWrapper) { + $sourceSchema = $this->connection->createSchema(); $targetSchema = $toSchema->getWrappedSchema(); - // TODO re-enable once stable14 is branched of: https://github.com/nextcloud/server/issues/10518 - // $this->ensureOracleIdentifierLengthLimit($targetSchema, strlen($this->connection->getPrefix())); + $this->ensureOracleIdentifierLengthLimit($sourceSchema, $targetSchema, strlen($this->connection->getPrefix())); $this->connection->migrateToSchema($targetSchema); $toSchema->performDropTableCalls(); } @@ -472,34 +472,39 @@ class MigrationService { $this->markAsExecuted($version); } - public function ensureOracleIdentifierLengthLimit(Schema $schema, int $prefixLength) { - $sequences = $schema->getSequences(); + public function ensureOracleIdentifierLengthLimit(Schema $sourceSchema, Schema $targetSchema, int $prefixLength) { + $sequences = $targetSchema->getSequences(); - foreach ($schema->getTables() as $table) { - if (\strlen($table->getName()) - $prefixLength > 27) { - throw new \InvalidArgumentException('Table name "' . $table->getName() . '" is too long.'); + foreach ($targetSchema->getTables() as $table) { + try { + $sourceTable = $sourceSchema->getTable($table->getName()); + } catch (SchemaException $e) { + if (\strlen($table->getName()) - $prefixLength > 27) { + throw new \InvalidArgumentException('Table name "' . $table->getName() . '" is too long.'); + } + $sourceTable = null; } foreach ($table->getColumns() as $thing) { - if (\strlen($thing->getName()) - $prefixLength > 27) { + if ((!$sourceTable instanceof Table || !$sourceTable->hasColumn($thing->getName())) && \strlen($thing->getName()) - $prefixLength > 27) { throw new \InvalidArgumentException('Column name "' . $table->getName() . '"."' . $thing->getName() . '" is too long.'); } } foreach ($table->getIndexes() as $thing) { - if (\strlen($thing->getName()) - $prefixLength > 27) { + if ((!$sourceTable instanceof Table || !$sourceTable->hasIndex($thing->getName())) && \strlen($thing->getName()) - $prefixLength > 27) { throw new \InvalidArgumentException('Index name "' . $table->getName() . '"."' . $thing->getName() . '" is too long.'); } } foreach ($table->getForeignKeys() as $thing) { - if (\strlen($thing->getName()) - $prefixLength > 27) { + if ((!$sourceTable instanceof Table || !$sourceTable->hasForeignKey($thing->getName())) && \strlen($thing->getName()) - $prefixLength > 27) { throw new \InvalidArgumentException('Foreign key name "' . $table->getName() . '"."' . $thing->getName() . '" is too long.'); } } $primaryKey = $table->getPrimaryKey(); - if ($primaryKey instanceof Index) { + if ($primaryKey instanceof Index && (!$sourceTable instanceof Table || !$sourceTable->hasPrimaryKey())) { $indexName = strtolower($primaryKey->getName()); $isUsingDefaultName = $indexName === 'primary'; @@ -528,7 +533,7 @@ class MigrationService { } foreach ($sequences as $sequence) { - if (\strlen($sequence->getName()) - $prefixLength > 27) { + if (!$sourceSchema->hasSequence($sequence->getName()) && \strlen($sequence->getName()) - $prefixLength > 27) { throw new \InvalidArgumentException('Sequence name "' . $sequence->getName() . '" is too long.'); } } From c09fa1ee65dc944a8e9c689a441da9c7a440dd02 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 6 Aug 2018 18:36:38 +0200 Subject: [PATCH 2/3] Only check the Oracle schema conditions if the app supports it Signed-off-by: Joas Schilling --- lib/private/DB/MigrationService.php | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/lib/private/DB/MigrationService.php b/lib/private/DB/MigrationService.php index 62072accbc..97f1dd269d 100644 --- a/lib/private/DB/MigrationService.php +++ b/lib/private/DB/MigrationService.php @@ -30,6 +30,7 @@ use Doctrine\DBAL\Schema\Schema; use Doctrine\DBAL\Schema\SchemaException; use Doctrine\DBAL\Schema\Sequence; use Doctrine\DBAL\Schema\Table; +use OC\App\InfoParser; use OC\IntegrityCheck\Helpers\AppLocator; use OC\Migration\SimpleOutput; use OCP\AppFramework\App; @@ -51,6 +52,8 @@ class MigrationService { private $connection; /** @var string */ private $appName; + /** @var bool */ + private $checkOracle; /** * MigrationService constructor. @@ -72,6 +75,7 @@ class MigrationService { if ($appName === 'core') { $this->migrationsPath = \OC::$SERVERROOT . '/core/Migrations'; $this->migrationsNamespace = 'OC\\Core\\Migrations'; + $this->checkOracle = true; } else { if (null === $appLocator) { $appLocator = new AppLocator(); @@ -80,6 +84,21 @@ class MigrationService { $namespace = App::buildAppNamespace($appName); $this->migrationsPath = "$appPath/lib/Migration"; $this->migrationsNamespace = $namespace . '\\Migration'; + + $infoParser = new InfoParser(); + $info = $infoParser->parse($appPath . '/appinfo/info.xml'); + if (!isset($info['dependencies']['database'])) { + $this->checkOracle = true; + } else { + $this->checkOracle = false; + foreach ($info['dependencies']['database'] as $database) { + if (\is_string($database) && $database === 'oci') { + $this->checkOracle = true; + } else if (\is_array($database) && isset($database['@value']) && $database['@value'] === 'oci') { + $this->checkOracle = true; + } + } + } } } @@ -456,9 +475,11 @@ class MigrationService { }, ['tablePrefix' => $this->connection->getPrefix()]); if ($toSchema instanceof SchemaWrapper) { - $sourceSchema = $this->connection->createSchema(); $targetSchema = $toSchema->getWrappedSchema(); - $this->ensureOracleIdentifierLengthLimit($sourceSchema, $targetSchema, strlen($this->connection->getPrefix())); + if ($this->checkOracle) { + $sourceSchema = $this->connection->createSchema(); + $this->ensureOracleIdentifierLengthLimit($sourceSchema, $targetSchema, strlen($this->connection->getPrefix())); + } $this->connection->migrateToSchema($targetSchema); $toSchema->performDropTableCalls(); } From 85a0e10b4f3caec73de26e26e71bd6895358c916 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 10 Oct 2018 10:45:10 +0200 Subject: [PATCH 3/3] Update the tests to the comparison logic Signed-off-by: Joas Schilling --- tests/lib/DB/MigrationsTest.php | 110 +++++++++++++++++++++++++++----- 1 file changed, 95 insertions(+), 15 deletions(-) diff --git a/tests/lib/DB/MigrationsTest.php b/tests/lib/DB/MigrationsTest.php index 7e20119108..8654c83a54 100644 --- a/tests/lib/DB/MigrationsTest.php +++ b/tests/lib/DB/MigrationsTest.php @@ -16,6 +16,7 @@ use Doctrine\DBAL\Schema\Column; use Doctrine\DBAL\Schema\ForeignKeyConstraint; use Doctrine\DBAL\Schema\Index; use Doctrine\DBAL\Schema\Schema; +use Doctrine\DBAL\Schema\SchemaException; use Doctrine\DBAL\Schema\Sequence; use Doctrine\DBAL\Schema\Table; use OC\DB\Connection; @@ -102,13 +103,12 @@ class MigrationsTest extends \Test\TestCase { ->method('migrateToSchema'); $wrappedSchema = $this->createMock(Schema::class); - // TODO re-enable once stable14 is branched of: https://github.com/nextcloud/server/issues/10518 - /*$wrappedSchema->expects($this->once()) + $wrappedSchema->expects($this->once()) ->method('getTables') ->willReturn([]); $wrappedSchema->expects($this->once()) ->method('getSequences') - ->willReturn([]);*/ + ->willReturn([]); $schemaResult = $this->createMock(SchemaWrapper::class); $schemaResult->expects($this->once()) @@ -239,12 +239,12 @@ class MigrationsTest extends \Test\TestCase { ->willReturn(\str_repeat('a', 30)); $table = $this->createMock(Table::class); - $table->expects($this->once()) + $table->expects($this->atLeastOnce()) ->method('getName') ->willReturn(\str_repeat('a', 30)); $sequence = $this->createMock(Sequence::class); - $sequence->expects($this->once()) + $sequence->expects($this->atLeastOnce()) ->method('getName') ->willReturn(\str_repeat('a', 30)); @@ -269,7 +269,15 @@ class MigrationsTest extends \Test\TestCase { ->method('getSequences') ->willReturn([$sequence]); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$schema, 3]); + $sourceSchema = $this->createMock(Schema::class); + $sourceSchema->expects($this->any()) + ->method('getTable') + ->willThrowException(new SchemaException()); + $sourceSchema->expects($this->any()) + ->method('hasSequence') + ->willReturn(false); + + self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); } public function testEnsureOracleIdentifierLengthLimitValidWithPrimaryKey() { @@ -304,7 +312,15 @@ class MigrationsTest extends \Test\TestCase { ->method('getSequences') ->willReturn([]); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$schema, 3]); + $sourceSchema = $this->createMock(Schema::class); + $sourceSchema->expects($this->any()) + ->method('getTable') + ->willThrowException(new SchemaException()); + $sourceSchema->expects($this->any()) + ->method('hasSequence') + ->willReturn(false); + + self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); } public function testEnsureOracleIdentifierLengthLimitValidWithPrimaryKeyDefault() { @@ -349,7 +365,15 @@ class MigrationsTest extends \Test\TestCase { ->method('getSequences') ->willReturn([]); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$schema, 3]); + $sourceSchema = $this->createMock(Schema::class); + $sourceSchema->expects($this->any()) + ->method('getTable') + ->willThrowException(new SchemaException()); + $sourceSchema->expects($this->any()) + ->method('hasSequence') + ->willReturn(false); + + self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); } /** @@ -366,7 +390,15 @@ class MigrationsTest extends \Test\TestCase { ->method('getTables') ->willReturn([$table]); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$schema, 3]); + $sourceSchema = $this->createMock(Schema::class); + $sourceSchema->expects($this->any()) + ->method('getTable') + ->willThrowException(new SchemaException()); + $sourceSchema->expects($this->any()) + ->method('hasSequence') + ->willReturn(false); + + self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); } /** @@ -411,7 +443,15 @@ class MigrationsTest extends \Test\TestCase { ->method('getTables') ->willReturn([$table]); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$schema, 3]); + $sourceSchema = $this->createMock(Schema::class); + $sourceSchema->expects($this->any()) + ->method('getTable') + ->willThrowException(new SchemaException()); + $sourceSchema->expects($this->any()) + ->method('hasSequence') + ->willReturn(false); + + self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); } /** @@ -446,7 +486,15 @@ class MigrationsTest extends \Test\TestCase { ->method('getTables') ->willReturn([$table]); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$schema, 3]); + $sourceSchema = $this->createMock(Schema::class); + $sourceSchema->expects($this->any()) + ->method('getTable') + ->willThrowException(new SchemaException()); + $sourceSchema->expects($this->any()) + ->method('hasSequence') + ->willReturn(false); + + self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); } /** @@ -472,7 +520,15 @@ class MigrationsTest extends \Test\TestCase { ->method('getTables') ->willReturn([$table]); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$schema, 3]); + $sourceSchema = $this->createMock(Schema::class); + $sourceSchema->expects($this->any()) + ->method('getTable') + ->willThrowException(new SchemaException()); + $sourceSchema->expects($this->any()) + ->method('hasSequence') + ->willReturn(false); + + self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); } /** @@ -501,7 +557,15 @@ class MigrationsTest extends \Test\TestCase { ->method('getTables') ->willReturn([$table]); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$schema, 3]); + $sourceSchema = $this->createMock(Schema::class); + $sourceSchema->expects($this->any()) + ->method('getTable') + ->willThrowException(new SchemaException()); + $sourceSchema->expects($this->any()) + ->method('hasSequence') + ->willReturn(false); + + self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); } /** @@ -533,7 +597,15 @@ class MigrationsTest extends \Test\TestCase { ->method('getTables') ->willReturn([$table]); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$schema, 3]); + $sourceSchema = $this->createMock(Schema::class); + $sourceSchema->expects($this->any()) + ->method('getTable') + ->willThrowException(new SchemaException()); + $sourceSchema->expects($this->any()) + ->method('hasSequence') + ->willReturn(false); + + self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); } /** @@ -553,6 +625,14 @@ class MigrationsTest extends \Test\TestCase { ->method('getSequences') ->willReturn([$sequence]); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$schema, 3]); + $sourceSchema = $this->createMock(Schema::class); + $sourceSchema->expects($this->any()) + ->method('getTable') + ->willThrowException(new SchemaException()); + $sourceSchema->expects($this->any()) + ->method('hasSequence') + ->willReturn(false); + + self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); } }