we have a passer
This commit is contained in:
@@ -20,10 +20,7 @@ CREATE TABLE script_dependencies (
|
|||||||
-- Additional context (column name, query snippet, etc.)
|
-- Additional context (column name, query snippet, etc.)
|
||||||
context_info JSONB,
|
context_info JSONB,
|
||||||
|
|
||||||
created_at TIMESTAMPTZ NOT NULL DEFAULT CURRENT_TIMESTAMP,
|
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)
|
|
||||||
);
|
);
|
||||||
|
|
||||||
-- Optimized indexes for fast cycle detection
|
-- Optimized indexes for fast cycle detection
|
||||||
|
|||||||
@@ -202,6 +202,7 @@ impl DependencyAnalyzer {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// Check for cycles in the dependency graph using proper DFS
|
/// 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(
|
pub async fn check_for_cycles(
|
||||||
&self,
|
&self,
|
||||||
tx: &mut sqlx::Transaction<'_, sqlx::Postgres>,
|
tx: &mut sqlx::Transaction<'_, sqlx::Postgres>,
|
||||||
@@ -210,6 +211,7 @@ impl DependencyAnalyzer {
|
|||||||
) -> Result<(), DependencyError> {
|
) -> Result<(), DependencyError> {
|
||||||
// FIRST: Validate that structured table access respects link constraints
|
// FIRST: Validate that structured table access respects link constraints
|
||||||
self.validate_link_constraints(tx, table_id, new_dependencies).await?;
|
self.validate_link_constraints(tx, table_id, new_dependencies).await?;
|
||||||
|
|
||||||
// Get current dependency graph for this schema
|
// Get current dependency graph for this schema
|
||||||
let current_deps = sqlx::query!(
|
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
|
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
|
.await
|
||||||
.map_err(|e| DependencyError::DatabaseError { error: e.to_string() })?;
|
.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<i64, Vec<i64>> = HashMap::new();
|
let mut graph: HashMap<i64, Vec<i64>> = HashMap::new();
|
||||||
let mut table_names: HashMap<i64, String> = HashMap::new();
|
let mut table_names: HashMap<i64, String> = HashMap::new();
|
||||||
|
|
||||||
for dep in current_deps {
|
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.source_table_id, dep.source_name);
|
||||||
table_names.insert(dep.target_table_id, dep.target_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 {
|
for dep in new_dependencies {
|
||||||
// Look up target table ID
|
// Look up target table ID
|
||||||
let target_id = sqlx::query_scalar!(
|
let target_id = sqlx::query_scalar!(
|
||||||
@@ -249,7 +254,10 @@ impl DependencyAnalyzer {
|
|||||||
script_context: format!("table_id_{}", table_id),
|
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
|
// Get table name for error reporting
|
||||||
if !table_names.contains_key(&table_id) {
|
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)?;
|
self.detect_cycles_dfs(&graph, &table_names, table_id)?;
|
||||||
|
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn dfs_visit(
|
||||||
|
&self,
|
||||||
|
node: i64,
|
||||||
|
states: &mut HashMap<i64, NodeState>,
|
||||||
|
graph: &HashMap<i64, Vec<i64>>,
|
||||||
|
path: &mut Vec<i64>,
|
||||||
|
table_names: &HashMap<i64, String>,
|
||||||
|
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<String> = 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
|
/// Validates that structured table access (steel_get_column functions) respects link constraints
|
||||||
/// Raw SQL access (steel_query_sql) is allowed to reference any table
|
/// Raw SQL access (steel_query_sql) is allowed to reference any table
|
||||||
/// SELF-REFERENCES are always allowed (table can access its own columns)
|
/// SELF-REFERENCES are always allowed (table can access its own columns)
|
||||||
@@ -379,60 +451,6 @@ impl DependencyAnalyzer {
|
|||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
fn dfs_visit(
|
|
||||||
&self,
|
|
||||||
node: i64,
|
|
||||||
states: &mut HashMap<i64, NodeState>,
|
|
||||||
graph: &HashMap<i64, Vec<i64>>,
|
|
||||||
path: &mut Vec<i64>,
|
|
||||||
table_names: &HashMap<i64, String>,
|
|
||||||
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<String> = 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
|
/// Save dependencies to database within an existing transaction
|
||||||
pub async fn save_dependencies(
|
pub async fn save_dependencies(
|
||||||
&self,
|
&self,
|
||||||
|
|||||||
@@ -34,6 +34,21 @@ async fn create_test_table(
|
|||||||
.expect("Failed to 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
|
/// Helper function to get default schema ID
|
||||||
async fn get_default_schema_id(pool: &PgPool) -> i64 {
|
async fn get_default_schema_id(pool: &PgPool) -> i64 {
|
||||||
sqlx::query_scalar!("SELECT id FROM schemas WHERE name = 'default'")
|
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,
|
table_definition_id: table_id,
|
||||||
target_column: "big_number".to_string(), // This is BIGINT
|
target_column: "big_number".to_string(), // This is BIGINT
|
||||||
script: r#"(+ "10" "20")"#.to_string(),
|
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;
|
let result = post_table_script(&pool, request).await;
|
||||||
@@ -155,6 +170,8 @@ async fn test_reject_text_in_mathematical_operations() {
|
|||||||
]
|
]
|
||||||
).await;
|
).await;
|
||||||
|
|
||||||
|
// No self-link needed - self-references are automatically allowed
|
||||||
|
|
||||||
let request = PostTableScriptRequest {
|
let request = PostTableScriptRequest {
|
||||||
table_definition_id: table_id,
|
table_definition_id: table_id,
|
||||||
target_column: "result".to_string(),
|
target_column: "result".to_string(),
|
||||||
@@ -191,6 +208,8 @@ async fn test_reject_boolean_in_mathematical_operations() {
|
|||||||
]
|
]
|
||||||
).await;
|
).await;
|
||||||
|
|
||||||
|
// No self-link needed - self-references are automatically allowed
|
||||||
|
|
||||||
let request = PostTableScriptRequest {
|
let request = PostTableScriptRequest {
|
||||||
table_definition_id: table_id,
|
table_definition_id: table_id,
|
||||||
target_column: "result".to_string(),
|
target_column: "result".to_string(),
|
||||||
@@ -226,6 +245,8 @@ async fn test_reject_bigint_in_mathematical_operations() {
|
|||||||
]
|
]
|
||||||
).await;
|
).await;
|
||||||
|
|
||||||
|
// No self-link needed - self-references are automatically allowed
|
||||||
|
|
||||||
let request = PostTableScriptRequest {
|
let request = PostTableScriptRequest {
|
||||||
table_definition_id: table_id,
|
table_definition_id: table_id,
|
||||||
target_column: "result".to_string(),
|
target_column: "result".to_string(),
|
||||||
@@ -263,6 +284,8 @@ async fn test_allow_valid_script_with_allowed_types() {
|
|||||||
]
|
]
|
||||||
).await;
|
).await;
|
||||||
|
|
||||||
|
// No self-link needed - self-references are automatically allowed
|
||||||
|
|
||||||
let request = PostTableScriptRequest {
|
let request = PostTableScriptRequest {
|
||||||
table_definition_id: table_id,
|
table_definition_id: table_id,
|
||||||
target_column: "computed_value".to_string(), // This is TEXT (allowed as target)
|
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;
|
).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 {
|
let request = PostTableScriptRequest {
|
||||||
table_definition_id: table_id,
|
table_definition_id: table_id,
|
||||||
target_column: "total".to_string(),
|
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(),
|
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;
|
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");
|
assert!(result.is_ok(), "Mathematical operations with INTEGER and NUMERIC should succeed");
|
||||||
let response = result.unwrap();
|
let response = result.unwrap();
|
||||||
assert!(response.id > 0);
|
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);
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user