-
Notifications
You must be signed in to change notification settings - Fork 89
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
Improve error message we can't connect to the database #6
Comments
Can we please reopen this one? All we are going to say in this case is that the tcp connection is closed, which is an improvement, but I would rather say we can't authenticate in the first place. Otherwise users will be clueless to why the connection was closed. :) |
It was occasionally closed. |
Thanks @jquadrin and @liveforeverx ! |
If nobody working on errors at the moment, I'll tackle this. |
@liveforeverx I just changed the message, feel free to close if you are working on it already |
The current message is fine. TCP can be closed in other situations. We just need to explicitly check those for authentication errors. I think postgrex even has specific tests for this kind of stuff. :) |
So, pushed to master the code, that tests false user/database and shows error. Next step to test the connection with password/false password, will do it somewhere tomorrow. Tried to find the test on postgrex, but didn't find, where it tested... @josevalim , see this line: https://github.com/liveforeverx/mariaex/blob/master/lib/mariaex/connection.ex#L64 (avoiding of a problem), sometimes it happens, that tcp_closed will got and process terminated(with the linked starter process), before starter process get error. This lead to undefined behaviour, if trap_exit set to false: iex(1)> Mariaex.Connection.start_link(database: "non_existing")
** (EXIT from #PID<0.104.0>) %Mariaex.Error{mariadb: nil, message: "connection closed"}
Interactive Elixir (1.1.0-dev) - press Ctrl+C to exit (type h() ENTER for help)
iex(1)>
22:52:39.904 [error] GenServer #PID<0.106.0> terminating
Last message: {:tcp_closed, #Port<0.15965>}
State: %{backend_key: nil, opts: [hostname: "localhost", password: nil, username: "dima", database: "non_existing"], parameters: %{}, queue: {[], []}, rows: [], seqnum: 0, sock: {Mariaex.Connection.Tcp, #Port<0.15965>}, sock_mod: Mariaex.Connection.Tcp, state: :running, statement: nil, tail: "", types: :types_removed}
** (exit) %Mariaex.Error{mariadb: nil, message: "connection closed"}
iex(1)> Mariaex.Connection.start_link(database: "non_existing")
{:error,
%Mariaex.Error{mariadb: %{code: 1044,
message: "Access denied for user ''@'localhost' to database 'non_existing'"},
message: "Access denied for user ''@'localhost' to database 'non_existing'"}} Do not tested it on postgrex, but it can have the same problem. |
So, tested with master(fix password authorization): iex> Mariaex.Connection.start_link(username: "pass", password: "wrong_pass", database: "ecto_test")
{:error,
%Mariaex.Error{mariadb: %{code: 1045,
message: "Access denied for user 'pass'@'localhost' (using password: YES)"},
message: nil}}
iex> Mariaex.Connection.start_link(username: "pass", password: "pass", database: "ecto_test")
{:ok, #PID<0.106.0>}
And, there are tests for all other cases(false user | database, impossible to connect to database(tcp error)) here: https://github.com/liveforeverx/mariaex/blob/master/test/start_test.exs |
Beautiful! José Valimwww.plataformatec.com.br |
I think, the issue can be closed for a long time. |
❤️ 💚 💙 💛 💜 |
Today, if you pass bad credentials when starting the connection to the database, it fails with a case clause error on :tcp_closed. We should instead handle this case and return a proper error, saying we were unable to connect to the database.
The text was updated successfully, but these errors were encountered: