Skip to content

Commit b89a569

Browse files
warmwaffleswarmwaffles
committed
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
1 parent 23dd28d commit b89a569

4 files changed

Lines changed: 125 additions & 30 deletions

File tree

‎lib/ecto/adapters/sqlite3/connection.ex‎

Lines changed: 24 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -860,7 +860,7 @@ defmodule Ecto.Adapters.SQLite3.Connection do
860860
quoted_key,
861861
" = ",
862862
quoted_key,
863-
" + " | expr(value, sources, query)
863+
" + " | maybe_paren(value, sources, query)
864864
]
865865
end
866866

@@ -1189,7 +1189,7 @@ defmodule Ecto.Adapters.SQLite3.Connection do
11891189

11901190
defp expr({:in, _, [left, right]}, sources, query) when is_list(right) do
11911191
args = Enum.map_intersperse(right, ?,, &expr(&1, sources, query))
1192-
[expr(left, sources, query), " IN (", args, ?)]
1192+
[maybe_paren(left, sources, query), " IN (", args, ?)]
11931193
end
11941194

11951195
defp expr({:in, _, [_, {:^, _, [_, 0]}]}, _sources, _query) do
@@ -1198,17 +1198,17 @@ defmodule Ecto.Adapters.SQLite3.Connection do
11981198

11991199
defp expr({:in, _, [left, {:^, _, [_, len]}]}, sources, query) do
12001200
args = Enum.intersperse(List.duplicate(??, len), ?,)
1201-
[expr(left, sources, query), " IN (", args, ?)]
1201+
[maybe_paren(left, sources, query), " IN (", args, ?)]
12021202
end
12031203

12041204
defp expr({:in, _, [left, %Ecto.SubQuery{} = subquery]}, sources, query) do
1205-
[expr(left, sources, query), " IN ", expr(subquery, sources, query)]
1205+
[maybe_paren(left, sources, query), " IN ", expr(subquery, sources, query)]
12061206
end
12071207

12081208
# Super Hack to handle arrays in json
12091209
defp expr({:in, _, [left, right]}, sources, query) do
12101210
[
1211-
expr(left, sources, query),
1211+
maybe_paren(left, sources, query),
12121212
" IN (SELECT value FROM JSON_EACH(",
12131213
expr(right, sources, query),
12141214
?),
@@ -1217,7 +1217,7 @@ defmodule Ecto.Adapters.SQLite3.Connection do
12171217
end
12181218

12191219
defp expr({:is_nil, _, [arg]}, sources, query) do
1220-
[expr(arg, sources, query) | " IS NULL"]
1220+
[maybe_paren(arg, sources, query) | " IS NULL"]
12211221
end
12221222

12231223
defp expr({:not, _, [expression]}, sources, query) do
@@ -1287,7 +1287,7 @@ defmodule Ecto.Adapters.SQLite3.Connection do
12871287
",",
12881288
expr(datetime, sources, query),
12891289
",",
1290-
interval(count, interval, sources),
1290+
interval(count, interval, sources, query),
12911291
") AS TEXT)"
12921292
]
12931293
end
@@ -1299,7 +1299,7 @@ defmodule Ecto.Adapters.SQLite3.Connection do
12991299
",",
13001300
expr(date, sources, query),
13011301
",",
1302-
interval(count, interval, sources),
1302+
interval(count, interval, sources, query),
13031303
") AS TEXT)"
13041304
]
13051305
end
@@ -1482,34 +1482,25 @@ defmodule Ecto.Adapters.SQLite3.Connection do
14821482
|> parens_for_select()
14831483
end
14841484

1485-
def interval(_, "microsecond", _sources) do
1485+
defp interval(_, "microsecond", _sources, _query) do
14861486
raise ArgumentError,
14871487
"SQLite does not support microsecond precision in datetime intervals"
14881488
end
14891489

1490-
def interval(count, "millisecond", sources) do
1491-
"(#{expr(count, sources, nil)} / 1000.0) || ' seconds'"
1490+
defp interval(count, "millisecond", sources, query) do
1491+
[?(, maybe_paren(count, sources, query), " / 1000.0) || ' seconds'"]
14921492
end
14931493

1494-
def interval(count, "week", sources) do
1495-
"(#{expr(count, sources, nil)} * 7) || ' days'"
1494+
defp interval(count, "week", sources, query) do
1495+
[?(, maybe_paren(count, sources, query), " * 7) || ' days'"]
14961496
end
14971497

1498-
def interval(count, interval, sources) do
1499-
"#{expr(count, sources, nil)} || ' #{interval}'"
1500-
end
1501-
1502-
defp op_to_binary({op, _, [_, _]} = expression, sources, query)
1503-
when op in @binary_ops do
1504-
paren_expr(expression, sources, query)
1505-
end
1506-
1507-
defp op_to_binary({:is_nil, _, [_]} = expression, sources, query) do
1508-
paren_expr(expression, sources, query)
1498+
defp interval(count, interval, sources, query) do
1499+
[maybe_paren(count, sources, query), " || ' ", interval, "'"]
15091500
end
15101501

15111502
defp op_to_binary(expression, sources, query) do
1512-
expr(expression, sources, query)
1503+
maybe_paren(expression, sources, query)
15131504
end
15141505

15151506
def create_names(query) do
@@ -1767,6 +1758,14 @@ defmodule Ecto.Adapters.SQLite3.Connection do
17671758
paren_expr(expr, sources, query)
17681759
end
17691760

1761+
defp maybe_paren({:not, _, [_]} = expr, sources, query) do
1762+
paren_expr(expr, sources, query)
1763+
end
1764+
1765+
defp maybe_paren({:in, _, [_, _]} = expr, sources, query) do
1766+
paren_expr(expr, sources, query)
1767+
end
1768+
17701769
defp maybe_paren(expr, sources, query) do
17711770
expr(expr, sources, query)
17721771
end

‎test/ecto/adapters/sqlite3/connection/datetime_add_test.exs‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,4 +25,48 @@ defmodule Ecto.Adapters.SQLite3.Connection.DatetimeAddTest do
2525
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")} ==
2626
all(query)
2727
end
28+
29+
test "parenthesizes compound day count" do
30+
query =
31+
"schema"
32+
|> where([s], datetime_add(s.foo, s.x + s.y, "day") > s.bar)
33+
|> select([], true)
34+
|> plan()
35+
36+
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")} ==
37+
all(query)
38+
end
39+
40+
test "parenthesizes compound week count" do
41+
query =
42+
"schema"
43+
|> where([s], datetime_add(s.foo, s.x + s.y, "week") > s.bar)
44+
|> select([], true)
45+
|> plan()
46+
47+
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")} ==
48+
all(query)
49+
end
50+
51+
test "parenthesizes compound millisecond count" do
52+
query =
53+
"schema"
54+
|> where([s], datetime_add(s.foo, s.x + s.y, "millisecond") > s.bar)
55+
|> select([], true)
56+
|> plan()
57+
58+
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")} ==
59+
all(query)
60+
end
61+
62+
test "date_add parenthesizes compound day count" do
63+
query =
64+
"schema"
65+
|> where([s], date_add(s.foo, s.x + s.y, "day") > s.bar)
66+
|> select([], true)
67+
|> plan()
68+
69+
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")} ==
70+
all(query)
71+
end
2872
end

‎test/ecto/adapters/sqlite3/connection/select_test.exs‎

Lines changed: 49 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -308,13 +308,40 @@ defmodule Ecto.Adapters.SQLite3.Connection.SelectTest do
308308

309309
test "is_nil with comparison" do
310310
query =
311-
"schema"
311+
Schema
312312
|> select([r], r.x == is_nil(r.y))
313313
|> plan()
314314

315315
assert ~s{SELECT s0."x" = (s0."y" IS NULL) FROM "schema" AS s0} == all(query)
316316
end
317317

318+
test "is_nil parenthesizes boolean and" do
319+
query =
320+
Schema
321+
|> select([r], is_nil(r.x and r.y))
322+
|> plan()
323+
324+
assert ~s{SELECT (s0."x" AND s0."y") IS NULL FROM "schema" AS s0} == all(query)
325+
end
326+
327+
test "is_nil parenthesizes not" do
328+
query =
329+
Schema
330+
|> select([r], is_nil(not r.x))
331+
|> plan()
332+
333+
assert ~s{SELECT (NOT (s0."x")) IS NULL FROM "schema" AS s0} == all(query)
334+
end
335+
336+
test "not parenthesizes when compared" do
337+
query =
338+
Schema
339+
|> select([r], not r.x < r.y)
340+
|> plan()
341+
342+
assert ~s{SELECT (NOT (s0."x")) < s0."y" FROM "schema" AS s0} == all(query)
343+
end
344+
318345
describe "casting" do
319346
test "as integer" do
320347
query =
@@ -403,10 +430,8 @@ defmodule Ecto.Adapters.SQLite3.Connection.SelectTest do
403430
|> select([e], e.x == ^0 or e.x in ^[1, 2, 3] or e.x == ^4)
404431
|> plan()
405432

406-
assert ~s{SELECT (} <>
407-
~s{(s0."x" = ?) OR s0."x" IN (?,?,?)} <>
408-
~s{) OR (s0."x" = ?) } <>
409-
~s{FROM "schema" AS s0} == all(query)
433+
assert ~s{SELECT ((s0."x" = ?) OR (s0."x" IN (?,?,?))) OR (s0."x" = ?) FROM "schema" AS s0} ==
434+
all(query)
410435
end
411436

412437
test "json each" do
@@ -436,6 +461,25 @@ defmodule Ecto.Adapters.SQLite3.Connection.SelectTest do
436461

437462
assert all(query) == ~s{SELECT ? IN (1,?,3) FROM "schema" AS s0}
438463
end
464+
465+
test "parenthesizes not on the left" do
466+
query =
467+
Schema
468+
|> select([e], (not e.x) in [true, false])
469+
|> plan()
470+
471+
assert ~s{SELECT (NOT (s0."x")) IN (SELECT value FROM JSON_EACH('[true,false]')) FROM "schema" AS s0} ==
472+
all(query)
473+
end
474+
475+
test "parenthesizes in when compared" do
476+
query =
477+
Schema
478+
|> select([e], true == e.x in ^[1, 2, 3])
479+
|> plan()
480+
481+
assert ~s{SELECT 1 = (s0."x" IN (?,?,?)) FROM "schema" AS s0} == all(query)
482+
end
439483
end
440484

441485
test "in subquery" do

‎test/ecto/adapters/sqlite3/connection/update_all_test.exs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,14 @@ defmodule Ecto.Adapters.SQLite3.Connection.UpdateAllTest do
2424
assert ~s{UPDATE "schema" AS s0 SET } <>
2525
~s{"x" = 0, "y" = "y" + 1, "z" = "z" + -3} == update_all(query)
2626

27+
query =
28+
from(m in Schema)
29+
|> update([m], inc: [x: m.y + m.z])
30+
|> plan(:update_all)
31+
32+
assert ~s{UPDATE "schema" AS s0 SET "x" = "x" + (s0."y" + s0."z")} ==
33+
update_all(query)
34+
2735
query =
2836
from(e in Schema)
2937
|> where([e], e.x == 123)

0 commit comments

Comments
 (0)