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

Update to DBConnection 2.0 #235

Merged
merged 19 commits into from
Sep 24, 2018
Merged

Update to DBConnection 2.0 #235

merged 19 commits into from
Sep 24, 2018

Conversation

josevalim
Copy link
Contributor

@josevalim josevalim commented Aug 31, 2018

Includes #196 and #234. Note DBConnection 2.0 has not been released yet. There is also some work pending:

  • Use database transaction status to prevent transaction errors
  • Support new low level cursor API with per fetch max rows
  • Handle execute errors while using cursors
  • Test that transaction status is updated for more query result packets

/cc @wojtekmach @fishcakez

@coveralls
Copy link

coveralls commented Aug 31, 2018

Coverage Status

Coverage decreased (-1.3%) to 83.673% when pulling ec14055 on josevalim:jv-db-connection-20 into 78cd8a7 on xerions:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+3.8%) to 88.803% when pulling 02bc0fb on josevalim:jv-db-connection-20 into 78cd8a7 on xerions:master.

assert capture_log(fn ->
{:ok, pid} = Mariaex.start_link([username: "non_existing", database: "mariaex_test"] ++ opts)
assert_receive {:EXIT, ^pid, :killed}, 5000
end) =~ "** (Mariaex.Error) (1045): Access denied for user 'non_existing'@'localhost'"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check up to and including the @ I guess. Unsure what happens on other languages though. Ideally we would have similar style to postgrex where every error code has a matching atom.

assert query("SELECT 42", []) == [[42]]
end

test "can not begin, commit or rollback savepoint transaction if not begun", context do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should mention savepoint in this name as its confusing to say can not begin if mnot not begun without that context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, we do mention savepoint. Or should we not mention it? Btw, you wrote this. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, this test is wonderful ;)

{:ok, {result, nil}, clean_state(s, flags)}
end

defp clean_state(state, flags) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to test that we always call this function after a query finishes.

@fishcakez
Copy link
Contributor

Handle execute errors while using cursors

Need some time to think about that this TODO means. Likely on the first handle_next call we need to test all code paths for cursors, and currently don't.

@fishcakez
Copy link
Contributor

fishcakez commented Sep 10, 2018

This looks good to merge if we open issues for the TODO items so we can unblock ecto dev.

lib/mariaex.ex Outdated
@@ -237,16 +236,9 @@ defmodule Mariaex do
Mariaex.execute(conn, query, ["%my%"])
"""
@spec execute(conn, Mariaex.Query.t, list, Keyword.t) ::
{:ok, Mariaex.Result.t} | {:error, Mariaex.Error.t}
{:ok, Mariaex.Query.t, Mariaex.Result.t} | {:error, Mariaex.Error.t}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can now return other exceptions in the error case?

lib/mariaex.ex Outdated
{:error, err} ->
raise err
err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think this right, don't we want to return the error tuple?

lib/mariaex.ex Outdated
{:error, err} ->
raise err
err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Contributor

@fishcakez fishcakez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems I reviewed only one commit before and you fixed all the comments in later ones. If we have Exception.t as return make sure docs have that everywhere, I left comment in one place.

lib/mariaex.ex Outdated
@@ -205,6 +209,58 @@ defmodule Mariaex do
end
end

@doc """
Prepares and executes a query and returns the result as
`{:ok, %Mariaex.Query{}, %Mariaex.Result{}}` or `{:error, %Mariaex.Error{}}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be any exception?

@wojtekmach wojtekmach mentioned this pull request Sep 24, 2018
@josevalim
Copy link
Contributor Author

This is ready to go. I have opened issue #238 to track what is left. We should close #196 after merging this.

@josevalim josevalim changed the title [WIP] Update to DBConnection 2.0 Update to DBConnection 2.0 Sep 24, 2018
@fishcakez fishcakez merged commit eac3104 into xerions:master Sep 24, 2018
@fishcakez
Copy link
Contributor

Thank you ❤️

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.

4 participants