[irods#7599,irods#7440] Rework logic in _cllExecSqlNoResult to return…

… proper error codes

plugins/database/src/low_level_odbc.cpp

@@ -86,6 +86,8 @@ SQLINTEGER columnLength[MAX_TOKEN];  /* change me ! */
 #ifndef ORA_ICAT
 static int didBegin = 0;
 #endif
+// Stores the affected row count of queries made through cllExecSqlNoResult
+// Read by cllGetRowCount
 static int noResultRowCount = 0;
 
 // =-=-=-=-=-=-=-
@@ -463,6 +465,7 @@ _cllExecSqlNoResult(
     HDBC myHdbc = icss->connectPtr;
     HSTMT myHstmt;
     SQLRETURN stat = SQLAllocHandle( SQL_HANDLE_STMT, myHdbc, &myHstmt );
+    SQL_INT_OR_LEN rowCount = 0;
     if ( stat != SQL_SUCCESS ) {
         rodsLog( LOG_ERROR, "_cllExecSqlNoResult: SQLAllocHandle failed for statement: %d", stat );
         return -1;
@@ -475,9 +478,6 @@ _cllExecSqlNoResult(
     rodsLogSql( sql );
 
     stat = SQLExecDirect( myHstmt, ( unsigned char * )sql, strlen( sql ) );
-    SQL_INT_OR_LEN rowCount = 0;
-    auto ec = SQLRowCount(myHstmt, (SQL_INT_OR_LEN*) &rowCount); // NOLINT(google-readability-casting)
-    IRODS_DB_TRACE("SQLRowCount returned [{}] and set [rowCount] to [{}].", ec, rowCount);
     switch ( stat ) {
     case SQL_SUCCESS:
         rodsLogSqlResult( "SUCCESS" );
@@ -502,6 +502,9 @@ _cllExecSqlNoResult(
     if ( stat == SQL_SUCCESS ||
             stat == SQL_SUCCESS_WITH_INFO ||
             stat == SQL_NO_DATA_FOUND ) {
+        const auto ec = SQLRowCount(myHstmt, (SQL_INT_OR_LEN*) &rowCount); // NOLINT(google-readability-casting)
+        IRODS_DB_TRACE("SQLRowCount returned [{}] and set [rowCount] to [{}].", ec, rowCount);
+
         cllCheckPending( sql, 0, icss->databaseType );
         result = 0;
         if ( stat == SQL_NO_DATA_FOUND ) {
@@ -534,15 +537,25 @@ _cllExecSqlNoResult(
         }
         rodsLog( LOG_NOTICE, "_cllExecSqlNoResult: SQLExecDirect error: %d sql:%s",
                  stat, sql );
+
+        // logPsgError returns an iRODS-specific error code for duplicate entries in the catalog
+        // Returns -2 otherwise.
         result = logPsgError( LOG_NOTICE, icss->environPtr, myHdbc, myHstmt,
                               icss->databaseType );
+        // On error cases, do not call SQLRowCount: it will cause a Function sequence error
+        // However, calling functions may read the return value of SQLRowCount via
+        // static variable noResultRowCount, which is set below.
+        // Thus, since SQLRowCount returns -1 on error cases, we emulate the result
+        // without calling SQLRowCount directly.
+        rowCount = -1;
     }
 
     stat = SQLFreeHandle( SQL_HANDLE_STMT, myHstmt );
     if ( stat != SQL_SUCCESS ) {
         rodsLog( LOG_ERROR, "_cllExecSqlNoResult: SQLFreeHandle for statement error: %d", stat );
     }
 
+    // Saves the affected row count to a static variable
     noResultRowCount = rowCount;
 
     return result;

scripts/irods/test/test_iadmin.py

@@ -1669,6 +1669,22 @@ def test_non_admins_are_not_allowed_to_invoke_iadmin_set_delay_server__issue_524
     def test_non_admins_are_not_allowed_to_invoke_iadmin_get_delay_server_info__issue_5249(self):
         self.user0.assert_icommand(['iadmin', 'get_delay_server_info'], 'STDERR', ['-830000 CAT_INSUFFICIENT_PRIVILEGE_LEVEL'])
 
+    def test_iadmin_mkuser_returns_correct_error_on_duplicate_user__issue_7599(self):
+        test_user_name = 'duplicate_user_issue_7599'
+        try:
+            self.admin.assert_icommand(['iadmin', 'mkuser', test_user_name, 'rodsuser'])
+            self.admin.assert_icommand(['iadmin', 'mkuser', test_user_name, 'rodsuser'], 'STDERR', ['-809000 CATALOG_ALREADY_HAS_ITEM_BY_THAT_NAME'])
+        finally:
+            self.admin.run_icommand(['iadmin', 'rmuser', test_user_name])
+
+    def test_iadmin_mkgroup_returns_correct_error_on_duplicate_group__issue_7599(self):
+        test_group_name = 'duplicate_group_issue_7599'
+        try:
+            self.admin.assert_icommand(['iadmin', 'mkgroup', test_group_name])
+            self.admin.assert_icommand(['iadmin', 'mkgroup', test_group_name], 'STDERR', ['-809000 CATALOG_ALREADY_HAS_ITEM_BY_THAT_NAME'])
+        finally:
+            self.admin.run_icommand(['iadmin', 'rmgroup', test_group_name])
+
 class Test_Iadmin_Resources(resource_suite.ResourceBase, unittest.TestCase):
 
     def setUp(self):

scripts/irods/test/test_imeta_error_handling.py

@@ -71,10 +71,7 @@ def test_imeta_addw_return_code(self):
         self.admin.assert_icommand(['imeta', 'addw', '-d', self.test_data_paths_wildcard, attr, val], 'STDOUT', desired_rc=0)
         _, err, ec = self.admin.run_icommand(['imeta', 'addw', '-d', self.test_data_paths_wildcard, attr, val])
         self.assertEqual(4, ec)
-        # In unixODBC versions > 2.3.5, the database plugin does not return CATALOG_ALREADY_HAS_ITEM_BY_THAT_NAME
-        # but instead it returns CAT_SQL_ERR. We support platforms that use both 2.3.4 and 2.3.6, so either error
-        # code is acceptable for this test.
-        self.assertTrue('CATALOG_ALREADY_HAS_ITEM_BY_THAT_NAME' in err or 'CAT_SQL_ERR' in err)
+        self.assertIn('CATALOG_ALREADY_HAS_ITEM_BY_THAT_NAME', err)
 
     def test_imeta_add_return_code(self):
         data_path = f'{self.test_data_path_base}0'
@@ -83,10 +80,7 @@ def test_imeta_add_return_code(self):
         self.admin.assert_icommand(['imeta', 'add', '-d', data_path, attr, val], 'STDOUT', desired_rc=0)
         _, err, ec = self.admin.run_icommand(['imeta', 'add', '-d', data_path, attr, val])
         self.assertEqual(4, ec)
-        # In unixODBC versions > 2.3.5, the database plugin does not return CATALOG_ALREADY_HAS_ITEM_BY_THAT_NAME
-        # but instead it returns CAT_SQL_ERR. We support platforms that use both 2.3.4 and 2.3.6, so either error
-        # code is acceptable for this test.
-        self.assertTrue('CATALOG_ALREADY_HAS_ITEM_BY_THAT_NAME' in err or 'CAT_SQL_ERR' in err)
+        self.assertIn('CATALOG_ALREADY_HAS_ITEM_BY_THAT_NAME', err)
 
     def test_imeta_addw_stderr_5184(self):
         self.admin.assert_icommand(['imeta', 'addw', '-d', self.test_data_paths_wildcard, '5184_attr', '5184_attr'], 'STDOUT')