Attempt to fix precendence issue with datetime_add (#182)

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

Author: warmwaffles

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

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_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))
-    [expr(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), ?,)
-    [expr(left, sources, query), " IN (", args, ?)]
+    [maybe_paren_expr(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_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
     [
-      expr(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
-    [expr(arg, sources, query) | " IS NULL"]
+    [maybe_paren_expr(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
@@ -1477,39 +1477,30 @@ 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
 
-  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_expr(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_expr(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_expr(count, sources, query), " || ' ", interval, "'"]
   end
 
   defp op_to_binary(expression, sources, query) do
-    expr(expression, sources, query)
+    maybe_paren_expr(expression, sources, query)
   end
 
   def create_names(query) do
@@ -1759,15 +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_expr({:is_nil, _, [_]} = expr, sources, query) do
+    paren_expr(expr, sources, query)
+  end
+
+  defp maybe_paren_expr({:not, _, [_]} = expr, sources, query) do
     paren_expr(expr, sources, query)
   end
 
-  defp maybe_paren({:is_nil, _, [_]} = 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
 

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

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,35 @@ 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
+
+    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
@@ -471,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 ->

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)