[AUTO-3289] General improvements, add DBConnection adapter #32
Conversation
|
|
||
| def to_string(%{statement: statement}) do | ||
| case statement do | ||
| statement when is_binary(statement) -> IO.iodata_to_binary(statement) |
There was a problem hiding this comment.
If it's already a binary, it should be OK to pass through directly.
| statement when is_binary(statement) -> IO.iodata_to_binary(statement) | |
| statement when is_binary(statement) -> statement |
There was a problem hiding this comment.
In Result.t(), statement is typed as String.t() | nil. If it can be iodata(), I would update the typespec to indicate that. A Query.t() typespec would also be a nice addition here.
| columns: Enum.map(columns, &to_string(&1)), | ||
| rows: rows, | ||
| num_rows: Enum.count(rows) | ||
| } |
There was a problem hiding this comment.
You'll probably want to set success and statement on the result (ignore if you were already planning to do this 🙂).
| def disconnect(_err, %{pid: pid}), do: Server.disconnect(pid) | ||
|
|
||
| @impl DBConnection | ||
| def checkout(state), do: {:ok, state} |
There was a problem hiding this comment.
I wonder if there should be any sort of limit on the maximum number of connections that can be checked out at once? Maybe it doesn't matter 🤷
lib/snowflex/db_connection.ex
Outdated
| opts = | ||
| Keyword.merge(config, | ||
| timeout: @timeout, | ||
| connection: connection | ||
| ) |
There was a problem hiding this comment.
isn't always override timeout from config with a compile-time @timeout ?
| @timeout timeout | ||
| @name __MODULE__ | ||
|
|
||
| def child_spec(_) do |
There was a problem hiding this comment.
why do we ignore child spec input options?
There was a problem hiding this comment.
opts should be provided statically when configuring your connection module with use Snowflake.DBConnection, opt1: _, opt2: _, .... Ignoring the opts in child_spec/1 ensures that the static config in the use macro is always correct, and not accidentally overridden in the supervision tree. I like this approach 👍
| end | ||
|
|
||
| defp parse_result(columns, rows) do | ||
| %Result{ |
There was a problem hiding this comment.
Should %Result structs always include statement? If not, it'd be nice to add something to the docs describing when it will/won't be set.
| case worker.param_query(state.pid, query.statement, params, opts) do | ||
| {:ok, {:selected, columns, rows}} -> | ||
| result = parse_result(columns, rows) | ||
| {:ok, query, result, state} | ||
|
|
||
| {:ok, {:selected, columns, rows, _}} -> | ||
| result = parse_result(columns, rows) | ||
| {:ok, query, result, state} | ||
|
|
||
| {:ok, result} -> | ||
| {:ok, query, result, state} | ||
|
|
||
| {:error, reason} -> | ||
| {:error, reason, state} | ||
| end |
There was a problem hiding this comment.
You could pull this case out into its own function so you don't have to duplicate it between sql_query and param_query.
lib/snowflex/db_connection/query.ex
Outdated
| def decode(_query, [], _opts), do: [] | ||
| def decode(_query, result, _opts), do: result |
There was a problem hiding this comment.
These can be combined
| def decode(_query, [], _opts), do: [] | |
| def decode(_query, result, _opts), do: result | |
| def decode(_query, result, _opts), do: result |
|
|
||
| def to_string(%{statement: statement}) do | ||
| case statement do | ||
| statement when is_binary(statement) -> IO.iodata_to_binary(statement) |
There was a problem hiding this comment.
In Result.t(), statement is typed as String.t() | nil. If it can be iodata(), I would update the typespec to indicate that. A Query.t() typespec would also be a nice addition here.
Ch4s3
changed the title
This is the first attempt to add
DBConnectionsupport.I tried to keep functionality the same, except for adding an extra function
process_result/2which will return results as current implementation instead of an{:ok, %Result{}}tuple.See
config/test.exsfor an example of configuration required, speciallypool_sizeoption.