From c98cab137cf6a4c2aee6af4f125537083e0b7a6f Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 11 Nov 2020 14:33:29 +0100 Subject: [PATCH 1/4] Fix exception messages spacing Signed-off-by: Joas Schilling --- lib/private/DB/MigrationService.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/private/DB/MigrationService.php b/lib/private/DB/MigrationService.php index 42f3b7de6a83e..30fef6d7ef7ab 100644 --- a/lib/private/DB/MigrationService.php +++ b/lib/private/DB/MigrationService.php @@ -571,7 +571,7 @@ public function ensureOracleIdentifierLengthLimit(Schema $sourceSchema, Schema $ if ($thing->getNotnull() && $thing->getDefault() === '' && $sourceTable instanceof Table && !$sourceTable->hasColumn($thing->getName())) { - throw new \InvalidArgumentException('Column name "' . $table->getName() . '"."' . $thing->getName() . '" is NotNull, but has empty string or null as default.'); + throw new \InvalidArgumentException('Column "' . $table->getName() . '"."' . $thing->getName() . '" is NotNull, but has empty string or null as default.'); } } @@ -583,7 +583,7 @@ public function ensureOracleIdentifierLengthLimit(Schema $sourceSchema, Schema $ foreach ($table->getForeignKeys() as $thing) { if ((!$sourceTable instanceof Table || !$sourceTable->hasForeignKey($thing->getName())) && \strlen($thing->getName()) > 30) { - throw new \InvalidArgumentException('Foreign key name "' . $table->getName() . '"."' . $thing->getName() . '" is too long.'); + throw new \InvalidArgumentException('Foreign key name "' . $table->getName() . '"."' . $thing->getName() . '" is too long.'); } } From 133a6f4fe4f1ec1e80011301607264beab12c852 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 11 Nov 2020 14:33:47 +0100 Subject: [PATCH 2/4] Document the constraints we test against Signed-off-by: Joas Schilling --- lib/private/DB/MigrationService.php | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/lib/private/DB/MigrationService.php b/lib/private/DB/MigrationService.php index 30fef6d7ef7ab..852ee8b701f0e 100644 --- a/lib/private/DB/MigrationService.php +++ b/lib/private/DB/MigrationService.php @@ -551,6 +551,23 @@ public function executeStep($version, $schemaOnly = false) { $this->markAsExecuted($version); } + /** + * Naming constraints: + * - Tables names must be 30 chars or shorter (27 + oc_ prefix) + * - Column names must be 30 chars or shorter + * - Index names must be 30 chars or shorter + * - Sequence names must be 30 chars or shorter + * - Primary key names must be set or the table name 23 chars or shorter + * + * Data constraints: + * - Columns with "NotNull" can not have empty string as default value + * - Columns with "NotNull" can not have number 0 as default value + * + * @param Schema $sourceSchema + * @param Schema $targetSchema + * @param int $prefixLength + * @throws \Doctrine\DBAL\Exception + */ public function ensureOracleIdentifierLengthLimit(Schema $sourceSchema, Schema $targetSchema, int $prefixLength) { $sequences = $targetSchema->getSequences(); From 3696ef5b965e5d3e44197479eaf310e26288151c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 11 Nov 2020 14:34:24 +0100 Subject: [PATCH 3/4] Don't allow Notnull for boolean columns Signed-off-by: Joas Schilling --- lib/private/DB/MigrationService.php | 5 ++++ tests/lib/DB/MigrationsTest.php | 41 +++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/lib/private/DB/MigrationService.php b/lib/private/DB/MigrationService.php index 852ee8b701f0e..4957706bb1d95 100644 --- a/lib/private/DB/MigrationService.php +++ b/lib/private/DB/MigrationService.php @@ -562,6 +562,7 @@ public function executeStep($version, $schemaOnly = false) { * Data constraints: * - Columns with "NotNull" can not have empty string as default value * - Columns with "NotNull" can not have number 0 as default value + * - Columns with type "bool" (which is in fact integer of length 1) can not be "NotNull" as it can not store 0/false * * @param Schema $sourceSchema * @param Schema $targetSchema @@ -590,6 +591,10 @@ public function ensureOracleIdentifierLengthLimit(Schema $sourceSchema, Schema $ && $sourceTable instanceof Table && !$sourceTable->hasColumn($thing->getName())) { throw new \InvalidArgumentException('Column "' . $table->getName() . '"."' . $thing->getName() . '" is NotNull, but has empty string or null as default.'); } + + if ($thing->getNotnull() && $thing->getType()->getName() === Types::BOOLEAN) { + throw new \InvalidArgumentException('Column "' . $table->getName() . '"."' . $thing->getName() . '" is type Bool and also NotNull, so it can not store "false".'); + } } foreach ($table->getIndexes() as $thing) { diff --git a/tests/lib/DB/MigrationsTest.php b/tests/lib/DB/MigrationsTest.php index 9b6dec006d69a..c8b1af3e08e0a 100644 --- a/tests/lib/DB/MigrationsTest.php +++ b/tests/lib/DB/MigrationsTest.php @@ -18,6 +18,7 @@ use Doctrine\DBAL\Schema\SchemaException; use Doctrine\DBAL\Schema\Sequence; use Doctrine\DBAL\Schema\Table; +use Doctrine\DBAL\Types\Type; use OC\DB\Connection; use OC\DB\MigrationService; use OC\DB\SchemaWrapper; @@ -632,4 +633,44 @@ public function testEnsureOracleIdentifierLengthLimitTooLongSequenceName() { self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); } + + + public function testEnsureOracleIdentifierLengthLimitBooleanNotNull() { + $this->expectException(\InvalidArgumentException::class); + + $column = $this->createMock(Column::class); + $column->expects($this->any()) + ->method('getName') + ->willReturn('aaaa'); + $column->expects($this->any()) + ->method('getType') + ->willReturn(Type::getType('boolean')); + $column->expects($this->any()) + ->method('getNotnull') + ->willReturn(true); + + $table = $this->createMock(Table::class); + $table->expects($this->any()) + ->method('getName') + ->willReturn(\str_repeat('a', 30)); + + $table->expects($this->once()) + ->method('getColumns') + ->willReturn([$column]); + + $schema = $this->createMock(Schema::class); + $schema->expects($this->once()) + ->method('getTables') + ->willReturn([$table]); + + $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]); + } } From f9d4fa2d38d6693d87b62ad448f38441b1876e9f Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 11 Nov 2020 14:40:26 +0100 Subject: [PATCH 4/4] Rename the method to match what it does Signed-off-by: Joas Schilling --- lib/private/DB/MigrationService.php | 6 ++-- tests/lib/DB/MigrationsTest.php | 44 ++++++++++++++--------------- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/lib/private/DB/MigrationService.php b/lib/private/DB/MigrationService.php index 4957706bb1d95..ee64b45be6444 100644 --- a/lib/private/DB/MigrationService.php +++ b/lib/private/DB/MigrationService.php @@ -459,7 +459,7 @@ public function migrateSchemaOnly($to = 'latest') { $targetSchema = $toSchema->getWrappedSchema(); if ($this->checkOracle) { $beforeSchema = $this->connection->createSchema(); - $this->ensureOracleIdentifierLengthLimit($beforeSchema, $targetSchema, strlen($this->connection->getPrefix())); + $this->ensureOracleConstraints($beforeSchema, $targetSchema, strlen($this->connection->getPrefix())); } $this->connection->migrateToSchema($targetSchema); $toSchema->performDropTableCalls(); @@ -536,7 +536,7 @@ public function executeStep($version, $schemaOnly = false) { $targetSchema = $toSchema->getWrappedSchema(); if ($this->checkOracle) { $sourceSchema = $this->connection->createSchema(); - $this->ensureOracleIdentifierLengthLimit($sourceSchema, $targetSchema, strlen($this->connection->getPrefix())); + $this->ensureOracleConstraints($sourceSchema, $targetSchema, strlen($this->connection->getPrefix())); } $this->connection->migrateToSchema($targetSchema); $toSchema->performDropTableCalls(); @@ -569,7 +569,7 @@ public function executeStep($version, $schemaOnly = false) { * @param int $prefixLength * @throws \Doctrine\DBAL\Exception */ - public function ensureOracleIdentifierLengthLimit(Schema $sourceSchema, Schema $targetSchema, int $prefixLength) { + public function ensureOracleConstraints(Schema $sourceSchema, Schema $targetSchema, int $prefixLength) { $sequences = $targetSchema->getSequences(); foreach ($targetSchema->getTables() as $table) { diff --git a/tests/lib/DB/MigrationsTest.php b/tests/lib/DB/MigrationsTest.php index c8b1af3e08e0a..507f3d0e228f9 100644 --- a/tests/lib/DB/MigrationsTest.php +++ b/tests/lib/DB/MigrationsTest.php @@ -220,7 +220,7 @@ public function testMigrate() { $this->migrationService->migrate(); } - public function testEnsureOracleIdentifierLengthLimitValid() { + public function testEnsureOracleConstraintsValid() { $column = $this->createMock(Column::class); $column->expects($this->once()) ->method('getName') @@ -275,10 +275,10 @@ public function testEnsureOracleIdentifierLengthLimitValid() { ->method('hasSequence') ->willReturn(false); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); + self::invokePrivate($this->migrationService, 'ensureOracleConstraints', [$sourceSchema, $schema, 3]); } - public function testEnsureOracleIdentifierLengthLimitValidWithPrimaryKey() { + public function testEnsureOracleConstraintsValidWithPrimaryKey() { $index = $this->createMock(Index::class); $index->expects($this->any()) ->method('getName') @@ -318,10 +318,10 @@ public function testEnsureOracleIdentifierLengthLimitValidWithPrimaryKey() { ->method('hasSequence') ->willReturn(false); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); + self::invokePrivate($this->migrationService, 'ensureOracleConstraints', [$sourceSchema, $schema, 3]); } - public function testEnsureOracleIdentifierLengthLimitValidWithPrimaryKeyDefault() { + public function testEnsureOracleConstraintsValidWithPrimaryKeyDefault() { $defaultName = 'PRIMARY'; if ($this->db->getDatabasePlatform() instanceof PostgreSqlPlatform) { $defaultName = \str_repeat('a', 26) . '_' . \str_repeat('b', 30) . '_seq'; @@ -371,11 +371,11 @@ public function testEnsureOracleIdentifierLengthLimitValidWithPrimaryKeyDefault( ->method('hasSequence') ->willReturn(false); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); + self::invokePrivate($this->migrationService, 'ensureOracleConstraints', [$sourceSchema, $schema, 3]); } - public function testEnsureOracleIdentifierLengthLimitTooLongTableName() { + public function testEnsureOracleConstraintsTooLongTableName() { $this->expectException(\InvalidArgumentException::class); $table = $this->createMock(Table::class); @@ -396,11 +396,11 @@ public function testEnsureOracleIdentifierLengthLimitTooLongTableName() { ->method('hasSequence') ->willReturn(false); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); + self::invokePrivate($this->migrationService, 'ensureOracleConstraints', [$sourceSchema, $schema, 3]); } - public function testEnsureOracleIdentifierLengthLimitTooLongPrimaryWithDefault() { + public function testEnsureOracleConstraintsTooLongPrimaryWithDefault() { $this->expectException(\InvalidArgumentException::class); $defaultName = 'PRIMARY'; @@ -449,11 +449,11 @@ public function testEnsureOracleIdentifierLengthLimitTooLongPrimaryWithDefault() ->method('hasSequence') ->willReturn(false); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); + self::invokePrivate($this->migrationService, 'ensureOracleConstraints', [$sourceSchema, $schema, 3]); } - public function testEnsureOracleIdentifierLengthLimitTooLongPrimaryWithName() { + public function testEnsureOracleConstraintsTooLongPrimaryWithName() { $this->expectException(\InvalidArgumentException::class); $index = $this->createMock(Index::class); @@ -492,11 +492,11 @@ public function testEnsureOracleIdentifierLengthLimitTooLongPrimaryWithName() { ->method('hasSequence') ->willReturn(false); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); + self::invokePrivate($this->migrationService, 'ensureOracleConstraints', [$sourceSchema, $schema, 3]); } - public function testEnsureOracleIdentifierLengthLimitTooLongColumnName() { + public function testEnsureOracleConstraintsTooLongColumnName() { $this->expectException(\InvalidArgumentException::class); $column = $this->createMock(Column::class); @@ -526,11 +526,11 @@ public function testEnsureOracleIdentifierLengthLimitTooLongColumnName() { ->method('hasSequence') ->willReturn(false); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); + self::invokePrivate($this->migrationService, 'ensureOracleConstraints', [$sourceSchema, $schema, 3]); } - public function testEnsureOracleIdentifierLengthLimitTooLongIndexName() { + public function testEnsureOracleConstraintsTooLongIndexName() { $this->expectException(\InvalidArgumentException::class); $index = $this->createMock(Index::class); @@ -563,11 +563,11 @@ public function testEnsureOracleIdentifierLengthLimitTooLongIndexName() { ->method('hasSequence') ->willReturn(false); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); + self::invokePrivate($this->migrationService, 'ensureOracleConstraints', [$sourceSchema, $schema, 3]); } - public function testEnsureOracleIdentifierLengthLimitTooLongForeignKeyName() { + public function testEnsureOracleConstraintsTooLongForeignKeyName() { $this->expectException(\InvalidArgumentException::class); $foreignKey = $this->createMock(ForeignKeyConstraint::class); @@ -603,11 +603,11 @@ public function testEnsureOracleIdentifierLengthLimitTooLongForeignKeyName() { ->method('hasSequence') ->willReturn(false); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); + self::invokePrivate($this->migrationService, 'ensureOracleConstraints', [$sourceSchema, $schema, 3]); } - public function testEnsureOracleIdentifierLengthLimitTooLongSequenceName() { + public function testEnsureOracleConstraintsTooLongSequenceName() { $this->expectException(\InvalidArgumentException::class); $sequence = $this->createMock(Sequence::class); @@ -631,11 +631,11 @@ public function testEnsureOracleIdentifierLengthLimitTooLongSequenceName() { ->method('hasSequence') ->willReturn(false); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); + self::invokePrivate($this->migrationService, 'ensureOracleConstraints', [$sourceSchema, $schema, 3]); } - public function testEnsureOracleIdentifierLengthLimitBooleanNotNull() { + public function testEnsureOracleConstraintsBooleanNotNull() { $this->expectException(\InvalidArgumentException::class); $column = $this->createMock(Column::class); @@ -671,6 +671,6 @@ public function testEnsureOracleIdentifierLengthLimitBooleanNotNull() { ->method('hasSequence') ->willReturn(false); - self::invokePrivate($this->migrationService, 'ensureOracleIdentifierLengthLimit', [$sourceSchema, $schema, 3]); + self::invokePrivate($this->migrationService, 'ensureOracleConstraints', [$sourceSchema, $schema, 3]); } }