Skip to content

Make column sync additive by default and dropping explicit #18

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 27, 2026
Merged

Make column sync additive by default and dropping explicit #18

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
55 changes: 48 additions & 7 deletions lib/Strategies/TableUpdateStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,25 @@ public function __construct(DatabaseStrategy $db)
}

/**
* Bring a table's columns in line with its definition, without removing
* anything.
*
* Adds missing columns and modifies changed ones. It does NOT drop a column
* the definition no longer declares, because this runs unattended and a
* deploy is not always ahead of its database: a rollback, a canary, or two
* services sharing one database all mean the running code can be missing a
* column the database legitimately holds. Dropping it there destroys data.
*
* Use {@see syncColumnsAllowingDrops()} when removal is the intent.
*
* @param Table $table
* @return void
* @throws TableUpdateFailedException
*/
public function syncColumns(Table $table): void
{
try {
$query = $this->buildSyncColumnsQuery($table);
$query = $this->buildSyncColumnsQuery($table, false);

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 27, 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 Keep additive sync from narrowing newer column types

When older code runs after a newer deployment has widened a column, passing false here prevents only DROP COLUMN; buildSyncColumnsQuery() still emits MODIFY COLUMN for every unequal type. For example, a rollback declaring VARCHAR(100) against the newer database's VARCHAR(255) narrows the column, potentially failing startup when longer values exist or truncating them under permissive settings. This defeats the rollback/canary safety promised by the new default, so narrowing or otherwise destructive modifications should also require the explicit destructive path.

Useful? React with 👍 / 👎.


if (!$query) {
return;
Expand All @@ -47,6 +58,30 @@ public function syncColumns(Table $table): void
*/
protected const KEY_ATTRIBUTES = ['PRIMARY KEY', 'UNIQUE KEY', 'UNIQUE'];

/**
* Sync columns and drop any the definition no longer declares.
*
* Destructive, and deliberately not the default. Only call this when the
* caller knows the definition is authoritative for the database it is
* pointed at, which an unattended deploy cannot know.
*
* @throws TableUpdateFailedException
*/
public function syncColumnsAllowingDrops(Table $table): void
{
try {
$query = $this->buildSyncColumnsQuery($table, true);

if (!$query) {
return;
}

$this->db->query($query);
} catch (\Exception $e) {
throw new TableUpdateFailedException($e);
}
}

protected function convertColumnToSql(Column $column, bool $includeKeys = true): string
{
// Get the column name and type
Expand Down Expand Up @@ -154,7 +189,11 @@ protected function normalizeType(string $type): string
* @param Table $table
* @return string|null
*/
protected function buildSyncColumnsQuery(Table $table): ?string
/**
* @param bool $allowDrops When false, a column the schema no longer declares
* is left in place rather than dropped.
*/
protected function buildSyncColumnsQuery(Table $table, bool $allowDrops = true): ?string
{
$currentColumns = $this->getCurrentColumns($table->getName());
$newColumns = $table->getColumns();
Expand All @@ -177,12 +216,14 @@ protected function buildSyncColumnsQuery(Table $table): ?string
}
}

$newColumnNames = Arr::pluck($newColumns, 'name');
if ($allowDrops) {
$newColumnNames = Arr::pluck($newColumns, 'name');

// Drop columns that no longer exist in the new definition
foreach ($currentColumns as $currentColumnName => $currentColumnData) {
if (!in_array($currentColumnName, $newColumnNames)) {
$queries[] = "DROP COLUMN `{$currentColumnName}`";
// Drop columns that no longer exist in the new definition
foreach ($currentColumns as $currentColumnName => $currentColumnData) {
if (!in_array($currentColumnName, $newColumnNames)) {
$queries[] = "DROP COLUMN `{$currentColumnName}`";
}
}
}

Expand Down
136 changes: 124 additions & 12 deletions tests/Unit/Strategies/TableUpdateStrategyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ 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
private function buildQueryAllowingDrops(array $currentColumns, array $schemaColumns): ?string
{
$rows = [];
foreach ($currentColumns as $name => $row) {
Expand All @@ -40,7 +40,34 @@ private function buildQuery(array $currentColumns, array $schemaColumns): ?strin
$build = new ReflectionMethod($strategy, 'buildSyncColumnsQuery');
$build->setAccessible(true);

return $build->invoke($strategy, $table);
return $build->invoke($strategy, $table, true);
}

/**
* @param array<string, array<string, string|null>> $currentColumns Keyed by column name.
* @param Column[] $schemaColumns
*/
private function buildAdditiveQuery(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, false);
}

/**
Expand All @@ -51,7 +78,7 @@ private function buildQuery(array $currentColumns, array $schemaColumns): ?strin
*/
public function test_integer_display_width_is_not_a_difference(): void
{
$query = $this->buildQuery(
$query = $this->buildQueryAllowingDrops(
['views' => ['COLUMN_TYPE' => 'int', 'IS_NULLABLE' => 'YES', 'COLUMN_DEFAULT' => null, 'EXTRA' => '']],
[new Column('views', 'INT', [11])]
);
Expand All @@ -61,7 +88,7 @@ public function test_integer_display_width_is_not_a_difference(): void

public function test_bigint_display_width_is_not_a_difference(): void
{
$query = $this->buildQuery(
$query = $this->buildQueryAllowingDrops(
['size' => ['COLUMN_TYPE' => 'bigint', 'IS_NULLABLE' => 'YES', 'COLUMN_DEFAULT' => null, 'EXTRA' => '']],
[new Column('size', 'BIGINT', [20])]
);
Expand All @@ -76,7 +103,7 @@ public function test_bigint_display_width_is_not_a_difference(): void
*/
public function test_widening_an_int_to_bigint_is_still_detected(): void
{
$query = $this->buildQuery(
$query = $this->buildQueryAllowingDrops(
['id' => ['COLUMN_TYPE' => 'int', 'IS_NULLABLE' => 'NO', 'COLUMN_DEFAULT' => null, 'EXTRA' => '']],
[new Column('id', 'BIGINT', null, 'NOT NULL')]
);
Expand All @@ -94,7 +121,7 @@ public function test_widening_an_int_to_bigint_is_still_detected(): void
*/
public function test_modify_does_not_re_declare_the_primary_key(): void
{
$query = $this->buildQuery(
$query = $this->buildQueryAllowingDrops(
['id' => ['COLUMN_TYPE' => 'int', 'IS_NULLABLE' => 'NO', 'COLUMN_DEFAULT' => null, 'EXTRA' => 'auto_increment']],
[(new PrimaryKeyFactory())->toColumn()]
);
Expand All @@ -111,7 +138,7 @@ public function test_modify_does_not_re_declare_the_primary_key(): void
*/
public function test_adding_a_new_primary_key_column_keeps_the_key_clause(): void
{
$query = $this->buildQuery(
$query = $this->buildQueryAllowingDrops(
[],
[(new PrimaryKeyFactory())->toColumn()]
);
Expand All @@ -123,7 +150,7 @@ public function test_adding_a_new_primary_key_column_keeps_the_key_clause(): voi

public function test_a_genuinely_new_column_is_added(): void
{
$query = $this->buildQuery(
$query = $this->buildQueryAllowingDrops(
['title' => ['COLUMN_TYPE' => 'varchar(255)', 'IS_NULLABLE' => 'NO', 'COLUMN_DEFAULT' => null, 'EXTRA' => '']],
[
new Column('title', 'VARCHAR', [255], 'NOT NULL'),
Expand All @@ -136,9 +163,10 @@ public function test_a_genuinely_new_column_is_added(): void
$this->assertStringNotContainsString('`title`', $query, 'An unchanged column must be left alone.');
}

public function test_a_removed_column_is_dropped(): void
/** Only the explicit drop-allowing path removes a column. */
public function test_a_removed_column_is_dropped_when_drops_are_allowed(): void
{
$query = $this->buildQuery(
$query = $this->buildQueryAllowingDrops(
[
'title' => ['COLUMN_TYPE' => 'varchar(255)', 'IS_NULLABLE' => 'NO', 'COLUMN_DEFAULT' => null, 'EXTRA' => ''],
'legacy' => ['COLUMN_TYPE' => 'varchar(255)', 'IS_NULLABLE' => 'YES', 'COLUMN_DEFAULT' => null, 'EXTRA' => ''],
Expand All @@ -152,7 +180,7 @@ public function test_a_removed_column_is_dropped(): void

public function test_a_varchar_length_change_is_detected(): void
{
$query = $this->buildQuery(
$query = $this->buildQueryAllowingDrops(
['title' => ['COLUMN_TYPE' => 'varchar(100)', 'IS_NULLABLE' => 'NO', 'COLUMN_DEFAULT' => null, 'EXTRA' => '']],
[new Column('title', 'VARCHAR', [255], 'NOT NULL')]
);
Expand All @@ -167,7 +195,7 @@ public function test_a_varchar_length_change_is_detected(): void
*/
public function test_a_table_in_sync_produces_no_query(): void
{
$query = $this->buildQuery(
$query = $this->buildQueryAllowingDrops(
[
'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' => ''],
Expand All @@ -182,4 +210,88 @@ public function test_a_table_in_sync_produces_no_query(): void

$this->assertNull($query);
}

// --- additive-only synchronisation ---

/**
* The reason this mode exists. A container startup script has to bring the
* schema forward before serving traffic, but a deploy is not always ahead of
* the database: a rollback, a canary, or two services sharing one database
* all mean the code can be missing a column the database legitimately holds.
* Dropping it there destroys data, so dropping must stay a deliberate act.
*/
public function test_additive_mode_never_drops_a_column_the_schema_does_not_declare(): void
{
$query = $this->buildAdditiveQuery(
[
'title' => ['COLUMN_TYPE' => 'varchar(255)', 'IS_NULLABLE' => 'NO', 'COLUMN_DEFAULT' => null, 'EXTRA' => ''],
'writtenByNewerCode' => ['COLUMN_TYPE' => 'varchar(255)', 'IS_NULLABLE' => 'YES', 'COLUMN_DEFAULT' => null, 'EXTRA' => ''],
],
[new Column('title', 'VARCHAR', [255], 'NOT NULL')]
);

$this->assertNull($query, 'An unknown column is left alone, so there is nothing to do.');
}

public function test_additive_mode_still_adds_a_missing_column(): void
{
$query = $this->buildAdditiveQuery(
['title' => ['COLUMN_TYPE' => 'varchar(255)', 'IS_NULLABLE' => 'NO', 'COLUMN_DEFAULT' => null, 'EXTRA' => '']],
[
new Column('title', 'VARCHAR', [255], 'NOT NULL'),
new Column('trustScore', 'INT', [11], 'NOT NULL', 'DEFAULT 500'),
]
);

$this->assertNotNull($query);
$this->assertStringContainsString('ADD COLUMN `trustScore` INT(11)', $query);
$this->assertStringNotContainsString('DROP COLUMN', $query);
}

public function test_additive_mode_still_widens_a_changed_column(): void
{
$query = $this->buildAdditiveQuery(
['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);
}

public function test_additive_mode_adds_and_keeps_an_unknown_column_in_the_same_table(): void
{
$query = $this->buildAdditiveQuery(
[
'title' => ['COLUMN_TYPE' => 'varchar(255)', 'IS_NULLABLE' => 'NO', 'COLUMN_DEFAULT' => null, 'EXTRA' => ''],
'writtenByNewerCode' => ['COLUMN_TYPE' => 'text', 'IS_NULLABLE' => 'YES', 'COLUMN_DEFAULT' => null, 'EXTRA' => ''],
],
[
new Column('title', 'VARCHAR', [255], 'NOT NULL'),
new Column('reviewedAt', 'DATETIME'),
]
);

$this->assertNotNull($query);
$this->assertStringContainsString('ADD COLUMN `reviewedAt` DATETIME', $query);
$this->assertStringNotContainsString('writtenByNewerCode', $query);
}

/**
* The default must not drop. This is the guard: syncColumns runs unattended
* from a container startup script, and a deploy is not always ahead of its
* database.
*/
public function test_the_default_does_not_drop_a_removed_column(): void
{
$query = $this->buildAdditiveQuery(
[
'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->assertNull($query, 'The default leaves an undeclared column alone.');
}
}
Loading