Merge branch 'fix-rows-affected-bulk'

Fix result of get_affected_rows() after bulk operations.

See #1199.

CHANGES

@@ -27,6 +27,7 @@ Changes affecting all or multiple backends:
  - Dynamic backends loading improvements (#1169).
  - Fix several problems detected by UBSAN and use it in the CI builds too now.
  - Improvements to connection string handling (#1176).
+ - Fix get_affected_rows() after partial updates for ODBC and SQLite (#1199).
 
 Backend-specific changes:
 

docs/beyond.md

@@ -19,11 +19,6 @@ if ( !st.get_affected_rows() )
 }
 ```
 
-### Portability note
-
-This method behaviour in case of partially executed update, i.e. when some records were updated or inserted while some other have failed to be updated or inserted, depends on the exact backend and, in the case of ODBC backend, on the exact ODBC driver used.
-It can return `-1`, meaning that the number of rows is unknown, the number of rows actually updated or the total number of affected rows.
-
 ## Sequences
 
 It is common to have auto-incrementing database fields or fields whose value come from a sequence.

src/backends/odbc/statement.cpp

@@ -141,53 +141,91 @@ void odbc_statement_backend::prepare(std::string const & query,
 statement_backend::exec_fetch_result
 odbc_statement_backend::execute(int number)
 {
-    // Store the number of rows processed by this call.
+    // Store the number of rows processed by this call and the operation result
+    // for each of them.
     SQLULEN rows_processed = 0;
+    std::vector<SQLUSMALLINT> status;
     if (hasVectorUseElements_)
     {
         SQLSetStmtAttr(hstmt_, SQL_ATTR_PARAMS_PROCESSED_PTR, &rows_processed, 0);
+
+        status.resize(number);
+        SQLSetStmtAttr(hstmt_, SQL_ATTR_PARAM_STATUS_PTR, &status[0], 0);
     }
 
     // if we are called twice for the same statement we need to close the open
     // cursor or an "invalid cursor state" error will occur on execute
     SQLCloseCursor(hstmt_);
 
     SQLRETURN rc = SQLExecute(hstmt_);
-    if (is_odbc_error(rc))
+
+    // Don't use is_odbc_error() here, as SQL_SUCCESS_WITH_INFO indicates an
+    // error if it corresponds to a partial update.
+    if (rc != SQL_SUCCESS && rc != SQL_NO_DATA)
     {
         // Construct the error object immediately, before calling any other
         // ODBC functions, in order to not lose the error message.
         const odbc_soci_error err(SQL_HANDLE_STMT, hstmt_, "executing statement");
 
-        // There is no universal way to determine the number of affected rows
-        // after a failed update.
-        rowsAffected_ = -1LL;
-
-        // If executing bulk operation a partial
-        // number of rows affected may be available.
-        if (hasVectorUseElements_)
+        bool error = true;
+        if (rc == SQL_SUCCESS_WITH_INFO)
         {
-            do
+            // Check for partial update when using array parameters.
+            if (hasVectorUseElements_)
             {
-                SQLLEN res = 0;
-                // SQLRowCount will return error after a partially executed statement.
-                // SQL_DIAG_ROW_COUNT returns the same info but must be collected immediatelly after the execution.
-                rc = SQLGetDiagField(SQL_HANDLE_STMT, hstmt_, 0, SQL_DIAG_ROW_COUNT, &res, 0, NULL);
-                if (!is_odbc_error(rc) && res != -1)
+                rowsAffected_ = 0;
+
+                SQLLEN firstErrorRow = -1;
+                for (SQLULEN i = 0; i < rows_processed; ++i)
                 {
-                  if (rowsAffected_ == -1LL)
-                    rowsAffected_ = res;
-                  else
-                    rowsAffected_ += res;
+                    switch (status[i])
+                    {
+                        case SQL_PARAM_SUCCESS:
+                        case SQL_PARAM_SUCCESS_WITH_INFO:
+                            ++rowsAffected_;
+                            break;
+
+                        case SQL_PARAM_ERROR:
+                            if (firstErrorRow == -1)
+                                firstErrorRow = i;
+                            break;
+
+                        case SQL_PARAM_UNUSED:
+                        case SQL_PARAM_DIAG_UNAVAILABLE:
+                            // We shouldn't get those, normally, but just
+                            // ignore them if we do.
+                            break;
+                    }
                 }
-                --rows_processed; // Avoid unnecessary calls to SQLGetDiagField
+
+                // In principle, it is possible to get success with info for an
+                // operation which succeeded for all rows -- even though this
+                // hasn't been observed so far. In this case, we shouldn't
+                // throw an error.
+                if (firstErrorRow == -1)
+                    error = false;
             }
-            // Move forward to the next result while there are rows processed.
-            while (rows_processed > 0 && SQLMoreResults(hstmt_) == SQL_SUCCESS);
+            else
+            {
+                // This is a weird case which has never been observed so far
+                // and it's not clear what it might correspond to, but don't
+                // handle it as an error to avoid throwing spurious exceptions
+                // when there is no real problem.
+                error = false;
+            }
+        }
+        else
+        {
+            // If the statement failed completely, no rows should have been
+            // affected.
+            rowsAffected_ = 0;
         }
-        throw err;
+
+        if (error)
+            throw err;
     }
-    else if (hasVectorUseElements_)
+
+    if (hasVectorUseElements_)
     {
         // We already have the number of rows, no need to do anything.
         rowsAffected_ = rows_processed;

src/backends/sqlite3/statement.cpp

@@ -265,9 +265,7 @@ sqlite3_statement_backend::bind_and_execute(int number)
 {
     statement_backend::exec_fetch_result retVal = ef_no_data;
 
-    long long rowsAffectedBulkTemp = 0;
-
-    rowsAffectedBulk_ = -1;
+    rowsAffectedBulk_ = 0;
 
     int const rows = static_cast<int>(useData_.size());
     for (int row = 0; row < rows; ++row)
@@ -341,8 +339,6 @@ sqlite3_statement_backend::bind_and_execute(int number)
 
             if (SQLITE_OK != bindRes)
             {
-                // preserve the number of rows affected so far.
-                rowsAffectedBulk_ = rowsAffectedBulkTemp;
                 throw sqlite3_soci_error("Failure to bind on bulk operations", bindRes);
             }
         }
@@ -356,10 +352,9 @@ sqlite3_statement_backend::bind_and_execute(int number)
 
         databaseReady_=true; // Mark sqlite engine is ready to perform sqlite3_step
         retVal = load_one(); // execute each bound line
-        rowsAffectedBulkTemp += get_affected_rows();
+        rowsAffectedBulk_ += sqlite3_changes(session_.conn_);
     }
 
-    rowsAffectedBulk_ = rowsAffectedBulkTemp;
     return retVal;
 }
 

tests/common/test-common.cpp

@@ -3256,29 +3256,23 @@ TEST_CASE_METHOD(common_tests, "Get affected rows", "[core][affected-rows]")
 
     CHECK(st5.get_affected_rows() == 5);
 
+    if (tc_.has_partial_update_bug())
+    {
+        WARN("Skipping partial update test due to a known backend bug");
+        return;
+    }
+
     std::vector<std::string> w(2, "1");
     w[1] = "a"; // this invalid value may cause an exception.
     statement st6 = (sql.prepare <<
         "insert into soci_test(val) values(:val)", use(w));
-    try { st6.execute(true); }
-    catch(...) {}
+    CHECK_THROWS_AS(st6.execute(true), soci_error);
+    CHECK(st6.get_affected_rows() == 1);
 
     // confirm the partial insertion.
     int val = 0;
     sql << "select count(val) from soci_test", into(val);
-    if(val != 0)
-    {
-        // Notice that some ODBC drivers don't return the number of updated
-        // rows at all in the case of partially executed statement like this
-        // one, while MySQL ODBC driver wrongly returns 2 affected rows even
-        // though only one was actually inserted.
-        //
-        // So we can't check for "get_affected_rows() == val" here, it would
-        // fail in too many cases -- just check that the backend doesn't lie to
-        // us about no rows being affected at all (even if it just honestly
-        // admits that it has no idea by returning -1).
-        CHECK(st6.get_affected_rows() != 0);
-    }
+    CHECK(val == 1);
 }
 
 // test fix for: Backend is not set properly with connection pool (pull #5)

tests/odbc/test-odbc-postgresql.cpp

@@ -225,6 +225,21 @@ class test_context : public test_context_common
         return !m_verDriver.is_initialized() || m_verDriver < odbc_version(9, 3, 400);
     }
 
+    bool has_partial_update_bug() const override
+    {
+        // ODBC driver has version-dependent bugs related to handling array
+        // parameters: after v13.02, it fails to insert anything, see
+        // https://github.com/postgresql-interfaces/psqlodbc/issues/89, and
+        // with the previous versions (e.g. v10.03) it's even worse, as it does
+        // insert the row with valid values but still returns fatal error at
+        // ODBC level.
+        //
+        // So far there is no known version where it works correctly, but if
+        // the issue above is fixed, we should check for the version including
+        // this fix here.
+        return true;
+    }
+
     std::string fix_crlf_if_necessary(std::string const& s) const override
     {
         // Version 9.03.0300 (ancient, but still used on AppVeyor CI) is known

tests/sqlite3/test-sqlite3.cpp

@@ -913,7 +913,13 @@ struct table_creator_for_get_affected_rows : table_creator_base
     table_creator_for_get_affected_rows(soci::session & sql)
         : table_creator_base(sql)
     {
-        sql << "create table soci_test(val integer)";
+        // The CHECK clause is needed to make SQLite refuse inserting "a" into
+        // this column: the test using this table relies on this to fail and
+        // this condition ensures it does.
+        //
+        // Note that more straightforward checks, like typeof(val) = 'integer',
+        // don't work with old SQLite version, such as 3.12 used on AppVeyor.
+        sql << R"(create table soci_test(val integer check (val < 100)))";
     }
 };
 

tests/test-context.h

@@ -193,6 +193,10 @@ class test_context_base
     // *exactly* the same value.
     virtual bool has_fp_bug() const { return false; }
 
+    // Override this if the backend doesn't handle partial success for
+    // operations using array parameters correctly.
+    virtual bool has_partial_update_bug() const { return false; }
+
     // Override this if the backend wrongly returns CR LF when reading a string
     // with just LFs from the database to strip the unwanted CRs.
     virtual std::string fix_crlf_if_necessary(std::string const& s) const { return s; }