From b14e54ac0aa8d9544e050284ec5c5130d936c33c Mon Sep 17 00:00:00 2001 From: Eddie A Tejeda <669988+eddietejeda@users.noreply.github.com> Date: Thu, 27 Aug 2026 14:28:58 -0700 Subject: [PATCH 1/7] fix(databases): hide internal connection ids from all output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Connections are an internal concept now that ingest creates catalogs, but `databases show` still printed a raw `conn…` id as "catalog id:" and listed attachments by connection id, and show/create/fork exposed `default_connection_id` in -o json/yaml. - `databases show` (table): drop the "catalog id:" line; render attachments by the catalog name they're reachable as (alias, else the connection's name via one connections-list call, skipped when every attachment is aliased), never the raw id - `databases show`/`create`/`fork` (json/yaml): stop serializing `default_connection_id` and `attachments[].connection_id`; attachments expose `catalog` instead - keep the fields internally (skip_serializing) — the managed load/delete paths, the detach-by-alias fallback, and the connection resolver still need them --- src/commands/databases.rs | 188 ++++++++++++++++++++++++++++++++++---- 1 file changed, 169 insertions(+), 19 deletions(-) diff --git a/src/commands/databases.rs b/src/commands/databases.rs index 868d9d3..c78b7c9 100644 --- a/src/commands/databases.rs +++ b/src/commands/databases.rs @@ -386,6 +386,11 @@ pub struct Database { pub name: Option, #[serde(default)] pub default_catalog: Option, + /// Internal handle for the database's own catalog — used to build the + /// managed-load/delete API paths and by the connection resolver. Connections + /// are an internal concept now (ingest creates catalogs), so it's never + /// serialized into `-o json`/`-o yaml` output. + #[serde(skip_serializing)] pub default_connection_id: String, #[serde(default)] pub expires_at: Option, @@ -397,6 +402,15 @@ pub struct Database { #[derive(Clone, Debug, Serialize, PartialEq, Eq)] struct DatabaseAttachment { + /// Catalog name this attachment is reachable as inside the database — the + /// alias when set, else the connection's name. `None` until resolved by + /// [`resolve_attachment_catalogs`] (only `get` displays attachments, so + /// only it pays the lookup). + catalog: Option, + /// Internal handle, kept for the detach-by-alias fallback and the resolver; + /// never shown — connections are an internal concept now (ingest creates + /// catalogs). + #[serde(skip_serializing)] connection_id: String, alias: Option, } @@ -425,6 +439,10 @@ struct CreateDatabaseResponse { name: Option, #[serde(default)] default_catalog: Option, + /// Internal handle (see [`Database::default_connection_id`]): deserialized + /// from the create/fork response for the managed-load path, never emitted + /// into `-o json`/`-o yaml` output. + #[serde(skip_serializing)] default_connection_id: String, #[serde(default)] expires_at: Option, @@ -452,9 +470,16 @@ impl From for Database { attachments: d .attachments .into_iter() - .map(|a| DatabaseAttachment { - connection_id: a.connection_id, - alias: a.alias.flatten(), + .map(|a| { + let alias = a.alias.flatten(); + DatabaseAttachment { + // The alias is the reachable catalog name when set; a + // non-aliased attachment needs a connections lookup + // (resolve_attachment_catalogs) to learn its name. + catalog: alias.clone(), + connection_id: a.connection_id, + alias, + } }) .collect(), } @@ -1121,9 +1146,37 @@ pub fn count(workspace_id: &str, format: &str) { } } +/// Fill each attachment's display `catalog` name: the alias when set (already +/// stamped by the `From` mapping), else the connection's name from one +/// `connections list` call. Skips the call entirely when every attachment is +/// aliased (or there are none). A connection the listing doesn't cover (or a +/// failed listing) falls back to the raw id — a degraded but still usable +/// handle for `databases detach`. +fn resolve_attachment_catalogs(api: &Api, db: &mut Database) { + if db.attachments.iter().all(|a| a.catalog.is_some()) { + return; + } + let names: std::collections::HashMap = + match block(api.client().connections().list()) { + Ok(resp) => resp.connections.into_iter().map(|c| (c.id, c.name)).collect(), + Err(_) => Default::default(), + }; + for a in &mut db.attachments { + if a.catalog.is_none() { + a.catalog = Some( + names + .get(&a.connection_id) + .cloned() + .unwrap_or_else(|| a.connection_id.clone()), + ); + } + } +} + pub fn get(workspace_id: &str, id_or_name: &str, format: &str) { let api = Api::new(Some(workspace_id)); - let db = resolve_database(&api, id_or_name); + let mut db = resolve_database(&api, id_or_name); + resolve_attachment_catalogs(&api, &mut db); match format { "json" => println!("{}", serde_json::to_string_pretty(&db).unwrap()), @@ -1145,11 +1198,6 @@ pub fn get(workspace_id: &str, id_or_name: &str, format: &str) { crate::util::format_date(ts).dark_grey() ); } - println!( - "{}{}", - label("catalog id:"), - db.default_connection_id.clone().dark_cyan() - ); let catalog = db .default_catalog .as_deref() @@ -1163,16 +1211,11 @@ pub fn get(workspace_id: &str, id_or_name: &str, format: &str) { if !db.attachments.is_empty() { println!("{}({})", label("attached catalogs:"), db.attachments.len()); for a in &db.attachments { - let alias = a - .alias - .as_deref() - .map(|al| format!(" as {al}")) - .unwrap_or_default(); - println!( - " {}{}", - a.connection_id.clone().dark_cyan(), - alias.dark_grey() - ); + // The resolved catalog name is what the attachment is + // reachable as in SQL (and what detach accepts) — the + // internal connection id is deliberately not shown. + let name = a.catalog.as_deref().unwrap_or("(unknown)"); + println!(" {}", name.cyan()); } } } @@ -2133,6 +2176,113 @@ mod tests { ) } + /// A `Database` for serialization tests, with one aliased and one bare + /// attachment. + fn db_with_attachments() -> Database { + Database { + id: "db_1".to_string(), + name: Some("sales".to_string()), + default_catalog: Some("sales".to_string()), + default_connection_id: "conn_own".to_string(), + expires_at: None, + created_at: None, + attachments: vec![ + DatabaseAttachment { + catalog: Some("gh".to_string()), + connection_id: "conn_a".to_string(), + alias: Some("gh".to_string()), + }, + DatabaseAttachment { + catalog: None, + connection_id: "conn_b".to_string(), + alias: None, + }, + ], + } + } + + #[test] + fn database_json_output_hides_connection_ids() { + // Connections are internal now (ingest creates catalogs): neither the + // database's own connection id nor an attachment's may reach `-o json`. + let json = serde_json::to_value(db_with_attachments()).unwrap(); + assert!(json.get("default_connection_id").is_none(), "json: {json}"); + let attachments = json["attachments"].as_array().unwrap(); + for a in attachments { + assert!(a.get("connection_id").is_none(), "attachment: {a}"); + } + // The reachable catalog name is what's exposed instead. + assert_eq!(attachments[0]["catalog"], serde_json::json!("gh")); + } + + #[test] + fn create_response_json_output_hides_connection_id() { + let result = CreateDatabaseResponse { + id: "db_new".to_string(), + name: Some("mydb".to_string()), + default_catalog: Some("default".to_string()), + default_connection_id: "conn_abc".to_string(), + expires_at: None, + }; + let json = serde_json::to_value(&result).unwrap(); + assert!(json.get("default_connection_id").is_none(), "json: {json}"); + } + + #[test] + fn resolve_attachment_catalogs_fills_names_from_connections_list() { + let mut server = mockito::Server::new(); + let list = server + .mock("GET", "/v1/connections") + .match_query(mockito::Matcher::Any) + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"connections":[{"id":"conn_b","name":"github","source_type":"postgres"}]}"#, + ) + .create(); + + let api = Api::test_new(&server.url(), "k", Some("ws")); + let mut db = db_with_attachments(); + resolve_attachment_catalogs(&api, &mut db); + + // Aliased attachment keeps its alias; the bare one gets the listed name. + assert_eq!(db.attachments[0].catalog.as_deref(), Some("gh")); + assert_eq!(db.attachments[1].catalog.as_deref(), Some("github")); + list.assert(); + } + + #[test] + fn resolve_attachment_catalogs_skips_lookup_when_all_aliased() { + // Every attachment already has a catalog name → no connections call. + // Point at a server with no mocks so a stray call fails loudly. + let server = mockito::Server::new(); + let api = Api::test_new(&server.url(), "k", Some("ws")); + let mut db = db_with_attachments(); + db.attachments.remove(1); // keep only the aliased one + resolve_attachment_catalogs(&api, &mut db); + assert_eq!(db.attachments[0].catalog.as_deref(), Some("gh")); + } + + #[test] + fn resolve_attachment_catalogs_falls_back_to_id_when_unlisted() { + // The listing doesn't cover the connection (e.g. database-scoped, or the + // call failed): degrade to the raw id — still a usable detach handle. + let mut server = mockito::Server::new(); + let list = server + .mock("GET", "/v1/connections") + .match_query(mockito::Matcher::Any) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"connections":[]}"#) + .create(); + + let api = Api::test_new(&server.url(), "k", Some("ws")); + let mut db = db_with_attachments(); + resolve_attachment_catalogs(&api, &mut db); + assert_eq!(db.attachments[1].catalog.as_deref(), Some("conn_b")); + list.assert(); + } + #[test] fn resolve_database_by_id_and_name() { let mut server = mockito::Server::new(); From 968baa1576275a83554d1dcaecf351036883baac Mon Sep 17 00:00:00 2001 From: Eddie A Tejeda <669988+eddietejeda@users.noreply.github.com> Date: Thu, 27 Aug 2026 14:30:10 -0700 Subject: [PATCH 2/7] fix(cli): accept database names and catalogs everywhere --database takes an id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `-d/--database` flags on query, query status, queries, results, context, and search show/remove passed their value raw into the X-Database-Id header (or a database-scoped path), so `-d ` failed with a server "not found" while `databases show ` resolved it fine. `databases use ` failed the same way. - new `resolve_database_flag`: a `dbid…` value passes through untouched (no extra round trip on the common path); a catalog or name resolves to its id first, so names work everywhere ids do - extract load's active-database disambiguation into `try_resolve_database_preferring_active` and use it for every catalog/name lookup: when several databases share a catalog (e.g. `default` across create/fork), the active one wins instead of erroring as ambiguous - `search create --from .…` now applies that same preference — it previously errored on an ambiguous catalog even when the active database's own catalog matched (and its error suggested setting an active database, which didn't actually help) - `databases use ` resolves names/catalogs; ids keep the 403-tolerant existence check so database API tokens still work - error-message grammar: "pass an instant database's catalog" --- src/commands/databases.rs | 196 +++++++++++++++++++++++++++++++++----- src/commands/queries.rs | 6 +- src/commands/query.rs | 15 +-- src/commands/results.rs | 6 +- src/commands/search.rs | 18 +++- src/main.rs | 23 +++-- 6 files changed, 215 insertions(+), 49 deletions(-) diff --git a/src/commands/databases.rs b/src/commands/databases.rs index c78b7c9..82d7160 100644 --- a/src/commands/databases.rs +++ b/src/commands/databases.rs @@ -634,6 +634,61 @@ pub fn resolve_database(api: &Api, id_or_name: &str) -> Database { } } +/// Like [`try_resolve_database`], but prefer the API's ambient database scope +/// (`HOTDATA_DATABASE` / current database) when its catalog or name matches +/// `key`. This disambiguates the common case where several databases share a +/// catalog (e.g. `default` across create/fork): the one the user is working in +/// wins instead of the lookup erroring as ambiguous. A key that names neither +/// the active database's catalog nor its name resolves like any other lookup. +pub fn try_resolve_database_preferring_active(api: &Api, key: &str) -> Result { + if let Some(active_id) = api.database_id().filter(|id| *id != key) + && let Some(active) = + none_if_404(get_database(api, active_id)).unwrap_or_else(|e| e.exit()) + && (active.default_catalog.as_deref() == Some(key) || active.name.as_deref() == Some(key)) + { + return Ok(active); + } + try_resolve_database(api, key) +} + +/// Exiting wrapper around [`try_resolve_database_preferring_active`], mirroring +/// [`resolve_database`]. +pub fn resolve_database_preferring_active(api: &Api, key: &str) -> Database { + match try_resolve_database_preferring_active(api, key) { + Ok(db) => db, + Err(e) => { + use crossterm::style::Stylize; + eprintln!("{}", format!("error: {e}").red()); + std::process::exit(1); + } + } +} + +/// Every database id carries this prefix; anything else in a `--database` flag +/// is a catalog or name that needs resolving before it can scope a request. +const DATABASE_ID_PREFIX: &str = "dbid"; + +/// True when a `--database` flag value is already an id (no resolve needed). +fn is_database_id(value: &str) -> bool { + value.starts_with(DATABASE_ID_PREFIX) +} + +/// Resolve a `--database` flag value to the id request scoping needs (the +/// `X-Database-Id` header or a database-scoped path segment). An id passes +/// through untouched — the common path pays no extra round trip — while a +/// catalog or name resolves like every other database lookup (preferring the +/// active database on an ambiguous catalog), so `-d ` works everywhere +/// `-d ` does instead of the server rejecting the raw name. Exits with the +/// resolver's message when nothing matches. +pub fn resolve_database_flag(workspace_id: &str, database: Option<&str>) -> Option { + let key = database?; + if is_database_id(key) { + return Some(key.to_string()); + } + let api = Api::new(Some(workspace_id)); + Some(resolve_database_preferring_active(&api, key).id) +} + fn schema_name(schema: Option<&str>) -> &str { schema.unwrap_or(DEFAULT_SCHEMA) } @@ -1614,15 +1669,28 @@ fn database_exists_or_unverifiable(result: Result) -> Result } } -pub fn set(workspace_id: &str, id: &str) { +pub fn set(workspace_id: &str, id_or_name: &str) { use crossterm::style::Stylize; // `set` only writes local config; the GET is just a friendly existence-check. let api = Api::new(Some(workspace_id)); - if !database_exists_or_unverifiable(get_database(&api, id)).unwrap_or_else(|e| e.exit()) { - eprintln!("{}", format!("error: no database with id '{id}'").red()); - std::process::exit(1); - } - if let Err(e) = crate::config::save_current_database("default", workspace_id, id) { + let id = if is_database_id(id_or_name) { + // Ids keep the 403-tolerant existence check: a database API token is + // denied `GET /v1/databases/{id}` (and the list fallback), so resolving + // would lock it out of `databases use` entirely. + if !database_exists_or_unverifiable(get_database(&api, id_or_name)) + .unwrap_or_else(|e| e.exit()) + { + eprintln!("{}", format!("error: no database with id '{id_or_name}'").red()); + std::process::exit(1); + } + id_or_name.to_string() + } else { + // A catalog or name: resolve it so `databases use ` works like + // every other database lookup. Config must hold the id — everything + // downstream (X-Database-Id, context paths) sends it raw. + resolve_database(&api, id_or_name).id + }; + if let Err(e) = crate::config::save_current_database("default", workspace_id, &id) { eprintln!("{}", format!("error saving current database: {e}").red()); std::process::exit(1); } @@ -1765,24 +1833,7 @@ pub fn tables_load( let api = Api::new(Some(workspace_id)); // Prefer the active database when its catalog or name matches the lookup key, // avoiding ambiguity when multiple databases share the same catalog name. - let active_id = crate::config::load_current_database("default", workspace_id); - let lookup_key = match active_id.as_deref() { - Some(id) => { - if let Some(active) = none_if_404(get_database(&api, id)).unwrap_or_else(|e| e.exit()) { - if active.default_catalog.as_deref() == Some(database.as_str()) - || active.name.as_deref() == Some(database.as_str()) - { - id.to_string() - } else { - database.clone() - } - } else { - database.clone() - } - } - None => database.clone(), - }; - let db = resolve_database(&api, &lookup_key); + let db = resolve_database_preferring_active(&api, &database); // A result load hits the connection-scoped endpoint, where the server scopes // the result by the X-Database-Id header; that must name the resolved target // database, not the ambient active one (which may be unset or different). An @@ -2283,6 +2334,103 @@ mod tests { list.assert(); } + /// Like [`full_detail`] but with an explicit catalog, for ambiguity tests. + fn detail_with_catalog(id: &str, name: &str, catalog: &str) -> String { + format!( + r#"{{"id":"{id}","name":"{name}","default_catalog":"{catalog}","default_schema":"main","default_connection_id":"conn_x","attachments":[]}}"# + ) + } + + #[test] + fn preferring_active_picks_active_on_matching_catalog() { + // Two databases could share catalog 'default'; the active one wins + // without a list round-trip. Only the active-detail GET is mocked — a + // fallback into try_resolve_database would hit unmocked routes and fail. + let mut server = mockito::Server::new(); + let active = server + .mock("GET", "/v1/databases/db_active") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(detail_with_catalog("db_active", "mine", "default")) + .create(); + + let api = Api::test_new_scoped(&server.url(), "k", Some("ws"), Some("db_active")); + let db = try_resolve_database_preferring_active(&api, "default").unwrap(); + assert_eq!(db.id, "db_active"); + active.assert(); + } + + #[test] + fn preferring_active_falls_back_when_active_does_not_match() { + // The active database's catalog/name don't match the key → the normal + // id → catalog → name resolution runs. + let mut server = mockito::Server::new(); + let active = server + .mock("GET", "/v1/databases/db_active") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(detail_with_catalog("db_active", "othername", "other")) + .create(); + let not_id = server + .mock("GET", "/v1/databases/sales") + .with_status(404) + .with_body(r#"{"error":"not found"}"#) + .create(); + let list = server + .mock("GET", "/v1/databases") + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"databases":[{"id":"db_s","name":"sales","default_catalog":"sales_cat","default_schema":"main"}]}"#, + ) + .create(); + let detail = server + .mock("GET", "/v1/databases/db_s") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(detail_with_catalog("db_s", "sales", "sales_cat")) + .create(); + + let api = Api::test_new_scoped(&server.url(), "k", Some("ws"), Some("db_active")); + let db = try_resolve_database_preferring_active(&api, "sales").unwrap(); + assert_eq!(db.id, "db_s"); + active.assert(); + not_id.assert(); + list.assert(); + detail.assert(); + } + + #[test] + fn preferring_active_without_scope_resolves_plainly() { + // No ambient database scope → straight to the normal resolution. + let mut server = mockito::Server::new(); + let by_id = server + .mock("GET", "/v1/databases/db_abc") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(full_detail("db_abc", "sales", "conn_1")) + .create(); + + let api = Api::test_new(&server.url(), "k", Some("ws")); + let db = try_resolve_database_preferring_active(&api, "db_abc").unwrap(); + assert_eq!(db.id, "db_abc"); + by_id.assert(); + } + + #[test] + fn database_flag_passes_ids_through_without_resolving() { + // An id needs no lookup (and must not construct an Api, which would + // read real user config in this test). + assert_eq!( + resolve_database_flag("ws", Some("dbid123abc")), + Some("dbid123abc".to_string()) + ); + assert_eq!(resolve_database_flag("ws", None), None); + assert!(is_database_id("dbideutulm48nc5l28ikc6u53gmjzr")); + assert!(!is_database_id("littlesis")); + assert!(!is_database_id("default")); + } + #[test] fn resolve_database_by_id_and_name() { let mut server = mockito::Server::new(); diff --git a/src/commands/queries.rs b/src/commands/queries.rs index 7203d4e..b8716c5 100644 --- a/src/commands/queries.rs +++ b/src/commands/queries.rs @@ -209,7 +209,8 @@ pub fn list( status: Option<&str>, format: &str, ) { - let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database); + let database = crate::commands::databases::resolve_database_flag(workspace_id, database); + let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database.as_deref()); let database_id = api.require_database(); let resp = crate::client::sdk::block_with_wakeup( @@ -269,7 +270,8 @@ pub fn list( } pub fn get(query_run_id: &str, workspace_id: &str, database: Option<&str>, format: &str) { - let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database); + let database = crate::commands::databases::resolve_database_flag(workspace_id, database); + let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database.as_deref()); let database_id = api.require_database(); let run: QueryRun = crate::client::sdk::block_with_wakeup( &api, diff --git a/src/commands/query.rs b/src/commands/query.rs index 69db1c7..ff3f378 100644 --- a/src/commands/query.rs +++ b/src/commands/query.rs @@ -441,11 +441,13 @@ fn fail_run(error_msg: &str) -> ! { } pub fn execute(sql: &str, workspace_id: &str, database: Option<&str>, format: &str, dialect: &str) { - // Scope to the explicit --database flag, else the active database resolved - // at construction (HOTDATA_DATABASE / current database). The scoped `Api` - // carries the database into submit_query's `X-Database-Id` header and into - // the database-scoped follow-up fetches (query-run poll, Arrow result). - let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database); + // Scope to the explicit --database flag (an id, catalog, or name — resolved + // to an id here), else the active database resolved at construction + // (HOTDATA_DATABASE / current database). The scoped `Api` carries the + // database into submit_query's `X-Database-Id` header and into the + // database-scoped follow-up fetches (query-run poll, Arrow result). + let database = crate::commands::databases::resolve_database_flag(workspace_id, database); + let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database.as_deref()); let database = api.database_id(); let mut request = hotdata::models::QueryRequest::new(sql.to_string()); @@ -551,7 +553,8 @@ pub fn execute(sql: &str, workspace_id: &str, database: Option<&str>, format: &s /// Poll a query run by ID. If succeeded and has a result_id, fetch and display the result. pub fn poll(query_run_id: &str, workspace_id: &str, database: Option<&str>, format: &str) { - let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database); + let database = crate::commands::databases::resolve_database_flag(workspace_id, database); + let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database.as_deref()); let run = crate::client::sdk::block( api.client() diff --git a/src/commands/results.rs b/src/commands/results.rs index 8215677..16b3a6f 100644 --- a/src/commands/results.rs +++ b/src/commands/results.rs @@ -59,7 +59,8 @@ pub fn list( offset: Option, format: &str, ) { - let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database); + let database = crate::commands::databases::resolve_database_flag(workspace_id, database); + let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database.as_deref()); // Results are database-scoped (the required `X-Database-Id` header the seam // sends from the active database). Fail early with a hint when none is set, // rather than surfacing the raw server error. @@ -152,7 +153,8 @@ pub fn list( } pub fn get(result_id: &str, workspace_id: &str, database: Option<&str>, format: &str) { - let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database); + let database = crate::commands::databases::resolve_database_flag(workspace_id, database); + let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database.as_deref()); let result = crate::commands::query::fetch_arrow_result(&api, result_id); crate::commands::query::print_result(&result, format); } diff --git a/src/commands/search.rs b/src/commands/search.rs index b13b5d9..16a1f26 100644 --- a/src/commands/search.rs +++ b/src/commands/search.rs @@ -266,20 +266,25 @@ fn create( // its own error (e.g. an ambiguous forked-catalog alias) is surfaced as-is. let db = match target { FromTarget::Database(db) => *db, - FromTarget::Catalog(catalog) => databases::try_resolve_database(&api, &catalog) - .unwrap_or_else(|e| { + // Prefer the active database when the catalog is ambiguous — the same + // rule `databases load --catalog` applies — so a `--from` naming the + // active database's own catalog (e.g. `default` shared across forks) + // resolves to it instead of erroring. + FromTarget::Catalog(catalog) => { + databases::try_resolve_database_preferring_active(&api, &catalog).unwrap_or_else(|e| { use crossterm::style::Stylize; eprintln!( "{}", format!( - "error: {e}\nSearch indexes are created on instant databases — pass a \ + "error: {e}\nSearch indexes are created on instant databases — pass an \ instant database's catalog or id, or 'schema.table' with an active \ database set via 'hotdata databases use '." ) .red() ); std::process::exit(1); - }), + }) + } }; let conn_id = db.default_connection_id; let auto_name = format!("{table}_{}_{index_type}", column.replace(',', "_")); @@ -321,7 +326,10 @@ fn list(workspace_id: &str, schema: Option<&str>, table: Option<&str>, output: & } fn locate_or_exit(workspace_id: &str, database: Option<&str>, name: &str) -> indexes::LocatedIndex { - indexes::locate_by_name(workspace_id, database, name).unwrap_or_else(|e| { + // `--database` accepts a catalog or name as well as an id, like every + // other database flag; locate_by_name needs the id. + let database = databases::resolve_database_flag(workspace_id, database); + indexes::locate_by_name(workspace_id, database.as_deref(), name).unwrap_or_else(|e| { use crossterm::style::Stylize; eprintln!("{}", e.red()); std::process::exit(1); diff --git a/src/main.rs b/src/main.rs index 16e5b4d..6ea6111 100644 --- a/src/main.rs +++ b/src/main.rs @@ -413,16 +413,19 @@ fn main() { } }, Some(DatabasesCommands::Context { database, command }) => { - let database_id = database - .or_else(|| { - config::load_current_database("default", &workspace_id) - }) - .unwrap_or_else(|| { - eprintln!( - "error: no active database. Pass -d/--database or set one with 'hotdata databases use '." - ); - std::process::exit(1); - }); + // The context endpoints take the database id as a path + // segment; resolve a catalog/name flag to it first. + let database_id = + databases::resolve_database_flag(&workspace_id, database.as_deref()) + .or_else(|| { + config::load_current_database("default", &workspace_id) + }) + .unwrap_or_else(|| { + eprintln!( + "error: no active database. Pass -d/--database or set one with 'hotdata databases use '." + ); + std::process::exit(1); + }); match command { ContextCommands::List { output, prefix } => context::list( &workspace_id, From 4f6748fdce859ffcfa308d393862b4e16ebdfe52 Mon Sep 17 00:00:00 2001 From: Eddie A Tejeda <669988+eddietejeda@users.noreply.github.com> Date: Thu, 27 Aug 2026 14:30:10 -0700 Subject: [PATCH 3/7] feat(workspaces): mark the default workspace in list JSON output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `workspaces list -o json` had no field identifying the workspace the CLI acts on: the table view marks it with a DEFAULT column, but the JSON only carried `active` — a server-side workspace-state flag that is true for every usable workspace, and a footgun for scripts that read it as "the current workspace". Add `default: bool` (exactly one true), stamped from the same source the table marker uses (HOTDATA_WORKSPACE, else the front of the configured list), and derive the table marker from it so the two views can't drift. --- src/commands/workspace.rs | 85 +++++++++++++++++++++++++++++++-------- 1 file changed, 68 insertions(+), 17 deletions(-) diff --git a/src/commands/workspace.rs b/src/commands/workspace.rs index 5a0ccec..6c73f71 100644 --- a/src/commands/workspace.rs +++ b/src/commands/workspace.rs @@ -24,6 +24,12 @@ pub enum WorkspaceCommands { struct Workspace { public_id: String, name: String, + /// True for the workspace commands act on by default (the one `workspaces + /// use` selected, or `HOTDATA_WORKSPACE`). Not to be confused with + /// `active`, a server-side workspace-state flag that is true for every + /// usable workspace — scripts looking for "the current workspace" need + /// this field, and before it existed the JSON offered nothing. + default: bool, active: bool, favorite: bool, provision_status: String, @@ -34,6 +40,8 @@ impl From<&hotdata::models::WorkspaceListItem> for Workspace { Workspace { public_id: w.public_id.clone(), name: w.name.clone(), + // Stamped by fetch_workspaces, which knows the configured default. + default: false, active: w.active, favorite: w.favorite, provision_status: w.provision_status.clone(), @@ -41,10 +49,30 @@ impl From<&hotdata::models::WorkspaceListItem> for Workspace { } } +/// The workspace commands act on when none is passed: `HOTDATA_WORKSPACE`, +/// else the front of the configured list (`workspaces use` moves its pick +/// there). +fn default_workspace_id() -> String { + std::env::var("HOTDATA_WORKSPACE").unwrap_or_else(|_| { + config::load("default") + .ok() + .and_then(|c| c.workspaces.first().map(|w| w.public_id.clone())) + .unwrap_or_default() + }) +} + fn fetch_workspaces() -> Vec { let api = Api::new(None); let body = api.list_workspaces(None).unwrap_or_else(|e| e.exit()); - body.workspaces.iter().map(Workspace::from).collect() + let default_id = default_workspace_id(); + body.workspaces + .iter() + .map(|w| { + let mut ws = Workspace::from(w); + ws.default = !default_id.is_empty() && ws.public_id == default_id; + ws + }) + .collect() } pub fn set(workspace_id: Option<&str>) { @@ -104,21 +132,6 @@ pub fn set(workspace_id: Option<&str>) { } pub fn list(format: &str) { - let profile_config = match config::load("default") { - Ok(c) => c, - Err(e) => { - eprintln!("{e}"); - std::process::exit(1); - } - }; - let default_id = std::env::var("HOTDATA_WORKSPACE").unwrap_or_else(|_| { - profile_config - .workspaces - .first() - .map(|w| w.public_id.clone()) - .unwrap_or_default() - }); - let workspaces = fetch_workspaces(); match format { @@ -136,7 +149,7 @@ pub fn list(format: &str) { let rows: Vec> = workspaces .iter() .map(|w| { - let marker = if w.public_id == default_id { "*" } else { "" }; + let marker = if w.default { "*" } else { "" }; vec![ marker.to_string(), w.public_id.clone(), @@ -154,3 +167,41 @@ pub fn list(format: &str) { _ => unreachable!(), } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn workspace_json_exposes_default_flag() { + // `active` is a server workspace-state flag (true for every usable + // workspace); `default` is the one the CLI acts on. Scripts need the + // latter in `-o json` — the table view's DEFAULT column has no JSON + // counterpart otherwise. + let ws = Workspace { + public_id: "work123".to_string(), + name: "Default Workspace".to_string(), + default: true, + active: true, + favorite: false, + provision_status: "success".to_string(), + }; + let json = serde_json::to_value(&ws).unwrap(); + assert_eq!(json["default"], serde_json::json!(true)); + assert_eq!(json["active"], serde_json::json!(true)); + } + + #[test] + fn from_list_item_defaults_to_not_default() { + // The wire item carries no default marker — it's stamped from local + // config by fetch_workspaces, so the raw mapping must start false. + let item = hotdata::models::WorkspaceListItem::new( + "wid".to_string(), + "n".to_string(), + true, + false, + "success".to_string(), + ); + assert!(!Workspace::from(&item).default); + } +} From 1b383ff8f06cf8b81ead1207f11792f654378cb5 Mon Sep 17 00:00:00 2001 From: Eddie A Tejeda <669988+eddietejeda@users.noreply.github.com> Date: Thu, 27 Aug 2026 14:47:15 -0700 Subject: [PATCH 4/7] fix(cli): address review nits on database-flag resolution and attachment display - an unresolved attachment keeps `catalog: null` in -o json/yaml instead of disguising the internal connection id as a SQL catalog name (an FQN built from it wouldn't be queryable); the table view falls back to the raw id itself, which detach still accepts - resolve_database_flag takes the caller's &Api instead of constructing a second one (a name/catalog flag paid two config loads + token preflights) - update the stale `--database` help on search show/remove and the top-level search command: the flag now takes an id, catalog, or name --- src/cli.rs | 2 +- src/commands/databases.rs | 97 ++++++++++++++++++++++++++------------- src/commands/queries.rs | 10 ++-- src/commands/query.rs | 10 ++-- src/commands/results.rs | 10 ++-- src/commands/search.rs | 11 +++-- src/main.rs | 27 ++++++----- 7 files changed, 106 insertions(+), 61 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index f84ae6f..6545f61 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -114,7 +114,7 @@ pub enum Commands { #[arg(long, visible_alias = "in")] index: Option, - /// Database the index lives in (id; defaults to the active database) + /// Database the index lives in (id, catalog, or name; defaults to the active database) #[arg(long, short = 'd')] database: Option, diff --git a/src/commands/databases.rs b/src/commands/databases.rs index 82d7160..3e486e7 100644 --- a/src/commands/databases.rs +++ b/src/commands/databases.rs @@ -675,18 +675,18 @@ fn is_database_id(value: &str) -> bool { /// Resolve a `--database` flag value to the id request scoping needs (the /// `X-Database-Id` header or a database-scoped path segment). An id passes -/// through untouched — the common path pays no extra round trip — while a -/// catalog or name resolves like every other database lookup (preferring the -/// active database on an ambiguous catalog), so `-d ` works everywhere -/// `-d ` does instead of the server rejecting the raw name. Exits with the -/// resolver's message when nothing matches. -pub fn resolve_database_flag(workspace_id: &str, database: Option<&str>) -> Option { - let key = database?; +/// through untouched — the common path pays no round trip — while a catalog +/// or name resolves like every other database lookup (preferring the active +/// database on an ambiguous catalog), so `-d ` works everywhere +/// `-d ` does instead of the server rejecting the raw name. Takes the +/// caller's `Api` (callers all have one, or are about to build one to scope) +/// rather than constructing a second. Exits with the resolver's message when +/// nothing matches. +pub fn resolve_database_flag(api: &Api, key: &str) -> String { if is_database_id(key) { - return Some(key.to_string()); + return key.to_string(); } - let api = Api::new(Some(workspace_id)); - Some(resolve_database_preferring_active(&api, key).id) + resolve_database_preferring_active(api, key).id } fn schema_name(schema: Option<&str>) -> &str { @@ -1205,8 +1205,9 @@ pub fn count(workspace_id: &str, format: &str) { /// stamped by the `From` mapping), else the connection's name from one /// `connections list` call. Skips the call entirely when every attachment is /// aliased (or there are none). A connection the listing doesn't cover (or a -/// failed listing) falls back to the raw id — a degraded but still usable -/// handle for `databases detach`. +/// failed listing) stays `None`: the JSON must not pass the internal id off as +/// a SQL catalog name — an FQN built from it wouldn't be queryable. The table +/// view falls back to the raw id itself, as a usable `detach` handle. fn resolve_attachment_catalogs(api: &Api, db: &mut Database) { if db.attachments.iter().all(|a| a.catalog.is_some()) { return; @@ -1218,12 +1219,7 @@ fn resolve_attachment_catalogs(api: &Api, db: &mut Database) { }; for a in &mut db.attachments { if a.catalog.is_none() { - a.catalog = Some( - names - .get(&a.connection_id) - .cloned() - .unwrap_or_else(|| a.connection_id.clone()), - ); + a.catalog = names.get(&a.connection_id).cloned(); } } } @@ -1267,9 +1263,12 @@ pub fn get(workspace_id: &str, id_or_name: &str, format: &str) { println!("{}({})", label("attached catalogs:"), db.attachments.len()); for a in &db.attachments { // The resolved catalog name is what the attachment is - // reachable as in SQL (and what detach accepts) — the - // internal connection id is deliberately not shown. - let name = a.catalog.as_deref().unwrap_or("(unknown)"); + // reachable as in SQL (and what detach accepts). When it + // couldn't be resolved, the raw connection id is the only + // handle `detach` still takes — show it here (table only; + // JSON keeps `catalog: null` rather than disguising the + // id as a catalog). + let name = a.catalog.as_deref().unwrap_or(&a.connection_id); println!(" {}", name.cyan()); } } @@ -2315,9 +2314,12 @@ mod tests { } #[test] - fn resolve_attachment_catalogs_falls_back_to_id_when_unlisted() { - // The listing doesn't cover the connection (e.g. database-scoped, or the - // call failed): degrade to the raw id — still a usable detach handle. + fn resolve_attachment_catalogs_leaves_unlisted_unresolved() { + // The listing doesn't cover the connection (e.g. database-scoped, or + // the call failed): `catalog` stays None so `-o json` emits null + // rather than disguising the internal id as a SQL catalog name (an + // FQN built from it wouldn't be queryable). The table view falls back + // to the raw id itself as a detach handle. let mut server = mockito::Server::new(); let list = server .mock("GET", "/v1/connections") @@ -2330,7 +2332,9 @@ mod tests { let api = Api::test_new(&server.url(), "k", Some("ws")); let mut db = db_with_attachments(); resolve_attachment_catalogs(&api, &mut db); - assert_eq!(db.attachments[1].catalog.as_deref(), Some("conn_b")); + assert_eq!(db.attachments[1].catalog, None); + let json = serde_json::to_value(&db).unwrap(); + assert_eq!(json["attachments"][1]["catalog"], serde_json::Value::Null); list.assert(); } @@ -2419,18 +2423,47 @@ mod tests { #[test] fn database_flag_passes_ids_through_without_resolving() { - // An id needs no lookup (and must not construct an Api, which would - // read real user config in this test). - assert_eq!( - resolve_database_flag("ws", Some("dbid123abc")), - Some("dbid123abc".to_string()) - ); - assert_eq!(resolve_database_flag("ws", None), None); + // An id needs no lookup: point the Api at a server with no mocks so + // any resolve attempt fails loudly. + let server = mockito::Server::new(); + let api = Api::test_new(&server.url(), "k", Some("ws")); + assert_eq!(resolve_database_flag(&api, "dbid123abc"), "dbid123abc"); assert!(is_database_id("dbideutulm48nc5l28ikc6u53gmjzr")); assert!(!is_database_id("littlesis")); assert!(!is_database_id("default")); } + #[test] + fn database_flag_resolves_names_to_ids() { + // A non-id flag resolves through the normal lookup and returns the id. + let mut server = mockito::Server::new(); + let not_id = server + .mock("GET", "/v1/databases/sales") + .with_status(404) + .with_body(r#"{"error":"not found"}"#) + .create(); + let list = server + .mock("GET", "/v1/databases") + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"databases":[{"id":"db_s","name":"sales","default_catalog":"sales_cat","default_schema":"main"}]}"#, + ) + .create(); + let detail = server + .mock("GET", "/v1/databases/db_s") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(detail_with_catalog("db_s", "sales", "sales_cat")) + .create(); + + let api = Api::test_new(&server.url(), "k", Some("ws")); + assert_eq!(resolve_database_flag(&api, "sales"), "db_s"); + not_id.assert(); + list.assert(); + detail.assert(); + } + #[test] fn resolve_database_by_id_and_name() { let mut server = mockito::Server::new(); diff --git a/src/commands/queries.rs b/src/commands/queries.rs index b8716c5..4ba67b0 100644 --- a/src/commands/queries.rs +++ b/src/commands/queries.rs @@ -209,8 +209,9 @@ pub fn list( status: Option<&str>, format: &str, ) { - let database = crate::commands::databases::resolve_database_flag(workspace_id, database); - let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database.as_deref()); + let api = Api::new(Some(workspace_id)); + let database = database.map(|d| crate::commands::databases::resolve_database_flag(&api, d)); + let api = api.scoped_to_database_opt(database.as_deref()); let database_id = api.require_database(); let resp = crate::client::sdk::block_with_wakeup( @@ -270,8 +271,9 @@ pub fn list( } pub fn get(query_run_id: &str, workspace_id: &str, database: Option<&str>, format: &str) { - let database = crate::commands::databases::resolve_database_flag(workspace_id, database); - let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database.as_deref()); + let api = Api::new(Some(workspace_id)); + let database = database.map(|d| crate::commands::databases::resolve_database_flag(&api, d)); + let api = api.scoped_to_database_opt(database.as_deref()); let database_id = api.require_database(); let run: QueryRun = crate::client::sdk::block_with_wakeup( &api, diff --git a/src/commands/query.rs b/src/commands/query.rs index ff3f378..a5f4fc8 100644 --- a/src/commands/query.rs +++ b/src/commands/query.rs @@ -446,8 +446,9 @@ pub fn execute(sql: &str, workspace_id: &str, database: Option<&str>, format: &s // (HOTDATA_DATABASE / current database). The scoped `Api` carries the // database into submit_query's `X-Database-Id` header and into the // database-scoped follow-up fetches (query-run poll, Arrow result). - let database = crate::commands::databases::resolve_database_flag(workspace_id, database); - let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database.as_deref()); + let api = Api::new(Some(workspace_id)); + let database = database.map(|d| crate::commands::databases::resolve_database_flag(&api, d)); + let api = api.scoped_to_database_opt(database.as_deref()); let database = api.database_id(); let mut request = hotdata::models::QueryRequest::new(sql.to_string()); @@ -553,8 +554,9 @@ pub fn execute(sql: &str, workspace_id: &str, database: Option<&str>, format: &s /// Poll a query run by ID. If succeeded and has a result_id, fetch and display the result. pub fn poll(query_run_id: &str, workspace_id: &str, database: Option<&str>, format: &str) { - let database = crate::commands::databases::resolve_database_flag(workspace_id, database); - let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database.as_deref()); + let api = Api::new(Some(workspace_id)); + let database = database.map(|d| crate::commands::databases::resolve_database_flag(&api, d)); + let api = api.scoped_to_database_opt(database.as_deref()); let run = crate::client::sdk::block( api.client() diff --git a/src/commands/results.rs b/src/commands/results.rs index 16b3a6f..538b59c 100644 --- a/src/commands/results.rs +++ b/src/commands/results.rs @@ -59,8 +59,9 @@ pub fn list( offset: Option, format: &str, ) { - let database = crate::commands::databases::resolve_database_flag(workspace_id, database); - let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database.as_deref()); + let api = Api::new(Some(workspace_id)); + let database = database.map(|d| crate::commands::databases::resolve_database_flag(&api, d)); + let api = api.scoped_to_database_opt(database.as_deref()); // Results are database-scoped (the required `X-Database-Id` header the seam // sends from the active database). Fail early with a hint when none is set, // rather than surfacing the raw server error. @@ -153,8 +154,9 @@ pub fn list( } pub fn get(result_id: &str, workspace_id: &str, database: Option<&str>, format: &str) { - let database = crate::commands::databases::resolve_database_flag(workspace_id, database); - let api = Api::new(Some(workspace_id)).scoped_to_database_opt(database.as_deref()); + let api = Api::new(Some(workspace_id)); + let database = database.map(|d| crate::commands::databases::resolve_database_flag(&api, d)); + let api = api.scoped_to_database_opt(database.as_deref()); let result = crate::commands::query::fetch_arrow_result(&api, result_id); crate::commands::query::print_result(&result, format); } diff --git a/src/commands/search.rs b/src/commands/search.rs index 16a1f26..1578a0d 100644 --- a/src/commands/search.rs +++ b/src/commands/search.rs @@ -76,7 +76,7 @@ pub enum SearchCommands { /// Index name name: String, - /// Database the index lives in (id; defaults to the active database) + /// Database the index lives in (id, catalog, or name; defaults to the active database) #[arg(long, short = 'd')] database: Option, @@ -90,7 +90,7 @@ pub enum SearchCommands { /// Index name name: String, - /// Database the index lives in (id; defaults to the active database) + /// Database the index lives in (id, catalog, or name; defaults to the active database) #[arg(long, short = 'd')] database: Option, }, @@ -327,8 +327,11 @@ fn list(workspace_id: &str, schema: Option<&str>, table: Option<&str>, output: & fn locate_or_exit(workspace_id: &str, database: Option<&str>, name: &str) -> indexes::LocatedIndex { // `--database` accepts a catalog or name as well as an id, like every - // other database flag; locate_by_name needs the id. - let database = databases::resolve_database_flag(workspace_id, database); + // other database flag; locate_by_name needs the id. The Api is built only + // when there's a non-id flag to resolve — the id fast-path never needs it. + let database = database.map(|d| { + databases::resolve_database_flag(&Api::new(Some(workspace_id)), d) + }); indexes::locate_by_name(workspace_id, database.as_deref(), name).unwrap_or_else(|e| { use crossterm::style::Stylize; eprintln!("{}", e.red()); diff --git a/src/main.rs b/src/main.rs index 6ea6111..798f738 100644 --- a/src/main.rs +++ b/src/main.rs @@ -414,18 +414,21 @@ fn main() { }, Some(DatabasesCommands::Context { database, command }) => { // The context endpoints take the database id as a path - // segment; resolve a catalog/name flag to it first. - let database_id = - databases::resolve_database_flag(&workspace_id, database.as_deref()) - .or_else(|| { - config::load_current_database("default", &workspace_id) - }) - .unwrap_or_else(|| { - eprintln!( - "error: no active database. Pass -d/--database or set one with 'hotdata databases use '." - ); - std::process::exit(1); - }); + // segment; resolve a catalog/name flag to it first. The + // Api is only built when there's a flag to resolve. + let database_id = match database.as_deref() { + Some(flag) => Some(databases::resolve_database_flag( + &client::sdk::Api::new(Some(&workspace_id)), + flag, + )), + None => config::load_current_database("default", &workspace_id), + } + .unwrap_or_else(|| { + eprintln!( + "error: no active database. Pass -d/--database or set one with 'hotdata databases use '." + ); + std::process::exit(1); + }); match command { ContextCommands::List { output, prefix } => context::list( &workspace_id, From 5c8b23d141889193e5a49edabe12730877e24ca7 Mon Sep 17 00:00:00 2001 From: Eddie A Tejeda <669988+eddietejeda@users.noreply.github.com> Date: Wed, 2 Sep 2026 15:22:55 -0700 Subject: [PATCH 5/7] revert(databases): keep connection ids in database output Maintainer feedback on the PR: connection ids are becoming catalog ids and should stay exposed, so drop the hiding from b14e54a (and the catalog-resolution follow-up from 1b383ff) -- databases show prints the catalog id line and raw attachment ids again, and -o json/yaml keeps default_connection_id and attachments[].connection_id. Also reformat locate_or_exit (the closure block didn't survive cargo fmt) and correct its comment: the Api is built for id flags too; it's the resolve that short-circuits. --- src/commands/databases.rs | 200 +++++--------------------------------- src/commands/search.rs | 9 +- 2 files changed, 28 insertions(+), 181 deletions(-) diff --git a/src/commands/databases.rs b/src/commands/databases.rs index 3e486e7..cdbd13d 100644 --- a/src/commands/databases.rs +++ b/src/commands/databases.rs @@ -386,11 +386,6 @@ pub struct Database { pub name: Option, #[serde(default)] pub default_catalog: Option, - /// Internal handle for the database's own catalog — used to build the - /// managed-load/delete API paths and by the connection resolver. Connections - /// are an internal concept now (ingest creates catalogs), so it's never - /// serialized into `-o json`/`-o yaml` output. - #[serde(skip_serializing)] pub default_connection_id: String, #[serde(default)] pub expires_at: Option, @@ -402,15 +397,6 @@ pub struct Database { #[derive(Clone, Debug, Serialize, PartialEq, Eq)] struct DatabaseAttachment { - /// Catalog name this attachment is reachable as inside the database — the - /// alias when set, else the connection's name. `None` until resolved by - /// [`resolve_attachment_catalogs`] (only `get` displays attachments, so - /// only it pays the lookup). - catalog: Option, - /// Internal handle, kept for the detach-by-alias fallback and the resolver; - /// never shown — connections are an internal concept now (ingest creates - /// catalogs). - #[serde(skip_serializing)] connection_id: String, alias: Option, } @@ -439,10 +425,6 @@ struct CreateDatabaseResponse { name: Option, #[serde(default)] default_catalog: Option, - /// Internal handle (see [`Database::default_connection_id`]): deserialized - /// from the create/fork response for the managed-load path, never emitted - /// into `-o json`/`-o yaml` output. - #[serde(skip_serializing)] default_connection_id: String, #[serde(default)] expires_at: Option, @@ -470,16 +452,9 @@ impl From for Database { attachments: d .attachments .into_iter() - .map(|a| { - let alias = a.alias.flatten(); - DatabaseAttachment { - // The alias is the reachable catalog name when set; a - // non-aliased attachment needs a connections lookup - // (resolve_attachment_catalogs) to learn its name. - catalog: alias.clone(), - connection_id: a.connection_id, - alias, - } + .map(|a| DatabaseAttachment { + connection_id: a.connection_id, + alias: a.alias.flatten(), }) .collect(), } @@ -642,8 +617,7 @@ pub fn resolve_database(api: &Api, id_or_name: &str) -> Database { /// the active database's catalog nor its name resolves like any other lookup. pub fn try_resolve_database_preferring_active(api: &Api, key: &str) -> Result { if let Some(active_id) = api.database_id().filter(|id| *id != key) - && let Some(active) = - none_if_404(get_database(api, active_id)).unwrap_or_else(|e| e.exit()) + && let Some(active) = none_if_404(get_database(api, active_id)).unwrap_or_else(|e| e.exit()) && (active.default_catalog.as_deref() == Some(key) || active.name.as_deref() == Some(key)) { return Ok(active); @@ -1201,33 +1175,9 @@ pub fn count(workspace_id: &str, format: &str) { } } -/// Fill each attachment's display `catalog` name: the alias when set (already -/// stamped by the `From` mapping), else the connection's name from one -/// `connections list` call. Skips the call entirely when every attachment is -/// aliased (or there are none). A connection the listing doesn't cover (or a -/// failed listing) stays `None`: the JSON must not pass the internal id off as -/// a SQL catalog name — an FQN built from it wouldn't be queryable. The table -/// view falls back to the raw id itself, as a usable `detach` handle. -fn resolve_attachment_catalogs(api: &Api, db: &mut Database) { - if db.attachments.iter().all(|a| a.catalog.is_some()) { - return; - } - let names: std::collections::HashMap = - match block(api.client().connections().list()) { - Ok(resp) => resp.connections.into_iter().map(|c| (c.id, c.name)).collect(), - Err(_) => Default::default(), - }; - for a in &mut db.attachments { - if a.catalog.is_none() { - a.catalog = names.get(&a.connection_id).cloned(); - } - } -} - pub fn get(workspace_id: &str, id_or_name: &str, format: &str) { let api = Api::new(Some(workspace_id)); - let mut db = resolve_database(&api, id_or_name); - resolve_attachment_catalogs(&api, &mut db); + let db = resolve_database(&api, id_or_name); match format { "json" => println!("{}", serde_json::to_string_pretty(&db).unwrap()), @@ -1249,6 +1199,11 @@ pub fn get(workspace_id: &str, id_or_name: &str, format: &str) { crate::util::format_date(ts).dark_grey() ); } + println!( + "{}{}", + label("catalog id:"), + db.default_connection_id.clone().dark_cyan() + ); let catalog = db .default_catalog .as_deref() @@ -1262,14 +1217,16 @@ pub fn get(workspace_id: &str, id_or_name: &str, format: &str) { if !db.attachments.is_empty() { println!("{}({})", label("attached catalogs:"), db.attachments.len()); for a in &db.attachments { - // The resolved catalog name is what the attachment is - // reachable as in SQL (and what detach accepts). When it - // couldn't be resolved, the raw connection id is the only - // handle `detach` still takes — show it here (table only; - // JSON keeps `catalog: null` rather than disguising the - // id as a catalog). - let name = a.catalog.as_deref().unwrap_or(&a.connection_id); - println!(" {}", name.cyan()); + let alias = a + .alias + .as_deref() + .map(|al| format!(" as {al}")) + .unwrap_or_default(); + println!( + " {}{}", + a.connection_id.clone().dark_cyan(), + alias.dark_grey() + ); } } } @@ -1679,7 +1636,10 @@ pub fn set(workspace_id: &str, id_or_name: &str) { if !database_exists_or_unverifiable(get_database(&api, id_or_name)) .unwrap_or_else(|e| e.exit()) { - eprintln!("{}", format!("error: no database with id '{id_or_name}'").red()); + eprintln!( + "{}", + format!("error: no database with id '{id_or_name}'").red() + ); std::process::exit(1); } id_or_name.to_string() @@ -2226,118 +2186,6 @@ mod tests { ) } - /// A `Database` for serialization tests, with one aliased and one bare - /// attachment. - fn db_with_attachments() -> Database { - Database { - id: "db_1".to_string(), - name: Some("sales".to_string()), - default_catalog: Some("sales".to_string()), - default_connection_id: "conn_own".to_string(), - expires_at: None, - created_at: None, - attachments: vec![ - DatabaseAttachment { - catalog: Some("gh".to_string()), - connection_id: "conn_a".to_string(), - alias: Some("gh".to_string()), - }, - DatabaseAttachment { - catalog: None, - connection_id: "conn_b".to_string(), - alias: None, - }, - ], - } - } - - #[test] - fn database_json_output_hides_connection_ids() { - // Connections are internal now (ingest creates catalogs): neither the - // database's own connection id nor an attachment's may reach `-o json`. - let json = serde_json::to_value(db_with_attachments()).unwrap(); - assert!(json.get("default_connection_id").is_none(), "json: {json}"); - let attachments = json["attachments"].as_array().unwrap(); - for a in attachments { - assert!(a.get("connection_id").is_none(), "attachment: {a}"); - } - // The reachable catalog name is what's exposed instead. - assert_eq!(attachments[0]["catalog"], serde_json::json!("gh")); - } - - #[test] - fn create_response_json_output_hides_connection_id() { - let result = CreateDatabaseResponse { - id: "db_new".to_string(), - name: Some("mydb".to_string()), - default_catalog: Some("default".to_string()), - default_connection_id: "conn_abc".to_string(), - expires_at: None, - }; - let json = serde_json::to_value(&result).unwrap(); - assert!(json.get("default_connection_id").is_none(), "json: {json}"); - } - - #[test] - fn resolve_attachment_catalogs_fills_names_from_connections_list() { - let mut server = mockito::Server::new(); - let list = server - .mock("GET", "/v1/connections") - .match_query(mockito::Matcher::Any) - .with_status(200) - .with_header("content-type", "application/json") - .with_body( - r#"{"connections":[{"id":"conn_b","name":"github","source_type":"postgres"}]}"#, - ) - .create(); - - let api = Api::test_new(&server.url(), "k", Some("ws")); - let mut db = db_with_attachments(); - resolve_attachment_catalogs(&api, &mut db); - - // Aliased attachment keeps its alias; the bare one gets the listed name. - assert_eq!(db.attachments[0].catalog.as_deref(), Some("gh")); - assert_eq!(db.attachments[1].catalog.as_deref(), Some("github")); - list.assert(); - } - - #[test] - fn resolve_attachment_catalogs_skips_lookup_when_all_aliased() { - // Every attachment already has a catalog name → no connections call. - // Point at a server with no mocks so a stray call fails loudly. - let server = mockito::Server::new(); - let api = Api::test_new(&server.url(), "k", Some("ws")); - let mut db = db_with_attachments(); - db.attachments.remove(1); // keep only the aliased one - resolve_attachment_catalogs(&api, &mut db); - assert_eq!(db.attachments[0].catalog.as_deref(), Some("gh")); - } - - #[test] - fn resolve_attachment_catalogs_leaves_unlisted_unresolved() { - // The listing doesn't cover the connection (e.g. database-scoped, or - // the call failed): `catalog` stays None so `-o json` emits null - // rather than disguising the internal id as a SQL catalog name (an - // FQN built from it wouldn't be queryable). The table view falls back - // to the raw id itself as a detach handle. - let mut server = mockito::Server::new(); - let list = server - .mock("GET", "/v1/connections") - .match_query(mockito::Matcher::Any) - .with_status(200) - .with_header("content-type", "application/json") - .with_body(r#"{"connections":[]}"#) - .create(); - - let api = Api::test_new(&server.url(), "k", Some("ws")); - let mut db = db_with_attachments(); - resolve_attachment_catalogs(&api, &mut db); - assert_eq!(db.attachments[1].catalog, None); - let json = serde_json::to_value(&db).unwrap(); - assert_eq!(json["attachments"][1]["catalog"], serde_json::Value::Null); - list.assert(); - } - /// Like [`full_detail`] but with an explicit catalog, for ambiguity tests. fn detail_with_catalog(id: &str, name: &str, catalog: &str) -> String { format!( diff --git a/src/commands/search.rs b/src/commands/search.rs index 1578a0d..8f5a30b 100644 --- a/src/commands/search.rs +++ b/src/commands/search.rs @@ -327,11 +327,10 @@ fn list(workspace_id: &str, schema: Option<&str>, table: Option<&str>, output: & fn locate_or_exit(workspace_id: &str, database: Option<&str>, name: &str) -> indexes::LocatedIndex { // `--database` accepts a catalog or name as well as an id, like every - // other database flag; locate_by_name needs the id. The Api is built only - // when there's a non-id flag to resolve — the id fast-path never needs it. - let database = database.map(|d| { - databases::resolve_database_flag(&Api::new(Some(workspace_id)), d) - }); + // other database flag; locate_by_name needs the id. Ids short-circuit in + // resolve_database_flag, so the Api built here costs no round trip then. + let database = + database.map(|d| databases::resolve_database_flag(&Api::new(Some(workspace_id)), d)); indexes::locate_by_name(workspace_id, database.as_deref(), name).unwrap_or_else(|e| { use crossterm::style::Stylize; eprintln!("{}", e.red()); From 008643dd8d52277733187d0f32edfc32c9d3b882 Mon Sep 17 00:00:00 2001 From: Eddie A Tejeda <669988+eddietejeda@users.noreply.github.com> Date: Wed, 2 Sep 2026 15:30:39 -0700 Subject: [PATCH 6/7] fix(workspaces): mark the default the credential actually targets The new `default` field in `workspaces list` resolved from `config.workspaces.first()`, but commands resolve their workspace through credentials::default_workspace_id, which pins an env/flag api key's own authorized workspace. With HOTDATA_API_KEY set to a database token, the listing only contains the key's workspace while the config cache front names another -- so nothing was marked default and the "exactly one true" contract broke. Route the marker through the same helper resolve_workspace uses (HOTDATA_WORKSPACE still wins), and add a test for an Env-source profile whose cache front is not a workspace the credential authorizes. --- src/commands/workspace.rs | 48 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/src/commands/workspace.rs b/src/commands/workspace.rs index 6c73f71..882985a 100644 --- a/src/commands/workspace.rs +++ b/src/commands/workspace.rs @@ -50,17 +50,25 @@ impl From<&hotdata::models::WorkspaceListItem> for Workspace { } /// The workspace commands act on when none is passed: `HOTDATA_WORKSPACE`, -/// else the front of the configured list (`workspaces use` moves its pick -/// there). +/// else the loaded profile's default via [`profile_default_workspace_id`]. fn default_workspace_id() -> String { std::env::var("HOTDATA_WORKSPACE").unwrap_or_else(|_| { config::load("default") .ok() - .and_then(|c| c.workspaces.first().map(|w| w.public_id.clone())) + .and_then(|c| profile_default_workspace_id(&c)) .unwrap_or_default() }) } +/// The workspace the `default` marker names: resolved by the same helper +/// `main`'s `resolve_workspace` uses, so it's the workspace commands actually +/// hit — NOT the front of the config cache. An `--api-key`/`HOTDATA_API_KEY` +/// credential pins its own authorized workspace, which the cache front (what +/// `workspaces use` saved for a session) need not match or even reach. +fn profile_default_workspace_id(profile: &config::ProfileConfig) -> Option { + crate::client::credentials::default_workspace_id(profile) +} + fn fetch_workspaces() -> Vec { let api = Api::new(None); let body = api.list_workspaces(None).unwrap_or_else(|e| e.exit()); @@ -191,6 +199,40 @@ mod tests { assert_eq!(json["active"], serde_json::json!(true)); } + #[test] + fn default_follows_the_credential_not_the_config_cache_front() { + // An env/flag api key resolves through credentials::default_workspace_id, + // which pins the credential's own authorized workspace. The config + // cache front (what `workspaces use` saved for a session) may be a + // workspace this key can't even reach; marking it — or, since `list` + // only returns the key's workspaces, marking nothing at all — would + // break the "exactly one default: true" contract scripts rely on. + let (_tmp, _guard) = config::test_helpers::with_temp_config_dir(); + let mut server = mockito::Server::new(); + let probe = server + .mock("GET", "/workspaces") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"workspaces":[{"public_id":"work_only","name":"Only"}]}"#) + .create(); + let profile = config::ProfileConfig { + api_key: Some("hd_dbtoken".to_string()), + api_key_source: config::ApiKeySource::Env, + api_url: config::ApiUrl(Some(server.url())), + app_url: config::AppUrl(Some(server.url())), + workspaces: vec![config::WorkspaceEntry { + public_id: "work_saved".to_string(), + name: "Saved".to_string(), + }], + ..Default::default() + }; + assert_eq!( + profile_default_workspace_id(&profile).as_deref(), + Some("work_only") + ); + probe.assert(); + } + #[test] fn from_list_item_defaults_to_not_default() { // The wire item carries no default marker — it's stamped from local From c92464b7888068f1a5eeb3017964c47df364acab Mon Sep 17 00:00:00 2001 From: Eddie A Tejeda <669988+eddietejeda@users.noreply.github.com> Date: Wed, 2 Sep 2026 15:41:29 -0700 Subject: [PATCH 7/7] feat(databases): name catalog ids as catalog in json/yaml output runtimedb removed connections as a concept (the engine is push-only: create_connection rejects every source type but `managed`, and v53 migrated external catalogs), so a connection id IS a catalog id. Expose it under that name in `-o json`/`-o yaml`: `default_connection_id` becomes `default_catalog_id` and `attachments[].connection_id` becomes `catalog_id`. The table view already said "catalog id:"; the wire and the internal field names still say connection, so CreateDatabaseResponse renames on serialize only. BREAKING: scripts reading `default_connection_id` from `databases show/create/fork -o json` must switch to `default_catalog_id`. --- src/commands/databases.rs | 53 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/src/commands/databases.rs b/src/commands/databases.rs index cdbd13d..a34b057 100644 --- a/src/commands/databases.rs +++ b/src/commands/databases.rs @@ -386,6 +386,11 @@ pub struct Database { pub name: Option, #[serde(default)] pub default_catalog: Option, + /// Id of the database's own catalog. runtimedb removed connections as a + /// concept (the engine is push-only; every connection row IS a catalog), + /// but the wire field is still named `default_connection_id` — the CLI's + /// own output already speaks catalog. + #[serde(rename = "default_catalog_id")] pub default_connection_id: String, #[serde(default)] pub expires_at: Option, @@ -397,6 +402,9 @@ pub struct Database { #[derive(Clone, Debug, Serialize, PartialEq, Eq)] struct DatabaseAttachment { + /// Catalog id under its post-connections-removal name (see + /// [`Database::default_connection_id`]); also the `detach` handle. + #[serde(rename = "catalog_id")] connection_id: String, alias: Option, } @@ -425,6 +433,9 @@ struct CreateDatabaseResponse { name: Option, #[serde(default)] default_catalog: Option, + /// Parses the server's `default_connection_id`, but `-o json`/`-o yaml` + /// emit it as `default_catalog_id` (see [`Database::default_connection_id`]). + #[serde(rename(serialize = "default_catalog_id"))] default_connection_id: String, #[serde(default)] expires_at: Option, @@ -2186,6 +2197,48 @@ mod tests { ) } + #[test] + fn database_json_names_catalog_ids_as_catalog() { + // runtimedb removed connections as a concept (push-only engine: every + // connection row IS a catalog), so the CLI exposes the ids — they're + // real handles, e.g. for detach — under the catalog name they'll keep. + // Only the wire still says `default_connection_id`. + let db = Database { + id: "db_1".to_string(), + name: Some("sales".to_string()), + default_catalog: Some("sales".to_string()), + default_connection_id: "conn_own".to_string(), + expires_at: None, + created_at: None, + attachments: vec![DatabaseAttachment { + connection_id: "conn_a".to_string(), + alias: Some("gh".to_string()), + }], + }; + let json = serde_json::to_value(&db).unwrap(); + assert_eq!(json["default_catalog_id"], serde_json::json!("conn_own")); + assert!(json.get("default_connection_id").is_none(), "json: {json}"); + assert_eq!( + json["attachments"][0]["catalog_id"], + serde_json::json!("conn_a") + ); + assert!( + json["attachments"][0].get("connection_id").is_none(), + "json: {json}" + ); + } + + #[test] + fn create_response_parses_wire_name_but_emits_catalog_id() { + // The server's create/fork responses still say `default_connection_id`; + // the CLI's own -o json/yaml renames it on the way out. + let resp: CreateDatabaseResponse = + serde_json::from_str(r#"{"id":"db_new","default_connection_id":"conn_abc"}"#).unwrap(); + let json = serde_json::to_value(&resp).unwrap(); + assert_eq!(json["default_catalog_id"], serde_json::json!("conn_abc")); + assert!(json.get("default_connection_id").is_none(), "json: {json}"); + } + /// Like [`full_detail`] but with an explicit catalog, for ambiguity tests. fn detail_with_catalog(id: &str, name: &str, catalog: &str) -> String { format!(