diff --git a/README.md b/README.md index cfb2fffe..78f8f5fb 100644 --- a/README.md +++ b/README.md @@ -224,8 +224,13 @@ clickhousectl local client --name dev # Connects to "dev" server clickhousectl local client --query "SHOW DATABASES" # Run a query clickhousectl local client --queries-file schema.sql # Run queries from a file clickhousectl local client --host remote-host --port 9000 # Connect to a specific host/port +clickhousectl local client --host remote-host --version 26.8.1.1760 # Use an exact installed client binary ``` +Named connections and direct connections select the client binary differently. A named connection uses the version recorded in the managed server's metadata, regardless of the global default. A direct connection (`--host`, `--port`, or both) uses the exact installed version passed with `--version`; if that flag is omitted, it uses the version selected by `local use`. + +Direct connections never infer a binary from the contents of `~/.clickhouse/versions`. With no valid default, omitting `--version` fails whether zero, one, or multiple versions are installed. Use `local list` to find an exact installed version, then pass it with `--version` or select it globally with `local use`. A missing explicit version and a default that points to a removed version both fail with repair instructions. Direct `--version` selection neither installs a binary nor changes the default. + ### Creating and managing ClickHouse servers Start and manage ClickHouse server instances. Each server gets its own isolated data directory at `.clickhouse/servers//data/`. diff --git a/crates/clickhousectl/src/error.rs b/crates/clickhousectl/src/error.rs index 10df5f1b..8ab93232 100644 --- a/crates/clickhousectl/src/error.rs +++ b/crates/clickhousectl/src/error.rs @@ -22,6 +22,21 @@ pub enum Error { #[error("No default version set. Run: clickhousectl local use ")] NoDefaultVersion, + #[error( + "No ClickHouse client version selected for the direct connection. Pass `--version `, or set a default with `clickhousectl local use ` (see `clickhousectl local list`)." + )] + DirectClientVersionRequired, + + #[error( + "ClickHouse client version {0} is not installed. Run `clickhousectl local install {0}`, or choose an exact version from `clickhousectl local list`." + )] + ClientVersionNotInstalled(String), + + #[error( + "Default ClickHouse version {0} is not installed. Repair it with `clickhousectl local use `, or pass `--version ` for this direct connection." + )] + StaleClientDefault(String), + #[error("Version {0} is already installed")] VersionAlreadyInstalled(String), diff --git a/crates/clickhousectl/src/local/cli.rs b/crates/clickhousectl/src/local/cli.rs index d4cd6d66..65d1791c 100644 --- a/crates/clickhousectl/src/local/cli.rs +++ b/crates/clickhousectl/src/local/cli.rs @@ -93,7 +93,9 @@ CONTEXT FOR AGENTS: Init, /// Connect to a running ClickHouse server with clickhouse-client - #[command(after_help = "\ + #[command( + group(clap::ArgGroup::new("direct").args(["host", "port"]).multiple(true)), + after_help = "\ CONTEXT FOR AGENTS: Two connection modes: 1. Named server: `clickhousectl local client --name dev` — looks up port and version from a @@ -103,18 +105,28 @@ CONTEXT FOR AGENTS: 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.")] + 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, conflicts_with_all = ["host", "port"])] + #[arg(long, short, conflicts_with_all = ["host", "port", "version"])] name: Option, + /// Exact installed ClickHouse version for a direct connection; does not change the default + #[arg(long, short = 'v', requires = "direct")] + version: Option, + /// Host to connect to (bypasses local server lookup) - #[arg(long)] + #[arg(long, group = "direct")] host: Option, /// TCP port to connect to (bypasses local server lookup if set) - #[arg(long, short, value_parser = clap::value_parser!(u16).range(1..))] + #[arg( + long, + short, + value_parser = clap::value_parser!(u16).range(1..), + group = "direct" + )] port: Option, /// Execute a SQL query @@ -683,6 +695,77 @@ mod tests { } } + #[test] + fn clickhouse_direct_client_version_selector_matrix() { + let cases = [ + ( + &["--host", "db.example", "--version", "25.12.9.61"][..], + "25.12.9.61", + ), + (&["--port", "9000", "-v", "26.1.2.3"][..], "26.1.2.3"), + ( + &[ + "--version", + "26.2.3.4", + "--host", + "db.example", + "--port", + "9440", + ][..], + "26.2.3.4", + ), + ]; + + for (selectors, expected_version) in cases { + let mut args = vec!["client"]; + args.extend_from_slice(selectors); + let LocalCommands::Client { version, .. } = local_command(&args) else { + panic!("expected local client command"); + }; + assert_eq!( + version.as_deref(), + Some(expected_version), + "selectors: {selectors:?}" + ); + } + } + + #[test] + fn clickhouse_client_version_requires_direct_connection() { + for selectors in [ + &["--version", "25.12.9.61"][..], + &["--name", "dev", "--version", "25.12.9.61"][..], + &["--version", "25.12.9.61", "--name", "dev"][..], + ] { + let mut args = vec!["client"]; + args.extend_from_slice(selectors); + let error = try_local_command(&args) + .err() + .expect("binary version should require direct mode"); + assert!( + matches!( + error.kind(), + clap::error::ErrorKind::MissingRequiredArgument + | clap::error::ErrorKind::ArgumentConflict + ), + "selectors: {selectors:?}\n{error}" + ); + } + } + + #[test] + fn clickhouse_client_help_describes_version_flag() { + let help = Cli::try_parse_from(["clickhousectl", "local", "client", "--help"]) + .err() + .expect("help should exit through clap") + .to_string(); + + assert!( + help.contains("Exact installed ClickHouse version"), + "{help}" + ); + } + #[test] fn parses_remove_without_force() { let LocalCommands::Remove { version, force } = local_command(&["remove", "25.12.5.44"]) diff --git a/crates/clickhousectl/src/local/mod.rs b/crates/clickhousectl/src/local/mod.rs index 5e07f2d5..7d1cefc3 100644 --- a/crates/clickhousectl/src/local/mod.rs +++ b/crates/clickhousectl/src/local/mod.rs @@ -38,12 +38,13 @@ pub async fn run(cmd: LocalCommands, json: bool) -> Result<()> { } LocalCommands::Client { name, + version, host, port, query, queries_file, args, - } => run_client(name, host, port, query, queries_file, args), + } => run_client(name, version, host, port, query, queries_file, args), LocalCommands::Server { command } => run_server_commands(command, json).await, LocalCommands::Postgres { command } => postgres::run(command, json).await, } @@ -253,6 +254,7 @@ fn which(json: bool) -> Result<()> { fn run_client( name: Option, + version: Option, host: Option, port: Option, query: Option, @@ -264,7 +266,7 @@ fn run_client( let (resolved_host, tcp_port, version) = if host.is_some() || port.is_some() { let h = host.unwrap_or_else(|| "localhost".to_string()); let p = port.unwrap_or(9000); - let v = version_manager::get_default_version()?; + let v = resolve_direct_client_version(version.as_deref())?; (h, p, v) } else { let server_name = name.as_deref().unwrap_or("default"); @@ -313,6 +315,24 @@ fn run_client( Err(Error::Exec(err.to_string())) } +fn resolve_direct_client_version(version: Option<&str>) -> Result { + if let Some(version) = version { + let installed = version_manager::list_installed_versions()?; + return installed + .iter() + .any(|installed| installed == version) + .then(|| version.to_string()) + .ok_or_else(|| Error::ClientVersionNotInstalled(version.to_string())); + } + + match version_manager::get_default_version() { + Ok(version) => Ok(version), + Err(Error::NoDefaultVersion) => Err(Error::DirectClientVersionRequired), + Err(Error::VersionNotFound(version)) => Err(Error::StaleClientDefault(version)), + Err(error) => Err(error), + } +} + #[allow(clippy::too_many_arguments)] async fn start_server( name: Option, diff --git a/crates/clickhousectl/tests/local_client_selectors_test.rs b/crates/clickhousectl/tests/local_client_selectors_test.rs index 245d4015..43a1d979 100644 --- a/crates/clickhousectl/tests/local_client_selectors_test.rs +++ b/crates/clickhousectl/tests/local_client_selectors_test.rs @@ -1,26 +1,58 @@ -//! End-to-end coverage for local client selector validation (issue #466). +//! End-to-end coverage for local client selectors (issues #466 and #469). +use serde_json::json; use std::os::unix::fs::PermissionsExt; use std::path::{Path, PathBuf}; use std::process::{Command, Output}; -const VERSION: &str = "25.12.9.61"; +const VERSION_A: &str = "25.12.9.61"; +const VERSION_B: &str = "26.1.2.3"; +const MISSING_VERSION: &str = "24.8.99.1"; fn clickhousectl_binary() -> PathBuf { PathBuf::from(env!("CARGO_BIN_EXE_clickhousectl")) } -fn install_fake_clickhouse(home: &Path) { +fn install_fake_clickhouse(home: &Path, version: &str) { let binary = home .join(".clickhouse/versions") - .join(VERSION) + .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"); + std::fs::write( + &binary, + format!("#!/bin/sh\nprintf 'binary={version}\\n'\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 set_default(home: &Path, version: &str) { + let base = home.join(".clickhouse"); + std::fs::create_dir_all(&base).expect("create ClickHouse home"); + std::fs::write(base.join("default"), version).expect("write default version"); +} + +fn write_server_metadata(project: &Path, name: &str, version: &str, tcp_port: u16) { + let servers = project.join(".clickhouse/servers"); + std::fs::create_dir_all(&servers).expect("create servers dir"); + std::fs::write( + servers.join(format!("{name}.json")), + serde_json::to_vec_pretty(&json!({ + "name": name, + "pid": std::process::id(), + "version": version, + "http_port": 8123, + "tcp_port": tcp_port, + "started_at": "1700000000", + "cwd": project.display().to_string(), + "engine": "clickhouse" + })) + .unwrap(), + ) + .expect("write server metadata"); } fn run(project: &Path, home: &Path, args: &[&str]) -> Output { @@ -33,11 +65,28 @@ fn run(project: &Path, home: &Path, args: &[&str]) -> Output { .expect("run clickhousectl") } +fn assert_client(output: Output, version: &str, expected_args: &[&str]) { + assert!( + output.status.success(), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let lines: Vec<_> = String::from_utf8(output.stdout) + .expect("fake ClickHouse output should be UTF-8") + .lines() + .map(str::to_string) + .collect(); + let mut expected = vec![format!("binary={version}")]; + expected.extend(expected_args.iter().map(|arg| (*arg).to_string())); + assert_eq!(lines, expected); +} + #[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()); + install_fake_clickhouse(home.path(), VERSION_A); + set_default(home.path(), VERSION_A); let cases = [ ( @@ -64,12 +113,227 @@ fn clickhouse_direct_client_defaults_missing_host_or_port() { let forwarded: Vec<_> = String::from_utf8(output.stdout) .expect("fake ClickHouse output should be UTF-8") .lines() + .skip(1) .map(str::to_string) .collect(); assert_eq!(forwarded, expected, "args: {args:?}"); } } +#[test] +fn direct_client_without_explicit_version_uses_only_a_valid_default() { + struct Case { + installed: &'static [&'static str], + default: Option<&'static str>, + expected_binary: Option<&'static str>, + expected_error: Option<&'static str>, + } + + let cases = [ + Case { + installed: &[], + default: None, + expected_binary: None, + expected_error: Some("No ClickHouse client version selected"), + }, + Case { + installed: &[VERSION_A], + default: None, + expected_binary: None, + expected_error: Some("No ClickHouse client version selected"), + }, + Case { + installed: &[VERSION_A, VERSION_B], + default: None, + expected_binary: None, + expected_error: Some("No ClickHouse client version selected"), + }, + Case { + installed: &[VERSION_A], + default: Some(VERSION_A), + expected_binary: Some(VERSION_A), + expected_error: None, + }, + Case { + installed: &[VERSION_A, VERSION_B], + default: Some(VERSION_B), + expected_binary: Some(VERSION_B), + expected_error: None, + }, + Case { + installed: &[VERSION_A], + default: Some(MISSING_VERSION), + expected_binary: None, + expected_error: Some("Default ClickHouse version 24.8.99.1 is not installed"), + }, + ]; + + for case in cases { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + for version in case.installed { + install_fake_clickhouse(home.path(), version); + } + if let Some(version) = case.default { + set_default(home.path(), version); + } + + let output = run( + project.path(), + home.path(), + &["local", "client", "--host", "db.example"], + ); + if let Some(version) = case.expected_binary { + assert_client( + output, + version, + &["client", "--host", "db.example", "--port", "9000"], + ); + } else { + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains(case.expected_error.unwrap()), + "installed: {:?}, default: {:?}\nstderr: {stderr}", + case.installed, + case.default + ); + assert!( + stderr.contains("clickhousectl local use") + || stderr.contains("clickhousectl local list"), + "stderr should be actionable: {stderr}" + ); + } + } +} + +#[test] +fn direct_client_explicit_version_matrix_is_installed_only_and_preserves_default() { + struct Case { + installed: &'static [&'static str], + default: Option<&'static str>, + requested: &'static str, + expected_binary: Option<&'static str>, + } + + let cases = [ + Case { + installed: &[], + default: None, + requested: VERSION_A, + expected_binary: None, + }, + Case { + installed: &[VERSION_A], + default: None, + requested: VERSION_A, + expected_binary: Some(VERSION_A), + }, + Case { + installed: &[VERSION_A, VERSION_B], + default: Some(VERSION_A), + requested: VERSION_B, + expected_binary: Some(VERSION_B), + }, + Case { + installed: &[VERSION_A, VERSION_B], + default: Some(MISSING_VERSION), + requested: VERSION_B, + expected_binary: Some(VERSION_B), + }, + Case { + installed: &[VERSION_A], + default: Some(VERSION_A), + requested: VERSION_B, + expected_binary: None, + }, + ]; + + for case in cases { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + for version in case.installed { + install_fake_clickhouse(home.path(), version); + } + if let Some(version) = case.default { + set_default(home.path(), version); + } + + let output = run( + project.path(), + home.path(), + &[ + "local", + "client", + "--port", + "9440", + "--version", + case.requested, + ], + ); + if let Some(version) = case.expected_binary { + assert_client( + output, + version, + &["client", "--host", "localhost", "--port", "9440"], + ); + } else { + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains(&format!( + "ClickHouse client version {} is not installed", + case.requested + )), + "stderr: {stderr}" + ); + assert!( + stderr.contains(&format!("clickhousectl local install {}", case.requested)) + && stderr.contains("clickhousectl local list"), + "stderr should be actionable: {stderr}" + ); + } + + let default_file = home.path().join(".clickhouse/default"); + match case.default { + Some(expected) => assert_eq!( + std::fs::read_to_string(default_file).unwrap(), + expected, + "explicit selection must preserve the default" + ), + None => assert!( + !default_file.exists(), + "explicit selection must not create a default" + ), + } + } +} + +#[test] +fn named_client_uses_recorded_version_even_with_a_stale_default() { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + install_fake_clickhouse(home.path(), VERSION_A); + install_fake_clickhouse(home.path(), VERSION_B); + set_default(home.path(), MISSING_VERSION); + write_server_metadata(project.path(), "dev", VERSION_B, 9440); + + let output = run( + project.path(), + home.path(), + &["local", "client", "--name", "dev"], + ); + assert_client( + output, + VERSION_B, + &["client", "--host", "localhost", "--port", "9440"], + ); + assert_eq!( + std::fs::read_to_string(home.path().join(".clickhouse/default")).unwrap(), + MISSING_VERSION + ); +} + #[test] fn invalid_client_selectors_are_usage_errors_before_resolution() { let project = tempfile::tempdir().expect("create project tempdir"); @@ -78,6 +342,9 @@ fn invalid_client_selectors_are_usage_errors_before_resolution() { &["local", "client", "--name", "dev", "--host", "db.example"][..], &["local", "client", "--port", "9000", "--name", "dev"][..], &["local", "client", "--port", "0"][..], + &["local", "client", "--version", VERSION_A][..], + &["local", "client", "--name", "dev", "--version", VERSION_A][..], + &["local", "client", "--version", VERSION_A, "--name", "dev"][..], &[ "local", "postgres",