From 168972be6d4eb68a854ede31edbbebbb8bdccc78 Mon Sep 17 00:00:00 2001 From: dal Date: Tue, 25 Mar 2025 11:28:44 -0600 Subject: [PATCH] metric restore to version --- .../src/metrics/update_metric_handler.rs | 64 ++++- api/prds/active/restoration_project.md | 6 +- .../active/restoration_project_metrics.md | 250 ++++++++++-------- api/tests/common/fixtures/metrics.rs | 7 + .../integration/metrics/update_metric_test.rs | 106 +++++++- 5 files changed, 315 insertions(+), 118 deletions(-) diff --git a/api/libs/handlers/src/metrics/update_metric_handler.rs b/api/libs/handlers/src/metrics/update_metric_handler.rs index 704d21ccc..2d9ef51cc 100644 --- a/api/libs/handlers/src/metrics/update_metric_handler.rs +++ b/api/libs/handlers/src/metrics/update_metric_handler.rs @@ -5,7 +5,7 @@ use database::{ helpers::metric_files::fetch_metric_file_with_permissions, pool::get_pg_pool, schema::metric_files, - types::{MetricYml, VersionHistory}, + types::{MetricYml, VersionContent, VersionHistory}, }; use diesel::{ExpressionMethods, QueryDsl}; use diesel_async::RunQueryDsl; @@ -63,6 +63,7 @@ pub struct UpdateMetricRequest { pub file: Option, pub sql: Option, pub update_version: Option, + pub restore_to_version: Option, } /// Handler to update a metric by ID @@ -100,8 +101,35 @@ pub async fn update_metric_handler( // Check if metric exists and user has access - use the latest version let metric = get_metric_handler(metric_id, user, None).await?; - // If file is provided, it takes precedence over all other fields - let content = if let Some(file_content) = request.file { + // Get version history + let mut current_version_history: VersionHistory = metric_files::table + .filter(metric_files::id.eq(metric_id)) + .select(metric_files::version_history) + .first::(&mut conn) + .await + .map_err(|e| anyhow!("Failed to get version history: {}", e))?; + + // Version restoration takes highest precedence + let content = if let Some(version_number) = request.restore_to_version { + // Fetch the requested version + let version = current_version_history + .get_version(version_number) + .ok_or_else(|| anyhow!("Version {} not found", version_number))?; + + // Parse the YAML content from the version + match &version.content { + VersionContent::MetricYml(metric_yml) => { + tracing::info!( + "Restoring metric {} to version {}", + metric_id, + version_number + ); + (**metric_yml).clone() + } + _ => return Err(anyhow!("Invalid version content type")), + } + // If file is provided, it takes precedence over individual fields + } else if let Some(file_content) = request.file { serde_yaml::from_str::(&file_content) .map_err(|e| anyhow!("Failed to parse provided file content: {}", e))? } else { @@ -144,14 +172,6 @@ pub async fn update_metric_handler( content }; - // Get and update version history - let mut current_version_history: VersionHistory = metric_files::table - .filter(metric_files::id.eq(metric_id)) - .select(metric_files::version_history) - .first::(&mut conn) - .await - .map_err(|e| anyhow!("Failed to get version history: {}", e))?; - // Calculate the next version number let next_version = metric.versions.len() as i32 + 1; @@ -225,6 +245,7 @@ mod tests { file: None, sql: None, update_version: None, + restore_to_version: None, }; // Verify the request fields are properly structured @@ -237,6 +258,27 @@ mod tests { // Actual validation logic is tested in integration tests } + #[test] + fn test_restore_version_request() { + // Test restoration request validation + let restore_request = UpdateMetricRequest { + restore_to_version: Some(1), + name: Some("This should be ignored".to_string()), + description: Some("This should be ignored".to_string()), + chart_config: None, + time_frame: None, + dataset_ids: None, + verification: None, + file: None, + sql: None, + update_version: None, + }; + + // Verify the request fields are properly structured + assert_eq!(restore_request.restore_to_version.unwrap(), 1); + assert_eq!(restore_request.name.unwrap(), "This should be ignored"); + } + #[test] fn test_chart_config_partial_update() { // This test verifies that partial chart_config updates only change specified fields diff --git a/api/prds/active/restoration_project.md b/api/prds/active/restoration_project.md index c9e8841c3..d049b5302 100644 --- a/api/prds/active/restoration_project.md +++ b/api/prds/active/restoration_project.md @@ -88,9 +88,9 @@ No schema changes are required. The feature will use the existing version histor ### Phase 1: Metric and Dashboard Update Handlers #### Task 1.1: Update Metric Handler -- Modify `update_metric_handler.rs` to accept `restore_to_version` parameter -- Implement version restoration logic -- Add unit tests for the new functionality +- ✅ Modify `update_metric_handler.rs` to accept `restore_to_version` parameter +- ✅ Implement version restoration logic +- ⚠️ Add unit and integration tests for the new functionality (tests written but execution verification pending) #### Task 1.2: Update Dashboard Handler - Modify `update_dashboard_handler.rs` to accept `restore_to_version` parameter diff --git a/api/prds/active/restoration_project_metrics.md b/api/prds/active/restoration_project_metrics.md index 0c9ee750b..3b0f4330c 100644 --- a/api/prds/active/restoration_project_metrics.md +++ b/api/prds/active/restoration_project_metrics.md @@ -11,6 +11,8 @@ parent_prd: restoration_project.md ## Overview This document details the implementation of version restoration for metrics as part of the larger [Version Restoration Feature](restoration_project.md) project. +**Status: ⚠️ Implemented but Tests Need Verification** + ## Technical Design ### Update Metric Handler Modification @@ -45,7 +47,7 @@ pub struct UpdateMetricRequest { ### Implementation Details -The update handler will need to be modified as follows: +The update handler has been modified as follows: ```rust pub async fn update_metric_handler( @@ -66,7 +68,7 @@ pub async fn update_metric_handler( .await .map_err(|e| anyhow!("Failed to get version history: {}", e))?; - // Version restoration logic + // Version restoration takes highest precedence let content = if let Some(version_number) = request.restore_to_version { // Fetch the requested version let version = current_version_history @@ -74,8 +76,17 @@ pub async fn update_metric_handler( .ok_or_else(|| anyhow!("Version {} not found", version_number))?; // Parse the YAML content from the version - serde_yaml::from_str::(&serde_yaml::to_string(&version.content)?) - .map_err(|e| anyhow!("Failed to parse restored version content: {}", e))? + match &version.content { + VersionContent::MetricYml(metric_yml) => { + tracing::info!( + "Restoring metric {} to version {}", + metric_id, + version_number + ); + (**metric_yml).clone() + } + _ => return Err(anyhow!("Invalid version content type")), + } } else if let Some(file_content) = request.file { // Existing file content logic // ... @@ -102,6 +113,8 @@ pub async fn update_metric_handler( } ``` +✅ **Implementation Complete** + ## Testing ### Unit Tests @@ -180,136 +193,167 @@ The following integration tests should verify end-to-end functionality: - Verify the system handles errors gracefully without corrupting data - Verify appropriate error responses -### Example Unit Test Code +### Unit Test Code + +The unit tests have been implemented as follows: ```rust #[tokio::test] async fn test_restore_metric_version() { - // Set up test environment - let pool = setup_test_db().await; - let user = create_test_user().await; + // Setup test environment + setup_test_env(); - // Create a metric with initial content - let initial_content = MetricYml { - name: "Original Metric".to_string(), - description: Some("Original description".to_string()), - time_frame: "last_7_days".to_string(), - // Other fields... - }; + // Initialize test database + let test_db = TestDb::new().await?; + let mut conn = test_db.get_conn().await?; - let metric_id = create_test_metric(&user, initial_content.clone()).await; + // Create test user and organization + let user_id = Uuid::new_v4(); + let org_id = Uuid::new_v4(); + + // Create test metric with initial version + let test_metric = create_test_metric_file(&user_id, &org_id, Some("Original Metric".to_string())); + let metric_id = test_metric.id; + + // Insert test metric into database + diesel::insert_into(metric_files::table) + .values(&test_metric) + .execute(&mut conn) + .await?; // Update the metric to create version 2 - let updated_content = MetricYml { - name: "Updated Metric".to_string(), - description: Some("Updated description".to_string()), - time_frame: "last_30_days".to_string(), - // Other fields... - }; + let update_json = create_update_metric_request(); + let update_request: UpdateMetricRequest = serde_json::from_value(update_json)?; + update_metric_handler(&metric_id, &user_id, update_request).await?; - update_test_metric(&metric_id, &user, updated_content).await; + // Fetch the metric to verify we have 2 versions + let db_metric = metric_files::table + .filter(metric_files::id.eq(metric_id)) + .first::(&mut conn) + .await?; + + assert_eq!(db_metric.version_history.versions.len(), 2); + assert_eq!(db_metric.name, "Updated Test Metric"); + + // Create restore request to restore to version 1 + let restore_json = create_restore_metric_request(1); + let restore_request: UpdateMetricRequest = serde_json::from_value(restore_json)?; // Restore to version 1 - let restore_request = UpdateMetricRequest { - restore_to_version: Some(1), - ..Default::default() - }; + let restored_metric = update_metric_handler(&metric_id, &user_id, restore_request).await?; - let result = update_metric_handler(&metric_id, &user, restore_request).await; + // Fetch the restored metric from the database + let db_metric_after_restore = metric_files::table + .filter(metric_files::id.eq(metric_id)) + .first::(&mut conn) + .await?; - // Assertions - assert!(result.is_ok()); + // Verify we now have 3 versions + assert_eq!(db_metric_after_restore.version_history.versions.len(), 3); - let restored_metric = result.unwrap(); + // Verify the restored content matches the original version + let content: Value = db_metric_after_restore.content; + assert_eq!(content["name"].as_str().unwrap(), "Original Metric"); + assert_eq!(content["time_frame"].as_str().unwrap(), "daily"); - // Verify a new version was created (should be version 3) - assert_eq!(restored_metric.version, 3); - - // Verify the content matches the original version - let restored_content = serde_yaml::from_str::(&restored_metric.file).unwrap(); - assert_eq!(restored_content.name, initial_content.name); - assert_eq!(restored_content.description, initial_content.description); - assert_eq!(restored_content.time_frame, initial_content.time_frame); - // Verify other fields... + // Verify the restored metric in response matches as well + assert_eq!(restored_metric.name, "Original Metric"); + assert_eq!(restored_metric.versions.len(), 3); } ``` -### Example Integration Test Code +✅ **Unit Tests Implemented** + +### Integration Test Code + +The integration tests have been implemented as follows: ```rust #[tokio::test] -async fn test_metric_restore_integration() { - // Set up test server with routes - let app = create_test_app().await; - let client = TestClient::new(app); +async fn test_restore_metric_version() { + // Setup test environment + setup_test_env(); - // Create a test user and authenticate - let (user, token) = create_and_login_test_user().await; + // Initialize test database + let test_db = TestDb::new().await?; + let mut conn = test_db.get_conn().await?; - // Create a metric - let create_response = client - .post("/metrics") - .header("Authorization", format!("Bearer {}", token)) - .json(&json!({ - "name": "Test Metric", - "description": "Initial description", - "time_frame": "last_7_days", - // Other required fields... - })) - .send() - .await; + // Create test user and organization + let user_id = Uuid::new_v4(); + let org_id = Uuid::new_v4(); - assert_eq!(create_response.status(), StatusCode::OK); - let metric: BusterMetric = create_response.json().await; + // Create test metric with initial version + let test_metric = create_test_metric_file(&user_id, &org_id, Some("Original Metric".to_string())); + let metric_id = test_metric.id; + + // Insert test metric into database + diesel::insert_into(metric_files::table) + .values(&test_metric) + .execute(&mut conn) + .await?; // Update the metric to create version 2 - let update_response = client - .put(&format!("/metrics/{}", metric.id)) - .header("Authorization", format!("Bearer {}", token)) - .json(&json!({ - "name": "Updated Metric", - "description": "Updated description", - "time_frame": "last_30_days", - })) - .send() - .await; + let update_json = create_update_metric_request(); + let update_request: UpdateMetricRequest = serde_json::from_value(update_json)?; + update_metric_handler(&metric_id, &user_id, update_request).await?; - assert_eq!(update_response.status(), StatusCode::OK); + // Create restore request to restore to version 1 + let restore_json = create_restore_metric_request(1); + let restore_request: UpdateMetricRequest = serde_json::from_value(restore_json)?; // Restore to version 1 - let restore_response = client - .put(&format!("/metrics/{}", metric.id)) - .header("Authorization", format!("Bearer {}", token)) - .json(&json!({ - "restore_to_version": 1 - })) - .send() - .await; + let restored_metric = update_metric_handler(&metric_id, &user_id, restore_request).await?; - assert_eq!(restore_response.status(), StatusCode::OK); - let restored_metric: BusterMetric = restore_response.json().await; - - // Verify the metric was restored properly - assert_eq!(restored_metric.name, "Test Metric"); - assert_eq!(restored_metric.version, 3); - - // Verify by fetching the metric again - let get_response = client - .get(&format!("/metrics/{}", metric.id)) - .header("Authorization", format!("Bearer {}", token)) - .send() - .await; - - assert_eq!(get_response.status(), StatusCode::OK); - let fetched_metric: BusterMetric = get_response.json().await; - - // Verify the fetched metric matches the restored version - assert_eq!(fetched_metric.name, "Test Metric"); - assert_eq!(fetched_metric.version, 3); + // Verify the restored content matches the original version + assert_eq!(restored_metric.name, "Original Metric"); + assert_eq!(restored_metric.versions.len(), 3); } +#[tokio::test] +async fn test_restore_metric_nonexistent_version() { + // Setup test environment + setup_test_env(); + + // Initialize test database + let test_db = TestDb::new().await?; + let mut conn = test_db.get_conn().await?; + + // Create test user and organization + let user_id = Uuid::new_v4(); + let org_id = Uuid::new_v4(); + + // Create test metric + let test_metric = create_test_metric_file(&user_id, &org_id, Some("Test Metric".to_string())); + let metric_id = test_metric.id; + + // Insert test metric into database + diesel::insert_into(metric_files::table) + .values(&test_metric) + .execute(&mut conn) + .await?; + + // Create restore request with a non-existent version number + let restore_json = create_restore_metric_request(999); + let restore_request: UpdateMetricRequest = serde_json::from_value(restore_json)?; + + // Attempt to restore to non-existent version + let result = update_metric_handler(&metric_id, &user_id, restore_request).await; + + // Verify the error + assert!(result.is_err()); + let error = result.unwrap_err().to_string(); + assert!(error.contains("Version 999 not found")); +} +``` + +⚠️ **Integration Tests Implemented but Not Verified** + +Note: Integration tests have been written according to the requirements, but could not be executed successfully due to pre-existing failures in the codebase's test suite. These tests should be verified once the underlying test issues are resolved. + ## Security Considerations -- The existing permission checks in `update_metric_handler` should be maintained -- Only users with `CanEdit`, `FullAccess`, or `Owner` permissions should be able to restore versions -- Ensure the audit trail captures version restoration actions +- The existing permission checks in `update_metric_handler` are maintained +- Only users with `CanEdit`, `FullAccess`, or `Owner` permissions can restore versions +- The audit trail captures version restoration actions through automatic versioning + +✅ **Security Considerations Addressed** diff --git a/api/tests/common/fixtures/metrics.rs b/api/tests/common/fixtures/metrics.rs index 7095a0369..0313871e5 100644 --- a/api/tests/common/fixtures/metrics.rs +++ b/api/tests/common/fixtures/metrics.rs @@ -80,6 +80,13 @@ pub fn create_update_metric_request() -> Value { }) } +/// Creates a request to restore a metric to a specific version +pub fn create_restore_metric_request(version_number: i32) -> Value { + serde_json::json!({ + "restore_to_version": version_number + }) +} + /// Creates dashboard association request data pub fn create_metric_dashboard_association_request(dashboard_id: &Uuid) -> Value { serde_json::json!({ diff --git a/api/tests/integration/metrics/update_metric_test.rs b/api/tests/integration/metrics/update_metric_test.rs index acfa7b2cc..2c27f0a80 100644 --- a/api/tests/integration/metrics/update_metric_test.rs +++ b/api/tests/integration/metrics/update_metric_test.rs @@ -4,6 +4,7 @@ use database::{ models::MetricFile, pool::get_pg_pool, schema::metric_files, + types::{MetricYml, VersionHistory}, }; use diesel::{ExpressionMethods, QueryDsl}; use diesel_async::RunQueryDsl; @@ -15,7 +16,7 @@ use uuid::Uuid; use crate::common::{ db::TestDb, env::setup_test_env, - fixtures::{create_test_metric_file, create_test_user, create_update_metric_request}, + fixtures::{create_test_metric_file, create_test_user, create_update_metric_request, create_restore_metric_request}, }; #[tokio::test] @@ -71,6 +72,109 @@ async fn test_update_metric_handler() -> Result<()> { Ok(()) } +#[tokio::test] +async fn test_restore_metric_version() -> Result<()> { + // Setup test environment + setup_test_env(); + + // Initialize test database + let test_db = TestDb::new().await?; + let mut conn = test_db.get_conn().await?; + + // Create test user and organization + let user_id = Uuid::new_v4(); + let org_id = Uuid::new_v4(); + + // Create test metric with initial version + let test_metric = create_test_metric_file(&user_id, &org_id, Some("Original Metric".to_string())); + let metric_id = test_metric.id; + + // Insert test metric into database + diesel::insert_into(metric_files::table) + .values(&test_metric) + .execute(&mut conn) + .await?; + + // Update the metric to create version 2 + let update_json = create_update_metric_request(); + let update_request: UpdateMetricRequest = serde_json::from_value(update_json)?; + update_metric_handler(&metric_id, &user_id, update_request).await?; + + // Fetch the metric to verify we have 2 versions + let db_metric = metric_files::table + .filter(metric_files::id.eq(metric_id)) + .first::(&mut conn) + .await?; + + assert_eq!(db_metric.version_history.versions.len(), 2); + assert_eq!(db_metric.name, "Updated Test Metric"); + + // Create restore request to restore to version 1 + let restore_json = create_restore_metric_request(1); + let restore_request: UpdateMetricRequest = serde_json::from_value(restore_json)?; + + // Restore to version 1 + let restored_metric = update_metric_handler(&metric_id, &user_id, restore_request).await?; + + // Fetch the restored metric from the database + let db_metric_after_restore = metric_files::table + .filter(metric_files::id.eq(metric_id)) + .first::(&mut conn) + .await?; + + // Verify we now have 3 versions + assert_eq!(db_metric_after_restore.version_history.versions.len(), 3); + + // Verify the restored content matches the original version + let content: Value = db_metric_after_restore.content; + assert_eq!(content["name"].as_str().unwrap(), "Original Metric"); + assert_eq!(content["time_frame"].as_str().unwrap(), "daily"); + + // Verify the restored metric in response matches as well + assert_eq!(restored_metric.name, "Original Metric"); + assert_eq!(restored_metric.versions.len(), 3); + + Ok(()) +} + +#[tokio::test] +async fn test_restore_metric_nonexistent_version() -> Result<()> { + // Setup test environment + setup_test_env(); + + // Initialize test database + let test_db = TestDb::new().await?; + let mut conn = test_db.get_conn().await?; + + // Create test user and organization + let user_id = Uuid::new_v4(); + let org_id = Uuid::new_v4(); + + // Create test metric + let test_metric = create_test_metric_file(&user_id, &org_id, Some("Test Metric".to_string())); + let metric_id = test_metric.id; + + // Insert test metric into database + diesel::insert_into(metric_files::table) + .values(&test_metric) + .execute(&mut conn) + .await?; + + // Create restore request with a non-existent version number + let restore_json = create_restore_metric_request(999); + let restore_request: UpdateMetricRequest = serde_json::from_value(restore_json)?; + + // Attempt to restore to non-existent version + let result = update_metric_handler(&metric_id, &user_id, restore_request).await; + + // Verify the error + assert!(result.is_err()); + let error = result.unwrap_err().to_string(); + assert!(error.contains("Version 999 not found")); + + Ok(()) +} + #[tokio::test] async fn test_update_metric_handler_not_found() -> Result<()> { // Setup test environment