From 255db517c7a7d74229997cac6da3151ca47f9b03 Mon Sep 17 00:00:00 2001 From: Daniel Moellenbeck Date: Wed, 18 Mar 2026 16:53:20 +0100 Subject: [PATCH] fix(rvlite): CypherEngine MATCH returns all matched rows with properties Previously, execute_match() merged all matched node contexts into a single ExecutionContext via context.bind(). Each call to bind() overwrote the variable, so only the last matched node survived into RETURN. This meant any MATCH query returned at most 1 row with empty properties. Changes in executor.rs: - execute_match() now returns Vec (one per matched row) - execute_return() iterates all contexts to produce multiple result rows - Column names use expression text ("p.name") instead of "?column?" - Added ContextValue::to_json_value() for proper WASM serialization - Added value_to_json() helper covering all Value variants incl List/Map Changes in mod.rs (WASM binding): - Serializes via serde_json + js_sys::JSON::parse instead of serde_wasm_bindgen::to_value which failed on ContextValue enums Tests added (cypher_integration_test.rs): - test_match_return_multiple_rows - test_match_where_return_multiple_rows - test_match_return_variable_node All 24 existing cypher unit tests pass. Fixes #269 --- crates/rvlite/src/cypher/executor.rs | 178 ++++++++++++++---- crates/rvlite/src/cypher/mod.rs | 12 +- .../rvlite/tests/cypher_integration_test.rs | 133 +++++++++++++ 3 files changed, 286 insertions(+), 37 deletions(-) diff --git a/crates/rvlite/src/cypher/executor.rs b/crates/rvlite/src/cypher/executor.rs index 74d3c2fc8..14b086f9f 100644 --- a/crates/rvlite/src/cypher/executor.rs +++ b/crates/rvlite/src/cypher/executor.rs @@ -92,6 +92,75 @@ impl ExecutionResult { pub fn add_row(&mut self, row: HashMap) { self.rows.push(row); } + + /// Convert to a simplified JSON-friendly format where ContextValue enums + /// are flattened to plain serde_json::Value for proper WASM serialization. + pub fn to_json_rows(&self) -> Vec> { + self.rows + .iter() + .map(|row| { + row.iter() + .map(|(k, v)| (k.clone(), v.to_json_value())) + .collect() + }) + .collect() + } +} + +impl ContextValue { + /// Convert to a plain serde_json::Value, stripping the enum wrapper. + pub fn to_json_value(&self) -> serde_json::Value { + match self { + ContextValue::Value(v) => value_to_json(v), + ContextValue::Node(n) => { + let mut map = serde_json::Map::new(); + map.insert("_id".to_string(), serde_json::json!(n.id)); + map.insert("_labels".to_string(), serde_json::json!(n.labels)); + for (k, v) in &n.properties { + map.insert(k.clone(), value_to_json(v)); + } + serde_json::Value::Object(map) + } + ContextValue::Edge(e) => { + let mut map = serde_json::Map::new(); + map.insert("_type".to_string(), serde_json::Value::String(e.edge_type.clone())); + map.insert("_from".to_string(), serde_json::json!(e.from)); + map.insert("_to".to_string(), serde_json::json!(e.to)); + for (k, v) in &e.properties { + map.insert(k.clone(), value_to_json(v)); + } + serde_json::Value::Object(map) + } + ContextValue::List(items) => { + serde_json::Value::Array(items.iter().map(|i| i.to_json_value()).collect()) + } + ContextValue::Map(m) => { + let obj: serde_json::Map = m + .iter() + .map(|(k, v)| (k.clone(), v.to_json_value())) + .collect(); + serde_json::Value::Object(obj) + } + } + } +} + +fn value_to_json(v: &Value) -> serde_json::Value { + match v { + Value::String(s) => serde_json::Value::String(s.clone()), + Value::Integer(i) => serde_json::json!(*i), + Value::Float(f) => serde_json::json!(*f), + Value::Boolean(b) => serde_json::Value::Bool(*b), + Value::Null => serde_json::Value::Null, + Value::List(items) => serde_json::Value::Array(items.iter().map(value_to_json).collect()), + Value::Map(m) => { + let obj: serde_json::Map = m + .iter() + .map(|(k, v)| (k.clone(), value_to_json(v))) + .collect(); + serde_json::Value::Object(obj) + } + } } /// Cypher query executor @@ -106,11 +175,12 @@ impl<'a> Executor<'a> { /// Execute a parsed Cypher query pub fn execute(&mut self, query: &Query) -> Result { - let mut context = ExecutionContext::new(); + // Track multiple row contexts for MATCH results + let mut contexts: Vec = vec![ExecutionContext::new()]; let mut result = None; for statement in &query.statements { - result = Some(self.execute_statement(statement, &mut context)?); + result = Some(self.execute_statement(statement, &mut contexts)?); } result.ok_or_else(|| ExecutionError::ExecutionError("No statements to execute".to_string())) @@ -119,14 +189,32 @@ impl<'a> Executor<'a> { fn execute_statement( &mut self, statement: &Statement, - context: &mut ExecutionContext, + contexts: &mut Vec, ) -> Result { match statement { - Statement::Create(clause) => self.execute_create(clause, context), - Statement::Match(clause) => self.execute_match(clause, context), - Statement::Return(clause) => self.execute_return(clause, context), - Statement::Set(clause) => self.execute_set(clause, context), - Statement::Delete(clause) => self.execute_delete(clause, context), + Statement::Create(clause) => { + // CREATE uses the first context for variable bindings + let ctx = contexts.first_mut().ok_or_else(|| { + ExecutionError::ExecutionError("No execution context".to_string()) + })?; + self.execute_create(clause, ctx) + } + Statement::Match(clause) => self.execute_match(clause, contexts), + Statement::Return(clause) => self.execute_return(clause, contexts), + Statement::Set(clause) => { + // SET applies to each context row + for ctx in contexts.iter() { + self.execute_set(clause, ctx)?; + } + Ok(ExecutionResult::new(vec![])) + } + Statement::Delete(clause) => { + // DELETE applies to each context row + for ctx in contexts.iter() { + self.execute_delete(clause, ctx)?; + } + Ok(ExecutionResult::new(vec![])) + } _ => Err(ExecutionError::UnsupportedOperation(format!( "Statement {:?} not yet implemented", statement @@ -250,7 +338,7 @@ impl<'a> Executor<'a> { fn execute_match( &mut self, clause: &MatchClause, - context: &mut ExecutionContext, + contexts: &mut Vec, ) -> Result { let mut matches = Vec::new(); @@ -267,12 +355,8 @@ impl<'a> Executor<'a> { }); } - // Merge matches into context - for match_ctx in matches { - for (var, val) in match_ctx.variables { - context.bind(var, val); - } - } + // Replace contexts with matched rows (one context per match) + *contexts = matches; Ok(ExecutionResult::new(vec![])) } @@ -417,32 +501,58 @@ impl<'a> Executor<'a> { fn execute_return( &self, clause: &ReturnClause, - context: &ExecutionContext, + contexts: &Vec, ) -> Result { - let mut columns = Vec::new(); - let mut row = HashMap::new(); + // Build column names from expressions + let columns: Vec = clause + .items + .iter() + .map(|item| { + item.alias + .clone() + .unwrap_or_else(|| Self::expression_to_column_name(&item.expression)) + }) + .collect(); + + let mut result = ExecutionResult::new(columns.clone()); + + // Produce one row per context + for context in contexts { + let mut row = HashMap::new(); + + for (item, col_name) in clause.items.iter().zip(columns.iter()) { + let value = self.evaluate_expression_ctx(&item.expression, context)?; + row.insert(col_name.clone(), value); + } - for item in &clause.items { - let col_name = item - .alias - .clone() - .unwrap_or_else(|| match &item.expression { - Expression::Variable(var) => var.clone(), - _ => "?column?".to_string(), - }); - - columns.push(col_name.clone()); - - let value = self.evaluate_expression_ctx(&item.expression, context)?; - row.insert(col_name, value); + result.add_row(row); } - let mut result = ExecutionResult::new(columns); - result.add_row(row); - Ok(result) } + /// Derive a human-readable column name from an expression (e.g., "n.name") + fn expression_to_column_name(expr: &Expression) -> String { + match expr { + Expression::Variable(var) => var.clone(), + Expression::Property { object, property } => { + let obj_name = Self::expression_to_column_name(object); + format!("{}.{}", obj_name, property) + } + Expression::FunctionCall { name, args } => { + let arg_names: Vec = + args.iter().map(Self::expression_to_column_name).collect(); + format!("{}({})", name, arg_names.join(", ")) + } + Expression::Integer(n) => n.to_string(), + Expression::Float(f) => f.to_string(), + Expression::String(s) => format!("'{}'", s), + Expression::Boolean(b) => b.to_string(), + Expression::Null => "NULL".to_string(), + _ => "?column?".to_string(), + } + } + fn execute_set( &mut self, clause: &SetClause, diff --git a/crates/rvlite/src/cypher/mod.rs b/crates/rvlite/src/cypher/mod.rs index 3208dc50c..9c27df620 100644 --- a/crates/rvlite/src/cypher/mod.rs +++ b/crates/rvlite/src/cypher/mod.rs @@ -59,9 +59,15 @@ impl CypherEngine { .execute(&ast) .map_err(|e| JsValue::from_str(&format!("Execution error: {}", e)))?; - // Convert to JS value - serde_wasm_bindgen::to_value(&result) - .map_err(|e| JsValue::from_str(&format!("Serialization error: {}", e))) + // Convert to JS value using flattened JSON rows to avoid enum wrapper issues + let json_result = serde_json::json!({ + "columns": result.columns, + "rows": result.to_json_rows() + }); + let json_str = serde_json::to_string(&json_result) + .map_err(|e| JsValue::from_str(&format!("Serialization error: {}", e)))?; + js_sys::JSON::parse(&json_str) + .map_err(|e| JsValue::from_str(&format!("JSON parse error: {:?}", e))) } /// Get graph statistics diff --git a/crates/rvlite/tests/cypher_integration_test.rs b/crates/rvlite/tests/cypher_integration_test.rs index 3c25e68d4..eec211c2a 100644 --- a/crates/rvlite/tests/cypher_integration_test.rs +++ b/crates/rvlite/tests/cypher_integration_test.rs @@ -243,3 +243,136 @@ fn test_expression_evaluation() { assert_eq!(node.get_property("age").unwrap().as_i64(), Some(30)); assert_eq!(node.get_property("active").unwrap().as_bool(), Some(true)); } + +#[test] +fn test_match_return_multiple_rows() { + let mut graph = PropertyGraph::new(); + + // Create three Person nodes + let create = + "CREATE (a:Person {name: 'Alice', age: 30}), (b:Person {name: 'Bob', age: 25}), (c:Person {name: 'Charlie', age: 35})"; + let ast = parse_cypher(create).expect("Failed to parse CREATE"); + let mut executor = Executor::new(&mut graph); + executor.execute(&ast).expect("Failed to execute CREATE"); + + assert_eq!(graph.stats().node_count, 3); + + // MATCH all persons and RETURN their properties + let match_query = "MATCH (n:Person) RETURN n.name, n.age"; + let ast = parse_cypher(match_query).expect("Failed to parse MATCH RETURN"); + let mut executor = Executor::new(&mut graph); + let result = executor.execute(&ast).expect("MATCH RETURN failed"); + + // Should have 3 rows (one per Person node) + assert_eq!( + result.rows.len(), + 3, + "Expected 3 rows but got {}. Rows: {:?}", + result.rows.len(), + result.rows + ); + + // Column names should be "n.name" and "n.age" (not "?column?") + assert_eq!(result.columns.len(), 2); + assert!( + result.columns.contains(&"n.name".to_string()), + "Expected column 'n.name', got {:?}", + result.columns + ); + assert!( + result.columns.contains(&"n.age".to_string()), + "Expected column 'n.age', got {:?}", + result.columns + ); + + // Collect all names from the result rows + let mut names: Vec = result + .rows + .iter() + .filter_map(|row| { + row.get("n.name").and_then(|v| { + if let ContextValue::Value(Value::String(s)) = v { + Some(s.clone()) + } else { + None + } + }) + }) + .collect(); + names.sort(); + + assert_eq!(names, vec!["Alice", "Bob", "Charlie"]); +} + +#[test] +fn test_match_where_return_multiple_rows() { + let mut graph = PropertyGraph::new(); + + // Create nodes with varying ages + let create = + "CREATE (a:Person {name: 'Alice', age: 30}), (b:Person {name: 'Bob', age: 17}), (c:Person {name: 'Charlie', age: 25})"; + let ast = parse_cypher(create).expect("Failed to parse CREATE"); + let mut executor = Executor::new(&mut graph); + executor.execute(&ast).expect("Failed to execute CREATE"); + + // MATCH with WHERE filter + let match_query = "MATCH (n:Person) WHERE n.age > 18 RETURN n.name"; + let ast = parse_cypher(match_query).expect("Failed to parse MATCH WHERE RETURN"); + let mut executor = Executor::new(&mut graph); + let result = executor.execute(&ast).expect("MATCH WHERE RETURN failed"); + + // Bob (age 17) should be filtered out + assert_eq!( + result.rows.len(), + 2, + "Expected 2 rows (adults only) but got {}", + result.rows.len() + ); + + let mut names: Vec = result + .rows + .iter() + .filter_map(|row| { + row.get("n.name").and_then(|v| { + if let ContextValue::Value(Value::String(s)) = v { + Some(s.clone()) + } else { + None + } + }) + }) + .collect(); + names.sort(); + + assert_eq!(names, vec!["Alice", "Charlie"]); +} + +#[test] +fn test_match_return_variable_node() { + let mut graph = PropertyGraph::new(); + + // Create two nodes + let create = "CREATE (a:Person {name: 'Alice'}), (b:Person {name: 'Bob'})"; + let ast = parse_cypher(create).expect("Failed to parse CREATE"); + let mut executor = Executor::new(&mut graph); + executor.execute(&ast).expect("Failed to execute CREATE"); + + // RETURN the whole node variable + let match_query = "MATCH (n:Person) RETURN n"; + let ast = parse_cypher(match_query).expect("Failed to parse MATCH RETURN n"); + let mut executor = Executor::new(&mut graph); + let result = executor.execute(&ast).expect("MATCH RETURN n failed"); + + assert_eq!(result.rows.len(), 2, "Expected 2 rows for RETURN n"); + assert_eq!(result.columns, vec!["n".to_string()]); + + // Each row should contain a Node + for row in &result.rows { + let val = row.get("n").expect("Missing 'n' column"); + assert!( + matches!(val, ContextValue::Node(_)), + "Expected Node but got {:?}", + val + ); + } +}