diff --git a/integration_test/sql/logging.exs b/integration_test/sql/logging.exs index 0f92e9a7f..01845dce6 100644 --- a/integration_test/sql/logging.exs +++ b/integration_test/sql/logging.exs @@ -178,6 +178,62 @@ defmodule Ecto.Integration.LoggingTest do end end + describe ":comments option" do + test "comments query operations" do + assert capture_log(fn -> + TestRepo.all(Post, comments: [pre: "list_posts_q"], log: :error) + end) =~ "/* list_posts_q */ SELECT" + + assert capture_log(fn -> + TestRepo.update_all(Post, [set: [visits: 0]], comments: [pre: "reset_visits_q"], log: :error) + end) =~ "/* reset_visits_q */ UPDATE" + + assert capture_log(fn -> + TestRepo.delete_all(Post, comments: [pre: "purge_posts_q"], log: :error) + end) =~ "/* purge_posts_q */ DELETE" + end + + test "supports both :pre and :post" do + assert capture_log(fn -> + TestRepo.all(Post, comments: [pre: "before_q", post: "after_q"], log: :error) + end) =~ ~r{/\* before_q \*/ SELECT.* /\* after_q \*/} + end + + test "renders with query_cache: false (the escape hatch for dynamic comments)" do + assert capture_log(fn -> + TestRepo.all(Post, comments: [pre: "dyn_#{System.unique_integer()}"], query_cache: false, log: :error) + end) =~ ~r{/\* dyn_-?\d+ \*/ SELECT} + end + + test "comments insert/update/delete/insert_all" do + assert capture_log(fn -> + TestRepo.insert!(%Post{title: "1"}, comments: [pre: "insert_create_post_q"], log: :error) + end) =~ "/* insert_create_post_q */ INSERT INTO" + + post = TestRepo.insert!(%Post{title: "x"}) + + assert capture_log(fn -> + post + |> Ecto.Changeset.change(title: "y") + |> TestRepo.update!(comments: [pre: "update_post_q"], log: :error) + end) =~ "/* update_post_q */ UPDATE" + + assert capture_log(fn -> + TestRepo.delete!(post, comments: [pre: "delete_post_q"], log: :error) + end) =~ "/* delete_post_q */ DELETE" + + assert capture_log(fn -> + TestRepo.insert_all(Post, [%{title: "a"}], comments: [pre: "bulk_insert_posts_q"], log: :error) + end) =~ "/* bulk_insert_posts_q */ INSERT INTO" + end + + test "rejects a comment that could break out of the comment block" do + assert_raise ArgumentError, ~r/cannot contain/, fn -> + TestRepo.insert!(%Post{title: "1"}, comments: [pre: "evil */ DROP TABLE posts"]) + end + end + end + describe "parameter logging" do @describetag :parameter_logging diff --git a/lib/ecto/adapters/myxql.ex b/lib/ecto/adapters/myxql.ex index 6026cb260..b12f4a2c4 100644 --- a/lib/ecto/adapters/myxql.ex +++ b/lib/ecto/adapters/myxql.ex @@ -380,6 +380,10 @@ defmodule Ecto.Adapters.MyXQL do opts end + # This adapter overrides insert/6 instead of going through + # Ecto.Adapters.SQL.struct/10, so wrap the `:comments` here too. + sql = Ecto.Adapters.SQL.wrap_comments(sql, opts) + case Ecto.Adapters.SQL.query(adapter_meta, sql, values ++ query_params, opts) do {:ok, %{num_rows: 0}} -> # With INSERT IGNORE (insert_mode: :ignore), 0 rows means the row diff --git a/lib/ecto/adapters/myxql/connection.ex b/lib/ecto/adapters/myxql/connection.ex index 833c6d35f..30f539848 100644 --- a/lib/ecto/adapters/myxql/connection.ex +++ b/lib/ecto/adapters/myxql/connection.ex @@ -118,8 +118,10 @@ if Code.ensure_loaded?(MyXQL) do limit = limit(query, sources) offset = offset(query, sources) lock = lock(query, sources) + {pre_comments, post_comments} = SQL.comments(query.comments) [ + pre_comments, cte, select, from, @@ -131,7 +133,8 @@ if Code.ensure_loaded?(MyXQL) do combinations, order_by, limit, - offset | lock + offset, + lock | post_comments ] end @@ -145,6 +148,7 @@ if Code.ensure_loaded?(MyXQL) do sources = create_names(query, []) cte = cte(query, sources) + {pre_comments, post_comments} = SQL.comments(query.comments) {from, name} = get_source(query, sources, 0, source) fields = @@ -158,7 +162,7 @@ if Code.ensure_loaded?(MyXQL) do prefix = prefix || ["UPDATE ", from, " AS ", name, join, " SET "] where = where(%{query | wheres: wheres ++ query.wheres}, sources) - [cte, prefix, fields | where] + [pre_comments, cte, prefix, fields, where | post_comments] end @impl true @@ -171,11 +175,12 @@ if Code.ensure_loaded?(MyXQL) do cte = cte(query, sources) {_, name, _} = elem(sources, 0) + {pre_comments, post_comments} = SQL.comments(query.comments) from = from(query, sources) join = join(query, sources) where = where(query, sources) - [cte, "DELETE ", name, ".*", from, join | where] + [pre_comments, cte, "DELETE ", name, ".*", from, join, where | post_comments] end @impl true diff --git a/lib/ecto/adapters/postgres/connection.ex b/lib/ecto/adapters/postgres/connection.ex index ec8bf4a09..b65efcca6 100644 --- a/lib/ecto/adapters/postgres/connection.ex +++ b/lib/ecto/adapters/postgres/connection.ex @@ -191,8 +191,10 @@ if Code.ensure_loaded?(Postgrex) do limit = limit(query, sources) offset = offset(query, sources) lock = lock(query, sources) + {pre_comments, post_comments} = Ecto.Adapters.SQL.comments(query.comments) [ + pre_comments, cte, select, from, @@ -204,7 +206,8 @@ if Code.ensure_loaded?(Postgrex) do combinations, order_by, limit, - offset | lock + offset, + lock | post_comments ] end @@ -213,13 +216,13 @@ if Code.ensure_loaded?(Postgrex) do sources = create_names(query, []) cte = cte(query, sources) {from, name} = get_source(query, sources, 0, source) - + {pre_comments, post_comments} = Ecto.Adapters.SQL.comments(query.comments) prefix = prefix || ["UPDATE ", from, " AS ", name | " SET "] fields = update_fields(query, sources) {join, wheres} = using_join(query, :update_all, "FROM", sources) where = where(%{query | wheres: wheres ++ query.wheres}, sources) - [cte, prefix, fields, join, where | returning(query, sources)] + [pre_comments, cte, prefix, fields, join, where, returning(query, sources) | post_comments] end @impl true @@ -227,11 +230,11 @@ if Code.ensure_loaded?(Postgrex) do sources = create_names(query, []) cte = cte(query, sources) {from, name} = get_source(query, sources, 0, from) - + {pre_comments, post_comments} = Ecto.Adapters.SQL.comments(query.comments) {join, wheres} = using_join(query, :delete_all, "USING", sources) where = where(%{query | wheres: wheres ++ query.wheres}, sources) - [cte, "DELETE FROM ", from, " AS ", name, join, where | returning(query, sources)] + [pre_comments, cte, "DELETE FROM ", from, " AS ", name, join, where, returning(query, sources) | post_comments] end @impl true diff --git a/lib/ecto/adapters/sql.ex b/lib/ecto/adapters/sql.ex index 4ae24c14e..d9476de0a 100644 --- a/lib/ecto/adapters/sql.ex +++ b/lib/ecto/adapters/sql.ex @@ -987,6 +987,8 @@ defmodule Ecto.Adapters.SQL do opts end + sql = wrap_comments(sql, opts) + all_params = placeholders ++ Enum.reverse(params, conflict_params) %{num_rows: num, rows: rows} = query!(adapter_meta, sql, all_params, [source: source] ++ opts) @@ -1166,6 +1168,42 @@ defmodule Ecto.Adapters.SQL do end end + @doc false + def wrap_comments(sql, opts) do + {pre, post} = comments(Keyword.get(opts, :comments, [])) + [pre, sql | post] + end + + @doc false + def comments(comments) when is_list(comments) do + {pre, post} = + Enum.reduce(comments, {[], []}, fn + {:pre, c}, {pre, post} -> {[["/* ", escape_comment!(c), " */ "] | pre], post} + {:post, c}, {pre, post} -> {pre, [[" /* ", escape_comment!(c), " */"] | post]} + other, _ -> raise ArgumentError, "expected {:pre, string} or {:post, string}, got: #{inspect(other)}" + end) + + {Enum.reverse(pre), Enum.reverse(post)} + end + + def comments(other) do + raise ArgumentError, + "comments must be a keyword list of [pre: string, post: string], got: #{inspect(other)}" + end + + defp escape_comment!(comment) when is_binary(comment) do + if String.contains?(comment, ["/*", "*/", <<0>>]) do + raise ArgumentError, + "a comment cannot contain `/*`, `*/`, or null bytes, got: #{inspect(comment)}. " + end + + comment + end + + defp escape_comment!(other) do + raise ArgumentError, "a comment must be a string, got: #{inspect(other)}" + end + @doc false def struct( adapter_meta, @@ -1186,6 +1224,8 @@ defmodule Ecto.Adapters.SQL do opts end + sql = wrap_comments(sql, opts) + case query(adapter_meta, sql, values, [source: source] ++ opts) do {:ok, %{rows: nil, num_rows: 1}} -> {:ok, []} diff --git a/lib/ecto/adapters/tds/connection.ex b/lib/ecto/adapters/tds/connection.ex index be1cf229b..a4ae41983 100644 --- a/lib/ecto/adapters/tds/connection.ex +++ b/lib/ecto/adapters/tds/connection.ex @@ -170,11 +170,12 @@ if Code.ensure_loaded?(Tds) do # limit = is handled in select (TOP X) offset = offset(query, sources) lock = lock(query, sources) + {pre_comments, post_comments} = SQL.comments(query.comments) if query.offset != nil and query.order_bys == [], do: error!(query, "ORDER BY is mandatory when OFFSET is set") - [cte, select, from, join, where, group_by, having, combinations, order_by, lock | offset] + [pre_comments, cte, select, from, join, where, group_by, having, combinations, order_by, lock, offset | post_comments] end @impl true @@ -188,8 +189,10 @@ if Code.ensure_loaded?(Tds) do join = join(query, sources) where = where(query, sources) lock = lock(query, sources) + {pre_comments, post_comments} = SQL.comments(query.comments) [ + pre_comments, cte, "UPDATE ", name, @@ -198,7 +201,8 @@ if Code.ensure_loaded?(Tds) do returning(query, 0, "INSERTED"), from, join, - where | lock + where, + lock | post_comments ] end @@ -213,8 +217,9 @@ if Code.ensure_loaded?(Tds) do join = join(query, sources) where = where(query, sources) lock = lock(query, sources) + {pre_comments, post_comments} = SQL.comments(query.comments) - [cte, delete, returning(query, 0, "DELETED"), from, join, where | lock] + [pre_comments, cte, delete, returning(query, 0, "DELETED"), from, join, where, lock | post_comments] end @impl true diff --git a/mix.lock b/mix.lock index 1bd9862fa..01bcc9719 100644 --- a/mix.lock +++ b/mix.lock @@ -3,7 +3,7 @@ "benchee_html": {:hex, :benchee_html, "1.0.1", "1e247c0886c3fdb0d3f4b184b653a8d6fb96e4ad0d0389267fe4f36968772e24", [:mix], [{:benchee, ">= 0.99.0 and < 2.0.0", [hex: :benchee, repo: "hexpm", optional: false]}, {:benchee_json, "~> 1.0", [hex: :benchee_json, repo: "hexpm", optional: false]}], "hexpm", "b00a181af7152431901e08f3fc9f7197ed43ff50421a8347b0c80bf45d5b3fef"}, "benchee_json": {:hex, :benchee_json, "1.0.0", "cc661f4454d5995c08fe10dd1f2f72f229c8f0fb1c96f6b327a8c8fc96a91fe5", [:mix], [{:benchee, ">= 0.99.0 and < 2.0.0", [hex: :benchee, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}], "hexpm", "da05d813f9123505f870344d68fb7c86a4f0f9074df7d7b7e2bb011a63ec231c"}, "db_connection": {:hex, :db_connection, "2.10.1", "d5465f6bcc125c1b8981c1dbf23c193ca16f446ec0b25832dc174f74f18be510", [:mix], [{:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "18ed94c6e627b4bf452dbd4df61b69a35a1e768525140bc1917b7a685026a6a3"}, - "decimal": {:hex, :decimal, "3.1.0", "9ede268cff827e6f0c4fb1b34747c82630dce5d7b877dfb22ec8f0cb25855fce", [:mix], [], "hexpm", "e8b3efb3bb3a13cb5e4268ffe128569067b1972e9dee013537c71a5b073168f9"}, + "decimal": {:hex, :decimal, "3.1.1", "430d87b04011ce6cbd4fd205be758311a81f87d552d40904abd00f015935b1d0", [:mix], [], "hexpm", "c5f25f2ced74a0587d03e6023f595db8e924c9d3922c8c8ffd9edfc4498cf1f6"}, "deep_merge": {:hex, :deep_merge, "1.0.0", "b4aa1a0d1acac393bdf38b2291af38cb1d4a52806cf7a4906f718e1feb5ee961", [:mix], [], "hexpm", "ce708e5f094b9cd4e8f2be4f00d2f4250c4095be93f8cd6d018c753894885430"}, "earmark_parser": {:hex, :earmark_parser, "1.4.44", "f20830dd6b5c77afe2b063777ddbbff09f9759396500cdbe7523efd58d7a339c", [:mix], [], "hexpm", "4778ac752b4701a5599215f7030989c989ffdc4f6df457c5f36938cc2d2a2750"}, "ecto": {:hex, :ecto, "3.14.0", "2fa64521eebfcb2670d907a86e4ad947290e9933706bb315e6fb5c21b172cb26", [:mix], [{:decimal, "~> 3.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}, {:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "130d69ffb4285f9ce4792b65dfbb994fd13ea4cbc3cbea2524b199aa3de84af3"}, diff --git a/test/ecto/adapters/myxql_test.exs b/test/ecto/adapters/myxql_test.exs index b062d7bbd..b4f5ba467 100644 --- a/test/ecto/adapters/myxql_test.exs +++ b/test/ecto/adapters/myxql_test.exs @@ -590,6 +590,21 @@ defmodule Ecto.Adapters.MyXQLTest do assert all(query) == ~s{SELECT TRUE FROM `schema` AS s0 UPDATE on s0} end + test "comments" do + query = Schema |> select([], true) |> plan() + assert all(%{query | comments: [pre: "q"]}) == ~s{/* q */ SELECT TRUE FROM `schema` AS s0} + assert all(%{query | comments: [post: "q"]}) == ~s{SELECT TRUE FROM `schema` AS s0 /* q */} + + assert all(%{query | comments: [pre: "a", post: "b"]}) == + ~s{/* a */ SELECT TRUE FROM `schema` AS s0 /* b */} + + query = Schema |> update([], set: [x: 0]) |> plan(:update_all) + assert update_all(%{query | comments: [pre: "upd_q"]}) == ~s{/* upd_q */ UPDATE `schema` AS s0 SET s0.`x` = 0} + + query = Schema |> plan(:delete_all) + assert delete_all(%{query | comments: [pre: "del_q"]}) == ~s{/* del_q */ DELETE s0.* FROM `schema` AS s0} + end + test "string escape" do query = "schema" |> where(foo: "'\\ ") |> select([], true) |> plan() assert all(query) == ~s{SELECT TRUE FROM `schema` AS s0 WHERE (s0.`foo` = '''\\\\ ')} diff --git a/test/ecto/adapters/postgres_test.exs b/test/ecto/adapters/postgres_test.exs index e588b29d5..e0ac7cb35 100644 --- a/test/ecto/adapters/postgres_test.exs +++ b/test/ecto/adapters/postgres_test.exs @@ -783,6 +783,21 @@ defmodule Ecto.Adapters.PostgresTest do assert all(query) == ~s{SELECT TRUE FROM "schema" AS s0 UPDATE on s0} end + test "comments" do + query = Schema |> select([], true) |> plan() + assert all(%{query | comments: [pre: "q"]}) == ~s{/* q */ SELECT TRUE FROM "schema" AS s0} + assert all(%{query | comments: [post: "q"]}) == ~s{SELECT TRUE FROM "schema" AS s0 /* q */} + + assert all(%{query | comments: [pre: "a", post: "b"]}) == + ~s{/* a */ SELECT TRUE FROM "schema" AS s0 /* b */} + + query = Schema |> update([], set: [x: 0]) |> plan(:update_all) + assert update_all(%{query | comments: [pre: "upd_q"]}) == ~s{/* upd_q */ UPDATE "schema" AS s0 SET "x" = 0} + + query = Schema |> plan(:delete_all) + assert delete_all(%{query | comments: [pre: "del_q"]}) == ~s{/* del_q */ DELETE FROM "schema" AS s0} + end + test "string escape" do query = "schema" |> where(foo: "'\\ ") |> select([], true) |> plan() assert all(query) == ~s{SELECT TRUE FROM \"schema\" AS s0 WHERE (s0.\"foo\" = '''\\ ')} diff --git a/test/ecto/adapters/sql_test.exs b/test/ecto/adapters/sql_test.exs new file mode 100644 index 000000000..8ce0ac09b --- /dev/null +++ b/test/ecto/adapters/sql_test.exs @@ -0,0 +1,57 @@ +defmodule Ecto.Adapters.SQLTest do + use ExUnit.Case, async: true + + defp comments(list) do + {pre, post} = Ecto.Adapters.SQL.comments(list) + {IO.iodata_to_binary(pre), IO.iodata_to_binary(post)} + end + + defp wrap(sql, opts) do + sql |> Ecto.Adapters.SQL.wrap_comments(opts) |> IO.iodata_to_binary() + end + + describe "comments/1" do + test "empty list renders nothing" do + assert comments([]) == {"", ""} + end + + test "renders :pre leading and :post trailing" do + assert comments(pre: "list_users") == {"/* list_users */ ", ""} + assert comments(post: "list_users") == {"", " /* list_users */"} + assert comments(pre: "a", post: "b") == {"/* a */ ", " /* b */"} + end + + test "preserves order and supports multiples" do + assert comments(pre: "a", pre: "b") == {"/* a */ /* b */ ", ""} + end + + test "rejects comment-delimiter sequences and null bytes" do + for bad <- ["evil */ x", "evil /* x", "x\0y"] do + assert_raise ArgumentError, ~r/cannot contain/, fn -> + Ecto.Adapters.SQL.comments(pre: bad) + end + end + end + + test "rejects bad shapes" do + assert_raise ArgumentError, ~r/expected \{:pre/, fn -> + Ecto.Adapters.SQL.comments(foo: "bar") + end + + assert_raise ArgumentError, ~r/keyword list/, fn -> + Ecto.Adapters.SQL.comments("nope") + end + end + end + + describe "wrap_comments/2" do + test "wraps the sql with pre/post from the :comments option" do + assert wrap("INSERT INTO posts ...", comments: [pre: "create_post", post: "v2"]) == + "/* create_post */ INSERT INTO posts ... /* v2 */" + end + + test "is a no-op without the :comments option" do + assert wrap("INSERT INTO posts ...", timeout: 5000) == "INSERT INTO posts ..." + end + end +end diff --git a/test/ecto/adapters/tds_test.exs b/test/ecto/adapters/tds_test.exs index 93413141f..7a50711d8 100644 --- a/test/ecto/adapters/tds_test.exs +++ b/test/ecto/adapters/tds_test.exs @@ -641,6 +641,21 @@ defmodule Ecto.Adapters.TdsTest do assert all(query) == ~s{SELECT CAST(1 as bit) FROM [schema] AS s0 OPTION (UPDATE on s0)} end + test "comments" do + query = Schema |> select([], true) |> plan() + assert all(%{query | comments: [pre: "q"]}) == ~s{/* q */ SELECT CAST(1 as bit) FROM [schema] AS s0} + assert all(%{query | comments: [post: "q"]}) == ~s{SELECT CAST(1 as bit) FROM [schema] AS s0 /* q */} + + assert all(%{query | comments: [pre: "a", post: "b"]}) == + ~s{/* a */ SELECT CAST(1 as bit) FROM [schema] AS s0 /* b */} + + query = Schema |> update([], set: [x: 0]) |> plan(:update_all) + assert update_all(%{query | comments: [pre: "upd_q"]}) == ~s{/* upd_q */ UPDATE s0 SET s0.[x] = 0 FROM [schema] AS s0} + + query = Schema |> plan(:delete_all) + assert delete_all(%{query | comments: [pre: "del_q"]}) == ~s{/* del_q */ DELETE s0 FROM [schema] AS s0} + end + test "string escape" do query = "schema" |> where(foo: "\'-- ") |> select([], true) |> plan()