diff --git a/CHANGELOG.md b/CHANGELOG.md index 36155cd..49f8898 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Upsert `ON CONFLICT DO UPDATE` SQL Generation** - Fixed two bugs that caused `"near ?: syntax error"` on any `INSERT ... ON CONFLICT DO UPDATE` query, breaking all upsert operations including Ash Framework's `upsert?` actions. (Thanks [@AlanMcCann](https://github.com/AlanMcCann) for [PR #95](https://github.com/ocean/ecto_libsql/pull/95)!) + - Missing `:identifier` expression handler: Ecto generates `ON CONFLICT UPDATE` clauses using `{:identifier, _, ["column_name"]}` fragment expressions. Without a matching `expr/3` clause these fell through to the catch-all, producing invalid SQL such as `SET "col" = EXCLUDED.?`. + - Bare `?` parameter placeholders: SQLite requires numbered positional parameters (`?1`, `?2`, …) when a statement contains multiple parameter groups (INSERT values + ON CONFLICT UPDATE). The adapter now uses numbered parameters throughout, consistent with the `ecto_sqlite3` adapter. + - `IN` clause with bound list parameters was also left using bare `?` placeholders. The `IN (?, ?, ?)` handler now generates `IN (?1, ?2, ?3)` with correct start-index offsets, matching the numbering scheme used everywhere else. + - Empty `IN` list edge case (`where: field in ^[]`) is handled explicitly with `IN (SELECT NULL WHERE 1=0)` since SQLite rejects `IN ()`. + - **PRAGMA Statement Routing** - PRAGMA statements are now correctly routed through the `query()` path rather than the execute path, fixing incorrect behaviour when reading PRAGMA values (e.g. `PRAGMA journal_mode`, `PRAGMA synchronous`) via the Ecto adapter. ### Changed diff --git a/lib/ecto/adapters/libsql/connection.ex b/lib/ecto/adapters/libsql/connection.ex index 9ed09c3..a10daa0 100644 --- a/lib/ecto/adapters/libsql/connection.ex +++ b/lib/ecto/adapters/libsql/connection.ex @@ -754,13 +754,14 @@ defmodule Ecto.Adapters.LibSql.Connection do @impl true def insert(prefix, table, header, rows, on_conflict, returning, placeholders) do + counter_offset = length(placeholders) + 1 fields = intersperse_map(header, ", ", "e_name/1) values = if rows == [] do [" DEFAULT VALUES"] else - [" VALUES ", encode_values(rows)] + [" VALUES ", encode_insert_values(rows, counter_offset)] end [ @@ -776,12 +777,26 @@ defmodule Ecto.Adapters.LibSql.Connection do ] end - defp encode_values(rows) do - rows - |> Enum.map(fn row -> - ["(", intersperse_map(row, ", ", fn _ -> "?" end), ")"] - end) - |> Enum.intersperse(", ") + # Generate VALUES with numbered positional parameters (?1, ?2, ...). + # SQLite requires numbered parameters when the same statement contains + # multiple parameter groups (e.g., INSERT values + ON CONFLICT UPDATE). + # Bare `?` causes "near ?: syntax error" in upsert queries. + defp encode_insert_values(rows, counter) do + {encoded, _} = + Enum.map_reduce(rows, counter, fn row, acc -> + {params, new_acc} = + Enum.map_reduce(row, acc, fn + {:placeholder, placeholder_index}, c -> + {[?? | placeholder_index], c} + + _, c -> + {[?? | Integer.to_string(c)], c + 1} + end) + + {["(", Enum.intersperse(params, ", "), ")"], new_acc} + end) + + Enum.intersperse(encoded, ", ") end # Helper for INSERT OR ... syntax (not used for now, keeping for SQLite REPLACE compatibility) @@ -1166,9 +1181,11 @@ defmodule Ecto.Adapters.LibSql.Connection do [?(, expr(expr, sources, query), ?)] end - # Parameter placeholder - defp expr({:^, [], [_ix]}, _sources, _query) do - ~c"?" + # Parameter placeholder - use numbered parameters (?1, ?2, ...). + # SQLite requires numbered parameters when a statement has multiple + # parameter groups (e.g., INSERT values + ON CONFLICT UPDATE). + defp expr({:^, [], [ix]}, _sources, _query) do + [?? | Integer.to_string(ix + 1)] end # Qualified field reference: s0.field @@ -1246,8 +1263,18 @@ defmodule Ecto.Adapters.LibSql.Connection do [expr(left, sources, query), " IN (", args, ?)] end - defp expr({:in, _, [left, {:^, _, [_, length]}]}, sources, query) do - args = Enum.intersperse(List.duplicate(??, length), ?,) + defp expr({:in, _, [left, {:^, _, [_start_ix, 0]}]}, sources, query) do + # Empty IN list - SQLite does not accept IN (), so use always-false predicate. + [expr(left, sources, query), " IN (SELECT NULL WHERE 1=0)"] + end + + defp expr({:in, _, [left, {:^, _, [start_ix, length]}]}, sources, query) do + args = + Enum.map(start_ix..(start_ix + length - 1)//1, fn ix -> + [?? | Integer.to_string(ix + 1)] + end) + |> Enum.intersperse(", ") + [expr(left, sources, query), " IN (", args, ?)] end @@ -1306,6 +1333,11 @@ defmodule Ecto.Adapters.LibSql.Connection do ["max(", expr(arg, sources, query), ?)] end + # Identifier expression (used in fragment expressions like EXCLUDED."column_name") + defp expr({:identifier, _, [name]}, _sources, _query) do + quote_name(name) + end + # Fragment for raw SQL defp expr({:fragment, _, parts}, sources, query) do Enum.map(parts, fn diff --git a/test/ecto_connection_test.exs b/test/ecto_connection_test.exs index 43cad84..d912a12 100644 --- a/test/ecto_connection_test.exs +++ b/test/ecto_connection_test.exs @@ -579,6 +579,88 @@ defmodule Ecto.Adapters.LibSql.ConnectionTest do end end + describe "IN clause parameter numbering" do + test "generates numbered parameters for bound IN list" do + # Simulates: where(query, [u], u.id in ^[1, 2, 3]) + # {:^, _, [start_ix, length]} where start_ix=0, length=3. + query = %Ecto.Query{ + from: %Ecto.Query.FromExpr{source: {"users", nil}}, + sources: {{"users", nil, nil}}, + select: %Ecto.Query.SelectExpr{fields: [{:&, [], [0]}]}, + wheres: [ + %Ecto.Query.BooleanExpr{ + op: :and, + expr: + {:in, [], + [ + {{:., [], [{:&, [], [0]}, :id]}, [], []}, + {:^, [], [0, 3]} + ]}, + params: [] + } + ] + } + + sql = Connection.all(query) |> IO.iodata_to_binary() + + # All three IN parameters must be numbered (?1, ?2, ?3), not bare ?. + assert sql =~ ~s["id" IN (?1, ?2, ?3)] + end + + test "IN parameter numbering is consistent with non-zero start index" do + # Simulates a query where previous bound params occupy ?1. + # start_ix=1, length=2 → should produce ?2, ?3. + query = %Ecto.Query{ + from: %Ecto.Query.FromExpr{source: {"users", nil}}, + sources: {{"users", nil, nil}}, + select: %Ecto.Query.SelectExpr{fields: [{:&, [], [0]}]}, + wheres: [ + %Ecto.Query.BooleanExpr{ + op: :and, + expr: + {:in, [], + [ + {{:., [], [{:&, [], [0]}, :id]}, [], []}, + {:^, [], [1, 2]} + ]}, + params: [] + } + ] + } + + sql = Connection.all(query) |> IO.iodata_to_binary() + + assert sql =~ ~s["id" IN (?2, ?3)] + end + + test "empty bound IN list generates always-false sentinel" do + # Simulates: where(query, [u], u.id in ^[]) + # After Ecto planning, the empty list becomes {:^, _, [0, 0]}. + query = %Ecto.Query{ + from: %Ecto.Query.FromExpr{source: {"users", nil}}, + sources: {{"users", nil, nil}}, + select: %Ecto.Query.SelectExpr{fields: [{:&, [], [0]}]}, + wheres: [ + %Ecto.Query.BooleanExpr{ + op: :and, + expr: + {:in, [], + [ + {{:., [], [{:&, [], [0]}, :id]}, [], []}, + {:^, [], [0, 0]} + ]}, + params: [] + } + ] + } + + sql = Connection.all(query) |> IO.iodata_to_binary() + + # SQLite rejects IN () so the zero-length guard emits an always-false subquery. + assert sql =~ ~s["id" IN (SELECT NULL WHERE 1=0)] + end + end + describe "on_conflict insert" do test "generates INSERT with ON CONFLICT DO NOTHING (no target)" do sql = @@ -595,7 +677,7 @@ defmodule Ecto.Adapters.LibSql.ConnectionTest do assert sql =~ ~s[INSERT INTO "users"] assert sql =~ ~s[("name", "email")] - assert sql =~ "VALUES (?, ?)" + assert sql =~ "VALUES (?1, ?2)" assert sql =~ "ON CONFLICT DO NOTHING" end @@ -614,7 +696,7 @@ defmodule Ecto.Adapters.LibSql.ConnectionTest do assert sql =~ ~s[INSERT INTO "users"] assert sql =~ ~s[("name", "email")] - assert sql =~ "VALUES (?, ?)" + assert sql =~ "VALUES (?1, ?2)" assert sql =~ ~s[ON CONFLICT ("email") DO NOTHING] end @@ -633,7 +715,7 @@ defmodule Ecto.Adapters.LibSql.ConnectionTest do assert sql =~ ~s[INSERT INTO "locations"] assert sql =~ ~s[("slug", "parent_slug", "name")] - assert sql =~ "VALUES (?, ?, ?)" + assert sql =~ "VALUES (?1, ?2, ?3)" assert sql =~ ~s[ON CONFLICT ("slug", "parent_slug") DO NOTHING] end @@ -652,7 +734,7 @@ defmodule Ecto.Adapters.LibSql.ConnectionTest do assert sql =~ ~s[INSERT INTO "users"] assert sql =~ ~s[("name", "email")] - assert sql =~ "VALUES (?, ?)" + assert sql =~ "VALUES (?1, ?2)" assert sql =~ ~s[ON CONFLICT ("email") DO UPDATE SET] assert sql =~ ~s["name" = excluded."name"] assert sql =~ ~s["email" = excluded."email"] @@ -673,7 +755,7 @@ defmodule Ecto.Adapters.LibSql.ConnectionTest do assert sql =~ ~s[INSERT INTO "users"] assert sql =~ ~s[("name", "email", "updated_at")] - assert sql =~ "VALUES (?, ?, ?)" + assert sql =~ "VALUES (?1, ?2, ?3)" assert sql =~ ~s[ON CONFLICT ("email") DO UPDATE SET] assert sql =~ ~s["name" = excluded."name"] assert sql =~ ~s["updated_at" = excluded."updated_at"] @@ -687,7 +769,7 @@ defmodule Ecto.Adapters.LibSql.ConnectionTest do assert sql =~ ~s[INSERT INTO "users"] assert sql =~ ~s[("name", "email")] - assert sql =~ "VALUES (?, ?)" + assert sql =~ "VALUES (?1, ?2)" refute sql =~ "ON CONFLICT" end @@ -706,7 +788,7 @@ defmodule Ecto.Adapters.LibSql.ConnectionTest do assert sql =~ ~s[INSERT INTO "users"] assert sql =~ ~s[("name", "email")] - assert sql =~ "VALUES (?, ?)" + assert sql =~ "VALUES (?1, ?2)" refute sql =~ "ON CONFLICT" end @@ -737,7 +819,7 @@ defmodule Ecto.Adapters.LibSql.ConnectionTest do assert sql =~ ~s[INSERT INTO "users"] assert sql =~ ~s[("name", "email")] - assert sql =~ "VALUES (?, ?)" + assert sql =~ "VALUES (?1, ?2)" assert sql =~ ~s[ON CONFLICT ("email") DO UPDATE SET] assert sql =~ ~s["name" = 'updated_name'] end diff --git a/test/ecto_integration_test.exs b/test/ecto_integration_test.exs index 56d4eab..fa86733 100644 --- a/test/ecto_integration_test.exs +++ b/test/ecto_integration_test.exs @@ -934,6 +934,78 @@ defmodule Ecto.Integration.EctoLibSqlTest do end end + describe "upsert via Repo.insert with on_conflict" do + # These tests exercise the bug fixed in PR #95: upsert queries were generating + # invalid SQL ("near ?: syntax error") because the adapter used bare ? placeholders + # instead of the numbered ?1, ?2, ... form that SQLite requires when a statement + # contains multiple parameter groups (INSERT values + ON CONFLICT UPDATE). + + test "Repo.insert with on_conflict: :replace_all upserts an existing record" do + {:ok, _user} = + TestRepo.insert(%User{name: "Alice", email: "alice@example.com"}, + on_conflict: :replace_all, + conflict_target: :email + ) + + {:ok, _updated} = + TestRepo.insert(%User{name: "Alice Updated", email: "alice@example.com"}, + on_conflict: :replace_all, + conflict_target: :email + ) + + # Fetch from the DB to confirm the change was actually persisted, not just + # reflected in the returned struct. + reloaded = TestRepo.get_by!(User, email: "alice@example.com") + assert reloaded.name == "Alice Updated" + assert TestRepo.aggregate(User, :count, :id) == 1 + end + + test "Repo.insert with on_conflict: :nothing leaves existing record unchanged" do + {:ok, original} = + TestRepo.insert(%User{name: "Bob", email: "bob@example.com"}) + + {:ok, _ignored} = + TestRepo.insert(%User{name: "Bob Changed", email: "bob@example.com"}, + on_conflict: :nothing, + conflict_target: :email + ) + + reloaded = TestRepo.get_by!(User, email: "bob@example.com") + assert reloaded.id == original.id + assert reloaded.name == "Bob" + end + + test "Repo.insert with on_conflict: {:replace, fields} updates only specified fields" do + {:ok, original} = + TestRepo.insert(%User{name: "Charlie", email: "charlie@example.com", age: 25}) + + {:ok, _result} = + TestRepo.insert(%User{name: "Charlie New Name", email: "charlie@example.com", age: 99}, + on_conflict: {:replace, [:name]}, + conflict_target: :email + ) + + reloaded = TestRepo.get_by!(User, email: "charlie@example.com") + # Name should be updated, age should remain unchanged. + assert reloaded.id == original.id + assert reloaded.name == "Charlie New Name" + assert reloaded.age == 25 + end + + test "Repo.insert with on_conflict: :replace_all inserts a new record when no conflict" do + assert TestRepo.aggregate(User, :count, :id) == 0 + + {:ok, user} = + TestRepo.insert(%User{name: "Dave", email: "dave@example.com"}, + on_conflict: :replace_all, + conflict_target: :email + ) + + assert user.id != nil + assert TestRepo.aggregate(User, :count, :id) == 1 + end + end + # Helper function to extract errors from changeset defp errors_on(changeset) do Ecto.Changeset.traverse_errors(changeset, fn {msg, opts} ->