Have the PerformTransaction class automatically handle nesting

No need to fiddle around with "startTransaction" booleans now that the
helper class is a little more intelligent. When the database is already
performing a transaction, no new one will be started.

7 files changed, 91 insertions, 84 deletions

diff --git a/src/account-server/storage.cpp b/src/account-server/storage.cpp
index 13047de..84dfedc 100644
--- a/src/account-server/storage.cpp
+++ b/src/account-server/storage.cpp
@@ -613,20 +613,15 @@ bool Storage::doesCharacterNameExist(const std::string& name)
  * received from a game server.
  *
  * @param ptr Character to store values in the database.
- * @param startTransaction set to false if this method is called as
- *                         nested transaction.
  * @return true on success
  */
-bool Storage::updateCharacter(Character *character,
-                              bool startTransaction)
+bool Storage::updateCharacter(Character *character)
 {
-    // Update the database Character data (see CharacterData for details)
-    if (startTransaction)
-    {
-        mDb->beginTransaction();
-    }
+    dal::PerformTransaction transaction(mDb);
+
     try
     {
+        // Update the database Character data (see CharacterData for details)
         std::ostringstream sqlUpdateCharacterInfo;
         sqlUpdateCharacterInfo
             << "update "        << CHARACTERS_TBL_NAME << " "
@@ -647,10 +642,6 @@ bool Storage::updateCharacter(Character *character,
     catch (const dal::DbSqlQueryExecFailure& e)
     {
         // TODO: throw an exception.
-        if (startTransaction)
-        {
-            mDb->rollbackTransaction();
-        }
         LOG_ERROR("(DALStorage::updateCharacter #1) SQL query failure: " << e.what());
         return false;
     }
@@ -671,9 +662,8 @@ bool Storage::updateCharacter(Character *character,
     }
     catch (const dal::DbSqlQueryExecFailure &e)
     {
-        if (startTransaction)
-            mDb->rollbackTransaction();
         LOG_ERROR("(DALStorage::updateCharacter #2) SQL query failure: " << e.what());
+        return false;
     }
 
     /**
@@ -691,10 +681,6 @@ bool Storage::updateCharacter(Character *character,
     catch (const dal::DbSqlQueryExecFailure& e)
     {
         // TODO: throw an exception.
-        if (startTransaction)
-        {
-            mDb->rollbackTransaction();
-        }
         LOG_ERROR("(DALStorage::updateCharacter #3) SQL query failure: " << e.what());
         return false;
     }
@@ -714,10 +700,6 @@ bool Storage::updateCharacter(Character *character,
     catch (const dal::DbSqlQueryExecFailure& e)
     {
         // TODO: throw an exception.
-        if (startTransaction)
-        {
-            mDb->rollbackTransaction();
-        }
         LOG_ERROR("(DALStorage::updateCharacter #4) SQL query failure: " << e.what());
         return false;
     }
@@ -748,10 +730,6 @@ bool Storage::updateCharacter(Character *character,
     catch (const dal::DbSqlQueryExecFailure& e)
     {
         // TODO: throw an exception.
-        if (startTransaction)
-        {
-            mDb->rollbackTransaction();
-        }
         LOG_ERROR("(DALStorage::updateCharacter #5) SQL query failure: " << e.what());
         return false;
     }
@@ -772,10 +750,6 @@ bool Storage::updateCharacter(Character *character,
     catch (const dal::DbSqlQueryExecFailure& e)
     {
         // TODO: throw an exception.
-        if (startTransaction)
-        {
-            mDb->rollbackTransaction();
-        }
         LOG_ERROR("(DALStorage::updateCharacter #5) SQL query failure: " << e.what());
         return false;
     }
@@ -824,10 +798,6 @@ bool Storage::updateCharacter(Character *character,
     catch (const dal::DbSqlQueryExecFailure& e)
     {
         // TODO: throw an exception.
-        if (startTransaction)
-        {
-            mDb->rollbackTransaction();
-        }
         LOG_ERROR("(DALStorage::updateCharacter #6) SQL query failure: " << e.what());
         return false;
     }
@@ -848,10 +818,6 @@ bool Storage::updateCharacter(Character *character,
     catch (const dal::DbSqlQueryExecFailure& e)
     {
         // TODO: throw an exception.
-        if (startTransaction)
-        {
-            mDb->rollbackTransaction();
-        }
         LOG_ERROR("(DALStorage::updateCharacter #7) SQL query failure: " << e.what());
         return false;
     }
@@ -867,17 +833,11 @@ bool Storage::updateCharacter(Character *character,
     catch (const dal::DbSqlQueryExecFailure& e)
     {
         // TODO: throw an exception
-        if (startTransaction)
-        {
-            mDb->rollbackTransaction();
-        }
         LOG_ERROR("(DALStorage::updateCharacter #8) SQL query failure: " << e.what());
         return false;
     }
-    if (startTransaction)
-    {
-        mDb->commitTransaction();
-    }
+
+    transaction.commit();
     return true;
 }
 
@@ -971,10 +931,7 @@ void Storage::flush(Account *account)
         {
             if ((*it)->getDatabaseID() >= 0)
             {
-                /* 2nd. parameter false means: don't start a transaction in
-                   the updateCharacter method, cause we did this already a few
-                   lines above */
-                updateCharacter(*it, false);
+                updateCharacter(*it);
             }
             else
             {
@@ -1061,7 +1018,7 @@ void Storage::flush(Account *account)
                 // Because as deleted, the RecordSet is also emptied
                 // That creates an error.
                 unsigned int charId = toUint(charInMemInfo(i, 1));
-                delCharacter(charId, false);
+                delCharacter(charId);
             }
         }
 
@@ -1685,15 +1642,12 @@ void Storage::banCharacter(int id, int duration)
  * Delete a character in the database.
  *
  * @param charId character identifier.
- * @param startTransaction indicates wheter the function should run in
- *        its own transaction or is called inline of another transaction
  */
-void Storage::delCharacter(int charId, bool startTransaction = true) const
+void Storage::delCharacter(int charId) const
 {
-    if (startTransaction)
-        mDb->beginTransaction();
     try
     {
+        dal::PerformTransaction transaction(mDb);
         std::ostringstream sql;
 
         // delete the inventory of the character
@@ -1743,13 +1697,10 @@ void Storage::delCharacter(int charId, bool startTransaction = true) const
             << " WHERE id = '" << charId << "';";
         mDb->execSql(sql.str());
 
-        if (startTransaction)
-            mDb->commitTransaction();
+        transaction.commit();
     }
     catch (const dal::DbSqlQueryExecFailure &e)
     {
-        if (startTransaction)
-            mDb->rollbackTransaction();
         LOG_ERROR("(DALStorage::delCharacter) SQL query failure: " << e.what());
     }
 }
@@ -1759,13 +1710,10 @@ void Storage::delCharacter(int charId, bool startTransaction = true) const
  * by this function!
  *
  * @param character character object.
- * @param startTransaction indicates wheter the function should run in
- *        its own transaction or is called inline of another transaction
  */
-void Storage::delCharacter(Character *character,
-                           bool startTransaction = true) const
+void Storage::delCharacter(Character *character) const
 {
-    delCharacter(character->getDatabaseID(), startTransaction);
+    delCharacter(character->getDatabaseID());
 }
 
 /**
diff --git a/src/account-server/storage.hpp b/src/account-server/storage.hpp
index 8651d20..27d1299 100644
--- a/src/account-server/storage.hpp
+++ b/src/account-server/storage.hpp
@@ -74,8 +74,8 @@ class Storage
 
         void banCharacter(int id, int duration);
 
-        void delCharacter(int charId, bool startTransaction) const;
-        void delCharacter(Character *character, bool startTransaction) const;
+        void delCharacter(int charId) const;
+        void delCharacter(Character *character) const;
 
         void checkBannedAccounts();
 
@@ -83,8 +83,7 @@ class Storage
         bool doesEmailAddressExist(const std::string &email);
         bool doesCharacterNameExist(const std::string &name);
 
-        bool updateCharacter(Character *ptr,
-                             bool startTransaction = true);
+        bool updateCharacter(Character *ptr);
 
         void flushSkill(const Character *character, int skill_id);
 
diff --git a/src/dal/dataprovider.cpp b/src/dal/dataprovider.cpp
index f6bcbdf..b0f05d3 100644
--- a/src/dal/dataprovider.cpp
+++ b/src/dal/dataprovider.cpp
@@ -27,21 +27,28 @@ namespace dal
 
 PerformTransaction::PerformTransaction(DataProvider *dataProvider)
     : mDataProvider(dataProvider)
+    , mTransactionStarted(false)
     , mCommitted(false)
 {
-    mDataProvider->beginTransaction();
+    if (!mDataProvider->inTransaction()) {
+        mDataProvider->beginTransaction();
+        mTransactionStarted = true;
+    }
 }
 
 PerformTransaction::~PerformTransaction()
 {
-    if (!mCommitted)
+    if (mTransactionStarted && !mCommitted)
         mDataProvider->rollbackTransaction();
 }
 
 void PerformTransaction::commit()
 {
-    mDataProvider->commitTransaction();
-    mCommitted = true;
+    if (mTransactionStarted) {
+        mDataProvider->commitTransaction();
+        mCommitted = true;
+        mTransactionStarted = false;
+    }
 }
 
 
diff --git a/src/dal/dataprovider.h b/src/dal/dataprovider.h
index 1764ef1..83d1c8c 100644
--- a/src/dal/dataprovider.h
+++ b/src/dal/dataprovider.h
@@ -59,6 +59,7 @@ public:
 
 private:
     DataProvider *mDataProvider;
+    bool mTransactionStarted;
     bool mCommitted;
 };
 
@@ -156,6 +157,11 @@ class DataProvider
             throw (std::runtime_error) = 0;
 
         /**
+         * Returns whether the data provider is currently in a transaction.
+         */
+        virtual bool inTransaction() const = 0;
+
+        /**
          * Returns the number of changed rows by the last executed SQL
          * statement.
          *
diff --git a/src/dal/mysqldataprovider.cpp b/src/dal/mysqldataprovider.cpp
index 3e3d1c6..4e5f286 100644
--- a/src/dal/mysqldataprovider.cpp
+++ b/src/dal/mysqldataprovider.cpp
@@ -43,6 +43,7 @@ const std::string  MySqlDataProvider::CFGPARAM_MYSQL_PWD_DEF  = "mana";
 MySqlDataProvider::MySqlDataProvider()
     throw()
         : mDb(0)
+        , mInTransaction(false)
 {
 }
 
@@ -229,7 +230,21 @@ void MySqlDataProvider::beginTransaction()
         throw std::runtime_error(error);
     }
 
-    mysql_autocommit(mDb, AUTOCOMMIT_OFF);
+    if (inTransaction())
+    {
+        const std::string error = "Trying to begin a transaction while anoter "
+            "one is still open!";
+        LOG_ERROR(error);
+        throw std::runtime_error(error);
+    }
+
+    if (mysql_autocommit(mDb, AUTOCOMMIT_OFF)) {
+        const std::string error = "Error while trying to disable autocommit";
+        LOG_ERROR(error);
+        throw std::runtime_error(error);
+    }
+
+    mInTransaction = true;
     execSql("BEGIN");
     LOG_DEBUG("SQL: started transaction");
 }
@@ -245,12 +260,27 @@ void MySqlDataProvider::commitTransaction()
         throw std::runtime_error(error);
     }
 
+    if (!inTransaction())
+    {
+        const std::string error = "Trying to commit a transaction while no "
+            "one is open!";
+        LOG_ERROR(error);
+        throw std::runtime_error(error);
+    }
+
     if (mysql_commit(mDb) != 0)
     {
         LOG_ERROR("MySqlDataProvider::commitTransaction: " << mysql_error(mDb));
         throw DbSqlQueryExecFailure(mysql_error(mDb));
     }
-    mysql_autocommit(mDb, AUTOCOMMIT_ON);
+
+    if (mysql_autocommit(mDb, AUTOCOMMIT_ON)) {
+        const std::string error = "Error while trying to enable autocommit";
+        LOG_ERROR(error);
+        throw std::runtime_error(error);
+    }
+
+    mInTransaction = false;
     LOG_DEBUG("SQL: commited transaction");
 }
 
@@ -265,15 +295,29 @@ void MySqlDataProvider::rollbackTransaction()
         throw std::runtime_error(error);
     }
 
+    if (!inTransaction())
+    {
+        const std::string error = "Trying to rollback a transaction while no "
+            "one is open!";
+        LOG_ERROR(error);
+        throw std::runtime_error(error);
+    }
+
     if (mysql_rollback(mDb) != 0)
     {
         LOG_ERROR("MySqlDataProvider::rollbackTransaction: " << mysql_error(mDb));
         throw DbSqlQueryExecFailure(mysql_error(mDb));
     }
     mysql_autocommit(mDb, AUTOCOMMIT_ON);
+    mInTransaction = false;
     LOG_DEBUG("SQL: transaction rolled back");
 }
 
+bool MySqlDataProvider::inTransaction() const
+{
+    return mInTransaction;
+}
+
 unsigned MySqlDataProvider::getModifiedRows() const
 {
     if (!mIsConnected)
diff --git a/src/dal/mysqldataprovider.h b/src/dal/mysqldataprovider.h
index c553032..528d4be 100644
--- a/src/dal/mysqldataprovider.h
+++ b/src/dal/mysqldataprovider.h
@@ -128,6 +128,8 @@ class MySqlDataProvider: public DataProvider
         void rollbackTransaction()
             throw (std::runtime_error);
 
+        bool inTransaction() const;
+
         /**
          * Returns the number of changed rows by the last executed SQL
          * statement.
@@ -197,6 +199,7 @@ class MySqlDataProvider: public DataProvider
         MYSQL *mDb; /**< the handle to the database connection */
         MYSQL_STMT *mStmt; /**< the prepared statement to process */
         std::vector<MYSQL_BIND*> mBind;
+        bool mInTransaction;
 };
 
 
diff --git a/src/dal/sqlitedataprovider.h b/src/dal/sqlitedataprovider.h
index bbe4b87..40ef361 100644
--- a/src/dal/sqlitedataprovider.h
+++ b/src/dal/sqlitedataprovider.h
@@ -121,6 +121,14 @@ class SqLiteDataProvider: public DataProvider
             throw (std::runtime_error);
 
         /**
+         * Returns wheter the connection has a open transaction or is in auto-
+         * commit mode.
+         *
+         * @return true, if a transaction is open.
+         */
+        bool inTransaction() const;
+
+        /**
          * Returns the number of changed rows by the last executed SQL
          * statement.
          *
@@ -169,14 +177,6 @@ class SqLiteDataProvider: public DataProvider
         /** defines the default value of the CFGPARAM_SQLITE_DB parameter */
         static const std::string CFGPARAM_SQLITE_DB_DEF;
 
-        /**
-         * Returns wheter the connection has a open transaction or is in auto-
-         * commit mode.
-         *
-         * @return true, if a transaction is open.
-         */
-        bool inTransaction() const;
-
         sqlite3 *mDb; /**< the handle to the database connection */
         sqlite3_stmt *mStmt; /**< the prepared statement to process */
 };