Merge pull request #95 from AlanMcCann/fix/upsert-on-conflict-identifier

Fix upsert ON CONFLICT: missing :identifier expr + numbered parameters

Author: ocean

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

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, ", ", &quote_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

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

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} ->