diff --git a/lib/ecto/query.ex b/lib/ecto/query.ex index 5af3cd3f4c..3a04c73eb0 100644 --- a/lib/ecto/query.ex +++ b/lib/ecto/query.ex @@ -397,6 +397,7 @@ defmodule Ecto.Query do """ defstruct prefix: nil, sources: nil, + comments: [], from: nil, joins: [], aliases: %{}, @@ -426,6 +427,11 @@ defmodule Ecto.Query do defstruct [:fun, :binding, :file, :line] end + defmodule CommentExpr do + @moduledoc false + defstruct [:expr, :file, :line, params: []] + end + defmodule QueryExpr do @moduledoc false defstruct [:expr, :file, :line, params: []] @@ -939,6 +945,7 @@ defmodule Ecto.Query do Ecto.Query.exclude(query, :distinct) Ecto.Query.exclude(query, :select) Ecto.Query.exclude(query, :combinations) + Ecto.Query.exclude(query, :comments) Ecto.Query.exclude(query, :with_ctes) Ecto.Query.exclude(query, :limit) Ecto.Query.exclude(query, :offset) @@ -980,6 +987,7 @@ defmodule Ecto.Query do defp do_exclude(%Ecto.Query{} = query, :order_by), do: %{query | order_bys: []} defp do_exclude(%Ecto.Query{} = query, :group_by), do: %{query | group_bys: []} defp do_exclude(%Ecto.Query{} = query, :combinations), do: %{query | combinations: []} + defp do_exclude(%Ecto.Query{} = query, :comments), do: %{query | comments: []} defp do_exclude(%Ecto.Query{} = query, :with_ctes), do: %{query | with_ctes: nil} defp do_exclude(%Ecto.Query{} = query, :having), do: %{query | havings: []} defp do_exclude(%Ecto.Query{} = query, :distinct), do: %{query | distinct: nil} @@ -1109,7 +1117,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, :comment] @binds [:lock, :where, :or_where, :select, :distinct, :order_by, :group_by, :windows] ++ [:having, :or_having, :limit, :offset, :preload, :update, :select_merge, :with_ctes] @@ -1734,6 +1742,10 @@ defmodule Ecto.Query do Builder.Select.build(:merge, query, binding, expr, __CALLER__) end + defmacro comment(query, comment) do + Builder.Comment.build(query, comment, __CALLER__) + end + @doc """ A distinct query expression. diff --git a/lib/ecto/query/builder/comment.ex b/lib/ecto/query/builder/comment.ex new file mode 100644 index 0000000000..f4977bae50 --- /dev/null +++ b/lib/ecto/query/builder/comment.ex @@ -0,0 +1,62 @@ +import Kernel, except: [apply: 2] + +defmodule Ecto.Query.Builder.Comment do + @moduledoc false + + alias Ecto.Query.Builder + alias Ecto.Query.CommentExpr + + @spec escape(Macro.t(), Macro.Env.t()) :: Macro.t() + def escape(comment, _env) when is_binary(comment), do: comment + + def escape(expr, env) do + {expr, {_params, _acc}} = + Builder.escape(expr, :any, {[], %{}}, [], env) + + expr + end + + @doc """ + Called at runtime to assemble comment. + """ + def comment!(query, comment, file, line) do + comment = %CommentExpr{ + expr: comment, + line: line, + file: file + } + + apply(query, comment) + end + + def build(query, {:^, _, [var]}, env) do + quote do + Ecto.Query.Builder.Comment.comment!( + unquote(query), + unquote(var), + unquote(env.file), + unquote(env.line) + ) + end + end + + def build(query, comment, env) do + Builder.apply_query(query, __MODULE__, [escape(comment, env)], env) + end + + def build(query, comment, _file, _line) do + apply(query, comment) + end + + @doc """ + The callback applied by `build/4` to build the query. + """ + @spec apply(Ecto.Queryable.t(), String.t() | CommentExpr.t()) :: Ecto.Query.t() + def apply(%Ecto.Query{comments: comments} = query, comment) do + %{query | comments: comments ++ [comment]} + end + + def apply(query, comment) do + apply(Ecto.Queryable.to_query(query), comment) + end +end diff --git a/lib/ecto/query/inspect.ex b/lib/ecto/query/inspect.ex index 3911c9ed3f..dc38071d7f 100644 --- a/lib/ecto/query/inspect.ex +++ b/lib/ecto/query/inspect.ex @@ -1,7 +1,7 @@ import Inspect.Algebra import Kernel, except: [to_string: 1] -alias Ecto.Query.{DynamicExpr, JoinExpr, QueryExpr, WithExpr, LimitExpr} +alias Ecto.Query.{DynamicExpr, JoinExpr, QueryExpr, WithExpr, LimitExpr, CommentExpr} defimpl Inspect, for: Ecto.Query.DynamicExpr do def inspect(%DynamicExpr{binding: binding} = dynamic, opts) do @@ -50,6 +50,11 @@ defimpl Inspect, for: Ecto.Query do result = container_doc("#Ecto.Query<", list, ">", opts, fn str, _ -> str end) + result = + if query.comments == [], + do: result, + else: concat([comment(query.comments), "\n", result]) + case query.with_ctes do %WithExpr{recursive: recursive, queries: [_ | _] = queries} -> with_ctes = @@ -161,6 +166,13 @@ defimpl Inspect, for: Ecto.Query do inspect(if source == schema.__schema__(:source), do: schema, else: {source, schema}) end + defp comment(comments) do + Enum.map_join(comments, "\n", fn + comment when is_binary(comment) -> "# #{comment}" + %CommentExpr{expr: comment} -> "# #{comment}" + end) + end + defp joins(joins, names) do joins |> Enum.with_index() diff --git a/lib/ecto/query/planner.ex b/lib/ecto/query/planner.ex index b4d16403e5..0f7045693c 100644 --- a/lib/ecto/query/planner.ex +++ b/lib/ecto/query/planner.ex @@ -5,6 +5,7 @@ defmodule Ecto.Query.Planner do alias Ecto.Query.{ BooleanExpr, ByExpr, + CommentExpr, DynamicExpr, FromExpr, JoinExpr, @@ -13,7 +14,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 @@ -977,6 +978,19 @@ defmodule Ecto.Query.Planner do end) end + defp merge_cache(:comment, _query, [], cache_and_params, _operation, _adapter) do + cache_and_params + end + + defp merge_cache(:comment, _query, comments, cache_and_params, _operation, _adapter) do + # binary comments can be cached, expression comments are dynamic so we don't cache the query + Enum.reduce_while(comments, cache_and_params, fn + comment, {cache, params} when is_binary(comment) -> {:cont, {merge_cache({:comment, comment}, cache, true), params}} + %CommentExpr{}, {_cache, params} -> {:halt, {:nocache, params}} + end) + end + + defp merge_cache(:with_cte, _query, nil, cache_and_params, _operation, _adapter) do cache_and_params end @@ -1348,10 +1362,23 @@ defmodule Ecto.Query.Planner do else {nil, counter} end + + end + + defp validate_and_increment(:comment, query, exprs, counter, _operation, adapter) do + {exprs, counter} = + Enum.reduce(exprs, {[], counter}, fn + comment, {list, acc} when is_binary(comment) -> {[comment | list], acc + 1} + %CommentExpr{} = expr, {list, acc} -> + {expr, acc} = prewalk(:comment, query, expr, acc, adapter) + {[expr | list], acc} + end) + + {Enum.reverse(exprs), counter} end defp validate_and_increment(kind, query, exprs, counter, _operation, adapter) - when kind in ~w(where group_by having order_by update)a do + when kind in ~w(where group_by having order_by update comment)a do {exprs, counter} = Enum.reduce(exprs, {[], counter}, fn %{expr: []}, {list, acc} -> @@ -2328,6 +2355,7 @@ defmodule Ecto.Query.Planner do ## Helpers @all_exprs [ + comment: :comments, with_cte: :with_ctes, distinct: :distinct, select: :select, @@ -2350,6 +2378,7 @@ defmodule Ecto.Query.Planner do # The only way to address it is by splitting how join # and their on expressions are processed. @update_all_exprs [ + comment: :comments, with_cte: :with_ctes, from: :from, update: :updates, @@ -2359,6 +2388,7 @@ defmodule Ecto.Query.Planner do ] @delete_all_exprs [ + comment: :comments, with_cte: :with_ctes, from: :from, join: :joins, @@ -2380,6 +2410,7 @@ defmodule Ecto.Query.Planner do Enum.reduce(exprs, {query, acc}, fn {kind, key}, {query, acc} -> {traversed, acc} = fun.(kind, query, Map.fetch!(query, key), acc) {%{query | key => traversed}, acc} + end) end diff --git a/test/ecto/query/inspect_test.exs b/test/ecto/query/inspect_test.exs index 8ae7040a5d..7cecd65ebd 100644 --- a/test/ecto/query/inspect_test.exs +++ b/test/ecto/query/inspect_test.exs @@ -571,6 +571,61 @@ defmodule Ecto.Query.InspectTest do assert i(plan(query)) == "from v0 in values (#{fields})" end + test "with static comments" do + query = from(p in "posts") |> comment("foo") |> comment("bar") + i = inspect(query, safe: false) + + assert i == """ + # foo + # bar + #Ecto.Query\ + """ + + i = inspect(plan(query), safe: false) + + assert i == """ + # foo + # bar + #Ecto.Query\ + """ + end + + test "with dynamic comments" do + query = from(p in "posts") |> comment(^"foo #{"bar"}") + i = inspect(query, safe: false) + + assert i == """ + # foo bar + #Ecto.Query\ + """ + + i = inspect(plan(query), safe: false) + + assert i == """ + # foo bar + #Ecto.Query\ + """ + end + + test "comments in keyword query" do + query = from(p in "posts", comment: "foo", comment: ^"bar #{"baz"}") + i = inspect(query, safe: false) + + assert i == """ + # foo + # bar baz + #Ecto.Query\ + """ + + i = inspect(plan(query), safe: false) + + assert i == """ + # foo + # bar baz + #Ecto.Query\ + """ + end + def plan(query) do {query, _, _} = Ecto.Adapter.Queryable.plan_query(:all, Ecto.TestAdapter, query) query diff --git a/test/ecto/query/planner_test.exs b/test/ecto/query/planner_test.exs index 2eca569430..ffd4bb54c4 100644 --- a/test/ecto/query/planner_test.exs +++ b/test/ecto/query/planner_test.exs @@ -666,6 +666,26 @@ defmodule Ecto.Query.PlannerTest do assert key == :nocache end + test "plan: dynamic comments are uncacheable" do + {_query, _params, key} = + Post + |> select([p], p.id) + |> comment(^"uncache#{"able"}") + |> Planner.plan(:all, Ecto.TestAdapter) + + assert key == :nocache + end + + test "plan: static comments are cacheable" do + {_query, _params, key} = + Post + |> select([p], p.id) + |> comment("cacheable") + |> Planner.plan(:all, Ecto.TestAdapter) + + assert key != :nocache + end + test "plan: normalizes prefixes" do # No schema prefix in from {query, _, _, _} = from(Comment, select: 1) |> plan() diff --git a/test/ecto/query_test.exs b/test/ecto/query_test.exs index 4a6e43a4a4..2056ea3606 100644 --- a/test/ecto/query_test.exs +++ b/test/ecto/query_test.exs @@ -1137,4 +1137,23 @@ defmodule Ecto.QueryTest do assert inspect(reverse_order(q)) == inspect(order_by(q, desc: :id)) end end + + + describe "query comments" do + test "allows compile time strings" do + query = + from(p in "posts") + |> comment("called from test") + + assert ["called from test"] == query.comments + end + + test "allows interpolated strings" do + query = + from(p in "posts") + |> comment(^"called from #{"test"}") + + assert "called from test" == hd(query.comments).expr + end + end end