From 0e5dc728b6ead5c04bdfae101e1ac00e3ab9f14e Mon Sep 17 00:00:00 2001 From: Alvaro Date: Wed, 10 Jun 2026 13:31:51 +0200 Subject: [PATCH] fix: polish error messages and noisy outputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six error-UX fixes from the prod-ready audit: - #19 — a table with `orderBy` but no `primaryKey` no longer crashes with a raw TypeError; PK defaults to orderBy (ClickHouse semantics). Guard normalizeKeyColumns against undefined. - #20 — built-in (core) command errors surface their clean message instead of the `Plugin "core" failed in command:` wrapper. Mark internal plugins and rethrow their command errors raw. - #24 — `chkit query` syntax errors no longer leak the injected FORMAT JSON clause, and the "Expected one of" token dump is capped. - #11 — connection errors whose reason is only in the message (no `.code`), e.g. a typo'd host, are recognized via message substrings and cleaned. - #38a — the post-apply message names the resolved journal table (CHKIT_JOURNAL_TABLE), not a hardcoded _chkit_migrations. - #38b — the ObsessionDB "no service selected" reminder is suppressed when a direct clickhouse target is configured (onInit now receives config). All fixes covered by unit + live e2e tests, verified against the .env.test cluster. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/error-message-polish.md | 15 ++++ packages/cli/src/bin/chkit.ts | 1 + packages/cli/src/commands/migrate/command.ts | 4 +- packages/cli/src/commands/query.ts | 54 +++++++++++--- packages/cli/src/plugins.ts | 8 +++ packages/cli/src/runtime/journal-store.ts | 2 +- .../cli/src/runtime/plugin-runtime/index.ts | 8 ++- .../src/test/journal-table-name.e2e.test.ts | 72 +++++++++++++++++++ .../cli/src/test/query-clean-error.test.ts | 40 +++++++++++ packages/cli/src/test/query-error.e2e.test.ts | 48 +++++++++++++ packages/clickhouse/src/index.test.ts | 16 +++++ packages/clickhouse/src/index.ts | 48 +++++++++---- packages/core/src/canonical.ts | 10 ++- packages/core/src/index.test.ts | 21 ++++++ packages/core/src/key-clause.ts | 4 +- packages/plugin-obsessiondb/src/index.test.ts | 39 ++++++++++ packages/plugin-obsessiondb/src/index.ts | 7 +- 17 files changed, 367 insertions(+), 30 deletions(-) create mode 100644 .changeset/error-message-polish.md create mode 100644 packages/cli/src/test/journal-table-name.e2e.test.ts create mode 100644 packages/cli/src/test/query-clean-error.test.ts create mode 100644 packages/cli/src/test/query-error.e2e.test.ts diff --git a/.changeset/error-message-polish.md b/.changeset/error-message-polish.md new file mode 100644 index 00000000..6179d375 --- /dev/null +++ b/.changeset/error-message-polish.md @@ -0,0 +1,15 @@ +--- +"chkit": patch +"@chkit/core": patch +"@chkit/clickhouse": patch +"@chkit/plugin-obsessiondb": patch +--- + +Polish error messages and a few noisy outputs: + +- A table defined with `orderBy` but no `primaryKey` no longer crashes with a raw `TypeError`; the primary key defaults to the order-by columns, matching ClickHouse. +- Built-in command errors (e.g. a rejected migration) surface their own clean message instead of being wrapped in `Plugin "core" failed in ...`. +- `chkit query` syntax errors no longer leak the injected `FORMAT JSON` clause the user never typed, and the "Expected one of" token dump is capped. +- Connection errors whose reason is only in the message (no `.code`) — e.g. a typo'd host — are now recognized and cleaned instead of leaking the raw client string. +- The post-apply message names the resolved journal table (respecting `CHKIT_JOURNAL_TABLE`) instead of a hardcoded `_chkit_migrations`. +- The ObsessionDB "authenticated but no service selected" reminder is suppressed when a direct `clickhouse` target is configured (it was layered in from a global login, not chosen for the project). diff --git a/packages/cli/src/bin/chkit.ts b/packages/cli/src/bin/chkit.ts index ab07c0bf..f693abb6 100644 --- a/packages/cli/src/bin/chkit.ts +++ b/packages/cli/src/bin/chkit.ts @@ -201,6 +201,7 @@ async function run(): Promise { isInteractive: process.stdin.isTTY === true && process.stderr.isTTY === true, jsonMode: argv.includes('--json'), flags: initFlags, + config, } await pluginRuntime.runOnInit(initCtx) diff --git a/packages/cli/src/commands/migrate/command.ts b/packages/cli/src/commands/migrate/command.ts index 8c9d2d93..35f3173b 100644 --- a/packages/cli/src/commands/migrate/command.ts +++ b/packages/cli/src/commands/migrate/command.ts @@ -5,7 +5,7 @@ import { defineFlags, typedFlags, type ChxPluginCommand } from '../../plugins.js import { resolveDirs } from '../../runtime/config.js' import { GLOBAL_FLAGS } from '../../runtime/global-flags.js' import { emitJson } from '../../runtime/json-output.js' -import { createJournalStore } from '../../runtime/journal-store.js' +import { createJournalStore, resolveJournalTableName } from '../../runtime/journal-store.js' import { extractMigrationMetadata } from '../../runtime/migration-metadata.js' import { findChecksumMismatches, @@ -221,6 +221,6 @@ async function cmdMigrate( return 0 } - console.log('\nMigrations recorded in ClickHouse _chkit_migrations table.') + console.log(`\nMigrations recorded in ClickHouse ${resolveJournalTableName()} table.`) return 0 } diff --git a/packages/cli/src/commands/query.ts b/packages/cli/src/commands/query.ts index aee38153..e93d3947 100644 --- a/packages/cli/src/commands/query.ts +++ b/packages/cli/src/commands/query.ts @@ -30,19 +30,57 @@ export const queryCommand: ChxPluginCommand = { } if (jsonMode) { - const payload = pluginContext.executor.queryJson - ? await pluginContext.executor.queryJson(sql) - : rowsToJsonResult(await pluginContext.executor.query>(sql)) - printQueryJson(payload) - return 0 + try { + const payload = pluginContext.executor.queryJson + ? await pluginContext.executor.queryJson(sql) + : rowsToJsonResult(await pluginContext.executor.query>(sql)) + printQueryJson(payload) + return 0 + } catch (error) { + throw cleanQueryError(error) + } } - const rows = await pluginContext.executor.query>(sql) - printRows(rows) - return 0 + try { + const rows = await pluginContext.executor.query>(sql) + printRows(rows) + return 0 + } catch (error) { + throw cleanQueryError(error) + } }, } +const INJECTED_FORMAT_RE = /\s+FORMAT\s+JSON(?:EachRow)?\b/gi +const EXPECTED_TOKEN_CAP = 8 + +/** + * ClickHouse syntax errors echo back the query — including the `FORMAT JSON` + * clause chkit injects, which the user never typed — and can append a ~40-item + * "Expected one of" token dump. Strip the injected FORMAT clause and cap the + * token list so the message reflects the SQL the user actually wrote. Returns + * the original error untouched when there is nothing to clean (e.g. connection + * errors already formatted upstream). + */ +export function cleanQueryError(error: unknown): unknown { + if (!(error instanceof Error)) return error + const original = error.message + let message = original.replace(INJECTED_FORMAT_RE, '') + message = message.replace(/Expected one of: ([^.]*)\./, (_match, list: string) => { + const tokens = list + .split(',') + .map((token) => token.trim()) + .filter((token) => token.length > 0) + if (tokens.length <= EXPECTED_TOKEN_CAP) return `Expected one of: ${tokens.join(', ')}.` + const shown = tokens.slice(0, EXPECTED_TOKEN_CAP).join(', ') + return `Expected one of: ${shown}, … (${tokens.length - EXPECTED_TOKEN_CAP} more).` + }) + if (message === original) return error + const cleaned = new Error(message.trim()) + cleaned.stack = error.stack + return cleaned +} + function printQueryJson(payload: ClickHouseJsonQueryResult): void { console.log(formatQueryJson(payload)) } diff --git a/packages/cli/src/plugins.ts b/packages/cli/src/plugins.ts index 4813a420..d21ccff3 100644 --- a/packages/cli/src/plugins.ts +++ b/packages/cli/src/plugins.ts @@ -33,6 +33,13 @@ export interface LoadedPlugin { options: Record rawOptions: Record plugin: ChxPlugin + /** + * True for built-in plugins (e.g. `core`) loaded by the CLI itself rather + * than from the user's config. Their hook/command errors are surfaced raw, + * without the `Plugin "" failed in ...` wrapper that only makes sense + * for third-party plugins the user explicitly registered. + */ + internal?: boolean } export interface PluginRuntime { @@ -166,6 +173,7 @@ export interface ChxOnInitContext { isInteractive: boolean jsonMode: boolean flags: ParsedFlags + config: ResolvedChxConfig options: Record } diff --git a/packages/cli/src/runtime/journal-store.ts b/packages/cli/src/runtime/journal-store.ts index bf132711..4b5d3a9b 100644 --- a/packages/cli/src/runtime/journal-store.ts +++ b/packages/cli/src/runtime/journal-store.ts @@ -36,7 +36,7 @@ export interface JournalStore { const DEFAULT_JOURNAL_TABLE = '_chkit_migrations' -function resolveJournalTableName(): string { +export function resolveJournalTableName(): string { const candidate = process.env.CHKIT_JOURNAL_TABLE?.trim() if (!candidate) return DEFAULT_JOURNAL_TABLE if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(candidate)) { diff --git a/packages/cli/src/runtime/plugin-runtime/index.ts b/packages/cli/src/runtime/plugin-runtime/index.ts index 25ac82d3..4c21ac61 100644 --- a/packages/cli/src/runtime/plugin-runtime/index.ts +++ b/packages/cli/src/runtime/plugin-runtime/index.ts @@ -88,7 +88,7 @@ export async function loadPluginRuntime(input: { for (const plugin of input.internalPlugins ?? []) { if (byName.has(plugin.manifest.name)) continue const resolved = resolvePluginOptions(plugin, {}) - const item: LoadedPlugin = { plugin, options: resolved, rawOptions: {} } + const item: LoadedPlugin = { plugin, options: resolved, rawOptions: {}, internal: true } loaded.push(item) byName.set(plugin.manifest.name, item) } @@ -224,6 +224,12 @@ export async function loadPluginRuntime(input: { }) return typeof code === 'number' ? code : 0 } catch (error) { + // Built-in plugins (core) own the user-facing commands; their errors + // should read as the command's own clean message, not as a leaked + // internal-plugin failure. Only wrap third-party plugin errors. + if (item.internal) { + throw error instanceof Error ? error : new Error(String(error)) + } throw formatPluginError( item.plugin.manifest.name, `command:${commandName}`, diff --git a/packages/cli/src/test/journal-table-name.e2e.test.ts b/packages/cli/src/test/journal-table-name.e2e.test.ts new file mode 100644 index 00000000..a5ecdeaa --- /dev/null +++ b/packages/cli/src/test/journal-table-name.e2e.test.ts @@ -0,0 +1,72 @@ +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 journal-table message e2e (#38)', () => { + test('the post-apply message names the resolved journal table, not a hardcoded _chkit_migrations', async () => { + const liveEnv = getRequiredEnv() + const executor = createLiveExecutor(liveEnv) + const database = liveEnv.clickhouseDatabase + const journalTable = createJournalTableName('jname') + const prefix = createPrefix('jname') + const table = `${prefix}t` + const dir = await mkdtemp(join(tmpdir(), 'chkit-jname-e2e-')) + const migrationsDir = join(dir, 'chkit/migrations') + const configPath = join(dir, 'clickhouse.config.ts') + + 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', + ) + + await writeFile( + join(migrationsDir, '20990101000000_create.sql'), + `CREATE TABLE IF NOT EXISTS ${database}.${table} (id UInt64) ENGINE = MergeTree ORDER BY id;\n`, + 'utf8', + ) + + // Non-JSON apply so the human "recorded in ClickHouse " line prints. + const result = runCli(dir, ['migrate', '--config', configPath, '--execute'], { + CHKIT_JOURNAL_TABLE: journalTable, + CI: '1', + }) + + expect(result.exitCode).toBe(0) + expect(result.stdout).toContain(`recorded in ClickHouse ${journalTable} table`) + // The default name must NOT be printed when an override is in effect. + expect(result.stdout).not.toContain('_chkit_migrations table') + } 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/query-clean-error.test.ts b/packages/cli/src/test/query-clean-error.test.ts new file mode 100644 index 00000000..57117573 --- /dev/null +++ b/packages/cli/src/test/query-clean-error.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, test } from 'bun:test' + +import { cleanQueryError } from '../commands/query.js' + +describe('cleanQueryError (#24)', () => { + test('strips the injected FORMAT JSONEachRow chkit appends to the human query path', () => { + const raw = new Error( + 'Code: 62. DB::Exception: Syntax error: failed at position 13 ((): ( FROM users FORMAT JSONEachRow. Unmatched parentheses: (. (SYNTAX_ERROR)', + ) + const cleaned = cleanQueryError(raw) as Error + expect(cleaned.message).not.toContain('FORMAT JSONEachRow') + expect(cleaned.message).toContain('( FROM users.') + }) + + test('strips the injected FORMAT JSON chkit appends to the --json query path', () => { + const raw = new Error('Syntax error at position 5: SELECT FORMAT JSON. (SYNTAX_ERROR)') + const cleaned = cleanQueryError(raw) as Error + expect(cleaned.message).not.toContain('FORMAT JSON') + }) + + test('caps the "Expected one of" token dump', () => { + const raw = new Error( + 'Syntax error: failed at position 10 (2): 2 3. Expected one of: token, OR, AND, IS NULL, BETWEEN, LIKE, IN, MOD, AS, FROM, WHERE, GROUP BY, ORDER BY, LIMIT, FORMAT, end of query.', + ) + const cleaned = cleanQueryError(raw) as Error + expect(cleaned.message).toContain('Expected one of: token, OR, AND, IS NULL, BETWEEN, LIKE, IN, MOD, … (8 more).') + // None of the capped-off tokens leak. + expect(cleaned.message).not.toContain('GROUP BY') + }) + + test('returns the original error untouched when there is nothing to clean', () => { + const raw = new Error('Could not connect to ClickHouse at https://x (connection refused)') + expect(cleanQueryError(raw)).toBe(raw) + }) + + test('passes non-Error values through unchanged', () => { + expect(cleanQueryError('boom')).toBe('boom') + expect(cleanQueryError(undefined)).toBe(undefined) + }) +}) diff --git a/packages/cli/src/test/query-error.e2e.test.ts b/packages/cli/src/test/query-error.e2e.test.ts new file mode 100644 index 00000000..de83447d --- /dev/null +++ b/packages/cli/src/test/query-error.e2e.test.ts @@ -0,0 +1,48 @@ +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 { getRequiredEnv, runCli } from './e2e-testkit.js' + +describe('@chkit/cli query error surface e2e', () => { + test('a SQL syntax error is clean: no "Plugin core failed" wrapper (#20), no injected FORMAT (#24)', async () => { + const { clickhouseUrl, clickhouseUser, clickhousePassword, clickhouseDatabase } = + getRequiredEnv() + const dir = await mkdtemp(join(tmpdir(), 'chkit-query-e2e-')) + const configPath = join(dir, 'clickhouse.config.ts') + await writeFile( + configPath, + `export default {\n` + + ` schema: '${join(dir, 'schema.ts')}',\n` + + ` outDir: '${join(dir, 'chkit')}',\n` + + ` migrationsDir: '${join(dir, 'chkit/migrations')}',\n` + + ` metaDir: '${join(dir, 'chkit/meta')}',\n` + + ` clickhouse: {\n` + + ` url: '${clickhouseUrl}',\n` + + ` username: '${clickhouseUser}',\n` + + ` password: '${clickhousePassword}',\n` + + ` database: '${clickhouseDatabase}',\n` + + ` },\n}\n`, + 'utf8', + ) + + try { + const result = runCli(dir, ['query', 'SELECT count( FROM system.one', '--config', configPath], { + HOME: dir, + XDG_CONFIG_HOME: dir, + }) + expect(result.exitCode).toBe(1) + const combined = `${result.stdout}\n${result.stderr}` + // The underlying ClickHouse syntax error still surfaces... + expect(combined.toLowerCase()).toContain('syntax error') + // ...but core is a built-in plugin, so its command error is not wrapped. + expect(combined).not.toContain('Plugin "core" failed') + // ...and the FORMAT clause chkit injects is stripped (the user never typed it). + expect(combined).not.toContain('FORMAT JSON') + } finally { + await rm(dir, { recursive: true, force: true }) + } + }, 30_000) +}) diff --git a/packages/clickhouse/src/index.test.ts b/packages/clickhouse/src/index.test.ts index f6ed5950..b5e3cffb 100644 --- a/packages/clickhouse/src/index.test.ts +++ b/packages/clickhouse/src/index.test.ts @@ -466,6 +466,22 @@ describe('formatConnectionError', () => { ) }) + test('detects a host typo from the message when .code is stripped (#11)', () => { + // Some @clickhouse/client / Node versions surface the failure only in the + // message with no .code, so the bare-code match misses it and the raw + // library string leaks. + const error = new Error('getaddrinfo ENOTFOUND db.exampl-typo.com. Was there a typo in the url or port?') + expect(formatConnectionError(error, 'https://db.exampl-typo.com:8443')).toBe( + 'Could not connect to ClickHouse at https://db.exampl-typo.com:8443 (host not found)', + ) + }) + + test('detects connection refused from the message alone (no .code)', () => { + expect(formatConnectionError(new Error('connect ECONNREFUSED 127.0.0.1:8443'), 'https://x')).toBe( + 'Could not connect to ClickHouse at https://x (connection refused)', + ) + }) + test('returns undefined for unrecognized errors (caller rethrows raw)', () => { expect(formatConnectionError(new Error('Unknown data type family: Nope'), 'https://x')).toBeUndefined() }) diff --git a/packages/clickhouse/src/index.ts b/packages/clickhouse/src/index.ts index 1411373b..a270e80c 100644 --- a/packages/clickhouse/src/index.ts +++ b/packages/clickhouse/src/index.ts @@ -327,6 +327,31 @@ const NETWORK_ERROR_LABELS: Record = { EHOSTUNREACH: 'host unreachable', } +/** + * Some @clickhouse/client and Node versions surface a network failure with the + * reason only in the message (the `.code` is stripped), so a bare match on + * `.code` misses them and the raw library string leaks. Recover the label from + * the message as a fallback — e.g. a typo'd host that yields a `getaddrinfo + * ENOTFOUND` / "Was there a typo" string. + */ +const NETWORK_MESSAGE_PATTERNS: Array<[RegExp, string]> = [ + [/ENOTFOUND|getaddrinfo|EAI_AGAIN|Was there a typo/i, 'host not found'], + [/ECONNREFUSED/i, 'connection refused'], + [/ETIMEDOUT|timed out/i, 'connection timed out'], + [/ECONNRESET/i, 'connection reset'], + [/EHOSTUNREACH/i, 'host unreachable'], +] + +function networkLabelFromError(error: Error): string | undefined { + const code = + 'code' in error ? String((error as NodeJS.ErrnoException).code ?? '') : '' + if (code && NETWORK_ERROR_LABELS[code]) return NETWORK_ERROR_LABELS[code] + for (const [pattern, label] of NETWORK_MESSAGE_PATTERNS) { + if (pattern.test(error.message)) return label + } + return undefined +} + /** * ClickHouse reports a wrong/missing password with server-side error codes 194 * (REQUIRED_PASSWORD) or 516 (AUTHENTICATION_FAILED) and a multi-line message @@ -356,19 +381,16 @@ export function formatConnectionError( const who = username ? `user "${username}"` : 'the configured user' return `Authentication failed for ${who} at ${url}. Check CLICKHOUSE_USER / CLICKHOUSE_PASSWORD.` } - if ('code' in error) { - const code = (error as NodeJS.ErrnoException).code ?? '' - const label = NETWORK_ERROR_LABELS[code] - if (label) { - const isLocalhostDefault = - /^https?:\/\/(localhost|127\.0\.0\.1):8123\/?$/.test(url) - const envUnset = !process.env.CLICKHOUSE_URL - const hint = - isLocalhostDefault && envUnset - ? '\n Hint: CLICKHOUSE_URL is not set — chkit fell back to the default localhost endpoint. Set CLICKHOUSE_URL to point at your ClickHouse instance.' - : '' - return `Could not connect to ClickHouse at ${url} (${label})${hint}` - } + const label = networkLabelFromError(error) + if (label) { + const isLocalhostDefault = + /^https?:\/\/(localhost|127\.0\.0\.1):8123\/?$/.test(url) + const envUnset = !process.env.CLICKHOUSE_URL + const hint = + isLocalhostDefault && envUnset + ? '\n Hint: CLICKHOUSE_URL is not set — chkit fell back to the default localhost endpoint. Set CLICKHOUSE_URL to point at your ClickHouse instance.' + : '' + return `Could not connect to ClickHouse at ${url} (${label})${hint}` } return undefined } diff --git a/packages/core/src/canonical.ts b/packages/core/src/canonical.ts index c809133b..fa809d6d 100644 --- a/packages/core/src/canonical.ts +++ b/packages/core/src/canonical.ts @@ -60,6 +60,12 @@ function canonicalizeTable(def: TableDefinition): TableDefinition { ? sortByName(def.projections).map(canonicalizeProjection) : undefined + const orderBy = normalizeKeyColumns(def.orderBy) + // ClickHouse derives the primary key from ORDER BY when PRIMARY KEY is + // omitted. Mirror that so a table with `orderBy` but no `primaryKey` is + // valid instead of crashing on `undefined.flatMap`. + const primaryKey = normalizeKeyColumns(def.primaryKey) + return { ...def, database: def.database.trim(), @@ -72,8 +78,8 @@ function canonicalizeTable(def: TableDefinition): TableDefinition { : undefined, engine: normalizeEngine(def.engine), columns: def.columns.map(canonicalizeColumn), - primaryKey: normalizeKeyColumns(def.primaryKey), - orderBy: normalizeKeyColumns(def.orderBy), + primaryKey: primaryKey.length > 0 ? primaryKey : orderBy, + orderBy, uniqueKey: def.uniqueKey ? normalizeKeyColumns(def.uniqueKey) : undefined, partitionBy: def.partitionBy ? normalizeSQLFragment(def.partitionBy) : undefined, ttl: def.ttl ? normalizeSQLFragment(def.ttl) : undefined, diff --git a/packages/core/src/index.test.ts b/packages/core/src/index.test.ts index 707ca4f9..14f16646 100644 --- a/packages/core/src/index.test.ts +++ b/packages/core/src/index.test.ts @@ -77,6 +77,27 @@ describe('@chkit/core smoke', () => { expect(sql).toContain('UNIQUE KEY (`id`, `org_id`)') }) + test('table with orderBy but no primaryKey does not crash; PK defaults to orderBy (#19)', () => { + // A user can omit primaryKey at runtime (JS, or a `.ts` config without + // strict types). ClickHouse derives the PK from ORDER BY, so this must + // canonicalize cleanly instead of crashing on `undefined.flatMap`. + const events = table({ + database: 'app', + name: 'events', + columns: [{ name: 'id', type: 'UInt64' }], + engine: 'MergeTree()', + orderBy: ['id'], + // primaryKey intentionally omitted + } as Parameters[0]) + + const [canonical] = canonicalizeDefinitions([events]) + expect((canonical as { primaryKey: string[] }).primaryKey).toEqual(['id']) + + const sql = toCreateSQL(canonical) + expect(sql).toContain('PRIMARY KEY (`id`)') + expect(sql).toContain('ORDER BY (`id`)') + }) + test('collects and de-duplicates definitions from module exports', () => { const users = table({ database: 'app', diff --git a/packages/core/src/key-clause.ts b/packages/core/src/key-clause.ts index a813d1d6..f4dfb38a 100644 --- a/packages/core/src/key-clause.ts +++ b/packages/core/src/key-clause.ts @@ -48,6 +48,6 @@ export function splitTopLevelComma(input: string): string[] { return out } -export function normalizeKeyColumns(values: string[]): string[] { - return values.flatMap((value) => splitTopLevelComma(value.trim())) +export function normalizeKeyColumns(values: string[] | undefined): string[] { + return (values ?? []).flatMap((value) => splitTopLevelComma(value.trim())) } diff --git a/packages/plugin-obsessiondb/src/index.test.ts b/packages/plugin-obsessiondb/src/index.test.ts index 31065c88..4391366b 100644 --- a/packages/plugin-obsessiondb/src/index.test.ts +++ b/packages/plugin-obsessiondb/src/index.test.ts @@ -444,6 +444,45 @@ describe('obsessiondb onInit', () => { expect(log.mock.calls.flat().join('\n')).toContain('using service "prod"') log.mockRestore() }) + + test('warns "no service selected" when authenticated with no service and no clickhouse target (#38)', async () => { + await setupAuth() + + const log = spyOn(console, 'log').mockImplementation(() => {}) + const plugin = obsessiondb().plugin + await plugin.hooks.onInit({ + command: 'status', + configPath: join(tempDir, 'clickhouse.config.ts'), + isInteractive: false, + jsonMode: false, + flags: {}, + }) + + expect(log.mock.calls.flat().join('\n')).toContain('authenticated but no service selected') + log.mockRestore() + }) + + test('suppresses the "no service selected" notice when a direct clickhouse target is configured (#38)', async () => { + await setupAuth() + + const log = spyOn(console, 'log').mockImplementation(() => {}) + const plugin = obsessiondb().plugin + await plugin.hooks.onInit({ + command: 'status', + configPath: join(tempDir, 'clickhouse.config.ts'), + isInteractive: false, + jsonMode: false, + flags: {}, + config: { + clickhouse: { url: 'https://x', username: 'u', password: 'p', database: 'd' }, + } as never, + }) + + // ObsessionDB was layered in from a global login, not chosen for this + // project, so the reminder is noise for a user with their own target. + expect(log.mock.calls.flat().join('\n')).not.toContain('authenticated but no service selected') + log.mockRestore() + }) }) describe('obsessiondb getContext', () => { diff --git a/packages/plugin-obsessiondb/src/index.ts b/packages/plugin-obsessiondb/src/index.ts index 94ed29a9..fc140e21 100644 --- a/packages/plugin-obsessiondb/src/index.ts +++ b/packages/plugin-obsessiondb/src/index.ts @@ -84,6 +84,7 @@ interface ObsessionDBPlugin { isInteractive: boolean jsonMode: boolean flags: Record + config?: ResolvedChxConfig }): Promise onSchemaLoaded(context: { config: ResolvedChxConfig @@ -309,7 +310,11 @@ function createObsessionDBPlugin( } if (service) { console.log(`obsessiondb: using service "${service.service_name}"\n`) - } else if (context.command === 'query') { + } else if (context.command === 'query' || context.config?.clickhouse) { + // Suppress the "no service selected" reminder when the user has a + // direct `clickhouse` target configured: ObsessionDB was layered in + // from a global `chkit obsessiondb login` profile, not chosen for this + // project, so service selection is irrelevant and the notice is noise. return } else { console.log(