diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 1d3b735..7f68477 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -20,7 +20,7 @@ graph TB subgraph External["External Systems"] Metabase[Metabase API] FileSystem[Config Files] - EnvVar[MBR_API_KEY] + EnvVar[MBR_API_KEY / MBR_URL] end CLI --> Services @@ -193,13 +193,15 @@ flowchart LR **Configuration File:** `~/.config/mbr-cli/config.toml` ```toml -[profiles.default] url = "https://metabase.example.com" - -[profiles.production] -url = "https://metabase.prod.example.com" ``` +**Priority Order:** +1. CLI `--api-key` argument +2. `MBR_API_KEY` environment variable +3. `MBR_URL` environment variable (for URL) +4. `config.toml` file (for URL) + ## Query Execution Flow ```mermaid diff --git a/README.md b/README.md index 92c9730..27461bd 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ Rust-based CLI/TUI tool for Metabase API interaction. ## Features - **API Key Authentication**: Simple authentication via `MBR_API_KEY` environment variable -- **Multi-Profile Configuration**: TOML-based configuration with profile support +- **Simple Configuration**: TOML-based configuration with URL setting - **CLI Interface**: Command-line tool for scripting and quick operations - **Rich TUI Experience**: Interactive terminal UI with keyboard navigation (`mbr-tui`) - **Hierarchical Error Handling**: Comprehensive error system with troubleshooting hints @@ -89,7 +89,6 @@ mbr-cli query 123 --param date=2024-01-01 # Execute with parameters ### Global Options - `--verbose, -v` - Enable verbose output -- `--profile, -p ` - Use specific profile (defaults to 'default') - `--config-dir ` - Override default config directory - `--api-key ` - Set API key (also via `MBR_API_KEY` environment variable) @@ -108,6 +107,7 @@ mbr-cli query 123 --param date=2024-01-01 # Execute with parameters | Variable | Description | Required | |----------|-------------|----------| | `MBR_API_KEY` | Metabase API key for authentication | Yes | +| `MBR_URL` | Metabase server URL (alternative to config file) | No | ## Development diff --git a/crates/mbr-cli/src/cli/command_handlers.rs b/crates/mbr-cli/src/cli/command_handlers.rs index e23a3f3..f7c212f 100644 --- a/crates/mbr-cli/src/cli/command_handlers.rs +++ b/crates/mbr-cli/src/cli/command_handlers.rs @@ -32,13 +32,16 @@ impl ConfigHandler { "Attempting config show command using ConfigService", ); - let profiles = config_service.list_profiles(); - // Show the current configuration println!("Current Configuration:"); println!("====================="); - println!("Default Profile: {}", config_service.get_default_profile()); + // Show URL status + if let Some(url) = config_service.get_url() { + println!("URL: {}", url); + } else { + println!("URL: ❌ Not configured"); + } // Show API key status if has_api_key() { @@ -47,58 +50,28 @@ impl ConfigHandler { println!("API Key: ❌ Not set"); } - println!("\nProfiles:"); - if profiles.is_empty() { - println!(" No profiles configured"); - } else { - for (name, profile) in profiles { - println!(" [{}]", name); - println!(" URL: {}", profile.url); - if let Some(email) = &profile.email { - println!(" Email: {}", email); - } - } - } - Ok(()) } - ConfigCommands::Set { - profile, - url, - email, - } => { + ConfigCommands::Set { url } => { print_verbose( verbose, - &format!( - "Attempting config set using ConfigService - profile: {}, url: {:?}, email: {:?}", - profile, url, email - ), + &format!("Attempting config set using ConfigService - url: {:?}", url), ); - let mut updated_fields = Vec::new(); - // Handle URL setting if let Some(url_value) = url { mbr_core::utils::validation::validate_url(&url_value)?; - config_service.set_profile_field(&profile, "url", &url_value)?; - updated_fields.push(format!("URL to: {}", url_value)); - } - - // Handle email setting - if let Some(email_value) = email { - config_service.set_profile_field(&profile, "email", &email_value)?; - updated_fields.push(format!("email to: {}", email_value)); - } - - if updated_fields.is_empty() { + config_service.set_url(url_value.clone()); + config_service.save_config(None)?; + println!("✅ URL set to: {}", url_value); + println!("Configuration saved successfully."); + } else { return Err(AppError::Cli(CliError::InvalidArguments( - "No configuration values provided. Use --url and/or --email, or set MBR_URL/MBR_USERNAME environment variables".to_string(), + "No URL provided. Use --url or set MBR_URL environment variable" + .to_string(), ))); } - println!("✅ Set profile '{}' {}", profile, updated_fields.join(", ")); - config_service.save_config(None)?; - println!("Configuration saved successfully."); Ok(()) } ConfigCommands::Validate => { diff --git a/crates/mbr-cli/src/cli/dispatcher.rs b/crates/mbr-cli/src/cli/dispatcher.rs index a61f886..3319277 100644 --- a/crates/mbr-cli/src/cli/dispatcher.rs +++ b/crates/mbr-cli/src/cli/dispatcher.rs @@ -5,13 +5,12 @@ use crate::cli::main_types::Commands; use mbr_core::api::client::MetabaseClient; use mbr_core::core::services::config_service::ConfigService; use mbr_core::error::{AppError, CliError}; -use mbr_core::storage::config::{Config, Profile}; +use mbr_core::storage::config::Config; use mbr_core::storage::credentials::get_api_key; use mbr_core::utils::logging::print_verbose; pub struct Dispatcher { config: Config, - profile_name: String, verbose: bool, api_key: Option, } @@ -21,37 +20,9 @@ impl Dispatcher { print_verbose(self.verbose, msg); } - pub fn new( - mut config: Config, - profile_name: String, - verbose: bool, - api_key: Option, - config_path: Option, - ) -> Self { - // Create default profile if not exists - if config.get_profile(&profile_name).is_none() { - print_verbose( - verbose, - &format!("Creating default profile: {}", profile_name), - ); - - let default_profile = Profile { - url: "http://localhost:3000".to_string(), - email: None, - }; - - config.set_profile(profile_name.clone(), default_profile); - - if let Err(err) = config.save(config_path) { - print_verbose(verbose, &format!("Warning: Failed to save config: {}", err)); - } else { - print_verbose(verbose, "Successfully saved config file"); - } - } - + pub fn new(config: Config, verbose: bool, api_key: Option) -> Self { Self { config, - profile_name, verbose, api_key, } @@ -69,26 +40,24 @@ impl Dispatcher { get_api_key() } - // Helper method to get profile for the current credentials - fn get_current_profile(&self) -> Result<&Profile, AppError> { - self.config.get_profile(&self.profile_name).ok_or_else(|| { - AppError::Cli(CliError::AuthRequired { - message: format!("Profile '{}' not found", self.profile_name), - hint: "Use 'mbr-cli config set --profile --url ' to create a profile" - .to_string(), - available_profiles: self.config.profiles.keys().cloned().collect(), - }) + // Get URL from config or environment + fn get_url(&self) -> Result { + self.config.get_url().ok_or_else(|| { + AppError::Cli(CliError::InvalidArguments( + "Metabase URL is not configured. Use 'mbr-cli config set --url ' or set MBR_URL environment variable".to_string(), + )) }) } // Helper method to create MetabaseClient with API key - fn create_client(&self, profile: &Profile) -> Result { + fn create_client(&self) -> Result { + let url = self.get_url()?; if let Some(api_key) = self.get_effective_api_key() { self.log_verbose("Creating client with API key"); - Ok(MetabaseClient::with_api_key(profile.url.clone(), api_key)?) + Ok(MetabaseClient::with_api_key(url, api_key)?) } else { self.log_verbose("Creating client without API key"); - Ok(MetabaseClient::new(profile.url.clone())?) + Ok(MetabaseClient::new(url)?) } } @@ -102,28 +71,31 @@ impl Dispatcher { Commands::Config { command } => { let handler = ConfigHandler::new(); let mut config_service = self.create_config_service(); - let profile = self.get_current_profile()?; - let client = self.create_client(profile)?; + // For config commands, we may not have a URL yet + let client = match self.create_client() { + Ok(c) => c, + Err(_) => { + // Create a dummy client for config show/set (URL not needed) + MetabaseClient::new("http://localhost:3000".to_string())? + } + }; handler .handle(command, &mut config_service, client, self.verbose) .await } Commands::Query(args) => { let handler = QueryHandler::new(); - let profile = self.get_current_profile()?; - let client = self.create_client(profile)?; + let client = self.create_client()?; handler.handle(args, client, self.verbose).await } Commands::Collection { command } => { let handler = CollectionHandler::new(); - let profile = self.get_current_profile()?; - let client = self.create_client(profile)?; + let client = self.create_client()?; handler.handle(command, client, self.verbose).await } Commands::Database { command } => { let handler = DatabaseHandler::new(); - let profile = self.get_current_profile()?; - let client = self.create_client(profile)?; + let client = self.create_client()?; handler.handle(command, client, self.verbose).await } } diff --git a/crates/mbr-cli/src/cli/main_types.rs b/crates/mbr-cli/src/cli/main_types.rs index 0e5e1e0..5d8655e 100644 --- a/crates/mbr-cli/src/cli/main_types.rs +++ b/crates/mbr-cli/src/cli/main_types.rs @@ -22,10 +22,6 @@ pub struct Cli { #[arg(short, long, global = true)] pub verbose: bool, - /// Profile name to use (default: 'default') - #[arg(short, long, global = true)] - pub profile: Option, - /// Custom configuration directory path #[arg(long, global = true)] pub config_dir: Option, @@ -63,21 +59,14 @@ pub enum Commands { pub enum ConfigCommands { /// Show the current configuration Show, - /// Set configuration values for a profile + /// Set configuration values #[command(after_help = "Examples: mbr-cli config set --url http://localhost:3000 - mbr-cli config set --profile prod --url https://metabase.example.com - mbr-cli config set --email user@example.com")] + mbr-cli config set --url https://metabase.example.com")] Set { - /// Profile name to configure - #[arg(long, default_value = "default")] - profile: String, /// Metabase server URL #[arg(long, env = "MBR_URL")] url: Option, - /// Email address for this profile - #[arg(long, env = "MBR_USERNAME")] - email: Option, }, /// Validate API key and test connection to Metabase server #[command(after_help = "Examples: diff --git a/crates/mbr-cli/src/main.rs b/crates/mbr-cli/src/main.rs index 1424df3..b978d95 100644 --- a/crates/mbr-cli/src/main.rs +++ b/crates/mbr-cli/src/main.rs @@ -25,12 +25,8 @@ async fn main() -> Result<(), Box> { } }; - // Determine the profile to use (default to "default" if not specified) - let profile_name = cli.profile.clone().unwrap_or_else(|| "default".to_string()); - if cli.verbose { println!("Verbose mode is enabled"); - println!("Using profile: {}", profile_name); if let Some(config_dir) = &cli.config_dir { println!("Using config directory: {}", config_dir); @@ -42,7 +38,7 @@ async fn main() -> Result<(), Box> { } // Create dispatcher - let dispatcher = Dispatcher::new(config, profile_name, cli.verbose, cli.api_key, config_path); + let dispatcher = Dispatcher::new(config, cli.verbose, cli.api_key); // Execute the command if let Err(e) = dispatcher.dispatch(cli.command).await { diff --git a/crates/mbr-core/src/core/services/config_service.rs b/crates/mbr-core/src/core/services/config_service.rs index 766bcb3..764948d 100644 --- a/crates/mbr-core/src/core/services/config_service.rs +++ b/crates/mbr-core/src/core/services/config_service.rs @@ -1,3 +1,5 @@ +//! Configuration service for managing application configuration + use crate::AppError; use crate::storage::config::Config; use std::path::PathBuf; @@ -13,48 +15,14 @@ impl ConfigService { Self { config } } - /// Get profile by name - pub fn get_profile(&self, name: &str) -> Option<&crate::storage::config::Profile> { - self.config.profiles.get(name) - } - - /// Get default profile name (always returns "default") - pub fn get_default_profile(&self) -> &str { - "default" + /// Get configured URL + pub fn get_url(&self) -> Option { + self.config.get_url() } - /// Set profile field value - pub fn set_profile_field( - &mut self, - profile: &str, - field: &str, - value: &str, - ) -> Result<(), AppError> { - use crate::error::CliError; - - // Get or create profile - let profile_entry = self - .config - .profiles - .entry(profile.to_string()) - .or_insert_with(|| crate::storage::config::Profile { - url: "".to_string(), - email: None, - }); - - // Set field based on field name - only accept user-facing field names - match field { - "url" => profile_entry.url = value.to_string(), - "email" => profile_entry.email = Some(value.to_string()), - _ => { - return Err(AppError::Cli(CliError::InvalidArguments(format!( - "Unknown field: {}. Use 'url' or 'email'", - field - )))); - } - } - - Ok(()) + /// Set URL + pub fn set_url(&mut self, url: String) { + self.config.set_url(url); } /// Save configuration to file @@ -62,9 +30,9 @@ impl ConfigService { self.config.save(path).map_err(|e| e.into()) } - /// List all profiles - pub fn list_profiles(&self) -> Vec<(&String, &crate::storage::config::Profile)> { - self.config.profiles.iter().collect() + /// Check if URL is configured + pub fn has_url(&self) -> bool { + self.get_url().is_some() } } @@ -74,47 +42,57 @@ mod tests { #[test] fn test_config_service_new() { - let config = Config::default(); - let service = ConfigService::new(config); - - // Verify ConfigService is created successfully - // Test that service is created successfully - assert!(service.config.profiles.is_empty()); - } + // Temporarily clear MBR_URL to ensure test isolation + let original = std::env::var("MBR_URL").ok(); + unsafe { + std::env::remove_var("MBR_URL"); + } - #[test] - fn test_get_profile_returns_option() { let config = Config::default(); let service = ConfigService::new(config); - // Verify get_profile returns Option - let result = service.get_profile("default"); - assert!(result.is_some() || result.is_none()); + // Verify ConfigService is created successfully without URL + assert!(!service.has_url()); + + // Restore original state + unsafe { + if let Some(value) = original { + std::env::set_var("MBR_URL", value); + } + } } #[test] - fn test_list_profiles_returns_vec() { + fn test_get_url_returns_option() { + // Temporarily clear MBR_URL to ensure test isolation + let original = std::env::var("MBR_URL").ok(); + unsafe { + std::env::remove_var("MBR_URL"); + } + let config = Config::default(); let service = ConfigService::new(config); - // Verify list_profiles returns Vec - let profiles = service.list_profiles(); - assert!(profiles.is_empty() || !profiles.is_empty()); + // No URL by default + assert!(service.get_url().is_none()); + + // Restore original state + unsafe { + if let Some(value) = original { + std::env::set_var("MBR_URL", value); + } + } } #[test] - fn test_set_profile_field_returns_result() { + fn test_set_url() { let config = Config::default(); let mut service = ConfigService::new(config); - // Verify set_profile_field returns Result with user-facing field name - let result = service.set_profile_field("default", "url", "http://localhost:3000"); - assert!(result.is_ok()); + service.set_url("http://localhost:3000".to_string()); - // Verify the field was set - let profile = service.get_profile("default"); - assert!(profile.is_some()); - assert_eq!(profile.unwrap().url, "http://localhost:3000"); + assert!(service.has_url()); + assert_eq!(service.get_url(), Some("http://localhost:3000".to_string())); } #[test] @@ -126,26 +104,4 @@ mod tests { let result = service.save_config(None); assert!(result.is_ok() || result.is_err()); } - - #[test] - fn test_set_profile_field_rejects_internal_field_names() { - let config = Config::default(); - let mut service = ConfigService::new(config); - - // Should reject internal field names - let result = service.set_profile_field("test", "host", "http://example.com"); - assert!(result.is_err()); - assert!(format!("{:?}", result).contains("Unknown field: host")); - - let result = service.set_profile_field("test", "invalid_field", "http://example.com"); - assert!(result.is_err()); - assert!(format!("{:?}", result).contains("Unknown field: invalid_field")); - - // Should accept user-facing field names - let result = service.set_profile_field("test", "url", "http://example.com"); - assert!(result.is_ok()); - - let result = service.set_profile_field("test", "email", "test@example.com"); - assert!(result.is_ok()); - } } diff --git a/crates/mbr-core/src/lib.rs b/crates/mbr-core/src/lib.rs index 7aea72c..432aed4 100644 --- a/crates/mbr-core/src/lib.rs +++ b/crates/mbr-core/src/lib.rs @@ -78,7 +78,7 @@ pub mod prelude { pub use crate::core::services::question_service::QuestionService; // Storage - pub use crate::storage::config::{Config, Profile}; + pub use crate::storage::config::Config; pub use crate::storage::credentials::{get_api_key, has_api_key}; // Display utilities @@ -102,7 +102,7 @@ pub mod storage; /// Utilities layer - shared helpers and common functionality. /// /// Provides reusable utilities: -/// - [`utils::validation`]: URL, email, API key validation +/// - [`utils::validation`]: URL and API key validation /// - [`utils::text`]: Text formatting and truncation /// - [`utils::data`]: Data manipulation helpers pub mod utils; diff --git a/crates/mbr-core/src/storage/config.rs b/crates/mbr-core/src/storage/config.rs index 7694a6c..fe619b7 100644 --- a/crates/mbr-core/src/storage/config.rs +++ b/crates/mbr-core/src/storage/config.rs @@ -1,24 +1,24 @@ +//! Configuration management +//! +//! Simple configuration with URL stored in config file or environment variable. +//! Priority: CLI argument > MBR_URL environment variable > config.toml + use super::Result; use crate::error::StorageError; use dirs; use serde::{Deserialize, Serialize}; -use std::collections::HashMap; use std::fs; use std::path::PathBuf; +/// Application configuration #[derive(Serialize, Deserialize, Debug, Clone, Default)] pub struct Config { - #[serde(flatten)] - pub profiles: HashMap, -} - -#[derive(Serialize, Deserialize, Debug, Clone)] -pub struct Profile { - pub url: String, - pub email: Option, // Optional email for login convenience + /// Metabase server URL + pub url: Option, } impl Config { + /// Load configuration from file pub fn load(path: Option) -> Result { let config_path = match path { Some(p) => p, @@ -42,6 +42,7 @@ impl Config { Ok(config) } + /// Save configuration to file pub fn save(&self, path: Option) -> Result<()> { let config_path = match path { Some(p) => p, @@ -76,12 +77,16 @@ impl Config { Ok(config_file) } - pub fn get_profile(&self, name: &str) -> Option<&Profile> { - self.profiles.get(name) + /// Get URL with fallback to environment variable + pub fn get_url(&self) -> Option { + self.url + .clone() + .or_else(|| std::env::var("MBR_URL").ok().filter(|s| !s.is_empty())) } - pub fn set_profile(&mut self, name: String, profile: Profile) { - self.profiles.insert(name, profile); + /// Set URL + pub fn set_url(&mut self, url: String) { + self.url = Some(url); } } @@ -93,28 +98,16 @@ mod tests { #[test] fn test_config_default() { let config = Config::default(); - assert_eq!(config.profiles.len(), 0); + assert!(config.url.is_none()); } #[test] - fn test_profile_management() { + fn test_url_management() { let mut config = Config::default(); - // Create a profile - let profile = Profile { - url: "http://example.test".to_string(), - email: Some("test@example.com".to_string()), - }; - // Set the profile in the config - config.set_profile("test".to_string(), profile.clone()); - // Get the profile - let retrieved = config.get_profile("test"); - assert!(retrieved.is_some()); - if let Some(retrieved) = retrieved { - assert_eq!(retrieved.url, profile.url); - assert_eq!(retrieved.email, profile.email); - } - // Nonexistent profile should return None - assert!(config.get_profile("nonexistent").is_none()); + assert!(config.url.is_none()); + + config.set_url("http://example.test".to_string()); + assert_eq!(config.url, Some("http://example.test".to_string())); } #[test] @@ -124,25 +117,18 @@ mod tests { // Create a sample config let mut config = Config::default(); - config.profiles.insert( - "test".to_string(), - Profile { - url: "http://example.test".to_string(), - email: Some("test@example.com".to_string()), - }, - ); - - // Save the config to the temp directory + config.set_url("http://example.test".to_string()); + + // Save the config config .save(Some(config_path.clone())) .expect("Failed to save config"); - // Load the config from the temp directory + // Load the config let loaded_config = Config::load(Some(config_path)).expect("Failed to load config"); // Check if loaded config matches saved config - assert_eq!(loaded_config.profiles.len(), 1); - assert!(loaded_config.get_profile("test").is_some()); + assert_eq!(loaded_config.url, Some("http://example.test".to_string())); } #[test] @@ -155,6 +141,6 @@ mod tests { assert!(config.is_ok()); let config = config.expect("Failed to load default config"); - assert_eq!(config.profiles.len(), 0); + assert!(config.url.is_none()); } } diff --git a/crates/mbr-core/src/utils/validation.rs b/crates/mbr-core/src/utils/validation.rs index 601a541..2edae1c 100644 --- a/crates/mbr-core/src/utils/validation.rs +++ b/crates/mbr-core/src/utils/validation.rs @@ -75,43 +75,6 @@ pub fn validate_api_key(api_key: &str) -> crate::Result<()> { Ok(()) } -/// Validate email format -pub fn validate_email(email: &str) -> crate::Result<()> { - if email.is_empty() { - return Err(CliError::InvalidArguments("Email cannot be empty".to_string()).into()); - } - - // Basic email validation - must contain @ symbol - if !email.contains('@') { - return Err(CliError::InvalidArguments(format!( - "Invalid email '{}': Email must contain @ symbol", - email - )) - .into()); - } - - // Check for domain part (after @) and username part (before @) - let parts: Vec<&str> = email.split('@').collect(); - if parts.len() != 2 || parts[0].is_empty() || parts[1].is_empty() { - return Err(CliError::InvalidArguments(format!( - "Invalid email '{}': Email must have username and domain parts", - email - )) - .into()); - } - - // Basic domain validation - must contain at least one dot - if !parts[1].contains('.') { - return Err(CliError::InvalidArguments(format!( - "Invalid email '{}': Domain must contain dot", - email - )) - .into()); - } - - Ok(()) -} - #[cfg(test)] mod tests { use super::*; @@ -141,23 +104,6 @@ mod tests { assert!(validate_api_key("short").is_err()); } - #[test] - fn test_validate_email_accepts_valid_emails() { - assert!(validate_email("user@example.com").is_ok()); - assert!(validate_email("test.email@domain.org").is_ok()); - assert!(validate_email("admin@metabase.local").is_ok()); - } - - #[test] - fn test_validate_email_rejects_invalid_emails() { - assert!(validate_email("").is_err()); - assert!(validate_email("invalid").is_err()); - assert!(validate_email("@domain.com").is_err()); - assert!(validate_email("user@").is_err()); - assert!(validate_email("user@domain").is_err()); - assert!(validate_email("user@domain@com").is_err()); - } - // === EnvConfigReader Tests === #[test] diff --git a/crates/mbr-tui/src/service.rs b/crates/mbr-tui/src/service.rs index c424654..75012b0 100644 --- a/crates/mbr-tui/src/service.rs +++ b/crates/mbr-tui/src/service.rs @@ -235,11 +235,10 @@ pub fn init_service() -> Result, String> { // Try to load config for base URL let config = Config::load(None).ok(); - // Get base URL from config (default profile) or use default + // Get base URL from config or use default let base_url = config .as_ref() - .and_then(|c| c.get_profile("default")) - .map(|p| p.url.clone()) + .and_then(|c| c.get_url()) .unwrap_or_else(|| "http://localhost:3000".to_string()); ServiceClient::new(base_url, api_key).map(Arc::new)