Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .beads/last-touched

This file was deleted.

51 changes: 49 additions & 2 deletions lib/ecto/adapters/libsql/connection.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
37 changes: 35 additions & 2 deletions lib/ecto_libsql/native.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down Expand Up @@ -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 =
Expand Down
4 changes: 2 additions & 2 deletions lib/ecto_libsql/result.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
29 changes: 25 additions & 4 deletions native/ecto_libsql/src/tests/utils_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
70 changes: 63 additions & 7 deletions native/ecto_libsql/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand All @@ -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**
Expand All @@ -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;
Expand Down
Loading