Support pre_comment and post_comment options in ecto.

Author: alesasnouski

integration_test/sql/logging.exs

@@ -178,46 +178,58 @@ defmodule Ecto.Integration.LoggingTest do
     end
   end
 
-  describe ":label option" do
-    test "prepends a leading comment to query operations" do
+  describe ":comments option" do
+    test "comments query operations" do
       assert capture_log(fn ->
-               TestRepo.all(Post, label: "list_posts_q", log: :error)
+               TestRepo.all(Post, comments: [pre: "list_posts_q"], log: :error)
              end) =~ "/* list_posts_q */ SELECT"
 
       assert capture_log(fn ->
-               TestRepo.update_all(Post, [set: [visits: 0]], label: "reset_visits_q", log: :error)
+               TestRepo.update_all(Post, [set: [visits: 0]], comments: [pre: "reset_visits_q"], log: :error)
              end) =~ "/* reset_visits_q */ UPDATE"
 
       assert capture_log(fn ->
-               TestRepo.delete_all(Post, label: "purge_posts_q", log: :error)
+               TestRepo.delete_all(Post, comments: [pre: "purge_posts_q"], log: :error)
              end) =~ "/* purge_posts_q */ DELETE"
     end
 
-    test "prepends a leading comment to insert/update/delete/insert_all" do
+    test "supports both :pre and :post" do
       assert capture_log(fn ->
-               TestRepo.insert!(%Post{title: "1"}, label: "insert_create_post_q", log: :error)
+               TestRepo.all(Post, comments: [pre: "before_q", post: "after_q"], log: :error)
+             end) =~ ~r{/\* before_q \*/ SELECT.* /\* after_q \*/}
+    end
+
+    test "renders with query_cache: false (the escape hatch for dynamic comments)" do
+      assert capture_log(fn ->
+               TestRepo.all(Post, comments: [pre: "dyn_#{System.unique_integer()}"], query_cache: false, log: :error)
+             end) =~ ~r{/\* dyn_-?\d+ \*/ SELECT}
+    end
+
+    test "comments insert/update/delete/insert_all" do
+      assert capture_log(fn ->
+               TestRepo.insert!(%Post{title: "1"}, comments: [pre: "insert_create_post_q"], log: :error)
              end) =~ "/* insert_create_post_q */ INSERT INTO"
 
       post = TestRepo.insert!(%Post{title: "x"})
 
       assert capture_log(fn ->
                post
                |> Ecto.Changeset.change(title: "y")
-               |> TestRepo.update!(label: "update_post_q", log: :error)
+               |> TestRepo.update!(comments: [pre: "update_post_q"], log: :error)
              end) =~ "/* update_post_q */ UPDATE"
 
       assert capture_log(fn ->
-               TestRepo.delete!(post, label: "delete_post_q", log: :error)
+               TestRepo.delete!(post, comments: [pre: "delete_post_q"], log: :error)
              end) =~ "/* delete_post_q */ DELETE"
 
       assert capture_log(fn ->
-               TestRepo.insert_all(Post, [%{title: "a"}], label: "bulk_insert_posts_q", log: :error)
+               TestRepo.insert_all(Post, [%{title: "a"}], comments: [pre: "bulk_insert_posts_q"], log: :error)
              end) =~ "/* bulk_insert_posts_q */ INSERT INTO"
     end
 
-    test "rejects a label that could break out of the comment block" do
+    test "rejects a comment that could break out of the comment block" do
       assert_raise ArgumentError, ~r/cannot contain/, fn ->
-        TestRepo.insert!(%Post{title: "1"}, label: "evil */ DROP TABLE posts")
+        TestRepo.insert!(%Post{title: "1"}, comments: [pre: "evil */ DROP TABLE posts"])
       end
     end
   end

lib/ecto/adapters/myxql.ex

@@ -381,8 +381,8 @@ defmodule Ecto.Adapters.MyXQL do
       end
 
     # This adapter overrides insert/6 instead of going through
-    # Ecto.Adapters.SQL.struct/10, so prepend the `:label` comment here too.
-    {sql, opts} = Ecto.Adapters.SQL.prepend_label(sql, opts)
+    # Ecto.Adapters.SQL.struct/10, so wrap the `:comments` here too.
+    sql = Ecto.Adapters.SQL.wrap_comments(sql, opts)
 
     case Ecto.Adapters.SQL.query(adapter_meta, sql, values ++ query_params, opts) do
       {:ok, %{num_rows: 0}} ->

lib/ecto/adapters/myxql/connection.ex

@@ -118,10 +118,10 @@ if Code.ensure_loaded?(MyXQL) do
       limit = limit(query, sources)
       offset = offset(query, sources)
       lock = lock(query, sources)
-      label = label(query)
+      {pre_comments, post_comments} = SQL.comments(query.comments)
 
       [
-        label,
+        pre_comments,
         cte,
         select,
         from,
@@ -133,7 +133,8 @@ if Code.ensure_loaded?(MyXQL) do
         combinations,
         order_by,
         limit,
-        offset | lock
+        offset,
+        lock | post_comments
       ]
     end
 
@@ -147,7 +148,7 @@ if Code.ensure_loaded?(MyXQL) do
 
       sources = create_names(query, [])
       cte = cte(query, sources)
-      label = label(query)
+      {pre_comments, post_comments} = SQL.comments(query.comments)
       {from, name} = get_source(query, sources, 0, source)
 
       fields =
@@ -161,7 +162,7 @@ if Code.ensure_loaded?(MyXQL) do
       prefix = prefix || ["UPDATE ", from, " AS ", name, join, " SET "]
       where = where(%{query | wheres: wheres ++ query.wheres}, sources)
 
-      [label, cte, prefix, fields | where]
+      [pre_comments, cte, prefix, fields, where | post_comments]
     end
 
     @impl true
@@ -174,12 +175,12 @@ if Code.ensure_loaded?(MyXQL) do
       cte = cte(query, sources)
       {_, name, _} = elem(sources, 0)
 
-      label = label(query)
+      {pre_comments, post_comments} = SQL.comments(query.comments)
       from = from(query, sources)
       join = join(query, sources)
       where = where(query, sources)
 
-      [label, cte, "DELETE ", name, ".*", from, join | where]
+      [pre_comments, cte, "DELETE ", name, ".*", from, join, where | post_comments]
     end
 
     @impl true
@@ -640,9 +641,6 @@ if Code.ensure_loaded?(MyXQL) do
       end)
     end
 
-    defp label(%{label: nil}), do: []
-    defp label(%{label: label}), do: ["/* ", label, " */ "]
-
     defp lock(%{lock: nil}, _sources), do: []
     defp lock(%{lock: binary}, _sources) when is_binary(binary), do: [?\s | binary]
     defp lock(%{lock: expr} = query, sources), do: [?\s | expr(expr, sources, query)]

lib/ecto/adapters/postgres/connection.ex

@@ -191,10 +191,10 @@ if Code.ensure_loaded?(Postgrex) do
       limit = limit(query, sources)
       offset = offset(query, sources)
       lock = lock(query, sources)
-      label = label(query)
+      {pre_comments, post_comments} = Ecto.Adapters.SQL.comments(query.comments)
 
       [
-        label,
+        pre_comments,
         cte,
         select,
         from,
@@ -206,7 +206,8 @@ if Code.ensure_loaded?(Postgrex) do
         combinations,
         order_by,
         limit,
-        offset | lock
+        offset,
+        lock | post_comments
       ]
     end
 
@@ -215,25 +216,25 @@ if Code.ensure_loaded?(Postgrex) do
       sources = create_names(query, [])
       cte = cte(query, sources)
       {from, name} = get_source(query, sources, 0, source)
-      label = label(query)
+      {pre_comments, post_comments} = Ecto.Adapters.SQL.comments(query.comments)
       prefix = prefix || ["UPDATE ", from, " AS ", name | " SET "]
       fields = update_fields(query, sources)
       {join, wheres} = using_join(query, :update_all, "FROM", sources)
       where = where(%{query | wheres: wheres ++ query.wheres}, sources)
 
-      [label, cte, prefix, fields, join, where | returning(query, sources)]
+      [pre_comments, cte, prefix, fields, join, where, returning(query, sources) | post_comments]
     end
 
     @impl true
     def delete_all(%{from: from} = query) do
       sources = create_names(query, [])
       cte = cte(query, sources)
       {from, name} = get_source(query, sources, 0, from)
-      label = label(query)
+      {pre_comments, post_comments} = Ecto.Adapters.SQL.comments(query.comments)
       {join, wheres} = using_join(query, :delete_all, "USING", sources)
       where = where(%{query | wheres: wheres ++ query.wheres}, sources)
 
-      [label, cte, "DELETE FROM ", from, " AS ", name, join, where | returning(query, sources)]
+      [pre_comments, cte, "DELETE FROM ", from, " AS ", name, join, where, returning(query, sources) | post_comments]
     end
 
     @impl true
@@ -882,9 +883,6 @@ if Code.ensure_loaded?(Postgrex) do
       end)
     end
 
-    defp label(%{label: nil}), do: []
-    defp label(%{label: label}), do: ["/* ", label, " */ "]
-
     defp lock(%{lock: nil}, _sources), do: []
     defp lock(%{lock: binary}, _sources) when is_binary(binary), do: [?\s | binary]
     defp lock(%{lock: expr} = query, sources), do: [?\s | expr(expr, sources, query)]

lib/ecto/adapters/sql.ex

@@ -987,7 +987,7 @@ defmodule Ecto.Adapters.SQL do
         opts
       end
 
-    {sql, opts} = prepend_label(sql, opts)
+    sql = wrap_comments(sql, opts)
 
     all_params = placeholders ++ Enum.reverse(params, conflict_params)
 
@@ -1169,28 +1169,39 @@ defmodule Ecto.Adapters.SQL do
   end
 
   @doc false
-  def prepend_label(sql, opts) do
-    case Keyword.get(opts, :label) do
-      nil ->
-        {sql, opts}
-
-      label ->
-        label = validate_label!(label)
-        {["/* ", label, " */ ", sql], opts}
-    end
+  def wrap_comments(sql, opts) do
+    {pre, post} = comments(Keyword.get(opts, :comments, []))
+    [pre, sql | post]
+  end
+
+  @doc false
+  def comments(comments) when is_list(comments) do
+    {pre, post} =
+      Enum.reduce(comments, {[], []}, fn
+        {:pre, c}, {pre, post} -> {[["/* ", escape_comment!(c), " */ "] | pre], post}
+        {:post, c}, {pre, post} -> {pre, [[" /* ", escape_comment!(c), " */"] | post]}
+        other, _ -> raise ArgumentError, "expected {:pre, string} or {:post, string}, got: #{inspect(other)}"
+      end)
+
+    {Enum.reverse(pre), Enum.reverse(post)}
+  end
+
+  def comments(other) do
+    raise ArgumentError,
+          "comments must be a keyword list of [pre: string, post: string], got: #{inspect(other)}"
   end
 
-  defp validate_label!(label) when is_binary(label) do
-    if String.contains?(label, ["/*", "*/", <<0>>]) do
+  defp escape_comment!(comment) when is_binary(comment) do
+    if String.contains?(comment, ["/*", "*/", <<0>>]) do
       raise ArgumentError,
-            "a label cannot contain `/*`, `*/`, or null bytes, got: #{inspect(label)}. "
+            "a comment cannot contain `/*`, `*/`, or null bytes, got: #{inspect(comment)}. "
     end
 
-    label
+    comment
   end
 
-  defp validate_label!(other) do
-    raise ArgumentError, "a label must be a string, got: #{inspect(other)}"
+  defp escape_comment!(other) do
+    raise ArgumentError, "a comment must be a string, got: #{inspect(other)}"
   end
 
   @doc false
@@ -1213,7 +1224,7 @@ defmodule Ecto.Adapters.SQL do
         opts
       end
 
-    {sql, opts} = prepend_label(sql, opts)
+    sql = wrap_comments(sql, opts)
 
     case query(adapter_meta, sql, values, [source: source] ++ opts) do
       {:ok, %{rows: nil, num_rows: 1}} ->

lib/ecto/adapters/tds/connection.ex

@@ -170,12 +170,12 @@ if Code.ensure_loaded?(Tds) do
       # limit = is handled in select (TOP X)
       offset = offset(query, sources)
       lock = lock(query, sources)
-      label = label(query)
+      {pre_comments, post_comments} = SQL.comments(query.comments)
 
       if query.offset != nil and query.order_bys == [],
         do: error!(query, "ORDER BY is mandatory when OFFSET is set")
 
-      [label, cte, select, from, join, where, group_by, having, combinations, order_by, lock | offset]
+      [pre_comments, cte, select, from, join, where, group_by, having, combinations, order_by, lock, offset | post_comments]
     end
 
     @impl true
@@ -189,10 +189,10 @@ if Code.ensure_loaded?(Tds) do
       join = join(query, sources)
       where = where(query, sources)
       lock = lock(query, sources)
-      label = label(query)
+      {pre_comments, post_comments} = SQL.comments(query.comments)
 
       [
-        label,
+        pre_comments,
         cte,
         "UPDATE ",
         name,
@@ -201,7 +201,8 @@ if Code.ensure_loaded?(Tds) do
         returning(query, 0, "INSERTED"),
         from,
         join,
-        where | lock
+        where,
+        lock | post_comments
       ]
     end
 
@@ -216,9 +217,9 @@ if Code.ensure_loaded?(Tds) do
       join = join(query, sources)
       where = where(query, sources)
       lock = lock(query, sources)
-      label = label(query)
+      {pre_comments, post_comments} = SQL.comments(query.comments)
 
-      [label, cte, delete, returning(query, 0, "DELETED"), from, join, where | lock]
+      [pre_comments, cte, delete, returning(query, 0, "DELETED"), from, join, where, lock | post_comments]
     end
 
     @impl true
@@ -660,9 +661,6 @@ if Code.ensure_loaded?(Tds) do
     defp hints([_ | _] = hints), do: [" WITH (", Enum.intersperse(hints, ", "), ?)]
     defp hints([]), do: []
 
-    defp label(%{label: nil}), do: []
-    defp label(%{label: label}), do: ["/* ", label, " */ "]
-
     defp lock(%{lock: nil}, _sources), do: []
     defp lock(%{lock: binary}, _sources) when is_binary(binary), do: [" OPTION (", binary, ?)]
     defp lock(%{lock: expr} = query, sources), do: [" OPTION (", expr(expr, sources, query), ?)]

test/ecto/adapters/myxql_test.exs

@@ -590,15 +590,19 @@ defmodule Ecto.Adapters.MyXQLTest do
     assert all(query) == ~s{SELECT TRUE FROM `schema` AS s0 UPDATE on s0}
   end
 
-  test "label" do
+  test "comments" do
     query = Schema |> select([], true) |> plan()
-    assert all(%{query | label: "myquery"}) == ~s{/* myquery */ SELECT TRUE FROM `schema` AS s0}
+    assert all(%{query | comments: [pre: "q"]}) == ~s{/* q */ SELECT TRUE FROM `schema` AS s0}
+    assert all(%{query | comments: [post: "q"]}) == ~s{SELECT TRUE FROM `schema` AS s0 /* q */}
+
+    assert all(%{query | comments: [pre: "a", post: "b"]}) ==
+             ~s{/* a */ SELECT TRUE FROM `schema` AS s0 /* b */}
 
     query = Schema |> update([], set: [x: 0]) |> plan(:update_all)
-    assert update_all(%{query | label: "upd_q"}) == ~s{/* upd_q */ UPDATE `schema` AS s0 SET s0.`x` = 0}
+    assert update_all(%{query | comments: [pre: "upd_q"]}) == ~s{/* upd_q */ UPDATE `schema` AS s0 SET s0.`x` = 0}
 
     query = Schema |> plan(:delete_all)
-    assert delete_all(%{query | label: "del_q"}) == ~s{/* del_q */ DELETE s0.* FROM `schema` AS s0}
+    assert delete_all(%{query | comments: [pre: "del_q"]}) == ~s{/* del_q */ DELETE s0.* FROM `schema` AS s0}
   end
 
   test "string escape" do

test/ecto/adapters/postgres_test.exs

@@ -783,15 +783,19 @@ defmodule Ecto.Adapters.PostgresTest do
     assert all(query) == ~s{SELECT TRUE FROM "schema" AS s0 UPDATE on s0}
   end
 
-  test "label" do
+  test "comments" do
     query = Schema |> select([], true) |> plan()
-    assert all(%{query | label: "myquery"}) == ~s{/* myquery */ SELECT TRUE FROM "schema" AS s0}
+    assert all(%{query | comments: [pre: "q"]}) == ~s{/* q */ SELECT TRUE FROM "schema" AS s0}
+    assert all(%{query | comments: [post: "q"]}) == ~s{SELECT TRUE FROM "schema" AS s0 /* q */}
+
+    assert all(%{query | comments: [pre: "a", post: "b"]}) ==
+             ~s{/* a */ SELECT TRUE FROM "schema" AS s0 /* b */}
 
     query = Schema |> update([], set: [x: 0]) |> plan(:update_all)
-    assert update_all(%{query | label: "upd_q"}) == ~s{/* upd_q */ UPDATE "schema" AS s0 SET "x" = 0}
+    assert update_all(%{query | comments: [pre: "upd_q"]}) == ~s{/* upd_q */ UPDATE "schema" AS s0 SET "x" = 0}
 
     query = Schema |> plan(:delete_all)
-    assert delete_all(%{query | label: "del_q"}) == ~s{/* del_q */ DELETE FROM "schema" AS s0}
+    assert delete_all(%{query | comments: [pre: "del_q"]}) == ~s{/* del_q */ DELETE FROM "schema" AS s0}
   end
 
   test "string escape" do