From a3327122755c00ae9384320e519a1257b91d424f Mon Sep 17 00:00:00 2001 From: Darren Schroeder <343840+fdncred@users.noreply.github.com> Date: Tue, 3 Dec 2024 20:47:58 -0600 Subject: [PATCH] add function to make env vars case-insensitive (#14390) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description This PR adds a new function that allows one to get an env var case-insensitively. I did this so we can hopefully stop having problems when Windows has HKLM as path and HKCU as Path. Instead of just changing every function that used the original one, I chose the ones that I thought were specific to getting the path. I didn't want to go all in and make every env get case insensitive, but maybe we should? 🤷🏻‍♂️ closes #12676 # User-Facing Changes # Tests + Formatting # After Submitting --- .../src/completions/command_completions.rs | 3 +- crates/nu-cli/tests/completions/mod.rs | 10 ----- crates/nu-engine/src/env.rs | 45 ++++++------------- crates/nu-plugin-engine/src/context.rs | 2 +- crates/nu-protocol/src/engine/engine_state.rs | 15 ++----- crates/nu-protocol/src/engine/stack.rs | 35 +++++++++++++++ crates/nu-protocol/tests/test_value.rs | 29 +++++++++++- 7 files changed, 83 insertions(+), 56 deletions(-) diff --git a/crates/nu-cli/src/completions/command_completions.rs b/crates/nu-cli/src/completions/command_completions.rs index b12df4735a..ce4383483a 100644 --- a/crates/nu-cli/src/completions/command_completions.rs +++ b/crates/nu-cli/src/completions/command_completions.rs @@ -41,8 +41,7 @@ impl CommandCompletion { ) -> HashMap { let mut suggs = HashMap::new(); - // os agnostic way to get the PATH env var - let paths = working_set.permanent_state.get_path_env_var(); + let paths = working_set.permanent_state.get_env_var_insensitive("path"); if let Some(paths) = paths { if let Ok(paths) = paths.as_list() { diff --git a/crates/nu-cli/tests/completions/mod.rs b/crates/nu-cli/tests/completions/mod.rs index 78c36cad7e..e32ac8c4f2 100644 --- a/crates/nu-cli/tests/completions/mod.rs +++ b/crates/nu-cli/tests/completions/mod.rs @@ -1759,13 +1759,3 @@ fn alias_offset_bug_7754() { // This crashes before PR #7756 let _suggestions = completer.complete("ll -a | c", 9); } - -#[test] -fn get_path_env_var_8003() { - // Create a new engine - let (_, _, engine, _) = new_engine(); - // Get the path env var in a platform agnostic way - let the_path = engine.get_path_env_var(); - // Make sure it's not empty - assert!(the_path.is_some()); -} diff --git a/crates/nu-engine/src/env.rs b/crates/nu-engine/src/env.rs index f7aa3cf2e1..dc6c354fce 100644 --- a/crates/nu-engine/src/env.rs +++ b/crates/nu-engine/src/env.rs @@ -11,13 +11,6 @@ use std::{ sync::Arc, }; -#[cfg(windows)] -const ENV_PATH_NAME: &str = "Path"; -#[cfg(windows)] -const ENV_PATH_NAME_SECONDARY: &str = "PATH"; -#[cfg(not(windows))] -const ENV_PATH_NAME: &str = "PATH"; - const ENV_CONVERSIONS: &str = "ENV_CONVERSIONS"; enum ConversionResult { @@ -53,14 +46,14 @@ pub fn convert_env_values(engine_state: &mut EngineState, stack: &Stack) -> Resu #[cfg(not(windows))] { - error = error.or_else(|| ensure_path(&mut new_scope, ENV_PATH_NAME)); + error = error.or_else(|| ensure_path(&mut new_scope, "PATH")); } #[cfg(windows)] { - let first_result = ensure_path(&mut new_scope, ENV_PATH_NAME); + let first_result = ensure_path(&mut new_scope, "Path"); if first_result.is_some() { - let second_result = ensure_path(&mut new_scope, ENV_PATH_NAME_SECONDARY); + let second_result = ensure_path(&mut new_scope, "PATH"); if second_result.is_some() { error = error.or(first_result); @@ -107,7 +100,7 @@ pub fn env_to_string( ConversionResult::CellPathError => match value.coerce_string() { Ok(s) => Ok(s), Err(_) => { - if env_name == ENV_PATH_NAME { + if env_name.to_lowercase() == "path" { // Try to convert PATH/Path list to a string match value { Value::List { vals, .. } => { @@ -216,31 +209,21 @@ pub fn current_dir_const(working_set: &StateWorkingSet) -> Result Result { - let (pathname, pathval) = match stack.get_env_var(engine_state, ENV_PATH_NAME) { - Some(v) => Ok((ENV_PATH_NAME, v)), - None => { - #[cfg(windows)] - match stack.get_env_var(engine_state, ENV_PATH_NAME_SECONDARY) { - Some(v) => Ok((ENV_PATH_NAME_SECONDARY, v)), - None => Err(ShellError::EnvVarNotFoundAtRuntime { - envvar_name: ENV_PATH_NAME_SECONDARY.to_string(), - span, - }), - } - #[cfg(not(windows))] - Err(ShellError::EnvVarNotFoundAtRuntime { - envvar_name: ENV_PATH_NAME.to_string(), - span, - }) - } + let (pathname, pathval) = match stack.get_env_var_insensitive(engine_state, "path") { + Some(v) => Ok((if cfg!(windows) { "Path" } else { "PATH" }, v)), + None => Err(ShellError::EnvVarNotFoundAtRuntime { + envvar_name: if cfg!(windows) { + "Path".to_string() + } else { + "PATH".to_string() + }, + span, + }), }?; env_to_string(pathname, pathval, engine_state, stack) diff --git a/crates/nu-plugin-engine/src/context.rs b/crates/nu-plugin-engine/src/context.rs index 4c262fd3ed..5652cf1a5f 100644 --- a/crates/nu-plugin-engine/src/context.rs +++ b/crates/nu-plugin-engine/src/context.rs @@ -126,7 +126,7 @@ impl<'a> PluginExecutionContext for PluginExecutionCommandContext<'a> { } fn get_env_var(&self, name: &str) -> Result, ShellError> { - Ok(self.stack.get_env_var(&self.engine_state, name)) + Ok(self.stack.get_env_var_insensitive(&self.engine_state, name)) } fn get_env_vars(&self) -> Result, ShellError> { diff --git a/crates/nu-protocol/src/engine/engine_state.rs b/crates/nu-protocol/src/engine/engine_state.rs index 626033f915..716cb03ca9 100644 --- a/crates/nu-protocol/src/engine/engine_state.rs +++ b/crates/nu-protocol/src/engine/engine_state.rs @@ -14,6 +14,7 @@ use crate::{ use fancy_regex::Regex; use lru::LruCache; use nu_path::AbsolutePathBuf; +use nu_utils::IgnoreCaseExt; use std::{ collections::HashMap, num::NonZeroUsize, @@ -465,20 +466,12 @@ impl EngineState { None } - // Get the path environment variable in a platform agnostic way - pub fn get_path_env_var(&self) -> Option<&Value> { - let env_path_name_windows: &str = "Path"; - let env_path_name_nix: &str = "PATH"; - + pub fn get_env_var_insensitive(&self, name: &str) -> Option<&Value> { for overlay_id in self.scope.active_overlays.iter().rev() { let overlay_name = String::from_utf8_lossy(self.get_overlay_name(*overlay_id)); if let Some(env_vars) = self.env_vars.get(overlay_name.as_ref()) { - if let Some(val) = env_vars.get(env_path_name_nix) { - return Some(val); - } else if let Some(val) = env_vars.get(env_path_name_windows) { - return Some(val); - } else { - return None; + if let Some(v) = env_vars.iter().find(|(k, _)| k.eq_ignore_case(name)) { + return Some(v.1); } } } diff --git a/crates/nu-protocol/src/engine/stack.rs b/crates/nu-protocol/src/engine/stack.rs index ba14e67817..66f59b78f6 100644 --- a/crates/nu-protocol/src/engine/stack.rs +++ b/crates/nu-protocol/src/engine/stack.rs @@ -5,6 +5,7 @@ use crate::{ }, Config, IntoValue, OutDest, ShellError, Span, Value, VarId, ENV_VARIABLE_ID, NU_VARIABLE_ID, }; +use nu_utils::IgnoreCaseExt; use std::{ collections::{HashMap, HashSet}, fs::File, @@ -494,6 +495,40 @@ impl Stack { None } + // Case-Insensitive version of get_env_var + pub fn get_env_var_insensitive<'a>( + &'a self, + engine_state: &'a EngineState, + name: &str, + ) -> Option<&'a Value> { + for scope in self.env_vars.iter().rev() { + for active_overlay in self.active_overlays.iter().rev() { + if let Some(env_vars) = scope.get(active_overlay) { + if let Some(v) = env_vars.iter().find(|(k, _)| k.eq_ignore_case(name)) { + return Some(v.1); + } + } + } + } + + for active_overlay in self.active_overlays.iter().rev() { + let is_hidden = if let Some(env_hidden) = self.env_hidden.get(active_overlay) { + env_hidden.iter().any(|k| k.eq_ignore_case(name)) + } else { + false + }; + + if !is_hidden { + if let Some(env_vars) = engine_state.env_vars.get(active_overlay) { + if let Some(v) = env_vars.iter().find(|(k, _)| k.eq_ignore_case(name)) { + return Some(v.1); + } + } + } + } + None + } + pub fn has_env_var(&self, engine_state: &EngineState, name: &str) -> bool { for scope in self.env_vars.iter().rev() { for active_overlay in self.active_overlays.iter().rev() { diff --git a/crates/nu-protocol/tests/test_value.rs b/crates/nu-protocol/tests/test_value.rs index 376bd81f1f..fb26bea0e2 100644 --- a/crates/nu-protocol/tests/test_value.rs +++ b/crates/nu-protocol/tests/test_value.rs @@ -1,4 +1,7 @@ -use nu_protocol::{Config, Span, Value}; +use nu_protocol::{ + engine::{EngineState, Stack}, + Config, Span, Value, +}; use rstest::rstest; #[test] @@ -46,3 +49,27 @@ fn test_duration_to_string(#[case] in_ns: i64, #[case] expected: &str) { "expected != observed" ); } + +#[test] +fn test_case_insensitive_env_var() { + let mut engine_state = EngineState::new(); + let stack = Stack::new(); + + for (name, value) in std::env::vars() { + engine_state.add_env_var(name, Value::test_string(value)); + } + + let path_lower = engine_state.get_env_var_insensitive("path"); + let path_upper = engine_state.get_env_var_insensitive("PATH"); + let path_mixed = engine_state.get_env_var_insensitive("PaTh"); + + assert_eq!(path_lower, path_upper); + assert_eq!(path_lower, path_mixed); + + let stack_path_lower = stack.get_env_var_insensitive(&engine_state, "path"); + let stack_path_upper = stack.get_env_var_insensitive(&engine_state, "PATH"); + let stack_path_mixed = stack.get_env_var_insensitive(&engine_state, "PaTh"); + + assert_eq!(stack_path_lower, stack_path_upper); + assert_eq!(stack_path_lower, stack_path_mixed); +}