From 5c21cb770bfa23d3b6c33839b3f454923e67fa7c Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Fri, 7 Aug 2026 15:24:36 +0200 Subject: [PATCH 1/4] feat(#630): inert clickhouse-http workspace scaffold (Checkpoint 1) Introduce the repository's first npm workspace, packages/clickhouse-http, as a statement-free private package with no production imports yet: root package.json declares the workspace + a 0.0.0 dependency on @altinity/clickhouse-http, package-lock.json is regenerated via npm install (never hand-edited), tsconfig.json includes the package source, the Dockerfile copies packages/ into the build stage before npm ci, and the CI path filters add packages/** to the unit/build/bundle/e2e/docker classifications. Verified npm ci, the full six-command gate, and a real docker build are green at this checkpoint before any runtime cutover. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci.yml | 7 +++++++ Dockerfile | 1 + package-lock.json | 12 ++++++++++++ package.json | 4 ++++ packages/clickhouse-http/package.json | 9 +++++++++ packages/clickhouse-http/src/index.ts | 7 +++++++ tsconfig.json | 2 +- 7 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 packages/clickhouse-http/package.json create mode 100644 packages/clickhouse-http/src/index.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fd1b3271..f7ce5b3c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -54,6 +54,9 @@ jobs: - 'package.json' - 'package-lock.json' - '.github/workflows/**' + # #630 Phase 2: the first npm workspace — its source and + # manifest are unit-test/typecheck inputs too. + - 'packages/**' build: - 'src/**' - 'schemas/**' @@ -61,6 +64,7 @@ jobs: - 'package.json' - 'package-lock.json' - '.github/workflows/**' + - 'packages/**' bundle: - 'src/**' - 'schemas/**' @@ -70,6 +74,7 @@ jobs: - 'package.json' - 'package-lock.json' - '.github/workflows/**' + - 'packages/**' # #564: the browser suite is the ONLY gate that can see CSS layout, # container queries, real focus/hover, and IndexedDB — happy-dom sees # none of it. Anything that can move rendered geometry or the @@ -91,6 +96,7 @@ jobs: - 'package.json' - 'package-lock.json' - '.github/workflows/**' + - 'packages/**' docker: - 'Dockerfile' - 'deploy/caddy/**' @@ -101,6 +107,7 @@ jobs: - 'package.json' - 'package-lock.json' - '.github/workflows/**' + - 'packages/**' # Example-only PRs still validate every portable bundle, authored Dashboard, # filter contract, and generator normalization without paying for coverage, diff --git a/Dockerfile b/Dockerfile index 81c33c67..108756e6 100644 --- a/Dockerfile +++ b/Dockerfile @@ -12,6 +12,7 @@ FROM node:22-bookworm-slim AS build WORKDIR /app COPY package.json package-lock.json ./ +COPY packages ./packages COPY build ./build COPY examples ./examples COPY schemas ./schemas diff --git a/package-lock.json b/package-lock.json index 47aa8d17..a70838dd 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8,7 +8,11 @@ "name": "altinity-sql-browser", "version": "0.7.3", "license": "Apache-2.0", + "workspaces": [ + "packages/clickhouse-http" + ], "dependencies": { + "@altinity/clickhouse-http": "0.0.0", "@codemirror/autocomplete": "^6.20.3", "@codemirror/commands": "^6.10.4", "@codemirror/lang-json": "^6.0.2", @@ -43,6 +47,10 @@ "node": ">=22" } }, + "node_modules/@altinity/clickhouse-http": { + "resolved": "packages/clickhouse-http", + "link": true + }, "node_modules/@babel/helper-string-parser": { "version": "7.29.7", "resolved": "https://registry.npmjs.org/@babel/helper-string-parser/-/helper-string-parser-7.29.7.tgz", @@ -2928,6 +2936,10 @@ "optional": true } } + }, + "packages/clickhouse-http": { + "name": "@altinity/clickhouse-http", + "version": "0.0.0" } } } diff --git a/package.json b/package.json index 002ff338..bea1073f 100644 --- a/package.json +++ b/package.json @@ -8,6 +8,9 @@ "engines": { "node": ">=22" }, + "workspaces": [ + "packages/clickhouse-http" + ], "scripts": { "prebuild": "npm run check:schemas && npm run check:examples", "build": "node build/build.mjs", @@ -43,6 +46,7 @@ "vitest": "^4.1.10" }, "dependencies": { + "@altinity/clickhouse-http": "0.0.0", "@codemirror/autocomplete": "^6.20.3", "@codemirror/commands": "^6.10.4", "@codemirror/lang-json": "^6.0.2", diff --git a/packages/clickhouse-http/package.json b/packages/clickhouse-http/package.json new file mode 100644 index 00000000..6e60f1f2 --- /dev/null +++ b/packages/clickhouse-http/package.json @@ -0,0 +1,9 @@ +{ + "name": "@altinity/clickhouse-http", + "version": "0.0.0", + "private": true, + "type": "module", + "exports": { + ".": "./src/index.ts" + } +} diff --git a/packages/clickhouse-http/src/index.ts b/packages/clickhouse-http/src/index.ts new file mode 100644 index 00000000..ffd71247 --- /dev/null +++ b/packages/clickhouse-http/src/index.ts @@ -0,0 +1,7 @@ +// Issue #630 Phase 2 — inert workspace scaffold (Checkpoint 1). This package +// is the repository's first npm workspace: the low-level ClickHouse HTTP +// URL/request mechanics move here from `src/net/clickhouse-http-transport.ts` +// in Checkpoint 2. Statement-free on purpose until then — package source +// lands under strict TypeScript + coverage only together with its +// implementation and tests (see the Phase 2 plan's migration order). +export {}; diff --git a/tsconfig.json b/tsconfig.json index fc496581..4407f1e4 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -15,5 +15,5 @@ "lib": ["ES2022", "DOM", "DOM.Iterable"], "skipLibCheck": true }, - "include": ["src/**/*.ts", "tests/types/**/*.ts", "tests/unit/**/*.ts", "tests/helpers/**/*.ts", "tests/spike/clickhouse-client/**/*.ts"] + "include": ["src/**/*.ts", "tests/types/**/*.ts", "tests/unit/**/*.ts", "tests/helpers/**/*.ts", "tests/spike/clickhouse-client/**/*.ts", "packages/clickhouse-http/src/**/*.ts"] } From c24a880b2991754d7f6c7e00deda6a4e4190ef5f Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Fri, 7 Aug 2026 15:45:48 +0200 Subject: [PATCH 2/4] feat(#630): move chUrl/URL serialization and low-level request into @altinity/clickhouse-http (Checkpoint 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implement packages/clickhouse-http/src/url.ts (chUrl, moved verbatim) and client.ts (createClickHouseHttpClient/request(), the direct injected-fetch invocation) behind the package's public "." export, and cut over src/net/clickhouse-http-transport.ts to a temporary compatibility adapter whose send() delegates to the package's request() — streamLines() stays local, deferred to Phase 3. src/net/clickhouse-transport.types.ts now aliases TransportDeps/TransportRequest from the package's own types, and ch-client.ts binds its chUrl re-export to the package while keeping its eager pre-credential preflight call and composition graph unchanged. Architecture: build/check-boundaries.mjs gains Rules A-D (package must not import root src/**; package source has zero bare specifiers — an empty allowlist, since root hoists real runtime deps like @preact/signals-core that TS/esbuild could otherwise resolve; root src/** must not deep-import the package's own src/**; the bare package name is importable only under src/net/**, and its deep-import subpath form is banned everywhere), mirrored in tests/unit/clickhouse-http-package-policy.test.js with non-writing virtual- file sabotage probes for each rule plus a drift-bind check against the production RULES source. Tests: the exact chUrl literal suite and the Phase-1 contract suite move to tests/unit/clickhouse-http-package.test.ts, registered directly against the package's own request() (not just the compatibility adapter) — proving native Response identity, exactly-one-Fetch, exact SQL, opaque Authorization, live origin/fetch, and raw-byte safety hold at the package boundary. New focused tests pin the reviewed promise-settlement shape: a synchronous chUrl URIError must surface as a rejected promise, never a synchronous throw, at both the package request() and the compatibility adapter's send(). Bundle attribution treats packages/** as first-party project code (not "other"), proven against the real esbuild metafile. All 8 known raw-ESM e2e fixtures gain an import-map entry for the bare @altinity/clickhouse-http specifier; a full esbuild-transform-accurate dependency-graph audit of every other e2e fixture (which also excludes type-only `import type` chains erased at serve time, unlike a naive text scan) found no additional fixture needing one. Co-Authored-By: Claude Sonnet 5 --- build/check-boundaries.mjs | 66 ++++ build/size-report-lib.mjs | 6 + packages/clickhouse-http/src/client.ts | 72 ++++ packages/clickhouse-http/src/index.ts | 23 +- packages/clickhouse-http/src/url.ts | 37 ++ src/net/ch-client.ts | 12 +- src/net/clickhouse-http-transport.ts | 55 +-- src/net/clickhouse-transport.types.ts | 64 ++-- tests/e2e/clickhouse-http-transport.html | 12 +- tests/e2e/dashboard-kpi-move.html | 1 + tests/e2e/dashboard-membership.html | 1 + tests/e2e/dashboard-tree.html | 1 + tests/e2e/import-example-dashboard.html | 1 + tests/e2e/oauth-document-recovery/index.html | 1 + tests/e2e/tile-open-workbench.html | 1 + tests/e2e/variable-tab.html | 1 + tests/unit/ch-client.test.ts | 16 +- .../clickhouse-http-package-policy.test.js | 323 ++++++++++++++++++ tests/unit/clickhouse-http-package.test.ts | 145 ++++++++ tests/unit/clickhouse-http-transport.test.ts | 92 ++--- tests/unit/size-report.test.js | 36 ++ tests/vitest.config.ts | 2 +- 22 files changed, 818 insertions(+), 150 deletions(-) create mode 100644 packages/clickhouse-http/src/client.ts create mode 100644 packages/clickhouse-http/src/url.ts create mode 100644 tests/unit/clickhouse-http-package-policy.test.js create mode 100644 tests/unit/clickhouse-http-package.test.ts diff --git a/build/check-boundaries.mjs b/build/check-boundaries.mjs index fd048eb0..5a3b92dd 100644 --- a/build/check-boundaries.mjs +++ b/build/check-boundaries.mjs @@ -131,6 +131,23 @@ const RULES = [ 'src/net/oauth-config.ts', 'src/application', 'src/ui'], why: 'issue #585 Phase 1: the transport contract must not couple to auth/application policy or UI, even type-only', }, + // Issue #630 Phase 2 — Rule A: the new workspace package must not depend on + // ANY SQL Browser source, relatively. (A separate dedicated block below + // additionally bans a browser-root-literal or bare-specifier escape, since + // this generic loop only inspects specifiers starting with '.'.) + { + dir: 'packages/clickhouse-http/src', + forbidden: ['src'], + why: 'issue #630 Phase 2: clickhouse-http must not depend on SQL Browser source', + }, + // Issue #630 Phase 2 — Rule C: SQL Browser source must consume the package + // through its public export, never a relative deep import into the + // package's own src/** implementation files. + { + dir: 'src', + forbidden: ['packages/clickhouse-http/src'], + why: 'issue #630 Phase 2: SQL Browser must use the package public export', + }, ]; function collectFiles(target) { @@ -267,6 +284,55 @@ for (const file of collectFiles(path.join(repoRoot, 'src'))) { } } +// Issue #630 Phase 2 — Rule B: packages/clickhouse-http/src/** must have +// ZERO bare specifiers (an empty allowlist, not just an empty manifest +// `dependencies` object). Root hoists many runtime dependencies already +// (e.g. @preact/signals-core), so a zero-dependency manifest alone would not +// stop package source from importing one undeclared — TypeScript/esbuild +// could still resolve it. This block also catches a browser-root-literal +// import (e.g. `/src/net/ch-client.js`) reaching back into SQL Browser +// source: the generic RULES loop above only inspects specifiers that start +// with '.', so a literal absolute-looking path would otherwise slip past +// Rule A undetected — everything that isn't a relative specifier is a +// violation here, with no exceptions. +const PACKAGE_SRC_DIR = path.join(repoRoot, 'packages/clickhouse-http/src'); +if (fs.existsSync(PACKAGE_SRC_DIR)) { + for (const file of collectFiles(PACKAGE_SRC_DIR)) { + const relFile = path.relative(repoRoot, file).split(path.sep).join('/'); + checkedFiles += 1; + const source = fs.readFileSync(file, 'utf8'); + for (const spec of extractSpecifiers(source)) { + if (spec.startsWith('.')) continue; // relative — governed by Rule A above + violations.push(`${relFile} → ${spec} (issue #630 Phase 2: clickhouse-http has zero bare package imports)`); + } + } +} + +// Issue #630 Phase 2 — Rule D: the public package name may be imported by +// bare specifier only under src/net/** — the existing network-layer +// boundary — so src/core, src/workspace, src/dashboard, src/application, or +// UI code cannot bypass that layer merely because the low-level HTTP +// mechanics moved behind a bare package name. The deep-import subpath form +// (`@altinity/clickhouse-http/...`) is forbidden EVERYWHERE under src/** — +// only the package's "." export is public (contract A4). +const CLICKHOUSE_HTTP_SPECIFIER = '@altinity/clickhouse-http'; +for (const file of collectFiles(path.join(repoRoot, 'src'))) { + const relFile = path.relative(repoRoot, file).split(path.sep).join('/'); + checkedFiles += 1; + const source = fs.readFileSync(file, 'utf8'); + for (const spec of extractSpecifiers(source)) { + if (spec === CLICKHOUSE_HTTP_SPECIFIER) { + if (!relFile.startsWith('src/net/')) { + violations.push(`${relFile} → ${spec} (issue #630 Phase 2: @altinity/clickhouse-http may only be imported under src/net/**)`); + } + continue; + } + if (spec.startsWith(`${CLICKHOUSE_HTTP_SPECIFIER}/`)) { + violations.push(`${relFile} → ${spec} (issue #630 Phase 2: @altinity/clickhouse-http exposes only its "." export — deep imports are forbidden everywhere)`); + } + } +} + if (violations.length) { console.error('check-boundaries: architecture violations:'); for (const line of violations) console.error(` ${line}`); diff --git a/build/size-report-lib.mjs b/build/size-report-lib.mjs index 15e287b8..179a2a51 100644 --- a/build/size-report-lib.mjs +++ b/build/size-report-lib.mjs @@ -26,6 +26,11 @@ export function normalizeInputPath(p) { // Attribute one input file to an ownership bucket per the issue's rules: // src/generated/** -> generated project code // src/** (everything else) -> hand-written project source +// packages/** -> first-party project source (issue #630 +// Phase 2 — the in-repository npm workspace +// is project code, not a third-party +// runtime dependency, even though esbuild +// resolves it through node_modules) // node_modules//** -> external, grouped under // node_modules/@scope// -> external, grouped under @scope/ // Nested deps (a/node_modules/b) attribute to the *leaf* package (b), which is the @@ -42,6 +47,7 @@ export function classifyInput(rawPath) { } if (path.startsWith('src/generated/')) return { owner: 'generated', group: 'src/generated' }; if (path.startsWith('src/')) return { owner: 'project', group: 'src' }; + if (path.startsWith('packages/')) return { owner: 'project', group: 'packages' }; return { owner: 'other', group: 'other' }; } diff --git a/packages/clickhouse-http/src/client.ts b/packages/clickhouse-http/src/client.ts new file mode 100644 index 00000000..d6153a33 --- /dev/null +++ b/packages/clickhouse-http/src/client.ts @@ -0,0 +1,72 @@ +// Issue #630 Phase 2 — the low-level package client: request construction + +// one injected Fetch invocation. Moved (behaviorally unchanged) from +// `createHttpTransport`'s `send()` in `src/net/clickhouse-http-transport.ts`, +// which now delegates here through this package's public export instead of +// building the URL / calling `deps.fetch()` itself. +// +// Scope discipline (Phase 2 plan §7): no retries, no response-status +// inspection, no body access, no Authorization parsing/storage, no +// controller/signal bridging. `request()` resolves to the exact native +// `Response` Fetch returns — never a clone or wrapper — and every accessor +// is read live, per request, never snapshotted at construction time. + +import { chUrl } from './url.js'; + +/** Accessor-shaped dependencies: both must be read live, per request — a + * live, mutable `origin`/`fetch` (e.g. the SQL Browser's `ChCtx`, mutated in + * place on sign-in) must always be observed at its CURRENT value, never a + * value pinned at construction time. */ +export interface ClickHouseHttpClientDeps { + fetch(): typeof fetch; + origin(): string; +} + +/** One ClickHouse HTTP request, fully specified. No client-level defaults: + * `authorization` is the complete header value (scheme + credential), + * resolved by the caller for THIS request. */ +export interface ClickHouseHttpRequest { + /** Opaque SQL text — never parsed, rewritten, or appended to. */ + sql: string; + /** Exact ClickHouse format name sent as `default_format`. */ + defaultFormat: string; + /** HTTP query-string settings. */ + settings?: Record; + /** Query-string params riding alongside (native `param_*`, `query_id`, + * `session_id`, `role`, …). */ + params?: Record; + /** Complete Authorization header value. Never optional, never defaulted. */ + authorization: string; + signal?: AbortSignal; +} + +export interface ClickHouseHttpClient { + /** POST one query; resolves with the NATIVE fetch `Response` (never a + * clone/wrapper) at HTTP settlement — HTTP error statuses resolve; only + * network I/O failure / abort rejects the returned promise natively. + * Exactly one Fetch invocation per call; no retry. + * + * `request` is `async` on purpose even though its body has no `await` + * before the network call: a synchronous serializer failure (`chUrl` + * throwing `URIError` on an unencodable value) must surface as a REJECTED + * promise, matching the compatibility adapter's existing async `send()` + * settlement shape — never a synchronous throw out of this function. */ + request(request: ClickHouseHttpRequest): Promise; +} + +export function createClickHouseHttpClient(deps: ClickHouseHttpClientDeps): ClickHouseHttpClient { + return { + async request(request: ClickHouseHttpRequest): Promise { + const url = chUrl(deps.origin(), { + format: request.defaultFormat, + extra: request.settings, + params: request.params, + }); + return deps.fetch()(url, { + method: 'POST', + body: request.sql, + headers: { Authorization: request.authorization }, + signal: request.signal, + }); + }, + }; +} diff --git a/packages/clickhouse-http/src/index.ts b/packages/clickhouse-http/src/index.ts index ffd71247..d4ae581f 100644 --- a/packages/clickhouse-http/src/index.ts +++ b/packages/clickhouse-http/src/index.ts @@ -1,7 +1,16 @@ -// Issue #630 Phase 2 — inert workspace scaffold (Checkpoint 1). This package -// is the repository's first npm workspace: the low-level ClickHouse HTTP -// URL/request mechanics move here from `src/net/clickhouse-http-transport.ts` -// in Checkpoint 2. Statement-free on purpose until then — package source -// lands under strict TypeScript + coverage only together with its -// implementation and tests (see the Phase 2 plan's migration order). -export {}; +// Issue #630 Phase 2 — the package's public surface. Only "." is exported +// (see `package.json`'s `exports` map): implementation subpaths (`./url`, +// `./client`) are not part of the public contract, and `build/check- +// boundaries.mjs` mechanically forbids any root SQL Browser source from +// deep-importing them. Every re-export below is a relative import, matching +// this package's zero-bare-specifier rule. + +export { chUrl } from './url.js'; +export { createClickHouseHttpClient } from './client.js'; + +export type { ChUrlOpts } from './url.js'; +export type { + ClickHouseHttpClientDeps, + ClickHouseHttpRequest, + ClickHouseHttpClient, +} from './client.js'; diff --git a/packages/clickhouse-http/src/url.ts b/packages/clickhouse-http/src/url.ts new file mode 100644 index 00000000..bdb0ecd1 --- /dev/null +++ b/packages/clickhouse-http/src/url.ts @@ -0,0 +1,37 @@ +// Issue #630 Phase 2 — the authoritative ClickHouse HTTP URL serializer. +// Moved mechanically (behaviorally unchanged) from +// `src/net/clickhouse-http-transport.ts` — see that file's Phase 2 comment. +// This is the ONE `chUrl()` implementation in the repository (contract A5); +// every other importer (the root SQL Browser `src/net/**` layer) reaches it +// only through this package's public `.` export, never by relative/deep +// import (enforced by `build/check-boundaries.mjs`). +// +// Zero bare-specifier imports: this file (and every file under +// `packages/clickhouse-http/src/**`) may only use relative imports — see +// this package's `README`-equivalent rule in the Phase 2 plan §5. There is +// nothing to import here, so that constraint is trivially satisfied, but the +// architecture checker still scans this file for the sabotage regression. + +/** `chUrl`'s query-string options. */ +export interface ChUrlOpts { + format?: string; + extra?: Record; + params?: Record; +} + +/** Build a ClickHouse HTTP URL with query-string options. Pure. Synchronous: + * a caller-supplied `extra`/`params` value that `encodeURIComponent` cannot + * encode (e.g. a lone UTF-16 surrogate) throws a `URIError` directly out of + * this call — `client.ts`'s `request()` wraps that in its own async + * settlement so the low-level API's promise rejects instead of throwing. */ +export function chUrl(origin: string, opts: ChUrlOpts = {}): string { + const format = opts.format || 'JSONStringsEachRowWithProgress'; + let url = origin + '?default_format=' + format + '&enable_http_compression=1'; + for (const [k, v] of Object.entries(opts.extra || {})) { + url += '&' + k + '=' + encodeURIComponent(v); + } + for (const [k, v] of Object.entries(opts.params || {})) { + url += '&' + k + '=' + encodeURIComponent(v); + } + return url; +} diff --git a/src/net/ch-client.ts b/src/net/ch-client.ts index 61f27e97..75c85b2e 100644 --- a/src/net/ch-client.ts +++ b/src/net/ch-client.ts @@ -20,10 +20,18 @@ import { sqlString } from '../core/format.js'; // `createHttpTransport`; this module keeps every auth/epoch/retry policy, // product operation, and `ChCtx` exactly as before, delegating through the // transport instead of calling `chUrl`/`ctx.fetch` directly. -import { chUrl, createHttpTransport } from './clickhouse-http-transport.js'; +// +// Issue #630 Phase 2 — `chUrl` now comes from `@altinity/clickhouse-http` +// (the package is the ONE serializer implementation, contract A5); this +// module's re-export below keeps every existing importer (including the +// historical official-client spike, `tests/spike/clickhouse-client/current- +// adapter.ts`) resolving unchanged. `createHttpTransport` stays imported +// from the local compatibility adapter — its composition graph is untouched. +import { chUrl } from '@altinity/clickhouse-http'; +import { createHttpTransport } from './clickhouse-http-transport.js'; import type { TransportRequest } from './clickhouse-transport.types.js'; export { chUrl }; -export type { ChUrlOpts } from './clickhouse-http-transport.js'; +export type { ChUrlOpts } from '@altinity/clickhouse-http'; export type { ClickHouseTransport, StreamCallbacks, TransportDeps, TransportRequest } from './clickhouse-transport.types.js'; // ── Injected ctx seam ──────────────────────────────────────────────────────── diff --git a/src/net/clickhouse-http-transport.ts b/src/net/clickhouse-http-transport.ts index 475719d0..ce968894 100644 --- a/src/net/clickhouse-http-transport.ts +++ b/src/net/clickhouse-http-transport.ts @@ -1,38 +1,22 @@ // Issue #585 Phase 1 — the current custom ClickHouse HTTP transport, // re-seated behind the `ClickHouseTransport` contract (`clickhouse-transport.types.ts`). -// This is a pure move: `chUrl` (+ its `ChUrlOpts` parameter type) and the -// progress-line stream-read loop are relocated here verbatim from -// `ch-client.ts`, which re-imports/re-exports both so every existing importer -// keeps resolving. No behavior change; no product SQL; no auth/lifecycle -// policy (that stays app-side in `ch-client.ts`'s `authedFetch`). +// Issue #630 Phase 2 — this file is now a temporary COMPATIBILITY ADAPTER: +// `chUrl`/`ChUrlOpts`, the URL construction, and the direct injected +// `fetch()` invocation moved to `@altinity/clickhouse-http` (mechanically, +// behaviorally unchanged — see that package's `url.ts`/`client.ts`). `send()` +// below delegates to the package's `request()` instead of building the +// request itself. `streamLines()` (the progress-bearing JSON-lines read +// loop) stays local until Phase 3 — stream decoding is explicitly deferred. // -// Ownership boundary: this file may depend only on `src/core` — never on +// Ownership boundary: this file may depend only on `src/core` and the +// `@altinity/clickhouse-http` public package export — never on // `ch-client.ts`, `oauth.ts`, `oauth-config.ts`, `src/application/`, or // `src/ui/`. `build/check-boundaries.mjs` enforces this mechanically. +import { createClickHouseHttpClient } from '@altinity/clickhouse-http'; import type { ClickHouseTransport, StreamCallbacks, TransportDeps, TransportRequest } from './clickhouse-transport.types.js'; import type { StreamLine } from '../core/stream.js'; -/** `chUrl`'s query-string options. */ -export interface ChUrlOpts { - format?: string; - extra?: Record; - params?: Record; -} - -/** Build a ClickHouse HTTP URL with query-string options. Pure. */ -export function chUrl(origin: string, opts: ChUrlOpts = {}): string { - const format = opts.format || 'JSONStringsEachRowWithProgress'; - let url = origin + '?default_format=' + format + '&enable_http_compression=1'; - for (const [k, v] of Object.entries(opts.extra || {})) { - url += '&' + k + '=' + encodeURIComponent(v); - } - for (const [k, v] of Object.entries(opts.params || {})) { - url += '&' + k + '=' + encodeURIComponent(v); - } - return url; -} - /** Drives the progress-bearing JSON-lines read loop: decode, line split, * `JSON.parse` per line, trailing-buffer flush, malformed-line skip — * byte-for-byte the loop formerly inlined in `runQuery`. `onLine` fires per @@ -77,19 +61,16 @@ async function streamLines(body: ReadableStream, cbs: StreamCallback * `chCtx`, mutated in place on sign-in) is always observed at its current * value, never pinned to a stale snapshot (Adaptation A5). */ export function createHttpTransport(deps: TransportDeps): ClickHouseTransport { + const client = createClickHouseHttpClient(deps); return { + // Kept `async` even though its body is a single delegating call: this + // exactly matches today's adapter-level settlement shape (a synchronous + // preparation error, e.g. a URIError from URL encoding, must surface as + // a REJECTED promise here too, never a synchronous throw out of + // `send()`), and makes the compatibility intent explicit rather than + // relying solely on the package implementation's own async-ness. async send(request: TransportRequest): Promise { - const url = chUrl(deps.origin(), { - format: request.defaultFormat, - extra: request.settings, - params: request.params, - }); - return deps.fetch()(url, { - method: 'POST', - body: request.sql, - headers: { Authorization: request.authorization }, - signal: request.signal, - }); + return client.request(request); }, streamLines, }; diff --git a/src/net/clickhouse-transport.types.ts b/src/net/clickhouse-transport.types.ts index 09827020..11386601 100644 --- a/src/net/clickhouse-transport.types.ts +++ b/src/net/clickhouse-transport.types.ts @@ -3,16 +3,24 @@ // `.types.ts` suffix rather than the issue's suggested `clickhouse-transport.ts` // — "exact names may follow repository conventions" per the issue). Puts the // CURRENT custom HTTP implementation behind a contract a future official -// transport (Phase 2, gated on a new decision — ADR-0005 is Rejected) could -// also satisfy, without moving any product SQL or auth/lifecycle policy here. +// transport (gated on a new decision — ADR-0005 is Rejected) could also +// satisfy, without moving any product SQL or auth/lifecycle policy here. +// +// Issue #630 Phase 2 — `TransportDeps`/`TransportRequest` are now ALIASES of +// the low-level request/dependency types owned by `@altinity/clickhouse-http` +// (see that package's `client.ts`), not separate shapes: the package is the +// single source of truth for the low-level request boundary. `ClickHouseTransport` +// itself stays here — Phase 2 still has the SQL-Browser-local `streamLines` +// method, deferred to Phase 3. // // Ownership boundary: this file (and its implementation, // `clickhouse-http-transport.ts`) may depend only on `src/core` (the narrow -// `StreamLine` type) — never on `ch-client.ts`, `oauth.ts`, `oauth-config.ts`, -// `src/application/`, or `src/ui/`, even type-only. `build/check-boundaries.mjs` -// enforces this mechanically (twin `RULES` entries for this file and the -// implementation file). +// `StreamLine` type) and the `@altinity/clickhouse-http` public package export +// — never on `ch-client.ts`, `oauth.ts`, `oauth-config.ts`, `src/application/`, +// or `src/ui/`, even type-only. `build/check-boundaries.mjs` enforces this +// mechanically (twin `RULES` entries for this file and the implementation file). +import type { ClickHouseHttpClientDeps, ClickHouseHttpRequest } from '@altinity/clickhouse-http'; import type { StreamLine } from '../core/stream.js'; /** What the transport is allowed to see of the environment. Deliberately @@ -31,33 +39,18 @@ import type { StreamLine } from '../core/stream.js'; * so — exactly like A6's single-send discipline — this rule is enforced by * this doc comment and review, not by the compiler or the existing epoch * race test (whose proof stops at the `send` invocation boundary). */ -export interface TransportDeps { - fetch(): typeof fetch; - origin(): string; -} +export type TransportDeps = ClickHouseHttpClientDeps; /** One ClickHouse HTTP request, fully specified. No client-level defaults * exist: `authorization` is the complete header value (scheme + credential), - * resolved by the caller (SQL Browser auth policy) for THIS request. */ -export interface TransportRequest { - /** Opaque SQL text. The transport never parses, rewrites, or appends to it - * (hard invariant 16: an authored FORMAT clause always wins over - * `defaultFormat` server-side, exactly as today). */ - sql: string; - /** Exact ClickHouse format name sent as `default_format`. */ - defaultFormat: string; - /** HTTP query-string settings (wait_end_of_query, max_result_rows, - * result_overflow_mode, add_http_cors_header, readonly, …) — the caller's - * policy decides which; the transport only serializes. */ - settings?: Record; - /** Query-string params riding alongside: native `param_*` parameters, - * `query_id`, `session_id`, `role` — today's exact wire vocabulary, - * unchanged (Adaptation A2). */ - params?: Record; - /** Complete Authorization header value. Never optional, never defaulted. */ - authorization: string; - signal?: AbortSignal; -} + * resolved by the caller (SQL Browser auth policy) for THIS request. + * + * Field-level docs (moved to `@altinity/clickhouse-http`'s `client.ts`): + * `sql` is opaque (never parsed/rewritten/appended to — hard invariant 16: + * an authored FORMAT clause always wins over `defaultFormat` server-side); + * `settings`/`params` are today's exact wire vocabulary, unchanged + * (Adaptation A2); `authorization` is never optional or defaulted. */ +export type TransportRequest = ClickHouseHttpRequest; // No TransportResponse type in Phase 1 (Adaptation A3): `send` resolves with // the NATIVE fetch `Response`. A structural subset would be assignable only in @@ -84,8 +77,15 @@ export interface ClickHouseTransport { * `streamToFile(resp: Response, …)` consumer without casts. Exactly one * fetch invocation (contract-suite-asserted, incl. on non-2xx — A6); no * retry, no token read, no lifecycle callback, no error classification, no - * body consumption. HTTP error statuses resolve (they are responses); only - * network I/O failure / abort rejects. */ + * body consumption. HTTP error statuses resolve normally (they are + * responses); network I/O failure / abort rejects the returned promise + * natively. Since #630 Phase 2, `send` is implemented by delegating to + * `@altinity/clickhouse-http`'s async `request()`, which itself builds the + * request URL — so a REQUEST-PREPARATION failure (e.g. a `URIError` from + * malformed `settings`/`params`) also surfaces as a rejected promise here, + * not a synchronous throw. The transport performs no error classification + * or wrapping of either failure kind — that policy distinction is made by + * the caller (`ch-client.ts`'s `authedFetch`), not here. */ send(request: TransportRequest): Promise; /** Supported-stream mechanics for the progress-bearing JSON-lines formats: * drives the read loop (decode, line split, JSON.parse, trailing-buffer diff --git a/tests/e2e/clickhouse-http-transport.html b/tests/e2e/clickhouse-http-transport.html index 26f61cc9..2af9b183 100644 --- a/tests/e2e/clickhouse-http-transport.html +++ b/tests/e2e/clickhouse-http-transport.html @@ -16,7 +16,17 @@ Native `Response` objects cannot cross Playwright's page.evaluate() serialization boundary, so every scenario below does its own identity assertion INSIDE the page realm and returns only plain, serializable - results (booleans/numbers/strings) to the Node-side spec. --> + results (booleans/numbers/strings) to the Node-side spec. + + #630 Phase 2 — the transport module now imports the bare + `@altinity/clickhouse-http` package specifier. build/e2e-serve.mjs is a + raw file server, not a bundler/resolver, so this import map resolves + that ONE bare specifier to the workspace source directly. --> +