Simplified the use of binding when using MySQL.

This permits to avoid a memleak with the former vector form
and to use the 'place' variable when binding.
The badly prepared statements are also a bit better handled now.

With this patch, IMHO, the MySQL support is in shape.

Reviewed-by: Jaxad0127.

2 files changed, 82 insertions, 47 deletions

diff --git a/src/dal/mysqldataprovider.cpp b/src/dal/mysqldataprovider.cpp
index d0c488a..d297c95 100644
--- a/src/dal/mysqldataprovider.cpp
+++ b/src/dal/mysqldataprovider.cpp
@@ -42,8 +42,10 @@ const std::string  MySqlDataProvider::CFGPARAM_MYSQL_PWD_DEF  = "mana";
  */
 MySqlDataProvider::MySqlDataProvider()
     throw()
-        : mDb(0)
-        , mInTransaction(false)
+        : mDb(0),
+          mStmt(0),
+          mBind(0),
+          mInTransaction(false)
 {
 }
 
@@ -127,6 +129,7 @@ void MySqlDataProvider::connect()
     // Save the Db Name.
     mDbName = dbName;
 
+    // Initialize statement structure
     mStmt = mysql_stmt_init(mDb);
 
     mIsConnected = true;
@@ -207,6 +210,7 @@ void MySqlDataProvider::disconnect()
     // handle allocated by mysql_init().
     mysql_close(mDb);
 
+    // Clean possible statement.
     mysql_stmt_close(mStmt);
 
     // deinitialize the MySQL client library.
@@ -222,20 +226,21 @@ void MySqlDataProvider::beginTransaction()
     if (!mIsConnected)
     {
         const std::string error = "Trying to begin a transaction while not "
-            "connected to the database!";
+                                  "connected to the database!";
         LOG_ERROR(error);
         throw std::runtime_error(error);
     }
 
     if (inTransaction())
     {
-        const std::string error = "Trying to begin a transaction while anoter "
-            "one is still open!";
+        const std::string error = "Trying to begin a transaction while another "
+                                  "one is still open!";
         LOG_ERROR(error);
         throw std::runtime_error(error);
     }
 
-    if (mysql_autocommit(mDb, AUTOCOMMIT_OFF)) {
+    if (mysql_autocommit(mDb, AUTOCOMMIT_OFF))
+    {
         const std::string error = "Error while trying to disable autocommit";
         LOG_ERROR(error);
         throw std::runtime_error(error);
@@ -252,7 +257,7 @@ void MySqlDataProvider::commitTransaction()
     if (!mIsConnected)
     {
         const std::string error = "Trying to commit a transaction while not "
-            "connected to the database!";
+                                  "connected to the database!";
         LOG_ERROR(error);
         throw std::runtime_error(error);
     }
@@ -260,7 +265,7 @@ void MySqlDataProvider::commitTransaction()
     if (!inTransaction())
     {
         const std::string error = "Trying to commit a transaction while no "
-            "one is open!";
+                                  "one is open!";
         LOG_ERROR(error);
         throw std::runtime_error(error);
     }
@@ -271,7 +276,8 @@ void MySqlDataProvider::commitTransaction()
         throw DbSqlQueryExecFailure(mysql_error(mDb));
     }
 
-    if (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);
@@ -287,7 +293,7 @@ void MySqlDataProvider::rollbackTransaction()
     if (!mIsConnected)
     {
         const std::string error = "Trying to rollback a transaction while not "
-            "connected to the database!";
+                                  "connected to the database!";
         LOG_ERROR(error);
         throw std::runtime_error(error);
     }
@@ -295,16 +301,18 @@ void MySqlDataProvider::rollbackTransaction()
     if (!inTransaction())
     {
         const std::string error = "Trying to rollback a transaction while no "
-            "one is open!";
+                                  "one is open!";
         LOG_ERROR(error);
         throw std::runtime_error(error);
     }
 
     if (mysql_rollback(mDb) != 0)
     {
-        LOG_ERROR("MySqlDataProvider::rollbackTransaction: " << mysql_error(mDb));
+        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");
@@ -320,17 +328,16 @@ unsigned MySqlDataProvider::getModifiedRows() const
     if (!mIsConnected)
     {
         const std::string error = "Trying to getModifiedRows while not "
-            "connected to the database!";
+                                  "connected to the database!";
         LOG_ERROR(error);
         throw std::runtime_error(error);
     }
 
-    // FIXME: not sure if this is correct to bring 64bit int into int?
     const my_ulonglong affected = mysql_affected_rows(mDb);
 
     if (affected > INT_MAX)
         throw std::runtime_error(
-                  "MySqlDataProvider::getLastId exceeded INT_MAX");
+                               "MySqlDataProvider::getLastId exceeded INT_MAX");
 
     if (affected == (my_ulonglong)-1)
     {
@@ -350,10 +357,10 @@ unsigned MySqlDataProvider::getLastId() const
         throw std::runtime_error(error);
     }
 
-    // FIXME: not sure if this is correct to bring 64bit int into int?
     const my_ulonglong lastId = mysql_insert_id(mDb);
     if (lastId > UINT_MAX)
-        throw std::runtime_error("MySqlDataProvider::getLastId exceeded INT_MAX");
+        throw std::runtime_error(
+                              "MySqlDataProvider::getLastId exceeded UINT_MAX");
 
     return (unsigned) lastId;
 }
@@ -363,45 +370,48 @@ bool MySqlDataProvider::prepareSql(const std::string &sql)
     if (!mIsConnected)
         return false;
 
-    LOG_DEBUG("MySqlDataProvider::prepareSql Preparing SQL statement: "<<sql);
+    LOG_DEBUG("MySqlDataProvider::prepareSql Preparing SQL statement: " << sql);
 
-    mBind.clear();
+    if (mBind)
+    {
+        delete[] mBind;
+        mBind = 0;
+    }
 
     if (mysql_stmt_prepare(mStmt, sql.c_str(), sql.size()) != 0)
-    {
         return false;
-    }
+
+    // Allocate bind memory now that the prepared state is done.
+    mBind = new MYSQL_BIND[(int)mysql_stmt_param_count(mStmt)];
 
     return true;
 }
 
 const RecordSet &MySqlDataProvider::processSql()
 {
-    MYSQL_BIND* paramsBind;
-    unsigned int i;
-
-    if (!mIsConnected) {
+    if (!mIsConnected)
         throw std::runtime_error("not connected to database");
-    }
 
-    paramsBind = new MYSQL_BIND[mBind.size()];
-    for (i = 0; i < mBind.size(); ++i) {
-        paramsBind[i].buffer_type = mBind[i]->buffer_type;
-        paramsBind[i].buffer = mBind[i]->buffer;
-        paramsBind[i].buffer_length = mBind[i]->buffer_length;
-        paramsBind[i].is_null = 0;
-        paramsBind[i].length = mBind[i]->length;
+    // Since we'll have to return something in all cases,
+    // we clear the result member first.
+    mRecordSet.clear();
+
+    if (!mBind)
+    {
+        LOG_ERROR("MySqlDataProvider::processSql: "
+                  "No bind done before processing.");
+        return mRecordSet;
     }
 
-    if (mysql_stmt_bind_param(mStmt, paramsBind))
+    if (mysql_stmt_bind_param(mStmt, mBind))
     {
         LOG_ERROR("MySqlDataProvider::processSql Bind params failed: "
                   << mysql_stmt_error(mStmt));
+        return mRecordSet;
     }
 
     if (mysql_stmt_field_count(mStmt) > 0)
     {
-        mRecordSet.clear();
         MYSQL_BIND* resultBind;
         MYSQL_RES* res;
 
@@ -420,6 +430,7 @@ const RecordSet &MySqlDataProvider::processSql()
 
         resultBind = new MYSQL_BIND[mysql_num_fields(res)];
 
+        unsigned int i = 0;
         for (i = 0; i < mysql_num_fields(res); ++i)
         {
             resultBind[i].buffer_type = MYSQL_TYPE_STRING;
@@ -450,7 +461,7 @@ const RecordSet &MySqlDataProvider::processSql()
         {
             Row r;
 
-            for (unsigned int i = 0; i < nFields; ++i)
+            for (i = 0; i < nFields; ++i)
                 r.push_back(static_cast<char *>(resultBind[i].buffer));
 
             mRecordSet.add(r);
@@ -462,12 +473,12 @@ const RecordSet &MySqlDataProvider::processSql()
     {
         if (mysql_stmt_execute(mStmt))
         {
-            LOG_ERROR("MySqlDataProvider::processSql Execute failed: " << mysql_stmt_error(mStmt));
+            LOG_ERROR("MySqlDataProvider::processSql Execute failed: "
+                      << mysql_stmt_error(mStmt));
         }
     }
 
-    // free memory
-    delete[] paramsBind;
+    // Free memory
     mysql_stmt_free_result(mStmt);
 
     return mRecordSet;
@@ -482,9 +493,9 @@ void MySqlDataProvider::bindValue(int place, const std::string &value)
     bind->buffer= (void*) value.c_str();
     bind->buffer_length= value.size();
     bind->length = size;
+    bind->is_null = 0;
 
-    //FIXME : Isn't taking care of the place param
-    mBind.push_back(bind);
+    bindValue(place, bind);
 }
 
 void MySqlDataProvider::bindValue(int place, int value)
@@ -492,9 +503,21 @@ void MySqlDataProvider::bindValue(int place, int value)
     MYSQL_BIND* bind = new MYSQL_BIND;
     bind->buffer_type= MYSQL_TYPE_LONG;
     bind->buffer= &value;
+    bind->is_null = 0;
+
+    bindValue(place, bind);
+}
 
-    //FIXME : Isn't taking care of the place param
-    mBind.push_back(bind);
+void MySqlDataProvider::bindValue(int place, MYSQL_BIND* bind)
+{
+    if (!mBind)
+        LOG_ERROR("MySqlDataProvider::bindValue: "
+                  "Attempted to use an unprepared bind!");
+    else if (place > 0 && place <= (int)mysql_stmt_param_count(mStmt))
+        mBind[place - 1] = *bind;
+    else
+        LOG_ERROR("MySqlDataProvider::bindValue: "
+                  "Attempted bind index out of range");
 }
 
 } // namespace dal
diff --git a/src/dal/mysqldataprovider.h b/src/dal/mysqldataprovider.h
index 02fc8ea..4850a14 100644
--- a/src/dal/mysqldataprovider.h
+++ b/src/dal/mysqldataprovider.h
@@ -47,7 +47,8 @@ class MySqlDataProvider: public DataProvider
          * Replacement for mysql my_bool datatype used in mysql_autocommit()
          * function.
          */
-        enum {
+        enum
+        {
             AUTOCOMMIT_OFF = 0,
             AUTOCOMMIT_ON = 1
         };
@@ -173,6 +174,13 @@ class MySqlDataProvider: public DataProvider
         void bindValue(int place, int value);
 
     private:
+        /**
+         * Bind Value (MYSQL_BIND*)
+         * @param place - which parameter to bind to
+         * @param value - the MySQL bind structure itself
+         */
+        void bindValue(int place, MYSQL_BIND* bind);
+
         /** defines the name of the hostname config parameter */
         static const std::string CFGPARAM_MYSQL_HOST;
         /** defines the name of the server port config parameter */
@@ -196,9 +204,13 @@ class MySqlDataProvider: public DataProvider
         static const std::string CFGPARAM_MYSQL_PWD_DEF;
 
 
-        MYSQL *mDb; /**< the handle to the database connection */
-        MYSQL_STMT *mStmt; /**< the prepared statement to process */
-        std::vector<MYSQL_BIND*> mBind;
+        /** The handle to the database connection */
+        MYSQL *mDb;
+        /** The prepared statement to process */
+        MYSQL_STMT *mStmt;
+        /** The Bind structure used in prepared statement */
+        MYSQL_BIND* mBind;
+        /** Tells whether we're in the middle of a transaction */
         bool mInTransaction;
 };