Support pre_comment and post_comment options in ecto.

Author: alesasnouski

lib/ecto/query.ex

@@ -415,7 +415,7 @@ defmodule Ecto.Query do
             lock: nil,
             windows: [],
             with_ctes: nil,
-            label: nil
+            comments: []
 
   defmodule FromExpr do
     @moduledoc false
@@ -956,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)
@@ -1025,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: []}
@@ -1151,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]
 
@@ -2538,6 +2541,56 @@ 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

@@ -1073,7 +1073,7 @@ defmodule Ecto.Query.Planner do
   end
 
   defp finalize_cache(query, operation, cache) do
-    %{assocs: assocs, prefix: prefix, lock: lock, label: label, select: select, aliases: aliases} =
+    %{assocs: assocs, prefix: prefix, lock: lock, comments: comments, select: select, aliases: aliases} =
       query
     aliases = Map.delete(aliases, @parent_as)
 
@@ -1091,7 +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(label != nil, label: label)
+      |> prepend_if(comments != [], comments: comments)
       |> prepend_if(aliases != %{}, aliases: aliases)
 
     [operation | cache]
@@ -2384,34 +2384,23 @@ defmodule Ecto.Query.Planner do
   def attach_prefix(query, _), do: query
 
   @doc """
-  Puts the label given via `opts` into the given query, if available.
+  Appends the comments given via the `:comments` option into the query.
 
-  The label is rendered by the adapter as a leading `/* ... */` SQL comment
-  and becomes part of the query cache key. It is embedded verbatim, so it may
-  not contain `/*`, `*/`, or null bytes.
+  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_label(%{label: nil} = query, opts) when is_list(opts) do
-    case Keyword.fetch(opts, :label) do
-      {:ok, label} -> %{query | label: validate_label!(label)}
+  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_label(query, _), do: query
-
-  defp validate_label!(label) when is_binary(label) do
-    if String.contains?(label, ["/*", "*/", <<0>>]) do
-      raise ArgumentError,
-            "a label cannot contain `/*`, `*/`, or null bytes, got: #{inspect(label)}. " <>
-              "Allowing them would let the label break out of the surrounding `/* */` comment"
-    end
-
-    label
-  end
-
-  defp validate_label!(other) do
-    raise ArgumentError, "a label must be a string, got: #{inspect(other)}"
-  end
+  def attach_comments(query, _), do: query
 
   ## Helpers
 

lib/ecto/repo.ex

@@ -141,12 +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`.
-    * `:label` - A string embedded into the generated statement as a leading SQL comment,
-      such as `/* import_users */ INSERT ...`, to identify the query in database logs and
-      monitoring tools. It is rendered leading (rather than trailing) so it survives truncation
-      of long statements in logs. The string is embedded verbatim and therefore cannot contain
-      `/*`, `*/`, or null bytes. The label becomes part of the query cache key, so prefer a
-      stable identifier per call site over highly dynamic values such as per-request ids.
+    * `: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, attach_label: 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 = query |> attach_prefix(opts) |> attach_label(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 = query |> attach_prefix(opts) |> attach_label(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