diff --git a/.wiki/Architecture.md b/.wiki/Architecture.md index a911dffd..bf8da99c 100644 --- a/.wiki/Architecture.md +++ b/.wiki/Architecture.md @@ -51,11 +51,17 @@ module mocking. ## Query path 1. The editor/controller prepares SQL and typed parameters. -2. `src/net/ch-client.js` sends the HTTP request with injected auth/fetch context, - delegating generic request construction and stream mechanics through a narrow - transport contract (`src/net/clickhouse-transport.types.js` + - `src/net/clickhouse-http-transport.js`, #585 Phase 1) — auth/epoch/retry policy - stays in `ch-client.js`. +2. `src/net/ch-client.js`'s exported `queryJson`/`runQuery`/`exportQuery` send the + HTTP request through `src/net/authenticated-clickhouse-request.js` (#630 + Phase 6), which owns auth/epoch/retry/lifecycle policy (moved out of + `ch-client.js`'s former `authedFetch`/`transportFor(ctx)`, deleted outright) + and builds the `@altinity/clickhouse-http` package client directly, composing + it with the package's response consumers; the callers keep their own + product-level result/error handling. The narrow transport contract + (`src/net/clickhouse-transport.types.js` + `src/net/clickhouse-http-transport.js`, + #585 Phase 1) is no longer the ordinary path — it now remains only as the + frozen-lease `killQueryWithLease` bypass's compatibility route, through + Phase 6; Phase 7 is expected to retire it. 3. `JSONStringsEachRowWithProgress` is folded line by line by pure stream logic. 4. Results resolve through the panel registry to table, chart, logs, KPI, filter, text, or graph-oriented renderers. diff --git a/.wiki/Decisions-and-Roadmap.md b/.wiki/Decisions-and-Roadmap.md index 5ce218e7..b7c540a4 100644 --- a/.wiki/Decisions-and-Roadmap.md +++ b/.wiki/Decisions-and-Roadmap.md @@ -244,11 +244,46 @@ Two roadmap tracks are current: underlying generic mechanics changed owner, and the existing parser/ helper bodies moved rather than being redesigned. This required revising the architecture boundary itself (see below) since SQL Browser language - consumers now legitimately import the package outside `src/net/**`. Still - deferred to later phases: an authentication-composition rewrite (Phase 6), - and `runQuery`/`exportQuery`/the remaining request transport seam's own - eventual migration/deletion plus the Phase-4 consuming query APIs' actual - cutover (Phase 7). See + consumers now legitimately import the package outside `src/net/**`. + + **Phase 6** (merged) composes SQL Browser authentication through one + new module, `src/net/authenticated-clickhouse-request.ts` — a real + move+delete of the normal-request auth/epoch/refresh/lifecycle policy + that used to live in `ch-client.ts` as `authedFetch()`/a module-private + `transportFor(ctx)`: both are gone, with no forwarding alias, no second + retry loop, and no second Authorization constructor. The new module + builds the package client directly + (`createClickHouseHttpClient(...).request()`) rather than through the + compatibility transport adapter, and exposes `authenticatedRequest()` + (the moved trust-boundary loop) plus `authenticatedJson()`/ + `authenticatedText()`/`authenticatedProgress()`, each composing it with + exactly one matching package response consumer. `ChCtx` now `extends` + the new module's narrower `AuthenticatedRequestCtx` instead of + redeclaring its fields, adding only `dataLakeCatalogSettingUnsupported`. + `queryJson()` is the first real production consumer of the package's + JSON response consumer, translating the package's `ClickHouseError` + back to `queryJson`'s existing plain-`Error` compatibility shape (same + parsed message); `runQuery()`/`exportQuery()` switch only their + `authedFetch()` call to the new raw `authenticatedRequest()` entrypoint, + keeping their own result/error/body handling unchanged. + `killQueryWithLease()`'s frozen-lease bypass is untouched — it already + built its own one-shot transport directly from the frozen lease, never + through `ChCtx`, so it does not route through the new mutable-context + auth loop. `build/check-boundaries.mjs`'s two existing #585 + transport-leaf forbidden lists and the #512 `connectionAuthorityFiles` + lifecycle-authority list now name the new module too (a data extension + of existing rules, not a new scanner); `ch-client.ts` stays in those + lists through Phase 7. Real-browser coverage: authenticated-path + variants of the existing post-header cancellation scenarios 5-9 + (`tests/e2e/clickhouse-http-transport.{html,spec.js}`), proving the + identical native Fetch/Response/cancellation semantics survive being + driven through a real, production-shaped `AuthenticatedRequestCtx` + (synthetic test credentials, one deterministic epoch) in both Chromium + and WebKit. Still deferred to **Phase 7**: `runQuery`/`exportQuery`'s + cutover onto the package's convenience consuming query APIs and their + own result/export ownership migration, the remaining + `killQuery`/`killQueryWithLease` transport migration, and deletion of + the now-superseded transport-adapter compatibility seam. See [[Source-Map]] and [[Architecture]] for the file-level detail and `build/check-boundaries.mjs`'s Rules A–D plus the Phase 3/5 narrow legacy-owner rules for the mechanical boundary enforcement: package↔root-src diff --git a/.wiki/Source-Map.md b/.wiki/Source-Map.md index f9e73cb8..b1d3201f 100644 --- a/.wiki/Source-Map.md +++ b/.wiki/Source-Map.md @@ -16,10 +16,11 @@ 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`; #630 Phase 3: `streamLines` called directly, `parseExceptionText`/`findExceptionFrame`/`StreamLine`/`StreamCallbacks` re-exported; #630 Phase 4: unaffected — the package's new consuming query APIs/`killQuery` are additive and not yet consumed here; #630 Phase 5: `sqlString` also imported directly from the package, replacing the retired `../core/format.js` import) | -| `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, plus #630 Phase 4's `queryJson`/`queryText`/`queryProgress` convenience methods and stateless `killQuery` — since #630 Phase 5, `killQuery` quotes through this package's own `sql-quote.ts` `sqlString`, and the Phase-4 private `quoteKillQueryId` stopgap is gone), `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 — plus #630 Phase 4's minimal `ClickHouseError`), `response.ts` (#630 Phase 4, new — `ensureClickHouseSuccess`, `consumeJsonResponse`/`consumeTextResponse`/`consumeProgressResponse`), and — new in #630 Phase 5 — `sql-quote.ts` (`sqlString`/`quoteIdent`/`qualifyIdent`, the ONE ClickHouse SQL-quoting implementation, moved verbatim from `src/core/format.ts`), `clickhouse-type.ts` (`parseClickHouseType`/`analyzeTypeModifiers`/`canonicalType`/the wrapper+enum helpers, the ONE generic type-expression grammar, moved verbatim from `src/core/clickhouse-type.ts` minus SQL Browser's `isSupportedOptionScalar` policy, which stayed at `src/core/param-type.ts`), `sql-spans.ts` (`scanSpans`/`Span`/`SpanKind`, the ONE shared lexical scanner, re-exported because surviving SQL Browser SQL-analysis modules still need it, moved verbatim from `src/core/sql-spans.ts`), and package-private `quoted-span.ts` (`scanDelimited`, moved verbatim from `src/core/quoted-span.ts`, not re-exported) — public export only, zero runtime dependencies, zero bare-specifier imports, no SQL Browser `src/**` dependency (#630 Phase 2; progress-stream/exceptions since Phase 3; response/query/kill APIs since Phase 4 — additive, not yet consumed by any `src/**` caller; SQL quoting/type grammar/scanner since Phase 5 — real production consumers retargeted). Bare package access is now two categories: transport/protocol APIs stay `src/net/**`-only; the pure-language exports above (quoting, type grammar, scanner) may be imported by their real SQL Browser consumers anywhere outside `src/net/**` too (mechanically allowlisted, `build/check-boundaries.mjs` Rule D) | +| `src/net/ch-client.js` | ClickHouse HTTP execution and schema calls; 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; #630 Phase 4: unaffected — the package's new consuming query APIs/`killQuery` are additive and not yet consumed here; #630 Phase 5: `sqlString` also imported directly from the package, replacing the retired `../core/format.js` import; #630 Phase 6: auth/epoch/retry/lifecycle policy (`authedFetch`/`transportFor(ctx)`) MOVED to `authenticated-clickhouse-request.js` below — `ch-client.js` is now the product/query/export COMPATIBILITY owner: `ChCtx` `extends AuthenticatedRequestCtx` and adds only `dataLakeCatalogSettingUnsupported`; `queryJson()` delegates to `authenticatedJson()` with a `ClickHouseError`→`Error` compatibility translation; `runQuery`/`exportQuery` call the new module's raw `authenticatedRequest()`, keeping their own result/error/body handling; `killQueryWithLease`'s frozen-lease bypass is untouched) | +| `src/net/authenticated-clickhouse-request.js` | **New in #630 Phase 6.** The sole normal-request auth/epoch/refresh/lifecycle owner: `authenticatedRequest()` (the moved `authedFetch` trust-boundary loop, now building the package's `createClickHouseHttpClient(...).request()` directly instead of going through the compatibility transport) plus `authenticatedJson()`/`authenticatedText()`/`authenticatedProgress()`, each composing it with exactly one matching package response consumer (`consumeJsonResponse`/`consumeTextResponse`/`consumeProgressResponse`). Declares the narrow `AuthenticatedRequestCtx` seam `ch-client.js`'s `ChCtx` now extends. Named in `build/check-boundaries.mjs`'s #585 transport-leaf forbidden lists and the #512 `connectionAuthorityFiles` lifecycle-authority list | +| `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). Since #630 Phase 6, its one remaining production caller is `killQueryWithLease`'s frozen-lease bypass — the normal-request path moved to `authenticated-clickhouse-request.js`, which builds the package client directly | +| `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). Since #630 Phase 6, its one remaining production caller is `killQueryWithLease` | +| `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, plus #630 Phase 4's `queryJson`/`queryText`/`queryProgress` convenience methods and stateless `killQuery` — since #630 Phase 5, `killQuery` quotes through this package's own `sql-quote.ts` `sqlString`, and the Phase-4 private `quoteKillQueryId` stopgap is gone), `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 — plus #630 Phase 4's minimal `ClickHouseError`), `response.ts` (#630 Phase 4, new — `ensureClickHouseSuccess`, `consumeJsonResponse`/`consumeTextResponse`/`consumeProgressResponse`), and — new in #630 Phase 5 — `sql-quote.ts` (`sqlString`/`quoteIdent`/`qualifyIdent`, the ONE ClickHouse SQL-quoting implementation, moved verbatim from `src/core/format.ts`), `clickhouse-type.ts` (`parseClickHouseType`/`analyzeTypeModifiers`/`canonicalType`/the wrapper+enum helpers, the ONE generic type-expression grammar, moved verbatim from `src/core/clickhouse-type.ts` minus SQL Browser's `isSupportedOptionScalar` policy, which stayed at `src/core/param-type.ts`), `sql-spans.ts` (`scanSpans`/`Span`/`SpanKind`, the ONE shared lexical scanner, re-exported because surviving SQL Browser SQL-analysis modules still need it, moved verbatim from `src/core/sql-spans.ts`), and package-private `quoted-span.ts` (`scanDelimited`, moved verbatim from `src/core/quoted-span.ts`, not re-exported) — public export only, zero runtime dependencies, zero bare-specifier imports, no SQL Browser `src/**` dependency (#630 Phase 2; progress-stream/exceptions since Phase 3; response/query/kill APIs since Phase 4 — additive, not consumed by any `src/**` caller until Phase 6; SQL quoting/type grammar/scanner since Phase 5 — real production consumers retargeted). Since #630 Phase 6, `src/net/authenticated-clickhouse-request.js` is the first real `src/**` consumer of `request()` plus the non-consuming classifier/JSON/text/progress consumers — the convenience `queryJson`/`queryText`/`queryProgress` client methods themselves still have no `src/**` consumer (Phase 7). Bare package access is now two categories: transport/protocol APIs stay `src/net/**`-only; the pure-language exports above (quoting, type grammar, scanner) may be imported by their real SQL Browser consumers anywhere outside `src/net/**` too (mechanically allowlisted, `build/check-boundaries.mjs` Rule D) | | `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 2bd3551d..27ea2f28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,67 @@ auto-generated per-PR notes; this file is the curated, human-readable history. ## [Unreleased] ### Added +- **#630 Phase 6: compose SQL Browser authentication through one + `authenticated-clickhouse-request.ts` layer over the package's + `request()` and response consumers.** The normal-request auth/epoch/ + refresh/lifecycle policy that used to live in `src/net/ch-client.ts` as + `authedFetch()`/a module-private `transportFor(ctx)` moves to a new + module, `src/net/authenticated-clickhouse-request.ts` — a real move+ + delete, not an additive layer: both are gone from `ch-client.ts`, with + no forwarding alias, no second retry loop, and no second Authorization + constructor. The new module builds the `@altinity/clickhouse-http` + package client directly (`createClickHouseHttpClient(...).request()`) + instead of going through the compatibility transport adapter, and + exposes `authenticatedRequest()` (the moved trust-boundary loop) plus + `authenticatedJson()`/`authenticatedText()`/`authenticatedProgress()`, + each composing it with exactly one matching package response consumer + (`consumeJsonResponse`/`consumeTextResponse`/`consumeProgressResponse`). + `ChCtx` now `extends` the new module's narrower `AuthenticatedRequestCtx` + instead of redeclaring its fields, adding only + `dataLakeCatalogSettingUnsupported` — the one field genuinely specific + to the product client. `AuthenticatedCancellationLease` stays exported + from `ch-client.ts`, and `killQueryWithLease`'s frozen-lease bypass is + untouched: it already built its own one-shot transport directly from + the frozen lease, never through `ChCtx`, so it does not route through + the new mutable-context auth loop (hard invariant 8/13). + + `queryJson()` is the first real production consumer of the package's + response-consumer layer: it now delegates to `authenticatedJson()`, + translating the package's `ClickHouseError` back to `queryJson`'s + EXISTING plain-`Error` compatibility shape (same parsed message) so this + phase adopts the new consumer without changing an existing SQL Browser + API. `runQuery()`/`exportQuery()` switch only their `authedFetch()` call + to the new raw `authenticatedRequest()` entrypoint, keeping their own + Table/KPI/raw format mapping, row-cap settings, non-2xx parsing, and + streaming exactly as before — their full package-consumer/result/export + cutover remains Phase 7, as does `authenticatedText()`/ + `authenticatedProgress()`'s adoption by any other caller. + + `build/check-boundaries.mjs`'s two existing #585 transport-leaf + forbidden lists (`clickhouse-http-transport.ts`, + `clickhouse-transport.types.ts`) and the #512 `connectionAuthorityFiles` + lifecycle-authority list now name the new module as the current auth/ + lifecycle owner they must not reach/regain — a data extension of + existing rules, not a new scanner. `ch-client.ts` stays in the + transport-leaf forbidden lists too through Phase 7. + + Real-browser coverage: `tests/e2e/clickhouse-http-transport.{html,spec.js}` + gains authenticated-path variants of the existing post-header + cancellation scenarios 5-9, driving `authenticatedRequest()`/ + `authenticatedProgress()` through a real, production-shaped + `AuthenticatedRequestCtx` (synthetic test credentials, one deterministic + epoch) against the same real cross-origin fault server — proving the + identical native Fetch/Response/cancellation semantics survive SQL + Browser's own credential/epoch composition, in both Chromium and WebKit, + not just the compatibility transport/package client with an + already-resolved Authorization. + + Only A12 (one authenticated request owner over the package) and A13 + (epoch/refresh/lifecycle/cancellation invariants remain regression- + tested and unchanged) are newly claimed; A14-A18 (the remaining + `runQuery`/`exportQuery`/transport-seam migration and deletion) stay + deferred to Phase 7. + - **#630 Phase 5: move ClickHouse SQL quoting and generic type-expression grammar into `@altinity/clickhouse-http`.** `sqlString`, `quoteIdent`, and `qualifyIdent` now have one package implementation (`sql-quote.ts`), @@ -58,8 +119,8 @@ auto-generated per-PR notes; this file is the curated, human-readable history. `clickhouse-http-sql-spans.test.ts`, `clickhouse-http-sql-quote.test.ts`); the moved `isSupportedOptionScalar` describe block now lives in `tests/unit/param-type.test.ts` alongside its relocated implementation. - Phase 6 auth composition and Phase 7 query/export/transport-seam cutover - remain deferred. + Phase 6 auth composition landed next (see above); Phase 7's + query/export/transport-seam cutover remains deferred. - **#630 Phase 4: add consuming query APIs, a minimal ClickHouse HTTP error, and a stateless `KILL QUERY` to `@altinity/clickhouse-http`.** Purely diff --git a/CLAUDE.md b/CLAUDE.md index a4e622d0..bd9b3d7f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -43,10 +43,25 @@ all bundled — see hard rule 4). Quality is held by tests. consumers, `ClickHouseError`) remain importable only under `src/net/**`, exactly as Phase 2 established, alongside OAuth/Basic credential acquisition, refresh, epochs, lifecycle callbacks, retries, and SQL - Browser's own product operations/result modes; the Phase-4 consuming - query APIs (`queryJson`/`queryText`/`queryProgress`) remain additive and - not yet consumed by any `src/**` caller (that cutover is Phase 7). The - name/shape check has no type-only carve-out: `import type`/`export type` + Browser's own product operations/result modes. Since #630 Phase 6, the + normal-request auth/epoch/refresh/lifecycle policy this rule already + places under `src/net/` is owned by `src/net/authenticated-clickhouse- + request.ts` (moved out of `ch-client.ts`'s former `authedFetch`/ + `transportFor(ctx)`, deleted outright — no forwarding alias): it builds + the package client directly (`client.request()`) and composes it with + the package's non-consuming success classifier/response consumers + (`consumeJsonResponse`/`consumeTextResponse`/`consumeProgressResponse`), + never the package's own convenience `queryJson`/`queryText`/ + `queryProgress` methods — those need an already-resolved Authorization + and give this policy no chance to inspect the settled `Response` first. + `ch-client.ts`'s exported `queryJson()` is the first real production + consumer of that response-consumer layer; `runQuery`/`exportQuery` reach + the new module's raw request entrypoint but keep their own result/error/ + body handling. The Phase-4 convenience consuming query APIs + (`queryJson`/`queryText`/`queryProgress`) themselves remain additive and + not yet consumed by any `src/**` caller (that full cutover, plus + `runQuery`/`exportQuery`'s own result/export ownership migration, is + Phase 7). The name/shape check has no type-only carve-out: `import type`/`export type` and individual `import { type X }` specifiers of a transport/protocol name are flagged on exactly the same terms as a value reference — erasure before bundling does not exempt a source-level NAME ownership @@ -168,8 +183,8 @@ Touch these in one change: | Path | What | |---|---| | `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, the low-level injected-`fetch()` request, the progress-stream read loop and HTTP exception parsing/framing (Phase 3), (Phase 4) non-consuming success/error classification (`ensureClickHouseSuccess`), JSON/text/progress consumers, a minimal `ClickHouseError`, convenience `queryJson`/`queryText`/`queryProgress` client methods, and a stateless wire-level `killQuery`, and (Phase 5) the ONE ClickHouse SQL-quoting implementation (`sqlString`/`quoteIdent`/`qualifyIdent`), the ONE generic type-expression grammar (`parseClickHouseType`/`analyzeTypeModifiers`/`canonicalType`/wrapper/enum helpers), and the shared lexical scanner (`scanSpans`) — behind a public `.` export only; transport/protocol APIs stay `src/net/**`-only, while the pure-language exports above may be imported directly by their real SQL Browser consumers anywhere outside `src/net/**` too (mechanically allowlisted, `build/check-boundaries.mjs` Rule D); no `src/**` caller consumes the Phase-4 consuming query APIs yet | +| `src/net/*` | OAuth + ClickHouse client, injected fetch; `authenticated-clickhouse-request.ts` (#630 Phase 6) is the sole normal-request auth/epoch/refresh/lifecycle owner, over the package's `request()` and response consumers | +| `packages/clickhouse-http/src/*` | first-party npm workspace (repo's first, #630 Phase 2) — `chUrl`/URL serialization, the low-level injected-`fetch()` request, the progress-stream read loop and HTTP exception parsing/framing (Phase 3), (Phase 4) non-consuming success/error classification (`ensureClickHouseSuccess`), JSON/text/progress consumers, a minimal `ClickHouseError`, convenience `queryJson`/`queryText`/`queryProgress` client methods, and a stateless wire-level `killQuery`, and (Phase 5) the ONE ClickHouse SQL-quoting implementation (`sqlString`/`quoteIdent`/`qualifyIdent`), the ONE generic type-expression grammar (`parseClickHouseType`/`analyzeTypeModifiers`/`canonicalType`/wrapper/enum helpers), and the shared lexical scanner (`scanSpans`) — behind a public `.` export only; transport/protocol APIs stay `src/net/**`-only, while the pure-language exports above may be imported directly by their real SQL Browser consumers anywhere outside `src/net/**` too (mechanically allowlisted, `build/check-boundaries.mjs` Rule D); since Phase 6, `src/net/authenticated-clickhouse-request.ts` is a real production consumer of `request()` plus the non-consuming classifier/JSON/text/progress consumers — the convenience `queryJson`/`queryText`/`queryProgress` methods themselves still have no `src/**` consumer (that cutover is Phase 7) | | `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/build/check-boundaries.mjs b/build/check-boundaries.mjs index 4074ee49..90499cb3 100644 --- a/build/check-boundaries.mjs +++ b/build/check-boundaries.mjs @@ -144,17 +144,23 @@ const RULES = [ // file would leave the sibling contract file unguarded. `forbidden` targets // are resolved repo-relative paths (never raw specifier strings like // './ch-client.ts'), matching how the checker resolves and compares them. + // + // Issue #630 Phase 6: the normal-request auth/epoch/refresh/lifecycle + // policy moved out of `ch-client.ts` into the new + // `src/net/authenticated-clickhouse-request.ts` — the CURRENT auth-policy + // owner this transport leaf must not reach, so both forbidden lists below + // name it alongside `ch-client.ts`. { dir: 'src/net/clickhouse-http-transport.ts', - forbidden: ['src/net/ch-client.ts', 'src/net/oauth.ts', - 'src/net/oauth-config.ts', 'src/application', 'src/ui'], - why: 'issue #585 Phase 1: the generic transport cannot reach auth/application policy or UI', + forbidden: ['src/net/ch-client.ts', 'src/net/authenticated-clickhouse-request.ts', + 'src/net/oauth.ts', 'src/net/oauth-config.ts', 'src/application', 'src/ui'], + why: 'issue #585 Phase 1 / #630 Phase 6: the generic transport cannot reach auth/application policy or UI', }, { dir: 'src/net/clickhouse-transport.types.ts', - forbidden: ['src/net/ch-client.ts', 'src/net/oauth.ts', - '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', + forbidden: ['src/net/ch-client.ts', 'src/net/authenticated-clickhouse-request.ts', + 'src/net/oauth.ts', 'src/net/oauth-config.ts', 'src/application', 'src/ui'], + why: 'issue #585 Phase 1 / #630 Phase 6: 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 @@ -262,10 +268,18 @@ for (const rule of RULES) { // modules that own or project the lifecycle, and none may regain the retired // server-version shortcut. `serverVersion` remains legitimate catalog/query // capability metadata and user-menu display elsewhere. +// +// Issue #630 Phase 6: the normal-request lifecycle classification +// (`onTransportConnected`/`onTransportOffline`/`onSignedOut` dispatch) moved +// from `ch-client.ts` into `src/net/authenticated-clickhouse-request.ts` — +// the list below keeps `ch-client.ts` (its product-client `ChCtx`/callers +// still matter to this guard through Phase 6) and adds the new lifecycle- +// owning file explicitly, rather than replacing one with the other. const connectionAuthorityFiles = [ 'src/core/connection-lifecycle.ts', 'src/application/connection-session.ts', 'src/net/ch-client.ts', + 'src/net/authenticated-clickhouse-request.ts', 'src/ui/app-header.ts', 'src/ui/app-shell.ts', ]; diff --git a/docs/ADR-0005-clickhouse-web-client.md b/docs/ADR-0005-clickhouse-web-client.md index b164e071..178d15f4 100644 --- a/docs/ADR-0005-clickhouse-web-client.md +++ b/docs/ADR-0005-clickhouse-web-client.md @@ -1139,6 +1139,52 @@ a later #630 phase, tracked alongside the SQL quoting/type-grammar extraction (#630 Phase 5) and authenticated composition (#630 Phase 6) this ADR's Phase 2 addendum already named as deferred. +### #630 Phase 6 authenticated-composition addendum (2026-08-08) + +**This addendum, like the Phase 2/4 addenda above, 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, and — as with Phase 4 — issue #630's own phase numbering is +independent of, and unrelated to, this ADR's own Phase 1–4 numbering. + +Issue #630 Phase 6 moves SQL Browser's normal-request authentication +authority itself: `src/net/ch-client.ts`'s former `authedFetch()`/ +module-private `transportFor(ctx)` (credential acquisition, epoch fencing, +one-refresh retry, connect/offline/sign-out lifecycle classification) are +deleted outright and replaced by a new module, +`src/net/authenticated-clickhouse-request.ts`, which places that SAME +policy directly over the first-party package's `createClickHouseHttpClient( +...).request()` and its `consumeJsonResponse`/`consumeTextResponse`/ +`consumeProgressResponse` response consumers — never the official +`@clickhouse/client-web` package this ADR evaluated and rejected. As with +every prior #630 extraction addendum, no third-party HTTP client is +introduced and no cancellation semantics change: the new module still +passes the caller's own `AbortSignal` straight through to the package's +`request()`, which passes it straight through to 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 (see the +"Phase 2 cancellation-incompatibility addendum"). Proven again, directly +against the new authenticated composition, by real-browser Chromium/WebKit +scenarios extending the existing native-cancellation harness +(`tests/e2e/clickhouse-http-transport.{html,spec.js}`, scenarios 5-9's +authenticated-path variants) — a real, production-shaped +`AuthenticatedRequestCtx` (synthetic test credentials only) driving the +identical post-header body-lifetime/no-late-callback/concurrent-isolation +proofs this ADR's Phase 0/Phase 1 evidence already established for the raw +transport. + +Phase 6's own scope discipline: SQL Browser authentication authority now +lives in `authenticated-clickhouse-request.ts` over the Fetch-native +first-party package (`packages/clickhouse-http`, itself #630 Phase 2's +extraction of this ADR's own already-proven-correct hand-rolled mechanics — +see the Phase 2 addendum), not in the official client this ADR rejects. +`runQuery`/`exportQuery`'s own cutover onto the package's convenience +consuming query APIs, and the remaining transport-adapter compatibility +seam's eventual deletion, stay deferred to #630 Phase 7 as already recorded +above. Every historical spike result, gate outcome, and date elsewhere in +this ADR is unchanged by this addendum. + ```sh diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 82361730..a34dfb6f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -82,7 +82,7 @@ module is tested with plain stubs at the per-file coverage gate. |---|---| | `authenticated-execution-scope` (`app.executionScope`) | one disposable, epoch-fenced registry for authenticated operation owners; closes local work synchronously and performs best-effort remote cancellation from an immutable credential lease | | `query-execution-service` (`app.exec`) | the shared request/stream/normalize read core + the script transport loop (retry classification, stop-on-first-failure, per-attempt `query_id`); stateless `kill(queryId)` — cancellation is caller-owned (`AbortController`s live with the owning session) | -| `connection-session` (`app.conn`) | authoritative auth + connection lifecycle (`starting` / `connected` / `refreshing` / `offline` / `auth-required` / `reauthenticating` / `signed-out`), OAuth PKCE login/refresh, Basic probing, IdP config, identity, token storage, sign-out, and **the single live `chCtx` object** (mutated in place — `authConfirmed` by `net/ch-client`, `origin` by sign-in — never reconstructed) | +| `connection-session` (`app.conn`) | authoritative auth + connection lifecycle (`starting` / `connected` / `refreshing` / `offline` / `auth-required` / `reauthenticating` / `signed-out`), OAuth PKCE login/refresh, Basic probing, IdP config, identity, token storage, sign-out, and **the single live `chCtx` object** (mutated in place — `authConfirmed` by `net/authenticated-clickhouse-request`, `origin` by sign-in — never reconstructed) | | `schema-catalog-service` (`app.catalog`) | server version, schema tree, lazy columns, SQL reference/completions, entity-doc cache; catalog/schema/reference/docs transports share a connection-generation abort signal, and `invalidate()` synchronously aborts them while generation fences reject stale writes | | `workbench-parameter-session` (`app.params`) | `{name:Type}` analysis/prepare/gate policy, input-vs-execute hardening, enum inference, recent values; reads the live shared `AppState` slices through accessors | | `export-service` (`app.exports`) | direct + script export behind an injectable `ExportSink` (`pickFile`/`pickDirectory`); hold-back exception inspection, `.partial` semantics, its own cancellation state | @@ -192,8 +192,8 @@ session's credentials. This includes async HTTP error-body classification and IdP config discovery: refresh authority snapshots its epoch, then rechecks it after discovery and before token-endpoint I/O. A stale discovery therefore cannot rewrite the replacement epoch's auth-header policy. -`net/ch-client` reports only successful 2xx transport settlement as connected -and rejected, non-aborted `fetch` as offline. HTTP query failures — including a +`net/authenticated-clickhouse-request` reports only successful 2xx transport +settlement as connected and rejected, non-aborted `fetch` as offline. HTTP query failures — including a post-confirmation 401/403 — remain query outcomes, not connection state. The header chip is a pure projection of this lifecycle; `serverVersion` remains display metadata and is never connection authority. @@ -263,21 +263,25 @@ 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 +implementation can silently reappear. Through Phase 5, `ch-client.ts` kept +every auth/epoch/retry/lifecycle policy (`authedFetch`), product operation, +and `ChCtx` exactly as before; a module-private `transportFor(ctx)` delegated 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, -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 +gained no field and there was no runtime transport switch. (**#630 Phase 6**, +documented in its own section below, later moves that auth/epoch/retry/ +lifecycle policy itself out of `ch-client.ts` into a new module.) `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. Through +Phase 5, `authedFetch` snapshotted 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`. (Phase 6 +moves this exact preflight verbatim into the new authenticated module.) A +reusable contract-test-suite factory (`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 @@ -338,11 +342,15 @@ target into the HTTP request's own `params.query_id`. Its own private `quoteKillQueryId` reproduces only `src/core/format.ts`'s `sqlString()` backslash-then-quote escaping convention as a narrow, unexported Phase-4 stopgap — **Phase 5** replaces it with the package's own shared public -string-literal quoting API (below). Root `killQuery`/`killQueryWithLease` and -`queryJson`/`runQuery`/`exportQuery` are unaffected: they continue to reach -`authedFetch`'s auth/epoch/retry policy exactly as before, and none of them -have been migrated onto the new package consuming-query APIs — that cutover -is Phase 7. +string-literal quoting API (below). At this point (Phase 4) root +`killQuery`/`killQueryWithLease` and `queryJson`/`runQuery`/`exportQuery` +were unaffected: they continued to reach `authedFetch`'s auth/epoch/retry +policy exactly as before, and none of them had been migrated onto the new +package consuming-query APIs. **Phase 6** (below) moves that auth/epoch/ +retry/lifecycle policy to a new module and switches `queryJson` onto its +JSON response consumer; `runQuery`/`exportQuery`'s consuming-query-API +cutover, and the remaining `killQuery`/`killQueryWithLease` migration, +stay Phase 7. ### SQL quoting and the generic type grammar (#630 Phase 5) @@ -390,6 +398,85 @@ protocol package exports from pure-language ones, since SQL Browser language consumers now legitimately import the package from outside `src/net/**` — see the boundary rule text above for the mechanics. +### Authenticated request layer (#630 Phase 6) + +Phase 6 moves the SQL Browser normal-request auth/epoch/refresh/lifecycle +policy — `authedFetch()` and the module-private, `ChCtx`-based +`transportFor(ctx)` — out of `ch-client.ts` into a new module, +`src/net/authenticated-clickhouse-request.ts`. This is a real move+delete, +not an additive layer: both are gone from `ch-client.ts`, with no +forwarding alias, no second retry loop, and no second Authorization +constructor. + +``` +authenticatedRequest(ctx, request) the moved trust-boundary loop — + credential acquisition, epoch + fencing, one-refresh retry, + connect/offline/sign-out + classification — over the + package's client.request() +authenticatedJson(ctx, request) authenticatedRequest() + package + consumeJsonResponse() +authenticatedText(ctx, request) authenticatedRequest() + package + consumeTextResponse() +authenticatedProgress(ctx, request, cbs) authenticatedRequest() + package + consumeProgressResponse() +``` + +The module builds the package client directly +(`createClickHouseHttpClient({ fetch: () => ctx.fetch, origin: () => ctx.origin })`) +once per `authenticatedRequest()` invocation, before the retry loop, and +calls `client.request()` on every attempt — never `client.queryJson`/ +`queryText`/`queryProgress`: those convenience methods build a request from +an already-resolved Authorization and give this layer no chance to inspect +the settled `Response` before deciding whether a refresh/retry is +authorized, which is exactly the policy this module owns. The eager, +discarded `chUrl(...)` pre-credential preflight, the per-attempt complete +Authorization construction, the final epoch fence immediately before +`client.request()`, and every fence after a credential/body await all move +verbatim from the old `authedFetch()`. + +`AuthenticatedRequestCtx` is a narrow base seam — only the fields this +module actually needs (`fetch`, `origin`, `getToken`, `refresh`, +`onSignedOut`, and the optional `authHeader`/`authConfirmed`/ +`currentEpoch`/`onTransportConnected`/`onTransportOffline`). `ch-client.ts`'s +own `ChCtx` now `extends AuthenticatedRequestCtx` instead of redeclaring its +fields, adding only `dataLakeCatalogSettingUnsupported` — the one field +genuinely specific to this product client, kept out of the auth module on +purpose. `AuthenticatedCancellationLease` stays exported from `ch-client.ts` +unmoved: relocating it was not required to satisfy this phase and would +have created unrelated application import churn. + +`queryJson()` is the first real production consumer of the package's +response-consumer layer: it now delegates to `authenticatedJson()`, +translating the package's `ClickHouseError` back to `queryJson`'s EXISTING +plain-`Error` compatibility shape — same parsed message (both are derived +from the same `parseExceptionText`), different error class — so this phase +adopts the new consumer without changing an existing SQL Browser API. +`runQuery()`/`exportQuery()` switch only their `authedFetch()` call to the +new raw `authenticatedRequest()` entrypoint, keeping their own Table/KPI/raw +format mapping, row-cap settings, non-2xx parsing, and streaming exactly as +before. `killQuery()` inherits the new path indirectly through `queryJson()`. +`killQueryWithLease()`'s frozen-lease bypass is untouched: it already built +its own one-shot transport directly from the frozen lease's exact origin/ +Authorization/Fetch authority, never through `ChCtx`, so it does not — and +must not — route through the new mutable-context auth loop. + +`build/check-boundaries.mjs`'s two existing #585 transport-leaf forbidden +lists (`clickhouse-http-transport.ts`, `clickhouse-transport.types.ts`, both +above) and the #512 `connectionAuthorityFiles` lifecycle-authority list now +name `authenticated-clickhouse-request.ts` as the current auth/lifecycle +owner they must not reach or regain, alongside `ch-client.ts` (kept through +Phase 7). This is a data extension of two existing dependency rules plus one +existing lifecycle-authority list — no new scanner. + +Deferred to **Phase 7**: `runQuery`/`exportQuery`'s cutover onto the +package's consuming query APIs and result/export ownership, the remaining +`killQuery`/`killQueryWithLease` transport migration, and deletion of the +now-superseded `clickhouse-http-transport.ts`/`clickhouse-transport.types.ts` +compatibility seam (still used by `killQueryWithLease` and by the real- +browser harness's raw/unauthenticated scenarios through Phase 6). + ## Build `build/build.mjs` runs esbuild (bundle + minify, IIFE), inlines the script and diff --git a/src/application/connection-session.ts b/src/application/connection-session.ts index bf789051..dd300f50 100644 --- a/src/application/connection-session.ts +++ b/src/application/connection-session.ts @@ -84,8 +84,10 @@ export interface ConnectionSessionDeps { /** The live ClickHouse auth context this session owns — `origin`/ * `authConfirmed` are mutated IN PLACE by `signOut`/`connectBasic`/ - * `applyAuthSnapshot` here, and by `net/ch-client.js`'s `authedFetch` (its - * own one-shot latch) — this ONE object is never reconstructed, only ever + * `applyAuthSnapshot` here, and by `net/authenticated-clickhouse-request.js`'s + * `authenticatedRequest` (its own one-shot latch, reached through + * `net/ch-client.js`'s `queryJson`/`runQuery`/`exportQuery` — #630 Phase 6) + * — this ONE object is never reconstructed, only ever * mutated, so a caller holding a reference (or passing it straight into * ch-client's functions) always observes the current auth state. Assignable * to `net/ch-client.js`'s own `ChCtx` (whose `authConfirmed`/`authHeader` @@ -481,7 +483,8 @@ export function createConnectionSession(deps: ConnectionSessionDeps): Connection function refresh(): Promise { // Basic credentials don't expire and can't be refreshed; a surviving 401 - // means the password is wrong → authedFetch falls through to onSignedOut. + // means the password is wrong → authenticatedRequest (net/authenticated- + // clickhouse-request.js) falls through to onSignedOut. if (authMode === 'basic') return Promise.resolve(false); const epoch = connectionSignal.value.epoch; if (refreshSlot?.epoch === epoch) return refreshSlot.promise; @@ -529,7 +532,8 @@ export function createConnectionSession(deps: ConnectionSessionDeps): Connection } async function getToken(): Promise { - // In basic mode the stored credential is the "token" authedFetch carries. + // In basic mode the stored credential is the "token" authenticatedRequest + // (net/authenticated-clickhouse-request.js) carries. if (authMode === 'basic') return basicCreds(); if (!token) return null; if (!isTokenExpired(token)) return token; diff --git a/src/net/authenticated-clickhouse-request.ts b/src/net/authenticated-clickhouse-request.ts new file mode 100644 index 00000000..952cf50c --- /dev/null +++ b/src/net/authenticated-clickhouse-request.ts @@ -0,0 +1,240 @@ +// Issue #630 Phase 6 — the sole SQL Browser authenticated-request authority: +// credential acquisition, epoch fencing, one-refresh retry, and +// connect/offline/sign-out lifecycle classification, composed directly around +// the package's `createClickHouseHttpClient(...).request()` and its response +// consumers (`consumeJsonResponse`/`consumeTextResponse`/ +// `consumeProgressResponse`). This module is the moved trust-boundary logic +// that used to live as `authedFetch`/`transportFor(ctx)` inside +// `src/net/ch-client.ts` — mechanically unchanged behavior, new owner and new +// file (see that file's own module doc for the ownership-history trail). +// +// `AuthenticatedRequestCtx` is a NARROW base seam: it owns only the fields +// this module actually reads (credentials, epoch, lifecycle callbacks). SQL +// Browser's own product `ChCtx` (`ch-client.ts`) extends it with +// `dataLakeCatalogSettingUnsupported`, a product-operation latch this module +// has no business knowing about — keeping this file the auth authority +// without also becoming the product-client context. +// +// Scope discipline mirrors the package's own (client.ts's header comment): +// this module still does not call `client.queryJson`/`queryText`/ +// `queryProgress` — those convenience methods build a request from an +// ALREADY-RESOLVED Authorization and give this layer no chance to inspect +// the settled `Response` before deciding whether a refresh/retry is +// authorized. Instead, `authenticatedRequest` performs the trust-boundary +// loop itself around the low-level `client.request()`, and the three thin +// wrappers below (`authenticatedJson`/`authenticatedText`/ +// `authenticatedProgress`) each compose it with exactly one matching package +// response consumer — mirroring `client.ts`'s own queryJson/queryText/ +// queryProgress shape one layer up, over an authenticated `Response` instead +// of a directly-resolved one. + +import { + createClickHouseHttpClient, chUrl, parseExceptionText, + consumeJsonResponse, consumeTextResponse, consumeProgressResponse, +} from '@altinity/clickhouse-http'; +import type { ClickHouseHttpRequest, StreamCallbacks } from '@altinity/clickhouse-http'; +import { isAuthExpiredBody, authDeniedMessage } from '../core/stream.js'; + +/** The narrow side-effect seam this module needs: credential acquisition, + * refresh, sign-out, and the epoch/lifecycle hooks that let a superseded + * (stale) request's late settlement leave the replacement session's state + * untouched. `authHeader`/`authConfirmed`/`currentEpoch`/ + * `onTransportConnected`/`onTransportOffline` are optional, preserving the + * smaller seam existing callers that omit them already use — a client that + * supplies no epoch hook is backward-compatible: every request is current. */ +export interface AuthenticatedRequestCtx { + fetch: typeof fetch; + origin: string; + getToken(): Promise; + refresh(): Promise; + onSignedOut(detail?: string, expectedEpoch?: number): void; + /** Picks the Authorization scheme (Bearer vs Basic); defaults to Bearer + * inside `authenticatedRequest` when absent. */ + authHeader?: (token: string) => string; + authConfirmed?: boolean; + /** Identifies the active credential/session generation. A request captures + * this before its first await so stale work cannot affect its replacement. */ + currentEpoch?: () => number; + /** Receives a current request's successful HTTP 2xx transport settlement. */ + onTransportConnected?: () => void; + /** Receives a current non-abort rejection from the injected fetch seam. */ + onTransportOffline?: (error?: unknown) => void; +} + +/** One ClickHouse HTTP request with NO caller-supplied `authorization` — this + * module resolves the complete Authorization header for THIS request (and + * its at-most-one retry) itself, per attempt. Structurally prevents an + * ordinary caller from supplying or caching a value this module must + * recompute. */ +export type AuthenticatedClickHouseRequest = Omit; + +// A client that supplied no epoch hook remains backward-compatible: every +// request is current. When it did supply one, no stale request may alter the +// replacement credential generation's lifecycle/auth state. +function isCurrentEpoch(ctx: AuthenticatedRequestCtx, requestEpoch: number | undefined): boolean { + return requestEpoch === undefined || ctx.currentEpoch?.() === requestEpoch; +} + +// A request that was superseded before it can start (or retry) is cancellation, +// not an authentication failure. Keep the shape callers already treat as a +// silent cancellation without coupling this network module to a DOMException +// implementation. +function staleEpochAbort(): Error { + const error = new Error('request superseded by a newer authentication session'); + error.name = 'AbortError'; + return error; +} + +/** + * POST `request.sql` to ClickHouse with one automatic token-refresh retry. + * Resolves to the raw Response. Throws Error('signed out') after calling + * ctx.onSignedOut() when authentication cannot be recovered. `request` omits + * `authorization` — this function resolves the credential for THIS request + * (and its retry) itself; every other field is the caller's request, + * unchanged. + */ +export async function authenticatedRequest( + ctx: AuthenticatedRequestCtx, + request: AuthenticatedClickHouseRequest, +): Promise { + const requestEpoch = ctx.currentEpoch?.(); + // Centralized aliasing defense: snapshot the incoming request's + // settings/params synchronously HERE, at entry, before the first await + // (`ctx.getToken()`) — one mechanism for every present and future caller, + // rather than per-call-site defensive spreads. This preserves invocation- + // time capture (`chUrl` serializes both records into the URL string + // synchronously, before this function's first await), so a caller that + // retains and mutates either record while a token/refresh await is pending + // cannot change the request this function already committed to sending — + // on the initial attempt AND the one-refresh retry alike. + const settings = request.settings ? { ...request.settings } : undefined; + const params = request.params ? { ...request.params } : undefined; + const { sql, defaultFormat, signal } = request; + // Request-preparation failures are not transport failures. Every caller + // resolves its credential (this function's next line) only after this + // synchronous, discarded `chUrl` validation — so a URIError from malformed + // settings/params (e.g. `encodeURIComponent` on a lone UTF-16 surrogate) + // propagates as a synchronous throw with no token read, no fetch, and no + // `onTransportOffline` call. The package's `client.request()` builds this + // same URL again at actual send time (against the possibly-since-mutated + // live `ctx.origin`) and resolves that failure as a REJECTED promise + // instead, since it is async — so without this eager pre-credential + // preflight, the identical throw would surface only after `ctx.getToken()` + // and would be misclassified as a network failure. + chUrl(ctx.origin, { format: defaultFormat, extra: settings, params }); + const token = await ctx.getToken(); + // getToken may have awaited a sign-in/sign-out replacement. Its credential + // belongs to that replacement and this request must not send it. + if (!isCurrentEpoch(ctx, requestEpoch)) throw staleEpochAbort(); + if (!token) { + ctx.onSignedOut(undefined, requestEpoch); + throw new Error('not signed in'); + } + let bearer = token; + let attempt = 0; + // ctx.authHeader(token) lets the app pick the scheme (Bearer vs Basic); + // default to Bearer so the seam stays optional. + const authHeader = ctx.authHeader || ((t: string) => 'Bearer ' + t); + // Built once per `authenticatedRequest` invocation, before the retry loop, + // from LIVE accessors (never snapshotted values) — a live, mutable + // `ctx.origin`/`ctx.fetch` (mutated in place on sign-in, e.g. + // `connection-session.ts`) stays authoritative across the whole retry + // cycle, not just the first attempt. + const client = createClickHouseHttpClient({ fetch: () => ctx.fetch, origin: () => ctx.origin }); + for (;;) { + let resp: Response; + try { + // Fence every attempt immediately before the injected side effect. A + // retry must never send a replacement session's newly-read credential. + // (Precision: `client.request` internally evaluates its own + // `origin()`/`fetch()` accessors and builds the URL AFTER this fence, + // immediately before the fetch itself — both accessors are required to + // be plain, synchronous, side-effect-free property reads, matching the + // package's own `ClickHouseHttpClientDeps` contract.) + const authorization = authHeader(bearer); + if (!isCurrentEpoch(ctx, requestEpoch)) throw staleEpochAbort(); + resp = await client.request({ sql, defaultFormat, settings, params, authorization, signal }); + } catch (e) { + // Only a rejected fetch is a transport failure. HTTP failures are normal + // responses and caller cancellation is deliberately invisible here. + const aborted = signal?.aborted || (e as { name?: unknown } | null)?.name === 'AbortError'; + if (isCurrentEpoch(ctx, requestEpoch) && !aborted) ctx.onTransportOffline?.(e); + throw e; + } + // The request may have crossed a sign-in/sign-out boundary while fetch was + // pending. Its response still belongs to its caller, but cannot change the + // new epoch's connection/auth state or start a refresh with its token. + if (!isCurrentEpoch(ctx, requestEpoch)) return resp; + // A 2xx confirms the credentials are good for the rest of the session. + if (resp.ok) { + ctx.authConfirmed = true; + ctx.onTransportConnected?.(); + } + let authExpired = resp.status === 401 || resp.status === 403; + if (!authExpired && !resp.ok) { + const peek = await resp.clone().text(); + // Reading an error body is another async boundary. If this request was + // superseded while it was pending, its expiry marker must not start a + // refresh against the replacement session's credentials. + if (!isCurrentEpoch(ctx, requestEpoch)) return resp; + if (isAuthExpiredBody(peek)) authExpired = true; + } + if (authExpired) { + // Once this session has authenticated successfully, the same credentials + // are still valid — so a later 401/403 is a *query-level* error ClickHouse + // maps to that HTTP status (ACCESS_DENIED, or UNKNOWN_USER from e.g. + // `SHOW CREATE USER `), not a sign-in problem. Return it so the + // caller shows it as a normal query error instead of force-logging-out. + if (ctx.authConfirmed) return resp; + if (attempt === 0 && (await ctx.refresh())) { + if (!isCurrentEpoch(ctx, requestEpoch)) throw staleEpochAbort(); + // A successful refresh always yields a fresh, usable token — the + // refresh() contract this seam relies on. + bearer = (await ctx.getToken())!; + if (!isCurrentEpoch(ctx, requestEpoch)) throw staleEpochAbort(); + attempt++; + continue; + } + if (!isCurrentEpoch(ctx, requestEpoch)) return resp; + // First-contact 401/403 with a non-expired token: CH rejected the login + // itself — an authorization/identity problem, not session expiry. Surface + // CH's own reason so it's diagnosable. + const reason = parseExceptionText(await resp.clone().text()); + if (!isCurrentEpoch(ctx, requestEpoch)) return resp; + ctx.onSignedOut(authDeniedMessage(resp.status, reason), requestEpoch); + throw new Error('signed out'); + } + return resp; + } +} + +/** One `authenticatedRequest()` + the package's `consumeJsonResponse()`. + * Throws the package's `ClickHouseError` on a resolved non-2xx response; + * native JSON/network/abort errors propagate unchanged. */ +export async function authenticatedJson( + ctx: AuthenticatedRequestCtx, + request: AuthenticatedClickHouseRequest, +): Promise { + return consumeJsonResponse(await authenticatedRequest(ctx, request)); +} + +/** One `authenticatedRequest()` + the package's `consumeTextResponse()`. */ +export async function authenticatedText( + ctx: AuthenticatedRequestCtx, + request: AuthenticatedClickHouseRequest, +): Promise { + return consumeTextResponse(await authenticatedRequest(ctx, request)); +} + +/** One `authenticatedRequest()` + the package's `consumeProgressResponse()` — + * drives the authenticated response's body through the package's ONE + * progress-stream read loop, with the caller's original `AbortSignal` + * (passed into `authenticatedRequest` above, never a derived controller) + * still governing the whole response lifetime including body streaming. */ +export async function authenticatedProgress( + ctx: AuthenticatedRequestCtx, + request: AuthenticatedClickHouseRequest, + callbacks?: StreamCallbacks, +): Promise { + return consumeProgressResponse(await authenticatedRequest(ctx, request), callbacks); +} diff --git a/src/net/ch-client.ts b/src/net/ch-client.ts index 557defb8..b13f25e1 100644 --- a/src/net/ch-client.ts +++ b/src/net/ch-client.ts @@ -7,7 +7,6 @@ // onSignedOut() } // so the whole module is unit-testable with plain stubs. -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'; // Issue #585 Phase 1 — the transport seam. `chUrl` moved verbatim to @@ -15,9 +14,10 @@ import type { SchemaGraphTableRow, SchemaGraphDictRow } from '../core/schema-gra // parameter type) so every existing importer — including // `tests/spike/clickhouse-client/current-adapter.ts` — keeps resolving. The // 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. +// at the time this module kept every auth/epoch/retry policy, product +// operation, and `ChCtx` exactly as before, delegating through the transport +// instead of calling `chUrl`/`ctx.fetch` directly (the auth/epoch/retry +// policy itself later moved out — see the Phase 6 note below). // // Issue #630 Phase 2 — `chUrl` now comes from `@altinity/clickhouse-http` // (the package is the ONE serializer implementation, contract A5); this @@ -31,10 +31,8 @@ import type { SchemaGraphTableRow, SchemaGraphDictRow } from '../core/schema-gra // (`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`/ +// seam violation) instead of going through a ChCtx-based transport for the +// stream half. `parseExceptionText`/`findExceptionFrame`/`StreamLine`/ // `StreamCallbacks` are re-exported below as zero-logic migration plumbing: // `src/application/**` cannot import the package directly (Rule D — its // language-export allowlist is for the SQL Browser layers that consume @@ -49,12 +47,30 @@ import type { SchemaGraphTableRow, SchemaGraphDictRow } from '../core/schema-gra // full surface (transport APIs and language exports alike), so it imports // `sqlString` directly rather than through `../core/format.js` (which no // longer declares it at all). +// +// Issue #630 Phase 6 — the normal-request auth/epoch/refresh/lifecycle +// policy that used to live here as `authedFetch`/`transportFor(ctx)` MOVED +// to `src/net/authenticated-clickhouse-request.ts` — a real move+delete, not +// an additive layer: both are gone from this file, with no forwarding +// alias, no second retry loop, and no second Authorization constructor. +// `queryJson` below now delegates to that module's `authenticatedJson()` +// (the first real production consumer of the package's response-consumer +// layer); `runQuery`/`exportQuery` call its raw `authenticatedRequest()` +// entrypoint directly, keeping their own result/error/body handling exactly +// as before (that further cutover is Phase 7). `ChCtx` extends the new +// module's narrower `AuthenticatedRequestCtx` rather than duplicating its +// fields — `dataLakeCatalogSettingUnsupported` is the one field genuinely +// specific to this product client, so it stays declared here, not there. +// `killQueryWithLease`'s frozen-lease bypass is UNTOUCHED and does not +// route through the new module — it never read mutable `ChCtx` auth state +// even before this move (see its own doc comment below). import { - chUrl, streamLines, parseExceptionText, findExceptionFrame, sqlString, + chUrl, streamLines, parseExceptionText, findExceptionFrame, sqlString, ClickHouseError, } 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'; +import { authenticatedJson, authenticatedRequest } from './authenticated-clickhouse-request.js'; +import type { AuthenticatedRequestCtx } from './authenticated-clickhouse-request.js'; export { chUrl, parseExceptionText, findExceptionFrame }; export type { ChUrlOpts, StreamLine, StreamCallbacks } from '@altinity/clickhouse-http'; export type { ClickHouseTransport, TransportDeps, TransportRequest } from './clickhouse-transport.types.js'; @@ -64,28 +80,18 @@ export type { ClickHouseTransport, TransportDeps, TransportRequest } from './cli /** The injected side-effect seam every function in this module takes as its * first argument. `fetch`/`getToken`/`refresh`/`onSignedOut` are the app's * real implementations in production, plain stubs in tests. `authConfirmed` - * and `dataLakeCatalogSettingUnsupported` are one-shot-then-remember latches - * `authedFetch`/`querySystemAware` set on `ctx` itself (see their docstrings) - * — optional here because they start unset. The epoch/lifecycle hooks are - * optional too, preserving the smaller seam used by existing callers. */ -export interface ChCtx { - fetch: typeof fetch; - origin: string; - getToken(): Promise; - refresh(): Promise; - onSignedOut(detail?: string, expectedEpoch?: number): void; - /** Picks the Authorization scheme (Bearer vs Basic); defaults to Bearer - * inside `authedFetch` when absent. */ - authHeader?: (token: string) => string; - authConfirmed?: boolean; + * is a one-shot-then-remember latch `authenticatedRequest` + * (`authenticated-clickhouse-request.ts`) sets on `ctx` itself; + * `dataLakeCatalogSettingUnsupported` is `querySystemAware`'s own latch (see + * their docstrings) — both optional here because they start unset. The + * epoch/lifecycle hooks are optional too, preserving the smaller seam used + * by existing callers. `ChCtx` extends `AuthenticatedRequestCtx` + * (#630 Phase 6) rather than redeclaring its fields: this interface adds + * only `dataLakeCatalogSettingUnsupported`, the one field genuinely specific + * to this product client — every other field is the narrower auth seam the + * new module actually needs. */ +export interface ChCtx extends AuthenticatedRequestCtx { dataLakeCatalogSettingUnsupported?: boolean; - /** Identifies the active credential/session generation. A request captures - * this before its first await so stale work cannot affect its replacement. */ - currentEpoch?: () => number; - /** Receives a current request's successful HTTP 2xx transport settlement. */ - onTransportConnected?: () => void; - /** Receives a current non-abort rejection from the injected fetch seam. */ - onTransportOffline?: (error?: unknown) => void; } /** Immutable authority retained only long enough to cancel work owned by a @@ -123,23 +129,6 @@ function errMessage(e: unknown): string { return typeof message === 'string' && message ? message : String(e); } -// A client that supplied no epoch hook remains backward-compatible: every -// request is current. When it did supply one, no stale request may alter the -// replacement credential generation's lifecycle/auth state. -function isCurrentEpoch(ctx: ChCtx, requestEpoch: number | undefined): boolean { - return requestEpoch === undefined || ctx.currentEpoch?.() === requestEpoch; -} - -// A request that was superseded before it can start (or retry) is cancellation, -// not an authentication failure. Keep the shape callers already treat as a -// silent cancellation without coupling this network module to a DOMException -// implementation. -function staleEpochAbort(): Error { - const error = new Error('request superseded by a newer authentication session'); - error.name = 'AbortError'; - return error; -} - /** Generic ClickHouse `FORMAT JSON` response shape — only `.data` is ever * read here; every other field (meta, statistics, rows_before_limit_at_least…) * is ignored by this module. */ @@ -147,142 +136,23 @@ export interface ChJsonResult> { data?: T[]; } -/** Delegates unconditionally to the single current transport implementation. - * `deps.fetch`/`deps.origin` are accessors reading the LIVE mutable `ctx` - * fields per request (never a snapshot) — `ctx.origin` is mutated in place on - * sign-in (`connection-session.ts`), so a request issued after that mutation - * must observe the new value. `ChCtx` itself gains no new field: there is no - * production runtime switch, only this one unconditional wiring (an - * injectable composition seam is introduced only when a second - * implementation actually exists — Phase 2, which requires a new decision). */ -function transportFor(ctx: ChCtx) { - return createHttpTransport({ fetch: () => ctx.fetch, origin: () => ctx.origin }); -} - -/** - * POST `request.sql` to ClickHouse with one automatic token-refresh retry. - * Resolves to the raw Response. Throws Error('signed out') after calling - * ctx.onSignedOut() when authentication cannot be recovered. `request` omits - * `authorization` — this function resolves the credential for THIS request - * (and its retry) itself; every other `TransportRequest` field is the - * caller's request, unchanged. - */ -export async function authedFetch(ctx: ChCtx, request: Omit): Promise { - const requestEpoch = ctx.currentEpoch?.(); - // Centralized aliasing defense (review finding folded in, pass-5 revision): - // snapshot the incoming request's settings/params synchronously HERE, at - // entry, before the first await (`ctx.getToken()`) — one mechanism for - // every present and future caller, rather than per-call-site defensive - // spreads. This preserves today's invocation-time capture (today `chUrl` - // serializes both records into the URL string synchronously, before this - // function's first await), so a caller that retains and mutates either - // record while a token/refresh await is pending cannot change the request - // this function already committed to sending — on the initial attempt AND - // the one-refresh retry alike. - const settings = request.settings ? { ...request.settings } : undefined; - const params = request.params ? { ...request.params } : undefined; - const { sql, defaultFormat, signal } = request; - // Request-preparation failures are not transport failures (ChatGPT review - // pass 2, P1). Pre-refactor, every caller built `chUrl(...)` as a plain - // argument BEFORE invoking authedFetch, so a URIError from malformed - // settings/params (e.g. `encodeURIComponent` on a lone UTF-16 surrogate) - // propagated as a synchronous throw with no token read, no fetch, and no - // `onTransportOffline` call. `transport.send` now builds that same URL - // internally, inside the try/catch that classifies fetch rejections as - // transport-offline — so without this eager, discarded validation call, the - // identical throw would surface only after `ctx.getToken()` and would be - // misclassified as a network failure. Calling `chUrl` here, before the first - // await, reproduces the exact original failure shape; `transport.send` still - // calls it again at actual send time against the (possibly since-mutated) - // live `ctx.origin` — origin is only concatenated, never `encodeURIComponent`- - // encoded, so it cannot itself throw and re-validating it changes nothing. - chUrl(ctx.origin, { format: defaultFormat, extra: settings, params }); - const token = await ctx.getToken(); - // getToken may have awaited a sign-in/sign-out replacement. Its credential - // belongs to that replacement and this request must not send it. - if (!isCurrentEpoch(ctx, requestEpoch)) throw staleEpochAbort(); - if (!token) { - ctx.onSignedOut(undefined, requestEpoch); - throw new Error('not signed in'); - } - let bearer = token; - let attempt = 0; - // ctx.authHeader(token) lets the app pick the scheme (Bearer vs Basic); - // default to Bearer so the seam stays optional. - const authHeader = ctx.authHeader || ((t: string) => 'Bearer ' + t); - const transport = transportFor(ctx); - for (;;) { - let resp: Response; - try { - // Fence every attempt immediately before the injected side effect. A - // retry must never send a replacement session's newly-read credential. - // (Precision: `transport.send` internally evaluates the REQUIRED-PURE - // `deps.origin()`/`deps.fetch()` accessors and builds the URL AFTER - // this fence, immediately before the fetch itself — see - // `clickhouse-transport.types.ts`'s `TransportDeps` doc comment.) - const authorization = authHeader(bearer); - if (!isCurrentEpoch(ctx, requestEpoch)) throw staleEpochAbort(); - resp = await transport.send({ sql, defaultFormat, settings, params, authorization, signal }); - } catch (e) { - // Only a rejected fetch is a transport failure. HTTP failures are normal - // responses and caller cancellation is deliberately invisible here. - const aborted = signal?.aborted || (e as { name?: unknown } | null)?.name === 'AbortError'; - if (isCurrentEpoch(ctx, requestEpoch) && !aborted) ctx.onTransportOffline?.(e); - throw e; - } - // The request may have crossed a sign-in/sign-out boundary while fetch was - // pending. Its response still belongs to its caller, but cannot change the - // new epoch's connection/auth state or start a refresh with its token. - if (!isCurrentEpoch(ctx, requestEpoch)) return resp; - // A 2xx confirms the credentials are good for the rest of the session. - if (resp.ok) { - ctx.authConfirmed = true; - ctx.onTransportConnected?.(); - } - let authExpired = resp.status === 401 || resp.status === 403; - if (!authExpired && !resp.ok) { - const peek = await resp.clone().text(); - // Reading an error body is another async boundary. If this request was - // superseded while it was pending, its expiry marker must not start a - // refresh against the replacement session's credentials. - if (!isCurrentEpoch(ctx, requestEpoch)) return resp; - if (isAuthExpiredBody(peek)) authExpired = true; - } - if (authExpired) { - // Once this session has authenticated successfully, the same credentials - // are still valid — so a later 401/403 is a *query-level* error ClickHouse - // maps to that HTTP status (ACCESS_DENIED, or UNKNOWN_USER from e.g. - // `SHOW CREATE USER `), not a sign-in problem. Return it so the - // caller shows it as a normal query error instead of force-logging-out. - if (ctx.authConfirmed) return resp; - if (attempt === 0 && (await ctx.refresh())) { - if (!isCurrentEpoch(ctx, requestEpoch)) throw staleEpochAbort(); - // A successful refresh always yields a fresh, usable token — the - // refresh() contract this seam relies on. - bearer = (await ctx.getToken())!; - if (!isCurrentEpoch(ctx, requestEpoch)) throw staleEpochAbort(); - attempt++; - continue; - } - if (!isCurrentEpoch(ctx, requestEpoch)) return resp; - // First-contact 401/403 with a non-expired token: CH rejected the login - // itself — an authorization/identity problem, not session expiry. Surface - // CH's own reason so it's diagnosable. - const reason = parseExceptionText(await resp.clone().text()); - if (!isCurrentEpoch(ctx, requestEpoch)) return resp; - ctx.onSignedOut(authDeniedMessage(resp.status, reason), requestEpoch); - throw new Error('signed out'); - } - return resp; - } -} - /** * Run a query and return parsed JSON (FORMAT JSON). Throws on CH error. `signal` * (optional) aborts the request. `extra` (optional) adds HTTP query-string * settings (e.g. `{ readonly: 2 }` for a read-only tile). `params` (optional) * adds `param_` query-string args for native ClickHouse query parameters * (#134) — omitted for every existing call site, so this is backward compatible. + * + * #630 Phase 6 — delegates to `authenticated-clickhouse-request.ts`'s + * `authenticatedJson()`, the first real production consumer of the package's + * JSON response consumer. Preserves this function's own EXISTING outward + * non-2xx behavior (a plain `Error` carrying CH's parsed exception message) + * by translating the package's `ClickHouseError` back to that shape — + * `authenticatedJson`'s `ClickHouseError.message` is itself derived from the + * same `parseExceptionText`, so the message text is unchanged; only the + * thrown error's class/identity is translated. Native JSON/body/network/abort + * errors are never `ClickHouseError` and propagate unchanged. (Phase 7 may + * later remove this translation as part of its broader consumer cutover.) */ export async function queryJson>( ctx: ChCtx, @@ -291,9 +161,12 @@ export async function queryJson>( extra?: Record, params?: Record, ): Promise> { - const resp = await authedFetch(ctx, { sql, defaultFormat: 'JSON', settings: extra, params, signal }); - if (!resp.ok) throw new Error(parseExceptionText(await resp.text())); - return resp.json(); + try { + return await authenticatedJson>(ctx, { sql, defaultFormat: 'JSON', settings: extra, params, signal }); + } catch (e) { + if (e instanceof ClickHouseError) throw new Error(e.message); + throw e; + } } /** @@ -311,7 +184,8 @@ export async function queryJson>( * latches so every later call on this connection (schema loads, table * expands, lineage BFS) goes straight to the plain query instead of paying a * doomed extra round trip forever — the same one-shot-then-remember shape as - * `ctx.authConfirmed` in `authedFetch`. + * `ctx.authConfirmed` in `authenticated-clickhouse-request.ts`'s + * `authenticatedRequest`. * * Any OTHER error (e.g. a per-table Iceberg/Glue metadata failure inside the * catalog itself — ClickHouse's `system.tables` aborts the whole query for a @@ -325,10 +199,10 @@ export async function queryJson>( * * Two error classes are rethrown immediately, before that check: a * caller-aborted signal (matching `tryQueryData`'s cancellation contract), and - * 'not signed in' / 'signed out' — `authedFetch` has already exhausted its own - * retry and called `ctx.onSignedOut()` for those, so retrying here would just - * repeat the whole token/refresh/sign-out handshake (and its side effects) a - * second time for no benefit. + * 'not signed in' / 'signed out' — `authenticatedRequest` (via `queryJson`) + * has already exhausted its own retry and called `ctx.onSignedOut()` for + * those, so retrying here would just repeat the whole token/refresh/sign-out + * handshake (and its side effects) a second time for no benefit. */ async function querySystemAware>(ctx: ChCtx, sqlBody: string, signal?: AbortSignal): Promise> { const plain = () => queryJson(ctx, sqlBody + '\nFORMAT JSON', signal); @@ -388,9 +262,10 @@ export async function killQuery(ctx: ChCtx, queryId: string | null | undefined, } /** Best-effort server cancellation through a frozen execution-scope lease. - * Unlike `killQuery`, this deliberately bypasses `authedFetch`: no token read, - * refresh, retry, lifecycle callback, or mutable auth-scheme lookup is allowed - * while a dead scope is closing. */ + * Unlike `killQuery`, this deliberately bypasses `authenticatedRequest` + * (`authenticated-clickhouse-request.ts`): no token read, refresh, retry, + * lifecycle callback, or mutable auth-scheme lookup is allowed while a dead + * scope is closing. */ export async function killQueryWithLease( lease: AuthenticatedCancellationLease, queryId: string | null | undefined, @@ -398,9 +273,9 @@ export async function killQueryWithLease( ): Promise { if (!queryId) return; try { - // A one-shot transport built directly from the frozen lease — never - // `transportFor(ctx)` / `authedFetch` — so cleanup reads no mutable auth, - // token, or refresh state (hard invariant 8). + // A one-shot transport built directly from the frozen lease — never the + // mutable-`ChCtx` `authenticatedRequest` — so cleanup reads no mutable + // auth, token, or refresh state (hard invariant 8/13). const transport = createHttpTransport({ fetch: () => lease.fetch, origin: () => lease.origin }); await transport.send({ sql: 'KILL QUERY WHERE query_id = ' + sqlString(queryId) + ' ASYNC', @@ -1039,7 +914,10 @@ export interface ExportQueryOptions { */ export async function exportQuery(ctx: ChCtx, sql: string, opts: ExportQueryOptions = {}): Promise { const { queryId, signal, format, params } = opts; - const resp = await authedFetch(ctx, { + // #630 Phase 6 — routes through the new module's raw `authenticatedRequest` + // entrypoint (was `authedFetch`); its own non-2xx parsing and successful + // raw-`Response` ownership below are unchanged (Phase 7 concern). + const resp = await authenticatedRequest(ctx, { sql, defaultFormat: format || 'TabSeparatedWithNames', params: { ...(queryId ? { query_id: queryId } : {}), ...(params || {}) }, @@ -1113,7 +991,11 @@ export async function runQuery(ctx: ChCtx, sql: string, o: RunQueryOptions = {}) const cap: Record = (o.resultRowLimit ?? 0) > 0 ? { max_result_rows: o.resultRowLimit!, result_overflow_mode: 'break' } : {}; - const resp = await authedFetch(ctx, { + // #630 Phase 6 — routes through the new module's raw `authenticatedRequest` + // entrypoint (was `authedFetch`); the Table/KPI/raw format mapping, row-cap + // settings, non-2xx parsing, and streaming below are unchanged (Phase 7 + // concern). + const resp = await authenticatedRequest(ctx, { sql, defaultFormat: fmtParam, // wait_end_of_query buffers the whole response server-side so the HTTP @@ -1136,11 +1018,10 @@ export async function runQuery(ctx: ChCtx, sql: string, o: RunQueryOptions = {}) return { raw: await resp.text() }; } // 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). + // than through a ChCtx-based transport's own stream member (retired that + // phase): this module is itself under `src/net/**` (the one layer allowed + // to import the package by bare specifier), and 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 5d2e96da..da2add59 100644 --- a/src/net/clickhouse-http-transport.ts +++ b/src/net/clickhouse-http-transport.ts @@ -16,10 +16,17 @@ // production stream implementation in the repository now — the package's; // this file does not reintroduce a second one, forwarding or otherwise. // +// Issue #630 Phase 6 — this adapter's one remaining production caller is +// `killQueryWithLease`'s frozen-lease bypass (`ch-client.ts`); the normal +// mutable-`ChCtx` request path moved to +// `src/net/authenticated-clickhouse-request.ts`, which builds the package +// client directly rather than through this adapter. +// // 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. +// `ch-client.ts`, `authenticated-clickhouse-request.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, TransportDeps, TransportRequest } from './clickhouse-transport.types.js'; diff --git a/src/net/clickhouse-transport.types.ts b/src/net/clickhouse-transport.types.ts index efe783c9..d8d18004 100644 --- a/src/net/clickhouse-transport.types.ts +++ b/src/net/clickhouse-transport.types.ts @@ -23,12 +23,20 @@ // stream implementation in the repository (the package's); this seam no // longer describes one. // +// Issue #630 Phase 6 — the normal-request auth/epoch/refresh/lifecycle +// policy moved out of `ch-client.ts` into +// `src/net/authenticated-clickhouse-request.ts`; this contract's own +// boundary is unaffected (this file never described that policy), but the +// forbidden-owner list below now names the new module too, since it is the +// current auth-policy owner this transport-leaf contract must not reach. +// // Ownership boundary: this file (and its implementation, // `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). +// `ch-client.ts`, `authenticated-clickhouse-request.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'; @@ -42,12 +50,19 @@ import type { ClickHouseHttpClientDeps, ClickHouseHttpRequest } from '@altinity/ * transport must observe the current value per request. * * REQUIRED-PURE: both accessors must be synchronous, side-effect-free plain - * property reads (production: `() => ctx.fetch` / `() => ctx.origin`). This - * matters because `send` evaluates them AFTER `authedFetch`'s final epoch - * fence and before the fetch itself; the type system cannot express purity, - * 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). */ + * property reads (production: `() => ctx.fetch` / `() => ctx.origin`, or — + * for `killQueryWithLease`, this contract's one remaining production + * caller since #630 Phase 6 — `() => lease.fetch` / `() => lease.origin`). + * This matters because `send` evaluates them immediately before the fetch + * itself; the type system cannot express purity, 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). (Until #630 Phase 6, this same + * accessor timing mattered relative to `ch-client.ts`'s own `authedFetch` + * final epoch fence; that normal-request caller now builds its package + * client directly in `src/net/authenticated-clickhouse-request.ts` instead + * of going through this contract at all — see that module's own final-fence + * comment.) */ export type TransportDeps = ClickHouseHttpClientDeps; /** One ClickHouse HTTP request, fully specified. No client-level defaults @@ -63,10 +78,12 @@ 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 -// the direction Response -> subset, so authedFetch/exportQuery could not keep -// their `Promise` signatures without an unsafe cast. Native Response -// gives raw bytes (`body`, hard invariant 17) and `clone()` for authedFetch's -// non-destructive error-body peek for free. +// the direction Response -> subset, so a caller needing the real Response +// (killQueryWithLease today; `authedFetch`/`exportQuery` before #630 Phase 6 +// moved the normal-request path off this contract) could not keep a +// `Promise` signature without an unsafe cast. Native Response gives +// raw bytes (`body`, hard invariant 17) and `clone()` for a non-destructive +// error-body peek for free. /** 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 @@ -77,19 +94,24 @@ export type TransportRequest = ClickHouseHttpRequest; export interface ClickHouseTransport { /** POST one query; resolves at HTTP settlement (headers received) with the * NATIVE fetch `Response` — Phase 1 defines no adapter-owned response type - * (Adaptation A3), which is what preserves `authedFetch`/`exportQuery`'s - * `Promise` signatures and `export-service.ts`'s - * `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 normally (they are - * responses); network I/O failure / abort rejects the returned promise - * natively. Since #630 Phase 2, `send` is implemented by delegating to + * (Adaptation A3), which is what preserves `export-service.ts`'s + * `streamToFile(resp: Response, …)` consumer without casts, and — since + * #630 Phase 6 — what lets `killQueryWithLease` (this contract's one + * remaining production caller) keep its own `Promise` best-effort + * wrapper without a cast either. 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 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. */ + * the caller (before #630 Phase 6, `ch-client.ts`'s own `authedFetch`; the + * normal-request path now builds the package client directly in + * `src/net/authenticated-clickhouse-request.ts` instead, never through + * this contract). */ send(request: TransportRequest): Promise; } diff --git a/tests/e2e/clickhouse-http-transport.html b/tests/e2e/clickhouse-http-transport.html index de186b29..c8490e7b 100644 --- a/tests/e2e/clickhouse-http-transport.html +++ b/tests/e2e/clickhouse-http-transport.html @@ -26,9 +26,10 @@ #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. + directly, exercising the same call path `ch-client.ts`'s `runQuery` + used through #630 Phase 5 (superseded by the authenticated path since + Phase 6 — see below) — no additional import-map entry is needed, + since the specifier is already mapped above. #630 Phase 4 — Scenario 9 (new, additive) exercises the package's own `createClickHouseHttpClient(...).queryProgress()` convenience method @@ -38,9 +39,34 @@ semantics (one real Fetch, the caller's own AbortSignal driving cancellation for the response's whole lifetime, no callbacks after rejection). Scenarios 1-8 are otherwise UNCHANGED: they still exercise - the still-live SQL Browser production composition - (`createHttpTransport().send()` -> package `streamLines`), which - remains the production path until Phase 7's cutover. --> + the raw `createHttpTransport().send()` -> package `streamLines` + composition directly, which was the ordinary SQL Browser production + path through #630 Phase 5. Since Phase 6, the actual production path + for `queryJson`/`runQuery`/`exportQuery` is the authenticated-path + composition below (`authenticatedRequest()`/`authenticatedProgress()` + -> package `request()`/response consumers) — `createHttpTransport` + itself now remains live only as the frozen-lease `killQueryWithLease` + bypass's compatibility route. Scenarios 1-8 stay as lower-layer + package/transport regression coverage; they are not claimed to + exercise the current ordinary production path. + + #630 Phase 6 — authenticated-path variants of the post-header + cancellation scenarios (5-9), proving the SAME native + Fetch/Response/cancellation semantics survive being driven through + SQL Browser's own credential/epoch composition + (`/src/net/authenticated-clickhouse-request.js`, raw ESM, same + mixed-tree .ts->.js serving as every other `/src/**` import here) — + not just the compatibility transport/package client with an + already-resolved Authorization. Each `*Auth` scenario below builds one + real-production-shaped `AuthenticatedRequestCtx` (`makeAuthCtx`) with + synthetic test credentials, one deterministic epoch, `refresh()` + disabled (returns `false`) except where a scenario needs a genuine + 401-then-retry, and captures `onTransportConnected`/ + `onTransportOffline`/`onSignedOut` calls — then passes each scenario's + original `AbortController.signal` straight through, exactly like the + raw scenarios above. Scenarios 1-4 stay raw/unauthenticated + (pre-header timing — optional to duplicate through auth per the plan); + only the post-header family (5-9) gets an authenticated variant. -->