fix(sqlite): read generated columns with pragma_table_xinfo (weasel#426)

docs/sqlite/tables.md

@@ -43,6 +43,10 @@ table.AddColumn("email_domain", "TEXT")
 <sup><a href='https://github.com/JasperFx/weasel/blob/master/src/DocSamples/SqliteSamples.cs#L56-L60' title='Snippet source file'>snippet source</a> | <a href='#snippet-sample_sqlite_generated_columns' title='Start of snippet'>anchor</a></sup>
 <!-- endSnippet -->
 
+Generated columns are read back during delta detection, so a table declaring one converges on the second migration run rather than re-adding the column every time. Adding a `Virtual` generated column to an existing table is an `ALTER TABLE ADD COLUMN`; adding a `Stored` one is not — SQLite rejects that outright, so Weasel migrates it through a table recreation instead.
+
+The generation *expression* is not read back from the database (unlike PostgreSQL, where it comes from the catalog). A column is matched on name and type only, so changing the expression of an existing generated column is not detected as a delta and will not migrate on its own.
+
 ## Foreign Keys
 
 Foreign keys must be defined inline at table creation. SQLite does not support `ALTER TABLE ADD CONSTRAINT`:

src/Weasel.Sqlite.Tests/Tables/GeneratedColumnDeltaTests.cs

@@ -0,0 +1,207 @@
+using Microsoft.Data.Sqlite;
+using Shouldly;
+using Weasel.Core;
+using Weasel.Sqlite.Tables;
+using Xunit;
+using DbCommandBuilder = Weasel.Core.DbCommandBuilder;
+
+namespace Weasel.Sqlite.Tests.Tables;
+
+/// <summary>
+///     Coverage for weasel#426: <c>pragma_table_info</c> does not list generated columns, so a table
+///     declaring one was read back without it. The delta reported the column missing on every run,
+///     emitted <c>ALTER TABLE ... ADD COLUMN</c>, and the second migration failed with
+///     <c>duplicate column name</c> -- i.e. such a table never converged.
+/// </summary>
+[Collection("integration")]
+public class GeneratedColumnDeltaTests
+{
+    private static async Task<SqliteConnection> OpenConnectionAsync()
+    {
+        var connection = new SqliteConnection("Data Source=:memory:");
+        await connection.OpenAsync();
+        return connection;
+    }
+
+    private static Table TableWithGeneratedColumn(GeneratedColumnType type)
+    {
+        var table = new Table("documents");
+        table.AddColumn<string>("id").AsPrimaryKey();
+        table.AddColumn<string>("data").NotNull();
+        table.AddColumn("name", "TEXT").GeneratedAs("json_extract(data, '$.name')", type);
+
+        return table;
+    }
+
+    [Theory]
+    [InlineData(GeneratedColumnType.Virtual)]
+    [InlineData(GeneratedColumnType.Stored)]
+    public async Task generated_column_is_read_back_from_the_database(GeneratedColumnType type)
+    {
+        await using var connection = await OpenConnectionAsync();
+
+        var table = TableWithGeneratedColumn(type);
+        await table.CreateAsync(connection);
+
+        var existing = await table.FetchExistingAsync(connection);
+
+        existing.ShouldNotBeNull();
+        existing.Columns.Select(x => x.Name)
+            .ShouldBe(["id", "data", "name"]);
+    }
+
+    [Theory]
+    [InlineData(GeneratedColumnType.Virtual)]
+    [InlineData(GeneratedColumnType.Stored)]
+    public async Task table_with_a_generated_column_converges(GeneratedColumnType type)
+    {
+        await using var connection = await OpenConnectionAsync();
+
+        var table = TableWithGeneratedColumn(type);
+        await table.CreateAsync(connection);
+
+        var delta = await table.FindDeltaAsync(connection);
+
+        delta.Difference.ShouldBe(SchemaPatchDifference.None);
+        delta.Columns.Missing.ShouldBeEmpty();
+        delta.RequiresTableRecreation.ShouldBeFalse();
+    }
+
+    [Fact]
+    public async Task adding_a_virtual_generated_column_migrates_and_then_converges()
+    {
+        await using var connection = await OpenConnectionAsync();
+
+        var table = new Table("documents");
+        table.AddColumn<string>("id").AsPrimaryKey();
+        table.AddColumn<string>("data").NotNull();
+
+        await table.CreateAsync(connection);
+
+        // SQLite permits ALTER TABLE ADD COLUMN for a VIRTUAL generated column, so this is an
+        // incremental alter rather than a recreation.
+        table.AddColumn("name", "TEXT")
+            .GeneratedAs("json_extract(data, '$.name')", GeneratedColumnType.Virtual);
+
+        var delta = await table.FindDeltaAsync(connection);
+        delta.Difference.ShouldBe(SchemaPatchDifference.Update);
+        delta.RequiresTableRecreation.ShouldBeFalse();
+
+        await ApplyAsync(connection, delta);
+
+        // The point of the issue: a second pass sees nothing to do rather than re-adding the column.
+        var afterDelta = await table.FindDeltaAsync(connection);
+        afterDelta.Difference.ShouldBe(SchemaPatchDifference.None);
+    }
+
+    [Fact]
+    public async Task adding_a_stored_generated_column_recreates_the_table_and_then_converges()
+    {
+        await using var connection = await OpenConnectionAsync();
+
+        var table = new Table("documents");
+        table.AddColumn<string>("id").AsPrimaryKey();
+        table.AddColumn<string>("data").NotNull();
+
+        await table.CreateAsync(connection);
+
+        await connection.CreateCommand(
+                "INSERT INTO \"documents\" (\"id\", \"data\") VALUES ('one', '{\"name\": \"Anne\"}');")
+            .ExecuteNonQueryAsync();
+
+        // SQLite rejects ALTER TABLE ADD COLUMN for a STORED generated column, so this has to go
+        // through the recreation path.
+        table.AddColumn("name", "TEXT")
+            .GeneratedAs("json_extract(data, '$.name')", GeneratedColumnType.Stored);
+
+        var delta = await table.FindDeltaAsync(connection);
+        delta.RequiresTableRecreation.ShouldBeTrue();
+
+        await ApplyAsync(connection, delta);
+
+        var afterDelta = await table.FindDeltaAsync(connection);
+        afterDelta.Difference.ShouldBe(SchemaPatchDifference.None);
+
+        // The recreation must not have dropped the row, and the generated value has to be computed
+        // from the copied 'data' column.
+        var name = await connection.CreateCommand("SELECT \"name\" FROM \"documents\" WHERE \"id\" = 'one';")
+            .ExecuteScalarAsync();
+        name.ShouldBe("Anne");
+    }
+
+    [Fact]
+    public async Task recreating_a_table_that_already_has_a_generated_column_does_not_copy_it()
+    {
+        await using var connection = await OpenConnectionAsync();
+
+        var table = TableWithGeneratedColumn(GeneratedColumnType.Virtual);
+        await table.CreateAsync(connection);
+
+        await connection.CreateCommand(
+                "INSERT INTO \"documents\" (\"id\", \"data\") VALUES ('one', '{\"name\": \"Anne\"}');")
+            .ExecuteNonQueryAsync();
+
+        // A foreign key change forces recreation; the generated column comes along for the ride and
+        // must be left out of the INSERT ... SELECT, since SQLite refuses writes to generated columns.
+        var users = new Table("users");
+        users.AddColumn<string>("id").AsPrimaryKey();
+        await users.CreateAsync(connection);
+
+        table.AddColumn<string>("user_id");
+        table.ForeignKeys.Add(new ForeignKey("fk_documents_users")
+        {
+            LinkedTable = new SqliteObjectName("users"),
+            ColumnNames = ["user_id"],
+            LinkedNames = ["id"]
+        });
+
+        var delta = await table.FindDeltaAsync(connection);
+        delta.RequiresTableRecreation.ShouldBeTrue();
+
+        await ApplyAsync(connection, delta);
+
+        var name = await connection.CreateCommand("SELECT \"name\" FROM \"documents\" WHERE \"id\" = 'one';")
+            .ExecuteScalarAsync();
+        name.ShouldBe("Anne");
+    }
+
+    [Fact]
+    public async Task the_column_query_does_not_widen_to_a_virtual_tables_hidden_columns()
+    {
+        await using var connection = await OpenConnectionAsync();
+
+        // table_xinfo reports a virtual table's hidden columns -- for fts5 that is the table-name
+        // column and 'rank' -- where table_info reported neither. Switching pragmas must not widen
+        // what Weasel considers a column, so the query filters them back out.
+        //
+        // This is asserted against the query rather than through FetchExistingAsync because fts5
+        // columns report an empty type, which TableColumn rejects: Weasel.Sqlite cannot introspect a
+        // virtual table at all, before or after this change. That gap is its own question.
+        await connection.CreateCommand("CREATE VIRTUAL TABLE search USING fts5(title, body);")
+            .ExecuteNonQueryAsync();
+
+        var queryCmd = connection.CreateCommand();
+        var builder = new DbCommandBuilder(queryCmd);
+        new Table("search").ConfigureQueryCommand(builder);
+        builder.Compile();
+
+        await using var reader = await queryCmd.ExecuteReaderAsync();
+        await reader.NextResultAsync(); // table SQL -> columns
+
+        var names = new List<string>();
+        while (await reader.ReadAsync())
+        {
+            names.Add(reader.GetString(1));
+        }
+
+        names.ShouldBe(["title", "body"]);
+    }
+
+    private static async Task ApplyAsync(SqliteConnection connection, TableDelta delta)
+    {
+        var writer = new StringWriter();
+        delta.WriteUpdate(new SqliteMigrator(), writer);
+
+        await connection.CreateCommand(writer.ToString()).ExecuteNonQueryAsync();
+    }
+}

src/Weasel.Sqlite/Tables/Table.FetchExisting.cs

@@ -19,7 +19,7 @@ SELECT sql FROM sqlite_master
 WHERE type = 'table' AND name = '{sanitizedName}';
 
 -- Get column information using PRAGMA (PRAGMA doesn't support parameter binding)
-SELECT * FROM pragma_table_info('{sanitizedName}');
+{ColumnQuery(sanitizedName)}
 
 -- Get index information
 SELECT name, sql FROM sqlite_master
@@ -30,6 +30,25 @@ SELECT sql FROM sqlite_master
 ");
     }
 
+    /// <summary>
+    /// The column introspection query, shared by <see cref="ConfigureQueryCommand" /> and
+    /// <see cref="FetchExistingAsync" /> so the two can never drift apart.
+    /// <para>
+    /// <c>table_xinfo</c> rather than <c>table_info</c>: <c>table_info</c> omits generated columns
+    /// entirely, so a table declaring one was read back without it and the delta re-added it on every
+    /// single run -- the second migration then failed with <c>duplicate column name</c> (weasel#426).
+    /// </para>
+    /// <para>
+    /// The columns are listed explicitly, and in <c>table_info</c>'s order, because
+    /// <see cref="readColumnsAsync" /> reads them positionally. <c>hidden</c> distinguishes the two:
+    /// 0 is an ordinary column, 2 and 3 are VIRTUAL and STORED generated columns (both of which we
+    /// want), and 1 is a virtual table's hidden column -- an fts5 table's, say -- which
+    /// <c>table_info</c> never showed us and which we have no business reporting as a real column.
+    /// </para>
+    /// </summary>
+    private static string ColumnQuery(string sanitizedName) =>
+        $"""SELECT cid, name, type, "notnull", dflt_value, pk FROM pragma_table_xinfo('{sanitizedName}') WHERE hidden <> 1;""";
+
     public async Task<Table?> FetchExistingAsync(SqliteConnection conn, CancellationToken ct = default)
     {
         // SQLite PRAGMAs don't support parameter binding, so we build the query directly
@@ -42,7 +61,7 @@ SELECT sql FROM sqlite_master
 WHERE type = 'table' AND name = '{tableName}';
 
 -- Get column information using PRAGMA
-SELECT * FROM pragma_table_info('{tableName}');
+{ColumnQuery(tableName)}
 
 -- Get index information
 SELECT name, sql FROM sqlite_master
@@ -118,7 +137,7 @@ private async Task readColumnsAsync(DbDataReader reader, Table existing, Cancell
     {
         var primaryKeys = new List<string>();
 
-        // PRAGMA table_info returns: cid, name, type, notnull, dflt_value, pk
+        // ColumnQuery projects: cid, name, type, notnull, dflt_value, pk
         while (await reader.ReadAsync(ct).ConfigureAwait(false))
         {
             var name = await reader.GetFieldValueAsync<string>(1, ct).ConfigureAwait(false); // name

src/Weasel.Sqlite/Tables/TableDelta.cs

@@ -170,6 +170,20 @@ private bool requiresTableRecreation()
             return true;
         }
 
+        // SQLite accepts ALTER TABLE ADD COLUMN for a VIRTUAL generated column, but rejects a STORED
+        // one outright ("cannot add a STORED column") -- it would have to compute and materialize a
+        // value for every existing row. Recreation is the only way to introduce one.
+        var renamedMissing = new HashSet<string>(
+            _renamedColumns.Select(r => r.Expected.Name), StringComparer.OrdinalIgnoreCase);
+
+        if (Columns.Missing.Any(c =>
+                c.GeneratedType == GeneratedColumnType.Stored
+                && c.GeneratedExpression.IsNotEmpty()
+                && !renamedMissing.Contains(c.Name)))
+        {
+            return true;
+        }
+
         // Check if columns being dropped are referenced by FKs or are part of the PK
         if (Columns.Extras.Any())
         {
@@ -315,6 +329,15 @@ private void writeTableRecreation(Migrator rules, TextWriter writer)
 
         foreach (var expectedCol in Expected.Columns)
         {
+            // SQLite refuses writes to a generated column, so it must stay out of the INSERT even when
+            // it exists on both sides. Its value re-derives from the base columns we do copy. Before
+            // weasel#426 this was accidentally safe -- generated columns were invisible in Actual, so
+            // they were never "in both" -- and reading them properly is what makes it necessary.
+            if (expectedCol.GeneratedExpression.IsNotEmpty())
+            {
+                continue;
+            }
+
             if (renameMap.TryGetValue(expectedCol.Name, out var oldName))
             {
                 // Renamed column: select from old name into new name