diff --git a/lib/ecto/query.ex b/lib/ecto/query.ex index 0037b34a4c..4818596c4e 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, nor + start with `!`, `+`, or `M!`. 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..b1b04b81dc --- /dev/null +++ b/lib/ecto/query/builder/comment.ex @@ -0,0 +1,73 @@ +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>>] + + # Placed right after `/*`, these prefixes would form MySQL/MariaDB executable + # comments (`/*!...*/`, `/*M!...*/`) or optimizer hints (`/*+...*/`), turning + # the comment into SQL that executes. + @forbidden_prefixes ["!", "+", "M!"] + + @doc """ + Validates the comment text, returning it unchanged. + + iex> validate!("my-query") + "my-query" + + """ + @spec validate!(Macro.t()) :: Macro.t() + def validate!(comment) when is_binary(comment) do + if String.contains?(comment, @forbidden) do + Builder.error!("a comment cannot contain `/*`, `*/`, or null bytes, got: `#{comment}`") + end + + if String.starts_with?(comment, @forbidden_prefixes) do + Builder.error!( + "a comment cannot start with `!`, `+`, or `M!`, as MySQL and MariaDB " <> + "treat such comments as executable SQL or optimizer hints, got: `#{comment}`" + ) + end + + comment + end + + def validate!({:^, _, [_]}) 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 validate!(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, validate!(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 df7d428c1f..3678315bf7 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 7a831a0247..ad972f0d16 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 @@ -1106,7 +1106,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 = @@ -1123,6 +1124,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] @@ -2452,6 +2454,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 a34455dfc7..03c7da0bbf 100644 --- a/lib/ecto/repo.ex +++ b/lib/ecto/repo.ex @@ -141,6 +141,22 @@ 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, nor start with `!`, `+`, or `M!`. Because this option takes runtime values, passing it + disables caching for that call, making dynamic comments (such as per-request ids) safe by + default: query operations (`all`/`update_all`/`delete_all`) skip Ecto's query cache, and + schema operations (`insert`/`update`/`delete`/`insert_all`) make SQL adapters skip their + default prepared statement caching. If your comments are static, you can keep + caching by passing `query_cache: true` (query operations) or an explicit `:cache_statement` + (schema operations on SQL adapters). Comments given as query expressions via + `Ecto.Query.pre_comment/2` and `Ecto.Query.post_comment/2` must be compile-time literals + and remain cached as usual. This option only applies to operations that build the statement + for you — raw SQL functions such as `Ecto.Adapters.SQL.query/4` (`Repo.query/4`) ignore it, + as you control the statement text there. ## Adapter-Specific Errors diff --git a/lib/ecto/repo/queryable.ex b/lib/ecto/repo/queryable.ex index dc462a6a60..4dd2c5ddeb 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,9 +37,9 @@ 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) + query_cache? = query_cache?(opts) {query_meta, prepared, cast_params, dump_params} = Planner.query(query, :all, cache, adapter, 0, query_cache?) @@ -218,13 +218,17 @@ defmodule Ecto.Repo.Queryable do ## Helpers + defp query_cache?(opts) do + Keyword.get_lazy(opts, :query_cache, fn -> Keyword.get(opts, :comments, []) == [] end) + end + defp execute(operation, name, query, {adapter_meta, opts} = tuplet) 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) + query_cache? = query_cache?(opts) {query_meta, prepared, cast_params, dump_params} = Planner.query(query, operation, cache, adapter, 0, query_cache?) diff --git a/test/ecto/query/builder/comment_test.exs b/test/ecto/query/builder/comment_test.exs new file mode 100644 index 0000000000..803fbd7be6 --- /dev/null +++ b/test/ecto/query/builder/comment_test.exs @@ -0,0 +1,83 @@ +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 + + # `/*!...*/` and `/*M!...*/` are executable comments and `/*+...*/` is an + # optimizer hint on MySQL/MariaDB, so these prefixes must never be able to + # land right after the opening `/*`. + test "raises on a comment starting with ! (MySQL executable comment)" do + assert_raise Ecto.Query.CompileError, ~r"cannot start with `!`, `\+`, or `M!`", fn -> + quote_and_eval(%Ecto.Query{} |> pre_comment("!40000 DROP TABLE posts")) + end + end + + test "raises on a comment starting with + (MySQL optimizer hint)" do + assert_raise Ecto.Query.CompileError, ~r"cannot start with `!`, `\+`, or `M!`", fn -> + quote_and_eval(%Ecto.Query{} |> post_comment("+MAX_EXECUTION_TIME(1)")) + end + end + + test "raises on a comment starting with M! (MariaDB executable comment)" do + assert_raise Ecto.Query.CompileError, ~r"cannot start with `!`, `\+`, or `M!`", fn -> + quote_and_eval(%Ecto.Query{} |> pre_comment("M!100000 DROP TABLE posts")) + 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 9689da3983..6119fcdca3 100644 --- a/test/ecto/query/planner_test.exs +++ b/test/ecto/query/planner_test.exs @@ -3226,4 +3226,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 diff --git a/test/ecto/repo_test.exs b/test/ecto/repo_test.exs index 97c01bfe4b..effc808f70 100644 --- a/test/ecto/repo_test.exs +++ b/test/ecto/repo_test.exs @@ -2774,6 +2774,43 @@ defmodule Ecto.RepoTest do end end + describe ":comments option and the query cache" do + alias Ecto.CachingTestRepo + + test "queries are cached by default" do + CachingTestRepo.all(MySchema) + assert_received {:cache_status, :cache} + end + + test "comments disable the query cache" do + CachingTestRepo.all(MySchema, comments: [pre: "dyn_123"]) + assert_received {:cache_status, :nocache} + end + + test "explicit query_cache: true keeps caching with comments" do + CachingTestRepo.all(MySchema, comments: [pre: "static_tag"], query_cache: true) + assert_received {:cache_status, :cache} + end + + test "explicit query_cache: false disables caching without comments" do + CachingTestRepo.all(MySchema, query_cache: false) + assert_received {:cache_status, :nocache} + end + + test "comments from query expressions keep the default cache" do + MySchema |> Ecto.Query.pre_comment("from_macro") |> CachingTestRepo.all() + assert_received {:cache_status, :cache} + end + + test "update_all and delete_all follow the same rule" do + CachingTestRepo.update_all(MySchema, [set: [x: "y"]], comments: [pre: "dyn"]) + assert_received {:cache_status, :nocache} + + CachingTestRepo.delete_all(MySchema, comments: [post: "dyn"]) + assert_received {:cache_status, :nocache} + end + end + defp select_fields(fields, ix) do for field <- fields do {{:., [writable: :always], [{:&, [], [ix]}, field]}, [], []} diff --git a/test/support/test_repo.exs b/test/support/test_repo.exs index 7d7b8a322f..5cb262218b 100644 --- a/test/support/test_repo.exs +++ b/test/support/test_repo.exs @@ -188,12 +188,13 @@ defmodule Ecto.CachingTestAdapter do # Return :cache to trigger default caching in the planner def prepare(operation, query), do: {:cache, {operation, query}} - def execute(_adapter_meta, _query_meta, {_cache_status, {:all, _query}}, _dump_params, _opts) do - [] - end + def execute(_adapter_meta, _query_meta, prepared, _dump_params, _opts) do + send(self(), {:cache_status, elem(prepared, 0)}) - def execute(_adapter_meta, _query_meta, {_cache_status, {_operation, _query}}, _dump_params, _opts) do - {1, nil} + case elem(prepared, tuple_size(prepared) - 1) do + {:all, _query} -> {0, []} + {_operation, _query} -> {1, nil} + end end def stream(_adapter_meta, _query_meta, _prepared, _dump_params, _opts) do @@ -201,6 +202,12 @@ defmodule Ecto.CachingTestAdapter do end end +Application.put_env(:ecto, Ecto.CachingTestRepo, []) + +defmodule Ecto.CachingTestRepo do + use Ecto.Repo, otp_app: :ecto, adapter: Ecto.CachingTestAdapter +end + Application.put_env(:ecto, Ecto.TestRepo, user: "invalid") defmodule Ecto.TestRepo do @@ -214,3 +221,4 @@ defmodule Ecto.TestRepo do end Ecto.TestRepo.start_link() +Ecto.CachingTestRepo.start_link()