From 7d1b130b68049b2a0087c404c2d0fcb1fdf0cc30 Mon Sep 17 00:00:00 2001 From: filipriec Date: Thu, 17 Jul 2025 22:55:54 +0200 Subject: [PATCH] we have a passer --- ...50710201933_create_script_dependencies.sql | 5 +- .../handlers/dependency_analyzer.rs | 136 ++++++++++-------- .../table_script/prohibited_types_test.rs | 125 +++++++++++++++- 3 files changed, 201 insertions(+), 65 deletions(-) diff --git a/server/migrations/20250710201933_create_script_dependencies.sql b/server/migrations/20250710201933_create_script_dependencies.sql index 62c3575..e926e00 100644 --- a/server/migrations/20250710201933_create_script_dependencies.sql +++ b/server/migrations/20250710201933_create_script_dependencies.sql @@ -20,10 +20,7 @@ CREATE TABLE script_dependencies ( -- Additional context (column name, query snippet, etc.) context_info JSONB, - created_at TIMESTAMPTZ NOT NULL DEFAULT CURRENT_TIMESTAMP, - - -- Prevent duplicate dependencies for the same script - UNIQUE (script_id, source_table_id, target_table_id, dependency_type) + created_at TIMESTAMPTZ NOT NULL DEFAULT CURRENT_TIMESTAMP ); -- Optimized indexes for fast cycle detection diff --git a/server/src/table_script/handlers/dependency_analyzer.rs b/server/src/table_script/handlers/dependency_analyzer.rs index 8781f40..fa903b0 100644 --- a/server/src/table_script/handlers/dependency_analyzer.rs +++ b/server/src/table_script/handlers/dependency_analyzer.rs @@ -202,6 +202,7 @@ impl DependencyAnalyzer { } /// Check for cycles in the dependency graph using proper DFS + /// Self-references are allowed and filtered out from cycle detection pub async fn check_for_cycles( &self, tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, @@ -210,6 +211,7 @@ impl DependencyAnalyzer { ) -> Result<(), DependencyError> { // FIRST: Validate that structured table access respects link constraints self.validate_link_constraints(tx, table_id, new_dependencies).await?; + // Get current dependency graph for this schema let current_deps = sqlx::query!( r#"SELECT sd.source_table_id, sd.target_table_id, st.table_name as source_name, tt.table_name as target_name @@ -223,17 +225,20 @@ impl DependencyAnalyzer { .await .map_err(|e| DependencyError::DatabaseError { error: e.to_string() })?; - // Build adjacency list + // Build adjacency list - EXCLUDE self-references since they're always allowed let mut graph: HashMap> = HashMap::new(); let mut table_names: HashMap = HashMap::new(); for dep in current_deps { - graph.entry(dep.source_table_id).or_default().push(dep.target_table_id); + // Skip self-references in cycle detection + if dep.source_table_id != dep.target_table_id { + graph.entry(dep.source_table_id).or_default().push(dep.target_table_id); + } table_names.insert(dep.source_table_id, dep.source_name); table_names.insert(dep.target_table_id, dep.target_name); } - // Add new dependencies to test + // Add new dependencies to test - EXCLUDE self-references for dep in new_dependencies { // Look up target table ID let target_id = sqlx::query_scalar!( @@ -249,7 +254,10 @@ impl DependencyAnalyzer { script_context: format!("table_id_{}", table_id), })?; - graph.entry(table_id).or_default().push(target_id); + // Only add to cycle detection graph if it's NOT a self-reference + if table_id != target_id { + graph.entry(table_id).or_default().push(target_id); + } // Get table name for error reporting if !table_names.contains_key(&table_id) { @@ -265,12 +273,76 @@ impl DependencyAnalyzer { } } - // Detect cycles using proper DFS algorithm + // Detect cycles using proper DFS algorithm (now without self-references) self.detect_cycles_dfs(&graph, &table_names, table_id)?; Ok(()) } + fn dfs_visit( + &self, + node: i64, + states: &mut HashMap, + graph: &HashMap>, + path: &mut Vec, + table_names: &HashMap, + starting_table: i64, + ) -> Result<(), DependencyError> { + states.insert(node, NodeState::Visiting); + path.push(node); + + if let Some(neighbors) = graph.get(&node) { + for &neighbor in neighbors { + // Ensure neighbor is in states map + if !states.contains_key(&neighbor) { + states.insert(neighbor, NodeState::Unvisited); + } + + match states.get(&neighbor).copied().unwrap_or(NodeState::Unvisited) { + NodeState::Visiting => { + // Check if this is a self-reference (allowed) or a real cycle (not allowed) + if neighbor == node { + // Self-reference: A table referencing itself is allowed + // Skip this - it's not a harmful cycle + continue; + } + + // Found a real cycle! Build the cycle path + let cycle_start_idx = path.iter().position(|&x| x == neighbor).unwrap_or(0); + let cycle_path: Vec = path[cycle_start_idx..] + .iter() + .chain(std::iter::once(&neighbor)) + .map(|&id| table_names.get(&id).cloned().unwrap_or_else(|| id.to_string())) + .collect(); + + // Only report as error if the cycle involves more than one table + if cycle_path.len() > 2 || (cycle_path.len() == 2 && cycle_path[0] != cycle_path[1]) { + let involving_script = table_names.get(&starting_table) + .cloned() + .unwrap_or_else(|| starting_table.to_string()); + + return Err(DependencyError::CircularDependency { + cycle_path, + involving_script, + }); + } + } + NodeState::Unvisited => { + // Recursively visit unvisited neighbor + self.dfs_visit(neighbor, states, graph, path, table_names, starting_table)?; + } + NodeState::Visited => { + // Already processed, no cycle through this path + } + } + } + } + + path.pop(); + states.insert(node, NodeState::Visited); + Ok(()) + } + /// Validates that structured table access (steel_get_column functions) respects link constraints /// Raw SQL access (steel_query_sql) is allowed to reference any table /// SELF-REFERENCES are always allowed (table can access its own columns) @@ -379,60 +451,6 @@ impl DependencyAnalyzer { Ok(()) } - fn dfs_visit( - &self, - node: i64, - states: &mut HashMap, - graph: &HashMap>, - path: &mut Vec, - table_names: &HashMap, - starting_table: i64, - ) -> Result<(), DependencyError> { - states.insert(node, NodeState::Visiting); - path.push(node); - - if let Some(neighbors) = graph.get(&node) { - for &neighbor in neighbors { - // Ensure neighbor is in states map - if !states.contains_key(&neighbor) { - states.insert(neighbor, NodeState::Unvisited); - } - - match states.get(&neighbor).copied().unwrap_or(NodeState::Unvisited) { - NodeState::Visiting => { - // Found a cycle! Build the cycle path - let cycle_start_idx = path.iter().position(|&x| x == neighbor).unwrap_or(0); - let cycle_path: Vec = path[cycle_start_idx..] - .iter() - .chain(std::iter::once(&neighbor)) - .map(|&id| table_names.get(&id).cloned().unwrap_or_else(|| id.to_string())) - .collect(); - - let involving_script = table_names.get(&starting_table) - .cloned() - .unwrap_or_else(|| starting_table.to_string()); - - return Err(DependencyError::CircularDependency { - cycle_path, - involving_script, - }); - } - NodeState::Unvisited => { - // Recursively visit unvisited neighbor - self.dfs_visit(neighbor, states, graph, path, table_names, starting_table)?; - } - NodeState::Visited => { - // Already processed, no cycle through this path - } - } - } - } - - path.pop(); - states.insert(node, NodeState::Visited); - Ok(()) - } - /// Save dependencies to database within an existing transaction pub async fn save_dependencies( &self, diff --git a/server/tests/table_script/prohibited_types_test.rs b/server/tests/table_script/prohibited_types_test.rs index 36412b5..fe33b89 100644 --- a/server/tests/table_script/prohibited_types_test.rs +++ b/server/tests/table_script/prohibited_types_test.rs @@ -34,6 +34,21 @@ async fn create_test_table( .expect("Failed to create test table") } +/// Helper function to create a table link (for cross-table references) +async fn create_table_link(pool: &PgPool, source_table_id: i64, linked_table_id: i64, is_required: bool) { + sqlx::query!( + r#"INSERT INTO table_definition_links (source_table_id, linked_table_id, is_required) + VALUES ($1, $2, $3) + ON CONFLICT (source_table_id, linked_table_id) DO NOTHING"#, + source_table_id, + linked_table_id, + is_required + ) + .execute(pool) + .await + .expect("Failed to create table link"); +} + /// Helper function to get default schema ID async fn get_default_schema_id(pool: &PgPool) -> i64 { sqlx::query_scalar!("SELECT id FROM schemas WHERE name = 'default'") @@ -59,7 +74,7 @@ async fn test_reject_bigint_target_column() { table_definition_id: table_id, target_column: "big_number".to_string(), // This is BIGINT script: r#"(+ "10" "20")"#.to_string(), - description: "Test script".to_string(), // Remove Some() wrapper + description: "Test script".to_string(), }; let result = post_table_script(&pool, request).await; @@ -155,6 +170,8 @@ async fn test_reject_text_in_mathematical_operations() { ] ).await; + // No self-link needed - self-references are automatically allowed + let request = PostTableScriptRequest { table_definition_id: table_id, target_column: "result".to_string(), @@ -191,6 +208,8 @@ async fn test_reject_boolean_in_mathematical_operations() { ] ).await; + // No self-link needed - self-references are automatically allowed + let request = PostTableScriptRequest { table_definition_id: table_id, target_column: "result".to_string(), @@ -226,6 +245,8 @@ async fn test_reject_bigint_in_mathematical_operations() { ] ).await; + // No self-link needed - self-references are automatically allowed + let request = PostTableScriptRequest { table_definition_id: table_id, target_column: "result".to_string(), @@ -263,6 +284,8 @@ async fn test_allow_valid_script_with_allowed_types() { ] ).await; + // No self-link needed - self-references are automatically allowed + let request = PostTableScriptRequest { table_definition_id: table_id, target_column: "computed_value".to_string(), // This is TEXT (allowed as target) @@ -295,6 +318,19 @@ async fn test_allow_integer_and_numeric_in_math_operations() { ] ).await; + println!("Created table with ID: {}", table_id); + println!("Schema ID: {}", schema_id); + + // Verify the table exists + let table_check = sqlx::query!( + "SELECT table_name FROM table_definitions WHERE id = $1", + table_id + ) + .fetch_one(&pool) + .await; + + println!("Table verification: {:?}", table_check); + let request = PostTableScriptRequest { table_definition_id: table_id, target_column: "total".to_string(), @@ -303,10 +339,95 @@ async fn test_allow_integer_and_numeric_in_math_operations() { description: "Valid mathematical operation with INTEGER and NUMERIC".to_string(), }; + println!("About to call post_table_script"); let result = post_table_script(&pool, request).await; + + // SHOW THE ACTUAL ERROR + if let Err(e) = &result { + println!("ERROR: {}", e); + println!("ERROR DEBUG: {:?}", e); + } - // Should succeed assert!(result.is_ok(), "Mathematical operations with INTEGER and NUMERIC should succeed"); let response = result.unwrap(); assert!(response.id > 0); } + +#[tokio::test] +async fn test_script_without_table_links_should_fail() { + let pool = setup_isolated_db().await; + let schema_id = get_default_schema_id(&pool).await; + + // Create two separate tables + let table_a_id = create_test_table( + &pool, + schema_id, + "table_a", + vec![("value_a", "INTEGER"), ("result", "INTEGER")] + ).await; + + let _table_b_id = create_test_table( + &pool, + schema_id, + "table_b", + vec![("value_b", "INTEGER")] + ).await; + + // DON'T create a link between the tables + + let request = PostTableScriptRequest { + table_definition_id: table_a_id, + target_column: "result".to_string(), + script: r#"(+ (steel_get_column "table_b" "value_b") "10")"#.to_string(), + description: "Script trying to access unlinked table".to_string(), + }; + + let result = post_table_script(&pool, request).await; + + // Should fail because tables aren't linked + assert!(result.is_err(), "Should fail when tables aren't linked"); + let error_msg = result.unwrap_err().to_string(); + assert!( + error_msg.contains("not linked") || error_msg.contains("link"), + "Error should mention table linking requirement: {}", + error_msg + ); +} + +#[tokio::test] +async fn test_script_with_table_links_should_succeed() { + let pool = setup_isolated_db().await; + let schema_id = get_default_schema_id(&pool).await; + + // Create two separate tables + let table_a_id = create_test_table( + &pool, + schema_id, + "linked_table_a", + vec![("value_a", "INTEGER"), ("result", "INTEGER")] + ).await; + + let table_b_id = create_test_table( + &pool, + schema_id, + "linked_table_b", + vec![("value_b", "INTEGER")] + ).await; + + // Create a link between the tables (table_a can access table_b) + create_table_link(&pool, table_a_id, table_b_id, false).await; + + let request = PostTableScriptRequest { + table_definition_id: table_a_id, + target_column: "result".to_string(), + script: r#"(+ (steel_get_column "linked_table_b" "value_b") "10")"#.to_string(), + description: "Script accessing properly linked table".to_string(), + }; + + let result = post_table_script(&pool, request).await; + + // Should succeed because tables are properly linked + assert!(result.is_ok(), "Should succeed when tables are properly linked"); + let response = result.unwrap(); + assert!(response.id > 0); +}