From 50acdf264237d62921814ec1e037b8f612634712 Mon Sep 17 00:00:00 2001 From: Danilo Alonso Date: Fri, 18 Sep 2026 17:43:16 -0400 Subject: [PATCH 1/2] fix(tui): stop stale rows from repeated row keys Repeated SelectList keys leave stale rows on screen. Index and FK names repeat across tables, PostgreSQL overloads share a name, and rule descriptions are free text. --- .changeset/tui-explore-duplicate-keys.md | 6 + docs/dev/explore.md | 1 + src/core/explore/dialects/postgres.ts | 10 +- src/core/explore/types.ts | 6 + src/tui/screens/db/DbTruncateScreen.tsx | 4 +- .../screens/db/explore/ExploreListScreen.tsx | 61 ++++--- .../settings/SettingsRulesListScreen.tsx | 54 ++++-- tests/cli/screens/db/explore-list.test.tsx | 161 ++++++++++++++++++ .../cli/screens/settings/rules-list.test.tsx | 61 +++++++ tests/integration/explore/postgres.test.ts | 54 ++++++ 10 files changed, 383 insertions(+), 35 deletions(-) create mode 100644 .changeset/tui-explore-duplicate-keys.md create mode 100644 tests/cli/screens/db/explore-list.test.tsx create mode 100644 tests/cli/screens/settings/rules-list.test.tsx diff --git a/.changeset/tui-explore-duplicate-keys.md b/.changeset/tui-explore-duplicate-keys.md new file mode 100644 index 00000000..2066d8d9 --- /dev/null +++ b/.changeset/tui-explore-duplicate-keys.md @@ -0,0 +1,6 @@ +--- +'@noormdev/cli': patch +'@noormdev/sdk': patch +--- + +Stop TUI lists from leaving stale rows on screen when two rows share an identity: explore indexes and foreign keys whose names repeat across tables (SQL Server's `IX_UserId`, MySQL's `PRIMARY`), PostgreSQL function and procedure overloads, and settings rules with the same description. `listFunctions` and `listProcedures` now return a `signature` on PostgreSQL that tells overloads apart. diff --git a/docs/dev/explore.md b/docs/dev/explore.md index c167e520..cf1856a2 100644 --- a/docs/dev/explore.md +++ b/docs/dev/explore.md @@ -222,6 +222,7 @@ interface FunctionSummary { schema?: string parameterCount: number returnType: string + signature?: string // PostgreSQL only: argument list that tells overloads apart } interface IndexSummary { diff --git a/src/core/explore/dialects/postgres.ts b/src/core/explore/dialects/postgres.ts index 14f7812b..f1c6233a 100644 --- a/src/core/explore/dialects/postgres.ts +++ b/src/core/explore/dialects/postgres.ts @@ -142,11 +142,13 @@ export const postgresExploreOperations: DialectExploreOperations = { proname: string; nspname: string; param_count: string; + identity_args: string; }>` SELECT p.proname, n.nspname, - p.pronargs::text as param_count + p.pronargs::text as param_count, + pg_get_function_identity_arguments(p.oid) as identity_args FROM pg_proc p JOIN pg_namespace n ON p.pronamespace = n.oid WHERE n.nspname ${schemaFilter(schema)} @@ -163,6 +165,7 @@ export const postgresExploreOperations: DialectExploreOperations = { name: row.proname, schema: row.nspname, parameterCount: parseInt(row.param_count, 10), + signature: row.identity_args, })); }, @@ -175,12 +178,14 @@ export const postgresExploreOperations: DialectExploreOperations = { nspname: string; param_count: string; return_type: string; + identity_args: string; }>` SELECT p.proname, n.nspname, p.pronargs::text as param_count, - pg_get_function_result(p.oid) as return_type + pg_get_function_result(p.oid) as return_type, + pg_get_function_identity_arguments(p.oid) as identity_args FROM pg_proc p JOIN pg_namespace n ON p.pronamespace = n.oid WHERE n.nspname ${schemaFilter(schema)} @@ -198,6 +203,7 @@ export const postgresExploreOperations: DialectExploreOperations = { schema: row.nspname, parameterCount: parseInt(row.param_count, 10), returnType: row.return_type, + signature: row.identity_args, })); }, diff --git a/src/core/explore/types.ts b/src/core/explore/types.ts index 9fd5f080..a38c14da 100644 --- a/src/core/explore/types.ts +++ b/src/core/explore/types.ts @@ -76,6 +76,9 @@ export interface ProcedureSummary { schema?: string; parameterCount: number; + /** Argument list that tells PostgreSQL overloads apart, e.g. `v integer, w text`. */ + signature?: string; + } /** @@ -88,6 +91,9 @@ export interface FunctionSummary { parameterCount: number; returnType: string; + /** Argument list that tells PostgreSQL overloads apart, e.g. `v integer, w text`. */ + signature?: string; + } /** diff --git a/src/tui/screens/db/DbTruncateScreen.tsx b/src/tui/screens/db/DbTruncateScreen.tsx index 89a53b48..bb4bdafe 100644 --- a/src/tui/screens/db/DbTruncateScreen.tsx +++ b/src/tui/screens/db/DbTruncateScreen.tsx @@ -274,7 +274,7 @@ export function DbTruncateScreen({ params: _params }: ScreenProps): ReactElement Tables to truncate ({toTruncate.length}): {toTruncate.length > 0 ? ( toTruncate.slice(0, 10).map((t) => ( - - {t.name} + - {t.name} )) ) : ( (none) @@ -288,7 +288,7 @@ export function DbTruncateScreen({ params: _params }: ScreenProps): ReactElement Preserved ({toPreserve.length}): {toPreserve.slice(0, 5).map((t) => ( - + {' '}- {t.name} {t.name.startsWith('__noorm_') ? '(system)' : '(settings)'} ))} diff --git a/src/tui/screens/db/explore/ExploreListScreen.tsx b/src/tui/screens/db/explore/ExploreListScreen.tsx index 2b2f4b80..e0a750cc 100644 --- a/src/tui/screens/db/explore/ExploreListScreen.tsx +++ b/src/tui/screens/db/explore/ExploreListScreen.tsx @@ -113,6 +113,44 @@ type AnySummary = | IndexSummary | ForeignKeySummary; +function exploreItemKey(item: AnySummary, label: string): string { + + if ('tableName' in item) return `${label} on ${item.tableName}`; + + if ('signature' in item && item.signature !== undefined) return `${label}(${item.signature})`; + + return label; + +} + +/** + * Builds the list rows for one explore category. + * + * `schema.name` alone repeats: index and foreign key names are unique only + * within their table (MySQL names every primary key `PRIMARY`), and + * PostgreSQL overloads share a name. + * + * @example + * const rows = exploreListItems('indexes', indexes); + * // rows[0].key === 'dbo.IX_UserId on AspNetUserLogins' + */ +export function exploreListItems(category: ExploreCategory, items: AnySummary[]): SelectListItem[] { + + return items.map((item) => { + + const label = item.schema ? `${item.schema}.${item.name}` : item.name; + + return { + key: exploreItemKey(item, label), + label, + value: item, + description: formatSummaryDescription(category, item), + }; + + }); + +} + /** * ExploreListScreen component. * @@ -182,25 +220,10 @@ export function ExploreListScreen({ params: _params }: ScreenProps): ReactElemen }, [db, dialect, meta, settings?.logging?.level]); // Convert items to SelectListItem format - const listItems = useMemo((): SelectListItem[] => { - - if (!meta) return []; - - return items.map((item) => { - - const name = item.name; - const schema = 'schema' in item ? (item as { schema?: string }).schema : undefined; - - return { - key: schema ? `${schema}.${name}` : name, - label: schema ? `${schema}.${name}` : name, - value: item, - description: formatSummaryDescription(meta.category, item), - }; - - }); - - }, [items, meta]); + const listItems = useMemo( + () => (meta ? exploreListItems(meta.category, items) : []), + [items, meta], + ); // Handle item selection const handleSelect = (item: SelectListItem) => { diff --git a/src/tui/screens/settings/SettingsRulesListScreen.tsx b/src/tui/screens/settings/SettingsRulesListScreen.tsx index ad4b844c..5dcc5ae5 100644 --- a/src/tui/screens/settings/SettingsRulesListScreen.tsx +++ b/src/tui/screens/settings/SettingsRulesListScreen.tsx @@ -14,7 +14,6 @@ import { useState, useCallback, useMemo } from 'react'; import { Box, Text, useInput } from 'ink'; import { attempt } from '@logosdx/utils'; -import v from 'voca'; import type { ReactElement } from 'react'; import type { ScreenProps } from '../../types.js'; @@ -35,7 +34,7 @@ import { /** * Rule list item value. */ -interface RuleListValue { +export interface RuleListValue { index: number; rule: Rule; } @@ -79,6 +78,46 @@ function formatEffect(rule: Rule): string { } +/** + * Builds the list rows for the settings rules. + * + * Keyed by description so the remembered cursor follows a rule the edit + * screen moves to the end on save; the occurrence count separates rules + * described alike. A rule without a description has only its position. + * + * @example + * const rows = ruleListItems(settings.rules ?? []); + * navigate('settings/rules/edit', { name: String(rows[0].value.index) }); + */ +export function ruleListItems(rules: Rule[]): SelectListItem[] { + + const occurrences = new Map(); + + return rules.map((rule, index) => { + + const { description } = rule; + let key = String(index); + + if (description) { + + const occurrence = (occurrences.get(description) ?? 0) + 1; + + occurrences.set(description, occurrence); + key = `${description}#${occurrence}`; + + } + + return { + key, + label: description || `Rule ${index + 1}`, + value: { index, rule }, + description: `${formatMatch(rule.match)} → ${formatEffect(rule)}`, + }; + + }); + +} + /** * SettingsRulesListScreen component. */ @@ -103,16 +142,7 @@ export function SettingsRulesListScreen({ params: _params }: ScreenProps): React }, [settings]); // Convert rules to list items - const items: SelectListItem[] = useMemo(() => { - - return rules.map((rule, index) => ({ - key: rule.description ? v.kebabCase(rule.description) : String(index), - label: rule.description || `Rule ${index + 1}`, - value: { index, rule }, - description: `${formatMatch(rule.match)} → ${formatEffect(rule)}`, - })); - - }, [rules]); + const items = useMemo(() => ruleListItems(rules), [rules]); // Set initial highlighted index useMemo(() => { diff --git a/tests/cli/screens/db/explore-list.test.tsx b/tests/cli/screens/db/explore-list.test.tsx new file mode 100644 index 00000000..16bc675d --- /dev/null +++ b/tests/cli/screens/db/explore-list.test.tsx @@ -0,0 +1,161 @@ +/** + * Explore list row-identity tests. + * + * `schema.name` does not identify an index: SQL Server reuses `IX_UserId` + * across tables and MySQL names every primary key `PRIMARY`. React keeps a + * row whose key repeats, so scrolling a `SelectList` leaves the outgoing rows + * drawn over the live window. + * + * Rows come from the real `exploreListItems` builder, so the keys under test + * are the keys the screen hands to the list. + */ +import { describe, it, expect } from 'bun:test'; +import { render } from 'ink-testing-library'; +import React from 'react'; + +import type { FunctionSummary, IndexSummary } from '../../../../src/core/explore/types.js'; + +import { FocusProvider } from '../../../../src/tui/focus.js'; +import { SelectList } from '../../../../src/tui/components/lists/index.js'; +import { exploreListItems } from '../../../../src/tui/screens/db/explore/ExploreListScreen.js'; + +/** Rows the list may draw, pinned so the assertions do not depend on a terminal. */ +const VISIBLE = 5; + +const DOWN = '\x1B[B'; + +// eslint-disable-next-line no-control-regex -- matching the ANSI SGR escape is the point +const ANSI_PATTERN = /\u001B\[[0-9;]*m/g; + +function strip(frame: string | undefined): string { + + return (frame ?? '').replace(ANSI_PATTERN, ''); + +} + +/** Table number of the row the cursor is on, or -1 before the list draws. */ +function cursorRow(frame: string | undefined): number { + + const match = /❯ .* on table_(\d+)/.exec(strip(frame)); + + return match ? Number(match[1]) : -1; + +} + +async function waitFor(predicate: () => boolean, timeoutMs = 2000): Promise { + + const deadline = Date.now() + timeoutMs; + + while (!predicate() && Date.now() < deadline) { + + await new Promise((resolve) => setTimeout(resolve, 10)); + + } + +} + +/** + * Press a key until the list says what we are waiting for. Counting presses + * does not survive Ink: writes in a tight loop coalesce into one event, and + * under load a fixed press budget runs out before the cursor arrives. + */ +async function pressUntil( + stdin: { write: (data: string) => void }, + sequence: string, + predicate: () => boolean, + timeoutMs = 10000, +): Promise { + + const deadline = Date.now() + timeoutMs; + + while (!predicate() && Date.now() < deadline) { + + stdin.write(sequence); + + await new Promise((resolve) => setTimeout(resolve, 10)); + + } + +} + +/** Three index names shared across many tables, the SQL Server shape. */ +function sharedNameIndexes(tableCount: number): IndexSummary[] { + + return Array.from({ length: tableCount }, (_, index) => ({ + name: `IX_${['UserId', 'ProjectId', 'EmployeeId'][index % 3]}`, + schema: 'dbo', + tableName: `table_${String(index).padStart(2, '0')}`, + tableSchema: 'dbo', + columns: ['id'], + isUnique: false, + isPrimary: false, + })); + +} + +describe('cli: explore list row identity', () => { + + it('should give every MySQL primary key its own row', () => { + + const indexes: IndexSummary[] = ['users', 'orders', 'products'].map((tableName) => ({ + name: 'PRIMARY', + schema: 'shop', + tableName, + tableSchema: 'shop', + columns: ['id'], + isUnique: true, + isPrimary: true, + })); + + const keys = exploreListItems('indexes', indexes).map((item) => item.key); + + expect(new Set(keys).size).toBe(indexes.length); + + }); + + it('should give every PostgreSQL overload its own row', () => { + + // Same name, same arity, same return type: only the argument list tells + // these apart. + const overloads: FunctionSummary[] = ['v integer', 'v text'].map((signature) => ({ + name: 'normalize', + schema: 'public', + parameterCount: 1, + returnType: 'text', + signature, + })); + + const keys = exploreListItems('functions', overloads).map((item) => item.key); + + expect(new Set(keys).size).toBe(overloads.length); + + }); + + it('should draw only the live window after scrolling indexes whose names repeat', async () => { + + const items = exploreListItems('indexes', sharedNameIndexes(40)); + + const { stdin, lastFrame, unmount } = render( + + + , + ); + + await waitFor(() => strip(lastFrame()).includes('table_00')); + + // At or past a row, not on it: under load a press can land before the + // frame is read, and an exact target then costs a full wrap of the list. + await pressUntil(stdin, DOWN, () => cursorRow(lastFrame()) >= 15); + + const lines = strip(lastFrame()).split('\n'); + const rows = lines.filter((line) => line.includes(' on table_')); + const cursors = lines.filter((line) => line.includes('❯')); + + expect(rows).toHaveLength(VISIBLE); + expect(cursors).toHaveLength(1); + + unmount(); + + }); + +}); diff --git a/tests/cli/screens/settings/rules-list.test.tsx b/tests/cli/screens/settings/rules-list.test.tsx new file mode 100644 index 00000000..8f7c8839 --- /dev/null +++ b/tests/cli/screens/settings/rules-list.test.tsx @@ -0,0 +1,61 @@ +/** + * Settings rules list row-identity tests. + * + * Rule descriptions are free text with no uniqueness check, and the rule edit + * screen saves by removing the rule and appending it, so neither the + * description alone nor the position identifies a rule across a save. + */ +import { describe, it, expect } from 'bun:test'; + +import type { Rule } from '../../../../src/core/settings/types.js'; + +import { ruleListItems } from '../../../../src/tui/screens/settings/SettingsRulesListScreen.js'; + +describe('cli: settings rules list row identity', () => { + + it('should give every rule its own row', () => { + + const rules: Rule[] = [ + { match: { isTest: true } }, + { description: 'Exclude test tables', match: { isTest: true }, exclude: ['seeds'] }, + { description: 'Exclude test tables', match: { protected: true }, exclude: ['seeds'] }, + { description: '0', match: { protected: true } }, + ]; + + const keys = ruleListItems(rules).map((item) => item.key); + + expect(new Set(keys).size).toBe(rules.length); + + }); + + it('should keep a described rule\'s key apart from every position', () => { + + // Description `1`, first occurrence, against the undescribed rule at + // index 11: joined without a separator both would read `11`. + const rules: Rule[] = [ + { description: '1', match: { isTest: true } }, + ...Array.from({ length: 11 }, (): Rule => ({ match: { protected: true } })), + ]; + + const keys = ruleListItems(rules).map((item) => item.key); + + expect(new Set(keys).size).toBe(rules.length); + + }); + + it('should keep a rule\'s key when saving an edit moves it to the end', () => { + + const seeds: Rule = { description: 'Seeds', match: { isTest: true } }; + const prod: Rule = { description: 'Prod', match: { protected: true } }; + const local: Rule = { description: 'Local', match: { type: 'local' } }; + + const keyOfSeeds = (rules: Rule[]) => ruleListItems(rules).find((item) => item.value.rule === seeds)?.key; + + // The list remembers the cursor by key, so this is what puts it back on + // the rule the user just edited. + expect(keyOfSeeds([seeds, prod, local])).toBeDefined(); + expect(keyOfSeeds([prod, local, seeds])).toBe(keyOfSeeds([seeds, prod, local])); + + }); + +}); diff --git a/tests/integration/explore/postgres.test.ts b/tests/integration/explore/postgres.test.ts index 49c005e1..7051441c 100644 --- a/tests/integration/explore/postgres.test.ts +++ b/tests/integration/explore/postgres.test.ts @@ -5,6 +5,7 @@ * Requires docker-compose.test.yml containers to be running. */ import { describe, it, expect, beforeAll, afterAll, beforeEach } from 'bun:test'; +import { sql } from 'kysely'; import type { Kysely } from 'kysely'; import { fetchOverview, fetchList, fetchDetail } from '../../../src/core/explore/index.js'; @@ -236,6 +237,59 @@ describe('integration: postgres explore', () => { }); + // Same arity on purpose, so parameterCount cannot stand in for the signature. + it('should tell same-arity function overloads apart by signature', async () => { + + await sql`CREATE FUNCTION overload_probe(v integer) RETURNS integer LANGUAGE sql AS 'SELECT v'`.execute(db); + await sql`CREATE FUNCTION overload_probe(v text) RETURNS integer LANGUAGE sql AS 'SELECT 1'`.execute(db); + + try { + + const functions = await fetchList(db, 'postgres', 'functions'); + const signatures = functions + .filter((f) => f.name === 'overload_probe') + .map((f) => f.signature); + + expect(signatures).not.toContain(undefined); + expect(new Set(signatures).size).toBe(2); + + } + finally { + + await sql`DROP FUNCTION IF EXISTS overload_probe(integer), overload_probe(text)`.execute(db); + + } + + }); + + }); + + describe('fetchList - procedures', () => { + + it('should tell same-arity procedure overloads apart by signature', async () => { + + await sql`CREATE PROCEDURE overload_probe_proc(v integer) LANGUAGE sql AS 'SELECT 1'`.execute(db); + await sql`CREATE PROCEDURE overload_probe_proc(v text) LANGUAGE sql AS 'SELECT 1'`.execute(db); + + try { + + const procedures = await fetchList(db, 'postgres', 'procedures'); + const signatures = procedures + .filter((p) => p.name === 'overload_probe_proc') + .map((p) => p.signature); + + expect(signatures).not.toContain(undefined); + expect(new Set(signatures).size).toBe(2); + + } + finally { + + await sql`DROP PROCEDURE IF EXISTS overload_probe_proc(integer), overload_probe_proc(text)`.execute(db); + + } + + }); + }); describe('fetchList - indexes', () => { From 46d5cfde70969d86a1cac21db0fcbc81ad12bb58 Mon Sep 17 00:00:00 2001 From: Danilo Alonso Date: Fri, 18 Sep 2026 17:33:15 -0400 Subject: [PATCH 2/2] fix: let non-admin logins connect, explain failures, guard typing The binary read .env through Bun (expanding $, cutting at #), and MSSQL connected via master, which contained and non-VIEW-ANY-DATABASE logins cannot. Connection errors name their reason and log the server's code; global TUI shortcuts hold while a text field has input. --- .changeset/binary-no-dotenv-autoload.md | 5 + .changeset/connection-error-reasons.md | 6 + .changeset/mssql-unprivileged-logins.md | 6 + .changeset/tui-typing-blocks-shortcuts.md | 5 + .claude/rules/tui-development.md | 12 +- docs/guide/troubleshooting.md | 23 + docs/tui.md | 4 + docs/wiki/core-db.md | 5 +- scripts/build-binary.mjs | 6 +- src/core/connection/dialects/mssql.ts | 116 ++-- src/core/connection/errors.ts | 418 +++++++++++++++ src/core/connection/factory.ts | 87 +-- src/core/connection/index.ts | 1 + src/core/observer.ts | 9 +- src/tui/app-context.tsx | 10 - src/tui/components/forms/TextInput.tsx | 8 +- .../components/overlays/LogViewerOverlay.tsx | 4 +- src/tui/components/terminal/ResultTable.tsx | 3 + src/tui/components/terminal/SqlInput.tsx | 3 + src/tui/focus.tsx | 48 +- src/tui/keyboard.tsx | 31 +- src/tui/screens/db/SqlTerminalScreen.tsx | 17 +- src/tui/types.ts | 12 + tests/cli/keyboard.test.tsx | 156 +++++- tests/core/connection/dialects/mssql.test.ts | 9 - tests/core/connection/errors.test.ts | 113 ++++ .../connection/connection-errors.test.ts | 498 ++++++++++++++++++ .../connection/mssql-login.test.ts | 165 ++++++ .../connection/timeout-abort.test.ts | 11 +- 29 files changed, 1618 insertions(+), 173 deletions(-) create mode 100644 .changeset/binary-no-dotenv-autoload.md create mode 100644 .changeset/connection-error-reasons.md create mode 100644 .changeset/mssql-unprivileged-logins.md create mode 100644 .changeset/tui-typing-blocks-shortcuts.md create mode 100644 src/core/connection/errors.ts create mode 100644 tests/core/connection/errors.test.ts create mode 100644 tests/integration/connection/connection-errors.test.ts create mode 100644 tests/integration/connection/mssql-login.test.ts diff --git a/.changeset/binary-no-dotenv-autoload.md b/.changeset/binary-no-dotenv-autoload.md new file mode 100644 index 00000000..92936ac7 --- /dev/null +++ b/.changeset/binary-no-dotenv-autoload.md @@ -0,0 +1,5 @@ +--- +'@noormdev/cli': patch +--- + +Stop the `noorm` binary from loading `.env` in the working directory. Bun's loader expanded `$` and cut values at `#`, so a `NOORM_CONNECTION_PASSWORD` containing those characters reached the database as a different password and login failed. `NOORM_*` variables now come only from the process environment: export them in the shell or set them in CI. diff --git a/.changeset/connection-error-reasons.md b/.changeset/connection-error-reasons.md new file mode 100644 index 00000000..cea2dbe2 --- /dev/null +++ b/.changeset/connection-error-reasons.md @@ -0,0 +1,6 @@ +--- +'@noormdev/cli': patch +'@noormdev/sdk': patch +--- + +Say why a database connection failed: refused port, unknown host, timeout, rejected TLS certificate, disabled, locked, or expired account, missing grant, connection limit, missing password, and SQLite file or directory faults. Where the server withholds the reason (SQL Server 18456, PostgreSQL 28P01, MySQL 1045), the message says so and lists the usual causes. Exhausted retries report the server's last error. `connection:error` log entries carry `serverCode` and `serverMessage`. diff --git a/.changeset/mssql-unprivileged-logins.md b/.changeset/mssql-unprivileged-logins.md new file mode 100644 index 00000000..c5f1d580 --- /dev/null +++ b/.changeset/mssql-unprivileged-logins.md @@ -0,0 +1,6 @@ +--- +'@noormdev/cli': patch +'@noormdev/sdk': patch +--- + +Let MSSQL logins without server-level access connect. Connecting no longer detours through `master` to look the target up in `sys.databases`, so contained database users (the usual account on Azure SQL Database) and logins without `VIEW ANY DATABASE` can connect, and the config add/edit connection test passes for them. A database that is missing, or that the login cannot open, now fails with that reason instead of "Login failed". diff --git a/.changeset/tui-typing-blocks-shortcuts.md b/.changeset/tui-typing-blocks-shortcuts.md new file mode 100644 index 00000000..94a03224 --- /dev/null +++ b/.changeset/tui-typing-blocks-shortcuts.md @@ -0,0 +1,5 @@ +--- +'@noormdev/cli': patch +--- + +Stop global TUI shortcuts from firing while you type. A capital `L` or `Q` typed into a form field, search box, or the SQL editor used to open the log viewer or the SQL terminal, and `?`, `D`, and `F` could open help or toggle dry-run and force mode. While a text field is taking input, those keys now type their character. diff --git a/.claude/rules/tui-development.md b/.claude/rules/tui-development.md index 06e9879f..794c67fd 100644 --- a/.claude/rules/tui-development.md +++ b/.claude/rules/tui-development.md @@ -49,7 +49,7 @@ useInput((input, key) => { useInput(handler, { isActive: isFocused }); ``` -`useFocusedInput(isFocused, handler)` in `src/tui/keyboard.tsx:249` wraps this correctly. Prefer it. +`useFocusedInput(isFocused, handler)` in `src/tui/keyboard.tsx:250` wraps this correctly. Prefer it. ### One focus owner per screen @@ -172,9 +172,13 @@ if (key.downArrow) { ### Global keys -`GlobalKeyboard` (`src/tui/keyboard.tsx:114`) owns Ctrl+C, Shift+L, Shift+Q, `?`, `D`, and `F`. It deliberately does **not** handle Esc: each screen handles its own, because a global handler fires alongside the screen handler and pops history twice. +`GlobalKeyboard` (`src/tui/keyboard.tsx:115`) owns Ctrl+C, Shift+L, Shift+Q, `?`, `D`, and `F`. It deliberately does **not** handle Esc: each screen handles its own, because a global handler fires alongside the screen handler and pops history twice. -`?`, `D`, and `F` only fire when `stack.length <= 1`, so they stay inert while a text input is focused. +Every one of them but Ctrl+C stands down while a text field is taking keystrokes. A field says so with `useTextEntry(active)` from `src/tui/focus.tsx`, which counts it while `active` is true and uncounts it on blur or unmount; `GlobalKeyboard` reads `isTyping()` at keypress time. The focus stack cannot answer this: a `TextInput` never pushes its own scope, so `stack.length` stays 1 while you type into a screen's only field. + +Registered: `TextInput` (while not `isDisabled`, which also covers `SearchableList` and `FilePicker` search), `SqlInput` (while active), the `LogViewerOverlay` search box, and the `ResultTable` filter box. **A new free-text entry must call `useTextEntry`**, or capital `L`/`Q`/`D`/`F` and `?` typed into it fire their shortcuts. + +`?`, `D`, and `F` additionally require `stack.length <= 1`, which keeps them out of nested scopes such as dialogs. ## @inkjs/ui components @@ -375,7 +379,7 @@ There is no `k` on Home. Secrets belong to a config, so `k` opens them from the | `c` | config list | copy | | `c` | DB list | create | -**Global (every screen, via `GlobalKeyboard`):** +**Global (every screen, via `GlobalKeyboard`; all but `Ctrl+C` inactive while typing in a field):** | Key | Action | |-----|--------| diff --git a/docs/guide/troubleshooting.md b/docs/guide/troubleshooting.md index 1f7ea6ba..f3949bbb 100644 --- a/docs/guide/troubleshooting.md +++ b/docs/guide/troubleshooting.md @@ -114,6 +114,29 @@ skips every other prompt. Two commands: See [Non-interactive operation](./automation/non-interactive.md). +## Login fails with a password I know is right + +SQL Server, PostgreSQL, and MySQL each send one error for a wrong password and +an unknown account, so a client cannot learn which accounts exist. noorm says so +and lists what that error covers; the server's own log names the cause: + +| Server | Error | Also sent for | +|------------|----------------------------------|------------------------------------------------------------------------| +| SQL Server | `Login failed` (18456) | unknown login, login denied `CONNECT SQL`, Windows-only authentication | +| PostgreSQL | `password authentication failed` | unknown role, password past `VALID UNTIL` | +| MySQL | `Access denied` (1045) | unknown user, no account for this client host, `REQUIRE SSL` account | + +The message noorm shows is reworded for the user. What the server itself +sent is in the log (`.noorm/state/noorm.log`): the `connection:error` +entry carries `serverCode` (error number, SQLSTATE, or driver code) and +`serverMessage` (the server's text, including any wrapped socket error). + +A password that reaches noorm through a `NOORM_CONNECTION_PASSWORD` +environment variable arrives exactly as the shell exported it. The +compiled `noorm` binary does not read `.env` files, so export the +variable or set it in CI. + + ## Related - [CLI flag conventions](../cli/flags.md) diff --git a/docs/tui.md b/docs/tui.md index 1a2fdbba..f822c8b1 100644 --- a/docs/tui.md +++ b/docs/tui.md @@ -207,6 +207,10 @@ either way. | `Escape` | Go back / Cancel | | `Ctrl+C` | Quit | +While a text field is taking input (a form field in edit mode, a search box, +the SQL editor), `Shift+L`, `Shift+Q`, `?`, `D`, and `F` type their character +instead. They work again once you leave the field. + ### Cancelling a Database Operation diff --git a/docs/wiki/core-db.md b/docs/wiki/core-db.md index 00c873e0..afdd77bd 100644 --- a/docs/wiki/core-db.md +++ b/docs/wiki/core-db.md @@ -18,10 +18,11 @@ description: Database lifecycle, connection factory, schema exploration, teardow - [`src/core/db/policy.ts`](../../src/core/db/policy.ts) — `assertDbPolicy`, the shared destructive-lifecycle gate used by `core/db` and `core/teardown` (both reached directly by the TUI and indirectly by the CLI via the SDK). - [`src/core/db/dual.ts`](../../src/core/db/dual.ts) — `withDualConnection`, generic two-connection lifecycle (connect both, run fn, always cleanup both) used by `transfer` and vault-copy. - [`src/core/db/dialects/postgres.ts`](../../src/core/db/dialects/postgres.ts), `mysql.ts`, `mssql.ts`, `sqlite.ts` — per-dialect `databaseExists`/`createDatabase`/`dropDatabase`/`getSystemDatabase`. -- [`src/core/connection/factory.ts`](../../src/core/connection/factory.ts) — `createConnection` (retry/backoff via `@logosdx/utils` `retry`, `shouldRetry` skips auth/config failures), `testConnection` (`testServerOnly` swaps to the dialect's system database: `postgres`, `master`, none for mysql/sqlite). +- [`src/core/connection/factory.ts`](../../src/core/connection/factory.ts) — `createConnection` (retry/backoff via `@logosdx/utils` `retry`, `shouldRetry` skips auth/config failures), `testConnection` (`testServerOnly` tries the target first and falls back to the dialect's system database only when the target does not exist, because a contained MSSQL user cannot open `master`; the system databases are `postgres`, `master`, and none for mysql/sqlite). - [`src/core/connection/manager.ts`](../../src/core/connection/manager.ts) — `ConnectionManager` singleton (`getConnectionManager`); tracks cached (by config name) and ephemeral connections plus `WorkerBridge` instances, closes everything on the `app:shutdown` observer event. - [`src/core/connection/defaults.ts`](../../src/core/connection/defaults.ts) — `DEFAULT_PORTS` per dialect and the shared `PortSchema` (1-65535) used by `core/config` and `core/settings`. -- [`src/core/connection/dialects/mssql.ts`](../../src/core/connection/dialects/mssql.ts) — `resolveTlsServerName`/`buildTediousOptions`; connecting to MSSQL by IP address needs a synthetic SNI ServerName (`UNVERIFIED_TLS_SERVER_NAME`) because RFC 6066 forbids an IP literal as SNI, and `verifyDatabaseExists` probes `sys.databases` on `master` before opening the real pool to avoid a cryptic tedious/tarn ECONNRESET hang. +- [`src/core/connection/dialects/mssql.ts`](../../src/core/connection/dialects/mssql.ts) — `resolveTlsServerName`/`buildTediousOptions`; connecting to MSSQL by IP address needs a synthetic SNI ServerName (`UNVERIFIED_TLS_SERVER_NAME`) because RFC 6066 forbids an IP literal as SNI, and `createMssqlConnection` opens the first pooled connection itself because tedious keeps only the last login error; the dialect records every login error's number and message and hands them to `explainMssqlLoginFailure`. +- [`src/core/connection/errors.ts`](../../src/core/connection/errors.ts) — `explainConnectionError`, applied by `createConnection` to every failure, and `explainMssqlLoginFailure`. Per-dialect tables map a code (SQL Server error number, SQLSTATE, mysql2 code, SQLite code, or a Node network/TLS code found through `cause` and `AggregateError.errors`) to a user message; the codes where the server withholds the reason (18456, 28P01, 1045) list the usual causes. The result is a `DatabaseConnectionError` whose `serverCode`/`serverMessage` keep what the server said, which `createConnection` puts on the `connection:error` event and so into the log. Its messages never say "does not exist" unless a database is missing, because the TUI offers to create one on that phrase. - [`src/core/connection/dialects/mssql-limit-plugin.ts`](../../src/core/connection/dialects/mssql-limit-plugin.ts) — `MssqlLimitPlugin`, a Kysely `OperationNodeTransformer` that rewrites `LimitNode` → `TopNode` because Kysely 0.28.x's `MssqlQueryCompiler` doesn't override `visitLimit()`. - [`src/core/connection/dialects/sqlite.ts`](../../src/core/connection/dialects/sqlite.ts) / `sqlite-bun.ts` — `better-sqlite3` vs `bun:sqlite` adapters; `factory.ts` picks the Bun one when `globalThis.Bun` is defined. - [`src/core/connection/dialects/bun-sqlite.d.ts`](../../src/core/connection/dialects/bun-sqlite.d.ts) — hand-written minimal `bun:sqlite` type declarations, to avoid depending on full `bun-types`. diff --git a/scripts/build-binary.mjs b/scripts/build-binary.mjs index 1d3f2910..4846ffec 100644 --- a/scripts/build-binary.mjs +++ b/scripts/build-binary.mjs @@ -28,7 +28,11 @@ for (const { bun: target, suffix } of targets) { const outfile = `packages/cli/bin/noorm-${suffix}`; console.log(` Building ${outfile} (${target})...`); - await $`bun build --compile --target=${target} --minify src/cli/index.ts src/workers/connection.ts src/workers/compute.ts --outfile ${outfile} --define __CLI_VERSION__=\"${version}\"`.quiet(); + // A compiled binary loads `.env` from the cwd by default, and Bun's parser + // expands `$` even inside single quotes and cuts unquoted values at `#`. A + // project `.env` holding NOORM_CONNECTION_PASSWORD reached the driver as a + // different password, so the binary reads only the real environment. + await $`bun build --compile --no-compile-autoload-dotenv --target=${target} --minify src/cli/index.ts src/workers/connection.ts src/workers/compute.ts --outfile ${outfile} --define __CLI_VERSION__=\"${version}\"`.quiet(); console.log(` ✓ ${outfile}`); diff --git a/src/core/connection/dialects/mssql.ts b/src/core/connection/dialects/mssql.ts index 5e9d1cfe..1db436fe 100644 --- a/src/core/connection/dialects/mssql.ts +++ b/src/core/connection/dialects/mssql.ts @@ -4,17 +4,19 @@ * Uses 'tedious' and 'tarn' packages for MSSQL connections. * Install with: npm install tedious tarn * - * Verifies database existence via sys.databases before connecting - * to the target database, avoiding cryptic ECONNRESET errors when - * the database doesn't exist. + * Connects straight to the target database, so a login needs no access to + * `master` or to other databases' rows in `sys.databases`. */ import { isIP } from 'node:net'; +import { attempt } from '@logosdx/utils'; import { Kysely, MssqlDialect, sql } from 'kysely'; import type { ConnectionConfiguration } from 'tedious'; import type { ConnectionConfig, ConnectionResult } from '../types.js'; import { DEFAULT_PORTS, connectTimeoutFor } from '../defaults.js'; +import { explainMssqlLoginFailure } from '../errors.js'; +import type { MssqlServerError } from '../errors.js'; import { MssqlLimitPlugin } from './mssql-limit-plugin.js'; /** @@ -119,16 +121,13 @@ export function resolveTlsServerName(config: ConnectionConfig): string | undefin /** * Build tedious connection options from noorm config. * - * Centralizes the tedious config so both the preflight check - * and the real pool use the same settings. + * Kept separate from the pool so the TLS and timeout choices can be asserted + * without a server. * * @example - * const options = buildTediousOptions(config, 'master'); + * const options = buildTediousOptions(config); */ -export function buildTediousOptions( - config: ConnectionConfig, - database?: string, -): ConnectionConfiguration { +export function buildTediousOptions(config: ConnectionConfig): ConnectionConfiguration { return { server: config.host ?? 'localhost', @@ -141,7 +140,7 @@ export function buildTediousOptions( }, options: { port: config.port ?? DEFAULT_PORTS.mssql, - database: database ?? config.database, + database: config.database, trustServerCertificate: !config.ssl, encrypt: true, serverName: resolveTlsServerName(config), @@ -157,78 +156,33 @@ export function buildTediousOptions( } /** - * Instantiate a tedious Connection for the given noorm config. + * Instantiate a tedious Connection that records every error the server sends + * while logging in. tedious keeps only the last one, which for a missing + * database or a withheld reason is the generic 18456 "Login failed". */ -function buildTediousConfig( +function buildTediousConnection( Tedious: typeof import('tedious'), config: ConnectionConfig, - database?: string, + loginErrors: MssqlServerError[], ) { - return new Tedious.Connection(buildTediousOptions(config, database)); - -} - -/** - * Verify the target database exists by querying sys.databases on master. - * - * Connects to 'master' first and checks sys.databases. Throws a clear - * error if the database is missing, instead of letting tedious hang - * with a cryptic ECONNRESET. - */ -async function verifyDatabaseExists( - Tedious: typeof import('tedious'), - Tarn: typeof import('tarn'), - config: ConnectionConfig, -): Promise { - - const masterDb = new Kysely({ - dialect: new MssqlDialect({ - tarn: { - ...Tarn, - options: { - min: 0, - max: 1, - propagateCreateError: true, - }, - }, - tedious: { - ...Tedious, - connectionFactory: () => buildTediousConfig(Tedious, config, 'master'), - }, - }), - plugins: [new MssqlLimitPlugin()], - }); - - try { + const connection = new Tedious.Connection(buildTediousOptions(config)); + const record = (token: MssqlServerError) => loginErrors.push({ number: token.number, message: token.message }); - const { rows } = await sql<{ name: string }>` - SELECT name FROM sys.databases WHERE name = ${config.database} - `.execute(masterDb); + // Pooled connections live on, and their query errors are not login errors. + connection.on('errorMessage', record); + connection.once('connect', () => connection.removeListener('errorMessage', record)); - if (rows.length === 0) { - - throw new Error( - `Database '${config.database}' does not exist on ${config.host ?? 'localhost'}:${config.port ?? DEFAULT_PORTS.mssql}`, - ); - - } - - } - finally { - - await masterDb.destroy(); - - } + return connection; } /** * Create a SQL Server connection. * - * Verifies the target database exists via master before opening - * the connection pool. This avoids the tedious/tarn hang that - * occurs when MSSQL rejects login for a non-existent database. + * Runs a first query before returning, so a failed login is reported here with + * the error numbers the server sent. They name a missing database, a login + * without access to it, or the causes a plain 18456 can stand for. * * @example * ```typescript @@ -251,9 +205,7 @@ export async function createMssqlConnection(config: ConnectionConfig): Promise({ dialect: new MssqlDialect({ @@ -267,12 +219,28 @@ export async function createMssqlConnection(config: ConnectionConfig): Promise buildTediousConfig(Tedious, config), + connectionFactory: () => { + + loginErrors = []; + + return buildTediousConnection(Tedious, config, loginErrors); + + }, }, }), plugins: [new MssqlLimitPlugin()], }); + const [, connectErr] = await attempt(() => sql`SELECT 1`.execute(db)); + + if (connectErr) { + + await db.destroy(); + + throw explainMssqlLoginFailure(loginErrors, connectErr, config); + + } + return { db, dialect: 'mssql', diff --git a/src/core/connection/errors.ts b/src/core/connection/errors.ts new file mode 100644 index 00000000..0dbefcc9 --- /dev/null +++ b/src/core/connection/errors.ts @@ -0,0 +1,418 @@ +/** + * Connection failures, worded for the user, with the server's own words kept + * for the log. + * + * pg and mysql2 reject a refused port with an empty message, tedious reports + * any socket failure as "Could not connect (sequence)", and SQLite gives one + * message for a missing directory, an unreadable file, and an unwritable + * directory. The codes still carry the reason. + * + * Some servers withhold the reason on purpose: SQL Server, PostgreSQL, and + * MySQL each send one error for a wrong password and an unknown account, so + * that a client cannot probe which accounts exist. For those the message says + * so and lists the usual causes, rather than implying a wrong password. + * + * No message may say "does not exist" unless a database is missing: the TUI + * offers to create a database on that phrase, and `testConnection` falls back + * to the system database on it. + */ +import { accessSync, constants as fsConstants, existsSync } from 'node:fs'; +import { dirname, resolve } from 'node:path'; + +import { attemptSync } from '@logosdx/utils'; + +import type { ConnectionConfig } from './types.js'; +import { DEFAULT_PORTS, connectTimeoutFor } from './defaults.js'; + +/** + * A connection failure worded for the user. + * + * `serverCode` and `serverMessage` keep what the server or driver actually + * said, so the log can show it next to the reworded message. + * + * @example + * if (err instanceof DatabaseConnectionError) log(err.serverCode, err.serverMessage); + */ +export class DatabaseConnectionError extends Error { + + override readonly name = 'DatabaseConnectionError' as const; + + constructor( + message: string, + public readonly serverCode: string | undefined, + public readonly serverMessage: string, + options?: ErrorOptions, + ) { + + super(message, options); + + } + +} + +/** + * One error message a SQL Server sent during login. + */ +export interface MssqlServerError { + number: number; + message: string; +} + +/** + * Inputs to an `Explain`. + */ +interface Failure { + user: string; + database: string; + host: string; + where: string; + config: ConnectionConfig; + + /** The driver's own message, for codes that cover several cases. */ + message: string; +} + +type Explain = (failure: Failure) => string | undefined; + +const noPassword: Explain = ({ user }) => + `No password was supplied for user '${user}', and the server requires one.`; + +const timedOut: Explain = ({ where, config }) => + `${where} did not answer within ${connectTimeoutFor(config)}ms. ` + + 'Check the host and port, and that no firewall is dropping the connection.'; + +const SERVER_FULL = 'The server has no free connections (max_connections reached). Try again later.'; + +/** + * Failures before the server had a say. + */ +const TRANSPORT_REASONS: [codes: string[], explain: Explain][] = [ + [['ECONNREFUSED'], ({ where }) => + `Connection refused at ${where}: nothing is listening there. ` + + 'Check the host and port, and that the server is running.'], + [['ENOTFOUND'], ({ host }) => `Host '${host}' could not be found. Check the host name.`], + [['EAI_AGAIN'], ({ host }) => `Host '${host}' could not be looked up: DNS did not answer. Check the network connection.`], + [['ETIMEDOUT', 'ETIMEOUT'], timedOut], + [['EHOSTUNREACH', 'ENETUNREACH'], ({ host }) => `${host} is unreachable from this machine: there is no network route to it.`], + [['ECONNRESET'], ({ where }) => + `${where} closed the connection during the handshake. ` + + 'Check that the port belongs to this database server and that the ssl setting matches what it expects.'], +]; + +const TLS_CERTIFICATE_CODES = new Set([ + 'DEPTH_ZERO_SELF_SIGNED_CERT', + 'SELF_SIGNED_CERT_IN_CHAIN', + 'UNABLE_TO_VERIFY_LEAF_SIGNATURE', + 'UNABLE_TO_GET_ISSUER_CERT_LOCALLY', + 'CERT_HAS_EXPIRED', + 'CERT_NOT_YET_VALID', + 'ERR_TLS_CERT_ALTNAME_INVALID', +]); + +/** + * SQL Server login errors by number, most specific first: 4060 arrives + * together with 18456, and 4060 is the reason. + */ +const MSSQL_REASONS: [number: number, explain: Explain][] = [ + [4060, ({ database, where }) => + `Cannot open database '${database}' on ${where}: it does not exist or this login has no access to it.`], + [40615, () => + 'The server\'s firewall does not allow this machine\'s IP address. Add it to the server\'s firewall rules.'], + [18470, ({ user }) => `Login '${user}' is disabled. An administrator can enable it with ALTER LOGIN ... ENABLE.`], + [18486, ({ user }) => `Login '${user}' is locked out after too many failed attempts.`], + [18487, ({ user }) => + `The password for login '${user}' has expired. Change it with a SQL Server client, then update this connection.`], + [18488, ({ user }) => + `The password for login '${user}' must be changed before it can log in. ` + + 'Change it with a SQL Server client, then update this connection.'], + [18456, ({ user }) => + `Login failed for user '${user}'. SQL Server does not tell clients why; the usual causes are a wrong password, ` + + 'an unknown login, a login denied CONNECT SQL, or a server that only accepts Windows authentication. ' + + 'The server\'s error log names the cause.'], +]; + +/** + * PostgreSQL errors by SQLSTATE. + */ +const POSTGRES_REASONS: Record = { + '28P01': ({ user }) => + `Password authentication failed for user '${user}'. PostgreSQL does not tell clients why; the usual causes ` + + 'are a wrong password, an unknown role, or an expired password (VALID UNTIL). The server log names the cause.', + '28000': ({ user, database, message }) => { + + if (message.includes('is not permitted to log in')) return `Role '${user}' is not allowed to log in (NOLOGIN).`; + + if (message.includes('pg_hba.conf')) { + + return `The server's pg_hba.conf has no rule that lets user '${user}' connect to '${database}' from this machine.`; + + } + + return undefined; + + }, + '3D000': ({ database, where }) => `Database '${database}' does not exist on ${where}.`, + '42501': ({ user, database, message }) => message.startsWith('permission denied for database') + ? `Role '${user}' may not connect to database '${database}': it lacks the CONNECT privilege.` + : undefined, + '53300': ({ user, message }) => message.includes('for role') + ? `Role '${user}' has used up its connection limit (CONNECTION LIMIT).` + : SERVER_FULL, + '57P03': () => 'The server is starting up or shutting down. Try again in a moment.', +}; + +/** + * PostgreSQL failures pg raises itself, with no SQLSTATE. + */ +const POSTGRES_CLIENT_REASONS: [text: string, explain: Explain][] = [ + // pg raises this itself when the server asks for a SCRAM password and the + // config has none. + ['client password must be a string', noPassword], + ['The server does not support SSL connections', () => + 'The server does not accept TLS connections. Turn ssl off for this connection.'], + ['Connection terminated due to connection timeout', timedOut], +]; + +/** + * MySQL errors by mysql2 code. + */ +const MYSQL_REASONS: Record = { + ER_ACCESS_DENIED_ERROR: (failure) => failure.message.includes('(using password: NO)') + ? noPassword(failure) + : `Access denied for user '${failure.user}'. MySQL does not tell clients why; the usual causes are a wrong password, ` + + 'an unknown user, no account for this machine\'s host, or an account that requires TLS (REQUIRE SSL).', + ER_DBACCESS_DENIED_ERROR: ({ user, database }) => `User '${user}' has no privileges on database '${database}'.`, + ER_BAD_DB_ERROR: ({ database, where }) => `Database '${database}' does not exist on ${where}.`, + ER_HOST_NOT_PRIVILEGED: () => 'The server accepts no account from this machine\'s host.', + ER_ACCOUNT_HAS_BEEN_LOCKED: ({ user }) => + `Account '${user}' is locked. An administrator can unlock it with ALTER USER ... ACCOUNT UNLOCK.`, + ER_MUST_CHANGE_PASSWORD_LOGIN: ({ user }) => + `The password for '${user}' has expired. Change it with a MySQL client, then update this connection.`, + ER_CON_COUNT_ERROR: () => SERVER_FULL, + ER_USER_LIMIT_REACHED: ({ user }) => `User '${user}' has used up its connection limit (MAX_USER_CONNECTIONS).`, + ER_HOST_IS_BLOCKED: () => + 'The server blocked this machine after too many connection errors. An administrator can clear its host cache.', + ER_SECURE_TRANSPORT_REQUIRED: () => 'The server only accepts TLS connections. Turn ssl on for this connection.', +}; + +/** + * Word a failed connection for the user and keep what the server said. + * + * An error with no code and nothing to explain comes back unchanged, so + * noorm's own errors (an abort, a TLS misconfiguration) keep their type. + * + * @example + * const [conn, err] = await attempt(() => openPool(config)); + * if (err) throw explainConnectionError(err, config); + */ +export function explainConnectionError(err: Error, config: ConnectionConfig): Error { + + if (err instanceof DatabaseConnectionError) return err; + + const codes = errorCodes(err); + const failure = describeFailure(err, config); + const reason = explainTransport(codes, failure) ?? explainServer(codes, failure); + const serverMessage = serverMessages(err).join(' | '); + + if (!reason && codes.length === 0) return err; + + return new DatabaseConnectionError( + reason ?? (err.message || serverMessage), + codes.join(', ') || undefined, + serverMessage, + { cause: err }, + ); + +} + +/** + * Word a failed SQL Server login from the errors the MSSQL dialect recorded + * during login. + * + * @example + * throw explainMssqlLoginFailure(recordedErrors, connectErr, config); + */ +export function explainMssqlLoginFailure( + serverErrors: readonly MssqlServerError[], + err: Error, + config: ConnectionConfig, +): Error { + + if (serverErrors.length === 0) return err; + + const failure = describeFailure(err, config); + const match = MSSQL_REASONS.find(([number]) => serverErrors.some((e) => e.number === number)); + + return new DatabaseConnectionError( + match?.[1](failure) ?? err.message, + serverErrors.map((e) => e.number).join(', '), + serverErrors.map((e) => e.message).join(' | '), + { cause: err }, + ); + +} + +function describeFailure(err: Error, config: ConnectionConfig): Failure { + + const host = config.host ?? 'localhost'; + + return { + user: config.user ?? '', + database: config.database, + host, + where: `${host}:${config.port ?? DEFAULT_PORTS[config.dialect]}`, + config, + message: err.message, + }; + +} + +/** + * Walk the error and what it wraps: tedious puts the socket error in `cause`, + * and a dual-stack refusal arrives as an AggregateError holding one error per + * address tried. + */ +function walk(err: unknown, visit: (error: object) => void, seen = new Set()): void { + + if (typeof err !== 'object' || err === null || seen.has(err)) return; + + const inner: unknown = Reflect.get(err, 'errors'); + + seen.add(err); + visit(err); + walk(Reflect.get(err, 'cause'), visit, seen); + + if (Array.isArray(inner)) { + + for (const each of inner) walk(each, visit, seen); + + } + +} + +function errorCodes(err: unknown): string[] { + + const codes = new Set(); + + walk(err, (error) => { + + const code: unknown = Reflect.get(error, 'code'); + + if (typeof code === 'string') codes.add(code); + + }); + + return [...codes]; + +} + +function serverMessages(err: unknown): string[] { + + const messages = new Set(); + + walk(err, (error) => { + + const message: unknown = Reflect.get(error, 'message'); + + if (typeof message === 'string' && message) messages.add(message); + + }); + + return [...messages]; + +} + +function explainTransport(codes: string[], failure: Failure): string | undefined { + + const transport = TRANSPORT_REASONS.find(([matches]) => matches.some((code) => codes.includes(code))); + + if (transport) return transport[1](failure); + + const certificateCode = codes.find((code) => TLS_CERTIFICATE_CODES.has(code)); + + if (!certificateCode) return undefined; + + // MSSQL encrypts either way; `ssl` only decides whether the certificate + // is validated. For the others it turns TLS on or off. + const withoutValidation = failure.config.dialect === 'mssql' + ? 'turn ssl off to keep encryption without validating the certificate' + : 'turn ssl off, which also turns off encryption'; + + return `The server's TLS certificate was rejected (${certificateCode}). ` + + `Trust the authority that issued it on this machine, or ${withoutValidation}.`; + +} + +function explainServer(codes: string[], failure: Failure): string | undefined { + + const code = codes[0] ?? ''; + + switch (failure.config.dialect) { + + case 'postgres': { + + const client = POSTGRES_CLIENT_REASONS.find(([text]) => failure.message.includes(text)); + + return POSTGRES_REASONS[code]?.(failure) ?? client?.[1](failure); + + } + + case 'mysql': + return MYSQL_REASONS[code]?.(failure); + + case 'sqlite': + return explainSqlite(code, failure); + + default: + return undefined; + + } + +} + +/** + * SQLite reports every unopenable file as SQLITE_CANTOPEN; the filesystem + * says which of the three it is. + */ +function explainSqlite(code: string, failure: Failure): string | undefined { + + const file = resolve(failure.config.filename ?? failure.database); + const dir = dirname(file); + + if (code === 'SQLITE_NOTADB') { + + return `'${file}' is not a SQLite database, or it is encrypted.`; + + } + + if (code !== 'SQLITE_CANTOPEN') return undefined; + + if (!existsSync(dir)) { + + return `Cannot open SQLite database '${file}': directory '${dir}' is missing.`; + + } + + if (existsSync(file) && !canAccess(file, fsConstants.R_OK | fsConstants.W_OK)) { + + return `Cannot open SQLite database '${file}': this process lacks read or write permission on the file.`; + + } + + if (!existsSync(file) && !canAccess(dir, fsConstants.W_OK)) { + + return `Cannot create SQLite database '${file}': this process lacks write permission on '${dir}'.`; + + } + + return undefined; + +} + +function canAccess(path: string, mode: number): boolean { + + const [, err] = attemptSync(() => accessSync(path, mode)); + + return !err; + +} diff --git a/src/core/connection/factory.ts b/src/core/connection/factory.ts index b5086a96..84c998b0 100644 --- a/src/core/connection/factory.ts +++ b/src/core/connection/factory.ts @@ -14,6 +14,7 @@ import { observer } from '../observer.js'; import { OperationAbortedError, raceAbort, throwIfAborted } from '../shared/abort.js'; import { getConnectionManager } from './manager.js'; import { connectTimeoutFor } from './defaults.js'; +import { DatabaseConnectionError, explainConnectionError } from './errors.js'; type DialectFactory = (config: ConnectionConfig) => ConnectionResult | Promise; @@ -228,6 +229,9 @@ async function openConnection( backoff, jitterFactor: 0.1, signal, + // Otherwise running out of retries reads "Max retries + // reached", and the server's own error is lost. + throwLastError: true, shouldRetry: (err) => { // Retrying something the caller walked away from would @@ -261,8 +265,13 @@ async function openConnection( if (err) { - observer.emit('connection:error', { configName, error: err.message }); - throw err; + const explained = explainConnectionError(err, config); + const server = explained instanceof DatabaseConnectionError + ? { serverCode: explained.serverCode, serverMessage: explained.serverMessage } + : {}; + + observer.emit('connection:error', { configName, error: explained.message, ...server }); + throw explained; } @@ -353,6 +362,40 @@ const SYSTEM_DATABASES: Record = { mssql: 'master', }; +/** + * Outcome of a connection test. `aborted` separates a caller who stopped + * waiting from a database that failed. + */ +interface ConnectionTestResult { + ok: boolean; + error?: string; + aborted?: boolean; +} + +async function probeConnection(config: ConnectionConfig, signal?: AbortSignal): Promise { + + const [conn, err] = await attempt(() => + createConnection(config, '__test__', {}, signal), + ); + + if (err) { + + if (err instanceof OperationAbortedError) { + + return { ok: false, error: err.message, aborted: true }; + + } + + return { ok: false, error: err.message }; + + } + + await conn!.destroy(); + + return { ok: true }; + +} + /** * Test a connection config without keeping the connection open. * @@ -360,8 +403,8 @@ const SYSTEM_DATABASES: Record = { * * @param config - Connection configuration to test * @param options - Test options - * @param options.testServerOnly - If true, connects to system database instead of target. - * Useful when the target database doesn't exist yet. + * @param options.testServerOnly - If true, a target database that does not exist yet + * is not a failure: the system database stands in for it. * @param options.signal - Abort to stop waiting. The result comes back with * `aborted: true` so a caller can say so honestly * instead of reporting a database error. @@ -382,21 +425,9 @@ const SYSTEM_DATABASES: Record = { export async function testConnection( config: ConnectionConfig, options: { testServerOnly?: boolean; signal?: AbortSignal } = {}, -): Promise<{ ok: boolean; error?: string; aborted?: boolean }> { - - let testConfig = config; +): Promise { - // If testing server only, swap to system database - if (options.testServerOnly && config.dialect !== 'sqlite') { - - const systemDb = SYSTEM_DATABASES[config.dialect]; - - testConfig = { - ...config, - database: systemDb ?? config.database, - }; - - } + const systemDb = SYSTEM_DATABASES[config.dialect]; // SQLite has no system database to swap to, so the probe would open the // target — and the driver creates the file. Probe the directory that @@ -423,24 +454,18 @@ export async function testConnection( } - const [conn, err] = await attempt(() => - createConnection(testConfig, '__test__', {}, options.signal), - ); - - if (err) { + const result = await probeConnection(config, options.signal); - if (err instanceof OperationAbortedError) { + // The target goes first because a login scoped to its own database (an + // Azure SQL contained user) cannot open the system database at all. + const targetMissing = !result.ok && !result.aborted && !!result.error?.includes('does not exist'); - return { ok: false, error: err.message, aborted: true }; + if (options.testServerOnly && systemDb && targetMissing) { - } - - return { ok: false, error: err.message }; + return probeConnection({ ...config, database: systemDb }, options.signal); } - await conn!.destroy(); - - return { ok: true }; + return result; } diff --git a/src/core/connection/index.ts b/src/core/connection/index.ts index 29d47352..0fec8041 100644 --- a/src/core/connection/index.ts +++ b/src/core/connection/index.ts @@ -4,6 +4,7 @@ * Provides database connection creation and management. */ export { createConnection, testConnection, discardConnection } from './factory.js'; +export { DatabaseConnectionError } from './errors.js'; export type { ConnectionRetryOptions } from './factory.js'; export { getConnectionManager, resetConnectionManager } from './manager.js'; export { DEFAULT_PORTS, PortSchema, DEFAULT_CONNECT_TIMEOUT_MS, connectTimeoutFor } from './defaults.js'; diff --git a/src/core/observer.ts b/src/core/observer.ts index a6bda480..0e56f0cb 100644 --- a/src/core/observer.ts +++ b/src/core/observer.ts @@ -172,7 +172,14 @@ export interface NoormEvents extends SettingsEvents, UpdateEvents, VaultEvents, database?: string; }; 'connection:close': { configName: string }; - 'connection:error': { configName: string; error: string }; + 'connection:error': { + configName: string; + /** Worded for the user. */ + error: string; + /** What the server or driver reported, for the log: its code(s) and message(s). */ + serverCode?: string; + serverMessage?: string; + }; // App lifecycle 'app:starting': { mode: AppMode }; diff --git a/src/tui/app-context.tsx b/src/tui/app-context.tsx index 0a88b023..afee742e 100644 --- a/src/tui/app-context.tsx +++ b/src/tui/app-context.tsx @@ -232,10 +232,6 @@ export interface AppContextValue { setExploreFilter: (category: string, filter: ExploreFilterEntry) => void; clearExploreFilters: () => void; - // Global key toggles (for disabling keys in text input contexts) - helpKeyEnabled: boolean; - setHelpKeyEnabled: (enabled: boolean) => void; - // Actions refresh: () => Promise; setActiveConfig: (name: string) => Promise; @@ -312,9 +308,6 @@ export function AppContextProvider({ // Explore filter state const [exploreFilters, setExploreFilters] = useState({}); - // Global key toggles - const [helpKeyEnabled, setHelpKeyEnabled] = useState(true); - /** * Toggle dry-run mode. */ @@ -700,8 +693,6 @@ export function AppContextProvider({ exploreFilters, setExploreFilter, clearExploreFilters, - helpKeyEnabled, - setHelpKeyEnabled, refresh, setActiveConfig: handleSetActiveConfig, }), @@ -726,7 +717,6 @@ export function AppContextProvider({ exploreFilters, setExploreFilter, clearExploreFilters, - helpKeyEnabled, refresh, handleSetActiveConfig, ], diff --git a/src/tui/components/forms/TextInput.tsx b/src/tui/components/forms/TextInput.tsx index 6a48c9e9..500934ee 100644 --- a/src/tui/components/forms/TextInput.tsx +++ b/src/tui/components/forms/TextInput.tsx @@ -1,8 +1,9 @@ /** * Single-line text input. * - * A copy of `@inkjs/ui`'s `TextInput` (MIT) with exactly one behavioural - * change: a mouse report is dropped instead of typed into the field. + * A copy of `@inkjs/ui`'s `TextInput` (MIT) with one behavioural change, a + * mouse report is dropped instead of typed into the field, plus a + * `useTextEntry` call so `GlobalKeyboard` leaves its keystrokes alone. * * **Why a copy and not a wrapper.** Upstream's handler ends in an * unconditional `state.insert(input)`, and Ink's `useInput` is subscriber-based @@ -38,6 +39,7 @@ import { Text, useInput } from 'ink'; import type { ReactElement, ReactNode } from 'react'; import { isMouseReport } from '../../mouse.js'; +import { useTextEntry } from '../../focus.js'; interface TextInputState { @@ -150,6 +152,8 @@ export function TextInput({ cursorOffset: defaultValue.length, }); + useTextEntry(!isDisabled); + const suggestion = useMemo(() => { if (state.value.length === 0) return undefined; diff --git a/src/tui/components/overlays/LogViewerOverlay.tsx b/src/tui/components/overlays/LogViewerOverlay.tsx index d323ebba..23deb510 100644 --- a/src/tui/components/overlays/LogViewerOverlay.tsx +++ b/src/tui/components/overlays/LogViewerOverlay.tsx @@ -20,7 +20,7 @@ import { Box, Text, useInput } from 'ink'; import type { ReactElement } from 'react'; -import { useFocusScope } from '../../focus.js'; +import { useFocusScope, useTextEntry } from '../../focus.js'; import { useAppContext } from '../../app-context.js'; import { readLogFile } from '../../../core/logger/reader.js'; import { Spinner } from '../feedback/index.js'; @@ -100,6 +100,8 @@ export function LogViewerOverlay({ onClose }: LogViewerOverlayProps): ReactEleme const [showDetail, setShowDetail] = useState(false); const [paused, setPaused] = useState(false); + useTextEntry(searchMode); + // Refs const searchTermRef = useRef(searchTerm); searchTermRef.current = searchTerm; diff --git a/src/tui/components/terminal/ResultTable.tsx b/src/tui/components/terminal/ResultTable.tsx index b98d247a..0032ceaf 100644 --- a/src/tui/components/terminal/ResultTable.tsx +++ b/src/tui/components/terminal/ResultTable.tsx @@ -38,6 +38,7 @@ import v from 'voca'; import type { ReactElement } from 'react'; import { isMouseReport, useRowMouse } from '../../mouse.js'; +import { useTextEntry } from '../../focus.js'; import { fitGridColumns } from './columnFit.js'; import { documentValue } from './rowDocument.js'; @@ -386,6 +387,8 @@ export function ResultTable({ const [internalRow, setInternalRow] = useState(0); const [sortColumnIndex, setSortColumnIndex] = useState(0); + useTextEntry(mode === 'filter'); + const highlightedRow = isControlled ? controlledRow : internalRow; // One mover for both cursors, so a controlled parent and an uncontrolled diff --git a/src/tui/components/terminal/SqlInput.tsx b/src/tui/components/terminal/SqlInput.tsx index d71008dc..60d50125 100644 --- a/src/tui/components/terminal/SqlInput.tsx +++ b/src/tui/components/terminal/SqlInput.tsx @@ -12,6 +12,7 @@ import { Box, Text, useInput } from 'ink'; import type { ReactElement } from 'react'; import { isMouseReport } from '../../mouse.js'; +import { useTextEntry } from '../../focus.js'; /** * Props for SqlInput component. @@ -53,6 +54,8 @@ export function SqlInput({ const [editMode, setEditMode] = useState(false); const [cursor, setCursor] = useState(value.length); + useTextEntry(isActive); + // Track what we last set to detect external changes const lastValueRef = useRef(value); diff --git a/src/tui/focus.tsx b/src/tui/focus.tsx index fa5762aa..82d333fc 100644 --- a/src/tui/focus.tsx +++ b/src/tui/focus.tsx @@ -23,7 +23,7 @@ * }) * ``` */ -import { createContext, useContext, useState, useCallback, useMemo, useId, useEffect } from 'react'; +import { createContext, useContext, useState, useCallback, useMemo, useId, useEffect, useRef } from 'react'; import type { ReactNode, ReactElement } from 'react'; @@ -56,6 +56,21 @@ export interface FocusProviderProps { export function FocusProvider({ children }: FocusProviderProps): ReactElement { const [stack, setStack] = useState([]); + const activeTextEntries = useRef(0); + + const isTyping = useCallback(() => activeTextEntries.current > 0, []); + + const beginTextEntry = useCallback(() => { + + activeTextEntries.current += 1; + + return () => { + + activeTextEntries.current -= 1; + + }; + + }, []); const push = useCallback((id: string, label?: string) => { @@ -128,8 +143,10 @@ export function FocusProvider({ children }: FocusProviderProps): ReactElement { isActive, activeId, stack, + isTyping, + beginTextEntry, }), - [push, pop, isActive, activeId, stack], + [push, pop, isActive, activeId, stack, isTyping, beginTextEntry], ); return {children}; @@ -225,6 +242,33 @@ export function useFocusScope(labelOrOptions?: string | UseFocusScopeOptions): { } +/** + * Mark a text field as taking keystrokes while `active`. + * + * Ink hands every keystroke to every handler, so without this a capital `L` + * typed into a field also opens the log viewer. `GlobalKeyboard` reads it and + * leaves its single-key shortcuts alone while any field counts. + * + * Does nothing outside a FocusProvider, so a field rendered on its own (as in + * its unit tests) still works. + * + * @example + * useTextEntry(!isDisabled); + */ +export function useTextEntry(active: boolean): void { + + const beginTextEntry = useContext(FocusContext)?.beginTextEntry; + + useEffect(() => { + + if (!active || !beginTextEntry) return; + + return beginTextEntry(); + + }, [active, beginTextEntry]); + +} + /** * Hook to check if a specific focus ID is active. * diff --git a/src/tui/keyboard.tsx b/src/tui/keyboard.tsx index 4174cc0c..d2474df0 100644 --- a/src/tui/keyboard.tsx +++ b/src/tui/keyboard.tsx @@ -22,7 +22,6 @@ import type { ReactNode, ReactElement } from 'react'; import { useFocusContext } from './focus.js'; import { useShutdown } from './shutdown.js'; -import { useAppContext } from './app-context.js'; /** * Props for GlobalKeyboard component. @@ -79,11 +78,14 @@ export interface GlobalKeyboardProps { * * Wraps the app and handles: * - Ctrl+C: Exit application - * - ?: Show help overlay (when not in text input) - * - D: Toggle dry-run mode (when not in text input) - * - F: Toggle force mode (when not in text input) + * - Shift+L / Shift+Q: Log viewer / SQL terminal (when not typing) + * - ?: Show help overlay (when not typing) + * - D: Toggle dry-run mode (when not typing) + * - F: Toggle force mode (when not typing) * - Esc: Navigate back (when nothing else handles it) * + * "Typing" means a text field has registered with `useTextEntry`. + * * Individual screens/components register their own handlers * via useInput with focus-aware filtering. */ @@ -108,11 +110,13 @@ export function GlobalKeyboard({ }: GlobalKeyboardProps): ReactElement { const { gracefulExit } = useShutdown(); - const { stack } = useFocusContext(); - const { helpKeyEnabled } = useAppContext(); + const { stack, isTyping } = useFocusContext(); useInput((input, key) => { + // Every shortcut but Ctrl+C is a printable key a text field may want. + const typing = isTyping(); + // Ctrl+C always exits gracefully if (key.ctrl && input === 'c') { @@ -122,8 +126,7 @@ export function GlobalKeyboard({ } - // Shift+L toggles log viewer (works from anywhere, even in text input) - if (key.shift && input === 'L') { + if (!typing && key.shift && input === 'L') { onToggleLogViewer?.(); @@ -131,8 +134,7 @@ export function GlobalKeyboard({ } - // Shift+Q opens SQL terminal (works from anywhere) - if (key.shift && input === 'Q') { + if (!typing && key.shift && input === 'Q') { onOpenSqlTerminal?.(); @@ -140,12 +142,11 @@ export function GlobalKeyboard({ } - // Global keys only work when not typing in a text input - // (focus stack > 1 means we're likely in an input component) - if (stack.length <= 1) { + // Help and the mode toggles also stay out of nested scopes such as + // dialogs, where the screen underneath is not what they act on. + if (!typing && stack.length <= 1) { - // ? shows help (can be disabled by components with text input) - if (input === '?' && helpKeyEnabled) { + if (input === '?') { // Toggle help on every press (help shows on odd presses, hides on even) onHelp?.(); diff --git a/src/tui/screens/db/SqlTerminalScreen.tsx b/src/tui/screens/db/SqlTerminalScreen.tsx index b05821dd..aaa23aed 100644 --- a/src/tui/screens/db/SqlTerminalScreen.tsx +++ b/src/tui/screens/db/SqlTerminalScreen.tsx @@ -50,7 +50,7 @@ export function SqlTerminalScreen({ params }: ScreenProps): ReactElement { const { navigate, back } = useRouter(); const { isFocused } = useFocusScope('SqlTerminal'); - const { activeConfig, activeConfigName, projectRoot, setHelpKeyEnabled } = useAppContext(); + const { activeConfig, activeConfigName, projectRoot } = useAppContext(); const { showToast } = useToast(); // useWindowSize, not useStdout: stdout.rows mutates on resize without asking // React for anything, so a memo keyed on it never recomputes. @@ -109,21 +109,6 @@ export function SqlTerminalScreen({ params }: ScreenProps): ReactElement { }, [params.name]); - // Disable help key when input has content (to allow typing '?') - useEffect(() => { - - const hasContent = query.trim() !== ''; - setHelpKeyEnabled(!hasContent); - - // Re-enable on unmount - return () => { - - setHelpKeyEnabled(true); - - }; - - }, [query, setHelpKeyEnabled]); - // Initialize connection and history useEffect(() => { diff --git a/src/tui/types.ts b/src/tui/types.ts index db0d3768..1b470ff5 100644 --- a/src/tui/types.ts +++ b/src/tui/types.ts @@ -341,6 +341,18 @@ export interface FocusContextValue { * Debug: get the full focus stack. */ stack: FocusEntry[]; + + /** + * Whether a text field is taking keystrokes right now. A function, read at + * keypress time, so a field gaining or losing focus re-renders nothing. + */ + isTyping: () => boolean; + + /** + * Count a text field as taking keystrokes until the returned function is + * called. Use `useTextEntry` rather than calling this directly. + */ + beginTextEntry: () => () => void; } /** diff --git a/tests/cli/keyboard.test.tsx b/tests/cli/keyboard.test.tsx index 7035bd21..f5aeb536 100644 --- a/tests/cli/keyboard.test.tsx +++ b/tests/cli/keyboard.test.tsx @@ -11,7 +11,9 @@ import { Text } from 'ink'; import { RouterProvider } from '../../src/tui/router.js'; import { FocusProvider, useFocusScope } from '../../src/tui/focus.js'; import { ShutdownProvider } from '../../src/tui/shutdown.js'; -import { useFocusedInput, useListKeys, useQuitHandler } from '../../src/tui/keyboard.js'; +import { GlobalKeyboard, useFocusedInput, useListKeys, useQuitHandler } from '../../src/tui/keyboard.js'; +import { TextInput } from '../../src/tui/components/forms/TextInput.js'; +import { SqlInput } from '../../src/tui/components/terminal/SqlInput.js'; import { resetLifecycleManager } from '../../src/core/lifecycle/manager.js'; // ANSI escape sequences for arrow keys @@ -570,4 +572,156 @@ describe('cli: keyboard', () => { }); + // Ink hands every keystroke to every handler, so GlobalKeyboard has to + // stand down on its own. + describe('GlobalKeyboard while a text field is focused', () => { + + const SHORTCUT_KEYS = ['L', 'Q', '?', 'D', 'F']; + + function shortcutHandlers() { + + return { + onToggleLogViewer: vi.fn(), + onOpenSqlTerminal: vi.fn(), + onHelp: vi.fn(), + onToggleDryRun: vi.fn(), + onToggleForce: vi.fn(), + }; + + } + + async function waitFor(predicate: () => boolean, timeoutMs = 2000): Promise { + + const deadline = Date.now() + timeoutMs; + + while (!predicate() && Date.now() < deadline) { + + await new Promise((resolve) => setTimeout(resolve, 10)); + + } + + } + + async function press(stdin: { write: (data: string) => void }, keys: string[]): Promise { + + for (const key of keys) { + + stdin.write(key); + await new Promise((resolve) => setTimeout(resolve, 20)); + + } + + } + + /** A screen with one text field, the way ChangeAddScreen has one. */ + function FieldScreen({ fieldFocused, onChange }: { fieldFocused: boolean; onChange: (value: string) => void }) { + + useFocusScope('FieldScreen'); + + return ; + + } + + function SqlScreen({ onChange }: { onChange: (value: string) => void }) { + + const [query, setQuery] = useState(''); + + useFocusScope('SqlScreen'); + + const change = useCallback((next: string) => { + + setQuery(next); + onChange(next); + + }, [onChange]); + + return {}} onHistoryNavigate={() => {}} />; + + } + + it('should type L, Q, ?, D and F into the field instead of running their shortcuts', async () => { + + const handlers = shortcutHandlers(); + let value = ''; + + const { stdin, unmount } = render( + + + { + + value = next; + + }} /> + + , + ); + + await new Promise((resolve) => setTimeout(resolve, 30)); + await press(stdin, SHORTCUT_KEYS); + await waitFor(() => value === 'LQ?DF'); + + expect(value).toBe('LQ?DF'); + + for (const handler of Object.values(handlers)) expect(handler).not.toHaveBeenCalled(); + + unmount(); + + }); + + it('should run the shortcuts again once the field loses focus', async () => { + + const handlers = shortcutHandlers(); + const tree = (fieldFocused: boolean) => ( + + + {}} /> + + + ); + + const { stdin, rerender, unmount } = render(tree(true)); + + await new Promise((resolve) => setTimeout(resolve, 30)); + rerender(tree(false)); + await new Promise((resolve) => setTimeout(resolve, 30)); + await press(stdin, SHORTCUT_KEYS); + await waitFor(() => handlers.onToggleForce.mock.calls.length > 0); + + for (const handler of Object.values(handlers)) expect(handler).toHaveBeenCalledTimes(1); + + unmount(); + + }); + + it('should leave the SQL editor\'s keystrokes alone too', async () => { + + const handlers = shortcutHandlers(); + let value = ''; + + const { stdin, unmount } = render( + + + { + + value = next; + + }} /> + + , + ); + + await new Promise((resolve) => setTimeout(resolve, 30)); + await press(stdin, ['L', 'Q']); + await waitFor(() => value === 'LQ'); + + expect(value).toBe('LQ'); + expect(handlers.onToggleLogViewer).not.toHaveBeenCalled(); + expect(handlers.onOpenSqlTerminal).not.toHaveBeenCalled(); + + unmount(); + + }); + + }); + }); diff --git a/tests/core/connection/dialects/mssql.test.ts b/tests/core/connection/dialects/mssql.test.ts index d045e7dc..56d66a51 100644 --- a/tests/core/connection/dialects/mssql.test.ts +++ b/tests/core/connection/dialects/mssql.test.ts @@ -165,15 +165,6 @@ describe('connection/dialects/mssql: buildTediousOptions', () => { }); - it('should carry the database override used by the master preflight', () => { - - const options = buildTediousOptions(mssqlConfig({ host: '10.0.0.5' }), 'master'); - - expect(options.options?.database).toBe('master'); - expect(options.options?.serverName).toBe(UNVERIFIED_TLS_SERVER_NAME); - - }); - it('should default the host to localhost', () => { const options = buildTediousOptions(mssqlConfig({ host: undefined })); diff --git a/tests/core/connection/errors.test.ts b/tests/core/connection/errors.test.ts new file mode 100644 index 00000000..892ff73d --- /dev/null +++ b/tests/core/connection/errors.test.ts @@ -0,0 +1,113 @@ +/** + * The integration suite produces every failure it can for real. These cover + * transport failures a container cannot produce on demand, and two contracts: + * an error with nothing to add comes back untouched, and a reworded one still + * carries the driver's error. + */ +import { describe, it, expect } from 'bun:test'; + +import { DatabaseConnectionError, explainConnectionError } from '../../../src/core/connection/errors.js'; +import type { ConnectionConfig } from '../../../src/core/connection/types.js'; + + +const config: ConnectionConfig = { + dialect: 'postgres', + host: 'db.example.com', + port: 5432, + database: 'app_test', + user: 'app', + connectTimeoutMs: 2_000, +}; + +/** A driver error with a Node network code, the way pg and mysql2 raise them. */ +function networkError(code: string): Error { + + return Object.assign(new Error(`connect ${code}`), { code }); + +} + + +describe('connection: explainConnectionError', () => { + + it('should name a connection that timed out, with the timeout that applied', () => { + + const explained = explainConnectionError(networkError('ETIMEDOUT'), config); + + expect(explained.message).toContain('db.example.com:5432 did not answer within 2000ms'); + + }); + + it('should read tedious\'s own timeout code the same way', () => { + + const explained = explainConnectionError(networkError('ETIMEOUT'), { ...config, dialect: 'mssql', port: 1433 }); + + expect(explained.message).toContain('db.example.com:1433 did not answer'); + + }); + + it('should name a host with no network route to it', () => { + + const explained = explainConnectionError(networkError('EHOSTUNREACH'), config); + + expect(explained.message).toContain('db.example.com is unreachable from this machine'); + + }); + + it('should name a handshake the server cut off', () => { + + const explained = explainConnectionError(networkError('ECONNRESET'), config); + + expect(explained.message).toContain('closed the connection during the handshake'); + + }); + + it('should find a code wrapped in cause and in an AggregateError', () => { + + const refused = new AggregateError([networkError('ECONNREFUSED')], ''); + const wrapped = new Error('Failed to connect - Could not connect (sequence)', { cause: refused }); + + const explained = explainConnectionError(wrapped, config); + + expect(explained.message).toContain('Connection refused at db.example.com:5432'); + + }); + + it('should keep the driver\'s error as the cause, so its code stays reachable', () => { + + const original = networkError('ECONNREFUSED'); + + expect(explainConnectionError(original, config).cause).toBe(original); + + }); + + it('should return an error with no code and nothing to explain unchanged, so noorm\'s own errors keep their type', () => { + + const original = new Error('Operation aborted'); + + expect(explainConnectionError(original, config)).toBe(original); + + }); + + it('should keep the driver\'s wording for a code it has no reason for, and still log the code', () => { + + const original = Object.assign(new Error('protocol violation'), { code: '08P01' }); + + const explained = explainConnectionError(original, config); + + expect(explained.message).toBe('protocol violation'); + expect(explained).toBeInstanceOf(DatabaseConnectionError); + expect(explained instanceof DatabaseConnectionError && explained.serverCode).toBe('08P01'); + + }); + + it('should log every message along the chain, since the outer one is often empty', () => { + + const refused = new AggregateError([networkError('ECONNREFUSED')], ''); + + const explained = explainConnectionError(refused, config); + + expect(explained instanceof DatabaseConnectionError && explained.serverMessage).toBe('connect ECONNREFUSED'); + + }); + +}); diff --git a/tests/integration/connection/connection-errors.test.ts b/tests/integration/connection/connection-errors.test.ts new file mode 100644 index 00000000..640a221b --- /dev/null +++ b/tests/integration/connection/connection-errors.test.ts @@ -0,0 +1,498 @@ +/** + * Integration test: a failed connection says why it failed. + * + * Every case is produced against the docker-compose.test.yml containers + * rather than faked, because the error shapes are the drivers' and change + * with them. + */ +import { describe, it, expect, beforeAll, afterAll } from 'bun:test'; +import { chmodSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { sql } from 'kysely'; + +import { attempt } from '@logosdx/utils'; + +import { createConnection, testConnection } from '../../../src/core/connection/factory.js'; +import type { ConnectionConfig, ConnectionResult } from '../../../src/core/connection/types.js'; +import { observer } from '../../../src/core/observer.js'; +import type { NoormEvents } from '../../../src/core/observer.js'; + +import { TEST_CONNECTIONS, assertTestDatabase, createTestConnection, skipIfNoContainer } from '../../utils/db.js'; + + +const LOGIN = 'noorm_err_login'; +const PASSWORD = 'Err!#$;pw1'; + +/** + * Nothing listens on port 1 on a developer machine or a CI runner, so the + * connection is refused rather than left to time out. + */ +const CLOSED_PORT = 1; + +/** RFC 2606 reserves `.invalid`, so this name never resolves. */ +const UNKNOWN_HOST = 'noorm-no-such-host.invalid'; + +/** A database the test login exists for but may not use. */ +const NO_ACCESS_DB = 'noorm_test_err_no_access'; + +async function connectionError(config: ConnectionConfig): Promise { + + const result = await testConnection(config); + + expect(result.ok).toBe(false); + + return result.error ?? ''; + +} + +/** + * The `connection:error` event a failed connection emits, which is what the + * log file records. + */ +async function loggedFailure(config: ConnectionConfig): Promise { + + const events: NoormEvents['connection:error'][] = []; + const stop = observer.on('connection:error', (data) => { + + events.push(data); + + }); + + await testConnection(config); + stop(); + + expect(events.length).toBeGreaterThan(0); + + return events.at(-1)!; + +} + +/** + * Open an admin connection that still passes the test-database guard, for the + * dialects whose test user cannot create accounts. + */ +async function adminConnection(config: ConnectionConfig): Promise { + + assertTestDatabase(config); + + return createConnection(config, '__test_admin__'); + +} + + +describe('connection: errors name their reason on mssql', () => { + + const base = { ...TEST_CONNECTIONS.mssql }; + let admin: ConnectionResult; + + beforeAll(async () => { + + await skipIfNoContainer('mssql'); + + admin = await createTestConnection('mssql'); + + await sql.raw(` + IF SUSER_ID('${LOGIN}') IS NULL + CREATE LOGIN ${LOGIN} WITH PASSWORD = '${PASSWORD}', CHECK_POLICY = OFF; + IF SUSER_ID('${LOGIN}_disabled') IS NULL + CREATE LOGIN ${LOGIN}_disabled WITH PASSWORD = '${PASSWORD}', CHECK_POLICY = OFF; + ALTER LOGIN ${LOGIN}_disabled DISABLE; + IF SUSER_ID('${LOGIN}_expired') IS NULL + CREATE LOGIN ${LOGIN}_expired WITH PASSWORD = '${PASSWORD}Long' MUST_CHANGE, + CHECK_EXPIRATION = ON, CHECK_POLICY = ON; + IF USER_ID('${LOGIN}') IS NULL CREATE USER ${LOGIN} FOR LOGIN ${LOGIN}; + IF USER_ID('${LOGIN}_disabled') IS NULL CREATE USER ${LOGIN}_disabled FOR LOGIN ${LOGIN}_disabled; + IF USER_ID('${LOGIN}_expired') IS NULL CREATE USER ${LOGIN}_expired FOR LOGIN ${LOGIN}_expired; + `).execute(admin.db); + + }, 30_000); + + afterAll(async () => { + + if (!admin) return; + + await attempt(() => sql.raw(` + IF USER_ID('${LOGIN}') IS NOT NULL DROP USER ${LOGIN}; + IF USER_ID('${LOGIN}_disabled') IS NOT NULL DROP USER ${LOGIN}_disabled; + IF USER_ID('${LOGIN}_expired') IS NOT NULL DROP USER ${LOGIN}_expired; + IF SUSER_ID('${LOGIN}') IS NOT NULL DROP LOGIN ${LOGIN}; + IF SUSER_ID('${LOGIN}_disabled') IS NOT NULL DROP LOGIN ${LOGIN}_disabled; + IF SUSER_ID('${LOGIN}_expired') IS NOT NULL DROP LOGIN ${LOGIN}_expired; + `).execute(admin.db)); + + await admin.destroy(); + + }); + + it('should connect with the right password, so the failures below are about the cases', async () => { + + expect(await testConnection({ ...base, user: LOGIN, password: PASSWORD })).toEqual({ ok: true }); + + }); + + it('should say SQL Server withholds the reason for a rejected login, and list what it could be', async () => { + + const error = await connectionError({ ...base, user: LOGIN, password: 'wrong' }); + + expect(error).toContain(`Login failed for user '${LOGIN}'`); + expect(error).toContain('wrong password'); + expect(error).toContain('error log'); + // The TUI offers to create the database when it reads this phrase. + expect(error).not.toContain('does not exist'); + + }); + + it('should log SQL Server\'s own error number and message next to the reworded one', async () => { + + const logged = await loggedFailure({ ...base, user: LOGIN, password: 'wrong' }); + + expect(logged.serverCode).toBe('18456'); + expect(logged.serverMessage).toContain(`Login failed for user '${LOGIN}'.`); + + }); + + it('should name a disabled login and how to enable it', async () => { + + const error = await connectionError({ ...base, user: `${LOGIN}_disabled`, password: PASSWORD }); + + expect(error).toContain(`Login '${LOGIN}_disabled' is disabled`); + expect(error).toContain('ALTER LOGIN'); + + }); + + it('should name a password that must be changed before login', async () => { + + const error = await connectionError({ ...base, user: `${LOGIN}_expired`, password: `${PASSWORD}Long` }); + + expect(error).toContain(`The password for login '${LOGIN}_expired' must be changed`); + + }); + + it('should log the socket error that tedious reports as "Could not connect (sequence)"', async () => { + + const logged = await loggedFailure({ ...base, host: 'localhost', port: CLOSED_PORT }); + + expect(logged.serverCode).toContain('ECONNREFUSED'); + expect(logged.serverMessage).toContain('Could not connect (sequence)'); + expect(logged.serverMessage).toContain('ECONNREFUSED'); + + }); + + it('should name a refused port instead of tedious\'s "Could not connect (sequence)"', async () => { + + const error = await connectionError({ ...base, host: 'localhost', port: CLOSED_PORT }); + + expect(error).toContain(`Connection refused at localhost:${CLOSED_PORT}`); + + }); + + it('should name a host that does not resolve', async () => { + + const error = await connectionError({ ...base, host: UNKNOWN_HOST }); + + expect(error).toContain(`Host '${UNKNOWN_HOST}' could not be found`); + + }); + + it('should name a rejected TLS certificate and keep encryption available without validation', async () => { + + const error = await connectionError({ ...base, host: 'localhost', ssl: true }); + + expect(error).toContain('TLS certificate was rejected'); + expect(error).toContain('DEPTH_ZERO_SELF_SIGNED_CERT'); + + }); + +}); + + +describe('connection: errors name their reason on postgres', () => { + + const base = { ...TEST_CONNECTIONS.postgres }; + let admin: ConnectionResult; + + beforeAll(async () => { + + await skipIfNoContainer('postgres'); + + admin = await createTestConnection('postgres'); + + await sql.raw(`DROP DATABASE IF EXISTS ${NO_ACCESS_DB}`).execute(admin.db); + await sql.raw(`DROP ROLE IF EXISTS ${LOGIN}, ${LOGIN}_limit, ${LOGIN}_nologin`).execute(admin.db); + await sql.raw(`CREATE ROLE ${LOGIN} LOGIN PASSWORD '${PASSWORD}'`).execute(admin.db); + await sql.raw(`CREATE ROLE ${LOGIN}_limit LOGIN PASSWORD '${PASSWORD}' CONNECTION LIMIT 0`).execute(admin.db); + await sql.raw(`CREATE ROLE ${LOGIN}_nologin NOLOGIN PASSWORD '${PASSWORD}'`).execute(admin.db); + await sql.raw(`CREATE DATABASE ${NO_ACCESS_DB}`).execute(admin.db); + await sql.raw(`REVOKE CONNECT ON DATABASE ${NO_ACCESS_DB} FROM PUBLIC`).execute(admin.db); + + }, 30_000); + + afterAll(async () => { + + if (!admin) return; + + await attempt(() => sql.raw(`DROP DATABASE IF EXISTS ${NO_ACCESS_DB}`).execute(admin.db)); + await attempt(() => sql.raw(`DROP ROLE IF EXISTS ${LOGIN}, ${LOGIN}_limit, ${LOGIN}_nologin`).execute(admin.db)); + await admin.destroy(); + + }); + + it('should connect with the right password, so the failures below are about the cases', async () => { + + expect(await testConnection({ ...base, user: LOGIN, password: PASSWORD })).toEqual({ ok: true }); + + }); + + it('should say PostgreSQL withholds the reason for a rejected password, and list what it could be', async () => { + + const error = await connectionError({ ...base, user: LOGIN, password: 'wrong' }); + + expect(error).toContain(`Password authentication failed for user '${LOGIN}'`); + expect(error).toContain('expired password'); + expect(error).not.toContain('does not exist'); + + }); + + it('should log PostgreSQL\'s SQLSTATE and message next to the reworded one', async () => { + + const logged = await loggedFailure({ ...base, user: LOGIN, password: 'wrong' }); + + expect(logged.serverCode).toBe('28P01'); + expect(logged.serverMessage).toContain(`password authentication failed for user "${LOGIN}"`); + + }); + + it('should name a role that may not log in', async () => { + + const error = await connectionError({ ...base, user: `${LOGIN}_nologin`, password: PASSWORD }); + + expect(error).toContain(`Role '${LOGIN}_nologin' is not allowed to log in (NOLOGIN)`); + + }); + + it('should name a missing CONNECT privilege', async () => { + + const error = await connectionError({ ...base, database: NO_ACCESS_DB, user: LOGIN, password: PASSWORD }); + + expect(error).toContain(`Role '${LOGIN}' may not connect to database '${NO_ACCESS_DB}'`); + + }); + + // "too many connections" is retried with backoff, which alone takes ~6s. + it('should surface the server\'s error when retries run out, not "Max retries reached"', async () => { + + const logged = await loggedFailure({ ...base, user: `${LOGIN}_limit`, password: PASSWORD }); + + expect(logged.error).toContain(`Role '${LOGIN}_limit' has used up its connection limit`); + expect(logged.serverMessage).toContain('too many connections for role'); + expect(logged.error).not.toContain('Max retries reached'); + + }, 30_000); + + it('should name a refused port instead of an empty message', async () => { + + const error = await connectionError({ ...base, host: 'localhost', port: CLOSED_PORT }); + + expect(error).toContain(`Connection refused at localhost:${CLOSED_PORT}`); + + }); + + it('should name a host that does not resolve', async () => { + + const error = await connectionError({ ...base, host: UNKNOWN_HOST }); + + expect(error).toContain(`Host '${UNKNOWN_HOST}' could not be found`); + + }); + +}); + + +describe('connection: errors name their reason on mysql', () => { + + // The test user cannot create accounts; root can, and the database name + // still passes the test-database guard. + const base = { ...TEST_CONNECTIONS.mysql }; + const root = { ...base, user: 'root', password: 'noorm_test' }; + let admin: ConnectionResult; + + beforeAll(async () => { + + await skipIfNoContainer('mysql'); + + admin = await adminConnection(root); + + await sql.raw(`DROP USER IF EXISTS '${LOGIN}'@'%', '${LOGIN}_locked'@'%', '${LOGIN}_expired'@'%'`).execute(admin.db); + await sql.raw(`CREATE USER '${LOGIN}'@'%' IDENTIFIED BY '${PASSWORD}'`).execute(admin.db); + await sql.raw(`GRANT ALL ON ${base.database}.* TO '${LOGIN}'@'%'`).execute(admin.db); + await sql.raw(`CREATE USER '${LOGIN}_locked'@'%' IDENTIFIED BY '${PASSWORD}' ACCOUNT LOCK`).execute(admin.db); + await sql.raw(`CREATE USER '${LOGIN}_expired'@'%' IDENTIFIED BY '${PASSWORD}' PASSWORD EXPIRE`).execute(admin.db); + await sql.raw(`GRANT ALL ON ${base.database}.* TO '${LOGIN}_expired'@'%'`).execute(admin.db); + await sql.raw(`CREATE DATABASE IF NOT EXISTS ${NO_ACCESS_DB}`).execute(admin.db); + + }, 30_000); + + afterAll(async () => { + + if (!admin) return; + + await attempt(() => sql.raw(`DROP DATABASE IF EXISTS ${NO_ACCESS_DB}`).execute(admin.db)); + await attempt(() => sql.raw( + `DROP USER IF EXISTS '${LOGIN}'@'%', '${LOGIN}_locked'@'%', '${LOGIN}_expired'@'%'`, + ).execute(admin.db)); + await admin.destroy(); + + }); + + it('should connect with the right password, so the failures below are about the cases', async () => { + + expect(await testConnection({ ...base, user: LOGIN, password: PASSWORD })).toEqual({ ok: true }); + + }); + + it('should say MySQL withholds the reason for a denied login, and list what it could be', async () => { + + const error = await connectionError({ ...base, user: LOGIN, password: 'wrong' }); + + expect(error).toContain(`Access denied for user '${LOGIN}'`); + expect(error).toContain('REQUIRE SSL'); + expect(error).not.toContain('does not exist'); + + }); + + it('should log MySQL\'s error code and message, which names the client host', async () => { + + const logged = await loggedFailure({ ...base, user: LOGIN, password: 'wrong' }); + + expect(logged.serverCode).toBe('ER_ACCESS_DENIED_ERROR'); + expect(logged.serverMessage).toContain(`Access denied for user '${LOGIN}'@`); + + }); + + it('should name a locked account', async () => { + + const error = await connectionError({ ...base, user: `${LOGIN}_locked`, password: PASSWORD }); + + expect(error).toContain(`Account '${LOGIN}_locked' is locked`); + + }); + + it('should name an expired password', async () => { + + const error = await connectionError({ ...base, user: `${LOGIN}_expired`, password: PASSWORD }); + + expect(error).toContain(`The password for '${LOGIN}_expired' has expired`); + + }); + + it('should name a database the user holds no privileges on', async () => { + + const error = await connectionError({ ...base, database: NO_ACCESS_DB, user: LOGIN, password: PASSWORD }); + + expect(error).toContain(`User '${LOGIN}' has no privileges on database '${NO_ACCESS_DB}'`); + + }); + + it('should name a missing password', async () => { + + const error = await connectionError({ ...base, user: LOGIN, password: undefined }); + + expect(error).toContain(`No password was supplied for user '${LOGIN}'`); + + }); + + it('should name a refused port instead of an empty message', async () => { + + const error = await connectionError({ ...base, host: 'localhost', port: CLOSED_PORT }); + + expect(error).toContain(`Connection refused at localhost:${CLOSED_PORT}`); + + }); + +}); + + +describe('connection: errors name their reason on sqlite', () => { + + let dir: string; + + beforeAll(() => { + + dir = mkdtempSync(join(tmpdir(), 'noorm-test-sqlite-errors-')); + + writeFileSync(join(dir, 'junk.db'), 'not a sqlite database '.repeat(40)); + writeFileSync(join(dir, 'locked.db'), ''); + chmodSync(join(dir, 'locked.db'), 0o000); + mkdirSync(join(dir, 'readonly')); + chmodSync(join(dir, 'readonly'), 0o555); + + }); + + afterAll(() => { + + chmodSync(join(dir, 'locked.db'), 0o644); + chmodSync(join(dir, 'readonly'), 0o755); + rmSync(dir, { recursive: true, force: true }); + + }); + + it('should name a missing directory', async () => { + + const error = await connectionError({ dialect: 'sqlite', database: join(dir, 'nope', 'app.db') }); + + expect(error).toContain(`directory '${join(dir, 'nope')}' is missing`); + expect(error).not.toContain('does not exist'); + + }); + + it('should name a file this process may not read or write', async () => { + + const error = await connectionError({ dialect: 'sqlite', database: join(dir, 'locked.db') }); + + expect(error).toContain('lacks read or write permission'); + + }); + + it('should name a directory a new database file cannot be created in', async () => { + + const error = await connectionError({ dialect: 'sqlite', database: join(dir, 'readonly', 'app.db') }); + + expect(error).toContain(`lacks write permission on '${join(dir, 'readonly')}'`); + + }); + + it('should name a file that is not a SQLite database', async () => { + + const error = await connectionError({ dialect: 'sqlite', database: join(dir, 'junk.db') }); + + expect(error).toContain('is not a SQLite database'); + + }); + +}); + + +// Last in the file on purpose. pg raises this error on the client side and +// leaves the socket open, so PostgreSQL keeps that backend in authentication +// until authentication_timeout (60s by default), and DROP DATABASE waits for +// it. Run earlier, it stalls the postgres suite's cleanup for most of a minute. +describe('connection: a missing postgres password is named', () => { + + beforeAll(async () => { + + await skipIfNoContainer('postgres'); + + }); + + it('should name a missing password instead of the SCRAM client message', async () => { + + // An unknown role gets the same SCRAM challenge, so no role is needed. + const error = await connectionError({ ...TEST_CONNECTIONS.postgres, user: `${LOGIN}_nopass`, password: '' }); + + expect(error).toContain(`No password was supplied for user '${LOGIN}_nopass'`); + expect(error).not.toContain('SCRAM'); + + }); + +}); diff --git a/tests/integration/connection/mssql-login.test.ts b/tests/integration/connection/mssql-login.test.ts new file mode 100644 index 00000000..761e0838 --- /dev/null +++ b/tests/integration/connection/mssql-login.test.ts @@ -0,0 +1,165 @@ +/** + * Integration test: MSSQL logins that hold no server-level privileges. + * + * A contained database user (the norm on Azure SQL Database) cannot log in to + * `master`, and a login without `VIEW ANY DATABASE` cannot see other rows in + * `sys.databases`; connecting must need neither. Passwords carry special + * characters so a password bug and a privilege bug stay distinguishable. + * + * Requires the docker-compose.test.yml MSSQL container on port 11433, and + * fails with a clear message when it is unreachable. + */ +import { describe, it, expect, beforeAll, afterAll } from 'bun:test'; +import { sql } from 'kysely'; + +import { attempt } from '@logosdx/utils'; + +import { createMssqlConnection } from '../../../src/core/connection/dialects/mssql.js'; +import { testConnection } from '../../../src/core/connection/factory.js'; +import type { ConnectionConfig, ConnectionResult } from '../../../src/core/connection/types.js'; + +import { TEST_CONNECTIONS, createTestConnection, skipIfNoContainer } from '../../utils/db.js'; + + +const CONTAINED_DB = 'noorm_test_login_contained'; +const PLAIN_DB = 'noorm_test_login_plain'; +const CONTAINED_USER = { user: 'noorm_contained_user', password: 'C0nt@ined!#$;pw' }; +const NO_VIEW_LOGIN = { user: 'noorm_no_view_any', password: 'N0V!ew#$;pw' }; + +function connectionTo(database: string): ConnectionConfig { + + return { ...TEST_CONNECTIONS.mssql, database }; + +} + +async function whoAmI(config: ConnectionConfig): Promise { + + const conn = await createMssqlConnection(config); + + const [rows, err] = await attempt(async () => { + + const result = await sql<{ who: string }>`SELECT USER_NAME() AS who`.execute(conn.db); + + return result.rows; + + }); + + await conn.destroy(); + + if (err) { + + throw err; + + } + + return rows![0]!.who; + +} + + +describe('connection/dialects/mssql: logins without server-level privileges', () => { + + let sa: ConnectionResult; + + beforeAll(async () => { + + await skipIfNoContainer('mssql'); + + sa = await createTestConnection('mssql'); + + const exec = (statement: string) => sql.raw(statement).execute(sa.db); + + await exec('EXEC sp_configure \'contained database authentication\', 1; RECONFIGURE;'); + + await exec(`IF DB_ID('${CONTAINED_DB}') IS NULL CREATE DATABASE ${CONTAINED_DB};`); + await exec(`ALTER DATABASE ${CONTAINED_DB} SET CONTAINMENT = PARTIAL;`); + await exec(`IF DB_ID('${PLAIN_DB}') IS NULL CREATE DATABASE ${PLAIN_DB};`); + + await exec(` + IF SUSER_ID('${NO_VIEW_LOGIN.user}') IS NULL + CREATE LOGIN ${NO_VIEW_LOGIN.user} WITH PASSWORD = '${NO_VIEW_LOGIN.password}', CHECK_POLICY = OFF; + `); + + // These statements only run from inside the database they act on, and + // this pool is pinned to the test database. + await exec(`EXEC('USE master; DENY VIEW ANY DATABASE TO ${NO_VIEW_LOGIN.user};')`); + + await exec(`EXEC('USE ${CONTAINED_DB}; + IF USER_ID(''${CONTAINED_USER.user}'') IS NULL + CREATE USER ${CONTAINED_USER.user} WITH PASSWORD = ''${CONTAINED_USER.password}'';')`); + + await exec(`EXEC('USE ${PLAIN_DB}; + IF USER_ID(''${NO_VIEW_LOGIN.user}'') IS NULL + CREATE USER ${NO_VIEW_LOGIN.user} FOR LOGIN ${NO_VIEW_LOGIN.user};')`); + + }); + + afterAll(async () => { + + if (!sa) return; + + const exec = (statement: string) => sql.raw(statement).execute(sa.db); + + await attempt(() => exec(` + IF DB_ID('${CONTAINED_DB}') IS NOT NULL + BEGIN + ALTER DATABASE ${CONTAINED_DB} SET SINGLE_USER WITH ROLLBACK IMMEDIATE; + DROP DATABASE ${CONTAINED_DB}; + END + IF DB_ID('${PLAIN_DB}') IS NOT NULL + BEGIN + ALTER DATABASE ${PLAIN_DB} SET SINGLE_USER WITH ROLLBACK IMMEDIATE; + DROP DATABASE ${PLAIN_DB}; + END + IF SUSER_ID('${NO_VIEW_LOGIN.user}') IS NOT NULL DROP LOGIN ${NO_VIEW_LOGIN.user}; + `)); + + await sa.destroy(); + + }); + + it('should connect as a contained database user, who cannot log in to master', async () => { + + const who = await whoAmI({ ...connectionTo(CONTAINED_DB), ...CONTAINED_USER }); + + expect(who).toBe(CONTAINED_USER.user); + + }); + + it('should connect as a login that cannot see other databases', async () => { + + const who = await whoAmI({ ...connectionTo(PLAIN_DB), ...NO_VIEW_LOGIN }); + + expect(who).toBe(NO_VIEW_LOGIN.user); + + }); + + it('should name the database, not the password, when the target does not exist', async () => { + + const [result, err] = await attempt(() => createMssqlConnection(connectionTo('noorm_test_login_missing'))); + + expect(result).toBeNull(); + expect(err?.message).toContain('\'noorm_test_login_missing\''); + expect(err?.message).toContain('does not exist'); + + }); + + // The config add/edit screens run this check before saving, so failing it + // leaves a contained user with no way to store a config at all. + it('should pass the server-only connection test as a contained database user', async () => { + + const result = await testConnection({ ...connectionTo(CONTAINED_DB), ...CONTAINED_USER }, { testServerOnly: true }); + + expect(result).toEqual({ ok: true }); + + }); + + it('should pass the server-only connection test before the target database exists', async () => { + + const result = await testConnection(connectionTo('noorm_test_login_missing'), { testServerOnly: true }); + + expect(result).toEqual({ ok: true }); + + }); + +}); diff --git a/tests/integration/connection/timeout-abort.test.ts b/tests/integration/connection/timeout-abort.test.ts index 595a6134..10602e29 100644 --- a/tests/integration/connection/timeout-abort.test.ts +++ b/tests/integration/connection/timeout-abort.test.ts @@ -10,6 +10,7 @@ import { describe, it, expect } from 'bun:test'; import { attempt } from '@logosdx/utils'; import { + DatabaseConnectionError, createConnection, testConnection, getConnectionManager, @@ -59,10 +60,12 @@ describe('integration: unreachable host', () => { expect(err).not.toBeInstanceOf(OperationAbortedError); expect(elapsed).toBeLessThan(SHORT_TIMEOUT_MS * 4); - // The driver's own message, not the generic wrapper's. The two - // deadlines race, and the driver has to win it: only its timeout tears - // the socket down, and only it can name what actually failed. - expect(err?.message).toContain('connection timeout'); + // The driver's deadline, not the generic wrapper's (which is the + // timeout plus a grace period). The two race, and the driver has to + // win: only its timeout tears the socket down, and only it can name + // what failed. + expect(err?.message).toContain(`did not answer within ${SHORT_TIMEOUT_MS}ms`); + expect(err instanceof DatabaseConnectionError && err.serverMessage).toContain('connection timeout'); }, 30_000);