Skip to content

fix(query): resolve --database name/catalog to id #247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
eddietejeda wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(query): resolve --database name/catalog to id #247

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 52 additions & 1 deletion src/commands/query.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,12 +439,23 @@ fn fail_run(error_msg: &str) -> ! {
std::process::exit(1);
}

/// Resolve an explicit `--database` name/catalog/id to its id for the
/// `X-Database-Id` scope, matching `databases delete`/`tables`. The header
/// requires an id, so a bare name/catalog would otherwise 404 as
/// "Database '<name>' not found". `None` (flag omitted) passes through with no
/// lookup so the construction-time default database is kept.
fn resolve_query_database(api: &Api, database: Option<&str>) -> Option<String> {
database.map(|d| crate::commands::databases::resolve_database(api, d).id)
}
Comment on lines +447 to +449

@claude claude Bot Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: this makes name→id resolution unconditional, which regresses --database <id> for database API token credentials.

The repo already documents that such a token can't reach these endpoints:

  • src/commands/databases.rs:1546-1554 — "A database API token can't call GET /v1/databases/{id} (denied by its allow-list), so skip the check for it and save the id directly."
  • src/commands/databases.rs:1689-1695 — "A database API token can't resolve names/catalogs … Route it through the database-scoped endpoints, addressed by database id."

With such a token, hotdata query --database <db_id> "SELECT …" works today because the flag is passed verbatim into X-Database-Id and the server accepts the scoped token. After this change the CLI first calls resolve_database → get_database, which the allow-list denies. none_if_404 (src/client/sdk.rs:384-392) only swallows a 404, so a 403/401 propagates straight to e.exit() inside try_resolve_database (src/commands/databases.rs:494) and the query never runs. If the denial happens to surface as 404, the fallback GET /v1/databases is denied too — same hard failure, or no database with id, catalog, or name '<id>' for a value that is a perfectly valid id.

Suggested fix: skip resolution for that credential and pass the flag through, mirroring the two existing guards. Since this is now the third site inlining the same check, it's worth extracting a helper (e.g. credentials::is_database_api_token()) rather than copying the config::load("default") + api_key_jwt_source dance a third time.

fn resolve_query_database(api: &Api, database: Option<&str>) -> Option<String> {
    let d = database?;
    // A database API token can't resolve names/catalogs (outside its
    // allow-list) — it addresses its database by id, so pass the flag through.
    if crate::client::credentials::is_database_api_token() {
        return Some(d.to_string());
    }
    Some(crate::commands::databases::resolve_database(api, d).id)
}

A unit test covering "database API token + --database <id> performs no lookup" would lock this in alongside the new resolution test.


pub fn execute(sql: &str, workspace_id: &str, database: Option<&str>, format: &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);
let api = Api::new(Some(workspace_id));
let resolved = resolve_query_database(&api, database);
let api = api.scoped_to_database_opt(resolved.as_deref());

@claude claude Bot Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: --database is defined once on the whole Query command (src/cli.rs:33-35) and feeds both subcommands, but only execute resolves it — poll still passes the raw value through (src/commands/query.rs:559, Api::new(..).scoped_to_database_opt(database)). So after this fix hotdata query -d mydb "SELECT …" works while hotdata query status <run_id> -d mydb still fails with Database 'mydb' not found: same flag, two behaviors. Reusing resolve_query_database in poll is a one-liner and closes the gap. (not blocking)

Same raw pass-through also exists in src/commands/queries.rs:212,272 and src/commands/results.rs:61,154 — out of scope here, but if you're moving the helper somewhere shared, those are the other call sites. (not blocking)

let database = api.database_id();

let mut request = hotdata::models::QueryRequest::new(sql.to_string());
Expand Down Expand Up @@ -717,6 +728,46 @@ mod tests {
resp
}

#[test]
fn query_database_flag_resolves_name_to_id() {
// Regression: `hotdata query --database <name>` must resolve a name or
// catalog alias to its id before scoping the X-Database-Id header —
// otherwise the server 404s as "Database '<name>' not found".
let mut server = mockito::Server::new();
// name isn't a valid id → 404, then list matches by name → detail by id.
server
.mock("GET", "/v1/databases/warehouse")
.with_status(404)
.with_body(r#"{"error":"not found"}"#)
.create();
server
.mock("GET", "/v1/databases")
.with_status(200)
.with_header("content-type", "application/json")
.with_body(
r#"{"databases":[{"id":"db_xyz","name":"warehouse","default_catalog":"wh","default_schema":"main"}]}"#,
)
.create();
server
.mock("GET", "/v1/databases/db_xyz")
.with_status(200)
.with_header("content-type", "application/json")
.with_body(
r#"{"id":"db_xyz","name":"warehouse","default_catalog":"wh","default_schema":"main","default_connection_id":"conn_1","attachments":[]}"#,
)
.create();

let api = Api::test_new(&server.url(), "k", Some("ws"));
assert_eq!(
resolve_query_database(&api, Some("warehouse")).as_deref(),
Some("db_xyz"),
"a --database name must resolve to its id"
);
// Flag omitted → no lookup, passes straight through as None so the
// construction-time default database is preserved.
assert_eq!(resolve_query_database(&api, None), None);
}

#[test]
fn hint_for_missing_database_context() {
let tip = cross_source_hint(
Expand Down
Loading