diff --git a/lib/Strategies/TableUpdateStrategy.php b/lib/Strategies/TableUpdateStrategy.php index b025370..2742683 100644 --- a/lib/Strategies/TableUpdateStrategy.php +++ b/lib/Strategies/TableUpdateStrategy.php @@ -38,7 +38,16 @@ public function syncColumns(Table $table): void } } - protected function convertColumnToSql(Column $column): string + /** + * Attributes that declare a key rather than describe the column. + * + * These are valid when a column is first added, and invalid on MODIFY: the + * key already exists, so MySQL rejects the statement ("Multiple primary key + * defined"). Keys are owned by the table's index definitions. + */ + protected const KEY_ATTRIBUTES = ['PRIMARY KEY', 'UNIQUE KEY', 'UNIQUE']; + + protected function convertColumnToSql(Column $column, bool $includeKeys = true): string { // Get the column name and type $columnName = $column->getName(); @@ -50,10 +59,22 @@ protected function convertColumnToSql(Column $column): string $columnType .= '(' . implode(',', $typeArgs) . ')'; } + $columnAttributes = $column->getAttributes(); + if (!$includeKeys) { + $columnAttributes = Arr::filter( + $columnAttributes, + static fn($attribute): bool => !in_array( + strtoupper(trim((string) $attribute)), + static::KEY_ATTRIBUTES, + true + ) + ); + } + // Handle attributes (e.g., NOT NULL, DEFAULT 'value') - $attributes = implode(' ', $column->getAttributes()); + $attributes = implode(' ', $columnAttributes); - return "`{$columnName}` {$columnType} {$attributes}"; + return rtrim("`{$columnName}` {$columnType} {$attributes}"); } protected function getCurrentColumns(string $tableName): array @@ -75,24 +96,56 @@ protected function getCurrentColumns(string $tableName): array return $columns; } + /** + * Integer types whose declared display width MySQL no longer stores. + * + * Since 8.0.17 the server drops the width from integer types, so a schema + * declaring INT(11) reads back as plain `int`. Comparing the two literally + * marks every integer column in the database as changed. + */ + protected const WIDTHLESS_INTEGER_TYPES = ['tinyint', 'smallint', 'mediumint', 'int', 'integer', 'bigint']; + protected function needsColumnModification(array $currentColumnData, Column $newColumn): bool { - // Check if the column type and type arguments match - $currentType = $currentColumnData['COLUMN_TYPE']; - $newType = $newColumn->getType(); + $currentType = $this->normalizeType((string) $currentColumnData['COLUMN_TYPE']); + $newType = $this->normalizeType($this->declaredType($newColumn)); + + // Compare for equality, not containment. `bigint` contains `int`, so a + // containment check reported a genuine int-to-bigint widening as already + // satisfied and skipped it. + return $currentType !== $newType; + } + + /** The type as written in the schema, including any arguments. */ + protected function declaredType(Column $column): string + { + $type = $column->getType(); + $typeArgs = $column->getTypeArgs(); - // Append type arguments to the new type if they exist - $typeArgs = $newColumn->getTypeArgs(); if (!empty($typeArgs)) { - $newType .= '(' . implode(',', $typeArgs) . ')'; + $type .= '(' . implode(',', $typeArgs) . ')'; } - // Check if the types are different - if (!str_contains(strtolower($currentType), strtolower($newType))) { - return true; + return $type; + } + + /** + * Reduce a type to the form MySQL actually stores, so a schema declaration + * and what the server reports back can be compared for equality. + */ + protected function normalizeType(string $type): string + { + $type = strtolower(trim($type)); + + // Strip the display width from integers, which the server discards. + // Anything else keeps its arguments, since VARCHAR(100) and VARCHAR(255) + // are genuinely different types. + if (preg_match('/^(\w+)\s*\(\s*\d+\s*\)$/', $type, $matches) === 1 + && in_array($matches[1], static::WIDTHLESS_INTEGER_TYPES, true)) { + return $matches[1]; } - return false; + return $type; } /** @@ -111,12 +164,15 @@ protected function buildSyncColumnsQuery(Table $table): ?string foreach ($newColumns as $newColumn) { $columnName = $newColumn->getName(); if (!array_key_exists($columnName, $currentColumns)) { - // Column does not exist, add it + // Column does not exist, add it. A key clause is correct here + // because there is no existing key to collide with. $queries[] = "ADD COLUMN " . $this->convertColumnToSql($newColumn); } else { - // Column exists, check if it needs to be modified + // Column exists, check if it needs to be modified. The key clause + // is dropped: the key already exists, and re-declaring it makes + // MySQL reject the whole statement. if ($this->needsColumnModification($currentColumns[$columnName], $newColumn)) { - $queries[] = "MODIFY COLUMN " . $this->convertColumnToSql($newColumn); + $queries[] = "MODIFY COLUMN " . $this->convertColumnToSql($newColumn, false); } } } diff --git a/tests/Unit/Strategies/TableUpdateStrategyTest.php b/tests/Unit/Strategies/TableUpdateStrategyTest.php new file mode 100644 index 0000000..4978d73 --- /dev/null +++ b/tests/Unit/Strategies/TableUpdateStrategyTest.php @@ -0,0 +1,185 @@ +> $currentColumns Keyed by column name. + * @param Column[] $schemaColumns + */ + private function buildQuery(array $currentColumns, array $schemaColumns): ?string + { + $rows = []; + foreach ($currentColumns as $name => $row) { + $rows[] = array_merge(['COLUMN_NAME' => $name], $row); + } + + $db = Mockery::mock(DatabaseStrategy::class); + $db->shouldReceive('parse')->andReturnUsing(fn (string $q) => $q); + $db->shouldReceive('query')->andReturn($rows); + + $table = Mockery::mock(Table::class); + $table->shouldReceive('getName')->andReturn('posts'); + $table->shouldReceive('getAlias')->andReturn('p'); + $table->shouldReceive('getColumns')->andReturn($schemaColumns); + + $strategy = new TableUpdateStrategy($db); + $build = new ReflectionMethod($strategy, 'buildSyncColumnsQuery'); + $build->setAccessible(true); + + return $build->invoke($strategy, $table); + } + + /** + * MySQL 8.0.17 removed display widths from integer types, so the schema's + * INT(11) reads back as plain `int`. Treating that as a difference makes + * every integer column in the database look perpetually out of date, and + * migrate then rewrites tables it has no reason to touch. + */ + public function test_integer_display_width_is_not_a_difference(): void + { + $query = $this->buildQuery( + ['views' => ['COLUMN_TYPE' => 'int', 'IS_NULLABLE' => 'YES', 'COLUMN_DEFAULT' => null, 'EXTRA' => '']], + [new Column('views', 'INT', [11])] + ); + + $this->assertNull($query, 'An int column matching INT(11) needs no change.'); + } + + public function test_bigint_display_width_is_not_a_difference(): void + { + $query = $this->buildQuery( + ['size' => ['COLUMN_TYPE' => 'bigint', 'IS_NULLABLE' => 'YES', 'COLUMN_DEFAULT' => null, 'EXTRA' => '']], + [new Column('size', 'BIGINT', [20])] + ); + + $this->assertNull($query); + } + + /** + * The reverse of the width problem. Comparing with str_contains means + * `bigint` contains `int`, so a genuine widening from int to bigint was + * read as already satisfied and silently skipped. + */ + public function test_widening_an_int_to_bigint_is_still_detected(): void + { + $query = $this->buildQuery( + ['id' => ['COLUMN_TYPE' => 'int', 'IS_NULLABLE' => 'NO', 'COLUMN_DEFAULT' => null, 'EXTRA' => '']], + [new Column('id', 'BIGINT', null, 'NOT NULL')] + ); + + $this->assertNotNull($query, 'An int column must still be widened to BIGINT.'); + $this->assertStringContainsString('MODIFY COLUMN `id` BIGINT', $query); + } + + /** + * The bug that made migrate unusable. A primary key column whose type has + * drifted needs modifying, but re-emitting PRIMARY KEY on MODIFY COLUMN + * makes MySQL reject the statement with "Multiple primary key defined", + * because the key already exists. Keys belong to index definitions, not to + * a column modification. + */ + public function test_modify_does_not_re_declare_the_primary_key(): void + { + $query = $this->buildQuery( + ['id' => ['COLUMN_TYPE' => 'int', 'IS_NULLABLE' => 'NO', 'COLUMN_DEFAULT' => null, 'EXTRA' => 'auto_increment']], + [(new PrimaryKeyFactory())->toColumn()] + ); + + $this->assertNotNull($query); + $this->assertStringContainsString('MODIFY COLUMN `id` BIGINT', $query); + $this->assertStringNotContainsString('PRIMARY KEY', $query); + $this->assertStringContainsString('AUTO_INCREMENT', $query, 'Auto increment is a column property and must stay.'); + } + + /** + * Adding a brand new column is the one place the key clause is correct, + * because there is no existing key to collide with. + */ + public function test_adding_a_new_primary_key_column_keeps_the_key_clause(): void + { + $query = $this->buildQuery( + [], + [(new PrimaryKeyFactory())->toColumn()] + ); + + $this->assertNotNull($query); + $this->assertStringContainsString('ADD COLUMN `id` BIGINT', $query); + $this->assertStringContainsString('PRIMARY KEY', $query); + } + + public function test_a_genuinely_new_column_is_added(): void + { + $query = $this->buildQuery( + ['title' => ['COLUMN_TYPE' => 'varchar(255)', 'IS_NULLABLE' => 'NO', 'COLUMN_DEFAULT' => null, 'EXTRA' => '']], + [ + new Column('title', 'VARCHAR', [255], 'NOT NULL'), + new Column('slug', 'VARCHAR', [255], 'NOT NULL'), + ] + ); + + $this->assertNotNull($query); + $this->assertStringContainsString('ADD COLUMN `slug` VARCHAR(255)', $query); + $this->assertStringNotContainsString('`title`', $query, 'An unchanged column must be left alone.'); + } + + public function test_a_removed_column_is_dropped(): void + { + $query = $this->buildQuery( + [ + 'title' => ['COLUMN_TYPE' => 'varchar(255)', 'IS_NULLABLE' => 'NO', 'COLUMN_DEFAULT' => null, 'EXTRA' => ''], + 'legacy' => ['COLUMN_TYPE' => 'varchar(255)', 'IS_NULLABLE' => 'YES', 'COLUMN_DEFAULT' => null, 'EXTRA' => ''], + ], + [new Column('title', 'VARCHAR', [255], 'NOT NULL')] + ); + + $this->assertNotNull($query); + $this->assertStringContainsString('DROP COLUMN `legacy`', $query); + } + + public function test_a_varchar_length_change_is_detected(): void + { + $query = $this->buildQuery( + ['title' => ['COLUMN_TYPE' => 'varchar(100)', 'IS_NULLABLE' => 'NO', 'COLUMN_DEFAULT' => null, 'EXTRA' => '']], + [new Column('title', 'VARCHAR', [255], 'NOT NULL')] + ); + + $this->assertNotNull($query); + $this->assertStringContainsString('MODIFY COLUMN `title` VARCHAR(255)', $query); + } + + /** + * A table already matching its definition must produce no statement at all, + * so migrate is a no-op rather than a rewrite of every table it inspects. + */ + public function test_a_table_in_sync_produces_no_query(): void + { + $query = $this->buildQuery( + [ + 'id' => ['COLUMN_TYPE' => 'bigint', 'IS_NULLABLE' => 'NO', 'COLUMN_DEFAULT' => null, 'EXTRA' => 'auto_increment'], + 'title' => ['COLUMN_TYPE' => 'varchar(255)', 'IS_NULLABLE' => 'NO', 'COLUMN_DEFAULT' => null, 'EXTRA' => ''], + 'views' => ['COLUMN_TYPE' => 'int', 'IS_NULLABLE' => 'YES', 'COLUMN_DEFAULT' => null, 'EXTRA' => ''], + ], + [ + (new PrimaryKeyFactory())->toColumn(), + new Column('title', 'VARCHAR', [255], 'NOT NULL'), + new Column('views', 'INT', [11]), + ] + ); + + $this->assertNull($query); + } +}