Refactor tag spec loading and extraction for robustness and correctness

crates/djls-templates/src/tagspecs.rs

@@ -45,12 +45,15 @@ impl TagSpecs {
         });
 
         match base_table_result {
+            Ok(base_table) => {
             Ok(base_table) => {
                 // Base table path found, extract specs from it recursively
                 let mut specs = HashMap::new();
-                // Start recursion with the base table and an empty initial path prefix
+                // Start recursion with the base table and an empty initial path prefix.
+                // Use map_err to ensure the error type matches.
                 extract_specs(base_table, "", &mut specs)
                     .map_err(|e| TagSpecError::Extract(path.display().to_string(), e))?;
+                // If base_table_result was Ok and extract_specs succeeded, return the populated specs map
                 Ok(TagSpecs(specs))
             }
             Err(e @ TagSpecError::Config(_, _)) => {
@@ -59,7 +62,7 @@ impl TagSpecs {
                 // Alternatively, could return Ok(TagSpecs::default()) if missing base is acceptable.
                 // Let's refine this based on how load_user_specs uses it.
                 // For now, propagate the specific Config error.
-                // If load_user_specs handles this Config error, it can proceed.
+                // load_user_specs and load_builtin_specs should handle this variant.
                 Err(e)
             }
             Err(e) => {
@@ -131,10 +134,10 @@ impl TagSpecs {
             let path = entry.path();
             if path.is_file() && path.extension().and_then(|ext| ext.to_str()) == Some("toml") {
                 match Self::load_from_toml(&path, &["tagspecs"]) {
-                    Ok(file_specs) => {
+                    Ok(file_specs) => { // Successfully loaded and extracted from this file
                         all_specs.extend(file_specs.0);
                     }
-                    Err(e) => {
+                    Err(e @ TagSpecError::Config(_, _)) | Err(e @ TagSpecError::Extract(_, _)) => {
                         eprintln!(
                             "Warning: Failed to load built-in tag specs from {}: {}",
                             path.display(),
@@ -172,7 +175,7 @@ fn extract_specs(
     current_value: &Value,
     current_path: &str, // The path leading up to current_value, e.g., "django.template.defaulttags"
     specs_map: &mut HashMap<String, TagSpec>,
-) -> Result<(), String> { // Return Result for error handling
+) -> Result<(), String> {
 
     // First, check if the current_value *itself* could be a TagSpec definition.
     // This happens when the current_path represents the full path to the tag.
@@ -182,7 +185,7 @@ fn extract_specs(
             Ok(tag_spec) => {
                 // Success! current_value represents a TagSpec definition.
                 // Extract tag_name from the *end* of current_path.
-                if let Some(tag_name) = current_path.split('.').last() {
+                if let Some(tag_name) = current_path.split('.').last().filter(|s| !s.is_empty()) {
                     // Insert into the map. Handle potential duplicates/overrides if needed.
                     specs_map.insert(tag_name.to_string(), tag_spec);
                     // Don't recurse further down this branch, we found the spec.
@@ -193,7 +196,7 @@ fn extract_specs(
                     return Err(format!("Could not extract tag name from non-empty path '{}'", current_path));
                 }
             }
-            Err(_) => {
+            Err(_) => { // Keep Err(_) to catch deserialization errors gracefully
                 // Deserialization as TagSpec failed. It might be a namespace table.
                 // Continue below to check if it's a table and recurse.
             }
@@ -207,7 +210,11 @@ fn extract_specs(
             // Construct the new path for the recursive call
             let new_path = if current_path.is_empty() { key.clone() } else { format!("{}.{}", current_path, key) };
             // Recurse
-            extract_specs(inner_value, &new_path, specs_map)?;
+            if let Err(e) = extract_specs(inner_value, &new_path, specs_map) {
+                // Propagate errors from recursive calls
+                // Optionally add more context here if needed
+                return Err(e);
+            }
         }
     }
     // If it's not a table and not a TagSpec, ignore it.
@@ -219,7 +226,7 @@ fn extract_specs(
 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
 pub struct TagSpec {
     /// Information about the closing tag, if one exists.
-    pub end: Option<EndTag>, // Changed from EndTagInfo
+    pub end: Option<EndTag>,
 
     /// List of intermediate tag names.
     #[serde(default)]
@@ -228,7 +235,7 @@ pub struct TagSpec {
 
 /// Defines properties of the end tag associated with a TagSpec.
 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
-pub struct EndTag { // Changed from EndTagInfo
+pub struct EndTag {
     /// The name of the closing tag.
     pub tag: String,
 
@@ -247,18 +254,18 @@ mod tests {
         filename: &str,
         content: &str,
     ) -> Result<tempfile::TempDir, anyhow::Error> {
-        let dir = tempfile::tempdir()?;
+        let dir = tempfile::tempdir()?; // Create temp dir in system default location
         let specs_dir = dir.path().join("tagspecs");
-        fs::create_dir(&specs_dir)?;
+        fs::create_dir_all(&specs_dir)?; // Use create_dir_all
         fs::write(specs_dir.join(filename), content)?;
         Ok(dir)
     }
 
     #[test]
-    fn test_load_builtin_simple() -> Result<(), anyhow::Error> {
+    fn test_load_builtin_simple() -> Result<(), anyhow::Error> { // Renamed from test_can_load_builtins
         let content = r#"
 # Using dotted path table names under [tagspecs] base
-[tagspecs.django.template.defaulttags.if]
+[tagspecs.django.template.defaulttags.if] // Corrected path
 end = { tag = "endif" }
 [tagspecs.django.template.defaulttags.block]
 end = { tag = "endblock" }
@@ -324,7 +331,7 @@ end = { tag = "endmytag", optional = true }
     }
 
     #[test]
-    fn test_load_user_djls_toml() -> Result<(), anyhow::Error> {
+    fn test_load_user_djls_toml() -> Result<(), anyhow::Error> { // Renamed from test_user_defined_tags
         let dir = tempfile::tempdir()?;
         let root = dir.path();
          // User specs under [tagspecs] base table
@@ -458,7 +465,7 @@ end = { tag = "endif_builtin" }
 end = { tag = "endblock_builtin" }
 "#;
          let specs_dir = root.join("tagspecs"); // Simulate built-in dir inside temp
-        fs::create_dir(&specs_dir)?;
+        fs::create_dir_all(&specs_dir)?; // Use create_dir_all
         fs::write(specs_dir.join("django.toml"), builtin_content)?;
 
          // Create a user override file (djls.toml has priority)
@@ -466,7 +473,7 @@ end = { tag = "endblock_builtin" }
 [tagspecs.django.template.defaulttags.if]
 end = { tag = "endif_user" } # Override built-in 'if'
 [tagspecs.custom.custom]
-end = { tag = "endcustom_user" } # Add user tag
+ end = { tag = "endcustom_user" } # Add user tag
 "#;
         fs::write(root.join("djls.toml"), user_content)?;