diff --git a/src/config.rs b/src/config.rs index 6a4224a..c020f2e 100644 --- a/src/config.rs +++ b/src/config.rs @@ -484,11 +484,10 @@ mod tests { #[test] fn legacy_api_key_in_yaml_is_ignored_on_load() { - // Older configs (pre-jwt-branch) had `api_key: hd_xxx` written - // to disk. After the migration, the api_key field is purely - // transient — `#[serde(skip)]` must drop any value present in - // YAML on load. This pins down the migration behavior so a - // stale entry can't silently reappear in profile.api_key. + // Some config files on disk carry `api_key: hd_xxx`. The api_key field + // is transient (`#[serde(skip)]`), so any value present in the YAML must + // be dropped on load — a stale entry must not silently reappear in + // profile.api_key. let (_tmp, _guard) = with_temp_config_dir(); let path = config_path().unwrap(); fs::create_dir_all(path.parent().unwrap()).unwrap(); diff --git a/src/connections.rs b/src/connections.rs index 5b989bc..cb951c0 100644 --- a/src/connections.rs +++ b/src/connections.rs @@ -34,8 +34,8 @@ impl Serialize for HealthStatus { } } -/// Render an [`ApiError`] the way the old raw `fetch_health` path did: the -/// server body through `api_error`, or the transport message verbatim. +/// Render an [`ApiError`] as text: the server body through `api_error`, or the +/// transport message verbatim. fn error_text(e: ApiError) -> String { match e { ApiError::Status { body, .. } => crate::util::api_error(body), @@ -138,8 +138,7 @@ pub fn types_get(workspace_id: &str, name: &str, format: &str) { let api = Api::new(Some(workspace_id)); let resp = block(api.client().connection_types().get(name)).unwrap_or_else(|e| e.exit()); // The SDK models nullable fields as `Option>`; flatten and - // drop an explicit JSON `null` to match the old behavior (the old struct - // deserialized a missing/`null` field to `None`). + // drop an explicit JSON `null` so a missing or null field renders as absent. let detail = ConnectionTypeDetail { name: resp.name, label: resp.label, diff --git a/src/connections_new.rs b/src/connections_new.rs index be9465b..7f61fe2 100644 --- a/src/connections_new.rs +++ b/src/connections_new.rs @@ -30,7 +30,7 @@ fn fetch_types(api: &Api) -> Vec { fn fetch_detail(api: &Api, name: &str) -> ConnectionTypeDetail { let detail = block(api.client().connection_types().get(name)).unwrap_or_else(|e| e.exit()); // The SDK models nullable fields as `Option>`; flatten and - // treat an explicit JSON `null` as absent to match the old `is_null()` check. + // treat an explicit JSON `null` as absent. let flatten = |field: Option>| -> Option { field.flatten().filter(|v| !v.is_null()) }; @@ -317,8 +317,8 @@ pub fn run(workspace_id: &str) { Unavailable(String), } - /// Render an [`ApiError`] the way the old raw paths did: the server body - /// through `api_error`, or the transport message verbatim. + /// Render an [`ApiError`] as text: the server body through `api_error`, or + /// the transport message verbatim. fn error_text(e: ApiError) -> String { match e { ApiError::Status { body, .. } => crate::util::api_error(body), diff --git a/src/context.rs b/src/context.rs index 7a4c035..c8775c3 100644 --- a/src/context.rs +++ b/src/context.rs @@ -119,7 +119,7 @@ fn local_md_path(name: &str) -> PathBuf { } /// Fetch a named context document. Returns `Ok(None)` on 404 (not found); -/// exits the process on any other error, matching the old behavior. +/// exits the process on any other error. fn fetch_context(api: &Api, database_id: &str, name: &str) -> Option { let result = crate::sdk::block(api.client().database_context().get(database_id, name)); match crate::sdk::none_if_404(result) { diff --git a/src/databases.rs b/src/databases.rs index b4f7de1..1aa9aff 100644 --- a/src/databases.rs +++ b/src/databases.rs @@ -131,8 +131,8 @@ impl From for DatabaseSummary { /// Fetch a database by id through the SDK's typed `databases().get` handle. /// /// The handle owns auth, scope headers, URL construction, and percent-encoding -/// of the id segment, so callers no longer hand-roll the path. The result is -/// mapped into the CLI's `Database`. +/// of the id segment, so the caller passes a bare id. The result is mapped into +/// the CLI's `Database`. pub(crate) fn get_database(api: &Api, id: &str) -> Result { block(api.client().databases().get(id)).map(Database::from) } @@ -579,9 +579,8 @@ fn mint_database_token(api: &Api, database_id: &str) -> DatabaseTokenResponse { .post_raw("/auth/database", &body) .unwrap_or_else(|e| e.exit()); if !status.is_success() { - // The old typed `api.post` routed non-success through `fail_response`, - // which upgrades a masked 401/403/404 into the re-auth hint. Reproduce - // that via the seam's auth-aware exit. + // Route non-success through the seam's auth-aware exit, which upgrades a + // masked 401/403/404 into the re-auth hint. crate::sdk::ApiError::Status { status, body: resp_body, diff --git a/src/jwt.rs b/src/jwt.rs index c9e3b60..a62ea08 100644 --- a/src/jwt.rs +++ b/src/jwt.rs @@ -368,11 +368,11 @@ pub fn ensure_access_token( /// Which credential source the [`CliTokenProvider`] serves bearers from. /// -/// Mirrors the 4-level auth-source precedence the old `ApiClient::new` -/// applied (database env -> sandbox env -> on-disk sandbox session -> -/// user session/api_key). The wrapper (`src/sdk.rs`) picks the variant at -/// construction time; the provider re-runs the corresponding *existing* -/// blocking CLI function on every request so session.json, the 30s leeway +/// Carries the 4-level auth-source precedence (database env -> sandbox env -> +/// on-disk sandbox session -> user session/api_key). The wrapper (`src/sdk.rs`) +/// picks the variant at construction time; the provider re-runs the +/// corresponding blocking CLI function on every request so session.json, the +/// 30s leeway /// table, no-clobber for Flag/Env, and clear-on-dead-refresh stay owned by /// the CLI — the SDK never re-implements JWT exchange. #[derive(Debug, Clone)] @@ -446,8 +446,7 @@ impl hotdata::auth::BearerTokenProvider for CliTokenProvider { resolved.map_err(|body| { // Surface as a 401 so `Configuration::resolve_bearer_token` logs the // cause and the request proceeds to a 401 the wrapper shapes into - // the "run hotdata auth" hint (the same end-state as the old - // ApiClient refresher returning None). + // the "run hotdata auth" hint. hotdata::auth::TokenExchangeError::Status { status: 401, body } }) } diff --git a/src/sandbox.rs b/src/sandbox.rs index dad2a71..5dc76f1 100644 --- a/src/sandbox.rs +++ b/src/sandbox.rs @@ -25,11 +25,10 @@ fn now_unix() -> u64 { /// Mint (or re-mint) a sandbox-scoped JWT via `POST /v1/auth/sandbox`. /// -/// This token-mint endpoint has no SDK operation, so it stays on the raw seam. -/// [`Api::post_raw`] still carries the user bearer + `X-Workspace-Id` like every -/// SDK call. Reproduces the old `ApiClient::post` behavior exactly: a transport -/// error or a non-success status prints the standard error and exits, and a -/// malformed body exits the same way `parse_json` did. +/// This token-mint endpoint has no SDK operation, so it uses the raw seam. +/// [`Api::post_raw`] carries the user bearer + `X-Workspace-Id` like every SDK +/// call. A transport error or a non-success status prints the standard error +/// and exits; a malformed body exits the same way. fn mint_sandbox_token(api: &Api, body: &serde_json::Value) -> SandboxTokenResponse { let (status, resp_body) = api .post_raw("/auth/sandbox", body) @@ -191,8 +190,8 @@ pub fn new(workspace_id: &str, name: Option<&str>, format: &str) { // POST /auth/sandbox creates the sandbox AND mints a sandbox-scoped // JWT (+ refresh token) in one round-trip. This token-mint endpoint has - // no SDK operation, so it stays on the raw seam (which still carries the - // user bearer + X-Workspace-Id like every SDK call). + // no SDK operation, so it uses the raw seam (which carries the user bearer + // + X-Workspace-Id like every SDK call). let resp = mint_sandbox_token(&api, &body); let sandbox_id = resp.sandbox_id.clone(); persist_sandbox_session(resp, workspace_id); diff --git a/src/sdk.rs b/src/sdk.rs index 0001946..d82b342 100644 --- a/src/sdk.rs +++ b/src/sdk.rs @@ -1,9 +1,8 @@ //! Synchronous wrapper over the async Hotdata Rust SDK. //! -//! This module is the seam that replaces the hand-rolled legacy -//! `ApiClient`. The 15 command modules stay -//! synchronous and call [`Api`] methods; [`Api`] drives the async SDK behind a -//! process-global multi-thread tokio runtime via `block_on`. +//! This module is the seam between the CLI and the SDK. The 15 command modules +//! stay synchronous and call [`Api`] methods; [`Api`] drives the async SDK +//! behind a process-global multi-thread tokio runtime via `block_on`. //! //! # Concurrency contract //! @@ -17,8 +16,8 @@ //! //! # Auth //! -//! Construction reproduces the old `ApiClient::new` 4-level auth-source -//! precedence by choosing the [`AuthMode`](crate::jwt::AuthMode) the installed +//! Construction follows a 4-level auth-source precedence, choosing the +//! [`AuthMode`](crate::jwt::AuthMode) the installed //! [`CliTokenProvider`](crate::jwt::CliTokenProvider) will serve. The provider //! returns a ready CLI-minted JWT (`client_id=hotdata-cli`, `/o/token/`), which //! the SDK passes through unchanged; the CLI keeps full ownership of @@ -69,11 +68,12 @@ pub struct Api { database_id: Option, } -/// Request timeout for SDK-routed calls. Mirrors the old `ApiClient` so a hung -/// server cannot stall the CLI indefinitely. The streaming `/files` upload -/// keeps its own no-timeout client on the raw-HTTP path. +/// Request timeout for SDK-routed calls, so a hung server cannot stall the CLI +/// indefinitely. The streaming `/files` upload runs on its own no-timeout +/// client ([`upload_reqwest_client`]), since a large upload legitimately +/// outlives this cap. const HTTP_REQUEST_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(300); -/// TCP keepalive probe interval, matching the old client. +/// TCP keepalive probe interval. const TCP_KEEPALIVE_INTERVAL: std::time::Duration = std::time::Duration::from_secs(30); /// Build the `reqwest::Client` backing every SDK call, with a request timeout + @@ -152,8 +152,8 @@ const _: fn() = || { /// SDK -> CLI error after mapping an `Error`. /// -/// Carries enough to reproduce the old `fail_response` behavior: the HTTP -/// status and a printable body, or a transport/parse description. +/// Carries enough for the CLI's failure rendering: the HTTP status and a +/// printable body, or a transport/parse description. #[derive(Debug)] pub enum ApiError { /// The server returned a non-success status. @@ -170,8 +170,8 @@ impl ApiError { /// /// `ResponseError` carries the HTTP status + raw body the CLI's /// `format_fail_message` consumes; everything else collapses to a - /// transport description (the old "error connecting to API" / "error - /// parsing response" paths). + /// transport description (the "error connecting to API" / "error parsing + /// response" messages). pub fn from_sdk(err: Error) -> Self { match err { Error::ResponseError(ResponseContent { @@ -253,8 +253,8 @@ where /// Map a result, returning `Ok(None)` on HTTP 404 instead of an error. /// -/// Reproduces `ApiClient::get_none_if_not_found` / the context-404 / indexes-404 -/// semantics: a missing resource is normal for these probes. +/// Used by the context-404 / indexes-404 probes, where a missing resource is +/// normal rather than an error. pub fn none_if_404(r: Result) -> Result, ApiError> { match r { Ok(v) => Ok(Some(v)), @@ -317,10 +317,9 @@ async fn apply_seam_headers( } impl Api { - /// Build an [`Api`], reproducing `ApiClient::new`'s auth-source precedence - /// by selecting the [`AuthMode`] the installed provider will serve. Exits - /// with a diagnostic if config can't load or no usable credential exists, - /// matching the old startup behavior. + /// Build an [`Api`], selecting the [`AuthMode`] the installed provider will + /// serve from the auth-source precedence below. Exits with a diagnostic if + /// config can't load or no usable credential exists. pub fn new(workspace_id: Option<&str>) -> Self { let profile_config = match config::load("default") { Ok(c) => c, @@ -331,15 +330,15 @@ impl Api { }; let api_url = profile_config.api_url.to_string(); - // Auth-source precedence (verbatim from the old ApiClient::new): + // Auth-source precedence: // 1. HOTDATA_DATABASE_TOKEN env (databases run child) // 2. HOTDATA_SANDBOX_TOKEN env (sandbox run child) // 3. ~/.hotdata/sandbox_session.json present (sandbox set ) // 4. ~/.hotdata/session.json + optional api_key fallback // - // We pre-flight the same way the old client did (so a dead/unusable - // credential exits at startup with the right hint), then hand the - // CliTokenProvider the matching mode to re-resolve on every request. + // Pre-flight the chosen source here (so a dead/unusable credential + // exits at startup with the right hint), then hand the CliTokenProvider + // the matching mode to re-resolve on every request. let mode = if std::env::var("HOTDATA_DATABASE_TOKEN").is_ok() { if crate::database_session::refresh_from_env(&api_url).is_none() { eprintln!( @@ -400,8 +399,8 @@ impl Api { } }; - // Resolve the sandbox/session id exactly as the old ApiClient::new did: - // HOTDATA_SANDBOX wins; otherwise, if we are a descendant of a + // Resolve the sandbox/session id: HOTDATA_SANDBOX wins; otherwise, if + // we are a descendant of a // `sandbox run` whose sandbox context was lost, exit (a restart is // required); else fall back to the persisted sandbox in config. This id // is sent as X-Session-Id to scope requests to the sandbox. @@ -439,8 +438,7 @@ impl Api { base_path: sdk_base_path(api_url), client: sdk_http_client(), // Attribute CLI traffic as the CLI, not the SDK default - // (`hotdata-rust/...`). The old ApiClient sent no User-Agent; an - // explicit CLI agent is the correct attribution. + // (`hotdata-rust/...`), so server-side request logs name the CLI. user_agent: Some(format!("hotdata-cli/{}", env!("CARGO_PKG_VERSION"))), ..Configuration::default() }; @@ -681,10 +679,9 @@ impl Api { /// `Configuration`, returning the raw status + body text. /// /// The seam's DELETE counterpart to [`post_raw`](Self::post_raw): used by - /// `databases.rs`, where the delete bodies feed the same CLI-side - /// `(status, body)` control flow as the old raw `delete_raw` (e.g. the - /// delete+recreate path inspects the failure body), so non-success is - /// returned as `Ok((status, body))` rather than an error. + /// `databases.rs`, where the delete+recreate path inspects the failure + /// body, so non-success is returned as `Ok((status, body))` rather than an + /// error for the caller's `(status, body)` control flow. pub fn delete_raw(&self, path: &str) -> Result<(reqwest::StatusCode, String), ApiError> { let cfg = self.client.configuration(); let url = format!("{}/v1{path}", cfg.base_path); @@ -733,8 +730,6 @@ impl Api { /// Decide what error text to print for a failed response. Pure function so the /// 4xx-to-re-auth-hint heuristic is unit-testable without HTTP or `exit`. -/// -/// Relocated verbatim from the old `api.rs`. pub fn format_fail_message( status: reqwest::StatusCode, body: &str, @@ -753,7 +748,7 @@ mod tests { use super::*; use auth::AuthStatus; - // --- format_fail_message: ported verbatim from api.rs (9 cases) ---------- + // --- format_fail_message (9 cases) --------------------------------------- #[test] fn format_fail_message_401_with_invalid_key_shows_reauth_hint() { @@ -919,8 +914,8 @@ mod tests { #[test] fn workspace_id_header_is_installed_on_scoped_calls() { - // Regression for the old api.rs:598 header assertion. `datasets().list` - // carries the X-Workspace-Id api_key; assert it reaches the wire. + // `datasets().list` carries the X-Workspace-Id api_key; assert it + // reaches the wire. let mut server = mockito::Server::new(); let m = server .mock("GET", "/v1/datasets") @@ -1076,8 +1071,8 @@ mod tests { #[test] fn post_raw_returns_error_status_without_mapping_to_err() { - // Non-success is returned as Ok((status, body)) so the caller reproduces - // the old `(status, body)` raw-post control flow verbatim. + // Non-success is returned as Ok((status, body)) so the caller can drive + // its own `(status, body)` control flow (e.g. inspect the failure body). let mut server = mockito::Server::new(); let _m = server .mock("POST", "/v1/refresh") @@ -1247,9 +1242,8 @@ mod tests { #[test] fn database_scope_sends_x_database_id_on_raw_calls() { - // Regression: `hotdata query --database X` must scope the request. The - // old ApiClient sent X-Database-Id on every request; the seam must too, - // and the raw /query submit path is where the scope is applied. + // `hotdata query --database X` must scope the request: the raw /query + // submit path is where X-Database-Id is applied. let mut server = mockito::Server::new(); let m = server .mock("POST", "/v1/query") diff --git a/src/tables.rs b/src/tables.rs index 7015e09..fe4842c 100644 --- a/src/tables.rs +++ b/src/tables.rs @@ -38,8 +38,8 @@ pub fn list( ) { let api = Api::new(Some(workspace_id)); - // The CLI only requests columns when a connection is specified, matching - // the old behavior (include_columns=true iff connection_id is set). + // Request columns only when a connection is specified + // (include_columns=true iff connection_id is set). let include_columns = connection_id.map(|_| true); let body = crate::sdk::block(api.client().information_schema().get(