[client-v2, jdbc-v2] Fix logging-layer defects (#2968)

CHANGELOG.md

@@ -19,6 +19,14 @@
 
 ### Bug Fixes 
 
+- **[client-v2, jdbc-v2]** Fixed several logging-layer defects. In `client-v2`, `HttpAPIClientHelper.shouldRetry`
+  threw a `ClassCastException` when a retryable `ServerException` was wrapped as the *cause* of another exception
+  (the branch matched on the cause but the cast used the outer exception); the retry decision is now taken from
+  whichever exception is the `ServerException`. Also in `client-v2`, a failure to build the HTTP client version
+  string is now logged at `WARN` with the throwable attached instead of a bare `INFO` message that discarded the
+  cause. In `jdbc-v2`, a failure to close the response after a query error now logs the close failure itself
+  instead of the already-propagated outer exception. (https://github.com/ClickHouse/clickhouse-java/issues/2968)
+
 - **[client-v2]** Fixed scalar `String` query parameters containing a tab (`0x09`), newline
   (`0x0a`) or backslash being mishandled through the server's `param_<name>` interface. A `{name:String}`
   parameter value is parsed by the server with `deserializeTextEscaped`, which treated a raw tab or

client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java

@@ -1029,7 +1029,7 @@ public boolean shouldRetry(Throwable ex, Map<String, Object> requestSettings) {
 
         // there are some db retryable error codes
         if (ex instanceof ServerException || ex.getCause() instanceof ServerException) {
-            ServerException se = (ServerException) ex;
+            ServerException se = (ServerException) (ex instanceof ServerException ? ex : ex.getCause());
             return se.isRetryable() && retryCauses.contains(ClientFaultCause.ServerRetryable);
         }
 
@@ -1124,7 +1124,7 @@ private String buildDefaultUserAgent() {
                     .append('/')
                     .append(httpClientVersion);
         } catch (Exception e) {
-            LOG.info("failed to construct http client version string");
+            LOG.warn("failed to construct http client version string", e);
         }
         return userAgent.toString();
     }

client-v2/src/test/java/com/clickhouse/client/api/internal/HttpAPIClientHelperTest.java

@@ -1,6 +1,7 @@
 package com.clickhouse.client.api.internal;
 
 import com.clickhouse.client.api.ClientConfigProperties;
+import com.clickhouse.client.api.ServerException;
 import com.clickhouse.client.api.enums.SSLMode;
 import com.clickhouse.client.api.internal.HttpAPIClientHelper.CustomSSLConnectionFactory;
 import com.clickhouse.client.api.transport.Endpoint;
@@ -14,6 +15,7 @@
 import org.mockito.ArgumentCaptor;
 import org.mockito.MockedConstruction;
 import org.testng.Assert;
+import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
 import javax.net.ssl.SNIHostName;
@@ -287,6 +289,35 @@ public void testExecuteRequestThrowsConnectExceptionOn503() throws Exception {
         }
     }
 
+    @DataProvider(name = "serverExceptionRetryCases")
+    public static Object[][] serverExceptionRetryCases() {
+        // Server code 159 (TIMEOUT_EXCEEDED) is retryable; code 60 (TABLE_NOT_FOUND) is not.
+        ServerException retryable = new ServerException(159, "TIMEOUT_EXCEEDED", 500, "q1");
+        ServerException nonRetryable = new ServerException(60, "TABLE_NOT_FOUND", 404, "q2");
+        return new Object[][]{
+                // ServerException thrown directly (behaviour that already worked; pinned as contrast).
+                {retryable, true},
+                {nonRetryable, false},
+                // ServerException wrapped as the cause of another exception: the branch matches on the
+                // cause, so the decision must come from the cause and must not throw ClassCastException.
+                {new RuntimeException("transport failure", retryable), true},
+                {new RuntimeException("transport failure", nonRetryable), false},
+        };
+    }
+
+    /**
+     * {@link HttpAPIClientHelper#shouldRetry} enters its ServerException branch when either the thrown
+     * exception or its cause is a {@link ServerException}, so it must derive the retry decision from
+     * whichever one is the {@code ServerException}. Casting the wrapper unconditionally throws
+     * {@link ClassCastException} when only the cause is a {@code ServerException}.
+     */
+    @Test(dataProvider = "serverExceptionRetryCases")
+    public void testShouldRetryUsesServerExceptionFromCause(Throwable ex, boolean expectedRetry) {
+        HttpAPIClientHelper helper = new HttpAPIClientHelper(new HashMap<>(), null, false, LZ4Factory.fastestInstance());
+        // Empty request settings -> default client_retry_on_failures, which includes ServerRetryable.
+        assertEquals(helper.shouldRetry(ex, new HashMap<>()), expectedRetry);
+    }
+
     /**
      * Builds an {@link HttpAPIClientHelper} and invokes {@link HttpAPIClientHelper#createHttpClient} with SSL
      * enabled while intercepting every {@link CustomSSLConnectionFactory} construction, returning the

jdbc-v2/src/main/java/com/clickhouse/jdbc/StatementImpl.java

@@ -223,7 +223,7 @@ protected ResultSetImpl executeQueryImpl(String sql, QuerySettings settings) thr
                 try {
                     response.close();
                 } catch (Exception ex) {
-                    LOG.warn("Failed to close response after exception", e);
+                    LOG.warn("Failed to close response after exception", ex);
                 }
             }
             handleSocketTimeoutException(e);