diff --git a/docs/sql-migrations.md b/docs/sql-migrations.md index 1c9d9c0c3..3accadd72 100644 --- a/docs/sql-migrations.md +++ b/docs/sql-migrations.md @@ -158,6 +158,13 @@ plan.AlterTable("users") | `.DropForeignKey(column)` | Remove foreign key constraint | | `.RenameTo(newName)` | Rename the table | +> **SQLite note:** SQLite has no native `ALTER TABLE … ALTER COLUMN` or `… ADD/DROP CONSTRAINT`, so +> `.AlterColumn(...)`, `.AddForeignKey(...)`, and `.DropForeignKey(...)` are applied by rebuilding the +> table. That rebuild runs only when the migration is applied through `MigrationManager` +> (`ApplyPendingMigrations()`); the generated `ToSql()` text for these operations is a +> `-- LIGHTWEIGHT_SQLITE_GUARD:` sentinel comment that does nothing if executed directly. Applying such +> a migration via `SqlStatement::MigrateDirect` throws rather than silently skipping the change. + **Conditional operations:** ```cpp diff --git a/src/Lightweight/QueryFormatter/PostgreSqlFormatter.hpp b/src/Lightweight/QueryFormatter/PostgreSqlFormatter.hpp index 52cb6b7c1..827f47ac5 100644 --- a/src/Lightweight/QueryFormatter/PostgreSqlFormatter.hpp +++ b/src/Lightweight/QueryFormatter/PostgreSqlFormatter.hpp @@ -16,7 +16,7 @@ class PostgreSqlFormatter final: public SQLiteQueryFormatter public: using SQLiteQueryFormatter::CreateTable; - [[nodiscard]] bool RequiresTableRebuildForForeignKeyChange() const noexcept override + [[nodiscard]] bool RequiresTableRebuildForSchemaChange() const noexcept override { return false; } diff --git a/src/Lightweight/QueryFormatter/SQLiteFormatter.hpp b/src/Lightweight/QueryFormatter/SQLiteFormatter.hpp index 332513aea..e73abfc8d 100644 --- a/src/Lightweight/QueryFormatter/SQLiteFormatter.hpp +++ b/src/Lightweight/QueryFormatter/SQLiteFormatter.hpp @@ -25,12 +25,12 @@ class SQLiteQueryFormatter: public SqlQueryFormatter } public: - /// SQLite has no native `ALTER TABLE … ADD/DROP CONSTRAINT`, so foreign-key - /// changes route through the migration executor's table-rebuild path. The - /// formatter signals that intent by emitting `-- LIGHTWEIGHT_SQLITE_GUARD:` - /// sentinels and overriding this hook so the executor takes the rebuild - /// branch instead of executing the (commented-out) sentinel script directly. - [[nodiscard]] bool RequiresTableRebuildForForeignKeyChange() const noexcept override + /// SQLite has no native `ALTER TABLE … ADD/DROP CONSTRAINT` and no `… ALTER COLUMN`, so foreign-key + /// changes and column type/nullability changes route through the migration executor's table-rebuild + /// path. The formatter signals that intent by emitting `-- LIGHTWEIGHT_SQLITE_GUARD:` sentinels and + /// overriding this hook so the executor takes the rebuild branch instead of executing the + /// (commented-out) sentinel script directly. + [[nodiscard]] bool RequiresTableRebuildForSchemaChange() const noexcept override { return true; } @@ -417,6 +417,24 @@ class SQLiteQueryFormatter: public SqlQueryFormatter } private: + /// @brief Escape an identifier for embedding inside a `"..."` field of a `LIGHTWEIGHT_SQLITE_GUARD` + /// sentinel by doubling embedded double-quotes, so a name containing `"` cannot desync the field + /// parsing on the executor side (which decodes `""` back to `"`). + /// @param identifier The raw identifier (table or column name). + /// @return The escaped text to place between the surrounding sentinel quotes. + [[nodiscard]] static std::string EscapeSentinelField(std::string_view identifier) + { + std::string out; + out.reserve(identifier.size()); + for (auto const ch: identifier) + { + out += ch; + if (ch == '"') + out += '"'; + } + return out; + } + [[nodiscard]] std::string FormatAlterTableCommand(std::string_view tableName, SqlAlterTableCommand const& command) const { auto const formatTable = [tableName]() { @@ -436,12 +454,21 @@ class SQLiteQueryFormatter: public SqlQueryFormatter ColumnType(actualCommand.columnType), actualCommand.nullable == SqlNullable::NotNull ? "NOT NULL" : "NULL"); }, - [&formatTable, this](AlterColumn const& actualCommand) -> std::string { - return std::format(R"(ALTER TABLE {} ALTER COLUMN "{}" {} {};)", - formatTable(), - actualCommand.columnName, - ColumnType(actualCommand.columnType), - actualCommand.nullable == SqlNullable::NotNull ? "NOT NULL" : "NULL"); + [&formatTable, tableName, this](AlterColumn const& actualCommand) -> std::string { + // SQLite has no `ALTER TABLE … ALTER COLUMN`. Route through the migration + // executor's table-rebuild path: emit a sentinel carrying the new column + // definition (type + nullability) that the executor recognises and applies by + // recreating the table with the modified column. The commented-out MSSQL-style + // ALTER below keeps dry-run output readable. Identifier fields are `""`-escaped so a + // name containing a double-quote cannot desync the sentinel's field parsing. + return std::format( + R"(-- LIGHTWEIGHT_SQLITE_GUARD: ALTER_COLUMN "{0}" "{1}" "{2}" "{3}" +-- ALTER TABLE {4} ALTER COLUMN "{1}" {2} {3};)", + EscapeSentinelField(tableName), + EscapeSentinelField(actualCommand.columnName), + ColumnType(actualCommand.columnType), + actualCommand.nullable == SqlNullable::NotNull ? "NOT NULL" : "NULL", + formatTable()); }, [&formatTable](RenameColumn const& actualCommand) -> std::string { return std::format(R"(ALTER TABLE {} RENAME COLUMN "{}" TO "{}";)", diff --git a/src/Lightweight/QueryFormatter/SqlServerFormatter.hpp b/src/Lightweight/QueryFormatter/SqlServerFormatter.hpp index 354278b3d..5bbc18cef 100644 --- a/src/Lightweight/QueryFormatter/SqlServerFormatter.hpp +++ b/src/Lightweight/QueryFormatter/SqlServerFormatter.hpp @@ -26,7 +26,7 @@ class SqlServerQueryFormatter final: public SQLiteQueryFormatter } public: - [[nodiscard]] bool RequiresTableRebuildForForeignKeyChange() const noexcept override + [[nodiscard]] bool RequiresTableRebuildForSchemaChange() const noexcept override { return false; } diff --git a/src/Lightweight/SqlConnection.cpp b/src/Lightweight/SqlConnection.cpp index 965f2a061..f4d5a3277 100644 --- a/src/Lightweight/SqlConnection.cpp +++ b/src/Lightweight/SqlConnection.cpp @@ -448,6 +448,11 @@ std::string SqlConnection::ServerVersion() const return GetInfoStringW(m_hDbc, SQL_DBMS_VER); } +bool SqlConnection::RequiresTableRebuildForSchemaChange() const noexcept +{ + return QueryFormatter().RequiresTableRebuildForSchemaChange(); +} + bool SqlConnection::TransactionActive() const noexcept { SQLUINTEGER state {}; diff --git a/src/Lightweight/SqlConnection.hpp b/src/Lightweight/SqlConnection.hpp index d8067ad76..29e7ff56d 100644 --- a/src/Lightweight/SqlConnection.hpp +++ b/src/Lightweight/SqlConnection.hpp @@ -138,6 +138,15 @@ class SqlConnection final /// Retrieves a query formatter suitable for the SQL server being connected. [[nodiscard]] SqlQueryFormatter const& QueryFormatter() const noexcept; + /// @brief Whether this backend must rebuild a table to apply an `ALTER TABLE` schema change it + /// cannot express in place (foreign-key add/drop, or column type/nullability change). + /// + /// The migration executor consults this to decide whether a `-- LIGHTWEIGHT_SQLITE_GUARD:` sentinel + /// must be turned into a table rebuild (`true` for SQLite) or executed directly. Exposed on the + /// connection so capability decisions go through the backend, delegating the dialect knowledge to + /// the query formatter. + [[nodiscard]] LIGHTWEIGHT_API bool RequiresTableRebuildForSchemaChange() const noexcept; + /// @brief Whether this connection's ODBC driver supports native parameter-array binding /// (`SQL_ATTR_PARAMSET_SIZE` > 1) for batched row-wise execution. /// diff --git a/src/Lightweight/SqlMigration.cpp b/src/Lightweight/SqlMigration.cpp index 3a6379e8a..d33cc353c 100644 --- a/src/Lightweight/SqlMigration.cpp +++ b/src/Lightweight/SqlMigration.cpp @@ -7,6 +7,7 @@ #include "SqlBackup/Sha256.hpp" #include "SqlConnection.hpp" #include "SqlErrorDetection.hpp" +#include "SqlLogger.hpp" #include "SqlMigration.hpp" #include "SqlSchema.hpp" #include "SqlTransaction.hpp" @@ -689,6 +690,7 @@ namespace AddForeignKey, DropForeignKey, AddCompositeForeignKey, + AlterColumn, }; Kind kind; @@ -700,6 +702,9 @@ namespace // Only populated for Kind::AddCompositeForeignKey. Empty otherwise. std::vector columns; std::vector referencedColumns; + // Only populated for Kind::AlterColumn. Empty / false otherwise. + std::string columnType; + bool notNull = false; }; [[nodiscard]] std::optional ParseSqliteGuardKind(std::string_view kindStr) @@ -714,25 +719,50 @@ namespace return SqliteGuard::Kind::DropForeignKey; if (kindStr == "ADD_COMPOSITE_FOREIGN_KEY") return SqliteGuard::Kind::AddCompositeForeignKey; + if (kindStr == "ALTER_COLUMN") + return SqliteGuard::Kind::AlterColumn; return std::nullopt; } - [[nodiscard]] std::optional> ExtractQuotedStrings(std::string_view directive, - size_t searchPos, - size_t expectedStrings) + /// Extract @p expectedStrings double-quoted fields from a sentinel directive, starting at + /// @p searchPos. A doubled `""` inside a field is treated as an escaped quote and decoded to a + /// single `"`, so identifiers containing a double-quote survive the round-trip (the formatter + /// escapes them the same way). Returns the decoded field values, or `std::nullopt` if fewer than + /// @p expectedStrings well-formed fields are present. + [[nodiscard]] std::optional> ExtractQuotedStrings(std::string_view directive, + size_t searchPos, + size_t expectedStrings) { - std::vector quoted; + std::vector quoted; quoted.reserve(expectedStrings); while (quoted.size() < expectedStrings) { auto const openQuote = directive.find('"', searchPos); if (openQuote == std::string_view::npos) return std::nullopt; - auto const closeQuote = directive.find('"', openQuote + 1); - if (closeQuote == std::string_view::npos) + std::string value; + size_t scan = openQuote + 1; + bool closed = false; + while (scan < directive.size()) + { + if (directive[scan] == '"') + { + if (scan + 1 < directive.size() && directive[scan + 1] == '"') + { + value += '"'; // doubled-quote escape inside the field + scan += 2; + continue; + } + closed = true; + break; + } + value += directive[scan]; + ++scan; + } + if (!closed) return std::nullopt; - quoted.push_back(directive.substr(openQuote + 1, closeQuote - openQuote - 1)); - searchPos = closeQuote + 1; + quoted.push_back(std::move(value)); + searchPos = scan + 1; } return quoted; } @@ -753,7 +783,7 @@ namespace return result; } - [[nodiscard]] SqliteGuard BuildSqliteGuard(SqliteGuard::Kind kind, std::span quoted) + [[nodiscard]] SqliteGuard BuildSqliteGuard(SqliteGuard::Kind kind, std::span quoted) { SqliteGuard guard { .kind = kind, @@ -763,6 +793,8 @@ namespace .referencedColumn = {}, .columns = {}, .referencedColumns = {}, + .columnType = {}, + .notNull = false, }; if (kind == SqliteGuard::Kind::AddForeignKey) { @@ -776,6 +808,11 @@ namespace guard.referencedTable = std::string { quoted[2] }; guard.referencedColumns = SplitCommaList(quoted[3]); } + else if (kind == SqliteGuard::Kind::AlterColumn) + { + guard.columnType = std::string { quoted[2] }; + guard.notNull = quoted[3] == "NOT NULL"; + } return guard; } @@ -810,9 +847,13 @@ namespace // Parse N quoted identifiers after the kind keyword. Single-column kinds take // exactly 2 quoted args (table, column); AddForeignKey takes 4 (adds referenced // table, referenced column); AddCompositeForeignKey also takes 4 but the 2nd - // and 4th are comma-joined column lists split by the consumer. + // and 4th are comma-joined column lists split by the consumer; AlterColumn takes + // 4 (adds the new column type and the NULL/NOT NULL token). size_t const expectedStrings = - (*kind == SqliteGuard::Kind::AddForeignKey || *kind == SqliteGuard::Kind::AddCompositeForeignKey) ? 4 : 2; + (*kind == SqliteGuard::Kind::AddForeignKey || *kind == SqliteGuard::Kind::AddCompositeForeignKey + || *kind == SqliteGuard::Kind::AlterColumn) + ? 4 + : 2; auto const quoted = ExtractQuotedStrings(directive, kindEnd, expectedStrings); if (!quoted) return std::nullopt; @@ -833,26 +874,83 @@ namespace return cursor.GetColumn(1) > 0; } + /// @brief Escape a value for embedding inside a single-quoted SQL string literal (doubles `'`). + /// @param value The raw value (e.g. a table name) to embed. + /// @return The escaped text, to be placed between the surrounding single quotes. + [[nodiscard]] std::string EscapeSqlStringLiteral(std::string_view value) + { + std::string out; + out.reserve(value.size()); + for (auto const ch: value) + { + out += ch; + if (ch == '\'') + out += '\''; + } + return out; + } + + /// @brief Escape a value for embedding inside a double-quoted SQL identifier (doubles `"`). + /// @param value The raw identifier (e.g. a table name) to embed. + /// @return The escaped text, to be placed between the surrounding double quotes. + [[nodiscard]] std::string EscapeSqlIdentifier(std::string_view value) + { + std::string out; + out.reserve(value.size()); + for (auto const ch: value) + { + out += ch; + if (ch == '"') + out += '"'; + } + return out; + } + /// Fetch the stored `CREATE TABLE` SQL for a SQLite table. [[nodiscard]] std::string FetchSqliteCreateTableSql(SqlStatement& stmt, std::string_view tableName) { - auto cursor = - stmt.ExecuteDirect(std::format(R"(SELECT sql FROM sqlite_schema WHERE type='table' AND name='{}')", tableName)); + auto cursor = stmt.ExecuteDirect(std::format(R"(SELECT sql FROM sqlite_schema WHERE type='table' AND name='{}')", + EscapeSqlStringLiteral(tableName))); if (!cursor.FetchRow()) throw std::runtime_error(std::format("SQLite rebuild: table '{}' not found in sqlite_schema", tableName)); return cursor.GetColumn(1); } - /// Fetch the column names of a SQLite table in declared order. + /// Fetch the insertable column names of a SQLite table in declared order. + /// + /// Uses `table_xinfo` so generated columns can be excluded: its `hidden` flag is 0 for an ordinary + /// column, 1 for a hidden column, and 2/3 for VIRTUAL/STORED generated columns. Only ordinary + /// columns may appear in an `INSERT`, so the rebuild's data-copy column list must skip the rest. [[nodiscard]] std::vector FetchSqliteColumnNames(SqlStatement& stmt, std::string_view tableName) { - auto cursor = stmt.ExecuteDirect(std::format(R"(PRAGMA table_info("{}"))", tableName)); + auto cursor = stmt.ExecuteDirect(std::format(R"(PRAGMA table_xinfo("{}"))", EscapeSqlIdentifier(tableName))); std::vector columns; while (cursor.FetchRow()) - columns.push_back(cursor.GetColumn(2)); // column 2 is `name` + // table_xinfo columns: 1=cid 2=name 3=type 4=notnull 5=dflt_value 6=pk 7=hidden. + if (cursor.GetColumn(7) == 0) // ordinary (insertable) column + columns.push_back(cursor.GetColumn(2)); return columns; } + /// @brief Fetch the DDL of explicitly-created indexes and triggers attached to a SQLite table. + /// + /// These live in their own `sqlite_schema` rows and are destroyed by `DROP TABLE`, so a table + /// rebuild must re-create them afterwards. Auto-indexes that back inline `UNIQUE` / `PRIMARY KEY` + /// constraints have a NULL `sql` and are excluded — SQLite recreates those with the table itself. + /// @param stmt A statement bound to the connection being rebuilt. + /// @param tableName The table whose dependent objects to collect. + /// @return The `CREATE INDEX` / `CREATE TRIGGER` statements in `sqlite_schema` order. + [[nodiscard]] std::vector FetchSqliteDependentObjectsSql(SqlStatement& stmt, std::string_view tableName) + { + auto cursor = stmt.ExecuteDirect(std::format( + R"(SELECT sql FROM sqlite_schema WHERE tbl_name = '{}' AND type IN ('index', 'trigger') AND sql IS NOT NULL)", + EscapeSqlStringLiteral(tableName))); + std::vector dependentObjectsSql; + while (cursor.FetchRow()) + dependentObjectsSql.push_back(cursor.GetColumn(1)); + return dependentObjectsSql; + } + /// Rebuild a SQLite table while transforming its stored `CREATE TABLE` SQL. /// /// The table rebuild follows the SQLite-recommended recipe: @@ -863,12 +961,15 @@ namespace /// /// The caller supplies `transformCreateSql` which receives the original SQL (with the /// table name already substituted for the temp name) and returns the final CREATE TABLE - /// text. The migration transaction covers all four steps for atomicity. + /// text. Explicitly-created indexes and triggers are captured beforehand and re-created + /// afterwards. The migration transaction covers every step for atomicity. /// - /// Assumes SQLite's default `foreign_keys = OFF` session setting, which is standard - /// during migrations — with enforcement on, `DROP TABLE orig` would succeed but the - /// brief interval between DROP and RENAME would leave FKs in other tables temporarily - /// dangling. + /// Foreign-key enforcement is NOT toggled here: SQLite only honours `PRAGMA foreign_keys` + /// outside a transaction, and migrations run inside one. The caller must therefore ensure + /// `foreign_keys = OFF` before migrating a table that other tables reference — with enforcement + /// on, the `DROP TABLE` can fire cascade actions or leave another table's FK dangling. (The + /// proper fix is to disable `foreign_keys` at the migration-runner level, before the + /// transaction is opened, gated on SQLite.) void RebuildSqliteTable(SqlConnection& connection, std::string_view tableName, auto&& transformCreateSql) { auto stmt = SqlStatement { connection }; @@ -877,19 +978,41 @@ namespace if (columns.empty()) throw std::runtime_error(std::format("SQLite rebuild: table '{}' has no columns", tableName)); + // Capture indexes/triggers before the drop destroys them; they are re-created after RENAME. + auto const dependentObjectsSql = FetchSqliteDependentObjectsSql(stmt, tableName); + std::string const tmpName = std::string { tableName } + "__lw_rebuild"; - // Substitute the original table name with the temp name in the stored SQL. The - // quoted form is the one Lightweight emits; the unquoted fallback covers tables - // created outside the library. - auto replaceFirst = [](std::string s, std::string_view from, std::string_view to) { - if (auto const pos = s.find(from); pos != std::string::npos) - s.replace(pos, from.size(), to); + // Substitute the original table name with the temp name in the stored SQL. Prefer the quoted + // form Lightweight emits. For tables created outside the library, fall back to a *word-boundary* + // match of the bare name so a short name (e.g. "T", a substring of "CREATE"/"TABLE") is not + // spliced into a keyword, a column name, or a self-referential REFERENCES clause. Only the first + // matching occurrence — the table declaration — is replaced. + auto const isIdentifierChar = [](char c) noexcept { + return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_'; + }; + auto replaceFirstWord = [&isIdentifierChar](std::string s, std::string_view word, std::string_view to) { + size_t from = 0; + while ((from = s.find(word, from)) != std::string::npos) + { + bool const leftOk = from == 0 || !isIdentifierChar(s[from - 1]); + size_t const after = from + word.size(); + bool const rightOk = after >= s.size() || !isIdentifierChar(s[after]); + if (leftOk && rightOk) + { + s.replace(from, word.size(), to); + break; + } + from = after; + } return s; }; - auto withTmpName = replaceFirst(originalSql, std::format(R"("{}")", tableName), std::format(R"("{}")", tmpName)); - if (withTmpName == originalSql) // unquoted fallback - withTmpName = replaceFirst(originalSql, tableName, tmpName); + auto const quotedName = std::format(R"("{}")", tableName); + std::string withTmpName = originalSql; + if (auto const pos = withTmpName.find(quotedName); pos != std::string::npos) + withTmpName.replace(pos, quotedName.size(), std::format(R"("{}")", tmpName)); + else // unquoted fallback for externally-created tables + withTmpName = replaceFirstWord(originalSql, tableName, tmpName); auto const newSql = transformCreateSql(std::move(withTmpName)); @@ -910,6 +1033,8 @@ namespace } }; + // Clear any leftover temp table from a previously-failed rebuild so the CREATE can't collide. + exec(std::format(R"(DROP TABLE IF EXISTS "{}")", tmpName), "DROP stale tmp"); exec(newSql, "CREATE TABLE (tmp)"); std::string columnList; @@ -923,6 +1048,11 @@ namespace "INSERT SELECT"); exec(std::format(R"(DROP TABLE "{}")", tableName), "DROP TABLE"); exec(std::format(R"(ALTER TABLE "{}" RENAME TO "{}")", tmpName, tableName), "ALTER TABLE RENAME"); + + // Re-create the indexes and triggers dropped with the original table. Their stored DDL + // names the table by its (now restored) original name, so it applies unchanged. + for (auto const& dependentObjectSql: dependentObjectsSql) + exec(dependentObjectSql, "recreate dependent object"); } /// Rebuild a SQLite table to add a new foreign key constraint. @@ -982,7 +1112,87 @@ namespace }); } + /// @brief Advance past a single-quoted SQL string literal. + /// Handles doubled-quote (`''`) escapes. If @p from is not at a quote, returns it unchanged so + /// the caller can keep scanning ordinary text. + /// @param s The text being scanned. + /// @param from Index to inspect. + /// @return Index just past the literal's closing quote, or `s.size()` if it is unterminated. + [[nodiscard]] size_t SkipStringLiteral(std::string_view s, size_t from) noexcept + { + if (from >= s.size() || s[from] != '\'') + return from; + size_t scan = from + 1; + while (scan < s.size()) + { + if (s[scan] == '\'') + { + if (scan + 1 < s.size() && s[scan + 1] == '\'') + { + scan += 2; // doubled-quote escape inside the literal + continue; + } + return scan + 1; + } + ++scan; + } + return s.size(); + } + + /// @brief Advance past a double-quoted SQL identifier (`"..."`), honouring `""` escapes. + /// If @p from is not at a double quote, returns it unchanged so the caller can keep scanning. + /// Quoted identifiers can legally contain `'`, `(`, `)`, and `,`, so treating them as opaque keeps + /// those characters from being misread as string-literal, paren, or column-list structure. + /// @param s The text being scanned. + /// @param from Index to inspect. + /// @return Index just past the identifier's closing quote, or `s.size()` if it is unterminated. + [[nodiscard]] size_t SkipQuotedIdentifier(std::string_view s, size_t from) noexcept + { + if (from >= s.size() || s[from] != '"') + return from; + size_t scan = from + 1; + while (scan < s.size()) + { + if (s[scan] == '"') + { + if (scan + 1 < s.size() && s[scan + 1] == '"') + { + scan += 2; // doubled-quote escape inside the identifier + continue; + } + return scan + 1; + } + ++scan; + } + return s.size(); + } + + /// @brief Decode a double-quoted SQL identifier into its raw name (collapsing `""` to `"`). + /// @param s The text containing the identifier. + /// @param openQuote Index of the opening `"`. + /// @param pastClose Index just past the closing `"` (as returned by @ref SkipQuotedIdentifier). + /// @return The unquoted, unescaped identifier text. + [[nodiscard]] std::string UnquoteSqlIdentifier(std::string_view s, size_t openQuote, size_t pastClose) + { + std::string out; + size_t const closeQuote = pastClose - 1; // index of the closing '"' + size_t k = openQuote + 1; + while (k < closeQuote) + { + if (s[k] == '"' && k + 1 < closeQuote && s[k + 1] == '"') + { + out += '"'; + k += 2; + } + else + out += s[k++]; + } + return out; + } + /// Advance past a `(...)` group starting at the first `(` at or after `from`. + /// Single-quoted string literals and double-quoted identifiers inside the group are skipped whole + /// so a `)` within a literal or identifier cannot unbalance the depth count. /// Returns the index just past the matching close paren, or `std::string::npos` /// if the text is malformed. [[nodiscard]] size_t SkipMatchingParens(std::string_view s, size_t from) @@ -994,6 +1204,16 @@ namespace size_t scan = open + 1; while (scan < s.size() && depth > 0) { + if (s[scan] == '\'') + { + scan = SkipStringLiteral(s, scan); + continue; + } + if (s[scan] == '"') + { + scan = SkipQuotedIdentifier(s, scan); + continue; + } if (s[scan] == '(') ++depth; else if (s[scan] == ')') @@ -1003,6 +1223,37 @@ namespace return depth == 0 ? scan : std::string_view::npos; } + /// @brief Find the next whitespace-delimited token in @p s starting at @p from, treating + /// parenthesised groups, single-quoted string literals, and double-quoted identifiers as opaque + /// (so commas, parens, quotes, and keywords inside them are never split out). + /// @param s The text being tokenised. + /// @param from Index to start scanning from. + /// @return The token's `[start, end)` range; an empty range `{s.size(), s.size()}` when none remains. + [[nodiscard]] std::pair NextSqlToken(std::string_view s, size_t from) + { + size_t i = from; + while (i < s.size() && (s[i] == ' ' || s[i] == '\t' || s[i] == '\n')) + ++i; + if (i >= s.size()) + return { s.size(), s.size() }; + size_t const start = i; + while (i < s.size() && s[i] != ' ' && s[i] != '\t' && s[i] != '\n') + { + if (s[i] == '(') + { + auto const end = SkipMatchingParens(s, i); + i = end == std::string_view::npos ? s.size() : end; + } + else if (s[i] == '\'') + i = SkipStringLiteral(s, i); + else if (s[i] == '"') + i = SkipQuotedIdentifier(s, i); + else + ++i; + } + return { start, i }; + } + /// Expand `[start..pos)` backwards to include adjacent whitespace and up to one /// trailing comma — so erasing `[newStart..end)` from a column list also removes /// the separator preceding the clause. @@ -1060,6 +1311,375 @@ namespace }); } + /// @brief ASCII-fold a character to upper case (locale-independent — SQL keywords are ASCII). + [[nodiscard]] constexpr char AsciiFold(char c) noexcept + { + return (c >= 'a' && c <= 'z') ? static_cast(c - ('a' - 'A')) : c; + } + + /// @brief ASCII case-insensitive string equality, without allocating. + /// @param a First string. + /// @param b Second string. + /// @return True iff @p a and @p b are equal ignoring ASCII letter case. + [[nodiscard]] bool IEqualsAscii(std::string_view a, std::string_view b) noexcept + { + return a.size() == b.size() && std::ranges::equal(a, b, [](char x, char y) { return AsciiFold(x) == AsciiFold(y); }); + } + + /// @brief Whether @p text contains @p keyword as a standalone ASCII case-insensitive token. + /// Unlike a substring search this only matches a real SQL token: @ref NextSqlToken treats + /// parenthesised groups, string literals, and quoted identifiers as opaque, so the keyword is + /// never matched inside a `DEFAULT 'value'`, a `CHECK(...)` expression, or an identifier. + /// @param text The text to search. + /// @param keyword The keyword to look for. + /// @return True iff @p keyword occurs in @p text as a token, ignoring ASCII letter case. + [[nodiscard]] bool ContainsTokenAscii(std::string_view text, std::string_view keyword) + { + size_t scan = 0; + while (true) + { + auto const [start, end] = NextSqlToken(text, scan); + if (start == end) + return false; + if (IEqualsAscii(text.substr(start, end - start), keyword)) + return true; + scan = end; + } + } + + /// @brief Drop the nullability keywords (`NOT NULL`, standalone `NULL`) from a column's inline + /// constraint list so a fresh nullability can be applied. A `NULL` that belongs to a + /// `DEFAULT NULL` clause is normally preserved; when @p makingNotNull is set the now contradictory + /// default is dropped as well, in all three spellings: `DEFAULT NULL`, `DEFAULT (NULL)`, and the + /// space-less `DEFAULT(NULL)` (which the tokeniser collapses into a single token). Parenthesised + /// groups (`CHECK(...)`), single-quoted string literals, and double-quoted identifiers are treated + /// as opaque so commas, quotes, and keywords inside them are never misread. + /// @param constraints The inline constraint text following a column's type token. + /// @param makingNotNull Whether the column is being changed to `NOT NULL`. + /// @return The constraint text with the nullability keywords removed. + /// + /// @note SQLite necessarily reconstructs the whole column here, so it normalises a contradictory + /// `DEFAULT NULL` away. The in-place `ALTER COLUMN` emitted for PostgreSQL / SQL Server leaves the + /// existing default untouched; that cross-dialect difference is intentional — only SQLite has to + /// re-render the column definition from scratch. + [[nodiscard]] std::string StripNullabilityKeywords(std::string_view constraints, bool makingNotNull) + { + std::vector tokens; // views into `constraints` — no per-token allocation + size_t scan = 0; + while (true) + { + auto const [start, end] = NextSqlToken(constraints, scan); + if (start == end) + break; + tokens.push_back(constraints.substr(start, end - start)); + scan = end; + } + + auto const isNullToken = [](std::string_view tok) { + return IEqualsAscii(tok, "NULL") || IEqualsAscii(tok, "(NULL)"); + }; + // A `DEFAULT(NULL)` written without a space is collapsed into one token by NextSqlToken. + auto const isDefaultNullToken = [](std::string_view tok) { + return IEqualsAscii(tok, "DEFAULT(NULL)"); + }; + + std::vector kept; + kept.reserve(tokens.size()); + size_t t = 0; + while (t < tokens.size()) + { + if (IEqualsAscii(tokens[t], "NOT") && t + 1 < tokens.size() && IEqualsAscii(tokens[t + 1], "NULL")) + { + t += 2; // skip `NOT NULL` + continue; + } + if (makingNotNull && IEqualsAscii(tokens[t], "DEFAULT") && t + 1 < tokens.size() && isNullToken(tokens[t + 1])) + { + t += 2; // a NULL default contradicts NOT NULL — drop the whole `DEFAULT NULL` clause + continue; + } + if (makingNotNull && isDefaultNullToken(tokens[t])) + { + ++t; // same, for the space-less `DEFAULT(NULL)` spelling + continue; + } + if (IEqualsAscii(tokens[t], "NULL") && (kept.empty() || !IEqualsAscii(kept.back(), "DEFAULT"))) + { + ++t; + continue; + } + kept.push_back(tokens[t]); + ++t; + } + + std::string out; + for (auto const& token: kept) + { + if (!out.empty()) + out += ' '; + out += token; + } + return out; + } + + /// @brief Find the byte offset of the column-list opening parenthesis in a `CREATE TABLE` statement. + /// Scans top-level text only, skipping quoted identifiers and string literals, so a `(` inside a + /// quoted name or a string literal is never mistaken for the column-list opener. + /// @param createSql The stored `CREATE TABLE` text. + /// @return The index of the `(` that opens the column list, or `std::string::npos` if none exists. + [[nodiscard]] size_t FindColumnListOpen(std::string_view createSql) noexcept + { + size_t pos = 0; + while (pos < createSql.size()) + { + char const ch = createSql[pos]; + if (ch == '"') + pos = SkipQuotedIdentifier(createSql, pos); + else if (ch == '\'') + pos = SkipStringLiteral(createSql, pos); + else if (ch == '(') + return pos; + else + ++pos; + } + return std::string::npos; + } + + /// @brief Test whether a column-list entry defines a given column, i.e. its first + /// whitespace-skipped token is the quoted column name. + /// @param createSql The stored `CREATE TABLE` text. + /// @param entryStart Index of the entry's first character. + /// @param entryEnd Index just past the entry (its terminating comma or the list's `)`). + /// @param columnName The column name to match. + /// @return The index just past the entry's quoted name when it defines @p columnName; otherwise + /// `std::string::npos`. + [[nodiscard]] size_t MatchColumnEntryName(std::string_view createSql, + size_t entryStart, + size_t entryEnd, + std::string_view columnName) + { + size_t nameStart = entryStart; + while (nameStart < entryEnd + && (createSql[nameStart] == ' ' || createSql[nameStart] == '\t' || createSql[nameStart] == '\n')) + ++nameStart; + if (nameStart >= entryEnd || createSql[nameStart] != '"') + return std::string::npos; + size_t const nameEnd = SkipQuotedIdentifier(createSql, nameStart); + if (nameEnd <= entryEnd && UnquoteSqlIdentifier(createSql, nameStart, nameEnd) == columnName) + return nameEnd; + return std::string::npos; + } + + /// @brief Locate a column's definition as a top-level entry of a `CREATE TABLE` column list. + /// The list entries are separated by top-level commas; a column definition is an entry whose first + /// token is the quoted column name. Nested parens, string literals, and quoted identifiers are + /// skipped so the match never lands on a `CHECK("name" > 0)`, a `REFERENCES "T"("name")`, or a + /// `DEFAULT '... "name" ...'` that merely mentions the name. + /// @param createSql The stored `CREATE TABLE` text. + /// @param listOpen The index of the column list's opening `(` (see @ref FindColumnListOpen). + /// @param columnName The column to locate. + /// @return `{afterName, defEnd}` — the index just past the column's quoted name and the index of + /// the entry's terminating comma or the list's `)`; `{npos, npos}` if the column is not found. + [[nodiscard]] std::pair LocateColumnDefinition(std::string_view createSql, + size_t listOpen, + std::string_view columnName) + { + size_t entryStart = listOpen + 1; + size_t pos = entryStart; + int depth = 0; + while (pos < createSql.size()) + { + char const ch = createSql[pos]; + if (ch == '"') + { + pos = SkipQuotedIdentifier(createSql, pos); + continue; + } + if (ch == '\'') + { + pos = SkipStringLiteral(createSql, pos); + continue; + } + if (ch == '(') + { + ++depth; + ++pos; + continue; + } + if (ch == ')' && depth > 0) + { + --depth; + ++pos; + continue; + } + + bool const isEntryComma = ch == ',' && depth == 0; + bool const isListEnd = ch == ')' && depth == 0; + if (isEntryComma || isListEnd) + { + if (size_t const afterName = MatchColumnEntryName(createSql, entryStart, pos, columnName); + afterName != std::string::npos) + return { afterName, pos }; + if (isListEnd) + break; + entryStart = pos + 1; + } + ++pos; + } + return { std::string::npos, std::string::npos }; + } + + /// Rewrite a single column's type and nullability inside a stored `CREATE TABLE` statement, + /// preserving the column's other inline constraints (PRIMARY KEY, UNIQUE, DEFAULT, …) and + /// every other column verbatim. Backs the SQLite `AlterColumn` table rebuild. + /// + /// @param createSql The stored `CREATE TABLE` text (temp-table-named by the rebuild helper). + /// @param columnName The column to alter. + /// @param newType The new column type (already formatted, e.g. `TEXT` or `VARCHAR(200)`). + /// @param notNull Whether the column should become `NOT NULL`. + /// @return The transformed `CREATE TABLE` text. + [[nodiscard]] std::string RewriteSqliteColumnDefinition(std::string createSql, + std::string_view columnName, + std::string_view newType, + bool notNull) + { + // Locate the target column's definition as a top-level entry of the column list, rather than by + // a naive substring search for `"name"`. The column list is the first top-level `(...)` group; + // its entries are separated by top-level commas, and a column definition is an entry whose first + // token is the quoted column name. Walking the structure (skipping nested parens, string + // literals, and quoted identifiers) keeps the match off a `CHECK("name" > 0)` expression, a + // `REFERENCES "T"("name")` clause, a `DEFAULT '... "name" ...'` literal, or any other column + // that merely mentions the name. + // + // We deliberately rewrite the stored DDL as text rather than reconstruct the column from PRAGMA + // metadata: PRAGMA cannot faithfully round-trip CHECK / COLLATE / generated-column / inline-FK + // clauses, all of which this text surgery preserves verbatim. + size_t const listOpen = FindColumnListOpen(createSql); + if (listOpen == std::string::npos) + throw std::runtime_error( + std::format(R"(SQLite rebuild: no column list found in CREATE TABLE for "{}")", columnName)); + + // `afterName` sits just past the target column's quoted name; `defEnd` at the entry's + // terminating comma or the list's ')'. + auto const [afterName, defEnd] = LocateColumnDefinition(createSql, listOpen, columnName); + if (afterName == std::string::npos) + throw std::runtime_error(std::format(R"(SQLite rebuild: column "{}" not found in CREATE TABLE)", columnName)); + + // Definition body after the column name: ` [type-params] `. The type token + // is the first whitespace-delimited token, with any `(...)` parameter group attached (NextSqlToken + // treats the group as opaque); the remainder is the surviving constraints. + auto const body = std::string_view { createSql }.substr(afterName, defEnd - afterName); + auto const [typeStart, typeEnd] = NextSqlToken(body, 0); + std::string const rest = + typeStart < typeEnd ? StripNullabilityKeywords(body.substr(typeEnd), notNull) : std::string {}; + + // SQLite only allows `AUTOINCREMENT` on an `INTEGER PRIMARY KEY` column. Changing such a column + // to any other type would emit DDL SQLite rejects mid-rebuild, so fail early with a clear, + // actionable message. The check is token-based so it is not fooled by the text `AUTOINCREMENT` + // appearing inside a DEFAULT literal or an identifier. + if (!IEqualsAscii(newType, "INTEGER") && ContainsTokenAscii(rest, "AUTOINCREMENT")) + throw std::runtime_error(std::format(R"(SQLite rebuild: cannot change AUTOINCREMENT column "{}" to type "{}" )" + R"((AUTOINCREMENT requires INTEGER PRIMARY KEY))", + columnName, + newType)); + + // Reassemble ` [NOT NULL] ` in place of the old definition + // body. `afterName` sits at the closing quote of the name, so a single leading space is included. + std::string rebuilt = " "; + rebuilt += newType; + if (notNull) + rebuilt += " NOT NULL"; + if (!rest.empty()) + { + rebuilt += ' '; + rebuilt += rest; + } + + createSql.replace(afterName, defEnd - afterName, rebuilt); + return createSql; + } + + /// Rebuild a SQLite table to change a column's type and/or nullability. + /// + /// SQLite has no `ALTER TABLE … ALTER COLUMN`, so the executor recreates the table from its + /// stored `CREATE TABLE` definition with the one column's type/nullability rewritten, then + /// copies the data across. Other columns, constraints, indexes, and triggers are preserved + /// (indexes/triggers via @ref RebuildSqliteTable's capture-and-recreate step). + void SqliteRebuildAlterColumn(SqlConnection& connection, SqliteGuard const& guard) + { + RebuildSqliteTable(connection, guard.tableName, [&](std::string createSql) { + return RewriteSqliteColumnDefinition(std::move(createSql), guard.columnName, guard.columnType, guard.notNull); + }); + } + + /// @brief RAII helper that disables SQLite foreign-key enforcement for the duration of a migration, + /// then restores it. A table rebuild's `DROP TABLE` would otherwise fire cascade actions or leave + /// another table's foreign key dangling when enforcement is on. + /// + /// SQLite only honours `PRAGMA foreign_keys` *outside* a transaction, and migrations run inside one, + /// so the toggle must bracket the transaction: construct this before the `SqlTransaction` and let it + /// outlive it (locals are destroyed in reverse order). It is a no-op on non-SQLite backends and when + /// enforcement is already off (the SQLite default), so the common case is untouched. + class SqliteForeignKeysGuard + { + public: + /// @param connection The connection whose foreign-key enforcement to toggle. + explicit SqliteForeignKeysGuard(SqlConnection& connection): + _connection { connection } + { + if (!connection.RequiresTableRebuildForSchemaChange()) + return; + bool enabled = false; + { + // Read the current value in its own scope so the result cursor is released before the + // `SET` below: reusing a statement while its cursor is still open is an invalid-cursor- + // state error on the SQLite ODBC driver. + auto stmt = SqlStatement { connection }; + auto cursor = stmt.ExecuteDirect("PRAGMA foreign_keys"); + enabled = cursor.FetchRow() && cursor.GetColumn(1) != 0; + } + if (enabled) + { + auto stmt = SqlStatement { connection }; + (void) stmt.ExecuteDirect("PRAGMA foreign_keys = OFF"); + _restore = true; + } + } + + ~SqliteForeignKeysGuard() + { + if (!_restore) + return; + // Destructors must not propagate. Route a failed restore to the active SqlLogger so a + // silently-left-disabled foreign-key enforcement doesn't become an invisible regression. + try + { + auto stmt = SqlStatement { _connection }; + (void) stmt.ExecuteDirect("PRAGMA foreign_keys = ON"); + } + catch (...) + { + try + { + SqlLogger::GetLogger().OnWarning("SqliteForeignKeysGuard: failed to restore PRAGMA foreign_keys = ON"); + } + // NOLINTNEXTLINE(bugprone-empty-catch) — destructor must never throw. + catch (...) + { + } + } + } + + SqliteForeignKeysGuard(SqliteForeignKeysGuard const&) = delete; + SqliteForeignKeysGuard& operator=(SqliteForeignKeysGuard const&) = delete; + SqliteForeignKeysGuard(SqliteForeignKeysGuard&&) = delete; + SqliteForeignKeysGuard& operator=(SqliteForeignKeysGuard&&) = delete; + + private: + SqlConnection& _connection; + bool _restore = false; + }; + /// Execute a SQL script that may be prefixed with a SQLite runtime-guard sentinel. /// /// If the script carries a guard, perform the presence check first and skip the DDL @@ -1068,7 +1688,7 @@ namespace void ExecuteScriptRespectingSqliteGuards(SqlStatement& stmt, SqlConnection& connection, std::string_view script) { auto const parsed = TryParseSqliteGuard(script); - if (!parsed || !connection.QueryFormatter().RequiresTableRebuildForForeignKeyChange()) + if (!parsed || !connection.RequiresTableRebuildForSchemaChange()) { (void) stmt.ExecuteDirect(script); return; @@ -1097,6 +1717,9 @@ namespace case SqliteGuard::Kind::AddCompositeForeignKey: SqliteRebuildAddCompositeForeignKey(connection, guard); return; + case SqliteGuard::Kind::AlterColumn: + SqliteRebuildAlterColumn(connection, guard); + return; } } } // namespace @@ -1218,6 +1841,9 @@ void MigrationManager::ApplySingleMigration(MigrationBase const& migration, Migr } auto& dm = GetDataMapper(); + // Disable SQLite FK enforcement around the transaction so a table rebuild's DROP TABLE cannot + // cascade or leave a referencing table's FK dangling (no-op off SQLite / when already disabled). + auto foreignKeysGuard = SqliteForeignKeysGuard { dm.Connection() }; auto transaction = SqlTransaction { dm.Connection(), SqlTransactionMode::ROLLBACK }; SqlMigrationQueryBuilder migrationBuilder = dm.Connection().Migration(); @@ -1283,6 +1909,9 @@ void MigrationManager::RevertSingleMigration(MigrationBase const& migration) } auto& dm = GetDataMapper(); + // Disable SQLite FK enforcement around the transaction so a table rebuild's DROP TABLE cannot + // cascade or leave a referencing table's FK dangling (no-op off SQLite / when already disabled). + auto foreignKeysGuard = SqliteForeignKeysGuard { dm.Connection() }; auto transaction = SqlTransaction { dm.Connection(), SqlTransactionMode::ROLLBACK }; SqlMigrationQueryBuilder migrationBuilder = dm.Connection().Migration(); diff --git a/src/Lightweight/SqlQuery/Migrate.hpp b/src/Lightweight/SqlQuery/Migrate.hpp index 70fd24255..ab7861ca2 100644 --- a/src/Lightweight/SqlQuery/Migrate.hpp +++ b/src/Lightweight/SqlQuery/Migrate.hpp @@ -140,14 +140,22 @@ class [[nodiscard]] SqlAlterTableQueryBuilder final /// /// @see SqlColumnTypeDefinition /// + /// @note On SQLite, `ALTER COLUMN` has no native SQL form: it is applied by rebuilding the table, + /// which only happens through @ref SqlMigration::MigrationManager. Executing the generated `ToSql()` + /// text yourself (the formatter emits a `-- LIGHTWEIGHT_SQLITE_GUARD:` sentinel comment for SQLite) + /// would silently do nothing — apply the migration via the manager, as shown below. + /// /// @code - /// auto stmt = SqlStatement(); - /// auto sqlMigration = stmt.Migration() - /// .AlterTable("Table") - /// .AlterColumn("column", Integer {}, SqlNullable::NotNull) - /// .GetPlan().ToSql(); - /// for (auto const& sql: sqlMigration) - /// stmt.ExecuteDirect(sql); + /// auto widen = SqlMigration::Migration<202602010001>( + /// "widen column", + /// [](SqlMigrationQueryBuilder& plan) { + /// plan.AlterTable("Table").AlterColumn("column", Integer {}, SqlNullable::NotNull); + /// }, + /// [](SqlMigrationQueryBuilder&) {}); + /// + /// auto& manager = SqlMigration::MigrationManager::GetInstance(); + /// manager.CreateMigrationHistory(); + /// manager.ApplyPendingMigrations(); /// @endcode LIGHTWEIGHT_API SqlAlterTableQueryBuilder& AlterColumn(std::string columnName, SqlColumnTypeDefinition columnType, diff --git a/src/Lightweight/SqlQueryFormatter.hpp b/src/Lightweight/SqlQueryFormatter.hpp index 3eb9f7005..e190baa59 100644 --- a/src/Lightweight/SqlQueryFormatter.hpp +++ b/src/Lightweight/SqlQueryFormatter.hpp @@ -178,13 +178,14 @@ class [[nodiscard]] LIGHTWEIGHT_API SqlQueryFormatter /// Retrieves the SQL query formatter for the given SqlServerType. static SqlQueryFormatter const* Get(SqlServerType serverType) noexcept; - /// @brief Whether the dialect must rebuild a table to add or drop a foreign-key - /// constraint (i.e. cannot express it via `ALTER TABLE … ADD/DROP CONSTRAINT`). + /// @brief Whether the dialect must rebuild a table to apply an `ALTER TABLE` schema change that it + /// cannot express in place — adding/dropping a foreign-key constraint, or altering a column's + /// type/nullability (`ALTER TABLE … ADD/DROP CONSTRAINT` / `… ALTER COLUMN`). /// - /// SQLite returns `true`; every other backend defaults to `false`. The migration - /// executor consults this to decide whether to take the table-rebuild path on - /// `AlterTable` steps that touch foreign keys. - [[nodiscard]] virtual bool RequiresTableRebuildForForeignKeyChange() const noexcept + /// SQLite returns `true`; every other backend defaults to `false`. The migration executor consults + /// this (via @ref SqlConnection::RequiresTableRebuildForSchemaChange) to decide whether to take the + /// table-rebuild path for the `-- LIGHTWEIGHT_SQLITE_GUARD:` sentinels these dialects emit. + [[nodiscard]] virtual bool RequiresTableRebuildForSchemaChange() const noexcept { return false; } diff --git a/src/Lightweight/SqlStatement.hpp b/src/Lightweight/SqlStatement.hpp index 607c1d49d..267b69ec5 100644 --- a/src/Lightweight/SqlStatement.hpp +++ b/src/Lightweight/SqlStatement.hpp @@ -1983,8 +1983,31 @@ void SqlStatement::MigrateDirect(Callable const& callable, std::source_location callable(migration); auto const queries = migration.GetPlan().ToSql(); ZoneValue(queries.size()); + + // A comment-only `-- LIGHTWEIGHT_SQLITE_GUARD:` script (e.g. ALTER COLUMN or a foreign-key change on + // SQLite) carries no executable DDL: the schema change is performed by the migration executor's + // table-rebuild path, which only runs via MigrationManager. Executing such a script directly here + // would silently do nothing, so fail loudly and point at the supported entry point instead. + auto const isCommentOnlyGuardScript = [](std::string_view script) { + constexpr std::string_view marker = "-- LIGHTWEIGHT_SQLITE_GUARD:"; + if (!script.starts_with(marker)) + return false; + auto const newline = script.find('\n'); + if (newline == std::string_view::npos) + return true; // sentinel line only, nothing executable follows + auto const body = script.substr(newline + 1); + auto const bodyStart = body.find_first_not_of(" \t\r\n"); + return bodyStart == std::string_view::npos || body.substr(bodyStart).starts_with("--"); + }; + for (auto const& query: queries) { + if (isCommentOnlyGuardScript(query)) + throw std::runtime_error( + std::format("SqlStatement::MigrateDirect cannot apply this SQLite schema change directly because it " + "requires a table rebuild (e.g. ALTER COLUMN or a foreign-key change). Apply it through " + "MigrationManager::ApplyPendingMigrations, which runs the rebuild executor.\n Script: {}", + query)); [[maybe_unused]] auto cursor = ExecuteDirect(query, location); } } diff --git a/src/tests/MigrationTests.cpp b/src/tests/MigrationTests.cpp index d8acc3fba..b9577ccec 100644 --- a/src/tests/MigrationTests.cpp +++ b/src/tests/MigrationTests.cpp @@ -1149,6 +1149,591 @@ TEST_CASE_METHOD(SqlMigrationTestFixture, "AlterTable AddCompositeForeignKey reb } } +TEST_CASE_METHOD(SqlMigrationTestFixture, "AlterTable AlterColumn rebuilds SQLite table", "[SqlMigration]") +{ + using namespace SqlColumnTypeDefinitions; + + // ALTER COLUMN is a SQLite-specific code path: SQLite has no `ALTER TABLE … ALTER COLUMN`, + // so the formatter emits an ALTER_COLUMN sentinel and the executor rebuilds the table. + // This test drives the change through MigrationManager::ApplyPendingMigrations() so the + // guard-aware executor (not a bare ExecuteDirect) runs — i.e. it exercises the real + // rebuild and is verified against live SQLite by CI's --test-env=sqlite3 matrix leg. + // Other dialects ALTER the column in place and are covered by the formatter-output test. + auto conn = SqlConnection {}; + auto skipStmt = SqlStatement { conn }; + UNSUPPORTED_DATABASE(skipStmt, SqlServerType::MICROSOFT_SQL); + UNSUPPORTED_DATABASE(skipStmt, SqlServerType::POSTGRESQL); + UNSUPPORTED_DATABASE(skipStmt, SqlServerType::MYSQL); + + auto& manager = SqlMigration::MigrationManager::GetInstance(); + + SECTION("widen type: rows, other columns, and the primary key all survive") + { + auto create = SqlMigration::Migration<202602010001>( + "create widen", + [](SqlMigrationQueryBuilder& plan) { + plan.CreateTable("Widen") + .PrimaryKey("id", Integer()) + .RequiredColumn("descr", Varchar(10)) + .Column("note", Varchar(20)); + }, + [](SqlMigrationQueryBuilder& plan) { plan.DropTable("Widen"); }); + + manager.CreateMigrationHistory(); + REQUIRE(manager.ApplyPendingMigrations() == 1); + + { + auto stmt = SqlStatement { conn }; + (void) stmt.ExecuteDirect(R"(INSERT INTO "Widen" ("id", "descr", "note") VALUES (1, 'short', 'keep'))"); + } + + auto widen = SqlMigration::Migration<202602010002>( + "widen descr", + [](SqlMigrationQueryBuilder& plan) { + plan.AlterTable("Widen").AlterColumn("descr", Text(), SqlNullable::NotNull); + }, + [](SqlMigrationQueryBuilder&) {}); + + REQUIRE(manager.ApplyPendingMigrations() == 1); + + // Row-preservation: the rebuild must not lose data, including the untouched 'note'. + { + auto stmt = SqlStatement { conn }; + auto cursor = stmt.ExecuteDirect(R"(SELECT "id", "descr", "note" FROM "Widen")"); + REQUIRE(cursor.FetchRow()); + CHECK(cursor.GetColumn(1) == 1); + CHECK(cursor.GetColumn(2) == "short"); + CHECK(cursor.GetColumn(3) == "keep"); + CHECK_FALSE(cursor.FetchRow()); + } + + // Schema: 'descr' is now TEXT and still NOT NULL; the primary key on 'id' survived. + { + auto stmt = SqlStatement { conn }; + auto cursor = stmt.ExecuteDirect(R"(PRAGMA table_info("Widen"))"); + bool checkedDescr = false; + bool checkedId = false; + while (cursor.FetchRow()) + { + auto const name = cursor.GetColumn(2); // name + if (name == "descr") + { + CHECK(cursor.GetColumn(3) == "TEXT"); // type + CHECK(cursor.GetColumn(4) == 1); // notnull + checkedDescr = true; + } + else if (name == "id") + { + CHECK(cursor.GetColumn(6) == 1); // pk + checkedId = true; + } + } + CHECK(checkedDescr); + CHECK(checkedId); + } + } + + SECTION("relax NOT NULL to NULL lets a NULL be inserted afterwards") + { + auto create = SqlMigration::Migration<202602020001>( + "create relax", + [](SqlMigrationQueryBuilder& plan) { + plan.CreateTable("Relax").PrimaryKey("id", Integer()).RequiredColumn("descr", Varchar(20)); + }, + [](SqlMigrationQueryBuilder& plan) { plan.DropTable("Relax"); }); + + manager.CreateMigrationHistory(); + REQUIRE(manager.ApplyPendingMigrations() == 1); + + auto relax = SqlMigration::Migration<202602020002>( + "relax descr", + [](SqlMigrationQueryBuilder& plan) { + plan.AlterTable("Relax").AlterColumn("descr", Varchar(20), SqlNullable::Null); + }, + [](SqlMigrationQueryBuilder&) {}); + + REQUIRE(manager.ApplyPendingMigrations() == 1); + + auto stmt = SqlStatement { conn }; + // A NULL is now accepted where the column previously was NOT NULL. + CHECK_NOTHROW(stmt.ExecuteDirect(R"(INSERT INTO "Relax" ("id", "descr") VALUES (1, NULL))")); + auto cursor = stmt.ExecuteDirect(R"(SELECT "descr" FROM "Relax" WHERE "id" = 1)"); + REQUIRE(cursor.FetchRow()); + CHECK(cursor.GetColumn>(1) == std::nullopt); + } + + SECTION("tighten NULL to NOT NULL enforces the constraint and keeps existing rows") + { + auto create = SqlMigration::Migration<202602030001>( + "create tighten", + [](SqlMigrationQueryBuilder& plan) { + plan.CreateTable("Tighten").PrimaryKey("id", Integer()).Column("descr", Varchar(20)); + }, + [](SqlMigrationQueryBuilder& plan) { plan.DropTable("Tighten"); }); + + manager.CreateMigrationHistory(); + REQUIRE(manager.ApplyPendingMigrations() == 1); + + { + auto stmt = SqlStatement { conn }; + (void) stmt.ExecuteDirect(R"(INSERT INTO "Tighten" ("id", "descr") VALUES (1, 'present'))"); + } + + auto tighten = SqlMigration::Migration<202602030002>( + "tighten descr", + [](SqlMigrationQueryBuilder& plan) { + plan.AlterTable("Tighten").AlterColumn("descr", Varchar(20), SqlNullable::NotNull); + }, + [](SqlMigrationQueryBuilder&) {}); + + REQUIRE(manager.ApplyPendingMigrations() == 1); + + auto stmt = SqlStatement { conn }; + // The existing non-null row survived the rebuild. + { + auto cursor = stmt.ExecuteDirect(R"(SELECT "descr" FROM "Tighten" WHERE "id" = 1)"); + REQUIRE(cursor.FetchRow()); + CHECK(cursor.GetColumn(1) == "present"); + } + // NOT NULL is now enforced. + CHECK_THROWS(stmt.ExecuteDirect(R"(INSERT INTO "Tighten" ("id", "descr") VALUES (2, NULL))")); + } + + SECTION("two successive AlterColumn calls in one migration fold and apply") + { + auto create = SqlMigration::Migration<202602040001>( + "create multi", + [](SqlMigrationQueryBuilder& plan) { + plan.CreateTable("Multi").PrimaryKey("id", Integer()).RequiredColumn("descr", Varchar(10)); + }, + [](SqlMigrationQueryBuilder& plan) { plan.DropTable("Multi"); }); + + manager.CreateMigrationHistory(); + REQUIRE(manager.ApplyPendingMigrations() == 1); + + { + auto stmt = SqlStatement { conn }; + (void) stmt.ExecuteDirect(R"(INSERT INTO "Multi" ("id", "descr") VALUES (1, 'x'))"); + } + + auto alter = SqlMigration::Migration<202602040002>( + "two alters", + [](SqlMigrationQueryBuilder& plan) { + plan.AlterTable("Multi").AlterColumn("descr", Varchar(50), SqlNullable::NotNull); + plan.AlterTable("Multi").AlterColumn("descr", Text(), SqlNullable::Null); + }, + [](SqlMigrationQueryBuilder&) {}); + + REQUIRE(manager.ApplyPendingMigrations() == 1); + + auto stmt = SqlStatement { conn }; + auto cursor = stmt.ExecuteDirect(R"(SELECT "id", "descr" FROM "Multi")"); + REQUIRE(cursor.FetchRow()); + CHECK(cursor.GetColumn(1) == 1); + CHECK(cursor.GetColumn(2) == "x"); + CHECK_FALSE(cursor.FetchRow()); + } + + SECTION("altering a non-existent column raises and leaves the table intact") + { + auto create = SqlMigration::Migration<202602050001>( + "create missing", + [](SqlMigrationQueryBuilder& plan) { + plan.CreateTable("Missing").PrimaryKey("id", Integer()).RequiredColumn("descr", Varchar(10)); + }, + [](SqlMigrationQueryBuilder& plan) { plan.DropTable("Missing"); }); + + manager.CreateMigrationHistory(); + REQUIRE(manager.ApplyPendingMigrations() == 1); + + { + auto stmt = SqlStatement { conn }; + (void) stmt.ExecuteDirect(R"(INSERT INTO "Missing" ("id", "descr") VALUES (1, 'here'))"); + } + + auto bad = SqlMigration::Migration<202602050002>( + "alter missing column", + [](SqlMigrationQueryBuilder& plan) { + plan.AlterTable("Missing").AlterColumn("nonexistent", Text(), SqlNullable::Null); + }, + [](SqlMigrationQueryBuilder&) {}); + + CHECK_THROWS(manager.ApplyPendingMigrations()); + + // The transform throws before any DDL runs, so the original table and its row remain. + auto stmt = SqlStatement { conn }; + auto cursor = stmt.ExecuteDirect(R"(SELECT "id", "descr" FROM "Missing")"); + REQUIRE(cursor.FetchRow()); + CHECK(cursor.GetColumn(1) == 1); + CHECK(cursor.GetColumn(2) == "here"); + } + + SECTION("indexes and uniqueness survive the rebuild") + { + // A column with a separately-created UNIQUE INDEX lives in its own sqlite_schema row, which + // DROP TABLE destroys. The rebuild must re-create it, or altering an unrelated column would + // silently drop the index and stop enforcing uniqueness. + auto create = SqlMigration::Migration<202602060001>( + "create indexed", + [](SqlMigrationQueryBuilder& plan) { + plan.CreateTable("Indexed") + .PrimaryKey("id", Integer()) + .RequiredColumn("code", Varchar(10)) + .Unique() + .Index() // emits a separate CREATE UNIQUE INDEX "Indexed_code_index" + .RequiredColumn("descr", Varchar(10)); + }, + [](SqlMigrationQueryBuilder& plan) { plan.DropTable("Indexed"); }); + + manager.CreateMigrationHistory(); + REQUIRE(manager.ApplyPendingMigrations() == 1); + + { + auto stmt = SqlStatement { conn }; + (void) stmt.ExecuteDirect(R"(INSERT INTO "Indexed" ("id", "code", "descr") VALUES (1, 'AAA', 'x'))"); + } + + auto alter = SqlMigration::Migration<202602060002>( + "widen descr keep index", + [](SqlMigrationQueryBuilder& plan) { + plan.AlterTable("Indexed").AlterColumn("descr", Text(), SqlNullable::NotNull); + }, + [](SqlMigrationQueryBuilder&) {}); + + REQUIRE(manager.ApplyPendingMigrations() == 1); + + auto stmt = SqlStatement { conn }; + // The explicitly-created index still exists after the rebuild of an unrelated column. + { + auto cursor = stmt.ExecuteDirect( + R"(SELECT COUNT(*) FROM sqlite_schema WHERE type='index' AND tbl_name='Indexed' AND name='Indexed_code_index')"); + REQUIRE(cursor.FetchRow()); + CHECK(cursor.GetColumn(1) == 1); + } + // ...and it still enforces uniqueness on 'code'. + CHECK_THROWS(stmt.ExecuteDirect(R"(INSERT INTO "Indexed" ("id", "code", "descr") VALUES (2, 'AAA', 'y'))")); + } + + SECTION("altering an AUTOINCREMENT primary key to a non-integer type is rejected") + { + auto create = SqlMigration::Migration<202602070001>( + "create autoinc", + [](SqlMigrationQueryBuilder& plan) { + plan.CreateTable("Autoinc") + .PrimaryKeyWithAutoIncrement("id", Integer()) + .RequiredColumn("descr", Varchar(10)); + }, + [](SqlMigrationQueryBuilder& plan) { plan.DropTable("Autoinc"); }); + + manager.CreateMigrationHistory(); + REQUIRE(manager.ApplyPendingMigrations() == 1); + + { + auto stmt = SqlStatement { conn }; + (void) stmt.ExecuteDirect(R"(INSERT INTO "Autoinc" ("descr") VALUES ('x'))"); + } + + auto bad = SqlMigration::Migration<202602070002>( + "retype autoinc pk", + [](SqlMigrationQueryBuilder& plan) { + plan.AlterTable("Autoinc").AlterColumn("id", Text(), SqlNullable::NotNull); + }, + [](SqlMigrationQueryBuilder&) {}); + + // SQLite cannot keep AUTOINCREMENT on a non-INTEGER column; the rebuild refuses up front + // rather than emitting DDL SQLite rejects mid-rebuild. + CHECK_THROWS(manager.ApplyPendingMigrations()); + + // The table and its row are intact (the rewrite throws before any DDL runs). + auto stmt = SqlStatement { conn }; + auto cursor = stmt.ExecuteDirect(R"(SELECT COUNT(*) FROM "Autoinc")"); + REQUIRE(cursor.FetchRow()); + CHECK(cursor.GetColumn(1) == 1); + } + + SECTION("rebuild drops a contradictory DEFAULT NULL and survives literal defaults") + { + manager.CreateMigrationHistory(); + + // A DEFAULT NULL column and a sibling whose default is a string literal with an unbalanced + // parenthesis — neither shape is producible via the CreateTable DSL, so build the table + // out-of-band, then drive the rebuilds through a migration. + { + auto stmt = SqlStatement { conn }; + (void) stmt.ExecuteDirect( + R"(CREATE TABLE "Defs" ("id" INTEGER PRIMARY KEY, "a" INTEGER DEFAULT NULL, "b" TEXT DEFAULT '(x'))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "Defs" ("id", "a", "b") VALUES (1, 5, 'keep'))"); + } + + auto alter = SqlMigration::Migration<202602080002>( + "tighten a and retype b", + [](SqlMigrationQueryBuilder& plan) { + // Tighten 'a' (DEFAULT NULL) to NOT NULL, then retype 'b' (paren-literal default). + plan.AlterTable("Defs").AlterColumn("a", Integer(), SqlNullable::NotNull); + plan.AlterTable("Defs").AlterColumn("b", Text(), SqlNullable::NotNull); + }, + [](SqlMigrationQueryBuilder&) {}); + + REQUIRE(manager.ApplyPendingMigrations() == 1); + + auto stmt = SqlStatement { conn }; + // The row survived; 'b''s literal default with a stray '(' did not corrupt the rebuild. + { + auto cursor = stmt.ExecuteDirect(R"(SELECT "id", "a", "b" FROM "Defs")"); + REQUIRE(cursor.FetchRow()); + CHECK(cursor.GetColumn(1) == 1); + CHECK(cursor.GetColumn(2) == 5); + CHECK(cursor.GetColumn(3) == "keep"); + } + // 'a' is now NOT NULL with the contradictory NULL default removed: omitting it must fail. + CHECK_THROWS(stmt.ExecuteDirect(R"(INSERT INTO "Defs" ("id", "b") VALUES (2, 'y'))")); + { + auto cursor = stmt.ExecuteDirect(R"(SELECT sql FROM sqlite_schema WHERE type='table' AND name='Defs')"); + REQUIRE(cursor.FetchRow()); + auto const sql = cursor.GetColumn(1); + CHECK(!sql.contains("NOT NULL DEFAULT NULL")); // no contradiction + CHECK(sql.contains("DEFAULT '(x'")); // literal default preserved verbatim + } + } + + SECTION("the column anchor is not confused by an earlier inline REFERENCES") + { + manager.CreateMigrationHistory(); + + // Externally-created tables: 'Child' has a first column whose inline REFERENCES names a + // column ("descr") sharing the name of a later column we then alter. A first-match without + // the trailing-space anchor would splice into the REFERENCES clause instead of the column. + { + auto stmt = SqlStatement { conn }; + (void) stmt.ExecuteDirect(R"(CREATE TABLE "Parent" ("descr" INTEGER PRIMARY KEY))"); + (void) stmt.ExecuteDirect( + R"(CREATE TABLE "Child" ("ref" INTEGER REFERENCES "Parent"("descr"), "descr" VARCHAR(10) NOT NULL))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "Parent" ("descr") VALUES (7))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "Child" ("ref", "descr") VALUES (7, 'hi'))"); + } + + auto alter = SqlMigration::Migration<202602090002>( + "widen child descr", + [](SqlMigrationQueryBuilder& plan) { + plan.AlterTable("Child").AlterColumn("descr", Text(), SqlNullable::NotNull); + }, + [](SqlMigrationQueryBuilder&) {}); + + REQUIRE(manager.ApplyPendingMigrations() == 1); + + auto stmt = SqlStatement { conn }; + { + auto cursor = stmt.ExecuteDirect(R"(SELECT sql FROM sqlite_schema WHERE type='table' AND name='Child')"); + REQUIRE(cursor.FetchRow()); + auto const sql = cursor.GetColumn(1); + // The FK clause stays intact and the real 'descr' column (not the FK reference) is retyped. + CHECK(sql.contains(R"(REFERENCES "Parent"("descr"))")); + CHECK(sql.contains(R"("descr" TEXT)")); + } + + auto rows = stmt.ExecuteDirect(R"(SELECT "ref", "descr" FROM "Child")"); + REQUIRE(rows.FetchRow()); + CHECK(rows.GetColumn(1) == 7); + CHECK(rows.GetColumn(2) == "hi"); + } + + SECTION("an earlier inline CHECK mentioning the column does not misanchor the rewrite") + { + manager.CreateMigrationHistory(); + + // The first column's inline CHECK references a *later* column ("descr") by quoted name followed + // by a space — a first-substring anchor would splice the new type into the CHECK clause. Build + // out-of-band since the DSL cannot emit such a CHECK. + { + auto stmt = SqlStatement { conn }; + (void) stmt.ExecuteDirect( + R"(CREATE TABLE "Chk" ("a" INTEGER CHECK("descr" <> ''), "descr" VARCHAR(10) NOT NULL))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "Chk" ("a", "descr") VALUES (1, 'x'))"); + } + + auto alter = SqlMigration::Migration<202602100002>( + "widen chk descr", + [](SqlMigrationQueryBuilder& plan) { + plan.AlterTable("Chk").AlterColumn("descr", Text(), SqlNullable::NotNull); + }, + [](SqlMigrationQueryBuilder&) {}); + + REQUIRE(manager.ApplyPendingMigrations() == 1); + + auto stmt = SqlStatement { conn }; + { + auto cursor = stmt.ExecuteDirect(R"(SELECT sql FROM sqlite_schema WHERE type='table' AND name='Chk')"); + REQUIRE(cursor.FetchRow()); + auto const sql = cursor.GetColumn(1); + CHECK(sql.contains(R"(CHECK("descr" <> ''))")); // CHECK clause intact + CHECK(sql.contains(R"("descr" TEXT)")); // real column retyped + } + auto rows = stmt.ExecuteDirect(R"(SELECT "a", "descr" FROM "Chk")"); + REQUIRE(rows.FetchRow()); + CHECK(rows.GetColumn(1) == 1); + CHECK(rows.GetColumn(2) == "x"); + } + + SECTION("a column whose quoted name contains an apostrophe survives the rebuild") + { + manager.CreateMigrationHistory(); + + // The apostrophe-bearing identifier precedes the altered column, so the rewriter must scan past + // it; treating the `'` as a string-literal delimiter would corrupt the column list. + { + auto stmt = SqlStatement { conn }; + (void) stmt.ExecuteDirect( + R"(CREATE TABLE "Apos" ("id" INTEGER PRIMARY KEY, "O'Brien" INTEGER, "descr" VARCHAR(10)))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "Apos" ("id", "O'Brien", "descr") VALUES (1, 7, 'x'))"); + } + + auto alter = SqlMigration::Migration<202602110002>( + "widen apos descr", + [](SqlMigrationQueryBuilder& plan) { plan.AlterTable("Apos").AlterColumn("descr", Text(), SqlNullable::Null); }, + [](SqlMigrationQueryBuilder&) {}); + + REQUIRE(manager.ApplyPendingMigrations() == 1); + + auto stmt = SqlStatement { conn }; + auto cursor = stmt.ExecuteDirect(R"(SELECT "id", "O'Brien", "descr" FROM "Apos")"); + REQUIRE(cursor.FetchRow()); + CHECK(cursor.GetColumn(1) == 1); + CHECK(cursor.GetColumn(2) == 7); + CHECK(cursor.GetColumn(3) == "x"); + } + + SECTION("a space-less DEFAULT(NULL) is dropped when tightening to NOT NULL") + { + manager.CreateMigrationHistory(); + + { + auto stmt = SqlStatement { conn }; + (void) stmt.ExecuteDirect(R"(CREATE TABLE "DefNs" ("id" INTEGER PRIMARY KEY, "a" INTEGER DEFAULT(NULL)))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "DefNs" ("id", "a") VALUES (1, 5))"); + } + + auto alter = SqlMigration::Migration<202602120002>( + "tighten defns a", + [](SqlMigrationQueryBuilder& plan) { + plan.AlterTable("DefNs").AlterColumn("a", Integer(), SqlNullable::NotNull); + }, + [](SqlMigrationQueryBuilder&) {}); + + REQUIRE(manager.ApplyPendingMigrations() == 1); + + auto stmt = SqlStatement { conn }; + { + auto cursor = stmt.ExecuteDirect(R"(SELECT sql FROM sqlite_schema WHERE type='table' AND name='DefNs')"); + REQUIRE(cursor.FetchRow()); + auto const sql = cursor.GetColumn(1); + CHECK(!sql.contains("DEFAULT(NULL)")); // contradictory default removed + } + // NOT NULL is now enforced even though the default was a no-space DEFAULT(NULL). + CHECK_THROWS(stmt.ExecuteDirect(R"(INSERT INTO "DefNs" ("id") VALUES (2))")); + } + + SECTION("a DEFAULT literal containing the word AUTOINCREMENT does not block retyping") + { + manager.CreateMigrationHistory(); + + { + auto stmt = SqlStatement { conn }; + (void) stmt.ExecuteDirect( + R"(CREATE TABLE "Faux" ("id" INTEGER PRIMARY KEY, "note" VARCHAR(40) DEFAULT 'has AUTOINCREMENT word'))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "Faux" ("id") VALUES (1))"); + } + + auto alter = SqlMigration::Migration<202602130002>( + "retype faux note", + [](SqlMigrationQueryBuilder& plan) { plan.AlterTable("Faux").AlterColumn("note", Text(), SqlNullable::Null); }, + [](SqlMigrationQueryBuilder&) {}); + + // The substring 'AUTOINCREMENT' inside the DEFAULT literal must not trip the INTEGER-PK guard. + REQUIRE(manager.ApplyPendingMigrations() == 1); + + auto stmt = SqlStatement { conn }; + auto cursor = stmt.ExecuteDirect(R"(SELECT "note" FROM "Faux" WHERE "id" = 1)"); + REQUIRE(cursor.FetchRow()); + CHECK(cursor.GetColumn(1) == "has AUTOINCREMENT word"); // default applied, literal preserved + } + + SECTION("a generated column is preserved and excluded from the data copy") + { + manager.CreateMigrationHistory(); + + // A STORED generated column lives in the column list but cannot be inserted into; the rebuild's + // INSERT...SELECT must skip it, and its expression (referencing the altered column) must survive. + { + auto stmt = SqlStatement { conn }; + (void) stmt.ExecuteDirect( + R"(CREATE TABLE "Gen" ("id" INTEGER PRIMARY KEY, "a" INTEGER, "b" INTEGER GENERATED ALWAYS AS ("a" * 2) STORED))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "Gen" ("id", "a") VALUES (1, 5))"); + } + + auto alter = SqlMigration::Migration<202602140002>( + "tighten gen a", + [](SqlMigrationQueryBuilder& plan) { plan.AlterTable("Gen").AlterColumn("a", Integer(), SqlNullable::NotNull); }, + [](SqlMigrationQueryBuilder&) {}); + + REQUIRE(manager.ApplyPendingMigrations() == 1); + + auto stmt = SqlStatement { conn }; + auto cursor = stmt.ExecuteDirect(R"(SELECT "id", "a", "b" FROM "Gen")"); + REQUIRE(cursor.FetchRow()); + CHECK(cursor.GetColumn(1) == 1); + CHECK(cursor.GetColumn(2) == 5); + CHECK(cursor.GetColumn(3) == 10); // generated column still computes a*2 + } + + SECTION("an externally-created table with an unquoted short name rebuilds correctly") + { + manager.CreateMigrationHistory(); + + // Unquoted table name "T" is a substring of "CREATE"/"TABLE"; a naive substring replace of the + // name would corrupt the stored DDL. The word-boundary fallback must target the declaration. + { + auto stmt = SqlStatement { conn }; + (void) stmt.ExecuteDirect(R"(CREATE TABLE T ("id" INTEGER PRIMARY KEY, "descr" VARCHAR(10)))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO T ("id", "descr") VALUES (1, 'x'))"); + } + + auto alter = SqlMigration::Migration<202602150002>( + "widen T descr", + [](SqlMigrationQueryBuilder& plan) { plan.AlterTable("T").AlterColumn("descr", Text(), SqlNullable::Null); }, + [](SqlMigrationQueryBuilder&) {}); + + REQUIRE(manager.ApplyPendingMigrations() == 1); + + auto stmt = SqlStatement { conn }; + auto cursor = stmt.ExecuteDirect(R"(SELECT "id", "descr" FROM T)"); + REQUIRE(cursor.FetchRow()); + CHECK(cursor.GetColumn(1) == 1); + CHECK(cursor.GetColumn(2) == "x"); + } +} + +TEST_CASE_METHOD(SqlMigrationTestFixture, "AlterColumn via MigrateDirect fails loudly on SQLite", "[SqlMigration]") +{ + using namespace SqlColumnTypeDefinitions; + + // ALTER COLUMN on SQLite needs a table rebuild that only MigrationManager performs. Applying it + // through the comment-only-sentinel-unaware MigrateDirect must throw rather than silently no-op. + auto conn = SqlConnection {}; + auto skipStmt = SqlStatement { conn }; + UNSUPPORTED_DATABASE(skipStmt, SqlServerType::MICROSOFT_SQL); + UNSUPPORTED_DATABASE(skipStmt, SqlServerType::POSTGRESQL); + UNSUPPORTED_DATABASE(skipStmt, SqlServerType::MYSQL); + + auto stmt = SqlStatement { conn }; + stmt.MigrateDirect([](SqlMigrationQueryBuilder& plan) { + plan.CreateTable("DirectAlter").PrimaryKey("id", Integer()).RequiredColumn("descr", Varchar(10)); + }); + + CHECK_THROWS(stmt.MigrateDirect([](SqlMigrationQueryBuilder& plan) { + plan.AlterTable("DirectAlter").AlterColumn("descr", Text(), SqlNullable::Null); + })); +} + TEST_CASE_METHOD(SqlMigrationTestFixture, "RegisterRelease stores and orders releases", "[SqlMigration]") { auto& manager = SqlMigration::MigrationManager::GetInstance(); diff --git a/src/tests/QueryBuilderTests.cpp b/src/tests/QueryBuilderTests.cpp index 7af859680..b8fcc3705 100644 --- a/src/tests/QueryBuilderTests.cpp +++ b/src/tests/QueryBuilderTests.cpp @@ -1423,6 +1423,53 @@ TEST_CASE_METHOD(SqlTestFixture, "AlterTable AlterColumn", "[SqlQueryBuilder][Mi } } +TEST_CASE_METHOD(SqlTestFixture, "AlterTable AlterColumn SQL", "[SqlQueryBuilder][Migration]") +{ + using namespace SqlColumnTypeDefinitions; + + // SQLite has no `ALTER TABLE … ALTER COLUMN`, so the formatter emits an ALTER_COLUMN + // sentinel that the migration executor turns into a table rebuild. The commented-out + // MSSQL-style ALTER on the second line keeps dry-run output readable. MS SQL Server and + // PostgreSQL alter the column in place. + SECTION("widen to TEXT, stay nullable (the shape that regressed CI on SQLite)") + { + CheckSqlQueryBuilder( + [](SqlQueryBuilder& q) { + auto migration = q.Migration(); + migration.AlterTable("Table").AlterColumn("column", Text {}, SqlNullable::Null); + return migration.GetPlan(); + }, + QueryExpectations { + .sqlite = R"sql( + -- LIGHTWEIGHT_SQLITE_GUARD: ALTER_COLUMN "Table" "column" "TEXT" "NULL" + -- ALTER TABLE "Table" ALTER COLUMN "column" TEXT NULL; + )sql", + .postgres = + R"sql(ALTER TABLE "Table" ALTER COLUMN "column" TYPE TEXT, ALTER COLUMN "column" DROP NOT NULL;)sql", + .sqlServer = R"sql(ALTER TABLE "Table" ALTER COLUMN "column" VARCHAR(MAX) NULL;)sql", + }); + } + + SECTION("change a parameterized type to NOT NULL") + { + CheckSqlQueryBuilder( + [](SqlQueryBuilder& q) { + auto migration = q.Migration(); + migration.AlterTable("Table").AlterColumn("column", Varchar { 200 }, SqlNullable::NotNull); + return migration.GetPlan(); + }, + QueryExpectations { + .sqlite = R"sql( + -- LIGHTWEIGHT_SQLITE_GUARD: ALTER_COLUMN "Table" "column" "VARCHAR(200)" "NOT NULL" + -- ALTER TABLE "Table" ALTER COLUMN "column" VARCHAR(200) NOT NULL; + )sql", + .postgres = + R"sql(ALTER TABLE "Table" ALTER COLUMN "column" TYPE VARCHAR(200), ALTER COLUMN "column" SET NOT NULL;)sql", + .sqlServer = R"sql(ALTER TABLE "Table" ALTER COLUMN "column" VARCHAR(200) NOT NULL;)sql", + }); + } +} + TEST_CASE_METHOD(SqlTestFixture, "AlterTable multiple AddColumn calls", "[SqlQueryBuilder][Migration]") { using namespace SqlColumnTypeDefinitions; diff --git a/src/tests/QueryFormatterTests.cpp b/src/tests/QueryFormatterTests.cpp index f16a58db7..b4f087900 100644 --- a/src/tests/QueryFormatterTests.cpp +++ b/src/tests/QueryFormatterTests.cpp @@ -181,14 +181,14 @@ TEST_CASE("SqlQueryFormatter::QueryServerVersion emits the per-DBMS version quer } // ================================================================================================ -// RequiresTableRebuildForForeignKeyChange — SQLite needs a full table rebuild, others don't +// RequiresTableRebuildForSchemaChange — SQLite needs a full table rebuild, others don't // ================================================================================================ -TEST_CASE("SqlQueryFormatter::RequiresTableRebuildForForeignKeyChange flag matches the dialect", "[SqlQueryFormatter]") +TEST_CASE("SqlQueryFormatter::RequiresTableRebuildForSchemaChange flag matches the dialect", "[SqlQueryFormatter]") { - CHECK(SqlQueryFormatter::Sqlite().RequiresTableRebuildForForeignKeyChange()); - CHECK_FALSE(SqlQueryFormatter::PostgrSQL().RequiresTableRebuildForForeignKeyChange()); - CHECK_FALSE(SqlQueryFormatter::SqlServer().RequiresTableRebuildForForeignKeyChange()); + CHECK(SqlQueryFormatter::Sqlite().RequiresTableRebuildForSchemaChange()); + CHECK_FALSE(SqlQueryFormatter::PostgrSQL().RequiresTableRebuildForSchemaChange()); + CHECK_FALSE(SqlQueryFormatter::SqlServer().RequiresTableRebuildForSchemaChange()); } // ================================================================================================