From 836ae4faa01a0d4485a29d6696cec06b2ffdaf5b Mon Sep 17 00:00:00 2001 From: Aliaksandr Sasnouski Date: Wed, 3 Jun 2026 15:04:26 +0200 Subject: [PATCH 1/6] Add Ecto.Query.label/2 query expression for tagging queries with an SQL comment --- lib/ecto/query.ex | 41 +++++++++++- lib/ecto/query/builder/label.ex | 82 +++++++++++++++++++++++ lib/ecto/query/inspect.ex | 2 + lib/ecto/query/planner.ex | 6 +- lib/ecto/repo.ex | 21 ++++++ test/ecto/query/builder/label_test.exs | 93 ++++++++++++++++++++++++++ 6 files changed, 241 insertions(+), 4 deletions(-) create mode 100644 lib/ecto/query/builder/label.ex create mode 100644 test/ecto/query/builder/label_test.exs diff --git a/lib/ecto/query.ex b/lib/ecto/query.ex index 0037b34a4c..4d37af99db 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, + label: nil 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, :label) 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, :label), do: %{query | label: nil} 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,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, :label] @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 +2540,40 @@ defmodule Ecto.Query do Builder.Lock.build(query, binding, expr, __CALLER__) end + @doc ~S""" + A label query expression. + + Adds the given text to the generated statement as a leading SQL comment, + immediately before the statement keyword: + + /* get_username_q */ SELECT ... + + This is useful to tag and identify queries in database logs and monitoring + tools. The label is rendered *leading* (rather than trailing) so it survives + truncation of long statements in logs. + + Because the label becomes part of the generated SQL, it is also part of the + query cache key. Avoid highly dynamic values (such as per-request ids) as they + would defeat Ecto's prepared-statement caching; prefer a stable identifier per + call site. + + ## Keywords example + + from(p in Post, label: "get_post_titles_q", select: p.title) + + ## Expressions example + + Post |> label("get_post_titles_q") |> select([p], p.title) + + ## Interpolation + + report = "get_posts_q" + from(p in Post, label: ^report) + """ + defmacro label(query, expr) do + Builder.Label.build(query, expr, __CALLER__) + end + @doc ~S""" An update query expression. diff --git a/lib/ecto/query/builder/label.ex b/lib/ecto/query/builder/label.ex new file mode 100644 index 0000000000..38089311d5 --- /dev/null +++ b/lib/ecto/query/builder/label.ex @@ -0,0 +1,82 @@ +import Kernel, except: [apply: 2] + +defmodule Ecto.Query.Builder.Label do + @moduledoc false + + alias Ecto.Query.Builder + + @forbidden ["*/", "/*", <<0>>] + + @doc """ + Escapes the label text. + + iex> escape(quote(do: "my-query")) + "my-query" + + """ + @spec escape(Macro.t()) :: Macro.t() + def escape(label) when is_binary(label) do + if String.contains?(label, @forbidden) do + Builder.error!(forbidden_message(label)) + end + + label + end + + def escape({:^, _, [expr]}) do + quote do + Ecto.Query.Builder.Label.runtime!(unquote(expr)) + end + end + + def escape(other) do + Builder.error!( + "`#{Macro.to_string(other)}` is not a valid label. " <> + "For security reasons, a label must be a literal string or an interpolated string" + ) + end + + @doc """ + Validates a label given at runtime via interpolation. + """ + @spec runtime!(term) :: String.t() + def runtime!(label) when is_binary(label) do + if String.contains?(label, @forbidden) do + raise ArgumentError, forbidden_message(label) + end + + label + end + + def runtime!(other) do + raise ArgumentError, "a label must be a string, got: `#{inspect(other)}`" + end + + defp forbidden_message(label) do + "a label cannot contain `/*`, `*/`, or null bytes, got: `#{inspect(label)}`. " + end + + @doc """ + Builds a quoted expression. + + The quoted expression should evaluate to a query at runtime. + If possible, it does all calculations at compile time to avoid + runtime work. + """ + @spec build(Macro.t(), Macro.t(), Macro.Env.t()) :: Macro.t() + def build(query, expr, env) do + Builder.apply_query(query, __MODULE__, [escape(expr)], env) + end + + @doc """ + The callback applied by `build/3` to build the query. + """ + @spec apply(Ecto.Queryable.t(), term) :: Ecto.Query.t() + def apply(%Ecto.Query{} = query, value) do + %{query | label: value} + end + + def apply(query, value) do + apply(Ecto.Queryable.to_query(query), value) + end +end diff --git a/lib/ecto/query/inspect.ex b/lib/ecto/query/inspect.ex index df7d428c1f..29e8daf08c 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) + label = kw_inspect(:label, query.label) 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, + label, distinct, updates, select, diff --git a/lib/ecto/query/planner.ex b/lib/ecto/query/planner.ex index 7a831a0247..8c6c04915c 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, label: label, 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(label != nil, label: label) |> prepend_if(aliases != %{}, aliases: aliases) [operation | cache] diff --git a/lib/ecto/repo.ex b/lib/ecto/repo.ex index a34455dfc7..e1f5b1b1e3 100644 --- a/lib/ecto/repo.ex +++ b/lib/ecto/repo.ex @@ -1581,6 +1581,13 @@ defmodule Ecto.Repo do in the schema. For more information see the ["Query Prefix"](`m:Ecto.Query#module-query-prefix`) section of the `Ecto.Query` documentation. + * `:label` - A string to tag the generated `UPDATE` with a leading SQL + comment, such as `/* bump_visits */ UPDATE ...`, to identify the statement + in database logs and monitoring tools. The string is embedded verbatim into + the SQL and therefore cannot contain `/*`, `*/`, or null bytes. Prefer a + stable identifier per call site, as the label becomes part of the query + cache key. See `Ecto.Query.label/2` to set it on the query itself instead. + See the ["Shared options"](#module-shared-options) section at the module documentation for remaining options. @@ -1630,6 +1637,13 @@ defmodule Ecto.Repo do in the schema. For more information see the ["Query Prefix"](`m:Ecto.Query#module-query-prefix`) section of the `Ecto.Query` documentation. + * `:label` - A string to tag the generated `DELETE` with a leading SQL + comment, such as `/* purge_stale */ DELETE ...`, to identify the statement + in database logs and monitoring tools. The string is embedded verbatim into + the SQL and therefore cannot contain `/*`, `*/`, or null bytes. Prefer a + stable identifier per call site, as the label becomes part of the query + cache key. See `Ecto.Query.label/2` to set it on the query itself instead. + See the ["Shared options"](#module-shared-options) section at the module documentation for remaining options. @@ -1732,6 +1746,13 @@ defmodule Ecto.Repo do * `:placeholders` - A map with placeholders. This feature is not supported by all databases. See the ["Placeholders" section](#c:insert_all/3-placeholders) for more information. + * `:label` - A string to tag the generated `INSERT` with a leading SQL + comment, such as `/* import_users */ INSERT ...`, to identify the statement + in database logs and monitoring tools. The string is embedded verbatim into + the SQL and therefore cannot contain `/*`, `*/`, or null bytes. Prefer a + stable identifier per call site, as the label becomes part of the query + cache key. + See the ["Shared options"](#module-shared-options) section at the module documentation for remaining options. diff --git a/test/ecto/query/builder/label_test.exs b/test/ecto/query/builder/label_test.exs new file mode 100644 index 0000000000..c93fbf77c2 --- /dev/null +++ b/test/ecto/query/builder/label_test.exs @@ -0,0 +1,93 @@ +Code.require_file "../../../support/eval_helpers.exs", __DIR__ + +defmodule Ecto.Query.Builder.LabelTest do + use ExUnit.Case, async: true + + import Ecto.Query.Builder.Label + doctest Ecto.Query.Builder.Label + + import Ecto.Query + import Support.EvalHelpers + + test "label with literal string" do + query = %Ecto.Query{} |> label("my-report") + assert query.label == "my-report" + end + + test "label via keyword syntax" do + query = from p in "posts", label: "list-posts" + assert query.label == "list-posts" + end + + test "label with interpolated string" do + report = "monthly-report" + query = %Ecto.Query{} |> label(^report) + assert query.label == "monthly-report" + end + + test "overrides on duplicated label" do + query = %Ecto.Query{} |> label("FOO") |> label("BAR") + assert query.label == "BAR" + end + + test "raises on non-string at compile time" do + assert_raise Ecto.Query.CompileError, ~r"`1` is not a valid label", fn -> + quote_and_eval(%Ecto.Query{} |> label(1)) + end + end + + test "raises on literal containing */" do + assert_raise Ecto.Query.CompileError, ~r"cannot contain `/\*`, `\*/`, or null bytes", fn -> + quote_and_eval(%Ecto.Query{} |> label("evil */ DROP TABLE")) + end + end + + test "raises on literal containing /*" do + assert_raise Ecto.Query.CompileError, ~r"cannot contain `/\*`, `\*/`, or null bytes", fn -> + quote_and_eval(%Ecto.Query{} |> label("evil /* nested")) + end + end + + test "raises on literal containing a null byte" do + assert_raise Ecto.Query.CompileError, ~r"cannot contain `/\*`, `\*/`, or null bytes", fn -> + quote_and_eval(%Ecto.Query{} |> label("nul\0byte")) + end + end + + test "raises on interpolated value containing a null byte" do + evil = "nul\0byte" + + assert_raise ArgumentError, ~r"cannot contain `/\*`, `\*/`, or null bytes", fn -> + %Ecto.Query{} |> label(^evil) + end + end + + test "raises on interpolated value containing */" do + evil = "evil */ DROP TABLE" + + assert_raise ArgumentError, ~r"cannot contain `/\*`, `\*/`, or null bytes", fn -> + %Ecto.Query{} |> label(^evil) + end + end + + test "raises on interpolated value containing /*" do + evil = "evil /* nested" + + assert_raise ArgumentError, ~r"cannot contain `/\*`, `\*/`, or null bytes", fn -> + %Ecto.Query{} |> label(^evil) + end + end + + test "raises on interpolated non-string" do + not_a_string = 123 + + assert_raise ArgumentError, ~r"must be a string", fn -> + %Ecto.Query{} |> label(^not_a_string) + end + end + + test "exclude resets the label" do + query = %Ecto.Query{} |> label("FOO") + assert Ecto.Query.exclude(query, :label).label == nil + end +end From cdffdb5de83e974f08a7dbfee5ab87fe11d8b8fa Mon Sep 17 00:00:00 2001 From: Aliaksandr Sasnouski Date: Thu, 4 Jun 2026 11:10:00 +0200 Subject: [PATCH 2/6] Make :label a repo option instead of a query expression. --- lib/ecto/query.ex | 37 +--------- lib/ecto/query/builder/label.ex | 82 ----------------------- lib/ecto/query/inspect.ex | 2 - lib/ecto/query/planner.ex | 30 +++++++++ lib/ecto/repo.ex | 27 ++------ lib/ecto/repo/queryable.ex | 6 +- test/ecto/query/builder/label_test.exs | 93 -------------------------- test/ecto/query/planner_test.exs | 46 +++++++++++++ 8 files changed, 86 insertions(+), 237 deletions(-) delete mode 100644 lib/ecto/query/builder/label.ex delete mode 100644 test/ecto/query/builder/label_test.exs diff --git a/lib/ecto/query.ex b/lib/ecto/query.ex index 4d37af99db..bedbccd868 100644 --- a/lib/ecto/query.ex +++ b/lib/ecto/query.ex @@ -956,7 +956,6 @@ defmodule Ecto.Query do Ecto.Query.exclude(query, :limit) Ecto.Query.exclude(query, :offset) Ecto.Query.exclude(query, :lock) - Ecto.Query.exclude(query, :label) Ecto.Query.exclude(query, :preload) Ecto.Query.exclude(query, :update) Ecto.Query.exclude(query, :windows) @@ -1026,7 +1025,6 @@ 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, :label), do: %{query | label: nil} 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: []} @@ -1153,7 +1151,7 @@ defmodule Ecto.Query do end @from_join_opts [:as, :prefix, :hints] - @no_binds [:union, :union_all, :except, :except_all, :intersect, :intersect_all, :label] + @no_binds [:union, :union_all, :except, :except_all, :intersect, :intersect_all] @binds [:lock, :where, :or_where, :select, :distinct, :order_by, :group_by, :windows] ++ [:having, :or_having, :limit, :offset, :preload, :update, :select_merge, :with_ctes] @@ -2540,39 +2538,6 @@ defmodule Ecto.Query do Builder.Lock.build(query, binding, expr, __CALLER__) end - @doc ~S""" - A label query expression. - - Adds the given text to the generated statement as a leading SQL comment, - immediately before the statement keyword: - - /* get_username_q */ SELECT ... - - This is useful to tag and identify queries in database logs and monitoring - tools. The label is rendered *leading* (rather than trailing) so it survives - truncation of long statements in logs. - - Because the label becomes part of the generated SQL, it is also part of the - query cache key. Avoid highly dynamic values (such as per-request ids) as they - would defeat Ecto's prepared-statement caching; prefer a stable identifier per - call site. - - ## Keywords example - - from(p in Post, label: "get_post_titles_q", select: p.title) - - ## Expressions example - - Post |> label("get_post_titles_q") |> select([p], p.title) - - ## Interpolation - - report = "get_posts_q" - from(p in Post, label: ^report) - """ - defmacro label(query, expr) do - Builder.Label.build(query, expr, __CALLER__) - end @doc ~S""" An update query expression. diff --git a/lib/ecto/query/builder/label.ex b/lib/ecto/query/builder/label.ex deleted file mode 100644 index 38089311d5..0000000000 --- a/lib/ecto/query/builder/label.ex +++ /dev/null @@ -1,82 +0,0 @@ -import Kernel, except: [apply: 2] - -defmodule Ecto.Query.Builder.Label do - @moduledoc false - - alias Ecto.Query.Builder - - @forbidden ["*/", "/*", <<0>>] - - @doc """ - Escapes the label text. - - iex> escape(quote(do: "my-query")) - "my-query" - - """ - @spec escape(Macro.t()) :: Macro.t() - def escape(label) when is_binary(label) do - if String.contains?(label, @forbidden) do - Builder.error!(forbidden_message(label)) - end - - label - end - - def escape({:^, _, [expr]}) do - quote do - Ecto.Query.Builder.Label.runtime!(unquote(expr)) - end - end - - def escape(other) do - Builder.error!( - "`#{Macro.to_string(other)}` is not a valid label. " <> - "For security reasons, a label must be a literal string or an interpolated string" - ) - end - - @doc """ - Validates a label given at runtime via interpolation. - """ - @spec runtime!(term) :: String.t() - def runtime!(label) when is_binary(label) do - if String.contains?(label, @forbidden) do - raise ArgumentError, forbidden_message(label) - end - - label - end - - def runtime!(other) do - raise ArgumentError, "a label must be a string, got: `#{inspect(other)}`" - end - - defp forbidden_message(label) do - "a label cannot contain `/*`, `*/`, or null bytes, got: `#{inspect(label)}`. " - end - - @doc """ - Builds a quoted expression. - - The quoted expression should evaluate to a query at runtime. - If possible, it does all calculations at compile time to avoid - runtime work. - """ - @spec build(Macro.t(), Macro.t(), Macro.Env.t()) :: Macro.t() - def build(query, expr, env) do - Builder.apply_query(query, __MODULE__, [escape(expr)], env) - end - - @doc """ - The callback applied by `build/3` to build the query. - """ - @spec apply(Ecto.Queryable.t(), term) :: Ecto.Query.t() - def apply(%Ecto.Query{} = query, value) do - %{query | label: value} - end - - def apply(query, value) do - apply(Ecto.Queryable.to_query(query), value) - end -end diff --git a/lib/ecto/query/inspect.ex b/lib/ecto/query/inspect.ex index 29e8daf08c..df7d428c1f 100644 --- a/lib/ecto/query/inspect.ex +++ b/lib/ecto/query/inspect.ex @@ -124,7 +124,6 @@ defimpl Inspect, for: Ecto.Query do updates = kw_exprs(:update, query.updates, names) lock = kw_inspect(:lock, query.lock) - label = kw_inspect(:label, query.label) offset = kw_expr(:offset, query.offset, names) select = kw_expr(:select, query.select, names) distinct = kw_expr(:distinct, query.distinct, names) @@ -141,7 +140,6 @@ defimpl Inspect, for: Ecto.Query do limit, offset, lock, - label, distinct, updates, select, diff --git a/lib/ecto/query/planner.ex b/lib/ecto/query/planner.ex index 8c6c04915c..b46e900faa 100644 --- a/lib/ecto/query/planner.ex +++ b/lib/ecto/query/planner.ex @@ -2454,6 +2454,36 @@ defmodule Ecto.Query.Planner do def attach_prefix(query, _), do: query + @doc """ + Puts the label given via `opts` into the given query, if available. + + 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. + """ + 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)} + :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 + ## Helpers @all_exprs [ diff --git a/lib/ecto/repo.ex b/lib/ecto/repo.ex index e1f5b1b1e3..b81996b5a7 100644 --- a/lib/ecto/repo.ex +++ b/lib/ecto/repo.ex @@ -141,6 +141,12 @@ 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. ## Adapter-Specific Errors @@ -1581,13 +1587,6 @@ defmodule Ecto.Repo do in the schema. For more information see the ["Query Prefix"](`m:Ecto.Query#module-query-prefix`) section of the `Ecto.Query` documentation. - * `:label` - A string to tag the generated `UPDATE` with a leading SQL - comment, such as `/* bump_visits */ UPDATE ...`, to identify the statement - in database logs and monitoring tools. The string is embedded verbatim into - the SQL and therefore cannot contain `/*`, `*/`, or null bytes. Prefer a - stable identifier per call site, as the label becomes part of the query - cache key. See `Ecto.Query.label/2` to set it on the query itself instead. - See the ["Shared options"](#module-shared-options) section at the module documentation for remaining options. @@ -1637,13 +1636,6 @@ defmodule Ecto.Repo do in the schema. For more information see the ["Query Prefix"](`m:Ecto.Query#module-query-prefix`) section of the `Ecto.Query` documentation. - * `:label` - A string to tag the generated `DELETE` with a leading SQL - comment, such as `/* purge_stale */ DELETE ...`, to identify the statement - in database logs and monitoring tools. The string is embedded verbatim into - the SQL and therefore cannot contain `/*`, `*/`, or null bytes. Prefer a - stable identifier per call site, as the label becomes part of the query - cache key. See `Ecto.Query.label/2` to set it on the query itself instead. - See the ["Shared options"](#module-shared-options) section at the module documentation for remaining options. @@ -1746,13 +1738,6 @@ defmodule Ecto.Repo do * `:placeholders` - A map with placeholders. This feature is not supported by all databases. See the ["Placeholders" section](#c:insert_all/3-placeholders) for more information. - * `:label` - A string to tag the generated `INSERT` with a leading SQL - comment, such as `/* import_users */ INSERT ...`, to identify the statement - in database logs and monitoring tools. The string is embedded verbatim into - the SQL and therefore cannot contain `/*`, `*/`, or null bytes. Prefer a - stable identifier per call site, as the label becomes part of the query - cache key. - See the ["Shared options"](#module-shared-options) section at the module documentation for remaining options. diff --git a/lib/ecto/repo/queryable.ex b/lib/ecto/repo/queryable.ex index dc462a6a60..56c70a2642 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_label: 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_label(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_label(opts) query_cache? = Keyword.get(opts, :query_cache, true) diff --git a/test/ecto/query/builder/label_test.exs b/test/ecto/query/builder/label_test.exs deleted file mode 100644 index c93fbf77c2..0000000000 --- a/test/ecto/query/builder/label_test.exs +++ /dev/null @@ -1,93 +0,0 @@ -Code.require_file "../../../support/eval_helpers.exs", __DIR__ - -defmodule Ecto.Query.Builder.LabelTest do - use ExUnit.Case, async: true - - import Ecto.Query.Builder.Label - doctest Ecto.Query.Builder.Label - - import Ecto.Query - import Support.EvalHelpers - - test "label with literal string" do - query = %Ecto.Query{} |> label("my-report") - assert query.label == "my-report" - end - - test "label via keyword syntax" do - query = from p in "posts", label: "list-posts" - assert query.label == "list-posts" - end - - test "label with interpolated string" do - report = "monthly-report" - query = %Ecto.Query{} |> label(^report) - assert query.label == "monthly-report" - end - - test "overrides on duplicated label" do - query = %Ecto.Query{} |> label("FOO") |> label("BAR") - assert query.label == "BAR" - end - - test "raises on non-string at compile time" do - assert_raise Ecto.Query.CompileError, ~r"`1` is not a valid label", fn -> - quote_and_eval(%Ecto.Query{} |> label(1)) - end - end - - test "raises on literal containing */" do - assert_raise Ecto.Query.CompileError, ~r"cannot contain `/\*`, `\*/`, or null bytes", fn -> - quote_and_eval(%Ecto.Query{} |> label("evil */ DROP TABLE")) - end - end - - test "raises on literal containing /*" do - assert_raise Ecto.Query.CompileError, ~r"cannot contain `/\*`, `\*/`, or null bytes", fn -> - quote_and_eval(%Ecto.Query{} |> label("evil /* nested")) - end - end - - test "raises on literal containing a null byte" do - assert_raise Ecto.Query.CompileError, ~r"cannot contain `/\*`, `\*/`, or null bytes", fn -> - quote_and_eval(%Ecto.Query{} |> label("nul\0byte")) - end - end - - test "raises on interpolated value containing a null byte" do - evil = "nul\0byte" - - assert_raise ArgumentError, ~r"cannot contain `/\*`, `\*/`, or null bytes", fn -> - %Ecto.Query{} |> label(^evil) - end - end - - test "raises on interpolated value containing */" do - evil = "evil */ DROP TABLE" - - assert_raise ArgumentError, ~r"cannot contain `/\*`, `\*/`, or null bytes", fn -> - %Ecto.Query{} |> label(^evil) - end - end - - test "raises on interpolated value containing /*" do - evil = "evil /* nested" - - assert_raise ArgumentError, ~r"cannot contain `/\*`, `\*/`, or null bytes", fn -> - %Ecto.Query{} |> label(^evil) - end - end - - test "raises on interpolated non-string" do - not_a_string = 123 - - assert_raise ArgumentError, ~r"must be a string", fn -> - %Ecto.Query{} |> label(^not_a_string) - end - end - - test "exclude resets the label" do - query = %Ecto.Query{} |> label("FOO") - assert Ecto.Query.exclude(query, :label).label == nil - end -end diff --git a/test/ecto/query/planner_test.exs b/test/ecto/query/planner_test.exs index 9689da3983..a57cf5d3be 100644 --- a/test/ecto/query/planner_test.exs +++ b/test/ecto/query/planner_test.exs @@ -3226,4 +3226,50 @@ defmodule Ecto.Query.PlannerTest do assert :ets.info(cache, :size) == 0 end end + + describe "attach_label/2" do + test "sets the label from opts" do + query = Planner.attach_label(from(p in "posts"), label: "get_posts_q") + assert query.label == "get_posts_q" + end + + test "leaves the query unchanged when no label is given" do + query = from(p in "posts") + assert Planner.attach_label(query, []).label == nil + 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 label -> + query + |> Planner.attach_label(label: label) + |> Planner.query(:all, cache, Ecto.CachingTestAdapter, 0, true) + end + + # Different labels produce distinct cache entries... + cache_query.("a") + cache_query.("b") + assert :ets.info(cache, :size) == 2 + + # ...while the same label reuses one. + cache_query.("a") + assert :ets.info(cache, :size) == 2 + end + + test "raises on a non-string label" do + assert_raise ArgumentError, ~r/must be a string/, fn -> + Planner.attach_label(from(p in "posts"), label: 123) + end + end + + test "raises on a label that could break out of the comment" do + for bad <- ["a */ b", "a /* b", "a\0b"] do + assert_raise ArgumentError, ~r/cannot contain/, fn -> + Planner.attach_label(from(p in "posts"), label: bad) + end + end + end + end end From c04714ff392349dd6d3da4e68f665cfee57d7956 Mon Sep 17 00:00:00 2001 From: Aliaksandr Sasnouski Date: Tue, 16 Jun 2026 15:37:23 +0200 Subject: [PATCH 3/6] Support pre_comment and post_comment options in ecto. --- lib/ecto/query.ex | 57 ++++++++++++++++++++- lib/ecto/query/builder/comment.ex | 63 ++++++++++++++++++++++++ lib/ecto/query/inspect.ex | 6 +++ lib/ecto/query/planner.ex | 37 +++++--------- lib/ecto/repo.ex | 17 ++++--- lib/ecto/repo/queryable.ex | 6 +-- test/ecto/query/builder/comment_test.exs | 62 +++++++++++++++++++++++ test/ecto/query/planner_test.exs | 57 ++++++++++++++------- 8 files changed, 251 insertions(+), 54 deletions(-) create mode 100644 lib/ecto/query/builder/comment.ex create mode 100644 test/ecto/query/builder/comment_test.exs diff --git a/lib/ecto/query.ex b/lib/ecto/query.ex index bedbccd868..914ed80c2f 100644 --- a/lib/ecto/query.ex +++ b/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. 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 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 b46e900faa..ad972f0d16 100644 --- a/lib/ecto/query/planner.ex +++ b/lib/ecto/query/planner.ex @@ -1106,7 +1106,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) @@ -1124,7 +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(label != nil, label: label) + |> prepend_if(comments != [], comments: comments) |> prepend_if(aliases != %{}, aliases: aliases) [operation | cache] @@ -2455,34 +2455,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 diff --git a/lib/ecto/repo.ex b/lib/ecto/repo.ex index b81996b5a7..d3e44bf982 100644 --- a/lib/ecto/repo.ex +++ b/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 diff --git a/lib/ecto/repo/queryable.ex b/lib/ecto/repo/queryable.ex index 56c70a2642..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, 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) 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 a57cf5d3be..6119fcdca3 100644 --- a/test/ecto/query/planner_test.exs +++ b/test/ecto/query/planner_test.exs @@ -3227,49 +3227,68 @@ defmodule Ecto.Query.PlannerTest do end end - describe "attach_label/2" do - test "sets the label from opts" do - query = Planner.attach_label(from(p in "posts"), label: "get_posts_q") - assert query.label == "get_posts_q" + 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 "leaves the query unchanged when no label is given" do + 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_label(query, []).label == nil + 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 label -> + cache_query = fn comment -> query - |> Planner.attach_label(label: label) + |> Planner.attach_comments(comments: [pre: comment]) |> Planner.query(:all, cache, Ecto.CachingTestAdapter, 0, true) end - # Different labels produce distinct cache entries... + # Different comments produce distinct cache entries... cache_query.("a") cache_query.("b") assert :ets.info(cache, :size) == 2 - # ...while the same label reuses one. + # ...while the same comment reuses one. cache_query.("a") assert :ets.info(cache, :size) == 2 end - test "raises on a non-string label" do - assert_raise ArgumentError, ~r/must be a string/, fn -> - Planner.attach_label(from(p in "posts"), label: 123) + 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 "raises on a label that could break out of the comment" do - for bad <- ["a */ b", "a /* b", "a\0b"] do - assert_raise ArgumentError, ~r/cannot contain/, fn -> - Planner.attach_label(from(p in "posts"), label: bad) - end - 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 From d0b2ce8e87a4b6070d5e5eaa0f60acd00b06e052 Mon Sep 17 00:00:00 2001 From: Aliaksandr Sasnouski Date: Sat, 29 Aug 2026 12:02:00 +0200 Subject: [PATCH 4/6] Skip the query cache when the :comments option is given. --- lib/ecto/repo.ex | 15 +++++++++------ lib/ecto/repo/queryable.ex | 8 ++++++-- test/ecto/repo_test.exs | 37 +++++++++++++++++++++++++++++++++++++ test/support/test_repo.exs | 18 +++++++++++++----- 4 files changed, 65 insertions(+), 13 deletions(-) diff --git a/lib/ecto/repo.ex b/lib/ecto/repo.ex index d3e44bf982..8b184e0e0d 100644 --- a/lib/ecto/repo.ex +++ b/lib/ecto/repo.ex @@ -146,12 +146,15 @@ defmodule Ecto.Repo do 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. + contain `/*`, `*/`, or null bytes. 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. ## Adapter-Specific Errors diff --git a/lib/ecto/repo/queryable.ex b/lib/ecto/repo/queryable.ex index 251cd93926..4dd2c5ddeb 100644 --- a/lib/ecto/repo/queryable.ex +++ b/lib/ecto/repo/queryable.ex @@ -39,7 +39,7 @@ defmodule Ecto.Repo.Queryable do {query, opts} = repo.prepare_query(:stream, 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 = 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/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() From 19eb7a3f24e683dd9bcd55b1071ae06bddecdd3c Mon Sep 17 00:00:00 2001 From: Aliaksandr Sasnouski Date: Sat, 29 Aug 2026 12:12:28 +0200 Subject: [PATCH 5/6] Reject comment prefixes MySQL/MariaDB treat as executable. --- lib/ecto/query.ex | 7 ++++--- lib/ecto/query/builder/comment.ex | 12 +++++++++++- lib/ecto/repo.ex | 2 +- test/ecto/query/builder/comment_test.exs | 21 +++++++++++++++++++++ 4 files changed, 37 insertions(+), 5 deletions(-) diff --git a/lib/ecto/query.ex b/lib/ecto/query.ex index 914ed80c2f..19e6e1720f 100644 --- a/lib/ecto/query.ex +++ b/lib/ecto/query.ex @@ -2555,9 +2555,10 @@ defmodule Ecto.Query do 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. + 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 diff --git a/lib/ecto/query/builder/comment.ex b/lib/ecto/query/builder/comment.ex index 3d52362b15..f8c72b4806 100644 --- a/lib/ecto/query/builder/comment.ex +++ b/lib/ecto/query/builder/comment.ex @@ -11,6 +11,11 @@ defmodule Ecto.Query.Builder.Comment do # 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 """ Escapes the comment text. @@ -21,8 +26,13 @@ defmodule Ecto.Query.Builder.Comment do @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 + + if String.starts_with?(comment, @forbidden_prefixes) do Builder.error!( - "a comment cannot contain `/*`, `*/`, or null bytes, got: `#{comment}`. " + "a comment cannot start with `!`, `+`, or `M!`, as MySQL and MariaDB " <> + "treat such comments as executable SQL or optimizer hints, got: `#{comment}`" ) end diff --git a/lib/ecto/repo.ex b/lib/ecto/repo.ex index 8b184e0e0d..a27bf0226d 100644 --- a/lib/ecto/repo.ex +++ b/lib/ecto/repo.ex @@ -146,7 +146,7 @@ defmodule Ecto.Repo do 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. Because this option takes runtime values, passing it + 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 diff --git a/test/ecto/query/builder/comment_test.exs b/test/ecto/query/builder/comment_test.exs index 5a7befedb0..803fbd7be6 100644 --- a/test/ecto/query/builder/comment_test.exs +++ b/test/ecto/query/builder/comment_test.exs @@ -55,6 +55,27 @@ defmodule Ecto.Query.Builder.CommentTest do 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 == [] From 9e91ed12b2e7aead2799713e0e6b2cd0abb00e77 Mon Sep 17 00:00:00 2001 From: Aliaksandr Sasnouski Date: Sat, 29 Aug 2026 12:18:33 +0200 Subject: [PATCH 6/6] Rename comment escape to validate! and polish docs. --- lib/ecto/query.ex | 1 - lib/ecto/query/builder/comment.ex | 14 +++++++------- lib/ecto/repo.ex | 4 +++- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/ecto/query.ex b/lib/ecto/query.ex index 19e6e1720f..4818596c4e 100644 --- a/lib/ecto/query.ex +++ b/lib/ecto/query.ex @@ -2592,7 +2592,6 @@ defmodule Ecto.Query 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 index f8c72b4806..b1b04b81dc 100644 --- a/lib/ecto/query/builder/comment.ex +++ b/lib/ecto/query/builder/comment.ex @@ -17,14 +17,14 @@ defmodule Ecto.Query.Builder.Comment do @forbidden_prefixes ["!", "+", "M!"] @doc """ - Escapes the comment text. + Validates the comment text, returning it unchanged. - iex> escape("my-query") + iex> validate!("my-query") "my-query" """ - @spec escape(Macro.t()) :: Macro.t() - def escape(comment) when is_binary(comment) do + @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 @@ -39,7 +39,7 @@ defmodule Ecto.Query.Builder.Comment do comment end - def escape({:^, _, [_]}) do + 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 " <> @@ -47,7 +47,7 @@ defmodule Ecto.Query.Builder.Comment do ) end - def escape(other) do + def validate!(other) do Builder.error!("`#{Macro.to_string(other)}` is not a valid comment, it must be a literal string") end @@ -56,7 +56,7 @@ defmodule Ecto.Query.Builder.Comment do """ @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) + Builder.apply_query(query, __MODULE__, [position, validate!(expr)], env) end @doc """ diff --git a/lib/ecto/repo.ex b/lib/ecto/repo.ex index a27bf0226d..03c7da0bbf 100644 --- a/lib/ecto/repo.ex +++ b/lib/ecto/repo.ex @@ -154,7 +154,9 @@ defmodule Ecto.Repo do 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. + 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