Add Ecto.Query.label/2 query expression for tagging queries with an S…

lib/ecto/query.ex

@@ -414,7 +414,8 @@ defmodule Ecto.Query do
             distinct: nil,
             lock: nil,
             windows: [],
-            with_ctes: nil
+            with_ctes: nil,
+            comments: []
 
   defmodule FromExpr do
     @moduledoc false
@@ -955,6 +956,7 @@ defmodule Ecto.Query do
       Ecto.Query.exclude(query, :limit)
       Ecto.Query.exclude(query, :offset)
       Ecto.Query.exclude(query, :lock)
+      Ecto.Query.exclude(query, :comments)
       Ecto.Query.exclude(query, :preload)
       Ecto.Query.exclude(query, :update)
       Ecto.Query.exclude(query, :windows)
@@ -1024,6 +1026,7 @@ defmodule Ecto.Query do
   defp do_exclude(%Ecto.Query{} = query, :limit), do: %{query | limit: nil}
   defp do_exclude(%Ecto.Query{} = query, :offset), do: %{query | offset: nil}
   defp do_exclude(%Ecto.Query{} = query, :lock), do: %{query | lock: nil}
+  defp do_exclude(%Ecto.Query{} = query, :comments), do: %{query | comments: []}
   defp do_exclude(%Ecto.Query{} = query, :preload), do: %{query | preloads: [], assocs: []}
   defp do_exclude(%Ecto.Query{} = query, :update), do: %{query | updates: []}
   defp do_exclude(%Ecto.Query{} = query, :windows), do: %{query | windows: []}
@@ -1150,7 +1153,8 @@ 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] ++
+              [:pre_comment, :post_comment]
   @binds [:lock, :where, :or_where, :select, :distinct, :order_by, :group_by, :windows] ++
            [:having, :or_having, :limit, :offset, :preload, :update, :select_merge, :with_ctes]
 
@@ -2537,6 +2541,57 @@ defmodule Ecto.Query do
     Builder.Lock.build(query, binding, expr, __CALLER__)
   end
 
+  @doc ~S"""
+  Adds a comment *before* the generated statement.
+
+  The text is rendered as a leading SQL comment, immediately before the
+  statement keyword:
+
+      /* list_users */ SELECT ...
+
+  This is useful to tag and identify queries in database logs and monitoring
+  tools. A leading comment (rather than a trailing one, see `post_comment/2`)
+  survives truncation of long statements in logs.
+
+  The comment must be a compile-time literal string, so the set of comments for
+  a given query stays bounded and the query remains safely cacheable. It is
+  embedded verbatim and therefore cannot contain `/*`, `*/`, or null bytes. For
+  dynamic comments, use the `:comments` option on the repository operation
+  instead. Multiple comments may be added and are rendered in order.
+
+  ## Keywords example
+
+      from(p in Post, pre_comment: "list_posts", select: p.title)
+
+  ## Expressions example
+
+      Post |> pre_comment("list_posts") |> select([p], p.title)
+  """
+  defmacro pre_comment(query, expr) do
+    Builder.Comment.build(:pre, query, expr, __CALLER__)
+  end
+
+  @doc ~S"""
+  Adds a comment *after* the generated statement.
+
+  Works like `pre_comment/2` but renders the text as a trailing SQL comment,
+  after the statement (the SQLCommenter convention):
+
+      SELECT ... /* list_users */
+
+  ## Keywords example
+
+      from(p in Post, post_comment: "list_posts", select: p.title)
+
+  ## Expressions example
+
+      Post |> post_comment("list_posts") |> select([p], p.title)
+  """
+  defmacro post_comment(query, expr) do
+    Builder.Comment.build(:post, query, expr, __CALLER__)
+  end
+
+
   @doc ~S"""
   An update query expression.
 

lib/ecto/query/builder/comment.ex

@@ -0,0 +1,63 @@
+import Kernel, except: [apply: 3]
+
+defmodule Ecto.Query.Builder.Comment do
+  @moduledoc false
+
+  alias Ecto.Query.Builder
+
+  # A comment is rendered verbatim inside a `/* ... */` SQL comment, so the only
+  # way for its text to escape into executable SQL is to manipulate the comment
+  # delimiters. Reject `*/` (closes the block early) and `/*` (opens a nested
+  # block that, where comments nest, swallows the closing `*/`), plus null bytes.
+  @forbidden ["*/", "/*", <<0>>]
+
+  @doc """
+  Escapes the comment text.
+
+      iex> escape("my-query")
+      "my-query"
+
+  """
+  @spec escape(Macro.t()) :: Macro.t()
+  def escape(comment) when is_binary(comment) do
+    if String.contains?(comment, @forbidden) do
+      Builder.error!(
+        "a comment cannot contain `/*`, `*/`, or null bytes, got: `#{comment}`. "
+      )
+    end
+
+    comment
+  end
+
+  def escape({:^, _, [_]}) do
+    Builder.error!(
+      "interpolation is not allowed in a query comment. " <>
+        "Comments must be compile-time literal strings so they stay a bounded set " <>
+        "and remain safe to cache. For dynamic comments use the `:comments` repo option"
+    )
+  end
+
+  def escape(other) do
+    Builder.error!("`#{Macro.to_string(other)}` is not a valid comment, it must be a literal string")
+  end
+
+  @doc """
+  Builds a quoted expression that appends a `{position, comment}` to the query.
+  """
+  @spec build(:pre | :post, Macro.t(), Macro.t(), Macro.Env.t()) :: Macro.t()
+  def build(position, query, expr, env) do
+    Builder.apply_query(query, __MODULE__, [position, escape(expr)], env)
+  end
+
+  @doc """
+  The callback applied by `build/4` to build the query.
+  """
+  @spec apply(Ecto.Queryable.t(), :pre | :post, term) :: Ecto.Query.t()
+  def apply(%Ecto.Query{} = query, position, value) do
+    %{query | comments: query.comments ++ [{position, value}]}
+  end
+
+  def apply(query, position, value) do
+    apply(Ecto.Queryable.to_query(query), position, value)
+  end
+end

lib/ecto/query/inspect.ex

@@ -124,6 +124,7 @@ defimpl Inspect, for: Ecto.Query do
     updates = kw_exprs(:update, query.updates, names)
 
     lock = kw_inspect(:lock, query.lock)
+    comments = comments(query.comments)
     offset = kw_expr(:offset, query.offset, names)
     select = kw_expr(:select, query.select, names)
     distinct = kw_expr(:distinct, query.distinct, names)
@@ -140,6 +141,7 @@ defimpl Inspect, for: Ecto.Query do
       limit,
       offset,
       lock,
+      comments,
       distinct,
       updates,
       select,
@@ -248,6 +250,10 @@ defimpl Inspect, for: Ecto.Query do
   defp kw_inspect(_key, nil), do: []
   defp kw_inspect(key, val), do: [{key, inspect(val)}]
 
+  defp comments(comments) do
+    for {position, comment} <- comments, do: {:"#{position}_comment", inspect(comment)}
+  end
+
   defp kw_as_and_prefix(%{as: as, prefix: prefix}) do
     kw_inspect(:as, as) ++ kw_inspect(:prefix, prefix)
   end

lib/ecto/query/planner.ex

@@ -13,7 +13,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
 
@@ -1073,7 +1073,8 @@ defmodule Ecto.Query.Planner do
   end
 
   defp finalize_cache(query, operation, cache) do
-    %{assocs: assocs, prefix: prefix, lock: lock, select: select, aliases: aliases} = query
+    %{assocs: assocs, prefix: prefix, lock: lock, comments: comments, select: select, aliases: aliases} =
+      query
     aliases = Map.delete(aliases, @parent_as)
 
     cache =
@@ -1090,6 +1091,7 @@ defmodule Ecto.Query.Planner do
       |> prepend_if(assocs != [], assocs: assocs)
       |> prepend_if(prefix != nil, prefix: prefix)
       |> prepend_if(lock != nil, lock: lock)
+      |> prepend_if(comments != [], comments: comments)
       |> prepend_if(aliases != %{}, aliases: aliases)
 
     [operation | cache]
@@ -2381,6 +2383,25 @@ defmodule Ecto.Query.Planner do
 
   def attach_prefix(query, _), do: query
 
+  @doc """
+  Appends the comments given via the `:comments` option into the query.
+
+  The option is a keyword list of `[pre: string, post: string]` entries, which
+  share the representation of the query's `comments` field. They become part of
+  the query cache key and are rendered by the adapter as `/* ... */` comments.
+  Validating and escaping them is left to the adapter, since the comment syntax
+  (and therefore what is unsafe) depends on the adapter.
+  """
+  def attach_comments(query, opts) when is_list(opts) do
+    case Keyword.fetch(opts, :comments) do
+      {:ok, comments} when is_list(comments) -> %{query | comments: query.comments ++ comments}
+      {:ok, other} -> raise ArgumentError, "the :comments option must be a keyword list of [pre: string, post: string], got: #{inspect(other)}"
+      :error -> query
+    end
+  end
+
+  def attach_comments(query, _), do: query
+
   ## Helpers
 
   @all_exprs [

lib/ecto/repo.ex

@@ -141,6 +141,17 @@ defmodule Ecto.Repo do
       in the cache and no cache update function will not be passed to the adapter. Note that
       this doesn't necessarily disable the database cache, it only affects Ecto's internal
       cache of normalized queries and adapter prepared statements. Defaults to `true`.
+    * `:comments` - A keyword list of `[pre: string, post: string]` entries embedded into the
+      generated statement as `/* ... */` SQL comments to identify the query in database logs and
+      monitoring tools. `:pre` comments are rendered before the statement (e.g.
+      `/* import_users */ INSERT ...`) so they survive truncation of long statements in logs;
+      `:post` comments are rendered after it. Strings are embedded verbatim and therefore cannot
+      contain `/*`, `*/`, or null bytes. Comments become part of the query cache key, so prefer
+      stable identifiers per call site over highly dynamic values such as per-request ids. If you
+      do pass dynamic comments to a query operation (`all`/`update_all`/`delete_all`), combine them
+      with `query_cache: false` so the unbounded set of comments does not grow Ecto's query cache
+      and the adapter's prepared statements. For query expressions you can also use
+      `Ecto.Query.pre_comment/2` and `Ecto.Query.post_comment/2`, which must be static.
 
   ## Adapter-Specific Errors
 

lib/ecto/repo/queryable.ex

@@ -6,7 +6,7 @@ defmodule Ecto.Repo.Queryable do
   alias Ecto.Query.Planner
   alias Ecto.Query.SelectExpr
 
-  import Ecto.Query.Planner, only: [attach_prefix: 2]
+  import Ecto.Query.Planner, only: [attach_prefix: 2, attach_comments: 2]
 
   require Ecto.Query
 
@@ -37,7 +37,7 @@ defmodule Ecto.Repo.Queryable do
       |> Ecto.Query.Planner.ensure_select(true)
 
     {query, opts} = repo.prepare_query(:stream, query, opts)
-    query = attach_prefix(query, opts)
+    query = query |> attach_prefix(opts) |> attach_comments(opts)
 
     query_cache? = Keyword.get(opts, :query_cache, true)
 
@@ -222,7 +222,7 @@ defmodule Ecto.Repo.Queryable do
     %{adapter: adapter, cache: cache, repo: repo} = adapter_meta
 
     {query, opts} = repo.prepare_query(operation, query, opts)
-    query = attach_prefix(query, opts)
+    query = query |> attach_prefix(opts) |> attach_comments(opts)
 
     query_cache? = Keyword.get(opts, :query_cache, true)
 

test/ecto/query/builder/comment_test.exs

@@ -0,0 +1,62 @@
+Code.require_file "../../../support/eval_helpers.exs", __DIR__
+
+defmodule Ecto.Query.Builder.CommentTest do
+  use ExUnit.Case, async: true
+
+  import Ecto.Query.Builder.Comment
+  doctest Ecto.Query.Builder.Comment
+
+  import Ecto.Query
+  import Support.EvalHelpers
+
+  test "pre_comment with literal string" do
+    query = %Ecto.Query{} |> pre_comment("list_posts")
+    assert query.comments == [pre: "list_posts"]
+  end
+
+  test "post_comment with literal string" do
+    query = %Ecto.Query{} |> post_comment("list_posts")
+    assert query.comments == [post: "list_posts"]
+  end
+
+  test "pre_comment via keyword syntax" do
+    query = from p in "posts", pre_comment: "list_posts"
+    assert query.comments == [pre: "list_posts"]
+  end
+
+  test "comments accumulate in order" do
+    query = %Ecto.Query{} |> pre_comment("a") |> post_comment("b") |> pre_comment("c")
+    assert query.comments == [{:pre, "a"}, {:post, "b"}, {:pre, "c"}]
+  end
+
+  test "raises on interpolation (comments must be static)" do
+    _report = "monthly"
+
+    assert_raise Ecto.Query.CompileError, ~r"interpolation is not allowed", fn ->
+      quote_and_eval(%Ecto.Query{} |> pre_comment(^_report))
+    end
+  end
+
+  test "raises on a non-literal" do
+    assert_raise Ecto.Query.CompileError, ~r"is not a valid comment", fn ->
+      quote_and_eval(%Ecto.Query{} |> pre_comment(1))
+    end
+  end
+
+  test "raises on a comment containing */" do
+    assert_raise Ecto.Query.CompileError, ~r"cannot contain `/\*`, `\*/`, or null bytes", fn ->
+      quote_and_eval(%Ecto.Query{} |> post_comment("evil */ DROP"))
+    end
+  end
+
+  test "raises on a comment containing /*" do
+    assert_raise Ecto.Query.CompileError, ~r"cannot contain `/\*`, `\*/`, or null bytes", fn ->
+      quote_and_eval(%Ecto.Query{} |> pre_comment("evil /* nested"))
+    end
+  end
+
+  test "exclude resets the comments" do
+    query = %Ecto.Query{} |> pre_comment("a") |> post_comment("b")
+    assert Ecto.Query.exclude(query, :comments).comments == []
+  end
+end