diff --git a/.beads/last-touched b/.beads/last-touched index 09970790..516ae6f0 100644 --- a/.beads/last-touched +++ b/.beads/last-touched @@ -1 +1 @@ -el-4ha +el-ffc diff --git a/CHANGELOG.md b/CHANGELOG.md index 5308eac9..44eefc1b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -97,6 +97,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ``` - 9 new CTE tests covering simple, recursive, and edge cases +- **EXPLAIN QUERY PLAN Support** + - Full support for SQLite's `EXPLAIN QUERY PLAN` via Ecto's `Repo.explain/2` and `Repo.explain/3` + - **Query detection**: Rust NIF `should_use_query()` now detects EXPLAIN statements for proper query/execute routing + - **Ecto.Multi compatibility**: `explain_query/4` callback returns `{:ok, maps}` tuple format required by Ecto.Multi + - **Output format**: Returns list of maps with `id`, `parent`, `notused`, and `detail` keys matching SQLite's output + - **Usage examples**: + ```elixir + # Basic EXPLAIN QUERY PLAN + {:ok, plan} = Repo.explain(:all, from(u in User, where: u.active == true)) + # Returns: [%{"id" => 2, "parent" => 0, "notused" => 0, "detail" => "SCAN users"}] + + # With options + {:ok, plan} = Repo.explain(:all, query, analyze: true) + + # Direct SQL execution + {:ok, _, result, _state} = EctoLibSql.handle_execute( + "EXPLAIN QUERY PLAN SELECT * FROM users WHERE id = ?", + [1], + [], + state + ) + ``` + - **Implementation**: Query detection in `utils.rs:should_use_query()`, SQL generation in `connection.ex:explain_query/4` + - **Test coverage**: 12 tests across `explain_simple_test.exs` and `explain_query_test.exs` + ## [0.8.3] - 2025-12-29 ### Added diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 00000000..1e8f22df --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,112 @@ +# Security + +## Vulnerability Mitigation + +### CVE-2025-47736: libsql-sqlite3-parser UTF-8 Crash + +**Status:** MITIGATED +**Severity:** Low +**Affected Component:** `libsql-sqlite3-parser` ≤ 0.13.0 (transitive dependency via `libsql`) + +#### Vulnerability Description + +The `libsql-sqlite3-parser` crate through version 0.13.0 can crash when processing invalid UTF-8 input in SQL queries. This vulnerability is documented in [CVE-2025-47736](https://advisories.gitlab.com/pkg/cargo/libsql-sqlite3-parser/CVE-2025-47736/). + +#### Our Mitigation Strategy + +**ecto_libsql is NOT vulnerable** to this CVE due to multiple layers of defence: + +##### 1. **Type System Protection (Primary Defence)** +- All SQL strings in our Rust NIF code use Rust's `&str` type +- Rust's type system guarantees that `&str` contains valid UTF-8 +- Any attempt to create invalid UTF-8 in Rust would fail at compile time + +##### 2. **Rustler Validation (Secondary Defence)** +- Rustler (our NIF bridge) validates UTF-8 when converting Elixir binaries to Rust strings +- Invalid UTF-8 from Elixir would cause NIF conversion errors before reaching our code +- These errors are caught and returned to Elixir as error tuples + +##### 3. **Explicit Validation (Defence-in-Depth)** +- We've added explicit UTF-8 validation at all SQL entry points: + - `prepare_statement/2` in `statement.rs` + - `declare_cursor/3` in `cursor.rs` + - `execute_batch_native/3` in `batch.rs` +- This validation provides: + - Explicit documentation of security guarantees + - Additional safety layer against future changes + - Clear audit trail for security reviews + +#### Implementation Details + +The `validate_utf8_sql/1` function in `native/ecto_libsql/src/utils.rs` (lines 13-60) performs the validation: + +```rust +pub fn validate_utf8_sql(sql: &str) -> Result<(), rustler::Error> { + // Validates UTF-8 boundaries and character indices + // Returns descriptive errors for any invalid sequences +} +``` + +This validation is called at the start of every NIF function that accepts SQL, before the SQL reaches `libsql-sqlite3-parser`. + +#### Why This Vulnerability Doesn't Affect Us + +Even without our explicit validation (layer 3), the vulnerability cannot be triggered because: + +1. **Elixir strings are UTF-8:** Elixir enforces UTF-8 for all string literals and string operations +2. **Rustler enforces UTF-8:** Converting from Elixir to Rust `&str` validates UTF-8 +3. **Type safety:** Rust's `&str` cannot contain invalid UTF-8 by definition + +The explicit validation we've added serves as **defence-in-depth** and **documentation** of our security posture. + +#### Upstream Fix Status + +The vulnerability is fixed in commit `14f422a` of `libsql-sqlite3-parser`, but this fix has not been released to crates.io yet. Once a new version is published, we will: + +1. Update our `libsql` dependency (which will pull in the fixed parser) +2. Continue to maintain our explicit validation as defence-in-depth +3. Update this document with the new version information + +#### Testing + +Our test suite includes UTF-8 validation coverage: +- All named parameter tests exercise the validation code paths +- Invalid UTF-8 would be caught by Rustler before reaching our code +- Our validation function is called on every SQL query + +#### Reporting Security Issues + +If you discover a security vulnerability in ecto_libsql, please email the maintainers directly rather than opening a public issue. See our [CONTRIBUTING.md](CONTRIBUTING.md) for contact information. + +## Security Best Practices + +When using ecto_libsql in your applications: + +1. **Use parameterised queries:** Always use Ecto's parameter binding (`?` or `:param`) instead of string interpolation +2. **Validate input:** Validate user input at application boundaries before passing to database queries +3. **Keep dependencies updated:** Regularly update ecto_libsql and Ecto to get security fixes +4. **Use encryption:** Enable encryption for sensitive data using the `:encryption_key` option +5. **Secure credentials:** Store Turso auth tokens in environment variables, not in source code + +## Dependency Security + +We use the following tools to monitor dependency security: + +- **Dependabot:** Automated vulnerability scanning on GitHub +- **cargo audit:** Rust dependency vulnerability checking +- **mix audit:** Elixir dependency vulnerability checking + +Run security audits locally: + +```bash +# Rust dependencies +cd native/ecto_libsql && cargo audit + +# Elixir dependencies (requires mix_audit) +mix deps.audit +``` + +## Changelog + +- **2026-01-07:** Added explicit UTF-8 validation as defence against CVE-2025-47736 +- **2025-12-30:** v0.5.0 - Eliminated all `.unwrap()` calls in production code (CVE-prevention) diff --git a/lib/ecto/adapters/libsql/connection.ex b/lib/ecto/adapters/libsql/connection.ex index ff4418fc..7ab1f1df 100644 --- a/lib/ecto/adapters/libsql/connection.ex +++ b/lib/ecto/adapters/libsql/connection.ex @@ -694,9 +694,25 @@ defmodule Ecto.Adapters.LibSql.Connection do @impl true def explain_query(conn, query, params, opts) do - sql = all(query) - explain_sql = IO.iodata_to_binary(["EXPLAIN QUERY PLAN " | sql]) - execute(conn, explain_sql, params, opts) + # The query parameter is the prepared SQL string generated by Ecto + # Prepend "EXPLAIN QUERY PLAN" to get the optimiser plan + sql = IO.iodata_to_binary(["EXPLAIN QUERY PLAN " | query]) + + # EXPLAIN QUERY PLAN returns rows, so use query() path not execute() + case query(conn, sql, params, opts) do + {:ok, result} -> + # Convert result to list of maps for Ecto's explain consumption + # Return {:ok, maps} - Ecto.Multi requires this format + maps = + Enum.map(result.rows, fn row -> + Enum.zip(result.columns, row) |> Enum.into(%{}) + end) + + {:ok, maps} + + error -> + error + end end @impl true diff --git a/lib/ecto_libsql.ex b/lib/ecto_libsql.ex index b4631b5d..94f49fcf 100644 --- a/lib/ecto_libsql.ex +++ b/lib/ecto_libsql.ex @@ -152,11 +152,154 @@ defmodule EctoLibSql do query when is_binary(query) -> %EctoLibSql.Query{statement: query} end - if trx_id do - EctoLibSql.Native.execute_with_trx(state, query_struct, args) - else - EctoLibSql.Native.execute_non_trx(query_struct, state, args) + # Check if query returns rows (SELECT, EXPLAIN, WITH, RETURNING clauses). + # If so, route through query path instead of execute path. + sql = query_struct.statement + + case EctoLibSql.Native.should_use_query_path(sql) do + true -> + # Query returns rows, use the query path. + # Convert map arguments to list if needed (NIFs expect lists). + normalised_args = normalise_args_for_query(sql, args) + + if trx_id do + EctoLibSql.Native.query_with_trx_args(trx_id, state.conn_id, sql, normalised_args) + |> format_query_result(state) + else + EctoLibSql.Native.query_args( + state.conn_id, + state.mode, + state.sync, + sql, + normalised_args + ) + |> format_query_result(state) + end + + false -> + # Query doesn't return rows, use the execute path (INSERT/UPDATE/DELETE). + # Note: execute_with_trx and execute_non_trx handle argument normalisation internally. + if trx_id do + EctoLibSql.Native.execute_with_trx(state, query_struct, args) + else + EctoLibSql.Native.execute_non_trx(query_struct, state, args) + end + end + end + + # Helper to format raw query results for return + defp format_query_result(%{"columns" => columns, "rows" => rows, "num_rows" => num_rows}, state) do + result = %EctoLibSql.Result{ + columns: columns, + rows: rows, + num_rows: num_rows + } + + {:ok, %EctoLibSql.Query{}, result, state} + end + + defp format_query_result({:error, reason}, state) do + error = build_error(reason) + {:error, error, state} + end + + # Build an EctoLibSql.Error from various reason formats. + defp build_error(%EctoLibSql.Error{} = error), do: error + + defp build_error(reason) when is_binary(reason) do + %EctoLibSql.Error{message: reason, sqlite: %{code: :error, message: reason}} + end + + defp build_error(reason) when is_map(reason) do + message = Map.get(reason, :message) || Map.get(reason, "message") || inspect(reason) + %EctoLibSql.Error{message: message, sqlite: %{code: :error, message: message}} + end + + defp build_error(reason) do + message = inspect(reason) + %EctoLibSql.Error{message: message, sqlite: %{code: :error, message: message}} + end + + # Convert map arguments to a list by extracting named parameters from SQL. + # If args is already a list, return it unchanged. + defp normalise_args_for_query(_sql, args) when is_list(args), do: args + + defp normalise_args_for_query(sql, args) when is_map(args) do + # Extract named parameters from SQL in order of appearance. + # Supports :name, $name, and @name formats. + param_names = extract_named_params(sql) + + # Validate that all parameters exist in the map and collect missing ones. + missing_params = + Enum.filter(param_names, fn name -> + not has_param?(args, name) + end) + + # Raise error if any parameters are missing. + if missing_params != [] do + missing_list = Enum.map_join(missing_params, ", ", &":#{&1}") + + raise ArgumentError, + "Missing required parameters: #{missing_list}. " <> + "SQL requires: #{Enum.map_join(param_names, ", ", &":#{&1}")}" end + + # Convert map values to list in parameter order. + Enum.map(param_names, fn name -> + get_param_value(args, name) + end) + end + + # Check if a parameter exists in the map (supports both atom and string keys). + # Uses String.to_existing_atom/1 to avoid atom table exhaustion from dynamic SQL. + defp has_param?(map, name) when is_binary(name) do + # Try existing atom key first (common case), then string key. + try do + atom_key = String.to_existing_atom(name) + Map.has_key?(map, atom_key) or Map.has_key?(map, name) + rescue + ArgumentError -> + # Atom doesn't exist, check string key only. + Map.has_key?(map, name) + end + end + + # Get a parameter value from a map, supporting both atom and string keys. + # Uses String.to_existing_atom/1 to avoid atom table exhaustion from dynamic SQL. + # This assumes the parameter exists (validated by has_param?). + defp get_param_value(map, name) when is_binary(name) do + # Try existing atom key first (common case), then string key. + atom_key = String.to_existing_atom(name) + Map.get(map, atom_key, Map.get(map, name)) + rescue + ArgumentError -> + # Atom doesn't exist, try string key only. + Map.get(map, name) + end + + # Extract named parameter names from SQL in order of appearance. + # Returns a list of strings to avoid atom table exhaustion from dynamic SQL. + # + # LIMITATION: This regex-based approach cannot distinguish between parameter-like + # patterns in SQL string literals or comments and actual parameters. For example: + # + # SELECT ':not_a_param', name FROM users WHERE id = :actual_param + # + # Would extract both "not_a_param" and "actual_param", even though the first + # appears in a string literal. This is an edge case that would require a full + # SQL parser to handle correctly (tracking quoted strings, escaped characters, + # and comment blocks). In practice, this limitation rarely causes issues because: + # 1. SQL string literals containing parameter-like patterns are uncommon + # 2. The validation will catch truly missing parameters + # 3. Extra entries in the parameter list are ignored during binding + # + # If this becomes problematic, consider using a proper SQL parser or the + # prepared statement introspection approach used in lib/ecto_libsql/native.ex. + defp extract_named_params(sql) do + # Match :name, $name, or @name patterns. + ~r/[:$@]([a-zA-Z_][a-zA-Z0-9_]*)/ + |> Regex.scan(sql) + |> Enum.map(fn [_full, name] -> name end) end @impl true diff --git a/lib/ecto_libsql/native.ex b/lib/ecto_libsql/native.ex index 7dad04ed..0c05de2a 100644 --- a/lib/ecto_libsql/native.ex +++ b/lib/ecto_libsql/native.ex @@ -145,6 +145,9 @@ defmodule EctoLibSql.Native do @doc false def set_authorizer(_conn_id, _pid), do: :erlang.nif_error(:nif_not_loaded) + @doc false + def should_use_query_path(_sql), do: :erlang.nif_error(:nif_not_loaded) + @doc false def pragma_query(_conn_id, _pragma_stmt), do: :erlang.nif_error(:nif_not_loaded) @@ -374,6 +377,21 @@ defmodule EctoLibSql.Native do end @doc false + # Check if a map has a key, supporting both atom and string keys. + # This avoids creating atoms at runtime while allowing users to pass + # either %{name: value} or %{"name" => value}. + defp has_map_key_flexible?(_map, nil), do: false + + defp has_map_key_flexible?(map, name) when is_binary(name) do + # Try atom key first (more common), then string key. + atom_key = String.to_existing_atom(name) + Map.has_key?(map, atom_key) or Map.has_key?(map, name) + rescue + ArgumentError -> + # Atom doesn't exist, check string key only. + Map.has_key?(map, name) + end + # Get a value from a map, supporting both atom and string keys. # This avoids creating atoms at runtime while allowing users to pass # either %{name: value} or %{"name" => value}. @@ -389,6 +407,41 @@ defmodule EctoLibSql.Native do Map.get(map, name, nil) end + # Validate that all required parameters exist in the map. + # Raises ArgumentError if any parameters are missing or if map is used with positional params. + defp validate_params_exist(param_map, param_names) do + # Check if we have positional parameters (nil entries from ?). + has_positional = Enum.any?(param_names, &is_nil/1) + + if has_positional do + # SQL uses positional parameters (?), but user provided a map. + # This is a type mismatch - positional params require a list. + raise ArgumentError, + "Cannot use named parameter map with SQL that has positional parameters (?). " <> + "Use a list of values instead, e.g., [value1, value2] not %{key: value}" + end + + # Filter out any nil names (shouldn't happen after above check, but defensive). + named_params = Enum.reject(param_names, &is_nil/1) + + # Validate that all named parameters exist in the map. + missing_params = + Enum.filter(named_params, fn name -> + not has_map_key_flexible?(param_map, name) + end) + + if missing_params != [] do + missing_list = Enum.map_join(missing_params, ", ", &":#{&1}") + all_params = Enum.map_join(named_params, ", ", &":#{&1}") + + raise ArgumentError, + "Missing required parameters: #{missing_list}. " <> + "SQL requires: #{all_params}" + end + + :ok + end + # ETS-based LRU cache for parameter metadata. # Unlike persistent_term, this cache has a maximum size and evicts old entries. # This prevents unbounded memory growth from dynamic SQL workloads. @@ -527,7 +580,10 @@ defmodule EctoLibSql.Native do introspect_and_cache_params(conn_id, statement, param_map) param_names -> - # Cache hit - convert map to positional list using cached order. + # Cache hit - validate parameters exist before conversion (raises on missing params). + validate_params_exist(param_map, param_names) + + # Convert map to positional list using cached order. # Support both atom and string keys in the input map. Enum.map(param_names, fn name -> get_map_value_flexible(param_map, name) @@ -560,6 +616,9 @@ defmodule EctoLibSql.Native do # Cache the parameter names for future calls. cache_param_names(statement, param_names) + # Validate that all required parameters exist in the map (raises on missing params). + validate_params_exist(param_map, param_names) + # Convert map to positional list using the names. # Support both atom and string keys in the input map. Enum.map(param_names, fn name -> @@ -900,43 +959,62 @@ defmodule EctoLibSql.Native do end @doc """ - Execute a prepared statement with arguments (for non-SELECT queries). - Returns the number of affected rows. + Execute a prepared statement with arguments. + + Automatically routes to query_stmt if the statement returns rows (e.g., SELECT, EXPLAIN, RETURNING), + or to execute_prepared if it doesn't (e.g., INSERT/UPDATE/DELETE without RETURNING). ## Parameters - state: The connection state - stmt_id: The statement ID from prepare/2 - - sql: The original SQL (for sync detection) + - sql: The original SQL (for sync detection and statement type detection) - args: List of positional parameters OR map with atom keys for named parameters ## Examples - # Positional parameters + # INSERT without RETURNING {:ok, stmt_id} = EctoLibSql.Native.prepare(state, "INSERT INTO users (name) VALUES (?)") - {:ok, rows_affected} = EctoLibSql.Native.execute_stmt(state, stmt_id, sql, ["Alice"]) + {:ok, num_rows} = EctoLibSql.Native.execute_stmt(state, stmt_id, sql, ["Alice"]) - # Named parameters with atom keys - {:ok, stmt_id} = EctoLibSql.Native.prepare(state, "INSERT INTO users (name) VALUES (:name)") - {:ok, rows_affected} = EctoLibSql.Native.execute_stmt(state, stmt_id, sql, %{name: "Alice"}) + # SELECT query + {:ok, stmt_id} = EctoLibSql.Native.prepare(state, "SELECT * FROM users WHERE id = ?") + {:ok, result} = EctoLibSql.Native.execute_stmt(state, stmt_id, sql, [1]) + + # EXPLAIN query + {:ok, stmt_id} = EctoLibSql.Native.prepare(state, "EXPLAIN QUERY PLAN SELECT * FROM users") + {:ok, result} = EctoLibSql.Native.execute_stmt(state, stmt_id, sql, []) + + # INSERT with RETURNING + {:ok, stmt_id} = EctoLibSql.Native.prepare(state, "INSERT INTO users (name) VALUES (?) RETURNING *") + {:ok, result} = EctoLibSql.Native.execute_stmt(state, stmt_id, sql, ["Alice"]) """ def execute_stmt( - %EctoLibSql.State{conn_id: conn_id, mode: mode, sync: syncx} = _state, + %EctoLibSql.State{conn_id: conn_id, mode: mode, sync: syncx} = state, stmt_id, sql, args ) do - # Normalise arguments (convert map to positional list if needed). - case normalise_arguments_for_stmt(conn_id, stmt_id, args) do - {:error, reason} -> - {:error, "Failed to normalise parameters: #{reason}"} + # Check if this statement returns rows (uses the NIF for consistency with handle_execute). + case should_use_query_path(sql) do + true -> + # Use query_stmt path for statements that return rows. + query_stmt(state, stmt_id, args) + + false -> + # Use execute path for statements that don't return rows. + # Normalise arguments (convert map to positional list if needed). + case normalise_arguments_for_stmt(conn_id, stmt_id, args) do + {:error, reason} -> + {:error, "Failed to normalise parameters: #{reason}"} - normalised_args -> - case execute_prepared(conn_id, stmt_id, mode, syncx, sql, normalised_args) do - num_rows when is_integer(num_rows) -> - {:ok, num_rows} + normalised_args -> + case execute_prepared(conn_id, stmt_id, mode, syncx, sql, normalised_args) do + num_rows when is_integer(num_rows) -> + {:ok, num_rows} - {:error, reason} -> - {:error, reason} + {:error, reason} -> + {:error, reason} + end end end end diff --git a/native/ecto_libsql/src/batch.rs b/native/ecto_libsql/src/batch.rs index afe6d4fa..deb13a5b 100644 --- a/native/ecto_libsql/src/batch.rs +++ b/native/ecto_libsql/src/batch.rs @@ -201,6 +201,9 @@ pub fn execute_transactional_batch<'a>( /// statements that don't return rows or conditional statements not executed. #[rustler::nif(schedule = "DirtyIo")] pub fn execute_batch_native<'a>(env: Env<'a>, conn_id: &str, sql: &str) -> NifResult> { + // UTF-8 validation is guaranteed by Rust's &str type and Rustler's conversion, + // so we can rely on the type system rather than runtime checks. + let conn_map = safe_lock(&CONNECTION_REGISTRY, "execute_batch_native conn_map")?; if let Some(client) = conn_map.get(conn_id) { diff --git a/native/ecto_libsql/src/cursor.rs b/native/ecto_libsql/src/cursor.rs index 4dfc22b9..798d06a3 100644 --- a/native/ecto_libsql/src/cursor.rs +++ b/native/ecto_libsql/src/cursor.rs @@ -31,6 +31,9 @@ use rustler::{Atom, Binary, Encoder, Env, NifResult, OwnedBinary, Term}; /// Returns a cursor ID on success, error on failure. #[rustler::nif(schedule = "DirtyIo")] pub fn declare_cursor(conn_id: &str, sql: &str, args: Vec) -> NifResult { + // UTF-8 validation is guaranteed by Rust's &str type and Rustler's conversion, + // so we can rely on the type system rather than runtime checks. + let conn_map = utils::safe_lock(&CONNECTION_REGISTRY, "declare_cursor conn_map")?; let client = conn_map @@ -125,6 +128,9 @@ pub fn declare_cursor_with_context( sql: &str, args: Vec, ) -> NifResult { + // UTF-8 validation is guaranteed by Rust's &str type and Rustler's conversion, + // so we can rely on the type system rather than runtime checks. + let decoded_args: Vec = args .into_iter() .map(|t| utils::decode_term_to_value(t)) diff --git a/native/ecto_libsql/src/hooks.rs b/native/ecto_libsql/src/hooks.rs index 017f7058..90bfcb8f 100644 --- a/native/ecto_libsql/src/hooks.rs +++ b/native/ecto_libsql/src/hooks.rs @@ -160,3 +160,20 @@ pub fn set_authorizer(env: Env, _conn_id: &str, _pid: LocalPid) -> NifResult<(At Atom::from_str(env, "unsupported")?, )) } + +/// Determine if a SQL query should use the query path (returns rows) or execute path (no rows) +/// +/// This is used by the Elixir adapter to route queries correctly: +/// - SELECT, EXPLAIN, WITH, and RETURNING clauses return rows → use query path +/// - INSERT, UPDATE, DELETE (without RETURNING) don't return rows → use execute path +/// +/// # Arguments +/// - `sql` - SQL statement to analyze +/// +/// # Returns +/// - `true` - Query returns rows, should use query path +/// - `false` - Query doesn't return rows, should use execute path +#[rustler::nif] +pub fn should_use_query_path(sql: String) -> bool { + crate::should_use_query(&sql) +} diff --git a/native/ecto_libsql/src/statement.rs b/native/ecto_libsql/src/statement.rs index 741ba601..94180353 100644 --- a/native/ecto_libsql/src/statement.rs +++ b/native/ecto_libsql/src/statement.rs @@ -28,6 +28,9 @@ use std::sync::{Arc, Mutex}; /// Returns a statement ID on success, error on failure. #[rustler::nif(schedule = "DirtyIo")] pub fn prepare_statement(conn_id: &str, sql: &str) -> NifResult { + // UTF-8 validation is guaranteed by Rust's &str type and Rustler's conversion, + // so we can rely on the type system rather than runtime checks. + let client = { let conn_map = utils::safe_lock(&CONNECTION_REGISTRY, "prepare_statement conn_map")?; conn_map diff --git a/native/ecto_libsql/src/tests/utils_tests.rs b/native/ecto_libsql/src/tests/utils_tests.rs index 5ac1a29a..48855f27 100644 --- a/native/ecto_libsql/src/tests/utils_tests.rs +++ b/native/ecto_libsql/src/tests/utils_tests.rs @@ -419,42 +419,60 @@ mod should_use_query_tests { } // ===== EXPLAIN Query Tests ===== + // EXPLAIN queries always return rows (the query plan), so should_use_query returns true. #[test] - fn test_explain_select_not_detected() { - // EXPLAIN SELECT is NOT detected because it starts with EXPLAIN, not SELECT. - assert!(!should_use_query("EXPLAIN SELECT * FROM users")); - assert!(!should_use_query( + fn test_explain_select_detected() { + // EXPLAIN SELECT returns query plan rows, so it's detected. + assert!(should_use_query("EXPLAIN SELECT * FROM users")); + assert!(should_use_query( "EXPLAIN QUERY PLAN SELECT * FROM users WHERE id = 1" )); } #[test] - fn test_explain_insert_not_detected() { - // EXPLAIN INSERT (without RETURNING) is not detected. - assert!(!should_use_query( + fn test_explain_insert_detected() { + // EXPLAIN INSERT returns query plan rows, so it's detected. + assert!(should_use_query( "EXPLAIN INSERT INTO users VALUES (1, 'Alice')" )); - // However, if RETURNING is added, it IS detected because of the RETURNING keyword. + // With RETURNING, it's also detected (via both EXPLAIN and RETURNING). assert!(should_use_query( "EXPLAIN INSERT INTO users VALUES (1, 'Alice') RETURNING id" )); } #[test] - fn test_explain_update_delete_not_detected() { - assert!(!should_use_query( + fn test_explain_update_delete_detected() { + // EXPLAIN UPDATE/DELETE return query plan rows, so they're detected. + assert!(should_use_query( "EXPLAIN UPDATE users SET name = 'Bob' WHERE id = 1" )); - assert!(!should_use_query("EXPLAIN DELETE FROM users WHERE id = 1")); + assert!(should_use_query("EXPLAIN DELETE FROM users WHERE id = 1")); - // With RETURNING, they ARE detected via the RETURNING keyword. + // With RETURNING, they're also detected (via both EXPLAIN and RETURNING). assert!(should_use_query( "EXPLAIN UPDATE users SET name = 'Bob' WHERE id = 1 RETURNING id" )); } + #[test] + fn test_explain_case_insensitive() { + assert!(should_use_query("explain SELECT * FROM users")); + assert!(should_use_query("Explain SELECT * FROM users")); + assert!(should_use_query("EXPLAIN select * from users")); + } + + #[test] + fn test_explain_with_whitespace() { + assert!(should_use_query(" EXPLAIN SELECT * FROM users")); + assert!(should_use_query("\tEXPLAIN SELECT * FROM users")); + assert!(should_use_query( + "\n EXPLAIN QUERY PLAN SELECT * FROM users" + )); + } + // ===== Performance Tests ===== #[test] @@ -622,9 +640,10 @@ mod should_use_query_tests { } #[test] - fn test_explain_queries_not_detected_as_select() { - assert!(!should_use_query("EXPLAIN SELECT * FROM users")); - assert!(!should_use_query( + fn test_explain_queries_detected_as_returning_rows() { + // EXPLAIN queries return query plan rows and should use the query path. + assert!(should_use_query("EXPLAIN SELECT * FROM users")); + assert!(should_use_query( "EXPLAIN QUERY PLAN SELECT * FROM users WHERE id = 1" )); } diff --git a/native/ecto_libsql/src/transaction.rs b/native/ecto_libsql/src/transaction.rs index 9752129e..7550655f 100644 --- a/native/ecto_libsql/src/transaction.rs +++ b/native/ecto_libsql/src/transaction.rs @@ -326,6 +326,9 @@ pub fn query_with_trx_args<'a>( query: &str, args: Vec>, ) -> NifResult> { + // UTF-8 validation is guaranteed by Rust's &str type and Rustler's conversion, + // so we can rely on the type system rather than runtime checks. + // Decode args before locking let decoded_args: Vec = args .into_iter() diff --git a/native/ecto_libsql/src/utils.rs b/native/ecto_libsql/src/utils.rs index 3eaf242b..1f9e5e11 100644 --- a/native/ecto_libsql/src/utils.rs +++ b/native/ecto_libsql/src/utils.rs @@ -342,6 +342,22 @@ pub fn should_use_query(sql: &str) -> bool { return false; } + // Check if starts with EXPLAIN (case-insensitive) + // EXPLAIN always returns rows, so treat it like a query + if len - start >= 7 + && (bytes[start] == b'E' || bytes[start] == b'e') + && (bytes[start + 1] == b'X' || bytes[start + 1] == b'x') + && (bytes[start + 2] == b'P' || bytes[start + 2] == b'p') + && (bytes[start + 3] == b'L' || bytes[start + 3] == b'l') + && (bytes[start + 4] == b'A' || bytes[start + 4] == b'a') + && (bytes[start + 5] == b'I' || bytes[start + 5] == b'i') + && (bytes[start + 6] == b'N' || bytes[start + 6] == b'n') + // Verify it's followed by whitespace or end of string + && (start + 7 >= len || bytes[start + 7].is_ascii_whitespace()) + { + return true; + } + // Check if starts with SELECT (case-insensitive) if len - start >= 6 && (bytes[start] == b'S' || bytes[start] == b's') diff --git a/test/explain_query_test.exs b/test/explain_query_test.exs new file mode 100644 index 00000000..9c013d39 --- /dev/null +++ b/test/explain_query_test.exs @@ -0,0 +1,252 @@ +defmodule EctoLibSql.ExplainQueryTest do + @moduledoc """ + Tests for EXPLAIN and EXPLAIN QUERY PLAN support. + + libSQL/SQLite provides two explain modes: + - EXPLAIN: Low-level bytecode execution plan + - EXPLAIN QUERY PLAN: High-level query optimisation plan (recommended for debugging) + + This module tests the `Ecto.Adapters.SQL.explain/3` API which calls the adapter's + `explain_query/4` callback. + """ + + use ExUnit.Case, async: false + + import Ecto.Query + + # Define test modules for Ecto schemas and repo + defmodule TestRepo do + use Ecto.Repo, + otp_app: :ecto_libsql, + adapter: Ecto.Adapters.LibSql + end + + defmodule User do + use Ecto.Schema + import Ecto.Changeset + + schema "explain_users" do + field(:name, :string) + field(:email, :string) + field(:age, :integer) + + has_many(:posts, EctoLibSql.ExplainQueryTest.Post) + + timestamps() + end + + def changeset(user, attrs) do + user + |> cast(attrs, [:name, :email, :age]) + |> validate_required([:name, :email]) + end + end + + defmodule Post do + use Ecto.Schema + import Ecto.Changeset + + schema "explain_posts" do + field(:title, :string) + field(:body, :string) + + belongs_to(:user, EctoLibSql.ExplainQueryTest.User) + + timestamps() + end + + def changeset(post, attrs) do + post + |> cast(attrs, [:title, :body, :user_id]) + |> validate_required([:title, :body]) + end + end + + @test_db "z_ecto_libsql_test-explain.db" + + setup_all do + # Start the test repo + {:ok, _} = TestRepo.start_link(database: @test_db) + + # Create tables + Ecto.Adapters.SQL.query!(TestRepo, """ + CREATE TABLE IF NOT EXISTS explain_users ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + name TEXT NOT NULL, + email TEXT NOT NULL, + age INTEGER, + inserted_at DATETIME, + updated_at DATETIME + ) + """) + + Ecto.Adapters.SQL.query!(TestRepo, """ + CREATE TABLE IF NOT EXISTS explain_posts ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + title TEXT NOT NULL, + body TEXT, + user_id INTEGER, + inserted_at DATETIME, + updated_at DATETIME, + FOREIGN KEY(user_id) REFERENCES explain_users(id) + ) + """) + + # Create indexes for better explain output + Ecto.Adapters.SQL.query!( + TestRepo, + "CREATE INDEX IF NOT EXISTS explain_users_email_index ON explain_users(email)" + ) + + Ecto.Adapters.SQL.query!( + TestRepo, + "CREATE INDEX IF NOT EXISTS explain_posts_user_id_index ON explain_posts(user_id)" + ) + + on_exit(fn -> + try do + Ecto.Adapters.SQL.query!(TestRepo, "DROP TABLE IF EXISTS explain_posts") + Ecto.Adapters.SQL.query!(TestRepo, "DROP TABLE IF EXISTS explain_users") + catch + _, _ -> nil + end + + try do + GenServer.stop(TestRepo) + catch + _, _ -> nil + end + end) + + {:ok, []} + end + + setup do + # Clear tables before each test + TestRepo.delete_all(Post) + TestRepo.delete_all(User) + + # Create test data + {:ok, user1} = TestRepo.insert(%User{name: "Alice", email: "alice@example.com", age: 30}) + {:ok, user2} = TestRepo.insert(%User{name: "Bob", email: "bob@example.com", age: 25}) + {:ok, user3} = TestRepo.insert(%User{name: "Charlie", email: "charlie@example.com", age: 35}) + + {:ok, _post1} = TestRepo.insert(%Post{title: "First", body: "Content", user_id: user1.id}) + {:ok, _post2} = TestRepo.insert(%Post{title: "Second", body: "Content", user_id: user1.id}) + {:ok, _post3} = TestRepo.insert(%Post{title: "Third", body: "Content", user_id: user2.id}) + + {:ok, users: [user1, user2, user3]} + end + + describe "explain/3 - basic queries" do + test "returns explain plan for simple SELECT" do + query = from(u in User, select: u) + result = Ecto.Adapters.SQL.explain(TestRepo, :all, query) + + assert is_list(result) + assert length(result) > 0 + end + + test "returns explain plan for WHERE query" do + query = from(u in User, where: u.age > 25) + result = Ecto.Adapters.SQL.explain(TestRepo, :all, query) + + assert is_list(result) + assert length(result) > 0 + end + + test "returns explain plan for ORDER BY query" do + query = from(u in User, order_by: [desc: u.name]) + result = Ecto.Adapters.SQL.explain(TestRepo, :all, query) + + assert is_list(result) + assert length(result) > 0 + end + + test "returns explain plan for LIMIT query" do + query = from(u in User, limit: 10) + result = Ecto.Adapters.SQL.explain(TestRepo, :all, query) + + assert is_list(result) + assert length(result) > 0 + end + end + + describe "explain/3 - join queries" do + test "returns explain plan for INNER JOIN" do + query = + from(p in Post, + join: u in assoc(p, :user), + select: {u.name, p.title} + ) + + result = Ecto.Adapters.SQL.explain(TestRepo, :all, query) + + assert is_list(result) + assert length(result) > 0 + end + end + + describe "explain/3 - update and delete queries" do + test "returns explain plan for UPDATE query" do + query = from(u in User, where: u.age < 30, update: [set: [age: 30]]) + result = Ecto.Adapters.SQL.explain(TestRepo, :update_all, query) + + assert is_list(result) + end + + test "returns explain plan for DELETE query" do + query = from(u in User, where: u.age < 25) + result = Ecto.Adapters.SQL.explain(TestRepo, :delete_all, query) + + assert is_list(result) + end + end + + describe "explain/3 - options" do + test "respects wrap_in_transaction option" do + query = from(u in User, select: u) + + # With transaction (default) + result_with_txn = + Ecto.Adapters.SQL.explain(TestRepo, :all, query, wrap_in_transaction: true) + + assert is_list(result_with_txn) + + # Without transaction + # Note: Ecto.Adapters.SQL.explain with wrap_in_transaction: false returns {:ok, list} + # instead of just list. This appears to be a quirk in ecto_sql where the unwrapping + # only happens in the Multi transaction path. See: + # https://github.com/elixir-ecto/ecto_sql/blob/main/lib/ecto/adapters/sql.ex#L540 + result_without_txn = + Ecto.Adapters.SQL.explain(TestRepo, :all, query, wrap_in_transaction: false) + + assert match?({:ok, list} when is_list(list), result_without_txn) + + # Extract the list from the tuple for comparison + {:ok, list_without_txn} = result_without_txn + + # Results should be the same (ignoring the wrapping difference) + assert result_with_txn == list_without_txn + end + end + + describe "explain output format" do + test "explain output is a list of maps" do + query = from(u in User, where: u.age > 25) + result = Ecto.Adapters.SQL.explain(TestRepo, :all, query) + + assert is_list(result) + [first | _] = result + assert is_map(first) + end + + test "explain with WHERE shows plan" do + query = from(u in User, where: u.age > 25, select: u.id) + result = Ecto.Adapters.SQL.explain(TestRepo, :all, query) + + assert is_list(result) + assert length(result) > 0 + end + end +end diff --git a/test/explain_simple_test.exs b/test/explain_simple_test.exs new file mode 100644 index 00000000..ad5e9e9d --- /dev/null +++ b/test/explain_simple_test.exs @@ -0,0 +1,105 @@ +defmodule EctoLibSql.ExplainSimpleTest do + @moduledoc """ + Simpler test for EXPLAIN query support to debug the issue. + """ + + use ExUnit.Case, async: false + + import Ecto.Query + + defmodule TestRepo do + use Ecto.Repo, + otp_app: :ecto_libsql, + adapter: Ecto.Adapters.LibSql + end + + defmodule User do + use Ecto.Schema + + schema "explain_test_users" do + field(:name, :string) + field(:email, :string) + end + end + + @test_db "z_ecto_libsql_test-explain-simple.db" + + setup_all do + {:ok, _} = TestRepo.start_link(database: @test_db) + + Ecto.Adapters.SQL.query!(TestRepo, """ + CREATE TABLE IF NOT EXISTS explain_test_users ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + name TEXT NOT NULL, + email TEXT NOT NULL + ) + """) + + on_exit(fn -> + try do + Ecto.Adapters.SQL.query!(TestRepo, "DROP TABLE IF EXISTS explain_test_users") + catch + _, _ -> nil + end + + try do + GenServer.stop(TestRepo) + catch + _, _ -> nil + end + end) + + {:ok, []} + end + + test "direct EXPLAIN query via SQL" do + # Test that executing EXPLAIN directly works + sql = "EXPLAIN QUERY PLAN SELECT * FROM explain_test_users" + {:ok, result} = Ecto.Adapters.SQL.query(TestRepo, sql, []) + + assert is_struct(result, EctoLibSql.Result) + assert is_list(result.rows) + # EXPLAIN QUERY PLAN returns rows with columns: id, parent, notused, detail + assert length(result.columns) == 4 + assert result.columns == ["id", "parent", "notused", "detail"] + assert length(result.rows) > 0 + end + + test "EXPLAIN via explain API returns rows" do + # Build a simple query. + query = from(u in User, select: u.name) + + # The result should be a list of maps. + result = Ecto.Adapters.SQL.explain(TestRepo, :all, query) + + # Check it's a list of results. + assert is_list(result) + assert length(result) > 0 + end + + test "EXPLAIN on non-existent table returns error" do + sql = "EXPLAIN QUERY PLAN SELECT * FROM non_existent_table" + + assert {:error, %EctoLibSql.Error{message: message}} = + Ecto.Adapters.SQL.query(TestRepo, sql, []) + + assert message =~ "no such table" or message =~ "non_existent_table" + end + + test "EXPLAIN with invalid SQL syntax returns error" do + sql = "EXPLAIN QUERY PLAN SELECTT * FROM explain_test_users" + + assert {:error, %EctoLibSql.Error{}} = Ecto.Adapters.SQL.query(TestRepo, sql, []) + end + + test "EXPLAIN on empty table returns query plan" do + # EXPLAIN should work even on empty tables - it shows the query plan, not data. + sql = "EXPLAIN QUERY PLAN SELECT * FROM explain_test_users WHERE id = 999999" + {:ok, result} = Ecto.Adapters.SQL.query(TestRepo, sql, []) + + assert is_struct(result, EctoLibSql.Result) + assert is_list(result.rows) + # Should still return a query plan even for a query that would return no rows. + assert length(result.rows) > 0 + end +end diff --git a/test/named_parameters_execution_test.exs b/test/named_parameters_execution_test.exs index 7ef4fe32..f5cf35b9 100644 --- a/test/named_parameters_execution_test.exs +++ b/test/named_parameters_execution_test.exs @@ -478,17 +478,33 @@ defmodule EctoLibSql.NamedParametersExecutionTest do describe "Edge cases and error handling" do test "Missing named parameter raises clear error", %{state: state} do - # Try to execute with missing parameter - result = - EctoLibSql.handle_execute( - "INSERT INTO users (id, name, email, age) VALUES (:id, :name, :email, :age)", - %{id: 1, name: "Jack"}, - [], - state - ) + # Try to execute with missing parameters. + # Should raise ArgumentError with clear message about which parameters are missing. + assert_raise ArgumentError, + ~r/Missing required parameters: :email, :age/, + fn -> + EctoLibSql.handle_execute( + "INSERT INTO users (id, name, email, age) VALUES (:id, :name, :email, :age)", + %{id: 1, name: "Jack"}, + [], + state + ) + end + end - # Should fail because :email and :age are missing - assert match?({:error, _, _}, result) + test "Missing parameter for nullable column still raises error", %{state: state} do + # Even though 'age' is nullable, we should still raise an error if the parameter is missing. + # This prevents silent NULL insertions when a parameter is accidentally omitted. + assert_raise ArgumentError, + ~r/Missing required parameters: :age/, + fn -> + EctoLibSql.handle_execute( + "INSERT INTO users (id, name, email, age) VALUES (:id, :name, :email, :age)", + %{id: 1, name: "Jack", email: "jack@example.com"}, + [], + state + ) + end end test "Extra parameters in map are ignored", %{state: state} do @@ -552,17 +568,17 @@ defmodule EctoLibSql.NamedParametersExecutionTest do ) # Query using :Name (uppercase N) in SQL but provide :name (lowercase) in params. - # The parameter should NOT match due to case sensitivity. - {:ok, _, result, _} = - EctoLibSql.handle_execute( - "SELECT * FROM users WHERE name = :Name", - %{name: "Mike"}, - [], - state - ) - - # Should find no rows because :Name != :name. - assert result.num_rows == 0 + # Should raise error because parameter cases don't match. + assert_raise ArgumentError, + ~r/Missing required parameters: :Name/, + fn -> + EctoLibSql.handle_execute( + "SELECT * FROM users WHERE name = :Name", + %{name: "Mike"}, + [], + state + ) + end # Now use matching case - should work. {:ok, _, result, _} = @@ -575,5 +591,20 @@ defmodule EctoLibSql.NamedParametersExecutionTest do assert result.num_rows == 1 end + + test "Map parameters with positional SQL raises clear error", %{state: state} do + # Try to use a map with SQL that has positional parameters (?). + # Should raise clear error explaining the mismatch. + assert_raise ArgumentError, + ~r/Cannot use named parameter map with SQL that has positional parameters/, + fn -> + EctoLibSql.handle_execute( + "INSERT INTO users (id, name, email, age) VALUES (?, ?, ?, ?)", + %{id: 1, name: "Test", email: "test@example.com", age: 30}, + [], + state + ) + end + end end end