elixir-ecto / db_connection

Database connection behaviour
http://hexdocs.pm/db_connection/DBConnection.html
306 stars 113 forks source link

Incorrect example in docs for DBConnection.prepare #63

Closed wojtekmach closed 7 years ago

wojtekmach commented 7 years ago

Hi, I was trying to follow docs (I was implementing an after_connect hook) and I noticed example is incorrect:

(db_connection.ex:400)

{ok, query}   = DBConnection.prepare(pid, "SELECT id FROM table")
{:ok, result} = DBConnection.execute(pid, query, [])
:ok           = DBConnection.close(pid, query)
  1. the second argument to DBConnection.prepare must be something that implements DBConnection.Query (e.g. Postgrex.Query), not a string
  2. there's no DBConnection.execute/3, it should be: DBConnection.execute(pid, query, [], [])
  3. (nitpick) the variable pid could be called conn to match function head

I'm happy to open up a PR with docs fixes (for some or all points) - what would be the best example of a query argument to prepare/2? Not sure it makes to assume Postgrex.Query, but at the same time it would be good to be concrete.

fishcakez commented 7 years ago
  1. Is tricky because nothing implements DBConnection.Query but we could just have %Query{}.
  2. The code should match the docs in this situation, it might be other places don't have optional opts argument. If you do that everywhere it would be good.
  3. Sure.

An after_connect hook should be using Postgrex and not call DBConnection directly. Of course it may work but it is not supported.

wojtekmach commented 7 years ago

An after_connect hook should be using Postgrex and not call DBConnection directly. Of course it may work but it is not supported.

Thank you, this is very helpful. I'll use Postgrex.execute over calling DBConnection.prepare_execute. For reference I was doing the following:

  def set_timezone(conn) do
    q = %Postgrex.Query{name: "set_timezone", statement: "SET TIME ZONE 'UTC';"}
    {:ok, query, _result} = DBConnection.prepare_execute(conn, q, [], [])
    {:ok, _} = DBConnection.close(conn, query)
  end
fishcakez commented 7 years ago

@wojtekmach I think you want Postgrex.query, not Postgrex.execute, because you aren't making use of named queries. A PR would be welcome for the suggested changes btw.

wojtekmach commented 7 years ago

I think you want Postgrex.query, not Postgrex.execute, because you aren't making use of named queries.

that makes sense, thanks for the tip!

Btw, as you can see, it wasn't immediately obvious to me how to use this hook. I'd be happy to add some docs around this, where would it appropriate? Ecto, Postgrex, both?

I was thinking that perhaps some parts of my use case could be useful for documenting this with an example:

config :my_app, MyApp.Repo,
  adapter: Ecto.Adapters.Postgres,
  after_connect: {MyApp.Repo, :set_timezone, []}

defmodule MyApp.Repo do
  def set_timezone(conn) do
    {:ok, _} = Postgrex.query(conn, "SET TIME ZONE 'UTC';", [])
  end
end

P.S. I went down this rabbit hole after reading this comment: https://github.com/elixir-ecto/ecto/issues/1466#issuecomment-229667413

fishcakez commented 7 years ago

Postgrex would be the place for the docs. I guess/hope after_connect is documented in some form in Postgrex.start_link.