From e43468b030f26b5d09f02096ab1aef9d4956eef8 Mon Sep 17 00:00:00 2001 From: Alex Standiford Date: Sun, 26 Jul 2026 19:16:14 -0400 Subject: [PATCH] Stop migrate re-declaring keys and chasing integer widths syncColumns could not run at all against MySQL 8. Two defects, both in the column comparison. A primary key column whose type had drifted was emitted as `MODIFY COLUMN id BIGINT AUTO_INCREMENT PRIMARY KEY`. The key already exists, so the server rejects the statement with "Multiple primary key defined" and the whole migration aborts. Keys belong to the table's index definitions, so the key clause is now dropped on MODIFY and kept on ADD, where there is no existing key to collide with. Since 8.0.17 MySQL discards display widths on integer types, so a schema declaring INT(11) reads back as plain `int`. The comparison treated that as a difference, which marked nearly every integer column in the database as changed. Against one real schema this meant rewriting 43 tables that needed nothing; after the fix, 5, all of them genuine drift. The comparison also used str_contains, so `bigint` contains `int` and a real widening from int to bigint was read as already satisfied and skipped. It now compares normalized types for equality, which catches the widening while still ignoring the discarded width. Verified against a live MySQL 8 database: the widening applied, the primary keys survived, a second run is a no-op, and nothing remains out of sync. --- lib/Strategies/TableUpdateStrategy.php | 88 +++++++-- .../Strategies/TableUpdateStrategyTest.php | 185 ++++++++++++++++++ 2 files changed, 257 insertions(+), 16 deletions(-) create mode 100644 tests/Unit/Strategies/TableUpdateStrategyTest.php 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); + } +}