From 59d08a081e31834cac02027a98c48405c0a100b1 Mon Sep 17 00:00:00 2001 From: ruslandoga <67764432+ruslandoga@users.noreply.github.com> Date: Sun, 3 Nov 2024 11:27:21 +0700 Subject: [PATCH] less escaping --- lib/ecto/adapters/clickhouse/connection.ex | 107 +++++++++++------- lib/ecto/adapters/clickhouse/migration.ex | 2 +- .../adapters/clickhouse/connection_test.exs | 106 ++++++++--------- 3 files changed, 118 insertions(+), 97 deletions(-) diff --git a/lib/ecto/adapters/clickhouse/connection.ex b/lib/ecto/adapters/clickhouse/connection.ex index ca5df17d..68117142 100644 --- a/lib/ecto/adapters/clickhouse/connection.ex +++ b/lib/ecto/adapters/clickhouse/connection.ex @@ -137,28 +137,34 @@ defmodule Ecto.Adapters.ClickHouse.Connection do update_op(op, quote_name(key), value, sources, params, query) end - Enum.intersperse(fields, ?,) + Enum.intersperse(fields, ", ") end defp update_op(:set, quoted_key, value, sources, params, query) do - [quoted_key, ?= | expr(value, sources, params, query)] + [quoted_key, " = " | expr(value, sources, params, query)] end defp update_op(:inc, quoted_key, value, sources, params, query) do - [quoted_key, ?=, quoted_key, ?+ | expr(value, sources, params, query)] + [quoted_key, " = ", quoted_key, " + " | expr(value, sources, params, query)] end defp update_op(:push, quoted_key, value, sources, params, query) do - [quoted_key, ?=, "arrayPushBack(", quoted_key, ?,, expr(value, sources, params, query), ?)] + [ + quoted_key, + " = arrayPushBack(", + quoted_key, + ", ", + expr(value, sources, params, query), + ?) + ] end defp update_op(:pull, quoted_key, value, sources, params, query) do [ quoted_key, - ?=, - "arrayFilter(x->x!=", + " = arrayFilter(x->x!=", expr(value, sources, params, query), - ?,, + ", ", quoted_key, ?) ] @@ -231,7 +237,7 @@ defmodule Ecto.Adapters.ClickHouse.Connection do ["INSERT INTO " | quote_table(prefix, table)] _not_empty -> - fields = [?(, intersperse_map(header, ?,, "e_name/1), ?)] + fields = [?(, intersperse_map(header, ", ", "e_name/1), ?)] ["INSERT INTO ", quote_table(prefix, table) | fields] end @@ -327,7 +333,7 @@ defmodule Ecto.Adapters.ClickHouse.Connection do defp select_fields([], _sources, _params, _query), do: "true" defp select_fields(fields, sources, params, query) do - intersperse_map(fields, ?,, fn + intersperse_map(fields, ", ", fn # TODO raise # this is useful in array joins lie # @@ -354,7 +360,7 @@ defmodule Ecto.Adapters.ClickHouse.Connection do defp distinct(%{expr: exprs}, sources, params, query) when is_list(exprs) do [ "DISTINCT ON (", - intersperse_map(exprs, ?,, &order_by_expr(&1, sources, params, query)) | ") " + intersperse_map(exprs, ", ", &order_by_expr(&1, sources, params, query)) | ") " ] end @@ -371,7 +377,7 @@ defmodule Ecto.Adapters.ClickHouse.Connection do recursive_opt = if recursive, do: "RECURSIVE ", else: "" ctes = - intersperse_map(queries, ?,, fn {name, _opts, cte} -> + intersperse_map(queries, ", ", fn {name, _opts, cte} -> [quote_name(name), " AS ", cte_query(cte, sources, params, query)] end) @@ -468,8 +474,8 @@ defmodule Ecto.Adapters.ClickHouse.Connection do defp group_by(%{group_bys: group_bys} = query, sources, params) do [ " GROUP BY " - | intersperse_map(group_bys, ?,, fn %ByExpr{expr: expr} -> - intersperse_map(expr, ?,, &expr(&1, sources, params, query)) + | intersperse_map(group_bys, ", ", fn %ByExpr{expr: expr} -> + intersperse_map(expr, ", ", &expr(&1, sources, params, query)) end) ] end @@ -479,7 +485,7 @@ defmodule Ecto.Adapters.ClickHouse.Connection do defp window(%{windows: windows} = query, sources, params) do [ " WINDOW " - | intersperse_map(windows, ?,, fn {name, %{expr: kw}} -> + | intersperse_map(windows, ", ", fn {name, %{expr: kw}} -> [quote_name(name), " AS " | window_exprs(kw, sources, params, query)] end) ] @@ -494,11 +500,11 @@ defmodule Ecto.Adapters.ClickHouse.Connection do end defp window_expr({:partition_by, fields}, sources, params, query) do - ["PARTITION BY " | intersperse_map(fields, ?,, &expr(&1, sources, params, query))] + ["PARTITION BY " | intersperse_map(fields, ", ", &expr(&1, sources, params, query))] end defp window_expr({:order_by, fields}, sources, params, query) do - ["ORDER BY " | intersperse_map(fields, ?,, &order_by_expr(&1, sources, params, query))] + ["ORDER BY " | intersperse_map(fields, ", ", &order_by_expr(&1, sources, params, query))] end defp window_expr({:frame, {:fragment, _, _} = fragment}, sources, params, query) do @@ -510,8 +516,8 @@ defmodule Ecto.Adapters.ClickHouse.Connection do defp order_by(%{order_bys: order_bys} = query, sources, params) do [ " ORDER BY " - | intersperse_map(order_bys, ?,, fn %{expr: expr} -> - intersperse_map(expr, ?,, &order_by_expr(&1, sources, params, query)) + | intersperse_map(order_bys, ", ", fn %{expr: expr} -> + intersperse_map(expr, ", ", &order_by_expr(&1, sources, params, query)) end) ] end @@ -667,13 +673,13 @@ defmodule Ecto.Adapters.ClickHouse.Connection do "#{inspect(name)} you desire" end - intersperse_map(fields, ?,, &[name, ?. | quote_name(&1)]) + intersperse_map(fields, ", ", &[name, ?. | quote_name(&1)]) end defp expr({:in, _, [_left, []]}, _sources, _params, _query), do: "0" defp expr({:in, _, [left, right]}, sources, params, query) when is_list(right) do - args = intersperse_map(right, ?,, &expr(&1, sources, params, query)) + args = intersperse_map(right, ", ", &expr(&1, sources, params, query)) [expr(left, sources, params, query), " IN (", args, ?)] end @@ -752,7 +758,7 @@ defmodule Ecto.Adapters.ClickHouse.Connection do end defp expr({:{}, _, elems}, sources, params, query) do - [?(, intersperse_map(elems, ?,, &expr(&1, sources, params, query)), ?)] + [?(, intersperse_map(elems, ", ", &expr(&1, sources, params, query)), ?)] end defp expr({:count, _, []}, _sources, _params, _query), do: "count(*)" @@ -809,12 +815,12 @@ defmodule Ecto.Adapters.ClickHouse.Connection do ] {:fun, fun} -> - [fun, ?(, intersperse_map(args, ?,, &expr(&1, sources, params, query)), ?)] + [fun, ?(, intersperse_map(args, ", ", &expr(&1, sources, params, query)), ?)] end end defp expr(list, sources, params, query) when is_list(list) do - [?[, intersperse_map(list, ?,, &expr(&1, sources, params, query)), ?]] + [?[, intersperse_map(list, ", ", &expr(&1, sources, params, query)), ?]] end defp expr(%Decimal{} = decimal, _sources, _params, _query) do @@ -920,29 +926,47 @@ defmodule Ecto.Adapters.ClickHouse.Connection do @doc false def build_params(ix, len, params) when len > 1 do - [build_param(ix, Enum.at(params, ix)), ?, | build_params(ix + 1, len - 1, params)] + [build_param(ix, Enum.at(params, ix)), ", " | build_params(ix + 1, len - 1, params)] end def build_params(ix, _len = 1, params), do: build_param(ix, Enum.at(params, ix)) def build_params(_ix, _len = 0, _params), do: [] @doc false - def quote_name(name, quoter \\ ?") - def quote_name(nil, _), do: [] + def quote_name(name) + def quote_name(nil), do: [] - def quote_name(names, quoter) when is_list(names) do - names - |> Enum.reject(&is_nil/1) - |> intersperse_map(?., "e_name(&1, nil)) - |> wrap_in(quoter) + def quote_name(segments) when is_list(segments) do + [ + ?`, + segments + |> Enum.reject(&is_nil/1) + |> intersperse_map(?., &escape_name(&1)), + ?` + ] end - def quote_name(name, quoter) when is_atom(name) do - name |> Atom.to_string() |> quote_name(quoter) + def quote_name(name) when is_atom(name) do + name |> Atom.to_string() |> quote_name() end - def quote_name(name, quoter) do - wrap_in(name, quoter) + # TODO faster + # TODO test against ClickHouse + def quote_name(name) when is_binary(name) do + if String.contains?(name, ["`", "\"", "\\"]) do + [?`, escape_name(name), ?`] + else + name + end + end + + @compile inline: [escape_name: 1] + defp escape_name(name) when is_atom(name) do + name |> Atom.to_string() |> escape_name() + end + + defp escape_name(name) when is_binary(name) do + :binary.replace(name, "`", "``", [:global]) end defp quote_qualified_name(name, sources, ix) do @@ -959,9 +983,6 @@ defmodule Ecto.Adapters.ClickHouse.Connection do def quote_table(nil, name), do: quote_name(name) def quote_table(prefix, name), do: [quote_name(prefix), ?., quote_name(name)] - defp wrap_in(value, nil), do: value - defp wrap_in(value, wrapper), do: [wrapper, value, wrapper] - @doc false # TODO faster? def escape_string(value) when is_binary(value) do @@ -1081,11 +1102,11 @@ defmodule Ecto.Adapters.ClickHouse.Connection do defp inline_param(%Decimal{} = dec), do: Decimal.to_string(dec, :normal) defp inline_param(a) when is_list(a) do - [?[, Enum.map_intersperse(a, ?,, &inline_param/1), ?]] + [?[, Enum.map_intersperse(a, ", ", &inline_param/1), ?]] end defp inline_param(t) when is_tuple(t) do - [?(, t |> Tuple.to_list() |> Enum.map_intersperse(?,, &inline_param/1), ?)] + [?(, t |> Tuple.to_list() |> Enum.map_intersperse(", ", &inline_param/1), ?)] end defp inline_param(%s{}) do @@ -1095,8 +1116,8 @@ defmodule Ecto.Adapters.ClickHouse.Connection do defp inline_param(m) when is_map(m) do [ "map(", - Enum.map_intersperse(m, ?,, fn {k, v} -> - [inline_param(k), ?,, inline_param(v)] + Enum.map_intersperse(m, ", ", fn {k, v} -> + [inline_param(k), ", ", inline_param(v)] end), ?) ] @@ -1154,7 +1175,7 @@ defmodule Ecto.Adapters.ClickHouse.Connection do [k | _] -> # TODO check whole list [v | _] = Map.values(m) - ["Map(", param_type(k), ?,, param_type(v), ?)] + ["Map(", param_type(k), ", ", param_type(v), ?)] [] -> "Map(Nothing,Nothing)" diff --git a/lib/ecto/adapters/clickhouse/migration.ex b/lib/ecto/adapters/clickhouse/migration.ex index 38eb1bad..0b876e18 100644 --- a/lib/ecto/adapters/clickhouse/migration.ex +++ b/lib/ecto/adapters/clickhouse/migration.ex @@ -268,7 +268,7 @@ defmodule Ecto.Adapters.ClickHouse.Migration do defp comment(%Table{} = table) do if comment = table.comment do - [" COMMENT ", @conn.quote_name(comment, ?')] + [" COMMENT '", @conn.escape_string(comment), ?'] else [] end diff --git a/test/ecto/adapters/clickhouse/connection_test.exs b/test/ecto/adapters/clickhouse/connection_test.exs index 45f6f8a9..997d255e 100644 --- a/test/ecto/adapters/clickhouse/connection_test.exs +++ b/test/ecto/adapters/clickhouse/connection_test.exs @@ -117,17 +117,17 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do test "from" do query = Schema |> select([r], r.x) - assert all(query) == ~s[SELECT s0."x" FROM "schema" AS s0] + assert all(query) == ~s[SELECT s0.x FROM schema AS s0] end test "from with hints" do # With string query = Schema |> from(hints: "USE INDEX FOO") |> select([r], r.x) - assert all(query) == ~s{SELECT s0."x" FROM "schema" AS s0 USE INDEX FOO} + assert all(query) == ~s{SELECT s0.x FROM schema AS s0 USE INDEX FOO} # With list of strings query = Schema |> from(hints: ["INDEXED BY FOO", "INDEXED BY BAR"]) |> select([r], r.x) - assert all(query) == ~s[SELECT s0."x" FROM "schema" AS s0 INDEXED BY FOO INDEXED BY BAR] + assert all(query) == ~s[SELECT s0.x FROM schema AS s0 INDEXED BY FOO INDEXED BY BAR] end # TODO merge with test above once ecto 3.10.4 is released @@ -146,16 +146,16 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do test "from without schema" do query = "posts" |> select([r], r.x) - assert all(query) == ~s[SELECT p0."x" FROM "posts" AS p0] + assert all(query) == ~s[SELECT p0.x FROM posts AS p0] # query = "posts" |> select([r], fragment("?", r)) # assert all(query) == ~s[SELECT p0 FROM "posts" AS p0] query = "Posts" |> select([r], r.x) - assert all(query) == ~s[SELECT P0."x" FROM "Posts" AS P0] + assert all(query) == ~s[SELECT P0.x FROM Posts AS P0] query = "0posts" |> select([:x]) - assert all(query) == ~s{SELECT t0."x" FROM "0posts" AS t0} + assert all(query) == ~s{SELECT t0.x FROM 0posts AS t0} end test "from with subquery" do @@ -166,8 +166,8 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do |> select([r], r.x) assert all(query) == """ - SELECT s0."x" \ - FROM (SELECT sp0."x" AS "x",sp0."y" AS "y" FROM "posts" AS sp0) AS s0\ + SELECT s0.x \ + FROM (SELECT sp0.x AS x, sp0.y AS y FROM posts AS sp0) AS s0\ """ query = @@ -177,8 +177,8 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do |> select([r], r) assert all(query) == """ - SELECT s0."x",s0."z" \ - FROM (SELECT sp0."x" AS "x",sp0."y" AS "z" FROM "posts" AS sp0) AS s0\ + SELECT s0.x,s0.z \ + FROM (SELECT sp0.x AS x, sp0.y AS z FROM posts AS sp0) AS s0\ """ query = @@ -190,12 +190,12 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do |> select([r], r) assert all(query) == """ - SELECT s0."x",s0."z" \ + SELECT s0.x, s0.z \ FROM (\ - SELECT ss0."x" AS "x",ss0."z" AS "z" \ + SELECT ss0.x AS x, ss0.z AS z \ FROM (\ - SELECT ssp0."x" AS "x",ssp0."y" AS "z" \ - FROM "posts" AS ssp0\ + SELECT ssp0.x AS x, ssp0.y AS z \ + FROM posts AS ssp0\ ) AS ss0\ ) AS s0\ """ @@ -222,14 +222,14 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do assert all(query) == """ - WITH RECURSIVE "tree" AS \ - (SELECT sc0."id" AS "id",1 AS "depth" FROM "categories" AS sc0 WHERE (isNull(sc0."parent_id")) \ + WITH RECURSIVE tree AS \ + (SELECT sc0.id AS id, 1 AS depth FROM categories AS sc0 WHERE (isNull(sc0.parent_id)) \ UNION ALL \ - (SELECT c0."id",t1."depth" + 1 FROM "categories" AS c0 \ - INNER JOIN "tree" AS t1 ON t1."id" = c0."parent_id")) \ - SELECT s0."x",t1."id",CAST(t1."depth" AS Int64) \ - FROM "schema" AS s0 \ - INNER JOIN "tree" AS t1 ON t1."id" = s0."category_id"\ + (SELECT c0.id, t1.depth + 1 FROM categories AS c0 \ + INNER JOIN tree AS t1 ON t1.id = c0.parent_id)) \ + SELECT s0.x, t1.id,CAST(t1.depth AS Int64) \ + FROM schema AS s0 \ + INNER JOIN tree AS t1 ON t1.id = s0.category_id\ """ end @@ -256,16 +256,16 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do assert all(query) == """ - WITH "comments_scope" AS (\ - SELECT sc0."entity_id" AS "entity_id",sc0."text" AS "text" \ - FROM "comments" AS sc0 WHERE (isNull(sc0."deleted_at"))) \ - SELECT p0."title",c1."text" \ - FROM "posts" AS p0 \ - INNER JOIN "comments_scope" AS c1 ON c1."entity_id" = p0."guid" \ + WITH comments_scope AS (\ + SELECT sc0.entity_id AS entity_id, sc0.text AS text \ + FROM comments AS sc0 WHERE (isNull(sc0.deleted_at))) \ + SELECT p0.title, c1.text \ + FROM posts AS p0 \ + INNER JOIN comments_scope AS c1 ON c1.entity_id = p0.guid \ UNION ALL \ - (SELECT v0."title",c1."text" \ - FROM "videos" AS v0 \ - INNER JOIN "comments_scope" AS c1 ON c1."entity_id" = v0."guid")\ + (SELECT v0.title, c1.text \ + FROM videos AS v0 \ + INNER JOIN comments_scope AS c1 ON c1.entity_id = v0.guid)\ """ end @@ -285,10 +285,10 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do assert all(query) == """ - WITH RECURSIVE "tree" AS (#{@raw_sql_cte}) \ - SELECT s0."x" \ - FROM "schema" AS s0 \ - INNER JOIN "tree" AS t1 ON t1."id" = s0."category_id"\ + WITH RECURSIVE tree AS (#{@raw_sql_cte}) \ + SELECT s0.x \ + FROM schema AS s0 \ + INNER JOIN tree AS t1 ON t1.id = s0.category_id\ """ end @@ -326,59 +326,59 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do test "select" do query = Schema |> select([r], {r.x, r.y}) - assert all(query) == ~s[SELECT s0."x",s0."y" FROM "schema" AS s0] + assert all(query) == ~s[SELECT s0.x, s0.y FROM schema AS s0] query = Schema |> select([r], [r.x, r.y]) - assert all(query) == ~s[SELECT s0."x",s0."y" FROM "schema" AS s0] + assert all(query) == ~s[SELECT s0.x, s0.y FROM schema AS s0] query = Schema |> select([r], struct(r, [:x, :y])) - assert all(query) == ~s[SELECT s0."x",s0."y" FROM "schema" AS s0] + assert all(query) == ~s[SELECT s0.x, s0.y FROM schema AS s0] end test "aggregates" do query = Schema |> select(count()) - assert all(query) == ~S[SELECT count(*) FROM "schema" AS s0] + assert all(query) == ~S[SELECT count(*) FROM schema AS s0] query = Schema |> select([s], count(s.x)) - assert all(query) == ~S[SELECT count(s0."x") FROM "schema" AS s0] + assert all(query) == ~S[SELECT count(s0.x) FROM schema AS s0] query = Schema |> select([s], count(s.x, :distinct)) - assert all(query) == ~S[SELECT countDistinct(s0."x") FROM "schema" AS s0] + assert all(query) == ~S[SELECT countDistinct(s0.x) FROM schema AS s0] end test "aggregate filters" do query = Schema |> select([r], count(r.x) |> filter(r.x > 10)) - assert all(query) == ~s[SELECT count(s0."x") FILTER (WHERE s0."x" > 10) FROM "schema" AS s0] + assert all(query) == ~s[SELECT count(s0.x) FILTER (WHERE s0.x > 10) FROM schema AS s0] query = Schema |> select([r], count(r.x) |> filter(r.x > 10 and r.x < 50)) assert all(query) == - ~s[SELECT count(s0."x") FILTER (WHERE (s0."x" > 10) AND (s0."x" < 50)) FROM "schema" AS s0] + ~s[SELECT count(s0.x) FILTER (WHERE (s0.x > 10) AND (s0.x < 50)) FROM schema AS s0] query = Schema |> select([r], count() |> filter(r.x > 10)) - assert all(query) == ~s[SELECT count(*) FILTER (WHERE s0."x" > 10) FROM "schema" AS s0] + assert all(query) == ~s[SELECT count(*) FILTER (WHERE s0.x > 10) FROM schema AS s0] end test "distinct" do query = Schema |> distinct([r], true) |> select([r], {r.x, r.y}) - assert all(query) == ~s[SELECT DISTINCT s0."x",s0."y" FROM "schema" AS s0] + assert all(query) == ~s[SELECT DISTINCT s0.x, s0.y FROM schema AS s0] query = Schema |> distinct([r], false) |> select([r], {r.x, r.y}) - assert all(query) == ~s[SELECT s0."x",s0."y" FROM "schema" AS s0] + assert all(query) == ~s[SELECT s0.x, s0.y FROM schema AS s0] query = Schema |> distinct(true) |> select([r], {r.x, r.y}) - assert all(query) == ~s[SELECT DISTINCT s0."x",s0."y" FROM "schema" AS s0] + assert all(query) == ~s[SELECT DISTINCT s0.x, s0.y FROM schema AS s0] query = Schema |> distinct(false) |> select([r], {r.x, r.y}) - assert all(query) == ~s[SELECT s0."x",s0."y" FROM "schema" AS s0] + assert all(query) == ~s[SELECT s0.x, s0.y FROM schema AS s0] query = Schema |> distinct([r], [r.x, r.y]) |> select([r], {r.x, r.y}) - assert all(query) == ~s[SELECT DISTINCT ON (s0."x",s0."y") s0."x",s0."y" FROM "schema" AS s0] + assert all(query) == ~s[SELECT DISTINCT ON (s0.x, s0.y) s0.x, s0.y FROM schema AS s0] end test "coalesce" do query = Schema |> select([s], coalesce(s.x, 5)) - assert all(query) == ~s[SELECT coalesce(s0."x",5) FROM "schema" AS s0] + assert all(query) == ~s[SELECT coalesce(s0.x, 5) FROM schema AS s0] end test "where" do @@ -389,16 +389,16 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do |> select([r], r.x) assert all(query) == - ~s[SELECT s0."x" FROM "schema" AS s0 WHERE (s0."x" = 42) AND (s0."y" != 43)] + ~s[SELECT s0.x FROM schema AS s0 WHERE (s0.x = 42) AND (s0.y != 43)] query = Schema |> where([r], {r.x, r.y} > {1, 2}) |> select([r], r.x) - assert all(query) == ~s[SELECT s0."x" FROM "schema" AS s0 WHERE ((s0."x",s0."y") > (1,2))] + assert all(query) == ~s[SELECT s0.x FROM schema AS s0 WHERE ((s0.x, s0.y) > (1, 2))] end test "where with big AND chain gets many parens" do assert all(from e in "events", where: true and true and true and true and true, select: true) == """ - SELECT true FROM "events" AS e0 WHERE ((((1 AND 1) AND 1) AND 1) AND 1)\ + SELECT true FROM events AS e0 WHERE ((((1 AND 1) AND 1) AND 1) AND 1)\ """ end @@ -412,7 +412,7 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do where: true, select: true ) == """ - SELECT true FROM "events" AS e0 WHERE (1) AND (1) AND (1) AND (1) AND (1)\ + SELECT true FROM events AS e0 WHERE (1) AND (1) AND (1) AND (1) AND (1)\ """ end