diff --git a/crates/clickhousectl/src/local/cli.rs b/crates/clickhousectl/src/local/cli.rs index aa66d61d..31c31df0 100644 --- a/crates/clickhousectl/src/local/cli.rs +++ b/crates/clickhousectl/src/local/cli.rs @@ -1,5 +1,27 @@ +use crate::version_manager::spec::VersionSpec; use clap::{Args, Subcommand}; +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum InstallVersionOperand { + ClickHouse(VersionSpec), + Postgres(String), +} + +fn parse_clickhouse_version_operand(input: &str) -> Result { + crate::version_manager::parse_version_spec(input).map_err(|error| error.to_string()) +} + +fn parse_install_version_operand(input: &str) -> Result { + if let Some(tag) = input + .strip_prefix("postgres@") + .or_else(|| input.strip_prefix("postgres:")) + { + return Ok(InstallVersionOperand::Postgres(tag.to_string())); + } + + parse_clickhouse_version_operand(input).map(InstallVersionOperand::ClickHouse) +} + #[derive(Args)] pub struct LocalArgs { /// Output as JSON @@ -18,7 +40,8 @@ CONTEXT FOR AGENTS: `clickhousectl local use ` will auto-install if the version is missing and set as default.")] Install { /// Version to install. Accepts: "latest" (recommended), "stable", "lts", partial like "25.12", or exact like "25.12.9.61". - version: String, + #[arg(value_parser = parse_install_version_operand)] + version: InstallVersionOperand, /// Force re-install even if version is already installed #[arg(long)] @@ -49,7 +72,8 @@ CONTEXT FOR AGENTS: Related: `clickhousectl local which` to verify, `clickhousectl local server start` to start a server.")] Use { /// Version to use as default. Accepts: "latest" (recommended), "stable", "lts", partial like "25.12", or exact like "25.12.5.44". - version: String, + #[arg(value_parser = parse_clickhouse_version_operand)] + version: VersionSpec, /// Do not create or update the ~/.local/bin/clickhouse symlink #[arg(long)] @@ -212,8 +236,8 @@ CONTEXT FOR AGENTS: name_flag: Option, /// ClickHouse version to use (e.g. "latest" (recommended), stable, lts, 25.12). Installs if needed. Does not change the default version. - #[arg(long, short = 'v')] - version: Option, + #[arg(long, short = 'v', value_parser = parse_clickhouse_version_operand)] + version: Option, /// HTTP port (default: 8123, auto-assigns a free port if in use) #[arg(long)] @@ -538,6 +562,34 @@ mod tests { } } + fn valid_version_operands() -> Vec<(&'static str, VersionSpec)> { + vec![ + ("latest", VersionSpec::Latest), + ( + "stable", + VersionSpec::Channel(crate::version_manager::list::Channel::Stable), + ), + ( + "lts", + VersionSpec::Channel(crate::version_manager::list::Channel::Lts), + ), + ("25", VersionSpec::Major(25)), + ("25.12", VersionSpec::Minor(25, 12)), + ("25.12.9.61", VersionSpec::Exact("25.12.9.61".to_string())), + ] + } + + fn assert_version_parse_error(args: &[&str], expected: &str) { + let mut argv = vec!["clickhousectl", "local"]; + argv.extend_from_slice(args); + let error = Cli::try_parse_from(argv) + .err() + .expect("invalid version should fail during parsing"); + assert_eq!(error.kind(), clap::error::ErrorKind::ValueValidation); + assert_eq!(error.exit_code(), 2); + assert!(error.to_string().contains(expected), "{error}"); + } + #[test] fn use_help_documents_standard_clickhouse_subcommands() { let error = Cli::try_parse_from(["clickhousectl", "local", "use", "--help"]) @@ -589,6 +641,16 @@ mod tests { } } + #[test] + fn install_accepts_documented_clickhouse_version_operands() { + for (input, expected) in valid_version_operands() { + let LocalCommands::Install { version, .. } = local_command(&["install", input]) else { + panic!("expected install"); + }; + assert_eq!(version, InstallVersionOperand::ClickHouse(expected)); + } + } + #[test] fn postgres_client_version_accepts_managed_modes() { let cases = [ @@ -675,6 +737,16 @@ mod tests { } } + #[test] + fn use_accepts_documented_clickhouse_version_operands() { + for (input, expected) in valid_version_operands() { + let LocalCommands::Use { version, .. } = local_command(&["use", input]) else { + panic!("expected use"); + }; + assert_eq!(version, expected); + } + } + #[test] fn client_ports_reject_zero_and_nonnumeric_values() { for postgres in [false, true] { @@ -697,6 +769,19 @@ mod tests { } } + #[test] + fn server_start_accepts_documented_clickhouse_version_operands() { + for (input, expected) in valid_version_operands() { + let LocalCommands::Server { + command: ServerCommands::Start { version, .. }, + } = local_command(&["server", "start", "--version", input]) + else { + panic!("expected server start"); + }; + assert_eq!(version, Some(expected)); + } + } + #[test] fn clickhouse_direct_client_version_selector_matrix() { let cases = [ @@ -732,6 +817,19 @@ mod tests { } } + #[test] + fn install_preserves_postgres_version_operand_forms() { + for (input, expected) in [("postgres@18", "18"), ("postgres:17-alpine", "17-alpine")] { + let LocalCommands::Install { version, .. } = local_command(&["install", input]) else { + panic!("expected install"); + }; + assert_eq!( + version, + InstallVersionOperand::Postgres(expected.to_string()) + ); + } + } + #[test] fn clickhouse_client_version_requires_direct_connection() { for selectors in [ @@ -863,6 +961,16 @@ mod tests { } } + #[test] + fn use_and_server_reject_postgres_install_operands() { + for args in [ + &["use", "postgres@18"][..], + &["server", "start", "--version", "postgres:18"][..], + ] { + assert_version_parse_error(args, "all parts must be numeric"); + } + } + #[test] fn clickhouse_client_query_help_documents_native_multiplicity_and_exclusion() { let help = Cli::try_parse_from(["clickhousectl", "local", "client", "--help"]) @@ -881,6 +989,28 @@ mod tests { ); } + #[test] + fn clickhouse_version_operands_reject_malformed_values() { + for args in [ + &["install", "not.a.version"][..], + &["use", "not.a.version"][..], + &["server", "start", "--version", "not.a.version"][..], + ] { + assert_version_parse_error(args, "all parts must be numeric"); + } + } + + #[test] + fn clickhouse_version_operands_reject_three_part_versions() { + for args in [ + &["install", "25.12.9"][..], + &["use", "25.12.9"][..], + &["server", "start", "--version", "25.12.9"][..], + ] { + assert_version_parse_error(args, "3-part version '25.12.9' is not supported"); + } + } + #[test] fn parses_remove_without_force() { let LocalCommands::Remove { version, force } = local_command(&["remove", "25.12.5.44"]) @@ -976,7 +1106,7 @@ mod tests { }; assert_eq!(name.as_deref(), Some("existing")); assert_eq!(name_flag, None); - assert_eq!(version.as_deref(), Some("25.12.9.61")); + assert_eq!(version, Some(VersionSpec::Exact("25.12.9.61".to_string()))); assert!(args.is_empty()); } @@ -1034,7 +1164,7 @@ mod tests { panic!("expected server start"); }; assert_eq!(name.as_deref(), Some("existing")); - assert_eq!(version.as_deref(), Some("25.12.9.61")); + assert_eq!(version, Some(VersionSpec::Exact("25.12.9.61".to_string()))); assert_eq!( args, ["--logger.level=trace", "--max_server_memory_usage=1000000"] diff --git a/crates/clickhousectl/src/local/mod.rs b/crates/clickhousectl/src/local/mod.rs index f67c93a3..17ccaa6d 100644 --- a/crates/clickhousectl/src/local/mod.rs +++ b/crates/clickhousectl/src/local/mod.rs @@ -7,7 +7,7 @@ pub mod postgres; pub mod server; pub mod symlink; -use cli::{LocalCommands, ServerCommands}; +use cli::{InstallVersionOperand, LocalCommands, ServerCommands}; use crate::error::{Error, Result}; use crate::{init, paths, version_manager}; @@ -17,7 +17,7 @@ use std::process::Command; pub async fn run(cmd: LocalCommands, json: bool) -> Result<()> { match cmd { - LocalCommands::Install { version, force } => install(&version, force, json).await, + LocalCommands::Install { version, force } => install(version, force, json).await, LocalCommands::List { remote } => { if remote { list_available(json).await @@ -50,14 +50,6 @@ pub async fn run(cmd: LocalCommands, json: bool) -> Result<()> { } } -/// If the version spec looks like `postgres@` or `postgres:`, extract -/// the tag. The CLI accepts both `@` (more shell-friendly, no need to quote) -/// and `:` (matches Docker image syntax). -fn parse_postgres_install_spec(spec: &str) -> Option<&str> { - spec.strip_prefix("postgres@") - .or_else(|| spec.strip_prefix("postgres:")) -} - async fn install_postgres(tag: &str, force: bool, json: bool) -> Result<()> { postgres::validate_pg_tag(tag)?; let docker = docker::connect().await?; @@ -83,11 +75,13 @@ async fn install_postgres(tag: &str, force: bool, json: bool) -> Result<()> { Ok(()) } -async fn install(version_spec: &str, force: bool, json: bool) -> Result<()> { - if let Some(tag) = parse_postgres_install_spec(version_spec) { - return install_postgres(tag, force, json).await; - } - let spec = version_manager::parse_version_spec(version_spec)?; +async fn install(version: InstallVersionOperand, force: bool, json: bool) -> Result<()> { + let spec = match version { + InstallVersionOperand::ClickHouse(spec) => spec, + InstallVersionOperand::Postgres(tag) => { + return install_postgres(&tag, force, json).await; + } + }; let platform = version_manager::platform::Platform::detect()?; let version = version_manager::install::install_local_first(&spec, &platform, force).await?; @@ -164,11 +158,14 @@ async fn list_available(json: bool) -> Result<()> { Ok(()) } -async fn use_version(version_spec: &str, no_global: bool, json: bool) -> Result<()> { - let spec = version_manager::parse_version_spec(version_spec)?; +async fn use_version( + spec: &version_manager::spec::VersionSpec, + no_global: bool, + json: bool, +) -> Result<()> { let platform = version_manager::platform::Platform::detect()?; - let version = version_manager::install::ensure_installed_local_first(&spec, &platform).await?; + let version = version_manager::install::ensure_installed_local_first(spec, &platform).await?; version_manager::set_default_version(&version)?; @@ -336,7 +333,7 @@ fn resolve_direct_client_version(version: Option<&str>) -> Result { #[allow(clippy::too_many_arguments)] async fn start_server( name: Option, - version_spec: Option, + version_spec: Option, http_port: Option, tcp_port: Option, foreground: bool, @@ -359,10 +356,9 @@ async fn start_server( return Err(Error::ServerAlreadyRunning(server_name)); } - let version = if let Some(spec_str) = &version_spec { - let spec = version_manager::parse_version_spec(spec_str)?; + let version = if let Some(spec) = &version_spec { let platform = version_manager::platform::Platform::detect()?; - version_manager::install::ensure_installed_local_first(&spec, &platform).await? + version_manager::install::ensure_installed_local_first(spec, &platform).await? } else { match version_manager::get_default_version() { Ok(v) => v, @@ -373,7 +369,7 @@ async fn start_server( // subsequent bare start too; `ensure_installed_local_first` returns the // already-installed build silently if `latest` still resolves to it, // otherwise it pulls the newer master build. - let spec = version_manager::parse_version_spec("latest")?; + let spec = version_manager::spec::VersionSpec::Latest; let platform = version_manager::platform::Platform::detect()?; // Says "using", not "installing": on repeat starts the build is // usually already installed and nothing is downloaded. The install @@ -1170,17 +1166,6 @@ mod tests { assert_eq!(output.servers[2].error, None); } - #[test] - fn parse_postgres_install_spec_recognizes_at_and_colon() { - assert_eq!(parse_postgres_install_spec("postgres@17"), Some("17")); - assert_eq!( - parse_postgres_install_spec("postgres:17-alpine"), - Some("17-alpine") - ); - assert_eq!(parse_postgres_install_spec("25.12"), None); - assert_eq!(parse_postgres_install_spec("stable"), None); - } - #[test] fn update_dotenv_postgres_prefix_isolates_clickhouse_vars() { let existing = "CLICKHOUSE_HOST=localhost\nCLICKHOUSE_PORT=9000\nDATABASE_URL=x\n"; diff --git a/crates/clickhousectl/tests/local_version_error_test.rs b/crates/clickhousectl/tests/local_version_error_test.rs index 2f30f894..18780b12 100644 --- a/crates/clickhousectl/tests/local_version_error_test.rs +++ b/crates/clickhousectl/tests/local_version_error_test.rs @@ -1,4 +1,4 @@ -//! Regression coverage for local version-spec error reporting. +//! Regression coverage for local version-spec parsing. use std::path::PathBuf; use std::process::Command; @@ -8,23 +8,36 @@ fn clickhousectl_binary() -> PathBuf { } #[test] -fn local_use_reports_an_invalid_version_without_a_lookup_wrapper() { - let tempdir = tempfile::tempdir().expect("create tempdir"); - let output = Command::new(clickhousectl_binary()) - .env("DO_NOT_TRACK", "1") - .env("HOME", tempdir.path()) - .args(["local", "use", "not.a.version"]) - .output() - .expect("run clickhousectl"); +fn invalid_local_versions_fail_as_clap_usage_errors() { + for (args, expected) in [ + ( + &["local", "use", "not.a.version"][..], + "all parts must be numeric", + ), + ( + &["local", "install", "25.12.9"][..], + "3-part version '25.12.9' is not supported", + ), + ( + &["local", "server", "start", "--version", "not.a.version"][..], + "all parts must be numeric", + ), + ] { + let tempdir = tempfile::tempdir().expect("create tempdir"); + let output = Command::new(clickhousectl_binary()) + .env("DO_NOT_TRACK", "1") + .env("HOME", tempdir.path()) + .args(args) + .output() + .expect("run clickhousectl"); - let stderr = String::from_utf8_lossy(&output.stderr); - assert_eq!(output.status.code(), Some(1), "stderr: {stderr}"); - assert!( - stderr.contains("Error: invalid version 'not.a.version': all parts must be numeric"), - "unexpected stderr: {stderr}" - ); - assert!( - !stderr.contains("No matching version found"), - "parse error was wrapped as a lookup miss: {stderr}" - ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert_eq!(output.status.code(), Some(2), "stderr: {stderr}"); + assert!(stderr.contains("error: invalid value"), "stderr: {stderr}"); + assert!(stderr.contains(expected), "stderr: {stderr}"); + assert!( + !stderr.contains("Error:"), + "version reached runtime dispatch: {stderr}" + ); + } } diff --git a/crates/clickhousectl/tests/telemetry_test.rs b/crates/clickhousectl/tests/telemetry_test.rs index 9ef241f0..18558706 100644 --- a/crates/clickhousectl/tests/telemetry_test.rs +++ b/crates/clickhousectl/tests/telemetry_test.rs @@ -160,6 +160,8 @@ async fn first_run_writes_marker_prints_notice_sends_nothing() { // Second run: the notice appears exactly once, ever. let output = sandbox.run(&["local", "list"]); assert!(!stderr_of(&output).contains("anonymous usage data")); + let payloads = sandbox.wait_for_requests(1).await; + assert_eq!(payloads[0]["command"], "local list"); } #[tokio::test] @@ -455,6 +457,51 @@ async fn failed_parse_after_positional_captures_later_flags_without_values() { assert!(!raw.contains("SECRET"), "argument value leaked: {raw}"); } +#[tokio::test] +async fn invalid_local_versions_report_invalid_value_without_leaking_operands() { + for (args, command, operand) in [ + ( + &["local", "install", "25.12.9"][..], + "local install", + "25.12.9", + ), + ( + &["local", "use", "not.a.version"][..], + "local use", + "not.a.version", + ), + ( + &["local", "server", "start", "--version", "25.12.9"][..], + "local server start", + "25.12.9", + ), + ] { + let sandbox = Sandbox::new().await; + sandbox.write_state(false); + + let output = sandbox.run(args); + assert_eq!( + output.status.code(), + Some(2), + "stderr: {}", + stderr_of(&output) + ); + assert!( + stderr_of(&output).contains("error: invalid value"), + "stderr: {}", + stderr_of(&output) + ); + + let payloads = sandbox.wait_for_requests(1).await; + let event = &payloads[0]; + assert_eq!(event["command"], command); + assert_eq!(event["exit_code"], 2); + assert_eq!(event["outcome"], "invalid_value"); + let raw = serde_json::to_string(event).unwrap(); + assert!(!raw.contains(operand), "version operand leaked: {raw}"); + } +} + #[tokio::test] async fn typo_carries_definition_derived_suggestion() { let sandbox = Sandbox::new().await;