Merge pull request #464 from bpvgoncalves/fix_brk_rev_dep

R/cpp11.R

@@ -16,14 +16,6 @@ connection_in_transaction <- function(con_) {
   .Call(`_RSQLite_connection_in_transaction`, con_)
 }
 
-connection_add_transaction <- function(con_) {
-  invisible(.Call(`_RSQLite_connection_add_transaction`, con_))
-}
-
-connection_rem_transaction <- function(con_) {
-  invisible(.Call(`_RSQLite_connection_rem_transaction`, con_))
-}
-
 connection_copy_database <- function(from, to) {
   invisible(.Call(`_RSQLite_connection_copy_database`, from, to))
 }

R/dbBegin_SQLiteConnection.R

@@ -7,7 +7,6 @@ dbBegin_SQLiteConnection <- function(conn, .name = NULL, ..., name = NULL) {
   } else {
     dbExecute(conn, paste0("SAVEPOINT ", dbQuoteIdentifier(conn, name)))
   }
-  connection_add_transaction(conn@ptr)
   invisible(TRUE)
 }
 #' @rdname sqlite-transaction

R/dbCommit_SQLiteConnection.R

@@ -7,7 +7,6 @@ dbCommit_SQLiteConnection <- function(conn, .name = NULL, ..., name = NULL) {
   } else {
     dbExecute(conn, paste0("RELEASE SAVEPOINT ", dbQuoteIdentifier(conn, name)))
   }
-  connection_rem_transaction(conn@ptr)
   invisible(TRUE)
 }
 #' @rdname sqlite-transaction

R/dbRollback_SQLiteConnection.R

@@ -15,7 +15,6 @@ dbRollback_SQLiteConnection <- function(conn, .name = NULL, ..., name = NULL) {
     dbExecute(conn, paste0("ROLLBACK TO ", name_quoted))
     dbExecute(conn, paste0("RELEASE SAVEPOINT ", name_quoted))
   }
-  connection_rem_transaction(conn@ptr)
   invisible(TRUE)
 }
 #' @rdname sqlite-transaction

src/DbConnection.cpp

@@ -5,8 +5,7 @@
 DbConnection::DbConnection(const std::string& path, const bool allow_ext, const int flags, const std::string& vfs, bool with_alt_types)
   : pConn_(NULL),
     with_alt_types_(with_alt_types),
-    busy_callback_(NULL),
-    transaction_(0) {
+    busy_callback_(NULL) {
 
   // Get the underlying database connection
   int rc = sqlite3_open_v2(path.c_str(), &pConn_, flags, vfs.empty() ? NULL : vfs.c_str());
@@ -114,21 +113,11 @@ int DbConnection::busy_callback_helper(void *data, int num)
 }
 
 bool DbConnection::in_transaction() const {
-  if (transaction_ > 0) {
+  int status = sqlite3_get_autocommit(pConn_);
+
+  if (status == 0) {
     return true;
   } else {
     return false;
   }
 }
-
-void DbConnection::add_transaction() {
-  transaction_ += 1;
-}
-
-void DbConnection::rem_transaction() {
-  if (transaction_ > 0) {
-    transaction_ -= 1;
-  } else {
-    cpp11::stop("Cannot remove non-existent transactions");
-  }
-}

src/DbConnection.h

@@ -53,16 +53,13 @@ class DbConnection : boost::noncopyable {
   void set_busy_handler(SEXP r_callback);
 
   bool in_transaction() const;
-  void add_transaction();
-  void rem_transaction();
 
 private:
   sqlite3* pConn_;
   const bool with_alt_types_;
   SEXP busy_callback_;
   void release_callback_data();
   static int busy_callback_helper(void *data, int num);
-  int transaction_;
 };
 
 #endif // __RSQLITE_SQLITE_CONNECTION__

src/connection.cpp

@@ -72,17 +72,6 @@ bool connection_in_transaction(cpp11::external_pointer<DbConnectionPtr> con_){
   DbConnectionPtr* con = con_.get();
   return (con->get()->in_transaction() != 0);
 }
-[[cpp11::register]]
-void connection_add_transaction(cpp11::external_pointer<DbConnectionPtr> con_){
-  DbConnectionPtr* con = con_.get();
-  con->get()->add_transaction();
-}
-[[cpp11::register]]
-void connection_rem_transaction(cpp11::external_pointer<DbConnectionPtr> con_){
-  DbConnectionPtr* con = con_.get();
-  con->get()->rem_transaction();
-}
-
 
 // Quoting
 

src/cpp11.cpp

@@ -35,22 +35,6 @@ extern "C" SEXP _RSQLite_connection_in_transaction(SEXP con_) {
   END_CPP11
 }
 // connection.cpp
-void connection_add_transaction(cpp11::external_pointer<DbConnectionPtr> con_);
-extern "C" SEXP _RSQLite_connection_add_transaction(SEXP con_) {
-  BEGIN_CPP11
-    connection_add_transaction(cpp11::as_cpp<cpp11::decay_t<cpp11::external_pointer<DbConnectionPtr>>>(con_));
-    return R_NilValue;
-  END_CPP11
-}
-// connection.cpp
-void connection_rem_transaction(cpp11::external_pointer<DbConnectionPtr> con_);
-extern "C" SEXP _RSQLite_connection_rem_transaction(SEXP con_) {
-  BEGIN_CPP11
-    connection_rem_transaction(cpp11::as_cpp<cpp11::decay_t<cpp11::external_pointer<DbConnectionPtr>>>(con_));
-    return R_NilValue;
-  END_CPP11
-}
-// connection.cpp
 void connection_copy_database(const cpp11::external_pointer<DbConnectionPtr>& from, const cpp11::external_pointer<DbConnectionPtr>& to);
 extern "C" SEXP _RSQLite_connection_copy_database(SEXP from, SEXP to) {
   BEGIN_CPP11
@@ -171,13 +155,11 @@ extern "C" SEXP _RSQLite_init_logging(SEXP log_level) {
 
 extern "C" {
 static const R_CallMethodDef CallEntries[] = {
-    {"_RSQLite_connection_add_transaction",   (DL_FUNC) &_RSQLite_connection_add_transaction,   1},
     {"_RSQLite_connection_connect",           (DL_FUNC) &_RSQLite_connection_connect,           5},
     {"_RSQLite_connection_copy_database",     (DL_FUNC) &_RSQLite_connection_copy_database,     2},
     {"_RSQLite_connection_import_file",       (DL_FUNC) &_RSQLite_connection_import_file,       6},
     {"_RSQLite_connection_in_transaction",    (DL_FUNC) &_RSQLite_connection_in_transaction,    1},
     {"_RSQLite_connection_release",           (DL_FUNC) &_RSQLite_connection_release,           1},
-    {"_RSQLite_connection_rem_transaction",   (DL_FUNC) &_RSQLite_connection_rem_transaction,   1},
     {"_RSQLite_connection_valid",             (DL_FUNC) &_RSQLite_connection_valid,             1},
     {"_RSQLite_extension_load",               (DL_FUNC) &_RSQLite_extension_load,               3},
     {"_RSQLite_init_logging",                 (DL_FUNC) &_RSQLite_init_logging,                 1},

tests/testthat/test-transactions.R

@@ -28,8 +28,8 @@ test_that("commit unnamed transactions", {
 
   expect_false(sqliteIsTransacting(con))
   dbBegin(con)
-  expect_true(sqliteIsTransacting(con))
   dbWriteTable(con, "a", data.frame(a = 1))
+  expect_true(sqliteIsTransacting(con))
   expect_equal(dbListTables(con), "a")
   expect_equal(dbListTables(con2), character())
   dbCommit(con)
@@ -56,8 +56,8 @@ test_that("rollback unnamed transactions", {
 
   expect_false(sqliteIsTransacting(con))
   dbBegin(con)
-  expect_true(sqliteIsTransacting(con))
   dbWriteTable(con, "a", data.frame(a = 1))
+  expect_true(sqliteIsTransacting(con))
   expect_equal(dbListTables(con), "a")
   expect_equal(dbListTables(con2), character())
   dbRollback(con)
@@ -85,6 +85,7 @@ test_that("no nested unnamed transactions (commit after error)", {
   dbBegin(con)
   expect_true(sqliteIsTransacting(con))
   expect_error(dbBegin(con))
+  expect_true(sqliteIsTransacting(con))
   dbCommit(con)
   expect_false(sqliteIsTransacting(con))
 
@@ -109,6 +110,7 @@ test_that("no nested unnamed transactions (rollback after error)", {
   dbBegin(con)
   expect_true(sqliteIsTransacting(con))
   expect_error(dbBegin(con))
+  expect_true(sqliteIsTransacting(con))
   dbRollback(con)
   expect_false(sqliteIsTransacting(con))
 
@@ -131,8 +133,8 @@ test_that("commit named transactions", {
 
   expect_false(sqliteIsTransacting(con))
   dbBegin(con, name = "tx")
-  expect_true(sqliteIsTransacting(con))
   dbWriteTable(con, "a", data.frame(a = 1))
+  expect_true(sqliteIsTransacting(con))
   expect_equal(dbListTables(con), "a")
   expect_equal(dbListTables(con2), character())
   dbCommit(con, name = "tx")
@@ -159,8 +161,8 @@ test_that("rollback named transactions", {
 
   expect_false(sqliteIsTransacting(con))
   dbBegin(con, name = "tx")
-  expect_true(sqliteIsTransacting(con))
   dbWriteTable(con, "a", data.frame(a = 1))
+  expect_true(sqliteIsTransacting(con))
   expect_equal(dbListTables(con), "a")
   expect_equal(dbListTables(con2), character())
 
@@ -188,8 +190,8 @@ test_that("nested named transactions (commit - commit)", {
 
   expect_false(sqliteIsTransacting(con))
   dbBegin(con, name = "tx")
-  expect_true(sqliteIsTransacting(con))
   dbWriteTable(con, "a", data.frame(a = 1))
+  expect_true(sqliteIsTransacting(con))
   expect_equal(dbListTables(con), "a")
   expect_equal(dbListTables(con2), character())
 
@@ -231,8 +233,8 @@ test_that("nested named transactions (commit - rollback)", {
 
   expect_false(sqliteIsTransacting(con))
   dbBegin(con, name = "tx")
-  expect_true(sqliteIsTransacting(con))
   dbWriteTable(con, "a", data.frame(a = 1))
+  expect_true(sqliteIsTransacting(con))
   expect_equal(dbListTables(con), "a")
   expect_equal(dbListTables(con2), character())
 
@@ -274,8 +276,8 @@ test_that("nested named transactions (rollback - commit)", {
 
   expect_false(sqliteIsTransacting(con))
   dbBegin(con, name = "tx")
-  expect_true(sqliteIsTransacting(con))
   dbWriteTable(con, "a", data.frame(a = 1))
+  expect_true(sqliteIsTransacting(con))
   expect_equal(dbListTables(con), "a")
   expect_equal(dbListTables(con2), character())
 
@@ -317,8 +319,8 @@ test_that("nested named transactions (rollback - rollback)", {
 
   expect_false(sqliteIsTransacting(con))
   dbBegin(con, name = "tx")
-  expect_true(sqliteIsTransacting(con))
   dbWriteTable(con, "a", data.frame(a = 1))
+  expect_true(sqliteIsTransacting(con))
   expect_equal(dbListTables(con), "a")
   expect_equal(dbListTables(con2), character())
 
@@ -360,8 +362,8 @@ test_that("named transactions with keywords", {
 
   expect_false(sqliteIsTransacting(con))
   dbBegin(con, name = "SELECT")
-  expect_true(sqliteIsTransacting(con))
   dbWriteTable(con, "a", data.frame(a = 1))
+  expect_true(sqliteIsTransacting(con))
   expect_equal(dbListTables(con), "a")
   expect_equal(dbListTables(con2), character())
   dbCommit(con, name = "SELECT")
@@ -370,12 +372,126 @@ test_that("named transactions with keywords", {
   expect_equal(dbListTables(con2), "a")
 
   dbBegin(con, name = "WHERE")
-  expect_true(sqliteIsTransacting(con))
   dbWriteTable(con, "b", data.frame(b = 1))
+  expect_true(sqliteIsTransacting(con))
   expect_equal(dbListTables(con), c("a", "b"))
   expect_equal(dbListTables(con2), "a")
   dbRollback(con, name = "WHERE")
   expect_false(sqliteIsTransacting(con))
   expect_equal(dbListTables(con), "a")
   expect_equal(dbListTables(con2), "a")
 })
+
+test_that("transactions managed without dbBegin+dbCommit", {
+  db_file <- tempfile("transactions", fileext = ".sqlite")
+  con <- dbConnect(SQLite(), db_file)
+  on.exit(dbDisconnect(con))
+
+  expect_false(sqliteIsTransacting(con))
+  dbExecute(con, "BEGIN")
+  expect_true(sqliteIsTransacting(con))
+  dbWriteTable(con, "a", data.frame(a = 1))
+  expect_true(sqliteIsTransacting(con))
+  dbExecute(con, "END")
+  expect_false(sqliteIsTransacting(con))
+
+  dbExecute(con, "BEGIN")
+  expect_true(sqliteIsTransacting(con))
+  dbWriteTable(con, "b", data.frame(b = 1))
+  expect_true(sqliteIsTransacting(con))
+  dbExecute(con, "COMMIT")
+  expect_false(sqliteIsTransacting(con))
+
+  expect_false(sqliteIsTransacting(con))
+  dbExecute(con, "BEGIN IMMEDIATE")
+  expect_true(sqliteIsTransacting(con))
+  dbWriteTable(con, "c", data.frame(c = 1))
+  expect_true(sqliteIsTransacting(con))
+  dbExecute(con, "END")
+  expect_false(sqliteIsTransacting(con))
+
+  dbExecute(con, "BEGIN IMMEDIATE")
+  expect_true(sqliteIsTransacting(con))
+  dbWriteTable(con, "d", data.frame(d = 1))
+  expect_true(sqliteIsTransacting(con))
+  dbExecute(con, "COMMIT")
+  expect_false(sqliteIsTransacting(con))
+})
+
+test_that("transactions managed without dbBegin+dbRollback", {
+  db_file <- tempfile("transactions", fileext = ".sqlite")
+  con <- dbConnect(SQLite(), db_file)
+  on.exit(dbDisconnect(con))
+
+  expect_false(sqliteIsTransacting(con))
+  dbExecute(con, "BEGIN")
+  expect_true(sqliteIsTransacting(con))
+  dbWriteTable(con, "a", data.frame(a = 1))
+  expect_true(sqliteIsTransacting(con))
+  dbExecute(con, "ROLLBACK")
+  expect_false(sqliteIsTransacting(con))
+
+  dbExecute(con, "BEGIN IMMEDIATE")
+  expect_true(sqliteIsTransacting(con))
+  dbWriteTable(con, "b", data.frame(b = 1))
+  expect_true(sqliteIsTransacting(con))
+  dbExecute(con, "ROLLBACK")
+  expect_false(sqliteIsTransacting(con))
+})
+
+test_that("mixed management of transactions", {
+  db_file <- tempfile("transactions", fileext = ".sqlite")
+  con <- dbConnect(SQLite(), db_file)
+  on.exit(dbDisconnect(con))
+
+  expect_false(sqliteIsTransacting(con))
+  dbExecute(con, "BEGIN")
+  expect_true(sqliteIsTransacting(con))
+  dbWriteTable(con, "a", data.frame(a = 1))
+  expect_true(sqliteIsTransacting(con))
+  dbCommit(con)
+  expect_false(sqliteIsTransacting(con))
+
+  dbExecute(con, "BEGIN IMMEDIATE")
+  expect_true(sqliteIsTransacting(con))
+  dbWriteTable(con, "b", data.frame(b = 1))
+  expect_true(sqliteIsTransacting(con))
+  dbCommit(con)
+  expect_false(sqliteIsTransacting(con))
+
+  expect_false(sqliteIsTransacting(con))
+  dbExecute(con, "BEGIN")
+  expect_true(sqliteIsTransacting(con))
+  dbWriteTable(con, "c", data.frame(c = 1))
+  expect_true(sqliteIsTransacting(con))
+  dbRollback(con)
+  expect_false(sqliteIsTransacting(con))
+
+  dbExecute(con, "BEGIN IMMEDIATE")
+  expect_true(sqliteIsTransacting(con))
+  dbWriteTable(con, "d", data.frame(d = 1))
+  expect_true(sqliteIsTransacting(con))
+  dbRollback(con)
+  expect_false(sqliteIsTransacting(con))
+
+  dbBegin(con)
+  expect_true(sqliteIsTransacting(con))
+  dbWriteTable(con, "e", data.frame(e = 1))
+  expect_true(sqliteIsTransacting(con))
+  dbExecute(con, "COMMIT")
+  expect_false(sqliteIsTransacting(con))
+
+  dbBegin(con)
+  expect_true(sqliteIsTransacting(con))
+  dbWriteTable(con, "f", data.frame(f = 1))
+  expect_true(sqliteIsTransacting(con))
+  dbExecute(con, "END")
+  expect_false(sqliteIsTransacting(con))
+
+  dbBegin(con)
+  expect_true(sqliteIsTransacting(con))
+  dbWriteTable(con, "g", data.frame(g = 1))
+  expect_true(sqliteIsTransacting(con))
+  dbExecute(con, "ROLLBACK")
+  expect_false(sqliteIsTransacting(con))
+})