From 17a45b94f8bb311f89c231f5a6ae77c40394d47a Mon Sep 17 00:00:00 2001 From: Alex Standiford Date: Sun, 26 Jul 2026 21:24:02 -0400 Subject: [PATCH] Make column sync additive by default and dropping explicit syncColumns dropped any column present in the database but absent from the schema definition. That is only correct when the definition is authoritative for the database it is pointed at, and an unattended caller cannot know that. A rollback, a canary, or two services sharing one database all mean the running code can be missing a column the database legitimately holds, and dropping it there destroys data. Navigator had already noticed. Its container startup script runs install before serving traffic but deliberately does not run migrate, guarded by a test named testStartupDoesNotRunDestructiveGenericColumnSynchronization. The consequence was that no column could be added to a deployed table at all, so a schema change could not ship. syncColumns now adds and modifies, and leaves a column it does not recognise alone. syncColumnsAllowingDrops keeps the old behaviour for a caller that means it. The safe behaviour is the default rather than the opt-in because the dangerous one is unrecoverable and the cautious one leaves, at worst, a dead column. No change to the core TableUpdateStrategy interface, so the sqlite and wordpress implementations are untouched. Adding a method there would have broken all three. Verified against a live MySQL 8 database: a needed column was dropped and an unknown one added by hand, then migrate restored the first and left the second in place. Before this change the second would have been destroyed. --- lib/Strategies/TableUpdateStrategy.php | 55 ++++++- .../Strategies/TableUpdateStrategyTest.php | 136 ++++++++++++++++-- 2 files changed, 172 insertions(+), 19 deletions(-) diff --git a/lib/Strategies/TableUpdateStrategy.php b/lib/Strategies/TableUpdateStrategy.php index 2742683..b66c755 100644 --- a/lib/Strategies/TableUpdateStrategy.php +++ b/lib/Strategies/TableUpdateStrategy.php @@ -19,6 +19,17 @@ 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 @@ -26,7 +37,7 @@ public function __construct(DatabaseStrategy $db) public function syncColumns(Table $table): void { try { - $query = $this->buildSyncColumnsQuery($table); + $query = $this->buildSyncColumnsQuery($table, false); if (!$query) { return; @@ -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 @@ -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(); @@ -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}`"; + } } } diff --git a/tests/Unit/Strategies/TableUpdateStrategyTest.php b/tests/Unit/Strategies/TableUpdateStrategyTest.php index 4978d73..f22348a 100644 --- a/tests/Unit/Strategies/TableUpdateStrategyTest.php +++ b/tests/Unit/Strategies/TableUpdateStrategyTest.php @@ -20,7 +20,7 @@ class TableUpdateStrategyTest extends TestCase * @param array> $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) { @@ -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> $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); } /** @@ -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])] ); @@ -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])] ); @@ -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')] ); @@ -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()] ); @@ -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()] ); @@ -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'), @@ -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' => ''], @@ -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')] ); @@ -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' => ''], @@ -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.'); + } }