diff --git a/CHANGELOG.md b/CHANGELOG.md index 8921e04..ae99db5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ Intermediate package/UI version bumps are grouped under the next published GitHu ### Fixed +- Deployment accepts trailing SQL comments and checks every pending migration's statement terminator before database changes, preventing false semicolon errors and avoidable partial upgrades. [#173](https://github.com/apptrackit/finance/issues/173) - Cash and investment balance changes use decimal arithmetic to prevent floating-point residue while preserving genuine fractional amounts. Payments, transfers, adjustments, pending confirmations and recurring execution update balances and audit records together, with guards against duplicate or conflicting changes. - Dashboard net worth respects investment holding exclusions; Investments continues to show all active holdings. - Buttons, tabs, switches, menus, selects, checkboxes and other enabled click targets show a pointer cursor again across the app; Tailwind CSS 4 had reset buttons to the default arrow. Disabled controls keep the default cursor. [#157](https://github.com/apptrackit/finance/issues/157) diff --git a/api/migrations/README.md b/api/migrations/README.md index 313282b..3920fb4 100644 --- a/api/migrations/README.md +++ b/api/migrations/README.md @@ -23,6 +23,7 @@ Example: `001-init.sql`, `002-add-user-table.sql` - Migrations already applied are skipped automatically - API/MCP-only deployments check migration history and stop if any are pending. Add `-- --migrations` to explicitly apply them before deploying that Worker. - A history-query failure stops deployment. Each migration and its history insert are submitted together; do not substitute Wrangler's separate built-in migration ledger. +- The final SQL statement must end with a semicolon; trailing line/block comments are allowed. All pending files are checked for this terminator before any migration write. Retrying a stopped deployment skips migrations already recorded by their complete name. ## For Developers diff --git a/scripts/deploy.mjs b/scripts/deploy.mjs index 70edad5..164586c 100644 --- a/scripts/deploy.mjs +++ b/scripts/deploy.mjs @@ -140,9 +140,40 @@ export function d1Rows(output) { return result.flatMap(item => item.results) } +function hasFinalSqlTerminator(sql) { + let lastToken + for (let index = 0; index < sql.length; index++) { + const char = sql[index] + if (/\s/.test(char)) continue + if (char === '-' && sql[index + 1] === '-') { + while (index < sql.length && !/[\r\n]/.test(sql[index])) index++ + continue + } + if (char === '/' && sql[index + 1] === '*') { + const end = sql.indexOf('*/', index + 2) + if (end === -1) return false + index = end + 1 + continue + } + // Comment markers and semicolons inside SQL strings/identifiers are data. + // SQLite escapes quotes by doubling them; bracket identifiers end at ]. + if (['\'', '"', '`', '['].includes(char)) { + const closing = char === '[' ? ']' : char + for (index++; index < sql.length; index++) { + if (sql[index] !== closing) continue + if (closing !== ']' && sql[index + 1] === closing) index++ + else break + } + if (index === sql.length) return false + lastToken = closing + } else lastToken = char + } + return lastToken === ';' +} + export function migrationSqlWithHistory(sql, name) { const migration = sql.trimEnd() - if (!migration.endsWith(';')) throw new Error(`Migration ${name} must end with a semicolon.`) + if (!hasFinalSqlTerminator(migration)) throw new Error(`Migration ${name} must end with a semicolon.`) // A second standalone semicolon is an empty statement rejected by D1's // remote file importer, even though the local executor accepts it. return `${migration}\nINSERT INTO migration_history (id, migration_name) VALUES ('${name}', '${name}');\n` @@ -374,14 +405,19 @@ export async function deploy(options, { root = repositoryRoot, run = runCommand, throw new Error(`Pending migrations: ${pending.join(', ')}. Run npm run deploy:migrations or add -- --migrations to deploy:${options.target}.`) } if (plan.migrations && pending.length) { - await d1('Initializing migration history', ['--command', 'CREATE TABLE IF NOT EXISTS migration_history (id TEXT PRIMARY KEY, migration_name TEXT NOT NULL UNIQUE, executed_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP)']) - for (const file of pending) { + // Validate every pending payload before the first remote write, so a + // later file-format error cannot leave an avoidable partial upgrade. + const prepared = await Promise.all(pending.map(async file => { const name = file.slice(0, -4) const sql = await readFile(join(root, 'api/migrations', file), 'utf8') + return { file, name, sql: migrationSqlWithHistory(sql, name) } + })) + await d1('Initializing migration history', ['--command', 'CREATE TABLE IF NOT EXISTS migration_history (id TEXT PRIMARY KEY, migration_name TEXT NOT NULL UNIQUE, executed_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP)']) + for (const { file, name, sql } of prepared) { const migrationFile = join(temporary, file) // Record success in the same SQL submission, rather than ignoring a // separate history-write failure and replaying ALTERs on the next run. - await writeFile(migrationFile, migrationSqlWithHistory(sql, name), { mode: 0o600 }) + await writeFile(migrationFile, sql, { mode: 0o600 }) // Wrangler prints file-import progress to stdout even with --json, so // its output cannot be parsed as a JSON query result. Verify the // history row with a separate read after the import exits successfully. diff --git a/scripts/deploy.test.mjs b/scripts/deploy.test.mjs index 35841ba..e066d8f 100644 --- a/scripts/deploy.test.mjs +++ b/scripts/deploy.test.mjs @@ -1,6 +1,6 @@ import assert from 'node:assert/strict' import { spawnSync } from 'node:child_process' -import { access, mkdir, mkdtemp, readFile, rm, stat, symlink, writeFile } from 'node:fs/promises' +import { access, mkdir, mkdtemp, readFile, readdir, rm, stat, symlink, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { dirname, join, resolve } from 'node:path' import { test } from 'node:test' @@ -289,6 +289,62 @@ test('migration-only deployment initializes an empty database without building o await f.assertClean() }) +test('migration validation accepts trailing comments without changing SQL or adding empty statements', async () => { + const statements = [ + "CREATE TABLE example (value TEXT DEFAULT '--not a comment;');", + 'CREATE TABLE "example/*not a comment*/" ("a""--" TEXT);', + 'CREATE TABLE `example``--` ([/*column*/] TEXT);', + "CREATE TRIGGER example AFTER DELETE ON entries BEGIN SELECT 'it''s; -- quoted'; END;", + ] + for (const statement of statements) { + for (const suffix of [' -- note', '\n-- note\r\n-- another', ' /* note */', '\n/* note\n; */ -- final']) { + const sql = statement + suffix + assert.equal(migrationSqlWithHistory(sql, 'test'), `${sql}\nINSERT INTO migration_history (id, migration_name) VALUES ('test', 'test');\n`) + } + } + for (const sql of [ + 'CREATE TABLE example (id TEXT) -- ;', + 'CREATE TABLE example (id TEXT) /* ; */', + "SELECT ';' -- ;", + 'SELECT "a;" /* ; */', + 'SELECT `a;`', + 'SELECT [a;]', + "SELECT 'unterminated;", + 'SELECT 1; /* unterminated;', + '-- only comments;', + '/* only comments; */', + ]) assert.throws(() => migrationSqlWithHistory(sql, 'test'), /must end with a semicolon/) + + const directory = resolve(dirname(script), '../api/migrations') + for (const file of (await readdir(directory)).filter(file => file.endsWith('.sql'))) { + const sql = await readFile(join(directory, file), 'utf8') + assert.ok(migrationSqlWithHistory(sql, file.slice(0, -4)).startsWith(sql.trimEnd() + '\nINSERT')) + } +}) + +test('an invalid later pending migration prevents all remote writes', async t => { + const f = await fixture(t, { applied: ['001-init'] }) + await writeFile(join(f.root, 'api/migrations/003-invalid.sql'), 'SELECT 1 -- missing terminator;') + await assert.rejects(f.execute(['api', '--migrations']), /Migration 003-invalid must end with a semicolon/) + assert.equal(f.mutations().length, 0) + await f.assertClean() +}) + +test('retry after recorded investment links applies commented split migration without replaying links', async t => { + const f = await fixture(t, { applied: ['001-init', '002-change', '017-investment-transfer-links'] }) + const directory = resolve(dirname(script), '../api/migrations') + for (const file of ['017-investment-transfer-links.sql', '017-transaction-splits.sql']) { + await writeFile(join(f.root, 'api/migrations', file), await readFile(join(directory, file), 'utf8')) + } + await f.execute(['migrations']) + assert.equal(f.sqlFiles.length, 1) + assert.match(f.sqlFiles[0], /INSERT INTO migration_history .*'017-transaction-splits'/) + assert.ok(f.sqlFiles[0].includes('-- keeps the shared fields in sync instead.\nINSERT INTO migration_history')) + await f.execute(['migrations']) + assert.equal(f.sqlFiles.length, 1, 'a second run must not replay either migration') + await f.assertClean() +}) + test('full release preserves MCP preference and completes checks before any remote mutation', async t => { const f = await fixture(t, { config: { ...fixtureConfig, DEPLOY_MCP: 'true' }, applied: ['001-init'] }) await f.execute([])