From b89a569db8edd2e163a6228aaaa817055d8a7da7 Mon Sep 17 00:00:00 2001 From: Matthew Johnston Date: Sun, 20 Sep 2026 10:33:24 -0500 Subject: [PATCH 1/3] Parenthesize SQL expressions with lower precedence SQLite binds `||` tighter than `+`, and `IS`, `IN`, and `=` tighter than `NOT` and `AND`. The generated SQL for `datetime_add`, `is_nil`, `not`, `in`, and update inc dropped parentheses around compound operands, so counts like `a + b` became `NULL`. Fixes: #181 --- lib/ecto/adapters/sqlite3/connection.ex | 49 +++++++++-------- .../sqlite3/connection/datetime_add_test.exs | 44 +++++++++++++++ .../sqlite3/connection/select_test.exs | 54 +++++++++++++++++-- .../sqlite3/connection/update_all_test.exs | 8 +++ 4 files changed, 125 insertions(+), 30 deletions(-) diff --git a/lib/ecto/adapters/sqlite3/connection.ex b/lib/ecto/adapters/sqlite3/connection.ex index 4a5aca8..ad3d938 100644 --- a/lib/ecto/adapters/sqlite3/connection.ex +++ b/lib/ecto/adapters/sqlite3/connection.ex @@ -860,7 +860,7 @@ defmodule Ecto.Adapters.SQLite3.Connection do quoted_key, " = ", quoted_key, - " + " | expr(value, sources, query) + " + " | maybe_paren(value, sources, query) ] end @@ -1189,7 +1189,7 @@ defmodule Ecto.Adapters.SQLite3.Connection do defp expr({:in, _, [left, right]}, sources, query) when is_list(right) do args = Enum.map_intersperse(right, ?,, &expr(&1, sources, query)) - [expr(left, sources, query), " IN (", args, ?)] + [maybe_paren(left, sources, query), " IN (", args, ?)] end defp expr({:in, _, [_, {:^, _, [_, 0]}]}, _sources, _query) do @@ -1198,17 +1198,17 @@ defmodule Ecto.Adapters.SQLite3.Connection do defp expr({:in, _, [left, {:^, _, [_, len]}]}, sources, query) do args = Enum.intersperse(List.duplicate(??, len), ?,) - [expr(left, sources, query), " IN (", args, ?)] + [maybe_paren(left, sources, query), " IN (", args, ?)] end defp expr({:in, _, [left, %Ecto.SubQuery{} = subquery]}, sources, query) do - [expr(left, sources, query), " IN ", expr(subquery, sources, query)] + [maybe_paren(left, sources, query), " IN ", expr(subquery, sources, query)] end # Super Hack to handle arrays in json defp expr({:in, _, [left, right]}, sources, query) do [ - expr(left, sources, query), + maybe_paren(left, sources, query), " IN (SELECT value FROM JSON_EACH(", expr(right, sources, query), ?), @@ -1217,7 +1217,7 @@ defmodule Ecto.Adapters.SQLite3.Connection do end defp expr({:is_nil, _, [arg]}, sources, query) do - [expr(arg, sources, query) | " IS NULL"] + [maybe_paren(arg, sources, query) | " IS NULL"] end defp expr({:not, _, [expression]}, sources, query) do @@ -1287,7 +1287,7 @@ defmodule Ecto.Adapters.SQLite3.Connection do ",", expr(datetime, sources, query), ",", - interval(count, interval, sources), + interval(count, interval, sources, query), ") AS TEXT)" ] end @@ -1299,7 +1299,7 @@ defmodule Ecto.Adapters.SQLite3.Connection do ",", expr(date, sources, query), ",", - interval(count, interval, sources), + interval(count, interval, sources, query), ") AS TEXT)" ] end @@ -1482,34 +1482,25 @@ defmodule Ecto.Adapters.SQLite3.Connection do |> parens_for_select() end - def interval(_, "microsecond", _sources) do + defp interval(_, "microsecond", _sources, _query) do raise ArgumentError, "SQLite does not support microsecond precision in datetime intervals" end - def interval(count, "millisecond", sources) do - "(#{expr(count, sources, nil)} / 1000.0) || ' seconds'" + defp interval(count, "millisecond", sources, query) do + [?(, maybe_paren(count, sources, query), " / 1000.0) || ' seconds'"] end - def interval(count, "week", sources) do - "(#{expr(count, sources, nil)} * 7) || ' days'" + defp interval(count, "week", sources, query) do + [?(, maybe_paren(count, sources, query), " * 7) || ' days'"] end - def interval(count, interval, sources) do - "#{expr(count, sources, nil)} || ' #{interval}'" - end - - defp op_to_binary({op, _, [_, _]} = expression, sources, query) - when op in @binary_ops do - paren_expr(expression, sources, query) - end - - defp op_to_binary({:is_nil, _, [_]} = expression, sources, query) do - paren_expr(expression, sources, query) + defp interval(count, interval, sources, query) do + [maybe_paren(count, sources, query), " || ' ", interval, "'"] end defp op_to_binary(expression, sources, query) do - expr(expression, sources, query) + maybe_paren(expression, sources, query) end def create_names(query) do @@ -1767,6 +1758,14 @@ defmodule Ecto.Adapters.SQLite3.Connection do paren_expr(expr, sources, query) end + defp maybe_paren({:not, _, [_]} = expr, sources, query) do + paren_expr(expr, sources, query) + end + + defp maybe_paren({:in, _, [_, _]} = expr, sources, query) do + paren_expr(expr, sources, query) + end + defp maybe_paren(expr, sources, query) do expr(expr, sources, query) end diff --git a/test/ecto/adapters/sqlite3/connection/datetime_add_test.exs b/test/ecto/adapters/sqlite3/connection/datetime_add_test.exs index 13aa851..048110a 100644 --- a/test/ecto/adapters/sqlite3/connection/datetime_add_test.exs +++ b/test/ecto/adapters/sqlite3/connection/datetime_add_test.exs @@ -25,4 +25,48 @@ defmodule Ecto.Adapters.SQLite3.Connection.DatetimeAddTest do assert ~s{SELECT 1 FROM "schema" AS s0 WHERE (CAST (strftime('%Y-%m-%dT%H:%M:%f000Z',CAST(s0.\"foo\" AS TEXT),1 || ' month') AS TEXT) > s0."bar")} == all(query) end + + test "parenthesizes compound day count" do + query = + "schema" + |> where([s], datetime_add(s.foo, s.x + s.y, "day") > s.bar) + |> select([], true) + |> plan() + + assert ~s{SELECT 1 FROM "schema" AS s0 WHERE (CAST (strftime('%Y-%m-%dT%H:%M:%f000Z',s0.\"foo\",(s0.\"x\" + s0.\"y\") || ' day') AS TEXT) > s0."bar")} == + all(query) + end + + test "parenthesizes compound week count" do + query = + "schema" + |> where([s], datetime_add(s.foo, s.x + s.y, "week") > s.bar) + |> select([], true) + |> plan() + + assert ~s{SELECT 1 FROM "schema" AS s0 WHERE (CAST (strftime('%Y-%m-%dT%H:%M:%f000Z',s0.\"foo\",((s0.\"x\" + s0.\"y\") * 7) || ' days') AS TEXT) > s0."bar")} == + all(query) + end + + test "parenthesizes compound millisecond count" do + query = + "schema" + |> where([s], datetime_add(s.foo, s.x + s.y, "millisecond") > s.bar) + |> select([], true) + |> plan() + + assert ~s{SELECT 1 FROM "schema" AS s0 WHERE (CAST (strftime('%Y-%m-%dT%H:%M:%f000Z',s0.\"foo\",((s0.\"x\" + s0.\"y\") / 1000.0) || ' seconds') AS TEXT) > s0."bar")} == + all(query) + end + + test "date_add parenthesizes compound day count" do + query = + "schema" + |> where([s], date_add(s.foo, s.x + s.y, "day") > s.bar) + |> select([], true) + |> plan() + + assert ~s{SELECT 1 FROM "schema" AS s0 WHERE (CAST (strftime('%Y-%m-%d',s0.\"foo\",(s0.\"x\" + s0.\"y\") || ' day') AS TEXT) > s0."bar")} == + all(query) + end end diff --git a/test/ecto/adapters/sqlite3/connection/select_test.exs b/test/ecto/adapters/sqlite3/connection/select_test.exs index 358e4b6..721e913 100644 --- a/test/ecto/adapters/sqlite3/connection/select_test.exs +++ b/test/ecto/adapters/sqlite3/connection/select_test.exs @@ -308,13 +308,40 @@ defmodule Ecto.Adapters.SQLite3.Connection.SelectTest do test "is_nil with comparison" do query = - "schema" + Schema |> select([r], r.x == is_nil(r.y)) |> plan() assert ~s{SELECT s0."x" = (s0."y" IS NULL) FROM "schema" AS s0} == all(query) end + test "is_nil parenthesizes boolean and" do + query = + Schema + |> select([r], is_nil(r.x and r.y)) + |> plan() + + assert ~s{SELECT (s0."x" AND s0."y") IS NULL FROM "schema" AS s0} == all(query) + end + + test "is_nil parenthesizes not" do + query = + Schema + |> select([r], is_nil(not r.x)) + |> plan() + + assert ~s{SELECT (NOT (s0."x")) IS NULL FROM "schema" AS s0} == all(query) + end + + test "not parenthesizes when compared" do + query = + Schema + |> select([r], not r.x < r.y) + |> plan() + + assert ~s{SELECT (NOT (s0."x")) < s0."y" FROM "schema" AS s0} == all(query) + end + describe "casting" do test "as integer" do query = @@ -403,10 +430,8 @@ defmodule Ecto.Adapters.SQLite3.Connection.SelectTest do |> select([e], e.x == ^0 or e.x in ^[1, 2, 3] or e.x == ^4) |> plan() - assert ~s{SELECT (} <> - ~s{(s0."x" = ?) OR s0."x" IN (?,?,?)} <> - ~s{) OR (s0."x" = ?) } <> - ~s{FROM "schema" AS s0} == all(query) + assert ~s{SELECT ((s0."x" = ?) OR (s0."x" IN (?,?,?))) OR (s0."x" = ?) FROM "schema" AS s0} == + all(query) end test "json each" do @@ -436,6 +461,25 @@ defmodule Ecto.Adapters.SQLite3.Connection.SelectTest do assert all(query) == ~s{SELECT ? IN (1,?,3) FROM "schema" AS s0} end + + test "parenthesizes not on the left" do + query = + Schema + |> select([e], (not e.x) in [true, false]) + |> plan() + + assert ~s{SELECT (NOT (s0."x")) IN (SELECT value FROM JSON_EACH('[true,false]')) FROM "schema" AS s0} == + all(query) + end + + test "parenthesizes in when compared" do + query = + Schema + |> select([e], true == e.x in ^[1, 2, 3]) + |> plan() + + assert ~s{SELECT 1 = (s0."x" IN (?,?,?)) FROM "schema" AS s0} == all(query) + end end test "in subquery" do diff --git a/test/ecto/adapters/sqlite3/connection/update_all_test.exs b/test/ecto/adapters/sqlite3/connection/update_all_test.exs index 7fcb8f3..60b23cd 100644 --- a/test/ecto/adapters/sqlite3/connection/update_all_test.exs +++ b/test/ecto/adapters/sqlite3/connection/update_all_test.exs @@ -24,6 +24,14 @@ defmodule Ecto.Adapters.SQLite3.Connection.UpdateAllTest do assert ~s{UPDATE "schema" AS s0 SET } <> ~s{"x" = 0, "y" = "y" + 1, "z" = "z" + -3} == update_all(query) + query = + from(m in Schema) + |> update([m], inc: [x: m.y + m.z]) + |> plan(:update_all) + + assert ~s{UPDATE "schema" AS s0 SET "x" = "x" + (s0."y" + s0."z")} == + update_all(query) + query = from(e in Schema) |> where([e], e.x == 123) From b095c32743be44e233fa6269b48a338596054fa1 Mon Sep 17 00:00:00 2001 From: Matthew Johnston Date: Sun, 20 Sep 2026 10:58:38 -0500 Subject: [PATCH 2/3] Make it more clear that we are maybe parenthesizing an expression --- lib/ecto/adapters/sqlite3/connection.ex | 33 +++++++++++++------------ 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/lib/ecto/adapters/sqlite3/connection.ex b/lib/ecto/adapters/sqlite3/connection.ex index ad3d938..9bbfc55 100644 --- a/lib/ecto/adapters/sqlite3/connection.ex +++ b/lib/ecto/adapters/sqlite3/connection.ex @@ -860,7 +860,7 @@ defmodule Ecto.Adapters.SQLite3.Connection do quoted_key, " = ", quoted_key, - " + " | maybe_paren(value, sources, query) + " + " | maybe_paren_expr(value, sources, query) ] end @@ -1189,7 +1189,7 @@ defmodule Ecto.Adapters.SQLite3.Connection do defp expr({:in, _, [left, right]}, sources, query) when is_list(right) do args = Enum.map_intersperse(right, ?,, &expr(&1, sources, query)) - [maybe_paren(left, sources, query), " IN (", args, ?)] + [maybe_paren_expr(left, sources, query), " IN (", args, ?)] end defp expr({:in, _, [_, {:^, _, [_, 0]}]}, _sources, _query) do @@ -1198,17 +1198,17 @@ defmodule Ecto.Adapters.SQLite3.Connection do defp expr({:in, _, [left, {:^, _, [_, len]}]}, sources, query) do args = Enum.intersperse(List.duplicate(??, len), ?,) - [maybe_paren(left, sources, query), " IN (", args, ?)] + [maybe_paren_expr(left, sources, query), " IN (", args, ?)] end defp expr({:in, _, [left, %Ecto.SubQuery{} = subquery]}, sources, query) do - [maybe_paren(left, sources, query), " IN ", expr(subquery, sources, query)] + [maybe_paren_expr(left, sources, query), " IN ", expr(subquery, sources, query)] end # Super Hack to handle arrays in json defp expr({:in, _, [left, right]}, sources, query) do [ - maybe_paren(left, sources, query), + maybe_paren_expr(left, sources, query), " IN (SELECT value FROM JSON_EACH(", expr(right, sources, query), ?), @@ -1217,7 +1217,7 @@ defmodule Ecto.Adapters.SQLite3.Connection do end defp expr({:is_nil, _, [arg]}, sources, query) do - [maybe_paren(arg, sources, query) | " IS NULL"] + [maybe_paren_expr(arg, sources, query) | " IS NULL"] end defp expr({:not, _, [expression]}, sources, query) do @@ -1477,7 +1477,7 @@ defmodule Ecto.Adapters.SQLite3.Connection do defp fragment_expr(parts, sources, query) do Enum.map(parts, fn {:raw, part} -> part - {:expr, expr} -> maybe_paren(expr, sources, query) + {:expr, expr} -> maybe_paren_expr(expr, sources, query) end) |> parens_for_select() end @@ -1488,19 +1488,19 @@ defmodule Ecto.Adapters.SQLite3.Connection do end defp interval(count, "millisecond", sources, query) do - [?(, maybe_paren(count, sources, query), " / 1000.0) || ' seconds'"] + [?(, maybe_paren_expr(count, sources, query), " / 1000.0) || ' seconds'"] end defp interval(count, "week", sources, query) do - [?(, maybe_paren(count, sources, query), " * 7) || ' days'"] + [?(, maybe_paren_expr(count, sources, query), " * 7) || ' days'"] end defp interval(count, interval, sources, query) do - [maybe_paren(count, sources, query), " || ' ", interval, "'"] + [maybe_paren_expr(count, sources, query), " || ' ", interval, "'"] end defp op_to_binary(expression, sources, query) do - maybe_paren(expression, sources, query) + maybe_paren_expr(expression, sources, query) end def create_names(query) do @@ -1750,23 +1750,24 @@ defmodule Ecto.Adapters.SQLite3.Connection do defp reference_on_update(:restrict), do: " ON UPDATE RESTRICT" defp reference_on_update(_), do: [] - defp maybe_paren({op, _, [_, _]} = expr, sources, query) when op in @binary_ops do + defp maybe_paren_expr({op, _, [_, _]} = expr, sources, query) + when op in @binary_ops do paren_expr(expr, sources, query) end - defp maybe_paren({:is_nil, _, [_]} = expr, sources, query) do + defp maybe_paren_expr({:is_nil, _, [_]} = expr, sources, query) do paren_expr(expr, sources, query) end - defp maybe_paren({:not, _, [_]} = expr, sources, query) do + defp maybe_paren_expr({:not, _, [_]} = expr, sources, query) do paren_expr(expr, sources, query) end - defp maybe_paren({:in, _, [_, _]} = expr, sources, query) do + defp maybe_paren_expr({:in, _, [_, _]} = expr, sources, query) do paren_expr(expr, sources, query) end - defp maybe_paren(expr, sources, query) do + defp maybe_paren_expr(expr, sources, query) do expr(expr, sources, query) end From 65a9efb67b7d280b5ba9f1af993f57a8e0c4902f Mon Sep 17 00:00:00 2001 From: Matthew Johnston Date: Mon, 21 Sep 2026 07:45:22 -0500 Subject: [PATCH 3/3] Add more tests --- integration_test/precedence_test.exs | 63 +++++++++++++++++++ .../sqlite3/connection/select_test.exs | 24 +++++++ 2 files changed, 87 insertions(+) create mode 100644 integration_test/precedence_test.exs diff --git a/integration_test/precedence_test.exs b/integration_test/precedence_test.exs new file mode 100644 index 0000000..0db9a25 --- /dev/null +++ b/integration_test/precedence_test.exs @@ -0,0 +1,63 @@ +defmodule Ecto.Integration.PrecedenceTest do + use Ecto.Integration.Case, async: true + + alias Ecto.Integration.Post + alias Ecto.Integration.TestRepo + import Ecto.Query + + @posted ~D[2014-01-01] + @inserted_at ~N[2014-01-01 02:00:00] + + setup do + TestRepo.insert!(%Post{ + posted: @posted, + inserted_at: @inserted_at, + visits: 1, + counter: 2, + public: true + }) + + :ok + end + + test "datetime_add parenthesizes a compound day count" do + assert [~N[2014-01-04 02:00:00]] = + TestRepo.all( + from(p in Post, select: datetime_add(p.inserted_at, p.visits + p.counter, "day")) + ) + end + + test "datetime_add parenthesizes a compound week count" do + assert [~N[2014-01-22 02:00:00]] = + TestRepo.all( + from(p in Post, select: datetime_add(p.inserted_at, p.visits + p.counter, "week")) + ) + end + + test "datetime_add parenthesizes a compound millisecond count" do + TestRepo.delete_all(Post) + + TestRepo.insert!(%Post{ + posted: @posted, + inserted_at: @inserted_at, + visits: 500, + counter: 500 + }) + + assert [~N[2014-01-01 02:00:01]] = + TestRepo.all( + from(p in Post, + select: datetime_add(p.inserted_at, p.visits + p.counter, "millisecond") + ) + ) + end + + test "date_add parenthesizes a compound day count" do + assert [~D[2014-01-04]] = + TestRepo.all(from(p in Post, select: date_add(p.posted, p.visits + p.counter, "day"))) + end + + test "is_nil parenthesizes not" do + assert [false] = TestRepo.all(from(p in Post, select: is_nil(not p.public))) + end +end diff --git a/test/ecto/adapters/sqlite3/connection/select_test.exs b/test/ecto/adapters/sqlite3/connection/select_test.exs index 721e913..fb110d6 100644 --- a/test/ecto/adapters/sqlite3/connection/select_test.exs +++ b/test/ecto/adapters/sqlite3/connection/select_test.exs @@ -480,6 +480,16 @@ defmodule Ecto.Adapters.SQLite3.Connection.SelectTest do assert ~s{SELECT 1 = (s0."x" IN (?,?,?)) FROM "schema" AS s0} == all(query) end + + test "parenthesizes equality on the left of in" do + query = + Schema + |> select([e], (e.x == e.y) in [true, false]) + |> plan() + + assert ~s{SELECT (s0."x" = s0."y") IN (1,0) FROM "schema" AS s0} == + all(query) + end end test "in subquery" do @@ -515,6 +525,20 @@ defmodule Ecto.Adapters.SQLite3.Connection.SelectTest do ~s{))} == all(query) end + test "parenthesizes addition on the left of in subquery" do + posts = subquery("posts" |> where(title: ^"hello") |> select([p], p.id)) + + query = + "comments" + |> where([c], (c.x + c.y) in subquery(posts)) + |> select([c], c.x) + |> plan() + + assert all(query) == + ~s{SELECT c0."x" FROM "comments" AS c0 } <> + ~s{WHERE ((c0."x" + c0."y") IN (SELECT sp0."id" FROM "posts" AS sp0 WHERE (sp0."title" = ?)))} + end + describe "arrays" do test "array of integers fragment is not supported" do assert_raise Ecto.QueryError, fn ->