From f3e75d5f56441c89278274414345599fabf1d2e7 Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Tue, 28 May 2024 12:22:12 -0400 Subject: [PATCH] make tds connection raise errors on using subqueries in forbidden cases --- integration_test/sql/subquery.exs | 2 + integration_test/tds/test_helper.exs | 7 +- lib/ecto/adapters/tds/connection.ex | 216 +++++++++++++++------------ test/ecto/adapters/tds_test.exs | 14 +- 4 files changed, 136 insertions(+), 103 deletions(-) diff --git a/integration_test/sql/subquery.exs b/integration_test/sql/subquery.exs index 79c90fa7..869b64b7 100644 --- a/integration_test/sql/subquery.exs +++ b/integration_test/sql/subquery.exs @@ -123,6 +123,7 @@ defmodule Ecto.Integration.SubQueryTest do end @tag :multicolumn_distinct + @tag :subquery_in_distinct test "subqueries in distinct" do TestRepo.insert!(%Post{visits: 10, title: "hello1"}) TestRepo.insert!(%Post{visits: 10, title: "hello2"}) @@ -133,6 +134,7 @@ defmodule Ecto.Integration.SubQueryTest do assert [%{title: "hello"}, %{title: "hello1"}] = TestRepo.all(query) end + @tag :subquery_in_group_by test "subqueries in group by" do TestRepo.insert!(%Post{visits: 10, title: "hello1"}) TestRepo.insert!(%Post{visits: 10, title: "hello2"}) diff --git a/integration_test/tds/test_helper.exs b/integration_test/tds/test_helper.exs index d8747c50..5faeb4a6 100644 --- a/integration_test/tds/test_helper.exs +++ b/integration_test/tds/test_helper.exs @@ -58,8 +58,11 @@ ExUnit.start( :selected_as_with_order_by_expression, # MSSQL doesn't support specifying columns for ON DELETE SET NULL :on_delete_nilify_column_list, - # MySQL doesnt' support anything except a single column in DISTINCT - :multicolumn_distinct + # MSSQL doesnt' support anything except a single column in DISTINCT + :multicolumn_distinct, + # MSSQL doesnt' support subqueries in group by or in distinct + :subquery_in_group_by, + :subquery_in_distinct ] ) diff --git a/lib/ecto/adapters/tds/connection.ex b/lib/ecto/adapters/tds/connection.ex index c0669e92..a1c29fad 100644 --- a/lib/ecto/adapters/tds/connection.ex +++ b/lib/ecto/adapters/tds/connection.ex @@ -439,7 +439,7 @@ if Code.ensure_loaded?(Tds) do [?~, ?(, select_expr(expr, sources, query), ?)] end - defp select_expr(value, sources, query), do: expr(value, sources, query) + defp select_expr(value, sources, query), do: expr(value, sources, query, :select) defp from(%{from: %{source: source, hints: hints}} = query, sources) do {from, name} = get_source(query, sources, 0, source) @@ -520,14 +520,14 @@ if Code.ensure_loaded?(Tds) do defp update_op(:set, key, value, sources, query) do {_table, name, _model} = elem(sources, 0) - [name, ?., quote_name(key), " = " | expr(value, sources, query)] + [name, ?., quote_name(key), " = " | expr(value, sources, query, :update)] end defp update_op(:inc, key, value, sources, query) do {_table, name, _model} = elem(sources, 0) quoted = quote_name(key) - [name, ?., quoted, " = ", name, ?., quoted, " + " | expr(value, sources, query)] + [name, ?., quoted, " = ", name, ?., quoted, " + " | expr(value, sources, query, :update)] end defp update_op(command, _key, _value, _sources, query) do @@ -543,7 +543,7 @@ if Code.ensure_loaded?(Tds) do %JoinExpr{on: %QueryExpr{expr: expr}, qual: qual, ix: ix, source: source, hints: hints} -> {join, name} = get_source(query, sources, ix, source) qual_text = join_qual(qual, query) - join = join || ["(", expr(source, sources, query) | ")"] + join = join || ["(", expr(source, sources, query, :join) | ")"] [qual_text, join, " AS ", name, hints(hints) | join_on(qual, expr, sources, query)] end) ] @@ -553,7 +553,7 @@ if Code.ensure_loaded?(Tds) do defp join_on(:inner_lateral, true, _sources, _query), do: [] defp join_on(:left_lateral, true, _sources, _query), do: [] defp join_on(_qual, true, _sources, _query), do: [" ON 1 = 1"] - defp join_on(_qual, expr, sources, query), do: [" ON " | expr(expr, sources, query)] + defp join_on(_qual, expr, sources, query), do: [" ON " | expr(expr, sources, query, :on)] defp join_qual(:inner, _), do: "INNER JOIN " defp join_qual(:left, _), do: "LEFT OUTER JOIN " @@ -567,11 +567,11 @@ if Code.ensure_loaded?(Tds) do do: error!(query, "join qualifier #{inspect(qual)} is not supported in the Tds adapter") defp where(%Query{wheres: wheres} = query, sources) do - boolean(" WHERE ", wheres, sources, query) + boolean(" WHERE ", wheres, sources, query, :where) end defp having(%Query{havings: havings} = query, sources) do - boolean(" HAVING ", havings, sources, query) + boolean(" HAVING ", havings, sources, query, :having) end defp window(%{windows: []}, _sources), do: [] @@ -585,7 +585,7 @@ if Code.ensure_loaded?(Tds) do [ " GROUP BY " | Enum.map_intersperse(group_bys, ", ", fn %ByExpr{expr: expr} -> - Enum.map_intersperse(expr, ", ", &top_level_expr(&1, sources, query)) + Enum.map_intersperse(expr, ", ", &top_level_expr(&1, sources, query, :group_by)) end) ] end @@ -602,7 +602,7 @@ if Code.ensure_loaded?(Tds) do end defp order_by_expr({dir, expr}, sources, query) do - str = top_level_expr(expr, sources, query) + str = top_level_expr(expr, sources, query, :order_by) case dir do :asc -> str @@ -627,7 +627,7 @@ if Code.ensure_loaded?(Tds) do ) do case Map.get(query, :offset) do nil -> - ["TOP(", expr(expr, sources, query), ") "] + ["TOP(", expr(expr, sources, query, :limit), ") "] _ -> [] @@ -643,10 +643,10 @@ if Code.ensure_loaded?(Tds) do defp offset(%{offset: offset, limit: limit} = query, sources) do [ " OFFSET ", - expr(offset.expr, sources, query), + expr(offset.expr, sources, query, :offset), " ROW", " FETCH NEXT ", - expr(limit.expr, sources, query), + expr(limit.expr, sources, query, :limit), " ROWS ONLY" ] end @@ -656,7 +656,9 @@ if Code.ensure_loaded?(Tds) do defp lock(%{lock: nil}, _sources), do: [] defp lock(%{lock: binary}, _sources) when is_binary(binary), do: [" OPTION (", binary, ?)] - defp lock(%{lock: expr} = query, sources), do: [" OPTION (", expr(expr, sources, query), ?)] + + defp lock(%{lock: expr} = query, sources), + do: [" OPTION (", expr(expr, sources, query, :lock), ?)] defp combinations(%{combinations: combinations}, as_prefix) do Enum.map(combinations, fn @@ -669,17 +671,18 @@ if Code.ensure_loaded?(Tds) do end) end - defp boolean(_name, [], _sources, _query), do: [] + defp boolean(_name, [], _sources, _query, _location), do: [] - defp boolean(name, [%{expr: expr, op: op} | query_exprs], sources, query) do + defp boolean(name, [%{expr: expr, op: op} | query_exprs], sources, query, location) do [ name - | Enum.reduce(query_exprs, {op, paren_expr(expr, sources, query)}, fn + | Enum.reduce(query_exprs, {op, paren_expr(expr, sources, query, location)}, fn %BooleanExpr{expr: expr, op: op}, {op, acc} -> - {op, [acc, operator_to_boolean(op), paren_expr(expr, sources, query)]} + {op, [acc, operator_to_boolean(op), paren_expr(expr, sources, query, location)]} %BooleanExpr{expr: expr, op: op}, {_, acc} -> - {op, [?(, acc, ?), operator_to_boolean(op), paren_expr(expr, sources, query)]} + {op, + [?(, acc, ?), operator_to_boolean(op), paren_expr(expr, sources, query, location)]} end) |> elem(1) ] @@ -696,19 +699,27 @@ if Code.ensure_loaded?(Tds) do end end - defp paren_expr(true, _sources, _query) do + defp paren_expr(true, _sources, _query, _location) do ["(1 = 1)"] end - defp paren_expr(false, _sources, _query) do + defp paren_expr(false, _sources, _query, _location) do ["(1 = 0)"] end - defp paren_expr(expr, sources, query) do - [?(, expr(expr, sources, query), ?)] + defp paren_expr(expr, sources, query, location) do + [?(, expr(expr, sources, query, location), ?)] + end + + defp top_level_expr(%Ecto.SubQuery{}, _sources, parent_query, location) + when location not in [:where, :join, :on, :from] do + error!( + parent_query, + "Tds adapter only supports subqueries in the `where`, `join`, `on` and `from` option, found one in #{inspect(location)}." + ) end - defp top_level_expr(%Ecto.SubQuery{query: query}, sources, parent_query) do + defp top_level_expr(%Ecto.SubQuery{query: query}, sources, parent_query, _location) do combinations = Enum.map(query.combinations, fn {type, combination_query} -> {type, put_in(combination_query.aliases[@parent_as], {parent_query, sources})} @@ -719,34 +730,34 @@ if Code.ensure_loaded?(Tds) do [all(query, subquery_as_prefix(sources))] end - defp top_level_expr(other, sources, parent_query) do - expr(other, sources, parent_query) + defp top_level_expr(other, sources, parent_query, location) do + expr(other, sources, parent_query, location) end # :^ - represents parameter ix is index number - defp expr({:^, [], [idx]}, _sources, _query) do + defp expr({:^, [], [idx]}, _sources, _query, _location) do "@#{idx + 1}" end - defp expr({{:., _, [{:parent_as, _, [as]}, field]}, _, []}, _sources, query) + defp expr({{:., _, [{:parent_as, _, [as]}, field]}, _, []}, _sources, query, _location) when is_atom(field) do {ix, sources} = get_parent_sources_ix(query, as) {_, name, _} = elem(sources, ix) [name, ?. | quote_name(field)] end - defp expr({{:., _, [{:&, _, [idx]}, field]}, _, []}, sources, _query) + defp expr({{:., _, [{:&, _, [idx]}, field]}, _, []}, sources, _query, _location) when is_atom(field) or is_binary(field) do {_, name, _} = elem(sources, idx) [name, ?. | quote_name(field)] end - defp expr({:&, _, [idx]}, sources, _query) do + defp expr({:&, _, [idx]}, sources, _query, _location) do {_table, source, _schema} = elem(sources, idx) source end - defp expr({:&, _, [idx, fields, _counter]}, sources, query) do + defp expr({:&, _, [idx, fields, _counter]}, sources, query, _location) do {_table, name, schema} = elem(sources, idx) if is_nil(schema) and is_nil(fields) do @@ -762,105 +773,110 @@ if Code.ensure_loaded?(Tds) do end # example from {:in, [], [1, {:^, [], [0, 0]}]} - defp expr({:in, _, [_left, []]}, _sources, _query) do + defp expr({:in, _, [_left, []]}, _sources, _query, _location) do "0=1" end # example from(p in Post, where: p.id in [1,2, ^some_id]) - defp expr({:in, _, [left, right]}, sources, query) when is_list(right) do - args = Enum.map_join(right, ",", &expr(&1, sources, query)) - [expr(left, sources, query), " IN (", args | ")"] + defp expr({:in, _, [left, right]}, sources, query, location) when is_list(right) do + args = Enum.map_join(right, ",", &expr(&1, sources, query, location)) + [expr(left, sources, query, location), " IN (", args | ")"] end # example from(p in Post, where: p.id in []) - defp expr({:in, _, [_, {:^, _, [_, 0]}]}, _sources, _query), do: "0=1" + defp expr({:in, _, [_, {:^, _, [_, 0]}]}, _sources, _query, _location), do: "0=1" # example from(p in Post, where: p.id in ^some_list) # or from(p in Post, where: p.id in ^[]) - defp expr({:in, _, [left, {:^, _, [idx, length]}]}, sources, query) do + defp expr({:in, _, [left, {:^, _, [idx, length]}]}, sources, query, location) do args = list_param_to_args(idx, length) - [expr(left, sources, query), " IN (", args | ")"] + [expr(left, sources, query, location), " IN (", args | ")"] end - defp expr({:in, _, [left, %Ecto.SubQuery{} = subquery]}, sources, query) do - [expr(left, sources, query), " IN ", expr(subquery, sources, query)] + defp expr({:in, _, [left, %Ecto.SubQuery{} = subquery]}, sources, query, location) do + [expr(left, sources, query, location), " IN ", expr(subquery, sources, query, location)] end - defp expr({:in, _, [left, right]}, sources, query) do - [expr(left, sources, query), " = ANY(", expr(right, sources, query) | ")"] + defp expr({:in, _, [left, right]}, sources, query, location) do + [ + expr(left, sources, query, location), + " = ANY(", + expr(right, sources, query, location) | ")" + ] end - defp expr({:is_nil, _, [arg]}, sources, query) do - "#{expr(arg, sources, query)} IS NULL" + defp expr({:is_nil, _, [arg]}, sources, query, location) do + "#{expr(arg, sources, query, location)} IS NULL" end - defp expr({:not, _, [expr]}, sources, query) do - ["NOT (", expr(expr, sources, query) | ")"] + defp expr({:not, _, [expr]}, sources, query, location) do + ["NOT (", expr(expr, sources, query, location) | ")"] end - defp expr({:filter, _, _}, _sources, query) do + defp expr({:filter, _, _}, _sources, query, _location) do error!(query, "Tds adapter does not support aggregate filters") end - defp expr(%Ecto.SubQuery{} = subquery, sources, parent_query) do - [?(, top_level_expr(subquery, sources, parent_query), ?)] + defp expr(%Ecto.SubQuery{} = subquery, sources, parent_query, location) do + [?(, top_level_expr(subquery, sources, parent_query, location), ?)] end - defp expr({:fragment, _, [kw]}, _sources, query) when is_list(kw) or tuple_size(kw) == 3 do + defp expr({:fragment, _, [kw]}, _sources, query, _location) + when is_list(kw) or tuple_size(kw) == 3 do error!(query, "Tds adapter does not support keyword or interpolated fragments") end - defp expr({:fragment, _, parts}, sources, query) do + defp expr({:fragment, _, parts}, sources, query, location) do Enum.map(parts, fn {:raw, part} -> part - {:expr, expr} -> expr(expr, sources, query) + {:expr, expr} -> expr(expr, sources, query, location) end) |> parens_for_select end - defp expr({:values, _, [types, idx, num_rows]}, _, _query) do + defp expr({:values, _, [types, idx, num_rows]}, _, _query, _location) do [?(, values_list(types, idx + 1, num_rows), ?)] end - defp expr({:literal, _, [literal]}, _sources, _query) do + defp expr({:literal, _, [literal]}, _sources, _query, _location) do quote_name(literal) end - defp expr({:splice, _, [{:^, _, [idx, length]}]}, _sources, _query) do + defp expr({:splice, _, [{:^, _, [idx, length]}]}, _sources, _query, _location) do list_param_to_args(idx, length) end - defp expr({:selected_as, _, [name]}, _sources, _query) do + defp expr({:selected_as, _, [name]}, _sources, _query, _location) do [quote_name(name)] end - defp expr({:datetime_add, _, [datetime, count, interval]}, sources, query) do + defp expr({:datetime_add, _, [datetime, count, interval]}, sources, query, location) do [ "DATEADD(", interval, ", ", - interval_count(count, sources, query), + interval_count(count, sources, query, location), ", CAST(", - expr(datetime, sources, query), + expr(datetime, sources, query, location), " AS datetime2(6)))" ] end - defp expr({:date_add, _, [date, count, interval]}, sources, query) do + defp expr({:date_add, _, [date, count, interval]}, sources, query, location) do [ "CAST(DATEADD(", interval, ", ", - interval_count(count, sources, query), + interval_count(count, sources, query, location), ", CAST(", - expr(date, sources, query), + expr(date, sources, query, location), " AS datetime2(6))" | ") AS date)" ] end - defp expr({:count, _, []}, _sources, _query), do: "count(*)" + defp expr({:count, _, []}, _sources, _query, _location), do: "count(*)" - defp expr({:json_extract_path, _, _}, _sources, query) do + defp expr({:json_extract_path, _, _}, _sources, query, _location) do error!( query, "Tds adapter does not support json_extract_path expression" <> @@ -868,7 +884,7 @@ if Code.ensure_loaded?(Tds) do ) end - defp expr({fun, _, args}, sources, query) when is_atom(fun) and is_list(args) do + defp expr({fun, _, args}, sources, query, location) when is_atom(fun) and is_list(args) do {modifier, args} = case args do [rest, :distinct] -> {"DISTINCT ", [rest]} @@ -878,33 +894,37 @@ if Code.ensure_loaded?(Tds) do case handle_call(fun, length(args)) do {:binary_op, op} -> [left, right] = args - [op_to_binary(left, sources, query), op | op_to_binary(right, sources, query)] + + [ + op_to_binary(left, sources, query, location), + op | op_to_binary(right, sources, query, location) + ] {:fun, fun} -> [ fun, ?(, modifier, - Enum.map_intersperse(args, ", ", &top_level_expr(&1, sources, query)), + Enum.map_intersperse(args, ", ", &top_level_expr(&1, sources, query, location)), ?) ] end end - defp expr(list, sources, query) when is_list(list) do - Enum.map_join(list, ", ", &expr(&1, sources, query)) + defp expr(list, sources, query, location) when is_list(list) do + Enum.map_join(list, ", ", &expr(&1, sources, query, location)) end - defp expr({string, :varchar}, _sources, _query) + defp expr({string, :varchar}, _sources, _query, _location) when is_binary(string) do "'#{escape_string(string)}'" end - defp expr(string, _sources, _query) when is_binary(string) do + defp expr(string, _sources, _query, _location) when is_binary(string) do "N'#{escape_string(string)}'" end - defp expr(%Decimal{exp: exp} = decimal, _sources, _query) do + defp expr(%Decimal{exp: exp} = decimal, _sources, _query, _location) do # this should help gaining precision for decimals values embedded in query # but this is still not good enough, for instance: # @@ -927,12 +947,14 @@ if Code.ensure_loaded?(Tds) do ] end - defp expr(%Tagged{value: binary, type: :binary}, _sources, _query) when is_binary(binary) do + defp expr(%Tagged{value: binary, type: :binary}, _sources, _query, _location) + when is_binary(binary) do hex = Base.encode16(binary, case: :lower) "0x#{hex}" end - defp expr(%Tagged{value: binary, type: :uuid}, _sources, _query) when is_binary(binary) do + defp expr(%Tagged{value: binary, type: :uuid}, _sources, _query, _location) + when is_binary(binary) do case binary do <<_::64, ?-, _::32, ?-, _::32, ?-, _::32, ?-, _::96>> -> {:ok, value} = Tds.Ecto.UUID.dump(binary) @@ -943,36 +965,36 @@ if Code.ensure_loaded?(Tds) do end end - defp expr(%Tagged{value: other, type: type}, sources, query) + defp expr(%Tagged{value: other, type: type}, sources, query, location) when type in [:varchar, :nvarchar] do - "CAST(#{expr(other, sources, query)} AS #{column_type(type, [])}(max))" + "CAST(#{expr(other, sources, query, location)} AS #{column_type(type, [])}(max))" end - defp expr(%Tagged{value: other, type: :integer}, sources, query) do - "CAST(#{expr(other, sources, query)} AS bigint)" + defp expr(%Tagged{value: other, type: :integer}, sources, query, location) do + "CAST(#{expr(other, sources, query, location)} AS bigint)" end - defp expr(%Tagged{value: other, type: type}, sources, query) do - "CAST(#{expr(other, sources, query)} AS #{column_type(type, [])})" + defp expr(%Tagged{value: other, type: type}, sources, query, location) do + "CAST(#{expr(other, sources, query, location)} AS #{column_type(type, [])})" end - defp expr(nil, _sources, _query), do: "NULL" - defp expr(true, _sources, _query), do: "1" - defp expr(false, _sources, _query), do: "0" + defp expr(nil, _sources, _query, _location), do: "NULL" + defp expr(true, _sources, _query, _location), do: "1" + defp expr(false, _sources, _query, _location), do: "0" - defp expr(literal, _sources, _query) when is_binary(literal) do + defp expr(literal, _sources, _query, _location) when is_binary(literal) do "'#{escape_string(literal)}'" end - defp expr(literal, _sources, _query) when is_integer(literal) do + defp expr(literal, _sources, _query, _location) when is_integer(literal) do Integer.to_string(literal) end - defp expr(literal, _sources, _query) when is_float(literal) do + defp expr(literal, _sources, _query, _location) when is_float(literal) do Float.to_string(literal) end - defp expr(field, _sources, query) do + defp expr(field, _sources, query, _location) do error!(query, "unsupported MSSQL expressions: `#{inspect(field)}`") end @@ -995,28 +1017,28 @@ if Code.ensure_loaded?(Tds) do end) end - defp op_to_binary({op, _, [_, _]} = expr, sources, query) when op in @binary_ops do - paren_expr(expr, sources, query) + defp op_to_binary({op, _, [_, _]} = expr, sources, query, location) when op in @binary_ops do + paren_expr(expr, sources, query, location) end - defp op_to_binary({:is_nil, _, [_]} = expr, sources, query) do - paren_expr(expr, sources, query) + defp op_to_binary({:is_nil, _, [_]} = expr, sources, query, location) do + paren_expr(expr, sources, query, location) end - defp op_to_binary(expr, sources, query) do - expr(expr, sources, query) + defp op_to_binary(expr, sources, query, location) do + expr(expr, sources, query, location) end - defp interval_count(count, _sources, _query) when is_integer(count) do + defp interval_count(count, _sources, _query, _location) when is_integer(count) do Integer.to_string(count) end - defp interval_count(count, _sources, _query) when is_float(count) do + defp interval_count(count, _sources, _query, _location) when is_float(count) do :erlang.float_to_binary(count, [:compact, decimals: 16]) end - defp interval_count(count, sources, query) do - expr(count, sources, query) + defp interval_count(count, sources, query, location) do + expr(count, sources, query, location) end defp returning([], _verb), do: [] @@ -1639,7 +1661,7 @@ if Code.ensure_loaded?(Tds) do defp get_source(query, sources, ix, source) do {expr, name, _schema} = elem(sources, ix) name = maybe_add_column_names(source, name) - {expr || expr(source, sources, query), name} + {expr || expr(source, sources, query, :from), name} end defp get_parent_sources_ix(query, as) do diff --git a/test/ecto/adapters/tds_test.exs b/test/ecto/adapters/tds_test.exs index bbc9a562..01b25b0b 100644 --- a/test/ecto/adapters/tds_test.exs +++ b/test/ecto/adapters/tds_test.exs @@ -529,8 +529,11 @@ defmodule Ecto.Adapters.TdsTest do |> select([r], r.x) |> plan() - assert all(query) == - ~s{SELECT s0.[x] FROM [schema] AS s0 ORDER BY exists(SELECT ss0.[x] AS [result] FROM [schema] AS ss0 WHERE (ss0.[x] = s0.[x]))} + assert_raise Ecto.QueryError, + ~r/Tds adapter only supports subqueries in the/, + fn -> + all(query) + end for dir <- [:asc_nulls_first, :asc_nulls_last, :desc_nulls_first, :desc_nulls_last] do assert_raise Ecto.QueryError, ~r"#{dir} is not supported in ORDER BY in MSSQL", fn -> @@ -900,8 +903,11 @@ defmodule Ecto.Adapters.TdsTest do ) |> plan() - assert all(query) == - ~s{SELECT s0.[x] FROM [schema] AS s0 GROUP BY exists(SELECT ss0.[x] AS [result] FROM [schema] AS ss0 WHERE (ss0.[x] = s0.[x]))} + assert_raise Ecto.QueryError, + ~r/Tds adapter only supports subqueries in the/, + fn -> + all(query) + end end test "interpolated values" do