Skip to content

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

Draft
eddietejeda wants to merge 1 commit into
mainfrom
fix/query-database-name-resolution
Draft

fix(query): resolve --database name/catalog to id#247
eddietejeda wants to merge 1 commit into
mainfrom
fix/query-database-name-resolution

Conversation

@eddietejeda

Copy link
Copy Markdown
Contributor

Summary

hotdata query --database <NAME> (or a catalog alias) failed with Database '<name>' not found, even when the database existed — only the opaque database id worked. This was inconsistent with databases tables, databases tables delete --database, and databases delete, which all accept a name or id. It also broke every hotdata query … --database <name> example in the quick-start docs (docs corrected separately in www.hotdata.dev#297).

Root cause

query::execute passed the raw --database flag straight into scoped_to_database_opt, which stores it verbatim as the X-Database-Id header value (src/client/sdk.rs):

pub fn scoped_to_database_opt(mut self, database: Option<&str>) -> Self {
    if let Some(db) = database { self.database_id = Some(db.to_string()); } // no name -> id lookup
    self
}

It never ran the name→id resolution the other subcommands perform via resolve_database (src/commands/databases.rs), so the server rejected any value that wasn't already an id.

Fix

Resolve the flag through resolve_database (id-first, then catalog alias, then name — with the existing ambiguity handling) before scoping the API. Omitting the flag still passes through untouched, preserving the construction-time default database (HOTDATA_DATABASE / current 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());

Testing

  • New unit test query_database_flag_resolves_name_to_id: a --database name resolves to its id; None passes through with no lookup.
  • Full suite green (304 unit tests + integration).
  • Verified end-to-end against the live API with a freshly built binary:
--database value before after
name (fixtest) Database 'fixtest' not found ✅ works
catalog (fixcat) Database 'fixcat' not found ✅ works
id (dbid…) ✅ works ✅ works
omitted (catalog-qualified query) ✅ works ✅ works
unknown value error: no database with id, catalog, or name '…' (same as other subcommands)

Related

  • Docs fix: hotdata-dev/www.hotdata.dev#297
  • Separate open item (server-side, not addressed here): the query engine rejects projections with no real column (SELECT COUNT(*), SELECT 1) with "must either specify a row count or at least one column".

`hotdata query --database <name>` (or a catalog alias) failed with
"Database '<name>' not found" -- only an opaque database id worked --
while `databases tables`, `tables delete --database`, and
`databases delete` all accept a name or id.

Root cause: query::execute passed the raw --database flag straight to
scoped_to_database_opt, which stores it verbatim as the X-Database-Id
header value. It never ran the name->id lookup the other subcommands do
via resolve_database, so the server rejected any non-id value.

Fix: resolve the flag through resolve_database (id-first, then catalog
alias, then name) before scoping. Omitting the flag still passes through
untouched, preserving the construction-time default database.

Added a regression test asserting a --database name resolves to its id
and that None passes through without a lookup. Verified end-to-end
against the live API: name, catalog, id, and no-flag all work; an unknown
value gives the same clean error as the other subcommands.
@eddietejeda
eddietejeda requested a review from a team as a code owner August 3, 2026 23:22
@eddietejeda
eddietejeda requested review from shefeek-jinnah and removed request for a team August 3, 2026 23:22
@eddietejeda
eddietejeda marked this pull request as draft August 3, 2026 23:23
@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.00000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/commands/query.rs 90.00% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

Comment thread src/commands/query.rs
Comment on lines +447 to +449
fn resolve_query_database(api: &Api, database: Option<&str>) -> Option<String> {
database.map(|d| crate::commands::databases::resolve_database(api, d).id)
}

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_databaseget_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.

Comment thread src/commands/query.rs
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());

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)

@claude claude Bot left a comment

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.

Review

Blocking Issues

  • src/commands/query.rs:447-449resolve_query_database resolves the --database flag unconditionally, which regresses hotdata query --database <db_id> for database API token credentials. Those tokens cannot call GET /v1/databases/{id} or GET /v1/databases (allow-list denied — documented at src/commands/databases.rs:1546-1554 and 1689-1695, which both branch around it). none_if_404 (src/client/sdk.rs:384-392) only swallows 404, so the denial propagates to e.exit() in try_resolve_database (src/commands/databases.rs:494) and the query never runs — for a value that is a valid id and works today.

Action Required

  • Skip name/id resolution when the credential is a database API token and pass the flag through verbatim, mirroring the existing guards in databases.rs. Since this would be the third copy of that check, extract it (e.g. credentials::is_database_api_token()).
  • Add a unit test asserting that a database API token with --database <id> performs no lookup.

Non-blocking notes left inline (query status still takes the raw flag).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant