From 9d9d55081c2d48aa26c334d4a97415ba31dc59b6 Mon Sep 17 00:00:00 2001 From: Alvaro Date: Thu, 11 Jun 2026 12:39:55 +0200 Subject: [PATCH] fix: clearer migration and config errors; document codec API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Error-clarity round 2 from the prod-ready audit: - #10 — a rejected migration statement now reports the migration file, the failed statement position ("statement N of M"), and a SQL preview on top of the cleaned ClickHouse message. statementError() wraps both the sync DDL and async paths; the journal still records the raw CH error. - #21 — a syntax error in clickhouse.config.ts now prints the actual build diagnostics (collectAggregateErrorMessages expands the AggregateError.errors) instead of only the "N errors building config.ts" summary. - #22 — documented the column `codec` API (general/preprocessor/raw codecs, chains, render output, and the codec_chain_* validation rules) in the schema DSL reference. Unit + live e2e tests against the .env.test cluster; all green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/error-clarity-round-2.md | 9 +++ .../src/content/docs/schema/dsl-reference.md | 44 +++++++++++ packages/cli/src/commands/migrate/apply.ts | 58 ++++++++++---- packages/cli/src/runtime/config.ts | 29 +++++++ .../cli/src/test/config-build-errors.test.ts | 56 +++++++++++++ .../test/migrate-statement-error.e2e.test.ts | 78 +++++++++++++++++++ packages/cli/src/test/statement-error.test.ts | 31 ++++++++ 7 files changed, 292 insertions(+), 13 deletions(-) create mode 100644 .changeset/error-clarity-round-2.md create mode 100644 packages/cli/src/test/config-build-errors.test.ts create mode 100644 packages/cli/src/test/migrate-statement-error.e2e.test.ts create mode 100644 packages/cli/src/test/statement-error.test.ts diff --git a/.changeset/error-clarity-round-2.md b/.changeset/error-clarity-round-2.md new file mode 100644 index 0000000..cf5c71e --- /dev/null +++ b/.changeset/error-clarity-round-2.md @@ -0,0 +1,9 @@ +--- +"chkit": patch +--- + +Clearer errors when things go wrong: + +- A rejected migration now reports the migration file, the failed statement's position (e.g. "statement 2 of 3"), and a SQL preview, alongside the ClickHouse message — instead of a bare exception with no context. +- A syntax error in `clickhouse.config.ts` now prints the actual build diagnostics (each underlying error), instead of only the "N errors building config.ts" summary. +- Documented the column `codec` API (general/preprocessor/raw codecs, chains, and the codec-chain validation rules) in the schema DSL reference. diff --git a/apps/docs/src/content/docs/schema/dsl-reference.md b/apps/docs/src/content/docs/schema/dsl-reference.md index 65dfdc4..b0c5cff 100644 --- a/apps/docs/src/content/docs/schema/dsl-reference.md +++ b/apps/docs/src/content/docs/schema/dsl-reference.md @@ -160,6 +160,47 @@ Column-level comment rendered in SQL. Previous column name for rename tracking. See [Rename support](#rename-support). +### `codec` (ColumnCodecSpec, optional) + +Sets the column compression codec, rendered as a `CODEC(...)` clause. A codec is an object with a `kind`, or an **array** forming a chain (zero or more preprocessors followed by exactly one general codec). + +```ts +columns: [ + { name: 'ts', type: 'DateTime64(3)', codec: { kind: 'Delta', size: 4 } }, + { name: 'amount', type: 'Float64', codec: { kind: 'ZSTD', level: 3 } }, + // chain: preprocessor then general codec + { name: 'seq', type: 'UInt64', codec: [{ kind: 'DoubleDelta' }, { kind: 'LZ4HC', level: 9 }] }, +] +``` + +**General codecs** (the compressor; at most one, and it must come last in a chain): + +| `kind` | Args | Renders | +|--------|------|---------| +| `NONE`, `LZ4`, `T64`, `GCD`, `ALP` | — | `CODEC(LZ4)` | +| `LZ4HC` | `level?: number` | `CODEC(LZ4HC(9))` | +| `ZSTD` | `level?: number` | `CODEC(ZSTD(3))` | + +**Preprocessing codecs** (placed before the general codec): + +| `kind` | Args | Renders | +|--------|------|---------| +| `Delta`, `DoubleDelta`, `Gorilla` | `size?: 1 \| 2 \| 4 \| 8` (bytes, defaults to 1) | `CODEC(Delta(4))` | +| `FPC` | `level: number`, `floatSize: 4 \| 8` | `CODEC(FPC(...))` | + +**Raw escape hatch** — for codecs not yet typed (new ClickHouse versions, unusual arg shapes), pass the inner expression through verbatim: + +```ts +{ name: 'blob', type: 'String', codec: { kind: 'raw', expression: 'T64, LZ4' } } +// → CODEC(T64, LZ4) +``` + +:::caution +Use the typed `{ kind: 'ZSTD', level: 3 }` shape — an unrecognized shape such as `codec: { general: 'ZSTD' }` is **not** a valid `ColumnCodecSpec` and renders an empty `CODEC()`, silently shipping a column with no codec. +::: + +Codec chains are validated (see [Validation rules](#validation-rules)): a chain must be non-empty, contain at most one general codec, and end with the general codec. + ## Skip indexes Each entry in the `indexes` array is a `SkipIndexDefinition`. The shared base fields are: @@ -371,6 +412,9 @@ chkit validates schema definitions and throws a `ChxValidationError` if any issu - **Duplicate projection names** -- repeated projection name within a table - **Primary key references missing column** -- `primaryKey` includes a column not in `columns` - **Order by references missing column** -- `orderBy` includes a column not in `columns` +- **Empty codec chain** (`codec_chain_empty`) -- a `codec` array with no steps; provide at least one codec or omit the field +- **Multiple general codecs** (`codec_chain_multiple_general`) -- more than one general codec in a chain; only one is allowed +- **Codec chain must end with a general codec** (`codec_chain_must_end_with_general`) -- preprocessors must precede the single general codec (`NONE`, `LZ4`, `LZ4HC`, `ZSTD`, `T64`, `GCD`, `ALP`) ## Structural vs. alterable properties diff --git a/packages/cli/src/commands/migrate/apply.ts b/packages/cli/src/commands/migrate/apply.ts index 90d7c4c..fab4bc9 100644 --- a/packages/cli/src/commands/migrate/apply.ts +++ b/packages/cli/src/commands/migrate/apply.ts @@ -36,6 +36,34 @@ function errorMessage(error: unknown): string { return error instanceof Error ? error.message : String(error) } +/** One-line, length-capped preview of a SQL statement for error messages. */ +export function previewStatement(sql: string, max = 120): string { + const oneLine = sql.replace(/\s+/g, ' ').trim() + return oneLine.length > max ? `${oneLine.slice(0, max)}…` : oneLine +} + +/** + * Wrap a statement-execution failure with the context a user needs to locate + * it (#10): the migration file, the failed statement's position, and a SQL + * preview — on top of the cleaned ClickHouse message. Without this, a rejected + * migration surfaces only the raw CH exception with no file or statement. + */ +export function statementError(input: { + file: string + index: number + total: number + statement: string + error: unknown +}): Error { + const wrapped = new Error( + `Migration ${input.file} failed at statement ${input.index + 1} of ${input.total}:\n` + + ` ${previewStatement(input.statement)}\n` + + errorMessage(input.error), + ) + if (input.error instanceof Error && input.error.stack) wrapped.stack = input.error.stack + return wrapped +} + function syncOperationState( index: number, operationType: string, @@ -105,18 +133,22 @@ export async function applyMigration(input: { const statement = statements[i] as string const operation = operationSummaries[i] if (operation?.mode === 'async') { - await applyAsyncStatement({ - db, - journalStore, - sql: statement, - migrationName: file, - migrationChecksum, - statementIndex: i, - operationType: operation.type, - operationKey: operation.key, - beforeRetry: operation.beforeRetry, - log: (line) => console.log(line), - }) + try { + await applyAsyncStatement({ + db, + journalStore, + sql: statement, + migrationName: file, + migrationChecksum, + statementIndex: i, + operationType: operation.type, + operationKey: operation.key, + beforeRetry: operation.beforeRetry, + log: (line) => console.log(line), + }) + } catch (error) { + throw statementError({ file, index: i, total: statements.length, statement, error }) + } // Async ops are DML (loads, backfills) — no DDL propagation to wait on. continue } @@ -144,7 +176,7 @@ export async function applyMigration(input: { Date.now, ), ) - throw error + throw statementError({ file, index: i, total: statements.length, statement, error }) } // Mark completed as soon as the statement has executed — BEFORE waiting for // DDL propagation. The statement already ran, so if the propagation wait diff --git a/packages/cli/src/runtime/config.ts b/packages/cli/src/runtime/config.ts index 0832d15..84010d8 100644 --- a/packages/cli/src/runtime/config.ts +++ b/packages/cli/src/runtime/config.ts @@ -134,6 +134,27 @@ export function parseMissingModule(message: string): string | undefined { return match?.[1] } +/** + * Transpile/build failures (e.g. a syntax error in clickhouse.config.ts) throw + * an AggregateError whose top-level message is only a summary like + * "2 errors building config.ts"; the actual diagnostics live in `.errors`. + * Surface them so the user can see what to fix (#21). + */ +export function collectAggregateErrorMessages(error: unknown): string[] { + if (!error || typeof error !== 'object') return [] + const sub = (error as { errors?: unknown }).errors + if (!Array.isArray(sub)) return [] + return sub + .map((entry) => { + if (entry instanceof Error) return entry.message + if (entry && typeof entry === 'object' && 'message' in entry) { + return String((entry as { message: unknown }).message) + } + return String(entry) + }) + .filter((message) => message.trim().length > 0) +} + function enrichConfigLoadError(error: unknown, configPath: string): Error { const message = error instanceof Error ? error.message : String(error) const missing = parseMissingModule(message) @@ -144,6 +165,14 @@ function enrichConfigLoadError(error: unknown, configPath: string): Error { { cause: error }, ) } + const subErrors = collectAggregateErrorMessages(error) + if (subErrors.length > 0) { + return new Error( + `Failed to build config ${configPath} (${subErrors.length} error${subErrors.length === 1 ? '' : 's'}):\n` + + subErrors.map((m) => ` - ${m}`).join('\n'), + { cause: error }, + ) + } return error instanceof Error ? error : new Error(message) } diff --git a/packages/cli/src/test/config-build-errors.test.ts b/packages/cli/src/test/config-build-errors.test.ts new file mode 100644 index 0000000..87deb84 --- /dev/null +++ b/packages/cli/src/test/config-build-errors.test.ts @@ -0,0 +1,56 @@ +import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +import { describe, expect, test } from 'bun:test' + +import { collectAggregateErrorMessages } from '../runtime/config.js' +import { runCli } from './testkit.test' + +describe('collectAggregateErrorMessages (#21)', () => { + test('extracts every sub-error message from an AggregateError', () => { + const agg = new AggregateError( + [new Error("Expected identifier but found \",\""), new Error('Expected "}" but found end of file')], + '2 errors building config.ts', + ) + expect(collectAggregateErrorMessages(agg)).toEqual([ + 'Expected identifier but found ","', + 'Expected "}" but found end of file', + ]) + }) + + test('handles plain { message } entries (Bun BuildMessage shape) and raw strings', () => { + expect(collectAggregateErrorMessages({ errors: [{ message: 'boom' }, 'raw'] })).toEqual([ + 'boom', + 'raw', + ]) + }) + + test('returns [] when there is no .errors array', () => { + expect(collectAggregateErrorMessages(new Error('plain'))).toEqual([]) + expect(collectAggregateErrorMessages(null)).toEqual([]) + }) +}) + +describe('@chkit/cli config syntax error surfaces the diagnostics (#21)', () => { + test('a config with a syntax error prints the underlying errors, not just the summary', async () => { + const dir = await mkdtemp(join(tmpdir(), 'chkit-badcfg-')) + const configPath = join(dir, 'clickhouse.config.ts') + // Deliberate syntax error (stray commas / unterminated object). + await writeFile( + configPath, + `export default {\n schema: './schema.ts',\n clickhouse: { url: 'http://x', database: 'd' ,,,\n}\n`, + 'utf8', + ) + try { + const result = runCli(['status', '--config', configPath], { HOME: dir, XDG_CONFIG_HOME: dir }) + expect(result.exitCode).not.toBe(0) + const combined = `${result.stdout}\n${result.stderr}` + expect(combined).toContain('Failed to build config') + // The actual diagnostic — not just "N errors building config.ts". + expect(combined).toContain('Expected') + } finally { + await rm(dir, { recursive: true, force: true }) + } + }) +}) diff --git a/packages/cli/src/test/migrate-statement-error.e2e.test.ts b/packages/cli/src/test/migrate-statement-error.e2e.test.ts new file mode 100644 index 0000000..8f1b2f5 --- /dev/null +++ b/packages/cli/src/test/migrate-statement-error.e2e.test.ts @@ -0,0 +1,78 @@ +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +import { describe, expect, test } from 'bun:test' + +import { + createJournalTableName, + createLiveExecutor, + createPrefix, + getRequiredEnv, + quoteIdent, + runCli, +} from './e2e-testkit.js' + +describe('@chkit/cli migrate statement-error context e2e (#10)', () => { + test('a rejected statement reports the migration file, statement position, and SQL preview', async () => { + const liveEnv = getRequiredEnv() + const executor = createLiveExecutor(liveEnv) + const database = liveEnv.clickhouseDatabase + const journalTable = createJournalTableName('stmterr') + const prefix = createPrefix('stmterr') + const table = `${prefix}t` + const dir = await mkdtemp(join(tmpdir(), 'chkit-stmterr-e2e-')) + const migrationsDir = join(dir, 'chkit/migrations') + const configPath = join(dir, 'clickhouse.config.ts') + const migrationName = '20990101000000_break.sql' + + try { + await mkdir(migrationsDir, { recursive: true }) + await writeFile( + configPath, + `export default {\n` + + ` schema: '${join(dir, 'schema.ts')}',\n` + + ` outDir: '${join(dir, 'chkit')}',\n` + + ` migrationsDir: '${migrationsDir}',\n` + + ` metaDir: '${join(dir, 'chkit/meta')}',\n` + + ` clickhouse: {\n` + + ` url: '${liveEnv.clickhouseUrl}',\n` + + ` username: '${liveEnv.clickhouseUser}',\n` + + ` password: '${liveEnv.clickhousePassword}',\n` + + ` database: '${database}',\n` + + ` },\n}\n`, + 'utf8', + ) + + // Statement 1 succeeds; statement 2 references an invalid type → CH rejects it. + await writeFile( + join(migrationsDir, migrationName), + `CREATE TABLE IF NOT EXISTS ${database}.${table} (id UInt64) ENGINE = MergeTree ORDER BY id;\n` + + `ALTER TABLE ${database}.${table} ADD COLUMN bad NotARealType123;\n`, + 'utf8', + ) + + const result = runCli(dir, ['migrate', '--config', configPath, '--execute'], { + CHKIT_JOURNAL_TABLE: journalTable, + CI: '1', + }) + + expect(result.exitCode).not.toBe(0) + const combined = `${result.stdout}\n${result.stderr}` + // Names the file and the failed statement position... + expect(combined).toContain(`Migration ${migrationName} failed at statement 2 of 2`) + // ...shows a preview of the offending SQL... + expect(combined).toContain('ADD COLUMN bad NotARealType123') + // ...and still carries the underlying ClickHouse message. + expect(combined).toContain('NotARealType123') + } finally { + await executor + .command(`DROP TABLE IF EXISTS ${quoteIdent(database)}.${quoteIdent(table)}`) + .catch(() => {}) + await executor + .command(`DROP TABLE IF EXISTS ${quoteIdent(database)}.${quoteIdent(journalTable)}`) + .catch(() => {}) + await rm(dir, { recursive: true, force: true }) + } + }, 60_000) +}) diff --git a/packages/cli/src/test/statement-error.test.ts b/packages/cli/src/test/statement-error.test.ts new file mode 100644 index 0000000..8bd1805 --- /dev/null +++ b/packages/cli/src/test/statement-error.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, test } from 'bun:test' + +import { previewStatement, statementError } from '../commands/migrate/apply.js' + +describe('previewStatement / statementError (#10)', () => { + test('previewStatement collapses whitespace to one line', () => { + expect(previewStatement(' ALTER TABLE x\n ADD COLUMN y UInt64 ')).toBe( + 'ALTER TABLE x ADD COLUMN y UInt64', + ) + }) + + test('previewStatement caps length with an ellipsis', () => { + const long = `SELECT ${'a'.repeat(200)}` + const preview = previewStatement(long) + expect(preview.length).toBe(121) // 120 chars + the ellipsis + expect(preview.endsWith('…')).toBe(true) + }) + + test('statementError reports file, statement position, SQL preview, and the original message', () => { + const err = statementError({ + file: '20260101000000_x.sql', + index: 1, + total: 3, + statement: 'ALTER TABLE app.events ADD COLUMN bad NotARealType', + error: new Error('Unknown data type family: NotARealType'), + }) + expect(err.message).toContain('Migration 20260101000000_x.sql failed at statement 2 of 3') + expect(err.message).toContain('ALTER TABLE app.events ADD COLUMN bad NotARealType') + expect(err.message).toContain('Unknown data type family: NotARealType') + }) +})