SNOW-577665: Wrong retry 403 logic (#916)

Fixed the bug for isNonRetryableHttpCode(403, true/false)
snowflakedb · Apr 21, 2022 · cd3b331 · cd3b331
1 parent 427ff6f
commit cd3b331
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 11 deletions.
diff --git a/src/main/java/net/snowflake/client/jdbc/RestRequest.java b/src/main/java/net/snowflake/client/jdbc/RestRequest.java
@@ -207,7 +207,7 @@ public static CloseableHttpResponse execute(
        * If we got a response and the status code is not one of those
        * transient failures, no more retry
        */
-      if (isCertificateRevoked(savedEx) || isNonretryableHTTPCode(response, retryHTTP403)) {
+      if (isCertificateRevoked(savedEx) || isNonRetryableHTTPCode(response, retryHTTP403)) {
         String msg = "Unknown cause";
         if (response != null) {
           logger.debug("HTTP response code: {}", response.getStatusLine().getStatusCode());
@@ -439,15 +439,15 @@ public static CloseableHttpResponse execute(
     return response;
   }
 
-  static boolean isNonretryableHTTPCode(CloseableHttpResponse response, boolean retryHTTP403) {
+  static boolean isNonRetryableHTTPCode(CloseableHttpResponse response, boolean retryHTTP403) {
     return response != null
         && (response.getStatusLine().getStatusCode() < 500
             || // service unavailable
             response.getStatusLine().getStatusCode() >= 600)
         && // gateway timeout
         response.getStatusLine().getStatusCode() != 408
         && // request timeout
-        (retryHTTP403 || response.getStatusLine().getStatusCode() != 403);
+        (!retryHTTP403 || response.getStatusLine().getStatusCode() != 403);
   }
 
   private static boolean isCertificateRevoked(Exception ex) {

diff --git a/src/test/java/net/snowflake/client/jdbc/RestRequestTest.java b/src/test/java/net/snowflake/client/jdbc/RestRequestTest.java
@@ -158,12 +158,12 @@ private CloseableHttpResponse anyStatusCodeResponse(int statusCode) {
   }
 
   @Test
-  public void testIsRetryableHTTPCode() throws Exception {
+  public void testIsNonRetryableHTTPCode() throws Exception {
     class TestCase {
       TestCase(int statusCode, boolean retryHTTP403, boolean result) {
         this.statusCode = statusCode;
         this.retryHTTP403 = retryHTTP403;
-        this.result = result;
+        this.result = result; // expected result of calling isNonRetryableHTTPCode()
       }
 
       public int statusCode;
@@ -172,6 +172,7 @@ class TestCase {
     }
     List<TestCase> testCases = new ArrayList<>();
     // no retry on HTTP 403 option
+
     testCases.add(new TestCase(100, false, true));
     testCases.add(new TestCase(101, false, true));
     testCases.add(new TestCase(103, false, true));
@@ -191,12 +192,12 @@ class TestCase {
     testCases.add(new TestCase(308, false, true));
     testCases.add(new TestCase(400, false, true));
     testCases.add(new TestCase(401, false, true));
-    testCases.add(new TestCase(403, false, false)); // no retry on HTTP 403
+    testCases.add(new TestCase(403, false, true)); // no retry on HTTP 403
     testCases.add(new TestCase(404, false, true));
     testCases.add(new TestCase(405, false, true));
     testCases.add(new TestCase(406, false, true));
     testCases.add(new TestCase(407, false, true));
-    testCases.add(new TestCase(408, false, false)); // no retry on HTTP 408
+    testCases.add(new TestCase(408, false, false)); // do retry on HTTP 408
     testCases.add(new TestCase(410, false, true));
     testCases.add(new TestCase(411, false, true));
     testCases.add(new TestCase(412, false, true));
@@ -244,12 +245,12 @@ class TestCase {
     testCases.add(new TestCase(308, true, true));
     testCases.add(new TestCase(400, true, true));
     testCases.add(new TestCase(401, true, true));
-    testCases.add(new TestCase(403, true, true)); // do retry on HTTP 403
+    testCases.add(new TestCase(403, true, false)); // do retry on HTTP 403
     testCases.add(new TestCase(404, true, true));
     testCases.add(new TestCase(405, true, true));
     testCases.add(new TestCase(406, true, true));
     testCases.add(new TestCase(407, true, true));
-    testCases.add(new TestCase(408, true, false)); // no retry on HTTP 408
+    testCases.add(new TestCase(408, true, false)); // do retry on HTTP 408
     testCases.add(new TestCase(410, true, true));
     testCases.add(new TestCase(411, true, true));
     testCases.add(new TestCase(412, true, true));
@@ -284,14 +285,14 @@ class TestCase {
             String.format(
                 "Result must be true but false: HTTP Code: %d, RetryHTTP403: %s",
                 t.statusCode, t.retryHTTP403),
-            RestRequest.isNonretryableHTTPCode(
+            RestRequest.isNonRetryableHTTPCode(
                 anyStatusCodeResponse(t.statusCode), t.retryHTTP403));
       } else {
         assertFalse(
             String.format(
                 "Result must be false but true: HTTP Code: %d, RetryHTTP403: %s",
                 t.statusCode, t.retryHTTP403),
-            RestRequest.isNonretryableHTTPCode(
+            RestRequest.isNonRetryableHTTPCode(
                 anyStatusCodeResponse(t.statusCode), t.retryHTTP403));
       }
     }