From 4c5d6ca97dd48c78fefb65266129c3d956e5b81d Mon Sep 17 00:00:00 2001 From: dal Date: Tue, 8 Apr 2025 13:25:19 -0600 Subject: [PATCH] update column settings on sql change --- .../src/metrics/get_metric_handler.rs | 78 +++++++------ .../src/metrics/update_metric_handler.rs | 107 +++++++++++++----- 2 files changed, 125 insertions(+), 60 deletions(-) diff --git a/api/libs/handlers/src/metrics/get_metric_handler.rs b/api/libs/handlers/src/metrics/get_metric_handler.rs index 2c4395766..fba1f96c4 100644 --- a/api/libs/handlers/src/metrics/get_metric_handler.rs +++ b/api/libs/handlers/src/metrics/get_metric_handler.rs @@ -85,9 +85,10 @@ pub async fn get_metric_handler( password: Option, ) -> Result { // 1. Fetch metric file with permission - let metric_file_with_permission_option = fetch_metric_file_with_permissions(metric_id, &user.id) - .await - .map_err(|e| anyhow!("Failed to fetch metric file with permissions: {}", e))?; + let metric_file_with_permission_option = + fetch_metric_file_with_permissions(metric_id, &user.id) + .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 - let (metric_content, version_num) = if let Some(version) = version_number { - // Get the specific version if it exists - if let Some(v) = metric_file.version_history.get_version(version) { + // Determine which content and version number to use + let metric_content: database::types::MetricYml; + let version_num: i32; + let current_data_metadata: Option = 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 - if let Some(v) = metric_file.version_history.get_latest_version() { - match &v.content { - database::types::VersionContent::MetricYml(content) => { - (content.clone(), v.version_number) - } - _ => return Err(anyhow!("Invalid version content type")), - } - } else { - // Fall back to current content if no version history - (Box::new(metric_file.content.clone()), 1) + // --- No specific version requested - use current state from the main table row --- + tracing::debug!(metric_id = %metric_id, "No specific version requested, using current metric file content"); + metric_content = metric_file.content; // Use the content directly from the fetched MetricFile + // Determine the latest version number from history, defaulting to 1 if none + version_num = metric_file.version_history.get_version_number(); + tracing::debug!(metric_id = %metric_id, latest_version = version_num, "Determined latest version number"); + } + + // Convert the selected content to pretty YAML for the 'file' field + let file = match serde_yaml::to_string(&metric_content) { + 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 - let file = match serde_yaml::to_string(&metric_content) { - 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; + // Data metadata always comes from the main table record (current state) + let data_metadata = current_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, }) } diff --git a/api/libs/handlers/src/metrics/update_metric_handler.rs b/api/libs/handlers/src/metrics/update_metric_handler.rs index f60d1edbe..d7baba08c 100644 --- a/api/libs/handlers/src/metrics/update_metric_handler.rs +++ b/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 - let default_formats = + // Generate default column formats based ONLY on the new metadata + let default_formats_map: indexmap::IndexMap = ColumnLabelFormat::generate_formats_from_metadata(&query_result.metadata); - // Get existing chart config - let existing_config = serde_json::to_value(&content.chart_config)?; + // Get mutable access to the BaseChartConfig within the ChartConfig enum + 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 - let column_formats_json = serde_json::to_value(&default_formats)?; - let format_update = serde_json::json!({ - "columnLabelFormats": column_formats_json - }); + // Clone existing formats for comparison + let existing_formats_map = base_chart_config.column_label_formats.clone(); - // Merge the formats with existing config - let merged_config = merge_json_objects(existing_config, format_update)?; + // Clear column_settings since the SQL has changed and old settings might + // 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 - content.chart_config = serde_json::from_value(merged_config)?; + // Build the final map, starting empty. This ensures only columns from the + // 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::(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())?;