Quellcode durchsuchen

fix(orm): resolve friendly names recursively in expression trees

When using friendly names like "id" in WHERE clauses with joins, the
generated SQL was ambiguous because friendly names were only resolved
at the top level of expressions. This caused issues when multiple tables
had columns with the same name.

Add recursive resolve_friendly_names method that walks the entire
expression tree (BinaryExpression, PropertyExpression, etc.) and
replaces VariableExpressions matching friendly names with their
underlying qualified expressions.

For example, .where(expr("id == $0", 1)) now correctly generates
"WHERE val_1_User.id = ?" instead of the ambiguous "WHERE id = ?".
Billy Barrow vor 1 Monat
Ursprung
Commit
5260173c5a
2 geänderte Dateien mit 333 neuen und 9 gelöschten Zeilen
  1. 183 6
      src/orm/projections/projection-sql-builder.vala
  2. 150 3
      src/tests/projection-test.vala

+ 183 - 6
src/orm/projections/projection-sql-builder.vala

@@ -319,10 +319,14 @@ namespace InvercargillSql.Orm.Projections {
          * Translates an Expression object to SQL.
          *
          * This method handles:
+         * - Friendly name resolution (e.g., "user_id" -> the underlying expression like "u.id")
          * - Variable substitution (e.g., "u.id" -> "val_1_User.id")
-         * - Friendly name resolution (e.g., "user_id" -> "val_1_User.id")
          * - SQL operator conversion (e.g., "==" -> "=")
          *
+         * Friendly names are resolved recursively throughout the expression tree,
+         * so expressions like "id == $0" where "id" is a friendly name mapping to
+         * "u.id" will be properly qualified.
+         *
          * @param expression The Expression object to translate
          * @return The translated SQL expression
          */
@@ -330,22 +334,195 @@ namespace InvercargillSql.Orm.Projections {
             // Ensure aliases are assigned
             assign_aliases();
             
-            // Check for simple friendly name (a single VariableExpression)
-            Expression? expr_to_translate = expression;
+            // First, resolve all friendly names in the expression tree
+            Expression resolved = resolve_friendly_names(expression);
+            
+            // Use the variable translator to translate the expression
+            return _translator.translate_expression(resolved);
+        }
+        
+        /**
+         * Recursively resolves friendly names in an expression tree.
+         *
+         * This method walks the expression tree and replaces any VariableExpression
+         * that matches a registered friendly name with its underlying expression.
+         *
+         * For example, if "id" is a friendly name mapping to "u.id", the expression
+         * "id == $0" becomes "u.id == $0".
+         *
+         * @param expression The expression to process
+         * @return A new expression with friendly names resolved
+         */
+        private Expression resolve_friendly_names(Expression expression) {
+            // Handle VariableExpression - check if it's a friendly name
             if (expression is VariableExpression) {
                 var var_expr = (VariableExpression) expression;
                 string var_name = var_expr.variable_name;
+                
                 // Check if it's a friendly name (no dots and registered)
                 if (!var_name.contains(".") && _resolver.is_friendly_name(var_name)) {
                     Expression? resolved = _resolver.resolve_to_expression(var_name);
                     if (resolved != null) {
-                        expr_to_translate = resolved;
+                        return resolved;
                     }
                 }
+                return expression;
             }
             
-            // Use the variable translator to translate the expression
-            return _translator.translate_expression(expr_to_translate);
+            // Handle PropertyExpression - recurse on target
+            if (expression is PropertyExpression) {
+                var prop_expr = (PropertyExpression) expression;
+                Expression resolved_target = resolve_friendly_names(prop_expr.target);
+                
+                // If target changed, create new PropertyExpression
+                if (resolved_target != prop_expr.target) {
+                    return new PropertyExpression(resolved_target, prop_expr.property_name);
+                }
+                return expression;
+            }
+            
+            // Handle BinaryExpression - recurse on both sides
+            if (expression is BinaryExpression) {
+                var binary = (BinaryExpression) expression;
+                Expression resolved_left = resolve_friendly_names(binary.left);
+                Expression resolved_right = resolve_friendly_names(binary.right);
+                
+                // If either side changed, create new BinaryExpression
+                if (resolved_left != binary.left || resolved_right != binary.right) {
+                    return new BinaryExpression(resolved_left, resolved_right, binary.op);
+                }
+                return expression;
+            }
+            
+            // Handle UnaryExpression - recurse on operand
+            if (expression is UnaryExpression) {
+                var unary = (UnaryExpression) expression;
+                Expression resolved_operand = resolve_friendly_names(unary.operand);
+                
+                if (resolved_operand != unary.operand) {
+                    return new UnaryExpression(unary.operator, resolved_operand);
+                }
+                return expression;
+            }
+            
+            // Handle TernaryExpression - recurse on all three parts
+            if (expression is TernaryExpression) {
+                var ternary = (TernaryExpression) expression;
+                Expression resolved_condition = resolve_friendly_names(ternary.condition);
+                Expression resolved_true = resolve_friendly_names(ternary.true_expression);
+                Expression resolved_false = resolve_friendly_names(ternary.false_expression);
+                
+                if (resolved_condition != ternary.condition ||
+                    resolved_true != ternary.true_expression ||
+                    resolved_false != ternary.false_expression) {
+                    return new TernaryExpression(
+                        resolved_condition, resolved_true, resolved_false
+                    );
+                }
+                return expression;
+            }
+            
+            // Handle BracketedExpression - recurse on inner
+            if (expression is BracketedExpression) {
+                var bracketed = (BracketedExpression) expression;
+                Expression resolved_inner = resolve_friendly_names(bracketed.inner);
+                
+                if (resolved_inner != bracketed.inner) {
+                    return new BracketedExpression(resolved_inner);
+                }
+                return expression;
+            }
+            
+            // Handle LambdaExpression - recurse on body
+            if (expression is LambdaExpression) {
+                var lambda = (LambdaExpression) expression;
+                Expression resolved_body = resolve_friendly_names(lambda.body);
+                
+                if (resolved_body != lambda.body) {
+                    return new LambdaExpression(lambda.parameter_name, resolved_body);
+                }
+                return expression;
+            }
+            
+            // Handle FunctionCallExpression - recurse on target and arguments
+            if (expression is FunctionCallExpression) {
+                var func = (FunctionCallExpression) expression;
+                Expression? resolved_target = null;
+                if (func.target != null) {
+                    resolved_target = resolve_friendly_names(func.target);
+                }
+                
+                Series<Expression>? resolved_args = null;
+                bool args_changed = false;
+                if (func.arguments != null) {
+                    resolved_args = new Series<Expression>();
+                    foreach (var arg in func.arguments) {
+                        Expression resolved_arg = resolve_friendly_names(arg);
+                        resolved_args.add_start(resolved_arg);
+                        if (resolved_arg != arg) {
+                            args_changed = true;
+                        }
+                    }
+                }
+                
+                if ((resolved_target != null && resolved_target != func.target) || args_changed) {
+                    return new FunctionCallExpression(
+                        resolved_target ?? func.target,
+                        func.function_name,
+                        resolved_args ?? func.arguments
+                    );
+                }
+                return expression;
+            }
+            
+            // Handle GlobalFunctionCallExpression - recurse on arguments
+            if (expression is GlobalFunctionCallExpression) {
+                var global_func = (GlobalFunctionCallExpression) expression;
+                
+                Series<Expression>? resolved_args = null;
+                bool args_changed = false;
+                if (global_func.arguments != null) {
+                    resolved_args = new Series<Expression>();
+                    foreach (var arg in global_func.arguments) {
+                        Expression resolved_arg = resolve_friendly_names(arg);
+                        resolved_args.add_start(resolved_arg);
+                        if (resolved_arg != arg) {
+                            args_changed = true;
+                        }
+                    }
+                }
+                
+                if (args_changed) {
+                    return new GlobalFunctionCallExpression(
+                        global_func.function_name,
+                        resolved_args
+                    );
+                }
+                return expression;
+            }
+            
+            // Handle LotLiteralExpression - recurse on elements
+            if (expression is LotLiteralExpression) {
+                var lot = (LotLiteralExpression) expression;
+                
+                Vector<Expression> resolved_elements = new Vector<Expression>();
+                bool elements_changed = false;
+                foreach (var elem in lot.elements) {
+                    Expression resolved_elem = resolve_friendly_names(elem);
+                    resolved_elements.add(resolved_elem);
+                    if (resolved_elem != elem) {
+                        elements_changed = true;
+                    }
+                }
+                
+                if (elements_changed) {
+                    return new LotLiteralExpression(resolved_elements.to_array());
+                }
+                return expression;
+            }
+            
+            // LiteralExpression, ParameterExpression - no children, return as-is
+            return expression;
         }
         
         /**

+ 150 - 3
src/tests/projection-test.vala

@@ -335,6 +335,11 @@ public int main(string[] args) {
         test_first_without_collection_selections();
         //  test_first_async_with_collection_selections();
         
+        // Friendly Name Resolution Tests
+        print("\n--- Friendly Name Resolution Tests ---\n");
+        test_friendly_name_resolution_in_where_with_ambiguous_columns();
+        test_friendly_name_resolution_in_complex_where();
+        
         print("\n=== All Projection tests passed! ===\n");
         return 0;
     } catch (Error e) {
@@ -2072,7 +2077,7 @@ void test_first_without_collection_selections() throws Error {
 //  void test_first_async_with_collection_selections() throws Error {
 //      var loop = new MainLoop();
 //      Error? error = null;
-    
+//     
 //      test_first_async_with_collection_selections_async.begin((obj, res) => {
 //          try {
 //              test_first_async_with_collection_selections_async.end(res);
@@ -2081,10 +2086,152 @@ void test_first_without_collection_selections() throws Error {
 //          }
 //          loop.quit();
 //      });
-    
+//     
 //      loop.run();
-    
+//     
 //      if (error != null) {
 //          throw error;
 //      }
 //  }
+
+// ========================================
+// Friendly Name Resolution in WHERE Clause Tests
+// ========================================
+
+/**
+ * Projection with ambiguous column names for friendly name resolution tests.
+ * 
+ * This projection maps "id" from u.id, but also has a join table with an "id" column.
+ * When querying with .where(expr("id == $0", ...)), the "id" should resolve to the
+ * underlying expression u.id and get properly qualified in the SQL.
+ */
+public class UserWithAmbiguousIdProjection : Object {
+    public int64 id { get; set; }
+    public string user_name { get; set; }
+    public int64 order_id { get; set; }
+    
+    public UserWithAmbiguousIdProjection() {
+        user_name = "";
+    }
+}
+
+/**
+ * Test: Friendly name resolution in WHERE clause with ambiguous column names.
+ *
+ * This test verifies that when a projection has a friendly name "id" that maps
+ * to a source table column (u.id), and there's a join table that also has an "id"
+ * column (o.id), the friendly name in a WHERE clause is properly resolved to the
+ * qualified column name.
+ *
+ * The bug was: .where(expr("id == $0", ...)) generates "WHERE id = ?" which is
+ * ambiguous because both "u.id" and "o.id" exist in the query.
+ *
+ * Expected: The WHERE clause should be "WHERE val_1_User.id = ?" after resolution.
+ */
+void test_friendly_name_resolution_in_where_with_ambiguous_columns() throws Error {
+    print("Test: Friendly name resolution in WHERE with ambiguous columns... ");
+    var ctx = setup_integration_context();
+    
+    // Register a projection where "id" is mapped from u.id, and there's a join with o.id
+    ctx.registry.register_projection<UserWithAmbiguousIdProjection>(
+        new ProjectionBuilder<UserWithAmbiguousIdProjection>(ctx.registry)
+            .source<ProjTestUser>("u")
+            .join<ProjTestOrder>("o", expr("u.id == o.user_id"))
+            .select<int64?>("id", expr("u.id"), (x, v) => x.id = v)
+            .select<string>("user_name", expr("u.name"), (x, v) => x.user_name = v)
+            .select<int64?>("order_id", expr("o.id"), (x, v) => x.order_id = v)
+            .build()
+    );
+    
+    // Query using the friendly name "id" in the WHERE clause
+    // This should resolve to u.id and generate qualified SQL
+    var query = ctx.session.query<UserWithAmbiguousIdProjection>()
+        .where(expr("id == $0", new NativeElement<int64?>(1)));
+    
+    string sql = query.to_sql();
+    print("\n  Generated SQL: %s\n", sql);
+    
+    // The SQL should NOT contain an unqualified "id" in the WHERE clause
+    // It should have the qualified form like "val_1_ProjTestUser.id"
+    assert("WHERE" in sql.up());
+    
+    // Check that "id" in the WHERE clause is qualified with the table alias
+    // The pattern should be "val_1_ProjTestUser.id = 1" not "id = 1"
+    assert("val_1_ProjTestUser.id" in sql || "val_1_User.id" in sql);
+    
+    // Ensure there's no unqualified "id = " (which would cause ambiguity)
+    // We check for "id = 1" without a table prefix by looking for the pattern
+    // that would appear if it wasn't qualified
+    int where_pos = sql.index_of("WHERE");
+    assert(where_pos >= 0);
+    string where_clause = sql.substring(where_pos);
+    
+    // The where clause should not have an unqualified "id" reference
+    // Unqualified would be like "id = 1" or "id == 1"
+    // Qualified would be like "val_1_ProjTestUser.id = 1"
+    assert(!(" id =" in where_clause) && !("(id =" in where_clause));
+    assert(!(" id ==" in where_clause) && !("(id ==" in where_clause));
+    
+    // Execute the query to verify it works
+    var results = query.materialise();
+    
+    // Alice (id=1) should have 2 orders, so we get 2 rows
+    assert(results.length == 2);
+    
+    foreach (var result in results) {
+        assert(result.id == 1);
+        assert(result.user_name == "Alice");
+    }
+    
+    print("PASSED\n");
+}
+
+/**
+ * Test: Friendly name resolution works in complex expressions.
+ *
+ * This test verifies that friendly names are resolved even in complex
+ * expressions like "id > $0 && user_name == $1".
+ */
+void test_friendly_name_resolution_in_complex_where() throws Error {
+    print("Test: Friendly name resolution in complex WHERE... ");
+    var ctx = setup_integration_context();
+    
+    ctx.registry.register_projection<UserWithAmbiguousIdProjection>(
+        new ProjectionBuilder<UserWithAmbiguousIdProjection>(ctx.registry)
+            .source<ProjTestUser>("u")
+            .join<ProjTestOrder>("o", expr("u.id == o.user_id"))
+            .select<int64?>("id", expr("u.id"), (x, v) => x.id = v)
+            .select<string>("user_name", expr("u.name"), (x, v) => x.user_name = v)
+            .select<int64?>("order_id", expr("o.id"), (x, v) => x.order_id = v)
+            .build()
+    );
+    
+    // Query using multiple friendly names in a complex expression
+    var query = ctx.session.query<UserWithAmbiguousIdProjection>()
+        .where(expr("id > $0 && user_name == $1", 
+            new NativeElement<int64?>(1),
+            new NativeElement<string?>("Charlie")));
+    
+    string sql = query.to_sql();
+    print("\n  Generated SQL: %s\n", sql);
+    
+    // Both "id" and "user_name" should be properly qualified
+    assert("WHERE" in sql.up());
+    
+    // The id reference should be qualified (not ambiguous)
+    assert("val_1_ProjTestUser.id" in sql || "val_1_User.id" in sql);
+    
+    // user_name should also be qualified
+    assert("val_1_ProjTestUser.name" in sql || "val_1_User.name" in sql);
+    
+    // Execute the query - should only get Charlie's orders
+    var results = query.materialise();
+    assert(results.length == 3);  // Charlie has 3 orders
+    
+    foreach (var result in results) {
+        assert(result.id == 3);
+        assert(result.user_name == "Charlie");
+    }
+    
+    print("PASSED\n");
+}