Merge pull request #61 from ocean/bugfix-queryable-execute-nil-return

ocean · web-flow · commit cee693941281 · 2026-01-18T17:09:33.000+11:00
Bugfix queryable execute nil return
diff --git a/.beads/last-touched b/.beads/last-touched
diff --git 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, ?,, &quote_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), ", ", &quote_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
@@ -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
@@ -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
@@ -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
@@ -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
