Start iteration 10

plans/phase-10-unified-expression-api.md

@@ -0,0 +1,449 @@
+# Phase 10: Unified Expression API Refactor
+
+## Overview
+
+This phase addresses the inconsistent expression handling throughout the ORM layer. Currently, the codebase uses raw strings for expressions in many places instead of `Invercargill.Expressions.Expression` objects.
+
+This violates the original design intent where **all expressions should be `Invercargill.Expressions.Expression` objects**.
+
+## Problem Statement
+
+### Current Issues
+
+1. **Dual API Surface**: The `where()` and `where_expr()` methods create confusion about which to use
+2. **String-based Internal Storage**: `_where_clauses` stores raw strings instead of Expression objects
+3. **ProjectionQuery Limitation**: `ProjectionQuery.where_expr()` throws an error, making Expression-based filtering impossible for projections
+4. **Design Violation**: The Query class and Projection system have created their own ad-hoc expression language instead of using `Invercargill.Expressions`
+5. **Widespread String Usage**: Expressions are stored and passed as strings throughout the projection system
+
+### Affected Areas
+
+| Area | Files | String Expression Usage |
+|------|-------|------------------------|
+| Query API | `query.vala`, `entity-query.vala`, `projection-query.vala` | `where()`, `or_where()`, `order_by()`, `order_by_desc()` |
+| Projection Builder | `projection-builder.vala` | `join()`, `select()`, `select_nested()`, `select_many()`, `group_by()` |
+| Selection Types | `selection-types.vala` | `ScalarSelection.expression`, `NestedProjectionSelection.entry_point_expression`, `CollectionProjectionSelection.entry_point_expression` |
+| Projection Definition | `projection-definition.vala` | `JoinDefinition.join_condition`, `group_by_expressions` |
+| SQL Builder | `projection-sql-builder.vala` | `build()`, `build_with_split()`, `translate_expression()` |
+| Variable Translator | `variable-translator.vala` | `translate_expression()` |
+| Aggregate Analyzer | `aggregate-analyzer.vala` | `analyze()`, `contains_aggregate()`, `split_expression()` |
+| Friendly Name Resolver | `friendly-name-resolver.vala` | `resolve_to_expression()` returns string |
+
+### Current Code Structure
+
+```
+Query<T>
+├── where(string expression)         → stores string in _where_clauses
+├── or_where(string expression)      → stores string in _where_clauses  
+├── where_expr(Expression expression) → abstract, implemented differently
+│
+├── EntityQuery<T>
+│   ├── where(string) → parses to Expression, stores in _filter
+│   └── where_expr(Expression) → stores in _filter
+│
+└── ProjectionQuery<T>
+    ├── where(string) → stores string, passes to ProjectionSqlBuilder
+    └── where_expr(Expression) → THROWS ERROR!
+```
+
+## Proposed Solution
+
+### New API Design
+
+All `string expression` parameters will be converted to `Expression expression` parameters. The `expr()` convenience function from `Invercargill.Expressions` will be used to create expressions ergonomically.
+
+```
+Query<T>
+├── where(Expression expression)      → stores Expression in _where_expressions
+├── or_where(Expression expression)   → stores Expression in _where_expressions
+├── order_by(Expression expression)   → stores Expression for ordering
+├── order_by_desc(Expression expression) → stores Expression for ordering DESC
+│
+├── EntityQuery<T>
+│   └── where(Expression) → combines with existing _filter using AND/OR
+│
+└── ProjectionQuery<T>
+    └── where(Expression) → translates Expression through VariableTranslator
+```
+
+### The `expr()` Convenience Function
+
+From `Invercargill.Expressions`:
+```vala
+public Expression expr(string expression, Element first, ...) throws ExpressionError {
+    return ExpressionParser.parse_with_params(expression, Wrap.va_list<Element>(first, va_list()));
+}
+```
+
+Usage examples:
+```vala
+// Simple expression
+var results = session.query<User>()
+    .where(expr("age > 25", new NativeElement<int?>(25)))
+    .materialise();
+
+// With parameters using $0, $1 syntax
+var results = session.query<User>()
+    .where(expr("age > $0 && name == $1", 
+        new NativeElement<int?>(25),
+        new NativeElement<string>("Alice")))
+    .materialise();
+
+// Reusable expressions
+var active_filter = expr("is_active == true", new NativeElement<bool>(true));
+var results = session.query<User>()
+    .where(active_filter)
+    .materialise();
+```
+
+## Implementation Plan
+
+### Step 1: Update Query<T> Base Class
+
+**File**: [`src/orm/query.vala`](src/orm/query.vala)
+
+Changes:
+1. Change `_where_clauses: Vector<string>` to `_where_expressions: Vector<Expression>`
+2. Change `where(string expression)` to `where(Expression expression)`
+3. Change `or_where(string expression)` to `or_where(Expression expression)`
+4. Change `order_by(string expression)` to `order_by(Expression expression)`
+5. Change `order_by_desc(string expression)` to `order_by_desc(Expression expression)`
+6. Remove abstract `where_expr(Expression expression)` method
+7. Update `get_combined_where()` to work with Expressions
+
+```vala
+// Before
+protected Vector<string> _where_clauses;
+public virtual Query<T> where(string expression) { ... }
+public abstract Query<T> where_expr(Expression expression);
+
+// After
+protected Vector<Expression> _where_expressions;
+public virtual Query<T> where(Expression expression) { ... }
+// where_expr() removed
+```
+
+### Step 2: Update OrderByClause
+
+**File**: [`src/orm/query.vala`](src/orm/query.vala)
+
+Changes:
+1. Change `expression: string` to `expression: Expression`
+
+```vala
+// Before
+public class OrderByClause : Object {
+    public string expression { get; construct; }
+    public OrderByClause(string expression, bool descending) { ... }
+}
+
+// After
+public class OrderByClause : Object {
+    public Expression expression { get; construct; }
+    public OrderByClause(Expression expression, bool descending) { ... }
+}
+```
+
+### Step 3: Update EntityQuery<T>
+
+**File**: [`src/orm/entity-query.vala`](src/orm/entity-query.vala)
+
+Changes:
+1. Remove `where(string expression)` override - base implementation now handles Expressions
+2. Remove `where_expr(Expression expression)` override
+3. Update `build_select_sql()` to handle Expression-based WHERE clauses
+4. Combine multiple WHERE expressions using AND/OR
+
+### Step 4: Update ProjectionQuery<T>
+
+**File**: [`src/orm/projections/projection-query.vala`](src/orm/projections/projection-query.vala)
+
+Changes:
+1. Remove `where_expr()` that throws error
+2. Update `materialise()` and `materialise_async()` to translate Expressions
+3. Use `VariableTranslator` to convert Expressions for projection context
+
+### Step 5: Update ProjectionBuilder
+
+**File**: [`src/orm/projections/projection-builder.vala`](src/orm/projections/projection-builder.vala)
+
+Changes:
+1. `join<TEntity>(string variable_name, string join_condition)` → `join<TEntity>(string variable_name, Expression join_condition)`
+2. `select<TValue>(string friendly_name, string expression, ...)` → `select<TValue>(string friendly_name, Expression expression, ...)`
+3. `select_nested<TNested>(string friendly_name, string entry_point_expression, ...)` → `select_nested<TNested>(string friendly_name, Expression entry_point_expression, ...)`
+4. `select_many<TItem>(string friendly_name, string entry_point_expression, ...)` → `select_many<TItem>(string friendly_name, Expression entry_point_expression, ...)`
+5. `group_by(params string[] expressions)` → `group_by(params Expression[] expressions)`
+
+### Step 6: Update Selection Types
+
+**File**: [`src/orm/projections/selection-types.vala`](src/orm/projections/selection-types.vala)
+
+Changes:
+1. `ScalarSelection.expression: string` → `Expression`
+2. `NestedProjectionSelection.entry_point_expression: string` → `Expression`
+3. `CollectionProjectionSelection.entry_point_expression: string` → `Expression`
+
+### Step 7: Update ProjectionDefinition and JoinDefinition
+
+**File**: [`src/orm/projections/projection-definition.vala`](src/orm/projections/projection-definition.vala)
+
+Changes:
+1. `JoinDefinition.join_condition: string` → `Expression`
+2. `ProjectionDefinition.group_by_expressions: Vector<string>` → `Vector<Expression>`
+3. `add_group_by(string expression)` → `add_group_by(Expression expression)`
+
+### Step 8: Update ProjectionSqlBuilder
+
+**File**: [`src/orm/projections/projection-sql-builder.vala`](src/orm/projections/projection-sql-builder.vala)
+
+Changes:
+1. `build(string? where_expression, ...)` → `build(Expression? where_expression, ...)`
+2. `build_with_split(string? where_expression, ...)` → `build_with_split(Expression? where_expression, ...)`
+3. `translate_expression(string expression)` → `translate_expression(Expression expression)`
+4. Update internal methods to work with Expression objects
+
+### Step 9: Update VariableTranslator
+
+**File**: [`src/orm/projections/variable-translator.vala`](src/orm/projections/variable-translator.vala)
+
+Changes:
+1. `translate_expression(string expression)` → `translate_expression(Expression expression)`
+2. Update to use `ExpressionToSqlVisitor` pattern for translation
+3. May need to accept `Expression` and output SQL directly
+
+### Step 10: Update AggregateAnalyzer
+
+**File**: [`src/orm/projections/aggregate-analyzer.vala`](src/orm/projections/aggregate-analyzer.vala)
+
+Changes:
+1. `analyze(string expression)` → `analyze(Expression expression)`
+2. `contains_aggregate(string expression)` → `contains_aggregate(Expression expression)`
+3. `split_expression(string expression)` → `split_expression(Expression expression)`
+4. Update internal scanning to work with Expression trees instead of strings
+
+### Step 11: Update FriendlyNameResolver
+
+**File**: [`src/orm/projections/friendly-name-resolver.vala`](src/orm/projections/friendly-name-resolver.vala)
+
+Changes:
+1. `resolve_to_expression(string friendly_name)` → Returns `Expression?` instead of `string?`
+2. Update internal caching to store Expression objects
+
+### Step 12: Update Tests
+
+**Files**: 
+- [`src/tests/orm-test.vala`](src/tests/orm-test.vala)
+- [`src/tests/projection-test.vala`](src/tests/projection-test.vala)
+
+Changes:
+1. Import `Invercargill.Expressions` namespace
+2. Convert all `where("string")` calls to `where(expr("string"))`
+3. Convert all `order_by("column")` calls to use Expression-based approach
+4. Convert all projection builder calls to use `expr()`
+5. Add tests for parameterized expressions using `$0`, `$1` syntax
+
+Example test conversions:
+
+```vala
+// Before
+var results = session.query<TestUser>()
+    .where("age > 28")
+    .materialise();
+
+// After
+var results = session.query<TestUser>()
+    .where(expr("age > $0", new NativeElement<int?>(28)))
+    .materialise();
+```
+
+```vala
+// Before - projection builder
+ctx.registry.register_projection<UserOrderStats>(new ProjectionBuilder<UserOrderStats>(ctx.registry)
+    .source<ProjTestUser>("u")
+    .join<ProjTestOrder>("o", "u.id == o.user_id")
+    .group_by("u.id")
+    .select<int64?>("user_id", "u.id", (x, v) => x.user_id = v)
+    .build()
+);
+
+// After - projection builder
+ctx.registry.register_projection<UserOrderStats>(new ProjectionBuilder<UserOrderStats>(ctx.registry)
+    .source<ProjTestUser>("u")
+    .join<ProjTestOrder>("o", expr("u.id == o.user_id"))
+    .group_by(expr("u.id"))
+    .select<int64?>("user_id", expr("u.id"), (x, v) => x.user_id = v)
+    .build()
+);
+```
+
+### Step 13: Update Demo Application
+
+**File**: [`examples/demo.vala`](examples/demo.vala)
+
+Changes:
+1. Add `using Invercargill.Expressions;`
+2. Convert all query expressions to use `expr()`
+
+```vala
+// Before
+var users_over_25 = session.query<User>()
+    .where("age > 25")
+    .materialise();
+
+// After
+var users_over_25 = session.query<User>()
+    .where(expr("age > $0", new NativeElement<int?>(25)))
+    .materialise();
+```
+
+## Detailed File Changes
+
+### Files to Modify
+
+| File | Changes |
+|------|---------|
+| `src/orm/query.vala` | Core API - where(), or_where(), order_by(), OrderByClause |
+| `src/orm/entity-query.vala` | Remove overrides, update SQL building |
+| `src/orm/projections/projection-query.vala` | Enable Expression support |
+| `src/orm/projections/projection-builder.vala` | join(), select(), group_by() signatures |
+| `src/orm/projections/selection-types.vala` | expression property types |
+| `src/orm/projections/projection-definition.vala` | JoinDefinition, group_by_expressions |
+| `src/orm/projections/projection-sql-builder.vala` | build() signatures, translate_expression() |
+| `src/orm/projections/variable-translator.vala` | translate_expression() signature |
+| `src/orm/projections/aggregate-analyzer.vala` | analyze(), contains_aggregate(), split_expression() |
+| `src/orm/projections/friendly-name-resolver.vala` | resolve_to_expression() return type |
+| `src/tests/orm-test.vala` | Convert all tests |
+| `src/tests/projection-test.vala` | Convert all tests |
+| `examples/demo.vala` | Update demo |
+
+### Files to Review (may need changes)
+
+| File | Reason |
+|------|--------|
+| `src/expressions/expression-to-sql-visitor.vala` | Ensure it works with projection contexts |
+
+## Migration Guide for Users
+
+### Before (Phase 9 and earlier)
+```vala
+// String-based expressions
+var users = session.query<User>()
+    .where("age > 18")
+    .order_by("name")
+    .materialise();
+
+// Projection queries
+var stats = session.query<UserOrderStats>()
+    .where("user_id > 100")
+    .materialise();
+
+// Projection definitions
+registry.register_projection<UserStats>(new ProjectionBuilder<UserStats>(registry)
+    .source<User>("u")
+    .join<Order>("o", "u.id == o.user_id")
+    .group_by("u.id")
+    .select<int64?>("user_id", "u.id", (x, v) => x.user_id = v)
+    .build()
+);
+```
+
+### After (Phase 10)
+```vala
+using Invercargill.Expressions;
+
+// Expression-based queries
+var users = session.query<User>()
+    .where(expr("age > $0", new NativeElement<int?>(18)))
+    .order_by(expr("name"))
+    .materialise();
+
+// Projection queries work the same way
+var stats = session.query<UserOrderStats>()
+    .where(expr("user_id > $0", new NativeElement<int?>(100)))
+    .materialise();
+
+// Projection definitions with Expressions
+registry.register_projection<UserStats>(new ProjectionBuilder<UserStats>(registry)
+    .source<User>("u")
+    .join<Order>("o", expr("u.id == o.user_id"))
+    .group_by(expr("u.id"))
+    .select<int64?>("user_id", expr("u.id"), (x, v) => x.user_id = v)
+    .build()
+);
+
+// Simple expressions without parameters
+var active = session.query<User>()
+    .where(expr("is_active == true"))
+    .materialise();
+```
+
+## Benefits
+
+1. **Type Safety**: Expressions are parsed at construction time, catching errors early
+2. **Consistency**: Single API across EntityQuery and ProjectionQuery
+3. **Composability**: Expressions can be stored, combined, and reused
+4. **Parameterized Queries**: Native support for parameterized expressions via `$0`, `$1` syntax
+5. **Design Alignment**: Properly uses `Invercargill.Expressions` throughout
+
+## Risks and Mitigations
+
+### Risk 1: Breaking Change
+- **Impact**: All existing code using string expressions will break
+- **Mitigation**: This is a major version change; provide clear migration guide
+
+### Risk 2: Verbosity
+- **Impact**: `expr("age > $0", new NativeElement<int?>(18))` is more verbose than `"age > 18"`
+- **Mitigation**: Document common patterns; simple expressions can omit parameters
+
+### Risk 3: Projection Expression Translation
+- **Impact**: ProjectionQuery needs to translate expressions differently than EntityQuery
+- **Mitigation**: Leverage existing `VariableTranslator` infrastructure
+
+## Testing Strategy
+
+1. **Unit Tests**: Update all existing tests to use `expr()`
+2. **Expression Parsing Tests**: Verify `expr()` handles all expression types
+3. **Integration Tests**: Ensure EntityQuery and ProjectionQuery produce correct SQL
+4. **Parameter Tests**: Test `$0`, `$1`, etc. parameter substitution
+5. **Error Handling**: Verify proper errors for invalid expressions
+
+## Success Criteria
+
+- [ ] `where_expr()` method removed from all Query classes
+- [ ] `where()` accepts `Expression` parameter in all Query classes
+- [ ] `order_by()` and `order_by_desc()` accept `Expression` parameters
+- [ ] `ProjectionBuilder` methods accept `Expression` parameters
+- [ ] Selection types store `Expression` objects
+- [ ] `ProjectionSqlBuilder` accepts `Expression` for WHERE
+- [ ] All analyzers and translators work with `Expression` objects
+- [ ] ProjectionQuery supports Expression-based filtering
+- [ ] All tests pass with new Expression-based API
+- [ ] Demo application updated and working
+- [ ] No string-based expression storage anywhere in ORM/Projections
+
+## Estimated Scope
+
+| Category | Count |
+|----------|-------|
+| Core files to modify | 10 |
+| Test files to update | 2 |
+| Example files to update | 1 |
+| Breaking changes | Yes (major API change) |
+
+## Dependencies
+
+- `Invercargill.Expressions.expr()` function
+- `Invercargill.Expressions.ExpressionParser`
+- `Invercargill.Expressions.Expression` type hierarchy
+- `Invercargill.NativeElement<T>` for parameter values
+
+## Implementation Order
+
+1. **Core Query Classes** - Foundation for everything else
+2. **Selection Types** - Data structures used by builders
+3. **Projection Builder** - API surface for defining projections
+4. **Projection Definition** - Storage of projection configuration
+5. **SQL Builder** - Translation to SQL
+6. **Analyzers and Translators** - Supporting infrastructure
+7. **Tests** - Verify all changes work correctly
+8. **Demo** - Update example application