onboard transaction.exs suite, various transaction fixes (#112)

* fix issue around not properly tracking transaction state, not properly rollingback when closing connection, onboard
  transaction.exs tests
* use make_atom instead

Author: kevinlang

c_src/sqlite3_nif.c

@@ -145,6 +145,7 @@ exqlite_close(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
     assert(env);
 
     connection_t* conn = NULL;
+    int rc = SQLITE_OK;
 
     if (argc != 1) {
         return enif_make_badarg(env);
@@ -154,7 +155,21 @@ exqlite_close(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
         return make_error_tuple(env, "invalid_connection");
     }
 
+    int autocommit = sqlite3_get_autocommit(conn->db);
+    if (autocommit == 0) {
+        rc = sqlite3_exec(conn->db, "ROLLBACK;", NULL, NULL, NULL);
+        if (rc != SQLITE_OK) {
+            return make_sqlite3_error_tuple(env, rc, conn->db);
+        }
+    }
+
+    // note: _v2 may not fully close the connection, hence why we check if 
+    // any transaction is open above, to make sure other connections aren't blocked. 
+    // v1 is guaranteed to close or error, but will return error if any
+    // unfinalized statements, which we likely have, as we rely on the destructors 
+    // to later run to clean those up 
     sqlite3_close_v2(conn->db);
+
     conn->db = NULL;
 
     return make_atom(env, "ok");
@@ -539,6 +554,27 @@ exqlite_last_insert_rowid(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
     return make_ok_tuple(env, enif_make_int64(env, last_rowid));
 }
 
+static ERL_NIF_TERM
+exqlite_transaction_status(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
+{
+    assert(env);
+
+    connection_t* conn = NULL;
+
+    if (argc != 1) {
+        return enif_make_badarg(env);
+    }
+
+    if (!enif_get_resource(env, argv[0], connection_type, (void**)&conn)) {
+        return make_error_tuple(env, "invalid_connection");
+    }
+
+    int autocommit = sqlite3_get_autocommit(conn->db);
+    return make_ok_tuple(
+        env,
+        autocommit == 0 ? make_atom(env, "transaction") : make_atom(env, "idle"));
+}
+
 static void
 connection_type_destructor(ErlNifEnv* env, void* arg)
 {
@@ -611,6 +647,7 @@ static ErlNifFunc nif_funcs[] = {
   {"step", 2, exqlite_step, ERL_NIF_DIRTY_JOB_IO_BOUND},
   {"columns", 2, exqlite_columns, ERL_NIF_DIRTY_JOB_IO_BOUND},
   {"last_insert_rowid", 1, exqlite_last_insert_rowid, ERL_NIF_DIRTY_JOB_IO_BOUND},
+  {"transaction_status", 1, exqlite_transaction_status, ERL_NIF_DIRTY_JOB_IO_BOUND},
 };
 
 ERL_NIF_INIT(Elixir.Exqlite.Sqlite3NIF, nif_funcs, on_load, NULL, NULL, NULL)

integration_test/exqlite/all_test.exs

@@ -22,7 +22,7 @@ Code.require_file "#{ecto_sql}/integration_test/sql/sandbox.exs", __DIR__
 Code.require_file "#{ecto_sql}/integration_test/sql/sql.exs", __DIR__
 Code.require_file "#{ecto_sql}/integration_test/sql/stream.exs", __DIR__
 Code.require_file "#{ecto_sql}/integration_test/sql/subquery.exs", __DIR__
-# Code.require_file "#{ecto_sql}/integration_test/sql/transaction.exs", __DIR__
+Code.require_file "#{ecto_sql}/integration_test/sql/transaction.exs", __DIR__
 
 # added :modify_column and :alter_foreign_key
 Code.require_file "./ecto_sql/migration.exs", __DIR__

lib/exqlite/connection.ex

@@ -437,6 +437,7 @@ defmodule Exqlite.Connection do
     with {:ok, query} <- bind_params(query, params, state),
          {:ok, columns} <- get_columns(query, state),
          {:ok, rows} <- get_rows(query, state),
+         {:ok, transaction_status} <- Sqlite3.transaction_status(state.db),
          changes <- maybe_changes(state.db, query) do
       case query.command do
         command when command in [:delete, :insert, :update] ->
@@ -448,7 +449,7 @@ defmodule Exqlite.Connection do
               num_rows: changes,
               rows: maybe_rows(rows)
             ),
-            state
+            %{state | transaction_status: transaction_status}
           }
 
         _ ->
@@ -461,7 +462,7 @@ defmodule Exqlite.Connection do
               rows: rows,
               num_rows: Enum.count(rows)
             ),
-            state
+            %{state | transaction_status: transaction_status}
           }
       end
     end
@@ -489,26 +490,16 @@ defmodule Exqlite.Connection do
   end
 
   defp handle_transaction(call, statement, state) do
-    case Sqlite3.execute(state.db, statement) do
-      :ok ->
-        result = %Result{
-          command: call,
-          rows: [],
-          columns: [],
-          num_rows: 0
-        }
-
-        case call do
-          :rollback ->
-            {:ok, result, %{state | transaction_status: :idle}}
-
-          :commit ->
-            {:ok, result, %{state | transaction_status: :idle}}
-
-          _ ->
-            {:ok, result, %{state | transaction_status: :transaction}}
-        end
-
+    with :ok <- Sqlite3.execute(state.db, statement),
+         {:ok, transaction_status} <- Sqlite3.transaction_status(state.db) do
+      result = %Result{
+        command: call,
+        rows: [],
+        columns: [],
+        num_rows: 0
+      }
+      {:ok, result, %{state | transaction_status: transaction_status}}
+    else
       {:error, reason} ->
         {:disconnect, %Error{message: reason}, state}
     end

lib/exqlite/sqlite3.ex

@@ -81,6 +81,9 @@ defmodule Exqlite.Sqlite3 do
   @spec last_insert_rowid(db()) :: {:ok, integer()}
   def last_insert_rowid(conn), do: Sqlite3NIF.last_insert_rowid(conn)
 
+  @spec transaction_status(db()) :: {:ok, :idle | :transaction}
+  def transaction_status(conn), do: Sqlite3NIF.transaction_status(conn)
+
   @doc """
   Causes the database connection to free as much memory as it can. This is
   useful if you are on a memory restricted system.

lib/exqlite/sqlite3_nif.ex

@@ -43,5 +43,8 @@ defmodule Exqlite.Sqlite3NIF do
   @spec last_insert_rowid(db()) :: {:ok, integer()}
   def last_insert_rowid(_conn), do: :erlang.nif_error(:not_loaded)
 
+  @spec transaction_status(db()) :: {:ok, :idle | :transaction}
+  def transaction_status(_conn), do: :erlang.nif_error(:not_loaded)
+
   # TODO: add statement inspection tooling https://sqlite.org/c3ref/expanded_sql.html
 end