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/.wiki/Decisions-and-Roadmap.md b/.wiki/Decisions-and-Roadmap.md index 0d86f847..e21b012a 100644 --- a/.wiki/Decisions-and-Roadmap.md +++ b/.wiki/Decisions-and-Roadmap.md @@ -164,6 +164,30 @@ Two roadmap tracks are current: fetch, or a deliberate renegotiation of the transport contract's cancellation semantics themselves. +- **#630 — extract the SQL Browser's own Fetch-native transport mechanics + into a first-party package.** Independent of the #585/ADR-0005 track above + (ADR-0005 rejects the *third-party* `@clickhouse/client-web` client; #630 + extracts SQL Browser's *own* hand-rolled, already-proven-correct + mechanics, and does not reopen or depend on ADR-0005's decision either + way). Phase 1 (merged, PR #640) froze the current `createHttpTransport`'s + native Fetch/Response/cancellation semantics as a real-browser + characterization suite, with no production code change — the behavioral + baseline Phase 2 is not allowed to alter. **Phase 2** moves `chUrl`/URL + serialization and the low-level injected-`fetch()` request into a new + `packages/clickhouse-http` — the repository's first npm workspace, + private, zero runtime dependencies, zero bare-specifier imports of its + 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 + [[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/**`). + 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 23845600..48f39811 100644 --- a/.wiki/Source-Map.md +++ b/.wiki/Source-Map.md @@ -16,9 +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) | -| `src/net/clickhouse-transport.types.js` | Type-only `ClickHouseTransport` contract (`send`/`streamLines`, `TransportDeps`, `TransportRequest`) (#585 Phase 1) | -| `src/net/clickhouse-http-transport.js` | `createHttpTransport` — the current custom HTTP implementation of that contract, plus `chUrl`/`ChUrlOpts` (#585 Phase 1) | +| `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/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 77383d10..dbaf499d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,38 @@ auto-generated per-PR notes; this file is the curated, human-readable history. ## [Unreleased] ### Added +- **#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 + low-level `createClickHouseHttpClient(deps).request(request)` — a direct, + branch-free `native fetch()` passthrough preserving exact SQL/Authorization, + live `origin()`/`fetch()` accessors, the caller's original `AbortSignal`, + and native `Response` identity (all nine Phase-1 invariants now pass + directly against the package's own `request()`, not only through the + compatibility path). `src/net/clickhouse-http-transport.ts`'s `send()` + becomes a thin delegating adapter to the package; `streamLines()` and all + SQL Browser auth/epoch/lifecycle/retry policy remain unchanged and local. + `ch-client.ts`'s composition graph (`ChCtx -> transportFor -> createHttpTransport + -> send`) is untouched; its eager pre-credential `chUrl()` validation now + calls the package's serializer. Root `package.json` gains a `workspaces` + entry and an explicit `@altinity/clickhouse-http` dependency; + `build/check-boundaries.mjs` gains four new architecture rules (package + source may not import root `src/**`; root may not deep-import package + internals; package source has zero bare import specifiers — closing a gap + the existing bare-specifier-skipping rule loop would otherwise leave open + for any hoisted root dependency; the bare `@altinity/clickhouse-http` + specifier itself is permitted only under `src/net/**`), each mirrored as a + coverage-gated unit test with a non-vacuous sabotage probe. + `Dockerfile` now copies `packages/` before `npm ci`; CI's `unit`/`build`/ + `bundle`/`e2e`/`docker` path classifications include `packages/**`. The + existing exact-literal `chUrl()` test suite moved (not duplicated) into the + new package test; the older duplicate in `ch-client.test.ts` is removed. + Bundle-size attribution now classifies `packages/**` as first-party project + code rather than `other`. Eight raw-ESM e2e fixtures that reach the + transport gained an `@altinity/clickhouse-http` import-map entry. This is + phase 2 of 8 (issue #630); consuming query APIs, SQL quoting/type grammar, + auth composition, and final migration/deletion remain later phases. + - **#630 Phase 1: characterize native Fetch/Response/cancellation semantics ahead of the `@altinity/clickhouse-http` extraction.** No production behavior changed — `src/net/clickhouse-http-transport.ts`, diff --git a/CLAUDE.md b/CLAUDE.md index ab7ada29..dd943055 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -20,8 +20,17 @@ all bundled — see hard rule 4). Quality is held by tests. layout, and application code goes in `src/dashboard/`, with dependency direction `model/layouts <- application <- UI`. App-level coordination and sessions go in `src/application/` and must not import `src/ui/` or - `src/editor/`. Network goes in `src/net/` with the fetch seam *injected*, - never imported. DOM rendering goes in `src/ui/` as functions that take the + `src/editor/`. SQL Browser's network *integration and application + 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`). + 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, and everything else addresses `app.sqlEditor` or `app.specEditor` explicitly. @@ -65,7 +74,12 @@ all bundled — see hard rule 4). Quality is held by tests. fail-closed policy: images/raw HTML/rejected links render as literal text; measured +44 KB raw / ~3% artifact delta) — all inlined into the artifact, so the page loads no runtime libraries from third-party CDNs. - Adding *another* runtime dependency is a deliberate decision (it grows the + `packages/clickhouse-http` (#630 Phase 2, the repository's first npm + workspace) is **project source, not an eighth bundled runtime + dependency**: it is private, ships no `dependencies`, and esbuild bundles + it exactly like hand-written `src/**` — `build/size-report-lib.mjs` + attributes it to the `project` ownership bucket, not `external`. Adding + *another* runtime dependency is a deliberate decision (it grows the single served file) — don't do it casually. When a feature needs a library, keep the testable logic pure in `src/core/` (chart axis/role/pivot math in `src/core/chart-data.js`; DOT→positions in `src/core/dot-layout.js`, both @@ -120,6 +134,7 @@ Touch these in one change: |---|---| | `src/core/*` | pure logic, 100% covered | | `src/net/*` | OAuth + ClickHouse client, injected fetch | +| `packages/clickhouse-http/src/*` | first-party npm workspace (repo's first, #630 Phase 2) — `chUrl`/URL serialization and the low-level injected-`fetch()` request, behind a public `.` export only; importable from `src/net/**` alone | | `src/application/*` | app-level coordination, sessions, and pure projections; no UI/editor imports | | `src/workspace/*` | pure stored-workspace aggregate, persistence contracts, and mutations | | `src/dashboard/*` | Dashboard model, layouts, and application runtime; dependency direction is mechanically checked | 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/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/docs/ADR-0005-clickhouse-web-client.md b/docs/ADR-0005-clickhouse-web-client.md index 029b922c..9ec226ae 100644 --- a/docs/ADR-0005-clickhouse-web-client.md +++ b/docs/ADR-0005-clickhouse-web-client.md @@ -1072,7 +1072,39 @@ review passes patching the same interception design are not expected to produce a different outcome; this addendum is the record of why the `/ship` run stopped attempting them. -## Reproduction commands +### #630 Phase 2 extraction addendum (2026-08-07) + +**This addendum does not reopen or otherwise touch the Rejected decision +above.** `@clickhouse/client-web` remains rejected for production adoption +for exactly the reasons the "Phase 2 cancellation-incompatibility addendum" +records; nothing here revisits that evidence. + +Issue #630 Phase 2 extracts the SQL Browser's OWN hand-rolled Fetch-native +request mechanics — `chUrl`'s URL serialization and the direct injected +`fetch()` invocation, both already proven correct by this ADR's own Phase 0 +evidence and by the Phase 1 seam this ADR's "Phase 1 addendum" describes — +out of `src/net/clickhouse-http-transport.ts` and into a new first-party, +in-repository npm workspace package, `packages/clickhouse-http`. This is a +pure internal reorganization of code this ADR already characterized as +"the current custom transport... remains authoritative": no third-party +HTTP client is introduced, no cancellation semantics change (the package's +`request()` still passes the caller's own `AbortSignal` directly into the +real `fetch()` for the response's whole lifetime — exactly the property +whose ABSENCE from `@clickhouse/client-web@1.23.1` is this ADR's own +rejection reason), and `src/net/clickhouse-http-transport.ts` becomes a thin +compatibility adapter delegating to the package rather than building +requests itself. `streamLines()` (the progress-bearing JSON-lines read loop) +stays local to SQL Browser, deferred to a later phase. + +The point of the extraction is reuse across a hypothetical future non-bundled +consumer of this exact Fetch-native request boundary — not a step back +toward `@clickhouse/client-web` or any other vendor client, and not itself a +reopening of Phases 2–4 above (those remain gated on a new decision, per the +Rejected verdict). Standalone package build/declaration/npm-pack +stabilization and any external-repository extraction are out of this +phase's scope, tracked separately by the issue's Phase 8. + + ```sh # spike-only test suite (does not run under normal `npm test`) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 90a8a2a1..a3eb651a 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -10,7 +10,8 @@ route-scoped sessions behind a small composition root. ``` core/ pure logic (no DOM, no globals, no imports from other layers) -net/ integration: OAuth + the ClickHouse HTTP client (fetch injected via ctx) +net/ integration: OAuth + the ClickHouse HTTP client (fetch injected via ctx); + low-level URL/request mechanics delegate to packages/clickhouse-http application/ route-agnostic services & sessions (no App, no DOM, no ui/editor imports) workspace/ pure stored-workspace aggregate, persistence contracts, and mutations dashboard/ Dashboard model/layouts/application runtime (model/layouts <- application <- UI) @@ -31,6 +32,12 @@ Dependency direction is strictly downward. Enforced mechanically by - `src/ui/workbench/**` and `src/ui/dashboard/**` never import each other, never import the editor (dashboard), and never import `src/ui/app.ts` — shells receive everything injected. +- `packages/clickhouse-http/src/**` never imports SQL Browser `src/**` and + has zero bare-specifier imports of its own (an empty allowlist — root + hoists many runtime dependencies the package must not resolve + undeclared); `src/**` never deep-imports the package's own `src/**`, and + its bare `@altinity/clickhouse-http` import (never a deep subpath) is + restricted to `src/net/**` (#630 Phase 2). Two known, deliberate exceptions predate #276 and are out of its scope: `core/saved-io.ts` imports a type from `editor/spec-editor.types.js`, and @@ -199,27 +206,44 @@ 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) +### Transport seam (#585 Phase 1) and the clickhouse-http package (#630 Phase 2) Generic request construction and stream mechanics are split out behind a narrow contract: `net/clickhouse-transport.types.ts` declares -`ClickHouseTransport` (`send`/`streamLines`), `TransportDeps` (`fetch`/ -`origin` accessors — read live per request, never snapshotted, since the -live `chCtx.origin` is mutated in place on sign-in), and `TransportRequest`. -`net/clickhouse-http-transport.ts`'s `createHttpTransport` is the current -custom HTTP implementation of that contract — `chUrl`/`ChUrlOpts` live there -now, re-exported unchanged from `ch-client.ts`. `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. `authedFetch` snapshots the caller's -`settings`/`params` synchronously at entry, before its first await, as one -centralized defense against a caller mutating those objects while a -token/refresh await is pending. A reusable contract-test-suite factory -(`tests/unit/clickhouse-transport-contract.ts`) registers against this one -implementation; a future official-client implementation (ADR-0005 is -Rejected; Phases 2–4 do not proceed without a new decision) would satisfy the -same contract and reuse the same suite. +`ClickHouseTransport` (`send`/`streamLines`), 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. +`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, +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. ## Build @@ -227,4 +251,12 @@ same contract and reuse the same suite. `styles.css` into `build/template.html` → a single `dist/sql.html`. Seven bundled runtime dependencies (CodeMirror 6, Chart.js + chartjs-adapter-date-fns + date-fns, dagre, `@preact/signals-core`, marked); -none is loaded from a third-party CDN. +none is loaded from a third-party CDN. `packages/clickhouse-http` (#630 +Phase 2, the repository's first npm workspace) is first-party project +source, not an eighth runtime dependency — esbuild resolves its bare +`@altinity/clickhouse-http` import through the workspace's `node_modules` +symlink and bundles it as ordinary source; `build/size-report-lib.mjs` +attributes every `packages/**` input to the `project` ownership bucket +accordingly, and the Dockerfile's build stage copies `packages/` alongside +`src/` before `npm ci && npm run build` so container/release builds resolve +it identically. 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/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 new file mode 100644 index 00000000..d4ae581f --- /dev/null +++ b/packages/clickhouse-http/src/index.ts @@ -0,0 +1,16 @@ +// 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. --> +