Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow map query params #614

Merged
merged 3 commits into from
Jun 15, 2024
Merged

allow map query params #614

merged 3 commits into from
Jun 15, 2024

Conversation

ruslandoga
Copy link
Contributor

@ruslandoga ruslandoga commented Jun 13, 2024

👋

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
Copy link
Member

@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
Copy link
Contributor Author

ruslandoga commented Jun 13, 2024

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 ruslandoga closed this Jun 13, 2024
@ruslandoga
Copy link
Contributor Author

ruslandoga commented Jun 14, 2024

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

I made the existing adapters raise on non-list params in a8ca22c

@josevalim josevalim merged commit 0c5581d into elixir-ecto:master Jun 15, 2024
19 of 20 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@ruslandoga ruslandoga deleted the allow-map-params branch June 15, 2024 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants