[java-plugin-storage-jdbc] branch main updated: JJDBC-30 Incomplete SQL transaction in create()

[Rod Widdowson]

This is an automated email from the git hooks/post-receive script.

rdw pushed a commit to branch main
in repository java-plugin-storage-jdbc.

View the commit online:
http://git.shibboleth.net/view/?p=java-plugin-storage-jdbc.git;a=commit;h=4a680925097aa3014040eb7d8f7d37cbb934160f

The following commit(s) were added to refs/heads/main by this push:
     new 4a68092  JJDBC-30 Incomplete SQL transaction in create()
4a68092 is described below

commit 4a680925097aa3014040eb7d8f7d37cbb934160f
Author: Rod Widdowson <rdw at steadingsoftware.com>
AuthorDate: Tue Sep 24 20:17:04 2024 +0100

    JJDBC-30 Incomplete SQL transaction in create()
    
    https://shibboleth.atlassian.net/browse/JJDBC-30
    
    Rework last change
     * Do not call commit or rollback on auito commit operations
     * Add asserts (i.e. only tested if explicitly turned on) to enforce exactly one of rollback/commit/autoCommit
---
 .../storage/jdbc/impl/JDBCStorageService.java      | 26 ++++++++++++++--------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/jdbc-storage-impl/src/main/java/net/shibboleth/plugin/storage/jdbc/impl/JDBCStorageService.java b/jdbc-storage-impl/src/main/java/net/shibboleth/plugin/storage/jdbc/impl/JDBCStorageService.java
index 35fd5ee..d0b5ff1 100644
--- a/jdbc-storage-impl/src/main/java/net/shibboleth/plugin/storage/jdbc/impl/JDBCStorageService.java
+++ b/jdbc-storage-impl/src/main/java/net/shibboleth/plugin/storage/jdbc/impl/JDBCStorageService.java
@@ -484,7 +484,6 @@ public final class JDBCStorageService extends AbstractStorageService
                     result.add(context);
                 }
             }
-            connection.rollback();
             return result;
         } catch (final SQLException e) {
             log.error("ReadContexts failed", e);
@@ -515,7 +514,6 @@ public final class JDBCStorageService extends AbstractStorageService
                           context, id, value, version, expires == null ? "<never>": expires);
                 result.add(new JDBCStorageRecord<>(value, expires, version));
             }
-            connection.rollback();
             return result;            
         } catch (final SQLException e) {
             log.error("ReadAll failed", e);
@@ -551,7 +549,6 @@ public final class JDBCStorageService extends AbstractStorageService
                     result.add(new JDBCStorageRecord<>(value, expires, version));
                 }
             }
-            connection.rollback();
             return result;
             
         } catch (final SQLException e) {
@@ -683,7 +680,6 @@ public final class JDBCStorageService extends AbstractStorageService
                 try (final ResultSet resultSet = stmnt.executeQuery()) {
                     if (!resultSet.next()) {
                         log.debug("Nothing returned");
-                        connection.rollback();
                         return new Pair<>();
                     }
                     final Long returnedVersion = resultSet.getLong(1);
@@ -694,7 +690,6 @@ public final class JDBCStorageService extends AbstractStorageService
                               returnedVersion, returnedExpires, returnedValue);
                     if (returnedExpires != null && System.currentTimeMillis() >= returnedExpires) {
                         log.debug("Read failed, key '{}' expired in context '{}'", key, context);
-                        connection.rollback();
                         return new Pair<>();
                     }
                     if (version != null && version.equals(returnedVersion)) {
@@ -706,7 +701,6 @@ public final class JDBCStorageService extends AbstractStorageService
                     }
                     final MutableStorageRecord<T> result =
                         new JDBCStorageRecord<>(returnedValue, returnedExpires, returnedVersion);
-                    connection.rollback();
                     return new Pair<>(version, result);
                 }
             } catch (final SQLException e) {
@@ -1085,7 +1079,6 @@ public final class JDBCStorageService extends AbstractStorageService
                 log.trace("UpdateContextExpiration:: '{}': 1: '{}'", deleteByContextSQL, context);
 
                 updateStmnt.execute();
-                connection.commit();
                 return;
             } catch (final SQLException e) {
                 boolean retry = false;
@@ -1140,7 +1133,6 @@ public final class JDBCStorageService extends AbstractStorageService
                     result.add(key);
                 }
             }
-            connection.rollback();
             return result;
             
         } catch (final SQLException e) {
@@ -1251,7 +1243,17 @@ public final class JDBCStorageService extends AbstractStorageService
 
         /** The lock we may or may not have set up. */
         @Nullable private  final Lock threadLock;
-
+        
+        
+        /** Was this created autocommit? */
+        private final boolean isAutoCommit;
+        
+        /** Has {@link #commit()} been called? */
+        private boolean isCommited;
+        
+        /** Has {@link #rollback()} been called? */
+        private boolean isRolledBack;
+        
         /** Constructor.
          * @param autoCommit What to set {@link Connection#setAutoCommit(boolean)} to
          * @param writeLock Whether to grab an write lock on the table (if we are locking)
@@ -1261,6 +1263,7 @@ public final class JDBCStorageService extends AbstractStorageService
             final Connection con = dataSource.getConnection();
             assert con != null;
             connection = con;
+            isAutoCommit = autoCommit;
             connection.setAutoCommit(autoCommit);
             if (transactionIsolation > Connection.TRANSACTION_NONE) {
                 connection.setTransactionIsolation(transactionIsolation);
@@ -1293,6 +1296,8 @@ public final class JDBCStorageService extends AbstractStorageService
          * @throws SQLException if encapsulated {@link Connection} does
          */
         public void commit() throws SQLException {
+            assert !isAutoCommit && !isCommited && !isRolledBack;
+            isCommited = true;
             connection.commit();
         }
 
@@ -1300,12 +1305,15 @@ public final class JDBCStorageService extends AbstractStorageService
          * @throws SQLException if encapsulated {@link Connection} does
          */
         public void rollback() throws SQLException {
+            assert !isAutoCommit && !isCommited && !isRolledBack;
+            isRolledBack = true;
             connection.rollback();
         }
 
 
         @Override
         public void close()  {
+            assert isAutoCommit || isCommited || isRolledBack;
             try {
                 connection.close();
             } catch (final SQLException e) {

-- 
To stop receiving notification emails like this one, please contact
the administrator of this repository.