update column settings on sql change

api/libs/handlers/src/metrics/get_metric_handler.rs

@@ -85,9 +85,10 @@ pub async fn get_metric_handler(
     password: Option<String>,
 ) -> Result<BusterMetric> {
     // 1. Fetch metric file with permission
-    let metric_file_with_permission_option = fetch_metric_file_with_permissions(metric_id, &user.id)
+    let metric_file_with_permission_option =
-        .await
+        fetch_metric_file_with_permissions(metric_id, &user.id)
-        .map_err(|e| anyhow!("Failed to fetch metric file with permissions: {}", e))?;
+            .await
+            .map_err(|e| anyhow!("Failed to fetch metric file with permissions: {}", e))?;
 
     let metric_file_with_permission = if let Some(mf) = metric_file_with_permission_option {
         mf
@@ -98,7 +99,7 @@ pub async fn get_metric_handler(
 
     let metric_file = metric_file_with_permission.metric_file;
     let direct_permission_level = metric_file_with_permission.permission;
-    
+
     // 2. Determine the user's access level and enforce access rules
     let permission: AssetPermissionRole;
     tracing::debug!(metric_id = %metric_id, user_id = %user.id, "Checking permissions for metric");
@@ -130,7 +131,7 @@ pub async fn get_metric_handler(
             return Err(anyhow!("You don't have permission to view this metric"));
         }
         tracing::debug!(metric_id = %metric_id, "Metric is publicly accessible.");
-        
+
         // Check if the public access has expired
         if let Some(expiry_date) = metric_file.public_expiry_date {
             tracing::debug!(metric_id = %metric_id, ?expiry_date, "Checking expiry date");
@@ -139,7 +140,7 @@ pub async fn get_metric_handler(
                 return Err(anyhow!("Public access to this metric has expired"));
             }
         }
-        
+
         // Check if a password is required
         tracing::debug!(metric_id = %metric_id, has_password = metric_file.public_password.is_some(), "Checking password requirement");
         if let Some(required_password) = &metric_file.public_password {
@@ -179,42 +180,53 @@ pub async fn get_metric_handler(
         }
     });
 
-    // Determine which version to use based on version_number parameter
+    // Determine which content and version number to use
-    let (metric_content, version_num) = if let Some(version) = version_number {
+    let metric_content: database::types::MetricYml;
-        // Get the specific version if it exists
+    let version_num: i32;
-        if let Some(v) = metric_file.version_history.get_version(version) {
+    let current_data_metadata: Option<database::types::DataMetadata> = metric_file.data_metadata;
+
+    if let Some(requested_version) = version_number {
+        // --- Specific version requested ---
+        tracing::debug!(metric_id = %metric_id, version = requested_version, "Attempting to retrieve specific version");
+        if let Some(v) = metric_file.version_history.get_version(requested_version) {
             match &v.content {
                 database::types::VersionContent::MetricYml(content) => {
-                    (content.clone(), v.version_number)
+                    metric_content = (**content).clone(); // Deref the Box and clone
+                    version_num = v.version_number;
+                    tracing::debug!(metric_id = %metric_id, version = requested_version, "Successfully retrieved specific version content");
+                }
+                _ => {
+                    tracing::error!(metric_id = %metric_id, version = requested_version, "Invalid content type found for requested version");
+                    return Err(anyhow!(
+                        "Invalid content type found for version {}",
+                        requested_version
+                    ));
                 }
-                _ => return Err(anyhow!("Invalid version content type")),
             }
         } else {
-            return Err(anyhow!("Version {} not found", version));
+            tracing::warn!(metric_id = %metric_id, version = requested_version, "Requested version not found in history");
+            return Err(anyhow!("Version {} not found", requested_version));
         }
     } else {
-        // Get the latest version
+        // --- No specific version requested - use current state from the main table row ---
-        if let Some(v) = metric_file.version_history.get_latest_version() {
+        tracing::debug!(metric_id = %metric_id, "No specific version requested, using current metric file content");
-            match &v.content {
+        metric_content = metric_file.content; // Use the content directly from the fetched MetricFile
-                database::types::VersionContent::MetricYml(content) => {
+                                              // Determine the latest version number from history, defaulting to 1 if none
-                    (content.clone(), v.version_number)
+        version_num = metric_file.version_history.get_version_number();
-                }
+        tracing::debug!(metric_id = %metric_id, latest_version = version_num, "Determined latest version number");
-                _ => return Err(anyhow!("Invalid version content type")),
+    }
-            }
+
-        } else {
+    // Convert the selected content to pretty YAML for the 'file' field
-            // Fall back to current content if no version history
+    let file = match serde_yaml::to_string(&metric_content) {
-            (Box::new(metric_file.content.clone()), 1)
+        Ok(yaml) => yaml,
+        Err(e) => {
+            tracing::error!(metric_id = %metric_id, error = %e, "Failed to serialize selected metric content to YAML");
+            return Err(anyhow!("Failed to convert metric content to YAML: {}", e));
         }
     };
 
-    // Convert content to pretty YAML
+    // Data metadata always comes from the main table record (current state)
-    let file = match serde_yaml::to_string(&metric_content) {
+    let data_metadata = current_data_metadata;
-        Ok(yaml) => yaml,
-        Err(e) => return Err(anyhow!("Failed to convert content to YAML: {}", e)),
-    };
-
-    // Data metadata is fetched directly from the metric_file database record
-    let data_metadata = metric_file.data_metadata;
 
     let mut conn = get_pg_pool().get().await?;
 
@@ -368,6 +380,6 @@ pub async fn get_metric_handler(
         publicly_accessible: metric_file.publicly_accessible,
         public_expiry_date: metric_file.public_expiry_date,
         public_enabled_by: public_enabled_by_user,
-        public_password: metric_file.public_password
+        public_password: metric_file.public_password,
     })
 }

api/libs/handlers/src/metrics/update_metric_handler.rs

@@ -14,6 +14,7 @@ use query_engine::data_source_query_routes::query_engine::query_engine;
 use serde_json::Value;
 use sharing::check_permission_access;
 use uuid::Uuid;
+use indexmap;
 
 /// Recursively merges two JSON objects.
 /// The second object (update) takes precedence over the first (base) where there are conflicts.
@@ -204,20 +205,6 @@ pub async fn update_metric_handler(
         content
     };
 
-    // Calculate the next version number
-    let next_version = metric.versions.len() as i32 + 1;
-
-    // Only add a new version if update_version is true (defaults to true)
-    let should_update_version = request.update_version.unwrap_or(true);
-
-    // Add the new version to the version history or update the latest version
-    if should_update_version {
-        current_version_history.add_version(next_version, content.clone());
-    } else {
-        // Overwrite the current version instead of creating a new one
-        current_version_history.update_latest_version(content.clone());
-    }
-
     // Calculate data_metadata if SQL changed
     let data_metadata = if request.sql.is_some()
         || request.file.is_some()
@@ -241,24 +228,74 @@ pub async fn update_metric_handler(
             .await
             .map_err(|e| anyhow!("Failed to execute SQL for metadata calculation: {}", e))?;
 
-        // Generate default column formats based on metadata using the new method
+        // Generate default column formats based ONLY on the new metadata
-        let default_formats =
+        let default_formats_map: indexmap::IndexMap<String, ColumnLabelFormat> =
             ColumnLabelFormat::generate_formats_from_metadata(&query_result.metadata);
 
-        // Get existing chart config
+        // Get mutable access to the BaseChartConfig within the ChartConfig enum
-        let existing_config = serde_json::to_value(&content.chart_config)?;
+        let base_chart_config = match &mut content.chart_config {
+            database::types::ChartConfig::Bar(config) => &mut config.base,
+            database::types::ChartConfig::Line(config) => &mut config.base,
+            database::types::ChartConfig::Scatter(config) => &mut config.base,
+            database::types::ChartConfig::Pie(config) => &mut config.base,
+            database::types::ChartConfig::Combo(config) => &mut config.base,
+            database::types::ChartConfig::Metric(config) => &mut config.base,
+            database::types::ChartConfig::Table(config) => &mut config.base,
+        };
 
-        // Create a new JSON object with column_label_formats
+        // Clone existing formats for comparison
-        let column_formats_json = serde_json::to_value(&default_formats)?;
+        let existing_formats_map = base_chart_config.column_label_formats.clone();
-        let format_update = serde_json::json!({
-            "columnLabelFormats": column_formats_json
-        });
 
-        // Merge the formats with existing config
+        // Clear column_settings since the SQL has changed and old settings might
-        let merged_config = merge_json_objects(existing_config, format_update)?;
+        // reference columns that no longer exist in the result set
+        base_chart_config.column_settings = None;
+        
+        // Also clear other column-specific configurations that might be invalidated by SQL changes
+        if let Some(trendlines) = &mut base_chart_config.trendlines {
+            trendlines.clear();
+        }
 
-        // Update the content's chart_config
+        // Build the final map, starting empty. This ensures only columns from the
-        content.chart_config = serde_json::from_value(merged_config)?;
+        // new metadata are included.
+        let mut final_formats_map = indexmap::IndexMap::new();
+
+        // Iterate through the new defaults (columns guaranteed to exist in the new SQL result)
+        for (column_name, new_default_format) in default_formats_map {
+            let final_format = match existing_formats_map.get(&column_name) {
+                // Column existed before, merge existing customizations onto new default structure
+                Some(existing_format) => {
+                    // Convert both to Value for merging
+                    // We need to clone new_default_format as it's moved in the loop otherwise
+                    let new_default_value = serde_json::to_value(new_default_format.clone())?;
+                    let existing_value = serde_json::to_value(existing_format.clone())?;
+
+                    // Merge existing settings onto the default structure
+                    let merged_value = merge_json_objects(new_default_value, existing_value)?;
+
+                    // Attempt to deserialize back into ColumnLabelFormat
+                    match serde_json::from_value::<ColumnLabelFormat>(merged_value) {
+                        Ok(merged_format) => merged_format,
+                        Err(e) => {
+                            // Log the error and fallback to the new default format from metadata
+                            tracing::warn!(
+                                metric_id = %metric_id,
+                                column_name = %column_name,
+                                error = %e,
+                                "Failed to merge existing column format. Using default format from new metadata."
+                            );
+                            // Use the original default format from the map iteration (before clone)
+                            new_default_format
+                        }
+                    }
+                }
+                // Column is new, use the generated default format
+                None => new_default_format,
+            };
+            final_formats_map.insert(column_name, final_format);
+        }
+
+        // Replace the formats in the base config with the newly constructed map
+        base_chart_config.column_label_formats = final_formats_map;
 
         // Return metadata
         Some(query_result.metadata)
@@ -266,6 +303,22 @@ pub async fn update_metric_handler(
         None
     };
 
+    // Calculate the next version number
+    let next_version = metric.versions.len() as i32 + 1;
+
+    // Only add a new version if update_version is true (defaults to true)
+    let should_update_version = request.update_version.unwrap_or(true);
+
+    // Add the new version to the version history or update the latest version
+    // IMPORTANT: This happens AFTER we've updated the column_label_formats
+    // to ensure the version history captures those changes
+    if should_update_version {
+        current_version_history.add_version(next_version, content.clone());
+    } else {
+        // Overwrite the current version instead of creating a new one
+        current_version_history.update_latest_version(content.clone());
+    }
+
     // Convert content to JSON for storage
     let content_json = serde_json::to_value(content.clone())?;