From 60ebcf79cd260f007aa46a6d76977791b3df5b43 Mon Sep 17 00:00:00 2001
From: dal <dallin@buster.so>
Date: Wed, 19 Mar 2025 10:49:56 -0600
Subject: [PATCH] created get_metric_handler_version_query_param

---
 .../src/dashboards/get_dashboard_handler.rs   |  4 +-
 .../src/metrics/get_metric_data_handler.rs    |  4 +-
 .../src/metrics/get_metric_handler.rs         | 52 +++++++++++++------
 .../src/metrics/update_metric_handler.rs      |  8 +--
 .../routes/rest/routes/metrics/get_metric.rs  | 22 ++++++--
 api/src/routes/ws/metrics/get_metric.rs       | 21 ++++++--
 6 files changed, 78 insertions(+), 33 deletions(-)

diff --git a/api/libs/handlers/src/dashboards/get_dashboard_handler.rs b/api/libs/handlers/src/dashboards/get_dashboard_handler.rs
index a8489f16b..baf3e2b70 100644
--- a/api/libs/handlers/src/dashboards/get_dashboard_handler.rs
+++ b/api/libs/handlers/src/dashboards/get_dashboard_handler.rs
@@ -89,10 +89,10 @@ pub async fn get_dashboard_handler(dashboard_id: &Uuid, user_id: &Uuid) -> Resul
         })
         .collect();
 
-    // Fetch all metrics concurrently
+    // Fetch all metrics concurrently (latest versions)
     let metric_futures: Vec<_> = metric_ids
         .iter()
-        .map(|metric_id| get_metric_handler(metric_id, user_id))
+        .map(|metric_id| get_metric_handler(metric_id, user_id, None))
         .collect();
 
     let metric_results = join_all(metric_futures).await;
diff --git a/api/libs/handlers/src/metrics/get_metric_data_handler.rs b/api/libs/handlers/src/metrics/get_metric_data_handler.rs
index 4ab6d109f..361bc2f48 100644
--- a/api/libs/handlers/src/metrics/get_metric_data_handler.rs
+++ b/api/libs/handlers/src/metrics/get_metric_data_handler.rs
@@ -103,8 +103,8 @@ pub async fn get_metric_data_handler(
 
     let user_id = user.id;
 
-    // Retrieve the metric definition
-    let metric = get_metric_handler(&request.metric_id, &user_id).await?;
+    // Retrieve the metric definition (latest version)
+    let metric = get_metric_handler(&request.metric_id, &user_id, None).await?;
 
     // Parse the metric definition from YAML to get SQL and dataset IDs
     let metric_yml = serde_yaml::from_str::<MetricYml>(&metric.file)?;
diff --git a/api/libs/handlers/src/metrics/get_metric_handler.rs b/api/libs/handlers/src/metrics/get_metric_handler.rs
index f24c59810..fb13925f5 100644
--- a/api/libs/handlers/src/metrics/get_metric_handler.rs
+++ b/api/libs/handlers/src/metrics/get_metric_handler.rs
@@ -48,8 +48,11 @@ struct UserInfo {
     avatar_url: Option<String>,
 }
 
-/// Handler to retrieve a metric by ID
-pub async fn get_metric_handler(metric_id: &Uuid, user_id: &Uuid) -> Result<BusterMetric> {
+/// Handler to retrieve a metric by ID with optional version number
+/// 
+/// If version_number is provided, returns that specific version of the metric.
+/// If version_number is None, returns the latest version of the metric.
+pub async fn get_metric_handler(metric_id: &Uuid, user_id: &Uuid, version_number: Option<i32>) -> Result<BusterMetric> {
     let mut conn = match get_pg_pool().get().await {
         Ok(conn) => conn,
         Err(e) => return Err(anyhow!("Failed to get database connection: {}", e)),
@@ -91,16 +94,38 @@ pub async fn get_metric_handler(metric_id: &Uuid, user_id: &Uuid) -> Result<Bust
         }
     });
 
-    let metric_yml = metric_file.content.clone();
+    // 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) {
+            match &v.content {
+                database::types::VersionContent::MetricYml(content) => (content.clone(), v.version_number),
+                _ => return Err(anyhow!("Invalid version content type"))
+            }
+        } else {
+            return Err(anyhow!("Version {} not found", 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
+            (metric_file.content.clone(), 1)
+        }
+    };
 
     // Convert content to pretty YAML
-    let file = match serde_yaml::to_string(&metric_file.content) {
+    let file = match serde_yaml::to_string(&metric_content) {
         Ok(yaml) => yaml,
         Err(e) => return Err(anyhow!("Failed to convert content to YAML: {}", e)),
     };
 
-    // Parse data metadata from MetricYml
-    let data_metadata = metric_yml.data_metadata.map(|metadata| {
+    // Parse data metadata from the selected version's MetricYml
+    let data_metadata = metric_content.data_metadata.map(|metadata| {
         DataMetadata {
             column_count: metadata.len() as i32,
             column_metadata: metadata
@@ -132,7 +157,7 @@ pub async fn get_metric_handler(metric_id: &Uuid, user_id: &Uuid) -> Result<Bust
 
     // Get dataset information for all dataset IDs
     let mut datasets = Vec::new();
-    for dataset_id in &metric_yml.dataset_ids {
+    for dataset_id in &metric_content.dataset_ids {
         if let Ok(dataset_info) = datasets::table
             .filter(datasets::id.eq(dataset_id))
             .filter(datasets::deleted_at.is_null())
@@ -164,25 +189,20 @@ pub async fn get_metric_handler(metric_id: &Uuid, user_id: &Uuid) -> Result<Bust
             updated_at: v.updated_at,
         })
         .collect();
-
-    // Get the latest version number from version history
-    let latest_version = metric_file.version_history.get_latest_version()
-        .map(|v| v.version_number)
-        .unwrap_or(1);
         
     // Construct BusterMetric
     Ok(BusterMetric {
         id: metric_file.id,
         metric_type: "metric".to_string(),
         title: metric_file.name,
-        version_number: latest_version,
-        description: metric_yml.description,
+        version_number: version_num,
+        description: metric_content.description,
         file_name: metric_file.file_name,
-        time_frame: metric_yml.time_frame,
+        time_frame: metric_content.time_frame,
         datasets,
         data_source_id: "".to_string(), // This would need to be fetched from another source
         error: None,
-        chart_config: Some(metric_yml.chart_config),
+        chart_config: Some(metric_content.chart_config),
         data_metadata,
         status: metric_file.verification,
         evaluation_score,
diff --git a/api/libs/handlers/src/metrics/update_metric_handler.rs b/api/libs/handlers/src/metrics/update_metric_handler.rs
index 1c2c5b989..dddd47aea 100644
--- a/api/libs/handlers/src/metrics/update_metric_handler.rs
+++ b/api/libs/handlers/src/metrics/update_metric_handler.rs
@@ -54,8 +54,8 @@ pub async fn update_metric_handler(
     let mut conn = get_pg_pool().get().await
         .map_err(|e| anyhow!("Failed to get database connection: {}", e))?;
 
-    // Check if metric exists and user has access
-    let metric = get_metric_handler(metric_id, user_id).await?;
+    // Check if metric exists and user has access - use the latest version
+    let metric = get_metric_handler(metric_id, user_id, None).await?;
 
     // If file is provided, it takes precedence over all other fields
     let content = if let Some(file_content) = request.file {
@@ -134,8 +134,8 @@ pub async fn update_metric_handler(
             .await
     }.map_err(|e| anyhow!("Failed to update metric: {}", e))?;
 
-    // Return the updated metric
-    get_metric_handler(metric_id, user_id).await
+    // Return the updated metric - latest version
+    get_metric_handler(metric_id, user_id, None).await
 }
 
 #[cfg(test)]
diff --git a/api/src/routes/rest/routes/metrics/get_metric.rs b/api/src/routes/rest/routes/metrics/get_metric.rs
index d150dca96..75ddf00c2 100644
--- a/api/src/routes/rest/routes/metrics/get_metric.rs
+++ b/api/src/routes/rest/routes/metrics/get_metric.rs
@@ -1,28 +1,42 @@
 use axum::{
-    extract::Path,
+    extract::{Path, Query},
     http::StatusCode,
     Extension,
 };
 use handlers::metrics::{get_metric_handler, BusterMetric};
+use serde::Deserialize;
 use uuid::Uuid;
 use middleware::AuthenticatedUser;
 
 use crate::routes::rest::ApiResponse;
 
+#[derive(Debug, Deserialize)]
+pub struct GetMetricQueryParams {
+    #[serde(rename = "version_number")]
+    version_number: Option<i32>,
+}
+
 pub async fn get_metric_rest_handler(
     Extension(user): Extension<AuthenticatedUser>,
     Path(id): Path<Uuid>,
+    Query(params): Query<GetMetricQueryParams>,
 ) -> Result<ApiResponse<BusterMetric>, (StatusCode, &'static str)> {
     tracing::info!(
-        "Processing GET request for metric with ID: {}, user_id: {}",
+        "Processing GET request for metric with ID: {}, user_id: {}, version_number: {:?}",
         id,
-        user.id
+        user.id,
+        params.version_number
     );
 
-    let metric = match get_metric_handler(&id, &user.id).await {
+    let metric = match get_metric_handler(&id, &user.id, params.version_number).await {
         Ok(response) => response,
         Err(e) => {
             tracing::error!("Error getting metric: {}", e);
+            let error_message = e.to_string();
+            // Return 404 if version not found, otherwise 500
+            if error_message.contains("Version") && error_message.contains("not found") {
+                return Err((StatusCode::NOT_FOUND, "Version not found"));
+            }
             return Err((StatusCode::INTERNAL_SERVER_ERROR, "Failed to get metric"));
         }
     };
diff --git a/api/src/routes/ws/metrics/get_metric.rs b/api/src/routes/ws/metrics/get_metric.rs
index 684c25855..5051d2259 100644
--- a/api/src/routes/ws/metrics/get_metric.rs
+++ b/api/src/routes/ws/metrics/get_metric.rs
@@ -14,25 +14,36 @@ use crate::routes::ws::{
 #[derive(Deserialize)]
 pub struct GetMetricWsRequest {
     pub id: Uuid,
+    #[serde(rename = "version_number")]
+    pub version_number: Option<i32>,
 }
 
 pub async fn get_metric(user: &AuthenticatedUser, request: GetMetricWsRequest) -> Result<()> {
     tracing::info!(
-        "Processing WebSocket GET request for metric with ID: {}, user_id: {}",
+        "Processing WebSocket GET request for metric with ID: {}, user_id: {}, version_number: {:?}",
         request.id,
-        user.id
+        user.id,
+        request.version_number
     );
 
-    let metric = match get_metric_handler(&request.id, &user.id).await {
+    let metric = match get_metric_handler(&request.id, &user.id, request.version_number).await {
         Ok(metric) => metric,
         Err(e) => {
             tracing::error!("Error getting metric: {}", e);
+            let error_message = e.to_string();
+            // Use appropriate error code based on the error
+            let error_code = if error_message.contains("Version") && error_message.contains("not found") {
+                WsErrorCode::NotFound
+            } else {
+                WsErrorCode::InternalServerError
+            };
+            
             send_error_message(
                 &user.id.to_string(),
                 WsRoutes::Metrics(MetricRoute::Get),
                 WsEvent::Metrics(MetricEvent::GetMetric),
-                WsErrorCode::InternalServerError,
-                "Failed to get metric.".to_string(),
+                error_code,
+                format!("Failed to get metric: {}", error_message),
                 user,
             )
             .await?;