fix(SQLite): preserve indexes and foreign keys when altering an introspected table

[fballiano]

Q	A
Type	bug
Fixed issues	n/a

Summary

alterTable() on SQLite silently drops every index and foreign key (and the primary key) of the altered table whenever the table was obtained through the new introspection API. Rows survive the recreation; the indexes and foreign keys are gone, with no error or deprecation.

$conn = DriverManager::getConnection(['driver' => 'pdo_sqlite', 'memory' => true]);
$sm = $conn->createSchemaManager();

$conn->executeStatement('CREATE TABLE products (id INTEGER PRIMARY KEY, sku TEXT, qty INTEGER)');
$conn->executeStatement('CREATE UNIQUE INDEX idx_sku ON products (sku)');
$conn->executeStatement('CREATE INDEX idx_qty ON products (qty)');

$old = $sm->introspectTableByUnquotedName('products');
$new = $old->edit()
    ->modifyColumnByUnquotedName('qty', static function (ColumnEditor $e): void {
        $e->setTypeName(Types::STRING)->setLength(32);
    })
    ->create();
$sm->alterTable($sm->createComparator()->compareTables($old, $new));

// idx_sku and idx_qty are gone

Root cause

The legacy column-name accessors of an introspected table are inconsistent. Column::getName() returns the bare, unquoted name (sku), but Index::getColumns() and ForeignKeyConstraint::getLocalColumns()/getForeignColumns() return the raw map keys, which the new introspection API marks as quoted ("sku"). The SQLite table-recreation path maps index and foreign-key columns to the altered table through getDiffColumnNameMap(), which is keyed by the unquoted column names, so every lookup misses and the branch meant to drop indexes referencing dropped columns:

if (! isset($nameMap[$normalizedColumnName])) {
    unset($indexes[$key]);
    continue 2;
}

fires for every index and foreign key of the table.

The existing testNonSimpleAlterTableCreatedFromDDL does not catch this because it uses the deprecated introspectTable(), which returns legacy-unquoted names that happen to match the map.

Fix

The table-recreation path now reads the referencing column names through the non-deprecated Index::getIndexedColumns() and ForeignKeyConstraint::getReferencingColumnNames() accessors and compares their unquoted identifier values. This makes the lookup agnostic of whether the table was introspected (which marks its column names as quoted) or built in memory, without relying on the quoted form of the deprecated getColumns() / getLocalColumns() accessors.

Note: normalizing those deprecated accessors to return unquoted names was considered but rejected, because their quoted form is load-bearing for quoting propagation in user-built schemas (it would break testQuotedColumnInForeignKeyPropagation across all platforms).

The regression test, which is not platform-specific, is added to SchemaManagerFunctionalTestCase so it runs against every platform. It is built through the Schema API and fails without the fix (indexes and foreign keys dropped) and passes with it.

[fballiano]

Reworked again after running the suite on CI.

Normalizing the legacy accessors (Index::getColumns(), ForeignKeyConstraint::getLocalColumns()/getForeignColumns()) to return unquoted names turned out not to be viable: their quoted form is load-bearing for quoting propagation in user-built schemas, so it breaks testQuotedColumnInForeignKeyPropagation on every platform.

Instead the fix stays in the SQLite table-recreation path but no longer relies on the quoted legacy accessors. It reads the referencing column names via the non-deprecated Index::getIndexedColumns() and ForeignKeyConstraint::getReferencingColumnNames() and compares their unquoted identifier values, so the lookup is agnostic of whether the table was introspected (which marks the names as quoted) or built in memory.

The regression test lives in SchemaManagerFunctionalTestCase so it runs against every platform.

I ran the full suite on my fork to check CI: fballiano#1, all green.

[fballiano]

Sorry @morozov I forgot to mark this for review 🤦🏻 Hope it's ok now, I'm not sure about your previous note about the test suite actually.

[morozov]

Directionally, this looks right. Still needs some polishing.

[fballiano]

@morozov thank you, I hope I addressed all the issues in the right way, CI is green again :-)