Skip to content

Stop migrate re-declaring keys and chasing integer widths #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alexstandiford merged 1 commit into from
Jul 26, 2026
Merged

Stop migrate re-declaring keys and chasing integer widths #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 72 additions & 16 deletions lib/Strategies/TableUpdateStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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
Expand All @@ -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));
Comment on lines +110 to +111

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Normalize type modifiers before exact comparison

For schema columns with modifiers such as UNSIGNED, these operands use different representations: COLUMN_TYPE is bigint unsigned, while declaredType() returns only bigint because UNSIGNED lives in the column attributes. The exact comparison therefore emits the same MODIFY COLUMN ... BIGINT UNSIGNED on every synchronization even though the database is already correct; server-expanded defaults such as DECIMAL becoming decimal(10,0) have the same problem. Compare parsed base types and modifiers/defaults, or construct both normalized values from equivalent inputs.

Useful? React with 👍 / 👎.


// 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;
}

/**
Expand All @@ -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);

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve newly declared keys on existing columns

When an existing non-key column is changed to a primary or unique key while its type also changes, always passing false strips the newly requested key clause. For example, migrating an existing INT id to the BIGINT PRIMARY KEY produced by PrimaryKeyFactory changes the type but never creates the primary key; later runs see the matching type and never retry the constraint. Only suppress a key that is confirmed to exist, or emit the key change separately.

Useful? React with 👍 / 👎.

}
}
}
Expand Down
185 changes: 185 additions & 0 deletions tests/Unit/Strategies/TableUpdateStrategyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
<?php

namespace PHPNomad\MySql\Integration\Tests\Unit\Strategies;

use Mockery;
use PHPNomad\Database\Factories\Column;
use PHPNomad\Database\Factories\Columns\PrimaryKeyFactory;
use PHPNomad\Database\Interfaces\Table;
use PHPNomad\MySql\Integration\Interfaces\DatabaseStrategy;
use PHPNomad\MySql\Integration\Strategies\TableUpdateStrategy;
use PHPNomad\MySql\Integration\Tests\TestCase;
use ReflectionMethod;

/**
* @covers \PHPNomad\MySql\Integration\Strategies\TableUpdateStrategy
*/
class TableUpdateStrategyTest extends TestCase
{
/**
* @param array<string, array<string, string|null>> $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);
}
}
Loading