From a3c9d5ba2bcc7bac978e5ba0d8bcd91cde79f4ef Mon Sep 17 00:00:00 2001 From: M Mahrous Date: Wed, 15 Oct 2025 14:07:41 +0200 Subject: [PATCH] [IMP] allow vars in diagnostic filters and log parsing errors --- server/src/core/config.rs | 64 ++++++++++++++++++++------- server/src/core/file_mgr.rs | 4 +- server/src/features/features_utils.rs | 2 +- server/src/server.rs | 3 +- server/src/utils.rs | 34 +++++++------- server/tests/config_tests.rs | 39 +++++++++++++++- 6 files changed, 110 insertions(+), 36 deletions(-) diff --git a/server/src/core/config.rs b/server/src/core/config.rs index 2d674804..53e157a0 100644 --- a/server/src/core/config.rs +++ b/server/src/core/config.rs @@ -11,6 +11,7 @@ use ruff_python_ast::{Expr, Mod}; use ruff_python_parser::{Mode, ParseOptions}; use serde::{Deserialize, Deserializer, Serialize, Serializer}; use schemars::{JsonSchema, Schema, SchemaGenerator}; +use tracing::error; use crate::constants::{CONFIG_WIKI_URL}; use crate::core::diagnostics::{DiagnosticCode, DiagnosticSetting, SchemaDiagnosticCodes}; @@ -94,13 +95,13 @@ impl Default for MergeMethod { #[derive(Debug, Deserialize, Serialize, Clone, Copy, JsonSchema)] #[serde(rename_all = "lowercase")] pub enum DiagnosticFilterPathType { - IN, - NOT_IN + In, + NotIn } impl Default for DiagnosticFilterPathType { fn default() -> Self { - DiagnosticFilterPathType::IN + DiagnosticFilterPathType::In } } @@ -551,7 +552,7 @@ fn process_version(var: Sourced, ws_folders: &HashMap, w }; let config_dir = config_path.parent().map(PathBuf::from).unwrap_or_else(|| PathBuf::from(".")); match fill_validate_path(ws_folders, workspace_name, var.value(), |p| PathBuf::from(p).exists(), HashMap::new(), &config_dir) { - Some(filled_path) => { + Ok(filled_path) => { let var_pb = PathBuf::from(&filled_path); if var_pb.is_file() { // If it is a file, we can return the file name @@ -580,8 +581,11 @@ fn process_version(var: Sourced, ws_folders: &HashMap, w }; Sourced { value: S!(suffix.as_os_str().to_string_lossy()), sources: var.sources.clone(), ..Default::default() } }, - // Not a valid path, just return the variable as is - None => var, + // Not a valid path, just return the variable as is, log info + Err(err) => { + error!("Failed to process $version path for variable {:?}: {}", var, err); + var + }, } } @@ -801,7 +805,7 @@ pub fn default_profile_name() -> String { "default".to_string() } -fn fill_or_canonicalize(sourced_path: &Sourced, ws_folders: &HashMap, workspace_name: Option<&String>, predicate: &F, var_map: HashMap) -> Option> +fn fill_or_canonicalize(sourced_path: &Sourced, ws_folders: &HashMap, workspace_name: Option<&String>, predicate: &F, var_map: HashMap) -> Result, String> where F: Fn(&String) -> bool, { @@ -811,7 +815,7 @@ F: Fn(&String) -> bool, let config_dir = config_path.parent().map(PathBuf::from).unwrap_or_else(|| PathBuf::from(".")); if has_template(&sourced_path.value) { return fill_validate_path(ws_folders, workspace_name, &sourced_path.value, predicate, var_map, &config_dir) - .and_then(|p| std::fs::canonicalize(PathBuf::from(p)).ok()) + .and_then(|p| std::fs::canonicalize(PathBuf::from(p)).map_err(|e| e.to_string())) .map(|p| p.sanitize()) .map(|path| Sourced { value: path, sources: sourced_path.sources.clone(), ..Default::default()}); } @@ -819,11 +823,13 @@ F: Fn(&String) -> bool, if path.is_relative() { path = config_dir.join(sourced_path.value.clone()); } - std::fs::canonicalize(path) - .map(|p| p.sanitize()) - .ok() - .filter(|p| predicate(&p)) - .map(|path| Sourced { value: path, sources: sourced_path.sources.clone(), ..Default::default()}) + let path = std::fs::canonicalize(path) + .map_err(|e| e.to_string())? + .sanitize(); + if !predicate(&path) { + return Err(format!("Path '{}' does not satisfy the required conditions", path)); + } + Ok(Sourced { value: path, sources: sourced_path.sources.clone(), ..Default::default() }) } fn process_paths( @@ -839,7 +845,10 @@ fn process_paths( var_map.insert(S!("base"), b.value().clone()); } entry.odoo_path = entry.odoo_path.as_ref() - .and_then(|p| fill_or_canonicalize(p, ws_folders, workspace_name, &is_odoo_path, var_map.clone())); + .and_then(|p| fill_or_canonicalize(p, ws_folders, workspace_name, &is_odoo_path, var_map.clone()) + .map_err(|err| error!("Failed to process odoo path for variable {:?}: {}", p, err)) + .ok() + ); let infer = entry.addons_paths.as_mut().map_or(true, |ps| { let initial_len = ps.len(); @@ -849,6 +858,8 @@ fn process_paths( entry.addons_paths = entry.addons_paths.as_ref().map(|paths| paths.iter().filter_map(|sourced| { fill_or_canonicalize(sourced, ws_folders, workspace_name, &is_addon_path, var_map.clone()) + .map_err(|err| error!("Failed to process addons path for variable {:?}: {}", sourced, err)) + .ok() }).collect() ); if infer { @@ -869,6 +880,8 @@ fn process_paths( Some(p.clone()) } else { fill_or_canonicalize(p, ws_folders, workspace_name, &is_python_path, var_map.clone()) + .map_err(|err| error!("Failed to fill or canonicalize python path for variable {:?}: {}", p, err)) + .ok() } }); entry.stdlib.as_mut().map(|std|{ @@ -883,6 +896,27 @@ fn process_paths( } } }); + entry.diagnostic_filters.iter_mut().for_each(|filter| { + let Some(config_path) = filter.sources.iter().next().map(PathBuf::from) else { + unreachable!("Expected at least one source for sourced_path: {:?}", filter); + }; + let config_dir = config_path.parent().map(PathBuf::from).unwrap_or_else(|| PathBuf::from(".")); + filter.value.paths = filter.value.paths.iter().filter_map(|pattern| { + let pattern_string = pattern.to_string(); + let processed_pattern = fill_validate_path(ws_folders, workspace_name, &pattern_string, &|_: &String| true, var_map.clone(), &config_dir) + .and_then(|p| Pattern::new(&p) + .map_err(|e| e.to_string())); + match processed_pattern { + Ok(p) => Some(p), + Err(err) => { + let message = format!("Failed to process pattern '{}': {}", pattern_string, err); + filter.info = filter.info.clone() + &message; + error!(message); + None + } + } + }).collect(); + }); } fn read_config_from_file>(path: P) -> Result, String> { @@ -1205,7 +1239,7 @@ fn load_config_from_workspace( let Some(parent_dir) = version_path.parent() else { continue; }; - let Some(parent_dir) = fill_or_canonicalize( + let Ok(parent_dir) = fill_or_canonicalize( &{Sourced { value: parent_dir.sanitize(), sources: version_var.sources.clone(), ..Default::default() }}, ws_folders, Some(workspace_name), diff --git a/server/src/core/file_mgr.rs b/server/src/core/file_mgr.rs index f113bc67..32b369ae 100644 --- a/server/src/core/file_mgr.rs +++ b/server/src/core/file_mgr.rs @@ -306,10 +306,10 @@ impl FileInfo { pub fn update_diagnostic_filters(&mut self, session: &SessionInfo) { self.diagnostic_filters = session.sync_odoo.config.diagnostic_filters.iter().cloned().filter(|filter| { match filter.path_type { - DiagnosticFilterPathType::IN => { + DiagnosticFilterPathType::In => { filter.paths.iter().any(|p| p.matches(&self.uri)) } - DiagnosticFilterPathType::NOT_IN => { + DiagnosticFilterPathType::NotIn => { !filter.paths.iter().any(|p| p.matches(&self.uri)) } } diff --git a/server/src/features/features_utils.rs b/server/src/features/features_utils.rs index 401fe935..2ea5ea26 100644 --- a/server/src/features/features_utils.rs +++ b/server/src/features/features_utils.rs @@ -622,7 +622,7 @@ impl FeaturesUtils { } /// Finds and returns useful links for an evaluation - fn get_useful_link(session: &mut SessionInfo, typ: &EvaluationSymbolPtr) -> String { + fn get_useful_link(_session: &mut SessionInfo, typ: &EvaluationSymbolPtr) -> String { // Possibly add more links in the future let Some(typ) = typ.upgrade_weak() else { return S!("") diff --git a/server/src/server.rs b/server/src/server.rs index 2e48b75a..efad6aab 100644 --- a/server/src/server.rs +++ b/server/src/server.rs @@ -1,4 +1,4 @@ -use std::{collections::HashMap, io::Error, panic, sync::{atomic::AtomicBool, Arc, Mutex}, thread::JoinHandle}; +use std::{io::Error, panic, sync::{atomic::AtomicBool, Arc, Mutex}, thread::JoinHandle}; use crossbeam_channel::{Receiver, Select, Sender}; use lsp_server::{Connection, IoThreads, Message, ProtocolError, RequestId, ResponseError}; @@ -125,6 +125,7 @@ impl Server { if let Some(initialize_params) = initialize_params.process_id { self.client_process_id = initialize_params; } + #[allow(deprecated)] if let Some(workspace_folders) = initialize_params.workspace_folders { let sync_odoo = self.sync_odoo.lock().unwrap(); let file_mgr = sync_odoo.get_file_mgr(); diff --git a/server/src/utils.rs b/server/src/utils.rs index 85902788..416f0f3e 100644 --- a/server/src/utils.rs +++ b/server/src/utils.rs @@ -249,19 +249,22 @@ pub fn has_template(template: &str) -> bool { TEMPLATE_REGEX.is_match(template) } -pub fn fill_template(template: &str, vars: &HashMap) -> Option { - let mut invalid = false; +pub fn fill_template(template: &str, vars: &HashMap) -> Result { + let mut invalid = None; let result = TEMPLATE_REGEX.replace_all(template, |captures: ®ex::Captures| -> String{ let key = captures[1].to_string(); if let Some(value) = vars.get(&key) { value.clone() } else { - invalid = true; + invalid = Some(format!("Invalid key ({}) in pattern", key)); S!("") } }); - if invalid {None} else {Some(S!(result))} + match invalid { + Some(err) => Err(err), + None => Ok(S!(result)), + } } @@ -282,7 +285,7 @@ pub fn build_pattern_map(ws_folders: &HashMap) -> HashMap(ws_folders: &HashMap, workspace_name: Option<&String>, template: &str, predicate: F, var_map: HashMap, parent_path: P) -> Option +pub fn fill_validate_path(ws_folders: &HashMap, workspace_name: Option<&String>, template: &str, predicate: F, var_map: HashMap, parent_path: P) -> Result where F: Fn(&String) -> bool, P: AsRef @@ -291,19 +294,18 @@ where if let Some(path) = workspace_name.and_then(|name| ws_folders.get(name)) { pattern_map.insert(S!("workspaceFolder"), path.clone()); } - if let Some(path) = fill_template(template, &pattern_map) { - if predicate(&path) { - return Some(path); - } - // Attempt to convert the path to an absolute path - if let Ok(abs_path) = std::fs::canonicalize(parent_path.as_ref().join(&path)) { - let abs_path = abs_path.sanitize(); - if predicate(&abs_path) { - return Some(abs_path); - } + let path = fill_template(template, &pattern_map)?; + if predicate(&path) { + return Ok(path); + } + // Attempt to convert the path to an absolute path + if let Ok(abs_path) = std::fs::canonicalize(parent_path.as_ref().join(&path)) { + let abs_path = abs_path.sanitize(); + if predicate(&abs_path) { + return Ok(abs_path); } } - None + Err(format!("Failed to fill and validate path: {} from template {}", path, template)) } fn is_really_module(directory_path: &str, entry: &DirEntry) -> bool { diff --git a/server/tests/config_tests.rs b/server/tests/config_tests.rs index 8c445b89..02ad29aa 100644 --- a/server/tests/config_tests.rs +++ b/server/tests/config_tests.rs @@ -1832,4 +1832,41 @@ fn test_base_and_version_resolve_for_workspace_subpaths() { ws_folders.insert(S!("ws_invalid"), vdir.path().sanitize().to_string()); let result = get_configuration(&ws_folders, &None); assert!(result.is_err(), "Expected error when $base is not a valid path"); -} \ No newline at end of file +} + +#[test] +fn test_diagnostic_filter_path_variable_expansion() { + use dirs; + use odoo_ls_server::utils::PathSanitizer; + // Setup temp workspace + let temp = TempDir::new().unwrap(); + let ws1 = temp.child("ws1"); + ws1.create_dir_all().unwrap(); + let version = "vX.Y.Z"; + // Write odools.toml with $userHome, ${workspaceFolder}, and $version in diagnostic_filters + let user_home = dirs::home_dir().map(|buf| buf.sanitize()).unwrap(); + let toml_content = format!(r#" + [[config]] + name = "default" + "$version" = "{}" + [[config.diagnostic_filters]] + paths = ["${{userHome}}/some/path", "${{workspaceFolder}}/foo", "${{version}}/bar"] + "#, version); + ws1.child("odools.toml").write_str(&toml_content).unwrap(); + + let mut ws_folders = HashMap::new(); + ws_folders.insert(S!("ws1"), ws1.path().sanitize().to_string()); + + let (config_map, _config_file) = get_configuration(&ws_folders, &None).unwrap(); + let config = config_map.get("default").unwrap(); + let filters = &config.diagnostic_filters; + assert!(!filters.is_empty(), "Expected at least one diagnostic filter"); + let filter = &filters[0]; + let patterns: Vec = filter.paths.iter().map(|p| p.as_str().to_string()).collect(); + // Check that $userHome was expanded + assert!(patterns.iter().any(|p| p.starts_with(&format!("{}/some/path", user_home))), "userHome variable not expanded: {:?}", patterns); + // Check that ${workspaceFolder} was expanded + assert!(patterns.iter().any(|p| p.ends_with("/foo") && p.contains(&ws1.path().sanitize())), "workspaceFolder variable not expanded: {:?}", patterns); + // Check that $version was expanded + assert!(patterns.iter().any(|p| p.ends_with("/bar") && p.contains(version)), "version variable not expanded: {:?}", patterns); +}