diff --git a/.beads/last-touched b/.beads/last-touched deleted file mode 100644 index 0b5d6651..00000000 --- a/.beads/last-touched +++ /dev/null @@ -1 +0,0 @@ -el-b21 diff --git a/lib/ecto/adapters/libsql/connection.ex b/lib/ecto/adapters/libsql/connection.ex index 19c10423..e72af65b 100644 --- a/lib/ecto/adapters/libsql/connection.ex +++ b/lib/ecto/adapters/libsql/connection.ex @@ -738,7 +738,7 @@ defmodule Ecto.Adapters.LibSql.Connection do {join, wheres} = using_join(query, :update_all, "FROM", sources) where = where(%{query | wheres: wheres}, sources) - ["UPDATE ", from, " AS ", name, " SET ", fields, join, where] + ["UPDATE ", from, " AS ", name, " SET ", fields, join, where | returning(query, sources)] end @impl true @@ -749,7 +749,7 @@ defmodule Ecto.Adapters.LibSql.Connection do {join, wheres} = using_join(query, :delete_all, "USING", sources) where = where(%{query | wheres: wheres}, sources) - ["DELETE FROM ", from, " AS ", name, join, where] + ["DELETE FROM ", from, " AS ", name, join, where | returning(query, sources)] end @impl true @@ -1359,6 +1359,53 @@ defmodule Ecto.Adapters.LibSql.Connection do [" RETURNING " | intersperse_map(returning, ?,, "e_name/1)] end + # Generate RETURNING clause from query select (for update_all/delete_all). + # Returns empty list if no select clause is present. + defp returning(%{select: nil}, _sources), do: [] + + defp returning(%{select: %{fields: fields}} = query, sources) do + [" RETURNING " | returning_fields(fields, sources, query)] + end + + # Format fields for RETURNING clause. + # SQLite's RETURNING clause doesn't support table aliases, so we use bare column names. + defp returning_fields([], _sources, _query), do: ["1"] + + defp returning_fields(fields, sources, query) do + intersperse_map(fields, ", ", fn + {:&, _, [idx]} -> + # Selecting all fields from a source - list all schema fields. + {_source, _name, schema} = elem(sources, idx) + + if schema do + Enum.map_join(schema.__schema__(:fields), ", ", "e_name/1) + else + "*" + end + + {key, value} when is_atom(key) -> + # Key-value pair (for maps) - return as "expr AS key". + # Use returning_expr to avoid table aliases. + [returning_expr(value, sources, query), " AS ", quote_name(key)] + + value -> + returning_expr(value, sources, query) + end) + end + + # Generate expressions for RETURNING clause without table aliases. + # SQLite doesn't support table aliases in RETURNING. + defp returning_expr({{:., _, [{:&, _, [_idx]}, field]}, _, []}, _sources, _query) + when is_atom(field) do + # Simple field reference like u.name - return just the quoted field name. + quote_name(field) + end + + defp returning_expr(value, sources, query) do + # For other expressions, fall back to the normal expr function. + expr(value, sources, query) + end + defp intersperse_map(list, separator, mapper) do intersperse_map(list, separator, mapper, []) end diff --git a/lib/ecto_libsql/native.ex b/lib/ecto_libsql/native.ex index 42ea9f2b..2a1ac78c 100644 --- a/lib/ecto_libsql/native.ex +++ b/lib/ecto_libsql/native.ex @@ -708,7 +708,7 @@ defmodule EctoLibSql.Native do end # For INSERT/UPDATE/DELETE without RETURNING, columns and rows will be empty - # Set them to nil to match Ecto's expectations + # Set them to nil to match Ecto's expectations for write operations {columns, rows} = if command in [:insert, :update, :delete] and columns == [] and rows == [] do {nil, nil} @@ -897,13 +897,46 @@ defmodule EctoLibSql.Native do @spec detect_command(String.t()) :: EctoLibSql.Result.command_type() def detect_command(query) when is_binary(query) do query - |> String.trim_leading() + |> skip_leading_comments_and_whitespace() |> extract_first_word() |> command_atom() end def detect_command(_), do: :unknown + # Skip leading whitespace and SQL comments (both -- and /* */ styles). + # This ensures queries starting with comments are correctly classified. + defp skip_leading_comments_and_whitespace(query) do + query + |> String.trim_leading() + |> do_skip_comments() + end + + defp do_skip_comments(<<"--", rest::binary>>) do + # Single-line comment: skip to end of line + rest + |> skip_to_newline() + |> skip_leading_comments_and_whitespace() + end + + defp do_skip_comments(<<"/*", rest::binary>>) do + # Block comment: skip to closing */ + rest + |> skip_to_block_end() + |> skip_leading_comments_and_whitespace() + end + + defp do_skip_comments(query), do: query + + defp skip_to_newline(<<"\n", rest::binary>>), do: rest + defp skip_to_newline(<<"\r\n", rest::binary>>), do: rest + defp skip_to_newline(<<_::binary-size(1), rest::binary>>), do: skip_to_newline(rest) + defp skip_to_newline(<<>>), do: <<>> + + defp skip_to_block_end(<<"*/", rest::binary>>), do: rest + defp skip_to_block_end(<<_::binary-size(1), rest::binary>>), do: skip_to_block_end(rest) + defp skip_to_block_end(<<>>), do: <<>> + defp extract_first_word(query) do # Extract first word more efficiently - stop at first whitespace first_word = diff --git a/lib/ecto_libsql/result.ex b/lib/ecto_libsql/result.ex index 073a364f..4e949468 100644 --- a/lib/ecto_libsql/result.ex +++ b/lib/ecto_libsql/result.ex @@ -9,8 +9,8 @@ defmodule EctoLibSql.Result do ## Fields - `:command` - The type of SQL command (`:select`, `:insert`, `:update`, `:delete`, `:create`, `:begin`, `:commit`, `:rollback`, `:pragma`, `:batch`, `:unknown`, `:other`, or `nil`) - - `:columns` - List of column names (for SELECT queries), or `nil` - - `:rows` - List of rows, where each row is a list of values, or `nil` + - `:columns` - List of column names (for SELECT queries), or `nil` for write operations + - `:rows` - List of rows, where each row is a list of values, or `nil` for write operations - `:num_rows` - Number of rows affected or returned ## Examples diff --git a/native/ecto_libsql/src/tests/utils_tests.rs b/native/ecto_libsql/src/tests/utils_tests.rs index 48855f27..4bb351e0 100644 --- a/native/ecto_libsql/src/tests/utils_tests.rs +++ b/native/ecto_libsql/src/tests/utils_tests.rs @@ -298,10 +298,31 @@ mod should_use_query_tests { #[test] fn test_sql_with_comments() { - // Comments BEFORE the statement: we don't parse SQL comments, - // so "-- Comment\nSELECT" won't detect SELECT (first non-whitespace is '-') - // This is fine - Ecto doesn't generate SQL with leading comments - assert!(!should_use_query("-- Comment\nSELECT * FROM users")); + // Leading single-line comments are now skipped correctly + assert!(should_use_query("-- Comment\nSELECT * FROM users")); + assert!(should_use_query( + "-- First comment\n-- Second comment\nSELECT * FROM users" + )); + + // Leading block comments are also skipped correctly + assert!(should_use_query("/* Block comment */ SELECT * FROM users")); + assert!(should_use_query( + "/* Multi\nline\nblock */ SELECT * FROM users" + )); + + // Mixed comments and whitespace + assert!(should_use_query(" -- Comment\n SELECT * FROM users")); + assert!(should_use_query( + "/* comment */ -- another\nSELECT * FROM users" + )); + + // Leading comments on other statements + assert!(!should_use_query( + "-- Comment\nINSERT INTO users VALUES (1)" + )); + assert!(should_use_query( + "-- Comment\nINSERT INTO users VALUES (1) RETURNING id" + )); // Comments WITHIN the statement are fine - we detect keywords/clauses assert!(should_use_query( diff --git a/native/ecto_libsql/src/utils.rs b/native/ecto_libsql/src/utils.rs index 1f9e5e11..baf44ec3 100644 --- a/native/ecto_libsql/src/utils.rs +++ b/native/ecto_libsql/src/utils.rs @@ -304,6 +304,60 @@ pub fn detect_query_type(query: &str) -> QueryType { } } +/// Skip leading whitespace and SQL comments in a byte slice. +/// +/// Handles both single-line comments (`-- comment`) and block comments (`/* comment */`). +/// Returns the index of the first non-whitespace, non-comment character. +#[inline] +fn skip_whitespace_and_comments(bytes: &[u8]) -> usize { + let len = bytes.len(); + let mut pos = 0; + + loop { + // Skip whitespace + while pos < len && bytes[pos].is_ascii_whitespace() { + pos += 1; + } + + if pos >= len { + return pos; + } + + // Check for single-line comment: -- ... + if pos + 1 < len && bytes[pos] == b'-' && bytes[pos + 1] == b'-' { + pos += 2; + // Skip to end of line + while pos < len && bytes[pos] != b'\n' { + pos += 1; + } + // Skip the newline if present + if pos < len && bytes[pos] == b'\n' { + pos += 1; + } + continue; + } + + // Check for block comment: /* ... */ + if pos + 1 < len && bytes[pos] == b'/' && bytes[pos + 1] == b'*' { + pos += 2; + // Find closing */ + while pos + 1 < len { + if bytes[pos] == b'*' && bytes[pos + 1] == b'/' { + pos += 2; + break; + } + pos += 1; + } + continue; + } + + // Not whitespace or comment, we're done + break; + } + + pos +} + /// Determines if a query should use query() or execute() /// /// Returns true if should use query() (SELECT or has RETURNING clause). @@ -314,10 +368,15 @@ pub fn detect_query_type(query: &str) -> QueryType { /// - Early termination on match /// - Case-insensitive ASCII comparison without allocations /// -/// ## Limitation: String and Comment Handling +/// ## Comment Handling +/// +/// This function correctly skips leading SQL comments (both `-- single line` +/// and `/* block */` style) before checking for query keywords. +/// +/// ## Limitation: String Literal Handling /// /// This function performs simple keyword matching and does not parse SQL syntax. -/// It will match keywords appearing in string literals or comments. +/// It will match keywords appearing in string literals. /// /// **Why this is acceptable**: /// - False positives (using `query()` when `execute()` would suffice) are **safe** @@ -332,11 +391,8 @@ pub fn should_use_query(sql: &str) -> bool { return false; } - // Find first non-whitespace character - let mut start = 0; - while start < len && bytes[start].is_ascii_whitespace() { - start += 1; - } + // Skip leading whitespace and comments + let start = skip_whitespace_and_comments(bytes); if start >= len { return false; diff --git a/test/returning_clause_test.exs b/test/returning_clause_test.exs new file mode 100644 index 00000000..8ab05174 --- /dev/null +++ b/test/returning_clause_test.exs @@ -0,0 +1,183 @@ +defmodule EctoLibSql.ReturningClauseTest do + @moduledoc """ + Tests for RETURNING clause support in update_all and delete_all operations. + + These tests verify that queries with select clauses correctly generate + RETURNING SQL and return the expected results instead of nil. + + This addresses the Protocol.UndefinedError that occurred when Ecto tried + to enumerate nil values returned from bulk operations with select clauses. + """ + + use EctoLibSql.Integration.Case, async: false + + alias EctoLibSql.Integration.TestRepo + alias EctoLibSql.Schemas.User + + import Ecto.Query + + @test_db "z_ecto_libsql_test-returning_clause.db" + + setup_all do + # Clean up existing database files first. + EctoLibSql.TestHelpers.cleanup_db_files(@test_db) + + # Configure the repo. + Application.put_env(:ecto_libsql, EctoLibSql.Integration.TestRepo, + adapter: Ecto.Adapters.LibSql, + database: @test_db + ) + + {:ok, _} = EctoLibSql.Integration.TestRepo.start_link() + + # Run migrations. + :ok = + Ecto.Migrator.up( + EctoLibSql.Integration.TestRepo, + 0, + EctoLibSql.Integration.Migration, + log: false + ) + + on_exit(fn -> + EctoLibSql.TestHelpers.cleanup_db_files(@test_db) + end) + + :ok + end + + setup do + # Clear users table before each test. + Ecto.Adapters.SQL.query!(TestRepo, "DELETE FROM users", []) + :ok + end + + describe "update_all with RETURNING" do + test "update_all without select returns {count, nil}" do + # Without a select clause, rows should be nil. + assert {0, nil} = TestRepo.update_all(User, set: [name: "Updated"]) + end + + test "update_all with select returns {count, []} when no rows match" do + query = from(u in User, where: u.name == "NonExistent", select: %{name: u.name}) + + # With select but no matching rows, should return empty list, not nil. + assert {0, []} = TestRepo.update_all(query, set: [name: "Updated"]) + end + + test "update_all with select returns {count, [results]} when rows match" do + {:ok, _} = TestRepo.insert(%User{name: "Alice"}) + {:ok, _} = TestRepo.insert(%User{name: "Bob"}) + + query = from(u in User, where: u.name == "Alice", select: %{name: u.name}) + {count, results} = TestRepo.update_all(query, set: [name: "Updated"]) + + assert count == 1 + assert is_list(results) + assert length(results) == 1 + assert [%{name: "Updated"}] = results + end + + test "update_all with select returns multiple results" do + {:ok, _} = TestRepo.insert(%User{name: "User1"}) + {:ok, _} = TestRepo.insert(%User{name: "User2"}) + {:ok, _} = TestRepo.insert(%User{name: "User3"}) + + query = from(u in User, select: %{name: u.name}) + {count, results} = TestRepo.update_all(query, set: [name: "AllUpdated"]) + + assert count == 3 + assert is_list(results) + assert length(results) == 3 + assert Enum.all?(results, fn r -> r.name == "AllUpdated" end) + end + + test "update_all with select returning multiple fields" do + {:ok, user} = TestRepo.insert(%User{name: "Alice"}) + + query = from(u in User, where: u.id == ^user.id, select: %{id: u.id, name: u.name}) + {count, results} = TestRepo.update_all(query, set: [name: "Updated"]) + + assert count == 1 + assert [%{id: id, name: "Updated"}] = results + assert id == user.id + end + end + + describe "delete_all with RETURNING" do + test "delete_all without select returns {count, nil}" do + {:ok, _} = TestRepo.insert(%User{name: "Alice"}) + + # Without a select clause, delete_all returns {count, nil}. + assert {1, nil} = TestRepo.delete_all(User) + end + + test "delete_all with select returns {count, []} when no rows match" do + query = from(u in User, where: u.name == "NonExistent", select: u) + + # With select but no matching rows, should return empty list. + assert {0, []} = TestRepo.delete_all(query) + end + + test "delete_all with select returns {count, [results]} when rows match" do + {:ok, _} = TestRepo.insert(%User{name: "ToDelete"}) + + query = from(u in User, where: u.name == "ToDelete", select: %{name: u.name}) + {count, results} = TestRepo.delete_all(query) + + assert count == 1 + assert is_list(results) + assert length(results) == 1 + assert [%{name: "ToDelete"}] = results + end + + test "delete_all with select returns multiple deleted records" do + {:ok, _} = TestRepo.insert(%User{name: "Delete1"}) + {:ok, _} = TestRepo.insert(%User{name: "Delete2"}) + + query = from(u in User, select: %{name: u.name}) + {count, results} = TestRepo.delete_all(query) + + assert count == 2 + assert is_list(results) + assert length(results) == 2 + end + end + + describe "edge cases" do + test "update_all with complex select expression" do + {:ok, _} = TestRepo.insert(%User{name: "Test"}) + + # Select with a constant value mixed with field. + query = from(u in User, select: %{name: u.name, constant: 42}) + {count, results} = TestRepo.update_all(query, set: [name: "Updated"]) + + assert count == 1 + assert [%{name: "Updated", constant: 42}] = results + end + + test "update_all in transaction with select" do + {:ok, _} = TestRepo.insert(%User{name: "TxTest"}) + + result = + TestRepo.transaction(fn -> + query = from(u in User, where: u.name == "TxTest", select: %{name: u.name}) + TestRepo.update_all(query, set: [name: "TxUpdated"]) + end) + + assert {:ok, {1, [%{name: "TxUpdated"}]}} = result + end + + test "delete_all in transaction with select" do + {:ok, _} = TestRepo.insert(%User{name: "TxDelete"}) + + result = + TestRepo.transaction(fn -> + query = from(u in User, where: u.name == "TxDelete", select: %{name: u.name}) + TestRepo.delete_all(query) + end) + + assert {:ok, {1, [%{name: "TxDelete"}]}} = result + end + end +end