dk-query comments

lib/ecto/query.ex

@@ -397,6 +397,7 @@ defmodule Ecto.Query do
   """
   defstruct prefix: nil,
             sources: nil,
+            comments: [],
             from: nil,
             joins: [],
             aliases: %{},
@@ -426,6 +427,11 @@ defmodule Ecto.Query do
     defstruct [:fun, :binding, :file, :line]
   end
 
+  defmodule CommentExpr do
+    @moduledoc false
+    defstruct [:expr, :file, :line, params: []]
+  end
+
   defmodule QueryExpr do
     @moduledoc false
     defstruct [:expr, :file, :line, params: []]
@@ -939,6 +945,7 @@ defmodule Ecto.Query do
       Ecto.Query.exclude(query, :distinct)
       Ecto.Query.exclude(query, :select)
       Ecto.Query.exclude(query, :combinations)
+      Ecto.Query.exclude(query, :comments)
       Ecto.Query.exclude(query, :with_ctes)
       Ecto.Query.exclude(query, :limit)
       Ecto.Query.exclude(query, :offset)
@@ -980,6 +987,7 @@ defmodule Ecto.Query do
   defp do_exclude(%Ecto.Query{} = query, :order_by), do: %{query | order_bys: []}
   defp do_exclude(%Ecto.Query{} = query, :group_by), do: %{query | group_bys: []}
   defp do_exclude(%Ecto.Query{} = query, :combinations), do: %{query | combinations: []}
+  defp do_exclude(%Ecto.Query{} = query, :comments), do: %{query | comments: []}
   defp do_exclude(%Ecto.Query{} = query, :with_ctes), do: %{query | with_ctes: nil}
   defp do_exclude(%Ecto.Query{} = query, :having), do: %{query | havings: []}
   defp do_exclude(%Ecto.Query{} = query, :distinct), do: %{query | distinct: nil}
@@ -1109,7 +1117,7 @@ defmodule Ecto.Query do
   end
 
   @from_join_opts [:as, :prefix, :hints]
-  @no_binds [:union, :union_all, :except, :except_all, :intersect, :intersect_all]
+  @no_binds [:union, :union_all, :except, :except_all, :intersect, :intersect_all, :comment]
   @binds [:lock, :where, :or_where, :select, :distinct, :order_by, :group_by, :windows] ++
            [:having, :or_having, :limit, :offset, :preload, :update, :select_merge, :with_ctes]
 
@@ -1734,6 +1742,10 @@ defmodule Ecto.Query do
     Builder.Select.build(:merge, query, binding, expr, __CALLER__)
   end
 
+  defmacro comment(query, comment) do
+    Builder.Comment.build(query, comment, __CALLER__)
+  end
+
   @doc """
   A distinct query expression.
 

lib/ecto/query/builder/comment.ex

@@ -0,0 +1,62 @@
+import Kernel, except: [apply: 2]
+
+defmodule Ecto.Query.Builder.Comment do
+  @moduledoc false
+
+  alias Ecto.Query.Builder
+  alias Ecto.Query.CommentExpr
+
+  @spec escape(Macro.t(), Macro.Env.t()) :: Macro.t()
+  def escape(comment, _env) when is_binary(comment), do: comment
+
+  def escape(expr, env) do
+    {expr, {_params, _acc}} =
+      Builder.escape(expr, :any, {[], %{}}, [], env)
+
+    expr
+  end
+
+  @doc """
+  Called at runtime to assemble comment.
+  """
+  def comment!(query, comment, file, line) do
+    comment = %CommentExpr{
+      expr: comment,
+      line: line,
+      file: file
+    }
+
+    apply(query, comment)
+  end
+
+  def build(query, {:^, _, [var]}, env) do
+    quote do
+      Ecto.Query.Builder.Comment.comment!(
+        unquote(query),
+        unquote(var),
+        unquote(env.file),
+        unquote(env.line)
+      )
+    end
+  end
+
+  def build(query, comment, env) do
+    Builder.apply_query(query, __MODULE__, [escape(comment, env)], env)
+  end
+
+  def build(query, comment, _file, _line) do
+    apply(query, comment)
+  end
+
+  @doc """
+  The callback applied by `build/4` to build the query.
+  """
+  @spec apply(Ecto.Queryable.t(), String.t() | CommentExpr.t()) :: Ecto.Query.t()
+  def apply(%Ecto.Query{comments: comments} = query, comment) do
+    %{query | comments: comments ++ [comment]}
+  end
+
+  def apply(query, comment) do
+    apply(Ecto.Queryable.to_query(query), comment)
+  end
+end

lib/ecto/query/inspect.ex

@@ -1,7 +1,7 @@
 import Inspect.Algebra
 import Kernel, except: [to_string: 1]
 
-alias Ecto.Query.{DynamicExpr, JoinExpr, QueryExpr, WithExpr, LimitExpr}
+alias Ecto.Query.{DynamicExpr, JoinExpr, QueryExpr, WithExpr, LimitExpr, CommentExpr}
 
 defimpl Inspect, for: Ecto.Query.DynamicExpr do
   def inspect(%DynamicExpr{binding: binding} = dynamic, opts) do
@@ -50,6 +50,11 @@ defimpl Inspect, for: Ecto.Query do
 
     result = container_doc("#Ecto.Query<", list, ">", opts, fn str, _ -> str end)
 
+    result =
+      if query.comments == [],
+        do: result,
+        else: concat([comment(query.comments), "\n", result])
+
     case query.with_ctes do
       %WithExpr{recursive: recursive, queries: [_ | _] = queries} ->
         with_ctes =
@@ -161,6 +166,13 @@ defimpl Inspect, for: Ecto.Query do
     inspect(if source == schema.__schema__(:source), do: schema, else: {source, schema})
   end
 
+  defp comment(comments) do
+    Enum.map_join(comments, "\n", fn
+      comment when is_binary(comment) -> "# #{comment}"
+      %CommentExpr{expr: comment} -> "# #{comment}"
+    end)
+  end
+
   defp joins(joins, names) do
     joins
     |> Enum.with_index()

lib/ecto/query/planner.ex

@@ -5,6 +5,7 @@ defmodule Ecto.Query.Planner do
   alias Ecto.Query.{
     BooleanExpr,
     ByExpr,
+    CommentExpr,
     DynamicExpr,
     FromExpr,
     JoinExpr,
@@ -13,7 +14,7 @@ defmodule Ecto.Query.Planner do
     LimitExpr
   }
 
-  if map_size(%Ecto.Query{}) != 21 do
+  if map_size(%Ecto.Query{}) != 22 do
     raise "Ecto.Query match out of date in builder"
   end
 
@@ -977,6 +978,19 @@ defmodule Ecto.Query.Planner do
     end)
   end
 
+  defp merge_cache(:comment, _query, [], cache_and_params, _operation, _adapter) do
+    cache_and_params
+  end
+
+  defp merge_cache(:comment, _query, comments, cache_and_params, _operation, _adapter) do
+    # binary comments can be cached, expression comments are dynamic so we don't cache the query
+    Enum.reduce_while(comments, cache_and_params, fn
+      comment, {cache, params} when is_binary(comment) -> {:cont, {merge_cache({:comment, comment}, cache, true), params}}
+      %CommentExpr{}, {_cache, params} -> {:halt, {:nocache, params}}
+    end)
+  end
+
+
   defp merge_cache(:with_cte, _query, nil, cache_and_params, _operation, _adapter) do
     cache_and_params
   end
@@ -1348,10 +1362,23 @@ defmodule Ecto.Query.Planner do
     else
       {nil, counter}
     end
+
+  end
+
+  defp validate_and_increment(:comment, query, exprs, counter, _operation, adapter) do
+    {exprs, counter} =
+    Enum.reduce(exprs, {[], counter}, fn
+      comment, {list, acc} when is_binary(comment) -> {[comment | list], acc + 1}
+      %CommentExpr{} = expr, {list, acc} ->
+        {expr, acc} = prewalk(:comment, query, expr, acc, adapter)
+        {[expr | list], acc}
+    end)
+
+    {Enum.reverse(exprs), counter}
   end
 
   defp validate_and_increment(kind, query, exprs, counter, _operation, adapter)
-       when kind in ~w(where group_by having order_by update)a do
+       when kind in ~w(where group_by having order_by update comment)a do
     {exprs, counter} =
       Enum.reduce(exprs, {[], counter}, fn
         %{expr: []}, {list, acc} ->
@@ -2328,6 +2355,7 @@ defmodule Ecto.Query.Planner do
   ## Helpers
 
   @all_exprs [
+    comment: :comments,
     with_cte: :with_ctes,
     distinct: :distinct,
     select: :select,
@@ -2350,6 +2378,7 @@ defmodule Ecto.Query.Planner do
   # The only way to address it is by splitting how join
   # and their on expressions are processed.
   @update_all_exprs [
+    comment: :comments,
     with_cte: :with_ctes,
     from: :from,
     update: :updates,
@@ -2359,6 +2388,7 @@ defmodule Ecto.Query.Planner do
   ]
 
   @delete_all_exprs [
+    comment: :comments,
     with_cte: :with_ctes,
     from: :from,
     join: :joins,
@@ -2380,6 +2410,7 @@ defmodule Ecto.Query.Planner do
     Enum.reduce(exprs, {query, acc}, fn {kind, key}, {query, acc} ->
       {traversed, acc} = fun.(kind, query, Map.fetch!(query, key), acc)
       {%{query | key => traversed}, acc}
+
     end)
   end
 

test/ecto/query/inspect_test.exs

@@ -455,7 +455,7 @@
     end
   else
     test "container values" do
       assert i(from(Post, select: <<1, 2, 3>>)) ==
                "from p0 in Inspect.Post, select: <<1, 2, 3>>"
 
       foo = <<1, 2, 3>>
@@ -571,6 +571,61 @@
     assert i(plan(query)) == "from v0 in values (#{fields})"
   end
 
+  test "with static comments" do
+    query = from(p in "posts") |> comment("foo") |> comment("bar")
+    i = inspect(query, safe: false)
+
+    assert i == """
+           # foo
+           # bar
+           #Ecto.Query<from p0 in "posts">\
+           """
+
+    i = inspect(plan(query), safe: false)
+
+    assert i == """
+           # foo
+           # bar
+           #Ecto.Query<from p0 in "posts">\
+           """
+  end
+
+  test "with dynamic comments" do
+    query = from(p in "posts") |> comment(^"foo #{"bar"}")
+    i = inspect(query, safe: false)
+
+    assert i == """
+           # foo bar
+           #Ecto.Query<from p0 in "posts">\
+           """
+
+    i = inspect(plan(query), safe: false)
+
+    assert i == """
+           # foo bar
+           #Ecto.Query<from p0 in "posts">\
+           """
+  end
+
+  test "comments in keyword query" do
+    query = from(p in "posts", comment: "foo", comment: ^"bar #{"baz"}")
+    i = inspect(query, safe: false)
+
+    assert i == """
+           # foo
+           # bar baz
+           #Ecto.Query<from p0 in "posts">\
+           """
+
+    i = inspect(plan(query), safe: false)
+
+    assert i == """
+           # foo
+           # bar baz
+           #Ecto.Query<from p0 in "posts">\
+           """
+  end
+
   def plan(query) do
     {query, _, _} = Ecto.Adapter.Queryable.plan_query(:all, Ecto.TestAdapter, query)
     query

test/ecto/query/planner_test.exs

@@ -666,6 +666,26 @@ defmodule Ecto.Query.PlannerTest do
     assert key == :nocache
   end
 
+  test "plan: dynamic comments are uncacheable" do
+    {_query, _params, key} =
+      Post
+      |> select([p], p.id)
+      |> comment(^"uncache#{"able"}")
+      |> Planner.plan(:all, Ecto.TestAdapter)
+
+    assert key == :nocache
+  end
+
+  test "plan: static comments are cacheable" do
+    {_query, _params, key} =
+      Post
+      |> select([p], p.id)
+      |> comment("cacheable")
+      |> Planner.plan(:all, Ecto.TestAdapter)
+
+    assert key != :nocache
+  end
+
   test "plan: normalizes prefixes" do
     # No schema prefix in from
     {query, _, _, _} = from(Comment, select: 1) |> plan()

test/ecto/query_test.exs

@@ -1137,4 +1137,23 @@ defmodule Ecto.QueryTest do
       assert inspect(reverse_order(q)) == inspect(order_by(q, desc: :id))
     end
   end
+
+
+  describe "query comments" do
+    test "allows compile time strings" do
+      query =
+        from(p in "posts")
+        |> comment("called from test")
+
+      assert ["called from test"] == query.comments
+    end
+
+    test "allows interpolated strings" do
+      query =
+        from(p in "posts")
+        |> comment(^"called from #{"test"}")
+
+      assert "called from test" == hd(query.comments).expr
+    end
+  end
 end