fix: Fix savepoint tests, update docs

ocean · ocean · commit 28af0101e084 · 2025-12-04T22:43:15.000+11:00
diff --git a/IMPLEMENTATION_ROADMAP_FOCUSED.md b/IMPLEMENTATION_ROADMAP_FOCUSED.md
@@ -3,7 +3,7 @@
 **Version**: 3.1.0 (Updated with Phase 1 & 2 Completion)
 **Date**: 2025-12-04
 **Current Version**: ecto_libsql v0.6.0 (v0.8.0-rc1 ready)
-**Target Version**: v1.0.0 (May 2026)
+**Target Version**: v1.0.0
 **LibSQL Version**: 0.9.24
 
 ---
@@ -15,10 +15,10 @@ This roadmap is **laser-focused** on delivering **100% of production-critical li
 **Status as of Dec 4, 2025**: 
 - Phase 1: ✅ 100% Complete (3/3 features)
 - Phase 2: ✅ 83% Complete (2.5/3 features)
-- Phase 3: 0% (scheduled for Q1 2026)
-- Phase 4: 0% (scheduled for Q2 2026)
+- Phase 3: 0%
+- Phase 4: 0%
 
-**Estimated Final**: 95%+ feature coverage by v1.0.0 (May 2026)
+**Estimated Final**: 95%+ feature coverage by v1.0.0
 
 ### Focus Areas
 
@@ -158,11 +158,8 @@ end)
 
 **Total Effort**: 8-9 days (2-3 weeks with testing/docs)
 **Impact**: Fixes critical performance issue, enables complex operations, improves DX
-**Release**: v0.7.0 (January 2026)
 
 **Completion Notes**:
-- All 3 Phase 1 features implemented and tested
-- 271 tests passing, 0 failures, 25 skipped
 - No `.unwrap()` panics - all errors handled gracefully
 - Ready to proceed with Phase 2
 
@@ -172,7 +169,6 @@ end)
 
 **Status**: ✅ **IMPLEMENTED** 
 
-**Target Date**: February 2026 (2-3 weeks)
 **Goal**: Full embedded replica monitoring and control
 **Impact**: **HIGH** - Enables production monitoring of replicas
 
@@ -344,7 +340,6 @@ let rows = query_result.into_iter().collect::<Vec<_>>();  // ← Loads EVERYTHIN
 
 ## Phase 3: Enable Advanced Use Cases (v0.9.0)
 
-**Target Date**: March-April 2026 (4-5 weeks)
 **Goal**: Hooks, extensions, custom functions
 **Impact**: **MEDIUM-HIGH** - Enables advanced patterns
 
@@ -521,13 +516,11 @@ end)
 
 **Total Effort**: 15-21 days (4-5 weeks with testing/docs)
 **Impact**: Enables advanced patterns (real-time, multi-tenant, extensions)
-**Release**: v0.9.0 (March-April 2026)
 
 ---
 
 ## Phase 4: Production Polish & v1.0.0
 
-**Target Date**: May 2026 (2 weeks)
 **Goal**: Production-grade polish, comprehensive docs
 **Impact**: **MEDIUM** - Completes feature set
 
@@ -650,15 +643,14 @@ end)
 - [ ] Large dataset processing example
 
 **Estimated Effort**: 5 days
-**Priority**: **HIGH** - Essential for v1.0.0
+**Priority**: **HIGH**
 
 ---
 
 ### Phase 4 Summary
 
 **Total Effort**: 15-19 days (2-3 weeks)
 **Impact**: Completes feature set, production-ready documentation
-**Release**: v1.0.0 (May 2026)
 
 ---
 
@@ -830,7 +822,6 @@ This roadmap focuses on:
 **Document Version**: 3.1.0 (Updated with Phase 1 & 2 Results)
 **Date**: 2025-12-04
 **Last Updated**: 2025-12-04 (Phase 1 & 2 completion)
-**Next Review**: After v0.8.0 release (February 2026)
 **Based On**: LIBSQL_FEATURE_MATRIX_FINAL.md v4.0.0
 
 ---
diff --git a/LIBSQL_FEATURE_MATRIX_FINAL.md b/LIBSQL_FEATURE_MATRIX_FINAL.md
@@ -16,17 +16,17 @@ This analysis is based on **authoritative sources**:
 3. ✅ Current ecto_libsql implementation audit (29 NIFs, 1,509 lines)
 4. ✅ Development guide requirements (`ecto_libsql_development_guide.md`)
 
-**Key Finding**: ecto_libsql implements **54% of libsql features** with **excellent coverage of production-critical features** (100% of P0) but **missing advanced features** needed for specific use cases.
+**Key Finding**: ecto_libsql implements **65% of libsql features** with **excellent coverage of production-critical features** (100% of P0) and **strong support for advanced features** including full transaction control, statement introspection, and replica monitoring.
 
 ### What's Implemented (Strong Foundation)
 
 ✅ **All 3 Connection Modes**: Local, Remote, Embedded Replica
-✅ **Full Transaction Support**: All 4 behaviours (Deferred, Immediate, Exclusive, Read-Only)
+✅ **Full Transaction Support**: All 4 behaviours (Deferred, Immediate, Exclusive, Read-Only) + Savepoints
 ✅ **Comprehensive Metadata**: last_insert_rowid, changes, total_changes, is_autocommit
 ✅ **Production Configuration**: busy_timeout, reset, interrupt, PRAGMA helpers
 ✅ **Batch Operations**: Native and manual, transactional and non-transactional
-✅ **Basic Replication**: Manual sync with timeout, auto-sync for writes
-✅ **Prepared Statements**: Prepare, execute, query (with re-prepare workaround)
+✅ **Advanced Replication**: Manual sync with timeout, auto-sync for writes, frame monitoring, sync_until, flush_replicator
+✅ **Prepared Statements**: Prepare, execute, query (with re-prepare workaround) + introspection (column_count, column_name, parameter_count)
 ✅ **Vector Search**: Helper functions for vector operations
 ✅ **Encryption**: AES-256 at rest
 
@@ -36,9 +36,6 @@ This analysis is based on **authoritative sources**:
 ❌ **No Custom Functions**: create_scalar_function, create_aggregate_function
 ❌ **No Extensions**: load_extension (FTS5, R-Tree, etc.)
 ❌ **Limited Streaming**: Cursors load all rows upfront (memory issue)
-❌ **No Savepoints**: Cannot nest transactions
-❌ **No Advanced Sync**: sync_until, flush_replicator, freeze, get_frame_no
-❌ **Limited Introspection**: No statement column_count, column_name
 
 ---
 
@@ -87,11 +84,11 @@ This analysis is based on **authoritative sources**:
 | READ_ONLY behaviour | ✅ | Line 130 | `TransactionBehavior::ReadOnly` | P0 |
 | Commit transaction | ✅ | `commit_or_rollback_transaction/5` (lib.rs:285) | `trx.commit()` | P0 |
 | Rollback transaction | ✅ | `commit_or_rollback_transaction/5` (lib.rs:285) | `trx.rollback()` | P0 |
-| Savepoints | ❌ | Not implemented | `trx.savepoint()` | P1 |
-| Release savepoint | ❌ | Not implemented | `trx.release_savepoint()` | P1 |
-| Rollback to savepoint | ❌ | Not implemented | `trx.rollback_to_savepoint()` | P1 |
+| Savepoints | ✅ | `savepoint/2` (lib.rs) | `SAVEPOINT` SQL | P1 |
+| Release savepoint | ✅ | `release_savepoint/1` (lib.rs) | `RELEASE SAVEPOINT` SQL | P1 |
+| Rollback to savepoint | ✅ | `rollback_to_savepoint/1` (lib.rs) | `ROLLBACK TO SAVEPOINT` SQL | P1 |
 
-**Assessment**: All basic transaction operations complete. Savepoints would enable nested transaction-like behaviour for complex operations.
+**Assessment**: All transaction operations complete, including savepoints for nested transaction-like behaviour. Savepoint support added in v0.6.0 (PR #27) enables complex error handling and partial rollbacks within transactions.
 
 **Why Savepoints Matter**:
 ```elixir
@@ -113,7 +110,7 @@ end)
 
 ---
 
-### 4. Prepared Statements (44% Coverage) ⚠️
+### 4. Prepared Statements (78% Coverage)
 
 | Feature | Status | Implementation | libsql API | Priority |
 |---------|--------|---------------|-----------|----------|
@@ -123,9 +120,9 @@ end)
 | Close statement | ✅ | `close/2` (lib.rs:336) | Registry cleanup | P0 |
 | Statement reset | ❌ | **Re-prepares!** | `stmt.reset()` | P0 |
 | Clear bindings | ❌ | Not implemented | `stmt.clear_bindings()` | P2 |
-| Column count | ❌ | Not implemented | `stmt.column_count()` | P1 |
-| Column name | ❌ | Not implemented | `stmt.column_name()` | P1 |
-| Parameter count | ❌ | Not implemented | `stmt.parameter_count()` | P1 |
+| Column count | ✅ | `get_statement_column_count/1` (lib.rs) | `stmt.column_count()` | P1 |
+| Column name | ✅ | `get_statement_column_name/2` (lib.rs) | `stmt.column_name()` | P1 |
+| Parameter count | ✅ | `get_statement_parameter_count/1` (lib.rs) | `stmt.parameter_count()` | P1 |
 
 **Critical Issue**: Lines 885-888 and 951-954 re-prepare statements on every execution, defeating the purpose of prepared statements.
 
@@ -140,21 +137,21 @@ let stmt = conn_guard.prepare(&sql).await  // ← Called EVERY time!
 
 ---
 
-### 5. Replica Sync Features (33% Coverage)
+### 5. Replica Sync Features (67% Coverage)
 
 | Feature | Status | Implementation | libsql API | Priority |
 |---------|--------|---------------|-----------|----------|
 | Manual sync | ✅ | `do_sync/2` (lib.rs:263) | `db.sync()` | P0 |
 | Sync with timeout | ✅ | `sync_with_timeout` (lib.rs:44) | Custom wrapper | P0 |
 | Auto-sync on writes | ✅ | Built-in | libsql automatic | P0 |
 | Sync frames | ❌ | Not implemented | `db.sync_frames()` | P2 |
-| Sync until frame | ❌ | Not implemented | `db.sync_until()` | P2 |
-| Get frame number | ❌ | Not implemented | `db.get_frame_no()` | P2 |
-| Flush replicator | ❌ | Not implemented | `db.flush_replicator()` | P2 |
+| Sync until frame | ✅ | `sync_until/2` (lib.rs) | `db.sync_until()` | P2 |
+| Get frame number | ✅ | `get_frame_number/1` (lib.rs) | `db.get_frame_no()` | P2 |
+| Flush replicator | ✅ | `flush_replicator/1` (lib.rs) | `db.flush_replicator()` | P2 |
 | Freeze database | ❌ | Not implemented | `db.freeze()` | P2 |
 | Flush writes | ❌ | Not implemented | `db.flush()` | P2 |
 
-**Assessment**: Core sync functionality works. Advanced features needed for monitoring replication lag and fine-grained control.
+**Assessment**: Excellent replica sync support! Core sync functionality and advanced monitoring features are implemented (added in v0.6.0, PR #27). Can now monitor replication lag via frame numbers and fine-tune sync behaviour.
 
 **Important Note** (from code comments lines 507-513, 737-738):
 > libsql automatically syncs writes to remote for embedded replicas. Manual sync is for pulling remote changes locally.
@@ -166,14 +163,17 @@ let stmt = conn_guard.prepare(&sql).await  // ← Called EVERY time!
 - ✅ Monotonic reads guaranteed
 - ❌ No global ordering guarantees
 
-**Use Cases for Missing Features**:
+**Example Usage of Advanced Sync Features**:
 ```elixir
-# Monitor replication lag
-frame = EctoLibSql.get_frame_number(repo)
-:ok = EctoLibSql.sync_until(repo, frame + 100)  # Wait for specific frame
+# Monitor replication lag (now available!)
+{:ok, frame} = EctoLibSql.Native.get_frame_number(state)
+{:ok, new_state} = EctoLibSql.Native.sync_until(state, frame + 100)
 
-# Disaster recovery
-:ok = EctoLibSql.freeze(repo)  # Convert replica to standalone DB
+# Flush pending writes (now available!)
+{:ok, new_state} = EctoLibSql.Native.flush_replicator(state)
+
+# Still missing: Disaster recovery
+# :ok = EctoLibSql.freeze(repo)  # Convert replica to standalone DB
 ```
 
 ---
@@ -366,16 +366,16 @@ Repo.query("SELECT * FROM docs ORDER BY #{distance} LIMIT 10")
 |----------|------------|---------|----------|----------|
 | Connection Management | 6 | 2 | **75%** | P0 |
 | Query Execution | 4 | 1 | **80%** | P0 |
-| Transactions | 8 | 3 | **73%** | P0 |
-| Prepared Statements | 4 | 5 | **44%** ⚠️ | P0 |
-| Replica Sync | 3 | 6 | **33%** | P1 |
+| Transactions | 11 | 0 | **100%** ✅ | P0 |
+| Prepared Statements | 7 | 2 | **78%** | P0 |
+| Replica Sync | 6 | 3 | **67%** | P1 |
 | Metadata | 4 | 3 | **57%** | P1 |
 | Configuration | 4 | 4 | **50%** | P0 |
 | Batch Execution | 4 | 1 | **80%** | P1 |
 | Cursors/Streaming | 4 | 3 | **57%** | P1 |
 | Hooks/Extensions | 0 | 8 | **0%** ❌ | P2 |
 | Vector Search | 3 | 2 | **60%** | P2 |
-| **TOTAL** | **44** | **38** | **54%** | - |
+| **TOTAL** | **53** | **29** | **65%** | - |
 
 ### By Priority (Production Readiness)
 
diff --git a/TESTING_PLAN_COMPREHENSIVE.md b/TESTING_PLAN_COMPREHENSIVE.md
@@ -990,7 +990,7 @@ mix test --only turso_remote
 
 - ✅ **Test Coverage Badge**: In README.md
 - ✅ **Performance Baselines**: Documented in PERFORMANCE.md
-- ✅ **Test Organization**: Clear file structure
+- ✅ **Test Organisation**: Clear file structure
 - ✅ **Example Usage**: Tests serve as examples
 
 ---
diff --git a/lib/ecto_libsql/native.ex b/lib/ecto_libsql/native.ex
@@ -168,8 +168,6 @@ defmodule EctoLibSql.Native do
   @doc false
   def flush_replicator(_conn_id), do: :erlang.nif_error(:nif_not_loaded)
 
-
-
   @doc false
   def freeze_database(_conn_id), do: :erlang.nif_error(:nif_not_loaded)
 
@@ -982,8 +980,9 @@ defmodule EctoLibSql.Native do
   def create_savepoint(%EctoLibSql.State{trx_id: trx_id} = _state, name)
       when is_binary(trx_id) and is_binary(name) do
     case savepoint(trx_id, name) do
-      {} -> :ok
+      :ok -> :ok
       {:error, reason} -> {:error, reason}
+      other -> {:error, "Unexpected response: #{inspect(other)}"}
     end
   end
 
@@ -1009,8 +1008,9 @@ defmodule EctoLibSql.Native do
   def release_savepoint_by_name(%EctoLibSql.State{trx_id: trx_id} = _state, name)
       when is_binary(trx_id) and is_binary(name) do
     case release_savepoint(trx_id, name) do
-      {} -> :ok
+      :ok -> :ok
       {:error, reason} -> {:error, reason}
+      other -> {:error, "Unexpected response: #{inspect(other)}"}
     end
   end
 
@@ -1046,8 +1046,9 @@ defmodule EctoLibSql.Native do
   def rollback_to_savepoint_by_name(%EctoLibSql.State{trx_id: trx_id} = _state, name)
       when is_binary(trx_id) and is_binary(name) do
     case rollback_to_savepoint(trx_id, name) do
-      {} -> :ok
+      :ok -> :ok
       {:error, reason} -> {:error, reason}
+      other -> {:error, "Unexpected response: #{inspect(other)}"}
     end
   end
 
@@ -1084,7 +1085,8 @@ defmodule EctoLibSql.Native do
   def get_frame_number_for_replica(conn_id) when is_binary(conn_id) do
     case get_frame_number(conn_id) do
       frame_no when is_integer(frame_no) -> {:ok, frame_no}
-      error -> {:error, error}
+      {:error, reason} -> {:error, reason}
+      other -> {:error, "Unexpected response: #{inspect(other)}"}
     end
   end
 
@@ -1119,7 +1121,8 @@ defmodule EctoLibSql.Native do
       when is_binary(conn_id) and is_integer(target_frame) do
     case sync_until(conn_id, target_frame) do
       :ok -> :ok
-      other -> {:error, other}
+      {:error, reason} -> {:error, reason}
+      other -> {:error, "Unexpected response: #{inspect(other)}"}
     end
   end
 
@@ -1150,7 +1153,8 @@ defmodule EctoLibSql.Native do
   def flush_and_get_frame(conn_id) when is_binary(conn_id) do
     case flush_replicator(conn_id) do
       frame_no when is_integer(frame_no) -> {:ok, frame_no}
-      error -> {:error, error}
+      {:error, reason} -> {:error, reason}
+      other -> {:error, "Unexpected response: #{inspect(other)}"}
     end
   end
 
diff --git a/native/ecto_libsql/src/lib.rs b/native/ecto_libsql/src/lib.rs
@@ -1635,25 +1635,35 @@ fn statement_parameter_count(conn_id: &str, stmt_id: &str) -> NifResult<usize> {
 /// Create a savepoint within a transaction.
 /// Savepoints allow partial rollback without aborting the entire transaction.
 #[rustler::nif(schedule = "DirtyIo")]
-fn savepoint(trx_id: &str, name: &str) -> NifResult<()> {
+fn savepoint(trx_id: &str, name: &str) -> NifResult<Atom> {
     let mut txn_registry = safe_lock(&TXN_REGISTRY, "savepoint")?;
 
     let trx = txn_registry
         .get_mut(trx_id)
         .ok_or_else(|| rustler::Error::Term(Box::new("Transaction not found")))?;
 
+    // Validate savepoint name is a valid SQL identifier (alphanumeric + underscore, not starting with digit)
+    if name.is_empty()
+        || !name.chars().all(|c| c.is_alphanumeric() || c == '_')
+        || name.chars().next().map_or(true, |c| c.is_ascii_digit())
+    {
+        return Err(rustler::Error::Term(Box::new(
+            "Invalid savepoint name: must be a valid SQL identifier",
+        )));
+    }
+
     let sql = format!("SAVEPOINT {}", name);
 
     TOKIO_RUNTIME
         .block_on(async { trx.execute(&sql, Vec::<Value>::new()).await })
         .map_err(|e| rustler::Error::Term(Box::new(format!("Savepoint failed: {}", e))))?;
 
-    Ok(())
+    Ok(rustler::types::atom::ok())
 }
 
 /// Release (commit) a savepoint, making its changes permanent within the transaction.
 #[rustler::nif(schedule = "DirtyIo")]
-fn release_savepoint(trx_id: &str, name: &str) -> NifResult<()> {
+fn release_savepoint(trx_id: &str, name: &str) -> NifResult<Atom> {
     let mut txn_registry = safe_lock(&TXN_REGISTRY, "release_savepoint")?;
 
     let trx = txn_registry
@@ -1666,13 +1676,13 @@ fn release_savepoint(trx_id: &str, name: &str) -> NifResult<()> {
         .block_on(async { trx.execute(&sql, Vec::<Value>::new()).await })
         .map_err(|e| rustler::Error::Term(Box::new(format!("Release savepoint failed: {}", e))))?;
 
-    Ok(())
+    Ok(rustler::types::atom::ok())
 }
 
 /// Rollback to a savepoint, undoing all changes made after the savepoint was created.
 /// The savepoint remains active and can be released or rolled back to again.
 #[rustler::nif(schedule = "DirtyIo")]
-fn rollback_to_savepoint(trx_id: &str, name: &str) -> NifResult<()> {
+fn rollback_to_savepoint(trx_id: &str, name: &str) -> NifResult<Atom> {
     let mut txn_registry = safe_lock(&TXN_REGISTRY, "rollback_to_savepoint")?;
 
     let trx = txn_registry
@@ -1687,7 +1697,7 @@ fn rollback_to_savepoint(trx_id: &str, name: &str) -> NifResult<()> {
             rustler::Error::Term(Box::new(format!("Rollback to savepoint failed: {}", e)))
         })?;
 
-    Ok(())
+    Ok(rustler::types::atom::ok())
 }
 
 /// Get the current frame number from a remote replica database.
diff --git a/test/prepared_statement_test.exs b/test/prepared_statement_test.exs
@@ -145,7 +145,9 @@ defmodule EctoLibSql.PreparedStatementTest do
       result = Native.query_stmt(state, stmt_id, [1])
 
       case result do
-        {:error, _reason} -> :ok
+        {:error, _reason} ->
+          :ok
+
         {:ok, result} ->
           # If it succeeds, it should return empty results (no matches)
           assert is_list(result.rows)
diff --git a/test/savepoint_test.exs b/test/savepoint_test.exs
