[IMP] allow vars in diagnostic filters and log parsing errors

mmahrouss · fda-odoo · commit fd023243f317 · 2025-11-04T14:46:27.000+01:00
diff --git 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};
@@ -66,13 +67,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
     }
 }
 
@@ -523,7 +524,7 @@ fn process_version(var: Sourced<String>, ws_folders: &HashMap<String, String>, 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
@@ -552,8 +553,11 @@ fn process_version(var: Sourced<String>, ws_folders: &HashMap<String, String>, 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
+        },
     }
 }
 
@@ -766,7 +770,7 @@ pub fn default_profile_name() -> String {
     "default".to_string()
 }
 
-fn fill_or_canonicalize<F>(sourced_path: &Sourced<String>, ws_folders: &HashMap<String, String>, workspace_name: Option<&String>, predicate: &F, var_map: HashMap<String, String>) -> Option<Sourced<String>>
+fn fill_or_canonicalize<F>(sourced_path: &Sourced<String>, ws_folders: &HashMap<String, String>, workspace_name: Option<&String>, predicate: &F, var_map: HashMap<String, String>) -> Result<Sourced<String>, String>
 where
 F: Fn(&String) -> bool,
 {
@@ -776,19 +780,21 @@ 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()});
     }
     let mut path = PathBuf::from(&sourced_path.value);
     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(
@@ -804,7 +810,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();
@@ -814,6 +823,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 {
@@ -834,6 +845,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|{
@@ -848,6 +861,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<P: AsRef<Path>>(path: P) -> Result<HashMap<String, ConfigEntryRaw>, String> {
@@ -1167,7 +1201,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
@@ -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
@@ -623,7 +623,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
@@ -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
@@ -249,19 +249,22 @@ pub fn has_template(template: &str) -> bool {
     TEMPLATE_REGEX.is_match(template)
 }
 
-pub fn fill_template(template: &str, vars: &HashMap<String, String>) -> Option<String> {
-    let mut invalid = false;
+pub fn fill_template(template: &str, vars: &HashMap<String, String>) -> Result<String, String> {
+    let mut invalid = None;
 
     let result = TEMPLATE_REGEX.replace_all(template, |captures: &regex::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<String, String>) -> HashMap<String
 /// While also checking it with the predicate function.
 /// pass `|_| true` to skip the predicate check.
 /// Currently, only the workspaceFolder[:workspace_name] and userHome variables are supported.
-pub fn fill_validate_path<F, P>(ws_folders: &HashMap<String, String>, workspace_name: Option<&String>, template: &str, predicate: F, var_map: HashMap<String, String>, parent_path: P) -> Option<String>
+pub fn fill_validate_path<F, P>(ws_folders: &HashMap<String, String>, workspace_name: Option<&String>, template: &str, predicate: F, var_map: HashMap<String, String>, parent_path: P) -> Result<String, String>
 where
     F: Fn(&String) -> bool,
     P: AsRef<Path>
@@ -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
@@ -1798,4 +1798,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");
-}
+}
+
+#[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<String> = 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);
+}

Original file line number	Diff line number	Diff line change
`@@ -306,10 +306,10 @@ impl FileInfo {`
`306`	`306`	`pub fn update_diagnostic_filters(&mut self, session: &SessionInfo) {`
`307`	`307`	`self.diagnostic_filters = session.sync_odoo.config.diagnostic_filters.iter().cloned().filter(\|filter\| {`
`308`	`308`	`match filter.path_type {`
`309`		`- DiagnosticFilterPathType::IN => {`
	`309`	`+ DiagnosticFilterPathType::In => {`
`310`	`310`	`filter.paths.iter().any(\|p\| p.matches(&self.uri))`
`311`	`311`	`}`
`312`		`- DiagnosticFilterPathType::NOT_IN => {`
	`312`	`+ DiagnosticFilterPathType::NotIn => {`
`313`	`313`	`!filter.paths.iter().any(\|p\| p.matches(&self.uri))`
`314`	`314`	`}`
`315`	`315`	`}`
Original file line number	Diff line number	Diff line change
`@@ -623,7 +623,7 @@ impl FeaturesUtils {`
`623`	`623`	`}`
`624`	`624`
`625`	`625`	`/// Finds and returns useful links for an evaluation`
`626`		`- fn get_useful_link(session: &mut SessionInfo, typ: &EvaluationSymbolPtr) -> String {`
	`626`	`+ fn get_useful_link(_session: &mut SessionInfo, typ: &EvaluationSymbolPtr) -> String {`
`627`	`627`	`// Possibly add more links in the future`
`628`	`628`	`let Some(typ) = typ.upgrade_weak() else {`
`629`	`629`	`return S!("")`
Original file line number	Diff line number	Diff line change
`@@ -125,6 +125,7 @@ impl Server {`
`125`	`125`	`if let Some(initialize_params) = initialize_params.process_id {`
`126`	`126`	`self.client_process_id = initialize_params;`
`127`	`127`	`}`
	`128`	`+ #[allow(deprecated)]`
`128`	`129`	`if let Some(workspace_folders) = initialize_params.workspace_folders {`
`129`	`130`	`let sync_odoo = self.sync_odoo.lock().unwrap();`
`130`	`131`	`let file_mgr = sync_odoo.get_file_mgr();`