diff --git a/crates/clickhousectl/src/local/cli.rs b/crates/clickhousectl/src/local/cli.rs index ccf6bb17..d4cd6d66 100644 --- a/crates/clickhousectl/src/local/cli.rs +++ b/crates/clickhousectl/src/local/cli.rs @@ -98,14 +98,15 @@ CONTEXT FOR AGENTS: Two connection modes: 1. Named server: `clickhousectl local client --name dev` — looks up port and version from a locally managed server started via `clickhousectl local server start`. Defaults to \"default\". - 2. Explicit host/port: `clickhousectl local client --host myhost --port 9000` — connects to any - ClickHouse server directly, bypassing local server lookup. + 2. Direct: pass --host, --port, or both to bypass local server lookup. A missing host defaults + to localhost (for example, `local client --port 9000`); a missing port defaults to 9000. + Named and direct selectors cannot be combined. --query and --queries-file execute SQL inline or from a file. Additional clickhouse-client args can be passed after --. Related: `clickhousectl local server start` to start a local server, `clickhousectl local server list` to see servers.")] Client { /// Server name to connect to (default: "default") - #[arg(long, short)] + #[arg(long, short, conflicts_with_all = ["host", "port"])] name: Option, /// Host to connect to (bypasses local server lookup) @@ -113,7 +114,7 @@ CONTEXT FOR AGENTS: host: Option, /// TCP port to connect to (bypasses local server lookup if set) - #[arg(long, short)] + #[arg(long, short, value_parser = clap::value_parser!(u16).range(1..))] port: Option, /// Execute a SQL query @@ -420,18 +421,20 @@ CONTEXT FOR AGENTS: 1. Named server: `clickhousectl local postgres client --name dev` — looks up the host port and credentials from a locally managed Postgres started via `local postgres start`. Defaults to \"default\". - 2. Explicit host/port: `clickhousectl local postgres client --host myhost --port 5432`. + 2. Direct: pass --host, --port, or both to bypass local server lookup. A missing host defaults + to 127.0.0.1; a missing port defaults to 5432. + Managed selectors (--name and --version) cannot be combined with direct selectors. If `psql` is on PATH on the host, it is execed directly. Otherwise, falls back to running `psql` inside the container via Docker exec (no host psql required). --query and --queries-file pass through to psql (-c / -f). Additional psql args can be passed after --.")] Client { /// Server name to connect to (default: "default") - #[arg(long, short)] + #[arg(long, short, conflicts_with_all = ["host", "port"])] name: Option, /// Postgres version to disambiguate when multiple share a name - #[arg(long, short = 'v')] + #[arg(long, short = 'v', conflicts_with_all = ["host", "port"])] version: Option, /// Host to connect to (bypasses local server lookup) @@ -439,7 +442,7 @@ CONTEXT FOR AGENTS: host: Option, /// TCP port to connect to (bypasses local server lookup if set) - #[arg(long, short)] + #[arg(long, short, value_parser = clap::value_parser!(u16).range(1..))] port: Option, /// Execute a single SQL query @@ -483,13 +486,42 @@ mod tests { use clap::Parser; fn local_command(args: &[&str]) -> LocalCommands { + try_local_command(args).unwrap() + } + + fn try_local_command(args: &[&str]) -> Result { let mut argv = vec!["clickhousectl", "local"]; argv.extend_from_slice(args); - let cli = Cli::try_parse_from(argv).unwrap(); + let cli = Cli::try_parse_from(argv)?; let Commands::Local(local) = cli.command else { panic!("expected local command"); }; - local.command + Ok(local.command) + } + + fn client_selectors( + postgres: bool, + selectors: &[&str], + ) -> (Option, Option, Option) { + let mut args = if postgres { + vec!["postgres", "client"] + } else { + vec!["client"] + }; + args.extend_from_slice(selectors); + + match local_command(&args) { + LocalCommands::Client { + name, host, port, .. + } + | LocalCommands::Postgres { + command: + PostgresCommands::Client { + name, host, port, .. + }, + } => (name, host, port), + _ => panic!("expected local client command"), + } } #[test] @@ -506,6 +538,151 @@ mod tests { assert!(help.contains("`clickhouse format`"), "{help}"); } + #[test] + fn client_selector_matrix_accepts_unambiguous_modes_and_port_bounds() { + let cases = [ + (&[][..], None, None, None), + (&["--name", "dev"][..], Some("dev"), None, None), + ( + &["--host", "db.example"][..], + None, + Some("db.example"), + None, + ), + (&["--port", "1"][..], None, None, Some(1)), + (&["--port", "65535"][..], None, None, Some(65535)), + ( + &["--host", "db.example", "--port", "9440"][..], + None, + Some("db.example"), + Some(9440), + ), + ( + &["--port", "9440", "--host", "db.example"][..], + None, + Some("db.example"), + Some(9440), + ), + ]; + + for postgres in [false, true] { + for (selectors, expected_name, expected_host, expected_port) in cases { + let (name, host, port) = client_selectors(postgres, selectors); + assert_eq!(name.as_deref(), expected_name, "selectors: {selectors:?}"); + assert_eq!(host.as_deref(), expected_host, "selectors: {selectors:?}"); + assert_eq!(port, expected_port, "selectors: {selectors:?}"); + } + } + } + + #[test] + fn postgres_client_version_accepts_managed_modes() { + let cases = [ + (&["--version", "17"][..], None), + (&["--name", "dev", "--version", "17"][..], Some("dev")), + (&["--version", "17", "--name", "dev"][..], Some("dev")), + ]; + + for (selectors, expected_name) in cases { + let mut args = vec!["postgres", "client"]; + args.extend_from_slice(selectors); + let LocalCommands::Postgres { + command: + PostgresCommands::Client { + name, + version, + host, + port, + .. + }, + } = local_command(&args) + else { + panic!("expected postgres client command"); + }; + + assert_eq!(name.as_deref(), expected_name, "selectors: {selectors:?}"); + assert_eq!(version.as_deref(), Some("17"), "selectors: {selectors:?}"); + assert_eq!(host, None, "selectors: {selectors:?}"); + assert_eq!(port, None, "selectors: {selectors:?}"); + } + } + + #[test] + fn postgres_client_version_rejects_direct_modes_in_either_order() { + let cases = [ + &["--version", "17", "--host", "db.example"][..], + &["--host", "db.example", "--version", "17"][..], + &["--version", "17", "--port", "5432"][..], + &["--port", "5432", "--version", "17"][..], + ]; + + for selectors in cases { + let mut args = vec!["postgres", "client"]; + args.extend_from_slice(selectors); + let error = try_local_command(&args) + .err() + .expect("managed and direct selectors should conflict"); + assert_eq!( + error.kind(), + clap::error::ErrorKind::ArgumentConflict, + "selectors: {selectors:?}" + ); + } + } + + #[test] + fn client_selector_matrix_rejects_named_and_direct_modes_in_either_order() { + let cases = [ + &["--name", "dev", "--host", "db.example"][..], + &["--host", "db.example", "--name", "dev"][..], + &["--name", "dev", "--port", "9000"][..], + &["--port", "9000", "--name", "dev"][..], + &["--name", "dev", "--host", "db.example", "--port", "9000"][..], + &["--host", "db.example", "--port", "9000", "--name", "dev"][..], + ]; + + for postgres in [false, true] { + for selectors in cases { + let mut args = if postgres { + vec!["postgres", "client"] + } else { + vec!["client"] + }; + args.extend_from_slice(selectors); + let error = try_local_command(&args) + .err() + .expect("selectors should conflict"); + assert_eq!( + error.kind(), + clap::error::ErrorKind::ArgumentConflict, + "selectors: {selectors:?}" + ); + } + } + } + + #[test] + fn client_ports_reject_zero_and_nonnumeric_values() { + for postgres in [false, true] { + for port in ["0", "not-a-port"] { + let mut args = if postgres { + vec!["postgres", "client"] + } else { + vec!["client"] + }; + args.extend(["--port", port]); + let error = try_local_command(&args) + .err() + .expect("port should be invalid"); + assert_eq!( + error.kind(), + clap::error::ErrorKind::ValueValidation, + "port: {port}" + ); + } + } + } + #[test] fn parses_remove_without_force() { let LocalCommands::Remove { version, force } = local_command(&["remove", "25.12.5.44"]) diff --git a/crates/clickhousectl/tests/local_client_selectors_test.rs b/crates/clickhousectl/tests/local_client_selectors_test.rs new file mode 100644 index 00000000..245d4015 --- /dev/null +++ b/crates/clickhousectl/tests/local_client_selectors_test.rs @@ -0,0 +1,110 @@ +//! End-to-end coverage for local client selector validation (issue #466). + +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +const VERSION: &str = "25.12.9.61"; + +fn clickhousectl_binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_clickhousectl")) +} + +fn install_fake_clickhouse(home: &Path) { + let binary = home + .join(".clickhouse/versions") + .join(VERSION) + .join("clickhouse"); + std::fs::create_dir_all(binary.parent().unwrap()).expect("create fake version dir"); + std::fs::write(&binary, b"#!/bin/sh\nprintf '%s\\n' \"$@\"\n").expect("write fake ClickHouse"); + let mut permissions = std::fs::metadata(&binary).unwrap().permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(binary, permissions).expect("make fake ClickHouse executable"); + std::fs::write(home.join(".clickhouse/default"), VERSION).expect("write default version"); +} + +fn run(project: &Path, home: &Path, args: &[&str]) -> Output { + Command::new(clickhousectl_binary()) + .env("DO_NOT_TRACK", "1") + .env("HOME", home) + .current_dir(project) + .args(args) + .output() + .expect("run clickhousectl") +} + +#[test] +fn clickhouse_direct_client_defaults_missing_host_or_port() { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + install_fake_clickhouse(home.path()); + + let cases = [ + ( + &["local", "client", "--host", "db.example"][..], + &["client", "--host", "db.example", "--port", "9000"][..], + ), + ( + &["local", "client", "--port", "1"][..], + &["client", "--host", "localhost", "--port", "1"][..], + ), + ( + &["local", "client", "--port", "65535"][..], + &["client", "--host", "localhost", "--port", "65535"][..], + ), + ]; + + for (args, expected) in cases { + let output = run(project.path(), home.path(), args); + assert!( + output.status.success(), + "args: {args:?}\nstderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let forwarded: Vec<_> = String::from_utf8(output.stdout) + .expect("fake ClickHouse output should be UTF-8") + .lines() + .map(str::to_string) + .collect(); + assert_eq!(forwarded, expected, "args: {args:?}"); + } +} + +#[test] +fn invalid_client_selectors_are_usage_errors_before_resolution() { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + let cases = [ + &["local", "client", "--name", "dev", "--host", "db.example"][..], + &["local", "client", "--port", "9000", "--name", "dev"][..], + &["local", "client", "--port", "0"][..], + &[ + "local", + "postgres", + "client", + "--host", + "db.example", + "--name", + "dev", + ][..], + &[ + "local", "postgres", "client", "--name", "dev", "--port", "5432", + ][..], + &["local", "postgres", "client", "--port", "0"][..], + ]; + + for args in cases { + let output = run(project.path(), home.path(), args); + assert_eq!(output.status.code(), Some(2), "args: {args:?}"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("error:"), + "args: {args:?}\nstderr: {stderr}" + ); + assert!( + !stderr.contains("No default version configured") + && !stderr.contains("Server 'dev' not found"), + "args: {args:?}\nstderr: {stderr}" + ); + } +}