From d012a358decf715340ebd0639e6241fe7b74b419 Mon Sep 17 00:00:00 2001 From: Jeff Wasty Date: Thu, 21 Nov 2024 11:18:11 -0800 Subject: [PATCH 1/7] Remove maximum testing as this is very dependent on pipeline behavior. --- .../ConfigurableRetryLogicTest.java | 57 +++++++------------ 1 file changed, 19 insertions(+), 38 deletions(-) diff --git a/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java b/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java index 3f522080d..a9f3c4300 100644 --- a/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java +++ b/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java @@ -56,7 +56,7 @@ public static void setupTests() throws Exception { /** * Test that the SQLServerConnection methods getRetryExec and setRetryExec correctly get the existing retryExec, and * set the retryExec connection parameter respectively. - * + * * @throws Exception * if an exception occurs */ @@ -108,7 +108,7 @@ public void testRetryConnConnectionStringOption() throws Exception { /** * Tests that statement retry with prepared statements correctly retries given the provided retryExec rule. - * + * * @throws Exception * if unable to connect or execute against db */ @@ -133,7 +133,7 @@ public void testStatementRetryPreparedStatement() throws Exception { /** * Tests that statement retry with callable statements correctly retries given the provided retryExec rule. - * + * * @throws Exception * if unable to connect or execute against db */ @@ -158,7 +158,7 @@ public void testStatementRetryCallableStatement() throws Exception { /** * Tests that statement retry with SQL server statements correctly retries given the provided retryExec rule. - * + * * @throws Exception * if unable to connect or execute against db */ @@ -223,8 +223,8 @@ public void testConnectionRetry(String replacedDbName, String addedRetryParams) /** * Tests that the correct number of retries are happening for all statement scenarios. Tests are expected to take - * a minimum of the sum of whatever has been defined for the waiting intervals, and maximum of the previous sum - * plus some amount of time to account for test environment slowness. + * a minimum of the sum of whatever has been defined for the waiting intervals. Maximum is not tested due to the + * unpredictable factor of slowness that can be applied to these tests. */ @Test public void statementTimingTests() { @@ -253,8 +253,6 @@ public void statementTimingTests() { totalTime = System.currentTimeMillis() - timerStart; assertTrue(totalTime > TimeUnit.SECONDS.toMillis(5), "total time: " + totalTime + ", expected minimum time: " + TimeUnit.SECONDS.toMillis(5)); - assertTrue(totalTime < TimeUnit.SECONDS.toMillis(15), - "total time: " + totalTime + ", expected maximum time: " + TimeUnit.SECONDS.toMillis(15)); } timerStart = System.currentTimeMillis(); @@ -268,8 +266,6 @@ public void statementTimingTests() { totalTime = System.currentTimeMillis() - timerStart; assertTrue(totalTime > TimeUnit.SECONDS.toMillis(2), "total time: " + totalTime + ", expected minimum time: " + TimeUnit.SECONDS.toMillis(8)); - assertTrue(totalTime < TimeUnit.SECONDS.toMillis(18), - "total time: " + totalTime + ", expected maximum time: " + TimeUnit.SECONDS.toMillis(18)); } timerStart = System.currentTimeMillis(); @@ -283,8 +279,6 @@ public void statementTimingTests() { totalTime = System.currentTimeMillis() - timerStart; assertTrue(totalTime > TimeUnit.SECONDS.toMillis(3), "total time: " + totalTime + ", expected minimum time: " + TimeUnit.SECONDS.toMillis(10)); - assertTrue(totalTime < TimeUnit.SECONDS.toMillis(20), - "total time: " + totalTime + ", expected maximum time: " + TimeUnit.SECONDS.toMillis(20)); } } @@ -397,7 +391,7 @@ public void testTooManyTimings() { /** * Tests that rules with an invalid retry error correctly fail. - * + * * @throws Exception * for the invalid parameter */ @@ -420,7 +414,7 @@ public void testRetryError() throws Exception { /** * Tests that rules with an invalid retry count correctly fail. - * + * * @throws Exception * for the invalid parameter */ @@ -443,7 +437,7 @@ public void testRetryCount() throws Exception { /** * Tests that rules with an invalid initial retry time correctly fail. - * + * * @throws Exception * for the invalid parameter */ @@ -466,7 +460,7 @@ public void testInitialRetryTime() throws Exception { /** * Tests that rules with an invalid operand correctly fail. - * + * * @throws Exception * for the invalid parameter */ @@ -481,7 +475,7 @@ public void testOperand() throws Exception { /** * Tests that rules with an invalid retry change correctly fail. - * + * * @throws Exception * for the invalid parameter */ @@ -496,14 +490,13 @@ public void testRetryChange() throws Exception { /** * Tests that the correct number of retries are happening for all connection scenarios. Tests are expected to take - * a minimum of the sum of whatever has been defined for the waiting intervals, and maximum of the previous sum - * plus some amount of time to account for test environment slowness. + * a minimum of the sum of whatever has been defined for the waiting intervals. Maximum is not tested due to the + * unpredictable factor of slowness that can be applied to these tests. */ @Test public void connectionTimingTest() { long totalTime; long timerStart = System.currentTimeMillis(); - long expectedMaxTime = 10; // No retries since CRL rules override, expected time ~1 second try { @@ -513,22 +506,14 @@ public void connectionTimingTest() { (e.getMessage().toLowerCase() .contains(TestResource.getResource("R_cannotOpenDatabase").toLowerCase())) || (TestUtils.getProperty(connectionString, "msiClientId") != null && e.getMessage() - .toLowerCase().contains(TestResource.getResource("R_loginFailedMI").toLowerCase())) + .toLowerCase().contains(TestResource.getResource("R_loginFailedMI").toLowerCase())) || ((isSqlAzure() || isSqlAzureDW()) && e.getMessage().toLowerCase() - .contains(TestResource.getResource("R_connectTimedOut").toLowerCase())), + .contains(TestResource.getResource("R_connectTimedOut").toLowerCase())), e.getMessage()); - - if (e.getMessage().toLowerCase().contains(TestResource.getResource("R_cannotOpenDatabase").toLowerCase())) { - // Only check the timing if the correct error, "cannot open database", is returned. - totalTime = System.currentTimeMillis() - timerStart; - assertTrue(totalTime < TimeUnit.SECONDS.toMillis(expectedMaxTime), - "total time: " + totalTime + ", expected time: " + TimeUnit.SECONDS.toMillis(expectedMaxTime)); - } } timerStart = System.currentTimeMillis(); long expectedMinTime = 10; - expectedMaxTime = 35; // (0s attempt + 0s attempt + 10s wait + 0s attempt) = expected 10s execution time try { @@ -538,16 +523,14 @@ public void connectionTimingTest() { (e.getMessage().toLowerCase() .contains(TestResource.getResource("R_cannotOpenDatabase").toLowerCase())) || (TestUtils.getProperty(connectionString, "msiClientId") != null && e.getMessage() - .toLowerCase().contains(TestResource.getResource("R_loginFailedMI").toLowerCase())) + .toLowerCase().contains(TestResource.getResource("R_loginFailedMI").toLowerCase())) || ((isSqlAzure() || isSqlAzureDW()) && e.getMessage().toLowerCase() - .contains(TestResource.getResource("R_connectTimedOut").toLowerCase())), + .contains(TestResource.getResource("R_connectTimedOut").toLowerCase())), e.getMessage()); if (e.getMessage().toLowerCase().contains(TestResource.getResource("R_cannotOpenDatabase").toLowerCase())) { // Only check the timing if the correct error, "cannot open database", is returned. totalTime = System.currentTimeMillis() - timerStart; - assertTrue(totalTime < TimeUnit.SECONDS.toMillis(expectedMaxTime), "total time: " + totalTime - + ", expected max time: " + TimeUnit.SECONDS.toMillis(expectedMaxTime)); assertTrue(totalTime > TimeUnit.SECONDS.toMillis(expectedMinTime), "total time: " + totalTime + ", expected min time: " + TimeUnit.SECONDS.toMillis(expectedMinTime)); } @@ -563,16 +546,14 @@ public void connectionTimingTest() { (e.getMessage().toLowerCase() .contains(TestResource.getResource("R_cannotOpenDatabase").toLowerCase())) || (TestUtils.getProperty(connectionString, "msiClientId") != null && e.getMessage() - .toLowerCase().contains(TestResource.getResource("R_loginFailedMI").toLowerCase())) + .toLowerCase().contains(TestResource.getResource("R_loginFailedMI").toLowerCase())) || ((isSqlAzure() || isSqlAzureDW()) && e.getMessage().toLowerCase() - .contains(TestResource.getResource("R_connectTimedOut").toLowerCase())), + .contains(TestResource.getResource("R_connectTimedOut").toLowerCase())), e.getMessage()); if (e.getMessage().toLowerCase().contains(TestResource.getResource("R_cannotOpenDatabase").toLowerCase())) { // Only check the timing if the correct error, "cannot open database", is returned. totalTime = System.currentTimeMillis() - timerStart; - assertTrue(totalTime < TimeUnit.SECONDS.toMillis(expectedMaxTime), "total time: " + totalTime - + ", expected max time: " + TimeUnit.SECONDS.toMillis(expectedMaxTime)); assertTrue(totalTime > TimeUnit.SECONDS.toMillis(expectedMinTime), "total time: " + totalTime + ", expected min time: " + TimeUnit.SECONDS.toMillis(expectedMinTime)); } From 50635636223365ede4b8f3914bd2eec9e7c5a5d8 Mon Sep 17 00:00:00 2001 From: Jeff Wasty Date: Thu, 9 Jan 2025 13:49:29 -0800 Subject: [PATCH 2/7] Added an additional check for mssql-jdbc.properties to prevent IndexOutOfBoundsExcelption --- .../sqlserver/jdbc/ConfigurableRetryLogic.java | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/ConfigurableRetryLogic.java b/src/main/java/com/microsoft/sqlserver/jdbc/ConfigurableRetryLogic.java index df9e6b956..d906d3143 100644 --- a/src/main/java/com/microsoft/sqlserver/jdbc/ConfigurableRetryLogic.java +++ b/src/main/java/com/microsoft/sqlserver/jdbc/ConfigurableRetryLogic.java @@ -36,7 +36,6 @@ public class ConfigurableRetryLogic { .getLogger("com.microsoft.sqlserver.jdbc.ConfigurableRetryLogic"); private static final String SEMI_COLON = ";"; private static final String COMMA = ","; - private static final String FORWARD_SLASH = "/"; private static final String EQUALS_SIGN = "="; private static final String RETRY_EXEC = "retryExec"; private static final String RETRY_CONN = "retryConn"; @@ -283,16 +282,25 @@ private static void createConnectionRules(LinkedList listOfRules) throws private static String getCurrentClassPath() throws SQLServerException { String location = ""; String className = ""; + String locationSuffix = "target/classes/"; try { className = new Object() {}.getClass().getEnclosingClass().getName(); location = Class.forName(className).getProtectionDomain().getCodeSource().getLocation().getPath(); - location = location.substring(0, location.length() - 16); - URI uri = new URI(location + FORWARD_SLASH); + // When the driver is used as a jar / as a dependency, the above will be the correct "main" directory where + // the props file should be placed (as per documentation). When testing, or building the driver manually, + // the above will return with a suffix "target/classes/" which must be removed, as, with the first case, the + // properties file should be in the main directory. + + if (location.endsWith(locationSuffix)) { + location = location.substring(0, location.length() - locationSuffix.length()); + } + + URI uri = new URI(location); return uri.getPath() + DEFAULT_PROPS_FILE; // For now, we only allow "mssql-jdbc.properties" as file name. } catch (URISyntaxException e) { MessageFormat form = new MessageFormat(SQLServerException.getErrString("R_URLInvalid")); - Object[] msgArgs = {location + FORWARD_SLASH}; + Object[] msgArgs = {location}; throw new SQLServerException(form.format(msgArgs), null, 0, e); } catch (ClassNotFoundException e) { MessageFormat form = new MessageFormat(SQLServerException.getErrString("R_UnableToFindClass")); From e65ec0ad9d188127677d5348cfbd950a592291c8 Mon Sep 17 00:00:00 2001 From: Jeff Wasty Date: Thu, 9 Jan 2025 13:53:51 -0800 Subject: [PATCH 3/7] Fixed formatting --- .../ConfigurableRetryLogicTest.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java b/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java index a9f3c4300..20741cb50 100644 --- a/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java +++ b/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java @@ -506,9 +506,9 @@ public void connectionTimingTest() { (e.getMessage().toLowerCase() .contains(TestResource.getResource("R_cannotOpenDatabase").toLowerCase())) || (TestUtils.getProperty(connectionString, "msiClientId") != null && e.getMessage() - .toLowerCase().contains(TestResource.getResource("R_loginFailedMI").toLowerCase())) + .toLowerCase().contains(TestResource.getResource("R_loginFailedMI").toLowerCase())) || ((isSqlAzure() || isSqlAzureDW()) && e.getMessage().toLowerCase() - .contains(TestResource.getResource("R_connectTimedOut").toLowerCase())), + .contains(TestResource.getResource("R_connectTimedOut").toLowerCase())), e.getMessage()); } @@ -523,9 +523,9 @@ public void connectionTimingTest() { (e.getMessage().toLowerCase() .contains(TestResource.getResource("R_cannotOpenDatabase").toLowerCase())) || (TestUtils.getProperty(connectionString, "msiClientId") != null && e.getMessage() - .toLowerCase().contains(TestResource.getResource("R_loginFailedMI").toLowerCase())) + .toLowerCase().contains(TestResource.getResource("R_loginFailedMI").toLowerCase())) || ((isSqlAzure() || isSqlAzureDW()) && e.getMessage().toLowerCase() - .contains(TestResource.getResource("R_connectTimedOut").toLowerCase())), + .contains(TestResource.getResource("R_connectTimedOut").toLowerCase())), e.getMessage()); if (e.getMessage().toLowerCase().contains(TestResource.getResource("R_cannotOpenDatabase").toLowerCase())) { @@ -546,9 +546,9 @@ public void connectionTimingTest() { (e.getMessage().toLowerCase() .contains(TestResource.getResource("R_cannotOpenDatabase").toLowerCase())) || (TestUtils.getProperty(connectionString, "msiClientId") != null && e.getMessage() - .toLowerCase().contains(TestResource.getResource("R_loginFailedMI").toLowerCase())) + .toLowerCase().contains(TestResource.getResource("R_loginFailedMI").toLowerCase())) || ((isSqlAzure() || isSqlAzureDW()) && e.getMessage().toLowerCase() - .contains(TestResource.getResource("R_connectTimedOut").toLowerCase())), + .contains(TestResource.getResource("R_connectTimedOut").toLowerCase())), e.getMessage()); if (e.getMessage().toLowerCase().contains(TestResource.getResource("R_cannotOpenDatabase").toLowerCase())) { From ae88ad2b53af7c3e60134294402752341e8f5c3d Mon Sep 17 00:00:00 2001 From: Jeff Wasty Date: Thu, 9 Jan 2025 13:56:47 -0800 Subject: [PATCH 4/7] Fixed formatting --- .../jdbc/configurableretry/ConfigurableRetryLogicTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java b/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java index 20741cb50..72c21ae18 100644 --- a/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java +++ b/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java @@ -108,7 +108,7 @@ public void testRetryConnConnectionStringOption() throws Exception { /** * Tests that statement retry with prepared statements correctly retries given the provided retryExec rule. - * + * * @throws Exception * if unable to connect or execute against db */ From 0c254e564a3b4071efbfa908d7d17cb4de4ff74c Mon Sep 17 00:00:00 2001 From: Jeff Wasty Date: Thu, 9 Jan 2025 13:57:34 -0800 Subject: [PATCH 5/7] Fixed formatting --- .../ConfigurableRetryLogicTest.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java b/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java index 72c21ae18..a694bbd65 100644 --- a/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java +++ b/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java @@ -133,7 +133,7 @@ public void testStatementRetryPreparedStatement() throws Exception { /** * Tests that statement retry with callable statements correctly retries given the provided retryExec rule. - * + * * @throws Exception * if unable to connect or execute against db */ @@ -158,7 +158,7 @@ public void testStatementRetryCallableStatement() throws Exception { /** * Tests that statement retry with SQL server statements correctly retries given the provided retryExec rule. - * + * * @throws Exception * if unable to connect or execute against db */ @@ -391,7 +391,7 @@ public void testTooManyTimings() { /** * Tests that rules with an invalid retry error correctly fail. - * + * * @throws Exception * for the invalid parameter */ @@ -414,7 +414,7 @@ public void testRetryError() throws Exception { /** * Tests that rules with an invalid retry count correctly fail. - * + * * @throws Exception * for the invalid parameter */ @@ -437,7 +437,7 @@ public void testRetryCount() throws Exception { /** * Tests that rules with an invalid initial retry time correctly fail. - * + * * @throws Exception * for the invalid parameter */ @@ -460,7 +460,7 @@ public void testInitialRetryTime() throws Exception { /** * Tests that rules with an invalid operand correctly fail. - * + * * @throws Exception * for the invalid parameter */ @@ -475,7 +475,7 @@ public void testOperand() throws Exception { /** * Tests that rules with an invalid retry change correctly fail. - * + * * @throws Exception * for the invalid parameter */ From 742108c0c709e4b96c300020cef82806e8d7eaf0 Mon Sep 17 00:00:00 2001 From: Jeff Wasty Date: Thu, 9 Jan 2025 13:58:03 -0800 Subject: [PATCH 6/7] Fixed formatting --- .../jdbc/configurableretry/ConfigurableRetryLogicTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java b/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java index a694bbd65..b4f8fe4c7 100644 --- a/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java +++ b/src/test/java/com/microsoft/sqlserver/jdbc/configurableretry/ConfigurableRetryLogicTest.java @@ -56,7 +56,7 @@ public static void setupTests() throws Exception { /** * Test that the SQLServerConnection methods getRetryExec and setRetryExec correctly get the existing retryExec, and * set the retryExec connection parameter respectively. - * + * * @throws Exception * if an exception occurs */ From f902c81c52f45b117c518acc96f294fbfc7ae5a6 Mon Sep 17 00:00:00 2001 From: Jeff Wasty Date: Thu, 30 Jan 2025 14:14:50 -0800 Subject: [PATCH 7/7] Fixed according to code review --- .../jdbc/ConfigurableRetryLogic.java | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/ConfigurableRetryLogic.java b/src/main/java/com/microsoft/sqlserver/jdbc/ConfigurableRetryLogic.java index d906d3143..e6e47dabb 100644 --- a/src/main/java/com/microsoft/sqlserver/jdbc/ConfigurableRetryLogic.java +++ b/src/main/java/com/microsoft/sqlserver/jdbc/ConfigurableRetryLogic.java @@ -12,6 +12,9 @@ import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; +import java.nio.file.Files; +import java.nio.file.Paths; +import java.security.CodeSource; import java.text.MessageFormat; import java.util.Collections; import java.util.Date; @@ -285,15 +288,23 @@ private static String getCurrentClassPath() throws SQLServerException { String locationSuffix = "target/classes/"; try { + // Attempt to get the location and CodeSource for this class className = new Object() {}.getClass().getEnclosingClass().getName(); location = Class.forName(className).getProtectionDomain().getCodeSource().getLocation().getPath(); - // When the driver is used as a jar / as a dependency, the above will be the correct "main" directory where - // the props file should be placed (as per documentation). When testing, or building the driver manually, - // the above will return with a suffix "target/classes/" which must be removed, as, with the first case, the - // properties file should be in the main directory. - - if (location.endsWith(locationSuffix)) { - location = location.substring(0, location.length() - locationSuffix.length()); + CodeSource codeSource = ConfigurableRetryLogic.class.getProtectionDomain().getCodeSource(); + + if (codeSource == null) { + // CodeSource should never be null, so throw an error + MessageFormat form = new MessageFormat(SQLServerException.getErrString("R_UnableToFindClass")); + Object[] msgArgs = {ConfigurableRetryLogic.class}; + throw new SQLServerException(form.format(msgArgs), null, 0, null); + } else { + if (Files.isDirectory(Paths.get(codeSource.getLocation().toURI()))) { + // We check if the Path we get from the CodeSource location is a directory. If so, we are running + // from class files and should remove a suffix (i.e. the props file is in a different location from the + // location returned) + location = location.substring(0, location.length() - locationSuffix.length()); + } } URI uri = new URI(location);