diff --git a/lib/ecto/query.ex b/lib/ecto/query.ex index 110331b23f..a71227f7bd 100644 --- a/lib/ecto/query.ex +++ b/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. diff --git a/lib/ecto/query/builder/comment.ex b/lib/ecto/query/builder/comment.ex new file mode 100644 index 0000000000..3d52362b15 --- /dev/null +++ b/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 diff --git a/lib/ecto/query/inspect.ex b/lib/ecto/query/inspect.ex index d5427ff0f0..3c291123e0 100644 --- a/lib/ecto/query/inspect.ex +++ b/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 diff --git a/lib/ecto/query/planner.ex b/lib/ecto/query/planner.ex index 55a437fbcb..e82a58fb24 100644 --- a/lib/ecto/query/planner.ex +++ b/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 [ diff --git a/lib/ecto/repo.ex b/lib/ecto/repo.ex index c99d5fabca..11bb22c581 100644 --- a/lib/ecto/repo.ex +++ b/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 diff --git a/lib/ecto/repo/queryable.ex b/lib/ecto/repo/queryable.ex index dc462a6a60..251cd93926 100644 --- a/lib/ecto/repo/queryable.ex +++ b/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) diff --git a/test/ecto/query/builder/comment_test.exs b/test/ecto/query/builder/comment_test.exs new file mode 100644 index 0000000000..5a7befedb0 --- /dev/null +++ b/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 diff --git a/test/ecto/query/planner_test.exs b/test/ecto/query/planner_test.exs index 93be9b9eab..84a215b9f1 100644 --- a/test/ecto/query/planner_test.exs +++ b/test/ecto/query/planner_test.exs @@ -2907,4 +2907,69 @@ defmodule Ecto.Query.PlannerTest do assert :ets.info(cache, :size) == 0 end end + + describe "attach_comments/2" do + test "appends the comments from opts" do + query = Planner.attach_comments(from(p in "posts"), comments: [pre: "get_posts_q"]) + assert query.comments == [pre: "get_posts_q"] + end + + test "appends to comments already on the query" do + query = + from(p in "posts") + |> Ecto.Query.pre_comment("from_macro") + |> Planner.attach_comments(comments: [post: "from_opts"]) + + assert query.comments == [{:pre, "from_macro"}, {:post, "from_opts"}] + end + + test "leaves the query unchanged when no comments are given" do + query = from(p in "posts") + assert Planner.attach_comments(query, []).comments == [] + end + + test "raises when :comments is not a list" do + assert_raise ArgumentError, ~r/must be a keyword list/, fn -> + Planner.attach_comments(from(p in "posts"), comments: "nope") + end + end + + test "is part of the query cache key" do + cache = Planner.new_query_cache(__MODULE__) + query = from(p in Post, where: p.title == ^"hello") + + cache_query = fn comment -> + query + |> Planner.attach_comments(comments: [pre: comment]) + |> Planner.query(:all, cache, Ecto.CachingTestAdapter, 0, true) + end + + # Different comments produce distinct cache entries... + cache_query.("a") + cache_query.("b") + assert :ets.info(cache, :size) == 2 + + # ...while the same comment reuses one. + cache_query.("a") + assert :ets.info(cache, :size) == 2 + end + + test "query_cache: false keeps the cache from growing with dynamic comments" do + cache = Planner.new_query_cache(__MODULE__) + query = from(p in Post, where: p.title == ^"hello") + + for comment <- ["dyn_a", "dyn_b", "dyn_c"] do + query + |> Planner.attach_comments(comments: [pre: comment]) + |> Planner.query(:all, cache, Ecto.CachingTestAdapter, 0, false) + end + + assert :ets.info(cache, :size) == 0 + end + + test "stores comments verbatim, leaving validation to the adapter" do + assert Planner.attach_comments(from(p in "posts"), comments: [pre: "a */ b"]).comments == + [pre: "a */ b"] + end + end end