elixir-ecto / ecto_sql

SQL-based adapters for Ecto and database migrations
https://hexdocs.pm/ecto_sql
Apache License 2.0
571 stars 312 forks source link

allow map query params #614

Closed ruslandoga closed 4 months ago

ruslandoga commented 4 months ago

👋

ClickHouse supports named params and Ch expresses them as maps:

 Ch.query(pid, "select {a:String}", %{"a" => "b"})

However using this via Ecto.Repo (and Ecto.Adapters.SQL) raises Dialyzer warnings:

Repo.query("select {a:String}", %{"a" => "b"})

This query works but gets a squiggly underline in VSCode and also gets flagged by mix dialyzer

The function call will not succeed.

Repo.query!(<<_::8, _::size(1)>>, %{:site_ids => [any()]})

will never return since the 2nd arguments differ
from the success typing arguments:

(
  binary() | maybe_improper_list(binary() | maybe_improper_list(any(), binary() | []) | byte(), binary() | []),
  [any()],
  Keyword.t()
)
josevalim commented 4 months ago

@ruslandoga this looks good to me, but I think we will need to change ecto_sql adapters to raise if they receive a map. Can you please send a PR on that side as well?

ruslandoga commented 4 months ago

I didn't think it through and didn't realize it would affect existing adapters.

Now I think it'd be safer to re-implement these Ecto.Adapters.SQL.query calls in ecto_ch but with a more permissive type signature.

Sorry!

ruslandoga commented 4 months ago

Since the alternative approach didn't work out, I'm reopening this PR :)

I made the existing adapters raise on non-list params in https://github.com/elixir-ecto/ecto_sql/pull/614/commits/a8ca22c7c4ecf056c86a3c14e92b075751489ce6

josevalim commented 4 months ago

:green_heart: :blue_heart: :purple_heart: :yellow_heart: :heart: