code review

Author: sergiyvamz

wrapper/src/main/java/software/amazon/jdbc/exceptions/AbstractPgExceptionHandler.java

@@ -23,6 +23,9 @@
 import software.amazon.jdbc.util.StringUtils;
 
 public abstract class AbstractPgExceptionHandler implements ExceptionHandler {
+
+  protected static final String READ_ONLY_CONNECTION_SQLSTATE = "25006";
+
   public abstract List<String> getNetworkErrors();
 
   public abstract List<String> getAccessErrors();
@@ -99,7 +102,7 @@ public boolean isLoginException(final String sqlState) {
   @Override
   public boolean isReadOnlyConnectionException(
       final @Nullable String sqlState, final @Nullable Integer errorCode) {
-    return "25006".equals(sqlState);
+    return READ_ONLY_CONNECTION_SQLSTATE.equals(sqlState);
   }
 
   @Override

wrapper/src/main/java/software/amazon/jdbc/exceptions/MySQLExceptionHandler.java

@@ -17,6 +17,9 @@
 package software.amazon.jdbc.exceptions;
 
 import java.sql.SQLException;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
 import org.checkerframework.checker.nullness.qual.Nullable;
 import software.amazon.jdbc.targetdriverdialect.TargetDriverDialect;
 import software.amazon.jdbc.util.StringUtils;
@@ -27,6 +30,11 @@ public class MySQLExceptionHandler implements ExceptionHandler {
   public static final String SET_NETWORK_TIMEOUT_ON_CLOSED_CONNECTION =
       "setNetworkTimeout cannot be called on a closed connection";
 
+  private static final Set<Integer> SQLSTATE_READ_ONLY_CONNECTION = new HashSet<>(Arrays.asList(
+      1290, // The MySQL server is running with the --read-only option, so it cannot execute this statement
+      1836 // Running in read-only mode
+  ));
+
   @Override
   public boolean isNetworkException(final Throwable throwable, @Nullable TargetDriverDialect targetDriverDialect) {
     Throwable exception = throwable;
@@ -106,7 +114,8 @@ public boolean isLoginException(final String sqlState) {
   @Override
   public boolean isReadOnlyConnectionException(
       final @Nullable String sqlState, final @Nullable Integer errorCode) {
-    return "HY000".equals(sqlState) && errorCode != null && (errorCode == 1290 || errorCode == 1836);
+    // HY000 - generic SQL state; use error code for more specific information
+    return "HY000".equals(sqlState) && errorCode != null && (SQLSTATE_READ_ONLY_CONNECTION.contains(errorCode));
   }
 
   @Override

wrapper/src/main/java/software/amazon/jdbc/plugin/failover/FailoverConnectionPlugin.java

@@ -894,6 +894,8 @@ protected boolean shouldExceptionTriggerConnectionSwitch(final Throwable t) {
       return true;
     }
 
+    // For STRICT_WRITER failover mode when connection exception indicate that the connection's in read-only mode,
+    // initiate a failover by returning true.
     return this.failoverMode == FailoverMode.STRICT_WRITER
       && this.pluginService.isReadOnlyConnectionException(t, this.pluginService.getTargetDriverDialect());
   }

wrapper/src/main/java/software/amazon/jdbc/plugin/failover2/FailoverConnectionPlugin.java

@@ -702,6 +702,8 @@ protected boolean shouldExceptionTriggerConnectionSwitch(final Throwable t) {
       return true;
     }
 
+    // For STRICT_WRITER failover mode when connection exception indicate that the connection's in read-only mode,
+    // initiate a failover by returning true.
     return this.failoverMode == FailoverMode.STRICT_WRITER
       && this.pluginService.isReadOnlyConnectionException(t, this.pluginService.getTargetDriverDialect());
   }

wrapper/src/test/java/software/amazon/jdbc/plugin/failover/FailoverConnectionPluginTest.java

@@ -18,12 +18,15 @@
 
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.spy;
@@ -441,4 +444,17 @@ private void initializePlugin() throws SQLException {
     spyPlugin.setReaderFailoverHandler(mockReaderFailoverHandler);
     // doReturn(mockConnectionService).when(spyPlugin).getConnectionService();
   }
+
+  @Test
+  void test_failover_when_read_only_connection() throws SQLException {
+    initializePlugin();
+    spyPlugin.failoverMode = FailoverMode.STRICT_WRITER;
+
+    when(mockPluginService.isReadOnlyConnectionException(any(), any(TargetDriverDialect.class))).thenReturn(true);
+    assertTrue(spyPlugin.shouldExceptionTriggerConnectionSwitch(new SQLException("test", "any")));
+
+    when(mockPluginService.isReadOnlyConnectionException(any(), any(TargetDriverDialect.class))).thenReturn(false);
+    assertFalse(spyPlugin.shouldExceptionTriggerConnectionSwitch(new SQLException("test", "any")));
+  }
+
 }