Skip to content

Commit 52642e2

Browse files
eddietejedaclaude
andcommitted
fix: address review feedback on dot-bracket notation
- Dataset auto-name now includes columns (dataset_{cols}_{type}) to avoid collision - Validate bracket closure in parse_index_target; reject unclosed/garbled brackets - Add quoting note to indexes create doc comment for shell glob safety - resolve_connection_id tries GET /connections/{id} directly for conn* prefixed IDs - Add unit tests for parse_db_target and parse_index_target (6 tests) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 74e39ce commit 52642e2

3 files changed

Lines changed: 84 additions & 3 deletions

File tree

src/command.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,9 @@ pub enum IndexesCommands {
330330
Create {
331331
/// Table and columns to index: `connection.table[col1,col2]`
332332
/// or `connection.schema.table[col1,col2]`. Schema defaults to `public`.
333+
///
334+
/// Quote the argument to prevent shell glob expansion:
335+
/// `hotdata indexes create 'airbnb.listings[description]' --type bm25`
333336
#[arg(conflicts_with = "dataset_id")]
334337
target: Option<String>,
335338

src/connections.rs

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,20 @@ struct ListResponse {
158158
}
159159

160160
/// Resolve a connection name or ID to a connection ID, exiting on failure.
161+
///
162+
/// If `name_or_id` looks like a raw connection ID (starts with "conn"), tries
163+
/// `GET /connections/{id}` directly first to avoid listing the full workspace.
164+
/// Falls back to listing and matching by name on a 404 or when given a plain name.
161165
pub fn resolve_connection_id(api: &ApiClient, name_or_id: &str) -> String {
166+
use crossterm::style::Stylize;
167+
168+
if name_or_id.starts_with("conn") {
169+
let (status, _) = api.get_raw(&format!("/connections/{name_or_id}"));
170+
if status.is_success() {
171+
return name_or_id.to_string();
172+
}
173+
}
174+
162175
let body: ListResponse = api.get("/connections");
163176
match body
164177
.connections
@@ -167,7 +180,6 @@ pub fn resolve_connection_id(api: &ApiClient, name_or_id: &str) -> String {
167180
{
168181
Some(conn) => conn.id.clone(),
169182
None => {
170-
use crossterm::style::Stylize;
171183
eprintln!(
172184
"{}",
173185
format!("error: no connection named or with id '{name_or_id}'").red()

src/main.rs

Lines changed: 68 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -621,7 +621,11 @@ fn main() {
621621
);
622622
std::process::exit(1);
623623
});
624-
let auto = format!("dataset_{type}", type = r#type);
624+
let auto = format!(
625+
"dataset_{cols}_{type}",
626+
cols = cols.replace(',', "_"),
627+
type = r#type
628+
);
625629
(
626630
(did.to_string(), String::new(), String::new()),
627631
cols.to_string(),
@@ -984,8 +988,14 @@ fn parse_index_target(target: &str) -> (String, String, String, Vec<String>) {
984988
);
985989
std::process::exit(1);
986990
};
991+
if !target.ends_with(']') {
992+
eprintln!(
993+
"error: target bracket is not closed — use e.g. 'airbnb.listings[col1,col2]'"
994+
);
995+
std::process::exit(1);
996+
}
987997
let table_part = &target[..bracket_pos];
988-
let cols_raw = target[bracket_pos + 1..].trim_end_matches(']');
998+
let cols_raw = &target[bracket_pos + 1..target.len() - 1];
989999

9901000
let parts: Vec<&str> = table_part.splitn(4, '.').collect();
9911001
let (conn, schema, table) = match parts.as_slice() {
@@ -1013,6 +1023,62 @@ fn parse_index_target(target: &str) -> (String, String, String, Vec<String>) {
10131023
(conn, schema, table, columns)
10141024
}
10151025

1026+
#[cfg(test)]
1027+
mod tests {
1028+
use super::*;
1029+
1030+
// --- parse_db_target ---
1031+
1032+
#[test]
1033+
fn db_target_two_parts_defaults_schema_to_public() {
1034+
let (db, schema, table) = parse_db_target("airbnb.listings");
1035+
assert_eq!(db, "airbnb");
1036+
assert_eq!(schema, "public");
1037+
assert_eq!(table, "listings");
1038+
}
1039+
1040+
#[test]
1041+
fn db_target_three_parts_uses_explicit_schema() {
1042+
let (db, schema, table) = parse_db_target("airbnb.staging.listings");
1043+
assert_eq!(db, "airbnb");
1044+
assert_eq!(schema, "staging");
1045+
assert_eq!(table, "listings");
1046+
}
1047+
1048+
// --- parse_index_target ---
1049+
1050+
#[test]
1051+
fn index_target_two_parts_defaults_schema_to_public() {
1052+
let (conn, schema, table, cols) = parse_index_target("airbnb.listings[description]");
1053+
assert_eq!(conn, "airbnb");
1054+
assert_eq!(schema, "public");
1055+
assert_eq!(table, "listings");
1056+
assert_eq!(cols, vec!["description"]);
1057+
}
1058+
1059+
#[test]
1060+
fn index_target_three_parts_uses_explicit_schema() {
1061+
let (conn, schema, table, cols) =
1062+
parse_index_target("airbnb.public.listings[name,description]");
1063+
assert_eq!(conn, "airbnb");
1064+
assert_eq!(schema, "public");
1065+
assert_eq!(table, "listings");
1066+
assert_eq!(cols, vec!["name", "description"]);
1067+
}
1068+
1069+
#[test]
1070+
fn index_target_multiple_columns() {
1071+
let (_, _, _, cols) = parse_index_target("db.tbl[a,b,c]");
1072+
assert_eq!(cols, vec!["a", "b", "c"]);
1073+
}
1074+
1075+
#[test]
1076+
fn index_target_trims_column_whitespace() {
1077+
let (_, _, _, cols) = parse_index_target("db.tbl[a, b]");
1078+
assert_eq!(cols, vec!["a", "b"]);
1079+
}
1080+
}
1081+
10161082
pub fn get_styles() -> clap::builder::Styles {
10171083
Styles::styled()
10181084
.header(AnsiColor::Yellow.on_default())

0 commit comments

Comments
 (0)