From 27961b461d5095cb3869f4bca359d0e45a1e17f9 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Fri, 7 Aug 2026 17:42:04 +0200 Subject: [PATCH 1/6] feat(#630): move progress-stream and exception-parsing primitives into @altinity/clickhouse-http MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3 of the #630 extraction: streamLines() (the progress-bearing JSON-lines read loop), parseExceptionText(), and findExceptionFrame() (now byte-oriented, no caller-side latin1 conversion) move into the package alongside their StreamLine/StreamCallbacks wire types. This is a real move+delete, not an additive layer: src/net/clickhouse-http- transport.ts and clickhouse-transport.types.ts are now request/send- only, and src/core/stream.ts no longer declares the moved protocol types/primitives — applyStreamLine narrows an open Record parsed record instead of re-declaring a second wire type. runQuery calls the package's streamLines directly; export-service.ts's streamToFile calls the package's byte-oriented findExceptionFrame through ch-client.ts's zero-logic re-export gateway, deleting its own latin1 converter. StreamResult, row caps, percentages, raw/result presentation, editor-caret positioning, and auth-expiry/ denial UI policy all stay in SQL Browser, unmoved. build/check-boundaries.mjs gains a narrow legacy-owner rule rejecting the three former owners (the transport adapter, its type contract, and core/stream.ts) from regaining any moved identifier, mirrored in clickhouse-http-package-policy.test.js with sabotage probes and a checker-source drift binding. Tests move (not duplicate) across the package boundary: the stream-mechanics suite into clickhouse-http-progress-stream.test.ts, the exception-parsing suite into clickhouse-http-exceptions.test.ts (byte fixtures, plus new byte-boundary and false-positive coverage), and the shared transport contract suite drops its one stream case to become request/send-only. The real-browser Chromium/WebKit fault harness now streams through the package's streamLines directly instead of a transport method. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz --- .wiki/Decisions-and-Roadmap.md | 36 +++- .wiki/Source-Map.md | 8 +- CHANGELOG.md | 37 ++++ CLAUDE.md | 14 +- build/check-boundaries.mjs | 53 +++++ docs/ARCHITECTURE.md | 81 ++++--- packages/clickhouse-http/src/exceptions.ts | 101 +++++++++ packages/clickhouse-http/src/index.ts | 8 + .../clickhouse-http/src/progress-stream.ts | 96 +++++++++ src/application/export-service.ts | 24 ++- src/core/stream.ts | 134 +++--------- src/net/ch-client.ts | 45 +++- src/net/clickhouse-http-transport.ts | 54 +---- src/net/clickhouse-transport.types.ts | 38 ++-- tests/e2e/clickhouse-http-transport.html | 30 ++- tests/e2e/clickhouse-http-transport.spec.js | 2 +- .../clickhouse-client/browser-harness.ts | 7 +- .../clickhouse-client/current-adapter.ts | 12 +- .../clickhouse-client/progress-bridge.ts | 34 ++- tests/unit/ch-client.test.ts | 3 +- tests/unit/clickhouse-http-exceptions.test.ts | 200 ++++++++++++++++++ .../clickhouse-http-package-policy.test.js | 135 +++++++++++- tests/unit/clickhouse-http-package.test.ts | 22 +- .../clickhouse-http-progress-stream.test.ts | 171 +++++++++++++++ tests/unit/clickhouse-http-transport.test.ts | 123 +---------- tests/unit/clickhouse-transport-contract.ts | 19 +- tests/unit/export-service.test.ts | 81 +++++++ tests/unit/query-execution-service.test.ts | 28 +++ tests/unit/stream.test.ts | 95 +-------- 29 files changed, 1208 insertions(+), 483 deletions(-) create mode 100644 packages/clickhouse-http/src/exceptions.ts create mode 100644 packages/clickhouse-http/src/progress-stream.ts create mode 100644 tests/unit/clickhouse-http-exceptions.test.ts create mode 100644 tests/unit/clickhouse-http-progress-stream.test.ts diff --git a/.wiki/Decisions-and-Roadmap.md b/.wiki/Decisions-and-Roadmap.md index e21b012a..dabea795 100644 --- a/.wiki/Decisions-and-Roadmap.md +++ b/.wiki/Decisions-and-Roadmap.md @@ -179,14 +179,36 @@ Two roadmap tracks are current: own, exposing only its `.` export — and turns `src/net/clickhouse-http-transport.ts` into a temporary compatibility adapter whose `send()` delegates to the package's `request()`; - `streamLines()` stays local, deferred to a later phase. `ch-client.ts`'s - composition graph, auth/epoch/retry policy, and eager pre-credential - `chUrl` preflight are all unchanged. See + `streamLines()` stayed local at that point, deferred to Phase 3. + `ch-client.ts`'s composition graph, auth/epoch/retry policy, and eager + pre-credential `chUrl` preflight are all unchanged. **Phase 3** (merged) + moves the progress-bearing JSON-lines read loop (`streamLines`, plus the + canonical `StreamLine`/`StreamCallbacks`/`ProgressMetaColumn` wire types) + and the HTTP exception-text parser + byte-safe late-exception framer + (`parseExceptionText`, `findExceptionFrame`/`ExceptionFrame` — now + `Uint8Array`-in, no caller-side latin1 conversion) into the package too — + a real move+delete, not an additive compatibility layer: the transport + adapter and its type contract are now request/send-only, and + `core/stream.ts` no longer declares a second copy of the wire type. + `runQuery` (itself under `src/net/**`) calls the package's `streamLines` + directly rather than through the transport seam, since there is exactly + one production stream implementation now. SQL Browser keeps `StreamResult`, + row caps, percentages, raw/result presentation, editor-caret positioning, + and auth-expiry/denial UI policy exactly where they were — + `applyStreamLine` now narrows an open `Record` parsed + record instead of re-declaring the package's wire type. Still deferred to + later phases: `queryJson`/`queryText`/`queryProgress` convenience APIs, + `ensureClickHouseSuccess`, `ClickHouseError`, package `KILL QUERY`, SQL + quoting/type-grammar extraction, an authentication-composition rewrite, + and `runQuery`/`exportQuery`/the remaining request transport seam's own + eventual migration/deletion (Phase 7). See [[Source-Map]] and [[Architecture]] for the file-level detail and - `build/check-boundaries.mjs`'s Rules A–D for the mechanical boundary - enforcement (package↔root-src ban, package zero-bare-specifier ban, - root↔package-deep-import ban, bare-import location restricted to - `src/net/**`). + `build/check-boundaries.mjs`'s Rules A–D plus the Phase 3 narrow + legacy-owner rule for the mechanical boundary enforcement (package↔root-src + ban, package zero-bare-specifier ban, root↔package-deep-import ban, + bare-import location restricted to `src/net/**`, and the former + transport/contract/`core/stream.ts` owners rejected from regaining any + moved identifier). Re-read GitHub before acting because issue state can change; a MERGED PR is not proof its code is on `main` (see the reset above). diff --git a/.wiki/Source-Map.md b/.wiki/Source-Map.md index 48f39811..ddb7c4a3 100644 --- a/.wiki/Source-Map.md +++ b/.wiki/Source-Map.md @@ -16,10 +16,10 @@ Back to [[Home]]. Related: [[Architecture]], [[Product-and-Features]]. | `src/dashboard/application/dashboard-repaint-plan.js` | pure repaint-decision arbitration extracted from `ui/dashboard.js`'s `renderDashboard` effect (#589) | | `src/ui/dashboard-tile-gestures.js` | Dashboard corner-drag resize, Command/Ctrl-drag reorder, and modifier-cue controller, extracted from `ui/dashboard.js` behind an injected `TileGestureDeps` seam (#589) | | `src/state.js` | signals-backed state model and persistence operations | -| `src/net/ch-client.js` | ClickHouse HTTP execution and schema calls; auth/epoch/retry policy, product operations, `ChCtx` (#585 Phase 1: generic request/stream mechanics delegate through the transport seam below; #630 Phase 2: `chUrl` re-exported from `@altinity/clickhouse-http`) | -| `src/net/clickhouse-transport.types.js` | Type-only `ClickHouseTransport` contract (`send`/`streamLines`); `TransportDeps`/`TransportRequest` alias the package's own types (#585 Phase 1; #630 Phase 2) | -| `src/net/clickhouse-http-transport.js` | `createHttpTransport` — temporary compatibility adapter: `send()` delegates to `@altinity/clickhouse-http`'s `request()`; `streamLines()` (progress-bearing JSON-lines loop) stays local (#585 Phase 1; #630 Phase 2) | -| `packages/clickhouse-http/src/` | First-party npm workspace package (repo's first) — `url.ts` (`chUrl`, the ONE URL-serializer implementation), `client.ts` (`createClickHouseHttpClient`, the low-level request/Fetch invocation); public export only, zero runtime dependencies, zero bare-specifier imports, no SQL Browser `src/**` dependency (#630 Phase 2) | +| `src/net/ch-client.js` | ClickHouse HTTP execution and schema calls; auth/epoch/retry policy, product operations, `ChCtx` (#585 Phase 1: generic request/stream mechanics delegate through the transport seam below; #630 Phase 2: `chUrl` re-exported from `@altinity/clickhouse-http`; #630 Phase 3: `streamLines` called directly, `parseExceptionText`/`findExceptionFrame`/`StreamLine`/`StreamCallbacks` re-exported) | +| `src/net/clickhouse-transport.types.js` | Type-only `ClickHouseTransport` contract — `send()` ONLY since #630 Phase 3 (`streamLines`/`StreamCallbacks` moved to the package); `TransportDeps`/`TransportRequest` alias the package's own types (#585 Phase 1; #630 Phase 2) | +| `src/net/clickhouse-http-transport.js` | `createHttpTransport` — temporary compatibility adapter, REQUEST/SEND-ONLY since #630 Phase 3: `send()` delegates to `@altinity/clickhouse-http`'s `request()`; no stream member at all (`ch-client.ts`'s `runQuery` calls the package's `streamLines` directly instead) (#585 Phase 1; #630 Phases 2-3) | +| `packages/clickhouse-http/src/` | First-party npm workspace package (repo's first) — `url.ts` (`chUrl`, the ONE URL-serializer implementation), `client.ts` (`createClickHouseHttpClient`, the low-level request/Fetch invocation), `progress-stream.ts` (`streamLines`, the ONE progress-bearing JSON-lines read loop, plus the canonical `StreamLine`/`StreamCallbacks`/`ProgressMetaColumn` wire types), `exceptions.ts` (`parseExceptionText`, `findExceptionFrame`/`ExceptionFrame` — byte-oriented, no caller-side latin1 conversion) — public export only, zero runtime dependencies, zero bare-specifier imports, no SQL Browser `src/**` dependency (#630 Phase 2; progress-stream/exceptions since Phase 3) | | `src/net/oauth.js` | OAuth flow/token exchange | | `src/editor/editor-port.js` | SQL editor contract and safe no-op port | | `src/editor/codemirror-adapter.js` | SQL CodeMirror 6 adapter | diff --git a/CHANGELOG.md b/CHANGELOG.md index dbaf499d..28528aba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,43 @@ auto-generated per-PR notes; this file is the curated, human-readable history. ## [Unreleased] ### Added +- **#630 Phase 3: move the progress-stream read loop and HTTP exception + parsing/late-exception framing into `@altinity/clickhouse-http`.** + `packages/clickhouse-http` now owns the progress-bearing JSON-lines read + loop (`streamLines`, plus the canonical `StreamLine`/`StreamCallbacks`/ + `ProgressMetaColumn` wire types — `progress-stream.ts`) and the ClickHouse + HTTP exception-text parser + byte-safe late-exception framer + (`parseExceptionText`, `findExceptionFrame`/`ExceptionFrame` — + `exceptions.ts`); `findExceptionFrame` now takes the retained tail as a raw + `Uint8Array` directly rather than a caller-supplied latin1 surrogate + string. This is a real move+delete, not an additive compatibility layer: + `src/net/clickhouse-http-transport.ts` and `clickhouse-transport.types.ts` + are now request/send-only (no `streamLines`/`StreamCallbacks` member at + all); `src/core/stream.ts` no longer declares `StreamLine`, `splitBuffer`, + `parseExceptionText`, `ExceptionFrame`, or `findExceptionFrame`. `runQuery` + (`src/net/ch-client.ts`, itself under `src/net/**`) calls the package's + `streamLines` directly instead of through the transport seam; + `export-service.ts`'s `streamToFile` calls the package's byte-oriented + `findExceptionFrame` on its retained holdback directly, through + `ch-client.ts`'s zero-logic re-export gateway, deleting the caller-side + latin1 converter it used to carry. SQL Browser keeps everything downstream + of the wire exactly where it was: `StreamResult`, row caps, progress + percentages, in-band exception → `result.error` folding (`applyStreamLine`, + now narrowed to an open `Record` parsed-record boundary + rather than a second declared copy of the wire type), raw/result + presentation, editor-caret positioning (`parseErrorPos`), and + auth-expiry/denial UI policy (`isAuthExpiredBody`, `authDeniedMessage`). + `build/check-boundaries.mjs` gains a narrow legacy-owner regression rule + rejecting the three former owners from regaining any moved identifier, + mirrored in `tests/unit/clickhouse-http-package-policy.test.js` with its + own sabotage probes and a checker-source drift binding. The real-browser + Chromium/WebKit transport fault harness + (`tests/e2e/clickhouse-http-transport.{html,spec.js}`) now streams through + the package's `streamLines` directly instead of a transport method. This is + phase 3 of 8 (issue #630); consuming query APIs, SQL quoting/type grammar, + auth composition, and final migration/deletion of `runQuery`/`exportQuery`/ + the remaining request transport seam remain later phases. + - **#630 Phase 2: create `@altinity/clickhouse-http`, the repository's first npm workspace package, and move low-level request/URL mechanics into it.** `packages/clickhouse-http` now owns `chUrl()`/URL serialization and a diff --git a/CLAUDE.md b/CLAUDE.md index dd943055..0fb80fec 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -24,12 +24,14 @@ all bundled — see hard rule 4). Quality is held by tests. policy* (OAuth, `ChCtx`, auth/epoch/retry, product operations) goes in `src/net/`, with the fetch seam *injected*, never imported. Reusable, product-agnostic ClickHouse HTTP/Fetch mechanics (URL serialization, the - low-level request) may live in the first-party workspace package - `packages/clickhouse-http` (#630 Phase 2) instead — `src/net/**` is the - only place allowed to import it, by its exact public package name, never - a deep import into its `src/**`; the package itself may depend on - nothing under SQL Browser `src/**` and declares zero runtime - dependencies (mechanically enforced, `build/check-boundaries.mjs`). + low-level request, the progress-stream wire shape and its reader/decoder + loop, and HTTP exception-text/late-exception byte framing) may live in the + first-party workspace package `packages/clickhouse-http` (#630 Phase 2; + the progress-stream/exception primitives since Phase 3) instead — + `src/net/**` is the only place allowed to import it, by its exact public + package name, never a deep import into its `src/**`; the package itself + may depend on nothing under SQL Browser `src/**` and declares zero + runtime dependencies (mechanically enforced, `build/check-boundaries.mjs`). DOM rendering goes in `src/ui/` as functions that take the `app` controller — except the editor, which lives in `src/editor/` behind the injected editor seams (#143/#212): only `main.js` imports concrete adapters, diff --git a/build/check-boundaries.mjs b/build/check-boundaries.mjs index 5a3b92dd..67ad1aab 100644 --- a/build/check-boundaries.mjs +++ b/build/check-boundaries.mjs @@ -333,6 +333,59 @@ for (const file of collectFiles(path.join(repoRoot, 'src'))) { } } +// Issue #630 Phase 3 — narrow legacy-owner regression rule: the former +// production owners of the moved progress-stream/exception-parsing +// primitives must not regain them. This mechanically rejects re-adding a +// `streamLines` forwarding wrapper to the old transport adapter, a +// `StreamCallbacks`/`streamLines` member to the transport contract, or a +// `StreamLine`/`splitBuffer`/`parseExceptionText`/`ExceptionFrame`/ +// `findExceptionFrame` declaration to `core/stream.ts` — the "no duplicate +// stream/error implementation remains" contract this phase requires. +// Deliberately narrower than a repository-wide function-name ban (Phase 8 +// owns broader anti-regrowth hardening): this names exactly the three former +// owners and the exact identifiers Phase 3 moved out of them. +// `applyStreamLine` stays explicitly allowed (SQL Browser result policy, +// never moved) — the word-boundary match below cannot mistake it for +// `StreamLine` (no boundary between "apply" and "StreamLine", so `\bStreamLine\b` +// never matches inside it). +// +// Comments are stripped (naive block/line-comment regex, matching this +// file's existing regex-only, non-AST approach) before matching, so this +// rule flags only a real code re-declaration/re-import — never this phase's +// own doc comments narrating the move. +function stripComments(source) { + return source.replace(/\/\*[\s\S]*?\*\//g, '').replace(/\/\/.*$/gm, ''); +} +const PHASE3_LEGACY_OWNER_RULES = [ + { + file: 'src/net/clickhouse-http-transport.ts', + forbiddenWords: ['streamLines'], + why: 'issue #630 Phase 3: the transport adapter must not regain a streamLines implementation/member — the package is the one stream owner', + }, + { + file: 'src/net/clickhouse-transport.types.ts', + forbiddenWords: ['StreamCallbacks', 'streamLines'], + why: 'issue #630 Phase 3: the transport contract must not regain StreamCallbacks or a streamLines member', + }, + { + file: 'src/core/stream.ts', + forbiddenWords: ['StreamLine', 'splitBuffer', 'parseExceptionText', 'ExceptionFrame', 'findExceptionFrame'], + why: 'issue #630 Phase 3: core/stream.ts must not regain the package-owned StreamLine/splitBuffer/parseExceptionText/ExceptionFrame/findExceptionFrame declarations', + }, +]; +for (const rule of PHASE3_LEGACY_OWNER_RULES) { + const file = path.join(repoRoot, rule.file); + if (!fs.existsSync(file)) continue; + checkedFiles += 1; + const code = stripComments(fs.readFileSync(file, 'utf8')); + for (const word of rule.forbiddenWords) { + const re = new RegExp(`\\b${word}\\b`); + if (re.test(code)) { + violations.push(`${rule.file} → regained ${word} (${rule.why})`); + } + } +} + if (violations.length) { console.error('check-boundaries: architecture violations:'); for (const line of violations) console.error(` ${line}`); diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index a3eb651a..32cea925 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -206,32 +206,45 @@ folded via the pure `applyStreamLine`; a single automatic token refresh on 401/403/`token_verification_exception` (before `authConfirmed` flips, an auth failure signs out; after, it is a query error). -### Transport seam (#585 Phase 1) and the clickhouse-http package (#630 Phase 2) +### Transport seam (#585 Phase 1) and the clickhouse-http package (#630 Phases 2-3) Generic request construction and stream mechanics are split out behind a narrow contract: `net/clickhouse-transport.types.ts` declares -`ClickHouseTransport` (`send`/`streamLines`), and re-exports its -`TransportDeps`/`TransportRequest` types as aliases of +`ClickHouseTransport` (`send()` only, since Phase 3 — see below), and +re-exports its `TransportDeps`/`TransportRequest` types as aliases of `@altinity/clickhouse-http`'s own `ClickHouseHttpClientDeps`/ `ClickHouseHttpRequest`. `net/clickhouse-http-transport.ts`'s -`createHttpTransport` is now a temporary COMPATIBILITY ADAPTER: its `send()` -delegates to the package's `createClickHouseHttpClient(deps).request()` -instead of building the request itself; `streamLines()` (the -progress-bearing JSON-lines read loop) stays local, deferred to a later -phase. `chUrl`/`ChUrlOpts` and the low-level `request()`/Fetch invocation now -live in `packages/clickhouse-http` — the repository's first npm workspace -(private, zero runtime dependencies, zero bare-specifier imports in its own -source, no dependency on SQL Browser `src/**`) — and are exposed only -through its public `.` export; `ch-client.ts` re-exports `chUrl` from that -package unchanged for its own existing importers. `build/check-boundaries.mjs` -mechanically restricts the bare `@altinity/clickhouse-http` import to -`src/net/**` and bans any deep import into the package's `src/**` -implementation (from either side), so the network-layer boundary can't be -bypassed just because the low-level mechanics moved behind a package name. -`ch-client.ts` keeps every auth/epoch/retry/lifecycle policy (`authedFetch`), -product operation, and `ChCtx` exactly as before; a module-private -`transportFor(ctx)` delegates unconditionally to `createHttpTransport` — -`ChCtx` gained no field and there is no runtime transport switch. +`createHttpTransport` is now a temporary COMPATIBILITY ADAPTER, and is +REQUEST/SEND-ONLY: its `send()` delegates to the package's +`createClickHouseHttpClient(deps).request()` instead of building the request +itself. `chUrl`/`ChUrlOpts`, the low-level `request()`/Fetch invocation, the +progress-bearing JSON-lines read loop (`streamLines`, plus its +`StreamLine`/`StreamCallbacks`/`ProgressMetaColumn` wire types), and the HTTP +exception-text parser + byte-safe late-exception framer +(`parseExceptionText`, `findExceptionFrame`/`ExceptionFrame`) all live in +`packages/clickhouse-http` — the repository's first npm workspace (private, +zero runtime dependencies, zero bare-specifier imports in its own source, no +dependency on SQL Browser `src/**`) — exposed only through its public `.` +export. `ch-client.ts` re-exports `chUrl`/`parseExceptionText`/ +`findExceptionFrame`/`StreamLine`/`StreamCallbacks`/`ChUrlOpts` from that +package unchanged, as zero-logic migration plumbing for existing importers +(including `export-service.ts`'s `findExceptionFrame` use, since +`src/application/**` cannot import the package directly). `build/check- +boundaries.mjs` mechanically restricts the bare `@altinity/clickhouse-http` +import to `src/net/**`, bans any deep import into the package's `src/**` +implementation (from either side), and — since Phase 3 — mechanically +rejects the three former owners (the transport adapter, the transport +contract, `core/stream.ts`) regaining any of the identifiers moved out of +them, so the network-layer boundary can't be bypassed just because the +mechanics moved behind a package name, and no duplicate stream/exception +implementation can silently reappear. `ch-client.ts` keeps every +auth/epoch/retry/lifecycle policy (`authedFetch`), product operation, and +`ChCtx` exactly as before; a module-private `transportFor(ctx)` delegates +unconditionally to `createHttpTransport` for the request/send half — `ChCtx` +gained no field and there is no runtime transport switch. `runQuery` (itself +under `src/net/**`) calls the package's `streamLines` directly rather than +going through the transport seam, since there is exactly one production +stream implementation and no longer a stream member on the contract. `authedFetch` snapshots the caller's `settings`/`params` synchronously at entry, before its first await, calling the package's `chUrl` directly as an eager pre-credential preflight (a malformed value throws synchronously here, @@ -239,11 +252,27 @@ before any token read), as one centralized defense against a caller mutating those objects while a token/refresh await is pending — the low-level `request()`/`send()` API instead resolves this same failure as a REJECTED promise, since both remain `async`. A reusable contract-test-suite factory -(`tests/unit/clickhouse-transport-contract.ts`) registers against both the -package's own `request()` and the compatibility adapter; a future -official-client implementation (ADR-0005 is Rejected; that cutover does not -proceed without a new decision) would satisfy the same -`ClickHouseTransport` contract and reuse the same suite. +(`tests/unit/clickhouse-transport-contract.ts`) is now request/send-only and +registers against both the package's own `request()` and the compatibility +adapter; the progress-stream loop is tested once, directly against the +package's `streamLines` (`tests/unit/clickhouse-http-progress-stream.test.ts`), +and the exception parser/framer once, directly against the package +(`tests/unit/clickhouse-http-exceptions.test.ts`) — deliberately not +dual-registered, since there is intentionally only one production +implementation of each. A future official-client implementation (ADR-0005 is +Rejected; that cutover does not proceed without a new decision) would +satisfy the same `ClickHouseTransport` contract and reuse the same suite. + +SQL Browser still owns everything downstream of the wire: `core/stream.ts` +keeps `StreamResult`, row caps, progress/percentage folding, and in-band +exception → `result.error` (`applyStreamLine`, now narrowed to accept an open +`Record` parsed-record boundary rather than re-declaring a +second copy of the package's `StreamLine` wire type), plus editor-caret +(`parseErrorPos`) and auth-expiry/denial (`isAuthExpiredBody`, +`authDeniedMessage`) UI policy. `export-service.ts`'s `streamToFile` calls +the package's byte-oriented `findExceptionFrame` directly on its retained +`Uint8Array` holdback — the caller-side latin1 conversion it used to carry +is gone, since the package computes byte-exact offsets internally. ## Build diff --git a/packages/clickhouse-http/src/exceptions.ts b/packages/clickhouse-http/src/exceptions.ts new file mode 100644 index 00000000..623001ca --- /dev/null +++ b/packages/clickhouse-http/src/exceptions.ts @@ -0,0 +1,101 @@ +// Issue #630 Phase 3 — ClickHouse HTTP exception-text parsing and byte-safe +// late-exception framing, moved from `src/core/stream.ts` (mechanically +// unchanged — see that file's pre-Phase-3 history). This is the ONE +// production implementation of both primitives (contract A7). +// +// `findExceptionFrame`'s public signature changed from a caller-supplied +// latin1 SURROGATE STRING to a raw `Uint8Array` (plan §5.3): the latin1 +// byte<->char mapping this module needs to search byte-exact offsets now +// happens INSIDE this module (`latin1View`, below) instead of being the +// caller's responsibility — removing the workaround +// `src/application/export-service.ts` used to carry for exactly this reason. + +/** + * Pull the ClickHouse exception out of an error response body. CH emits one + * `{"exception": "..."}` line; fall back to the raw text if absent. + */ +export function parseExceptionText(text: string): string { + for (const line of text.split('\n')) { + if (line.startsWith('{"exception"')) { + try { + return JSON.parse(line).exception; + } catch { + break; + } + } + } + return text; +} + +const EXCEPTION_MARKER = '__exception__'; // ClickHouse WriteBufferFromHTTPServerResponse + +// A byte<->char VIEW (not a decode): 1 byte -> 1 char (latin1), used only to +// locate byte-exact offsets via string search. Unlike a real UTF-8 +// TextDecoder — which can collapse a multi-byte sequence into one code point +// or substitute U+FFFD for an invalid one, both of which break a 1:1 +// index<->byte correspondence — this mapping is bijective and lossless for +// every byte value 0-255, so a string index found through it IS the exact +// byte offset into the original array. The clean prefix therefore never +// passes through a real decode; only the matched candidate exception text +// is later re-encoded and run through an actual `TextDecoder` (`utf8`, +// below) to produce the human-readable message. +function latin1View(bytes: Uint8Array): string { + let s = ''; + for (const b of bytes) s += String.fromCharCode(b); + return s; +} + +// Re-decode a latin1 (1 byte -> 1 char) slice back into proper UTF-8 text — +// applied only to an already-located candidate exception-message substring, +// never to the clean prefix. +const utf8 = (latin1: string): string => new TextDecoder().decode(Uint8Array.from(latin1, (c) => c.charCodeAt(0))); + +/** `findExceptionFrame`'s successful-match shape — the decoded/trimmed + * message plus how many leading BYTES of the tail are real data. + * `cleanBytes` is an index into the supplied `Uint8Array` — never a string + * index, code-point count, UTF-16 code-unit count, or re-encoded length. */ +export interface ExceptionFrame { + message: string; + cleanBytes: number; +} + +/** + * Find ClickHouse's mid-stream exception frame in the retained tail of a + * streamed HTTP response. Once headers (HTTP 200) are sent, a later server-side + * failure can't change the status — so ClickHouse (since v24.11) appends a + * structured frame to the very end of the body instead: + * \r\n__exception__\r\n\r\n\n \r\n__exception__\r\n + * `tag` is the 16-byte value ClickHouse ALSO sends up front in the + * `X-ClickHouse-Exception-Tag` response header — read it from the response and + * pass it here, so a server-chosen random tag (never present in real data by + * accident) frames the match with zero false positives. `tailBytes` is the + * retained tail of the body, as raw bytes — every offset this function + * computes and returns is a byte index into exactly this array; the caller + * needs no pre-conversion. + * + * Legacy fallback (`tag` falsy — servers < 24.11 send no tag header): scan for + * the plain-text `\nCode: . DB::Exception:` prefix instead (less precise + * excision, but still detected + reported). Anchored to the *end* of the tail + * (optionally one trailing newline) — a genuine unframed exception is always + * the last thing ClickHouse writes, and anchoring avoids misidentifying real + * exported data that happens to *contain* that text (e.g. a `system.query_log` + * `exception` column) as a server failure, so long as more data follows it. + * + * Returns `{ message, cleanBytes }` (`cleanBytes` = the byte length of real + * data before the frame — what the caller should keep) or `null` when the + * tail carries no exception frame. Pure. + */ +export function findExceptionFrame(tailBytes: Uint8Array | null | undefined, tag: string | null | undefined): ExceptionFrame | null { + const s = latin1View(tailBytes || new Uint8Array(0)); + if (tag) { + const open = '\r\n' + EXCEPTION_MARKER + '\r\n' + tag + '\r\n'; + const start = s.indexOf(open); + if (start < 0) return null; + const body = s.slice(start + open.length); + const close = body.indexOf('\r\n' + EXCEPTION_MARKER + '\r\n'); // closing trailer + const raw = close < 0 ? body : body.slice(0, body.lastIndexOf('\n', close - 1)); + return { message: utf8(raw).trim(), cleanBytes: start }; + } + const m = /\nCode:\s*\d+\.\s*DB::Exception:[^\n]*\n?$/.exec(s); + return m ? { message: utf8(m[0]).trim(), cleanBytes: m.index } : null; +} diff --git a/packages/clickhouse-http/src/index.ts b/packages/clickhouse-http/src/index.ts index d4ae581f..dce379af 100644 --- a/packages/clickhouse-http/src/index.ts +++ b/packages/clickhouse-http/src/index.ts @@ -4,9 +4,15 @@ // 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. +// +// Issue #630 Phase 3 — adds the progress-stream read loop and the HTTP +// exception-text/late-exception-frame parser (`progress-stream.ts`, +// `exceptions.ts`), same public-surface-only discipline. export { chUrl } from './url.js'; export { createClickHouseHttpClient } from './client.js'; +export { streamLines } from './progress-stream.js'; +export { parseExceptionText, findExceptionFrame } from './exceptions.js'; export type { ChUrlOpts } from './url.js'; export type { @@ -14,3 +20,5 @@ export type { ClickHouseHttpRequest, ClickHouseHttpClient, } from './client.js'; +export type { ProgressMetaColumn, StreamLine, StreamCallbacks } from './progress-stream.js'; +export type { ExceptionFrame } from './exceptions.js'; diff --git a/packages/clickhouse-http/src/progress-stream.ts b/packages/clickhouse-http/src/progress-stream.ts new file mode 100644 index 00000000..cf92f502 --- /dev/null +++ b/packages/clickhouse-http/src/progress-stream.ts @@ -0,0 +1,96 @@ +// Issue #630 Phase 3 — the canonical progress-bearing JSON-lines read loop +// (ClickHouse's JSONStringsEachRowWithProgress / JSONEachRowWithProgress wire +// format), moved verbatim from `src/net/clickhouse-http-transport.ts`'s +// former `streamLines()` (mechanically/behaviorally unchanged — see that +// file's Phase 2/3 comments). This is the ONE production implementation of +// the read loop (contract A6): decode, line split, JSON.parse, trailing- +// buffer flush, malformed-line skip, one `onLine` per parsed object, one +// `onChunk` per network chunk. +// +// Scope discipline (Phase 3 plan §5.2): no result normalization (no +// StreamResult/row-caps/percentages — that stays SQL-Browser-owned in +// `src/core/stream.ts`'s `applyStreamLine`), no wrapping of a reader +// rejection (the exact rejection object must escape), no package-owned +// AbortController/derived signal/cancellation registry, no fatal decoder, no +// extra flush call beyond what the original loop already performed. + +/** One column header, as reported by a `{meta}` line. Left open + * (`[key: string]: unknown`) — this is the wire shape, not a SQL Browser + * result-policy type. */ +export interface ProgressMetaColumn { + name: string; + type: string; + [key: string]: unknown; +} + +/** One line of ClickHouse's progress-bearing JSON-lines wire format. A + * parsed line carries at most one of `meta`/`row`/`progress`/`exception` — + * callers narrow the particular shape they need; this type only describes + * what the wire can send. */ +export interface StreamLine { + meta?: ProgressMetaColumn[]; + row?: Record; + progress?: { + total_rows_to_read?: unknown; + read_rows?: unknown; + read_bytes?: unknown; + elapsed_ns?: unknown; + }; + exception?: string; + [key: string]: unknown; +} + +/** Callbacks driving `streamLines`' read loop. `onLine` fires synchronously, + * in order, for every successfully parsed complete line; `onChunk` fires + * exactly once per successful `reader.read()` chunk, after every `onLine` + * call that chunk produced. */ +export interface StreamCallbacks { + onLine?: (line: StreamLine) => void; + onChunk?: () => void; +} + +/** + * Drive the progress-bearing JSON-lines read loop over `body`: decode, line + * split, `JSON.parse` per line, trailing-buffer flush, malformed-line skip — + * byte-for-byte the loop formerly inlined in `runQuery` (Phase 1) and then + * moved to `createHttpTransport` (Phase 1/2) before this move. `onLine` fires + * per parsed object; `onChunk` fires once per network chunk, after that + * chunk's `onLine` calls. A single `TextDecoder` used with `{ stream: true }` + * for the whole body (not per-chunk) so a multi-byte UTF-8 character split + * across two byte chunks still decodes correctly. + * + * A reader rejection (including an aborted signal's `AbortError`) propagates + * unmodified — this function performs no try/catch around `reader.read()`, + * so the exact rejection object the native reader produced is what rejects + * this promise, and the loop simply stops (no further callbacks). + */ +export async function streamLines(body: ReadableStream, cbs: StreamCallbacks): Promise { + const reader = body.getReader(); + const decoder = new TextDecoder(); + let buffer = ''; + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + buffer += decoder.decode(value, { stream: true }); + const lines = buffer.split('\n'); + buffer = lines[lines.length - 1]; + for (const line of lines.slice(0, -1)) { + if (!line) continue; + let json: StreamLine; + try { + json = JSON.parse(line); + } catch { + continue; + } + cbs.onLine && cbs.onLine(json); + } + cbs.onChunk && cbs.onChunk(); + } + if (buffer.trim()) { + try { + cbs.onLine && cbs.onLine(JSON.parse(buffer)); + } catch { + /* trailing partial line */ + } + } +} diff --git a/src/application/export-service.ts b/src/application/export-service.ts index d1e9a804..41255aea 100644 --- a/src/application/export-service.ts +++ b/src/application/export-service.ts @@ -53,7 +53,14 @@ import { mergedSourceArgs } from '../core/param-pipeline.js'; import type { PreparedSource } from '../core/param-pipeline.js'; import { prepareExportSql, isSchemaMutatingSql } from '../core/format.js'; import { formatFileMeta, exportFilename, scriptExportName } from '../core/export.js'; -import { findExceptionFrame } from '../core/stream.js'; +// Issue #630 Phase 3 — `findExceptionFrame` is package-owned +// (`@altinity/clickhouse-http`) and now takes raw bytes directly (no more +// caller-side latin1 conversion — see the deleted `latin1()` helper this +// file used to carry). `src/application/**` cannot import the package +// directly (Rule D), so this goes through `ch-client.ts`'s zero-logic +// re-export, the same gateway this file already depends on for `exportQuery`/ +// `runQuery`/`killQuery`. +import { findExceptionFrame } from '../net/ch-client.js'; import type { QueryTab } from '../state.js'; import { variableDoc } from '../state.js'; import type { ResultSort } from '../core/sort.js'; @@ -236,14 +243,6 @@ export interface ExportService { cancelExportScript(): void; } -// A latin1 decode (1 char per byte) for byte-accurate exception-frame slicing -// — pure, no injected deps. -const latin1 = (bytes: Uint8Array): string => { - let s = ''; - for (const b of bytes) s += String.fromCharCode(b); - return s; -}; - /** Build an `ExportService` bound to `deps`. Trivial constructor — no * validation, no defaulting; the caller supplies every field exactly as it * wants it used. */ @@ -466,8 +465,11 @@ export function createExportService(deps: ExportServiceDeps): ExportService { } held = merged.subarray(commit); } - // EOF: inspect the retained tail (latin1: 1 char per byte, for byte-accurate slicing). - const frame = findExceptionFrame(latin1(held), tag); + // EOF: inspect the retained tail. `findExceptionFrame` (package-owned, + // #630 Phase 3) takes the raw bytes directly — no caller-side latin1 + // conversion is needed any more; the package computes byte-exact + // offsets internally. + const frame = findExceptionFrame(held, tag); const clean = frame ? held.subarray(0, frame.cleanBytes) : held; if (signal.aborted) throw new DOMException('aborted', 'AbortError'); if (clean.length) { diff --git a/src/core/stream.ts b/src/core/stream.ts index b21a44a3..a171d071 100644 --- a/src/core/stream.ts +++ b/src/core/stream.ts @@ -6,6 +6,22 @@ // { exception: "..." } — server-side error // `applyStreamLine` folds one parsed object into a mutable result; keeping it // pure (no fetch, no DOM) makes the streaming parser fully unit-testable. +// +// Issue #630 Phase 3 — the canonical progress-line wire type (`StreamLine`) +// and the generic stream/exception-parsing primitives (`splitBuffer`, +// `parseExceptionText`, `ExceptionFrame`, `findExceptionFrame`) moved to +// `@altinity/clickhouse-http` (`progress-stream.ts`/`exceptions.ts`) — they +// are protocol mechanics, not SQL Browser result policy. `applyStreamLine` +// stays here (SQL Browser owns row caps, progress/percentage folding, and +// in-band exception -> result.error), but now accepts an open parsed-record +// boundary (`Record`) rather than re-declaring a second +// copy of the package's wire-shape interface — see this module's doc on +// `applyStreamLine` below for why. `parseErrorPos`/`isAuthExpiredBody`/ +// `authDeniedMessage` (editor caret + auth-expiry/denial UI policy) stay +// here unchanged; `src/core/**` cannot import `src/net/**` or the package +// (`build/check-boundaries.mjs` Rule for `src/core`), so this module never +// imports the package type either — it narrows the fields it needs from the +// open record instead. /** One streamed result column, as reported by a `{meta}` line. */ export interface StreamColumn { @@ -61,27 +77,23 @@ export function newResult(fmt: string, rowLimit = 0): StreamResult { }; } -/** One line of the streaming format — see the module doc above for the four - * shapes a line can take; unrecognized lines (no known key) are a no-op. */ -export interface StreamLine { - meta?: StreamColumn[]; - row?: Record; - progress?: { - total_rows_to_read?: unknown; - read_rows?: unknown; - read_bytes?: unknown; - elapsed_ns?: unknown; - }; - exception?: string; - [k: string]: unknown; -} - -/** Fold one parsed stream object into `result` (mutated in place). */ -export function applyStreamLine(json: StreamLine, result: StreamResult): StreamResult { +/** + * Fold one parsed progress-stream record into `result` (mutated in place). + * `json` is deliberately typed as an open `Record` boundary + * rather than a re-declared structural mirror of the package's `StreamLine` + * (issue #630 Phase 3 §8.1) — the canonical progress-line wire shape is + * package-owned (`@altinity/clickhouse-http`'s `StreamLine`); this module + * only needs to recognize the four property names a consumer must interpret + * (`meta`/`row`/`progress`/`exception`), narrowing each locally, without + * re-exporting a second declared wire contract. Unrecognized records are a + * no-op — the module doc above lists the four shapes a line can take. + */ +export function applyStreamLine(json: Record, result: StreamResult): StreamResult { if (json.meta) { - result.columns = json.meta.map((m) => ({ name: m.name, type: m.type })); + const meta = json.meta as { name: string; type: string }[]; + result.columns = meta.map((m) => ({ name: m.name, type: m.type })); } else if (json.row) { - const row = json.row; + const row = json.row as Record; // At the cap: drop the row (block-boundary overage from `break`) and flag it. if (result.rowLimit > 0 && result.rows.length >= result.rowLimit) { result.capped = true; @@ -89,7 +101,7 @@ export function applyStreamLine(json: StreamLine, result: StreamResult): StreamR result.rows.push(result.columns.map((c) => row[c.name])); } } else if (json.progress) { - const p = json.progress; + const p = json.progress as Record; const total = Number(p.total_rows_to_read) || 0; const read = Number(p.read_rows) || 0; result.progress = { @@ -100,91 +112,11 @@ export function applyStreamLine(json: StreamLine, result: StreamResult): StreamR }; result.pct = total > 0 ? Math.min(100, (read / total) * 100) : 0; } else if (json.exception) { - result.error = json.exception; + result.error = json.exception as string; } return result; } -/** - * Split a streaming text buffer into complete lines plus the trailing - * remainder. Returns { lines, rest } where `rest` is the (possibly partial) - * last line to carry into the next chunk. - */ -export function splitBuffer(buffer: string): { lines: string[]; rest: string } { - const lines = buffer.split('\n'); - const rest = lines[lines.length - 1]; - return { lines: lines.slice(0, -1).filter((l) => l !== ''), rest }; -} - -/** - * Pull the ClickHouse exception out of an error response body. CH emits one - * `{"exception": "..."}` line; fall back to the raw text if absent. - */ -export function parseExceptionText(text: string): string { - for (const line of text.split('\n')) { - if (line.startsWith('{"exception"')) { - try { - return JSON.parse(line).exception; - } catch { - break; - } - } - } - return text; -} - -const EXCEPTION_MARKER = '__exception__'; // ClickHouse WriteBufferFromHTTPServerResponse - -// Re-decode a latin1 (1 byte -> 1 char) slice back into proper UTF-8 text. -const utf8 = (latin1: string): string => new TextDecoder().decode(Uint8Array.from(latin1, (c) => c.charCodeAt(0))); - -/** `findExceptionFrame`'s successful-match shape — the decoded/trimmed - * message plus how many leading bytes of the tail are real data. */ -export interface ExceptionFrame { - message: string; - cleanBytes: number; -} - -/** - * Find ClickHouse's mid-stream exception frame in the retained tail of a - * streamed HTTP response. Once headers (HTTP 200) are sent, a later server-side - * failure can't change the status — so ClickHouse (since v24.11) appends a - * structured frame to the very end of the body instead: - * \r\n__exception__\r\n\r\n\n \r\n__exception__\r\n - * `tag` is the 16-byte value ClickHouse ALSO sends up front in the - * `X-ClickHouse-Exception-Tag` response header — read it from the response and - * pass it here, so a server-chosen random tag (never present in real data by - * accident) frames the match with zero false positives. `tailLatin1` is the - * retained tail of the body decoded 1 byte -> 1 char (so a char index is a byte - * offset, even though the message itself may be UTF-8 multibyte). - * - * Legacy fallback (`tag` falsy — servers < 24.11 send no tag header): scan for - * the plain-text `\nCode: . DB::Exception:` prefix instead (less precise - * excision, but still detected + reported). Anchored to the *end* of the tail - * (optionally one trailing newline) — a genuine unframed exception is always - * the last thing ClickHouse writes, and anchoring avoids misidentifying real - * exported data that happens to *contain* that text (e.g. a `system.query_log` - * `exception` column) as a server failure, so long as more data follows it. - * - * Returns `{ message, cleanBytes }` (`cleanBytes` = the byte length of real - * data before the frame — what the caller should keep) or `null` when the - * tail carries no exception frame. Pure. - */ -export function findExceptionFrame(tailLatin1: string | null | undefined, tag: string | null | undefined): ExceptionFrame | null { - const s = String(tailLatin1 || ''); - if (tag) { - const open = '\r\n' + EXCEPTION_MARKER + '\r\n' + tag + '\r\n'; - const start = s.indexOf(open); - if (start < 0) return null; - const body = s.slice(start + open.length); - const close = body.indexOf('\r\n' + EXCEPTION_MARKER + '\r\n'); // closing trailer - const raw = close < 0 ? body : body.slice(0, body.lastIndexOf('\n', close - 1)); - return { message: utf8(raw).trim(), cleanBytes: start }; - } - const m = /\nCode:\s*\d+\.\s*DB::Exception:[^\n]*\n?$/.exec(s); - return m ? { message: utf8(m[0]).trim(), cleanBytes: m.index } : null; -} - /** * The 0-based caret offset a ClickHouse error points at, or null. CH syntax * errors carry "failed at position N (token): …" where N is 1-based and relative diff --git a/src/net/ch-client.ts b/src/net/ch-client.ts index 75c85b2e..e702307a 100644 --- a/src/net/ch-client.ts +++ b/src/net/ch-client.ts @@ -7,8 +7,7 @@ // onSignedOut() } // so the whole module is unit-testable with plain stubs. -import { parseExceptionText, isAuthExpiredBody, authDeniedMessage } from '../core/stream.js'; -import type { StreamLine } from '../core/stream.js'; +import { isAuthExpiredBody, authDeniedMessage } from '../core/stream.js'; import { parseAstTables, buildSchemaGraph, externalDbs } from '../core/schema-graph.js'; import type { SchemaGraphTableRow, SchemaGraphDictRow } from '../core/schema-graph.js'; import { sqlString } from '../core/format.js'; @@ -16,10 +15,10 @@ import { sqlString } from '../core/format.js'; // `clickhouse-http-transport.ts`; re-exported here (with its `ChUrlOpts` // parameter type) so every existing importer — including // `tests/spike/clickhouse-client/current-adapter.ts` — keeps resolving. The -// generic request-construction/fetch/stream mechanics live in -// `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. +// generic request-construction/fetch mechanics live in `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. // // Issue #630 Phase 2 — `chUrl` now comes from `@altinity/clickhouse-http` // (the package is the ONE serializer implementation, contract A5); this @@ -27,12 +26,30 @@ import { sqlString } from '../core/format.js'; // 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'; +// +// Issue #630 Phase 3 — the progress-stream read loop and the HTTP +// exception-text/late-exception-frame parser are also package-owned now +// (`streamLines`/`parseExceptionText`/`findExceptionFrame`, plus the +// canonical `StreamLine`/`StreamCallbacks` wire types). `runQuery` calls +// package `streamLines` directly (it is itself under `src/net/**`, so no +// seam violation) instead of going through `transportFor(ctx)` for the +// stream half — `transportFor(ctx)` remains used by `authedFetch`'s/ +// `killQueryWithLease`'s request/send paths, which Phase 7 eventually +// retires. `parseExceptionText`/`findExceptionFrame`/`StreamLine`/ +// `StreamCallbacks` are re-exported below as zero-logic migration plumbing: +// `src/core/**`/`src/application/**` cannot import the package directly +// (Rule D), so `export-service.ts`'s `findExceptionFrame` use and this +// module's own callers of the removed root `core/stream.js` exports resolve +// through this one gateway instead. +import { + chUrl, streamLines, parseExceptionText, findExceptionFrame, +} from '@altinity/clickhouse-http'; +import type { StreamLine } 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 '@altinity/clickhouse-http'; -export type { ClickHouseTransport, StreamCallbacks, TransportDeps, TransportRequest } from './clickhouse-transport.types.js'; +export { chUrl, parseExceptionText, findExceptionFrame }; +export type { ChUrlOpts, StreamLine, StreamCallbacks } from '@altinity/clickhouse-http'; +export type { ClickHouseTransport, TransportDeps, TransportRequest } from './clickhouse-transport.types.js'; // ── Injected ctx seam ──────────────────────────────────────────────────────── @@ -1110,6 +1127,12 @@ export async function runQuery(ctx: ChCtx, sql: string, o: RunQueryOptions = {}) if (!isStreaming) { return { raw: await resp.text() }; } - await transportFor(ctx).streamLines(resp.body!, { onLine: o.onLine, onChunk: o.onChunk }); + // Issue #630 Phase 3 — calls the package's `streamLines` directly rather + // than `transportFor(ctx).streamLines(...)`: this module is itself under + // `src/net/**` (the one layer allowed to import the package by bare + // specifier), and the transport seam no longer has a stream member at all + // (there is exactly one production stream implementation now — the + // package's). + await streamLines(resp.body!, { onLine: o.onLine, onChunk: o.onChunk }); return { streamed: true }; } diff --git a/src/net/clickhouse-http-transport.ts b/src/net/clickhouse-http-transport.ts index ce968894..5d2e96da 100644 --- a/src/net/clickhouse-http-transport.ts +++ b/src/net/clickhouse-http-transport.ts @@ -5,8 +5,16 @@ // `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. +// request itself. +// +// Issue #630 Phase 3 — this file is now SEND-ONLY. `streamLines()` (the +// progress-bearing JSON-lines read loop) moved to +// `@altinity/clickhouse-http`'s own `streamLines` — `ch-client.ts`'s +// `runQuery` (itself under `src/net/**`) calls the package function +// directly instead of going through this transport seam, so this adapter no +// longer has (or forwards to) a stream member at all. There is exactly one +// production stream implementation in the repository now — the package's; +// this file does not reintroduce a second one, forwarding or otherwise. // // Ownership boundary: this file may depend only on `src/core` and the // `@altinity/clickhouse-http` public package export — never on @@ -14,46 +22,7 @@ // `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'; - -/** 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 - * parsed object, `onChunk` once per network chunk. A single `TextDecoder` - * used with `{ stream: true }` for the whole body (not per-chunk) so a - * multi-byte UTF-8 character split across two byte chunks still decodes - * correctly. */ -async function streamLines(body: ReadableStream, cbs: StreamCallbacks): Promise { - const reader = body.getReader(); - const decoder = new TextDecoder(); - let buffer = ''; - for (;;) { - const { done, value } = await reader.read(); - if (done) break; - buffer += decoder.decode(value, { stream: true }); - const lines = buffer.split('\n'); - buffer = lines[lines.length - 1]; - for (const line of lines.slice(0, -1)) { - if (!line) continue; - let json: StreamLine; - try { - json = JSON.parse(line); - } catch { - continue; - } - cbs.onLine && cbs.onLine(json); - } - cbs.onChunk && cbs.onChunk(); - } - if (buffer.trim()) { - try { - cbs.onLine && cbs.onLine(JSON.parse(buffer)); - } catch { - /* trailing partial line */ - } - } -} +import type { ClickHouseTransport, TransportDeps, TransportRequest } from './clickhouse-transport.types.js'; /** The current custom HTTP implementation of `ClickHouseTransport`. `deps`' * accessors are read per-request (REQUIRED-PURE — see the contract's doc @@ -72,6 +41,5 @@ export function createHttpTransport(deps: TransportDeps): ClickHouseTransport { async send(request: TransportRequest): Promise { return client.request(request); }, - streamLines, }; } diff --git a/src/net/clickhouse-transport.types.ts b/src/net/clickhouse-transport.types.ts index 11386601..efe783c9 100644 --- a/src/net/clickhouse-transport.types.ts +++ b/src/net/clickhouse-transport.types.ts @@ -10,18 +10,27 @@ // 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` +// itself stays here — Phase 2 still had the SQL-Browser-local `streamLines` // method, deferred to Phase 3. // +// Issue #630 Phase 3 — `streamLines`/`StreamCallbacks` are GONE from this +// contract: the progress-bearing JSON-lines read loop and its callback shape +// are now package-owned (`@altinity/clickhouse-http`'s `streamLines`/ +// `StreamCallbacks`), consumed directly by `ch-client.ts`'s `runQuery` +// (which is itself under `src/net/**`) rather than through this transport +// seam. `ClickHouseTransport` is now a REQUEST/SEND-ONLY compatibility +// adapter contract — `send()` is its only member. There is exactly one +// stream implementation in the repository (the package's); this seam no +// longer describes one. +// // Ownership boundary: this file (and its implementation, -// `clickhouse-http-transport.ts`) may depend only on `src/core` (the narrow -// `StreamLine` type) and the `@altinity/clickhouse-http` public package export -// — never on `ch-client.ts`, `oauth.ts`, `oauth-config.ts`, `src/application/`, +// `clickhouse-http-transport.ts`) 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/`, 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 * excludes tokens, refresh, epochs, lifecycle callbacks: a transport @@ -59,14 +68,10 @@ export type TransportRequest = ClickHouseHttpRequest; // gives raw bytes (`body`, hard invariant 17) and `clone()` for authedFetch's // non-destructive error-body peek for free. -/** Callbacks driving `streamLines`' progress-bearing JSON-lines read loop. */ -export interface StreamCallbacks { - onLine?: (line: StreamLine) => void; - onChunk?: () => void; -} - -/** The SQL Browser transport contract. In Phase 1 exactly one implementation - * exists (`createHttpTransport`, `clickhouse-http-transport.ts`); a Phase 2 +/** The SQL Browser transport contract. Since #630 Phase 3, request/send is + * the ONLY thing this contract describes — see the module doc above for why + * `streamLines` is gone. In Phase 1 exactly one implementation exists + * (`createHttpTransport`, `clickhouse-http-transport.ts`); a Phase 2 * official-client implementation (does not proceed without a new decision) * would satisfy the same contract. */ export interface ClickHouseTransport { @@ -87,11 +92,4 @@ export interface ClickHouseTransport { * 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 - * flush, malformed-line skip), invoking onLine per parsed object and - * onChunk per network chunk — byte-for-byte the loop currently inlined in - * runQuery. Consuming a body is a caller decision made AFTER policy has - * classified the settled response. */ - streamLines(body: ReadableStream, cbs: StreamCallbacks): Promise; } diff --git a/tests/e2e/clickhouse-http-transport.html b/tests/e2e/clickhouse-http-transport.html index 2af9b183..90914b8d 100644 --- a/tests/e2e/clickhouse-http-transport.html +++ b/tests/e2e/clickhouse-http-transport.html @@ -21,7 +21,14 @@ #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. --> + that ONE bare specifier to the workspace source directly. + + #630 Phase 3 — Scenario 6 and Scenario 8 below no longer call + `transport.streamLines()` (the transport adapter no longer has a + stream member at all): they import the package's own `streamLines` + directly, exercising the SAME real production call path + `ch-client.ts`'s `runQuery` uses — no additional import-map entry is + needed, since the specifier is already mapped above. -->