Revert "chore: back to original approach"

This reverts commit 0b3d0bc.

Author: diegomarquezp

google-http-client-apache-v5/src/main/java/com/google/api/client/http/apache/v5/Apache5HttpRequest.java

@@ -39,26 +39,8 @@ public final class Apache5HttpRequest extends LowLevelHttpRequest {
   Apache5HttpRequest(HttpClient httpClient, HttpUriRequestBase request) {
     this.httpClient = httpClient;
     this.request = request;
-    this.requestConfig = prepareRequestConfig(null);
-  }
-
-  Apache5HttpRequest(
-      HttpClient httpClient, HttpUriRequestBase request, RequestConfig defaultRequestConfig) {
-    this.httpClient = httpClient;
-    this.request = request;
-    this.requestConfig = prepareRequestConfig(defaultRequestConfig);
-  }
-
-  private RequestConfig.Builder prepareRequestConfig(RequestConfig defaultRequestConfig) {
-    RequestConfig.Builder config;
-    if (defaultRequestConfig != null) {
-      config = RequestConfig.copy(defaultRequestConfig);
-    } else {
-      config = RequestConfig.custom();
-    }
     // disable redirects as google-http-client handles redirects
-    config.setRedirectsEnabled(false);
-    return config;
+    this.requestConfig = RequestConfig.custom().setRedirectsEnabled(false);
   }
 
   @Override
@@ -70,6 +52,7 @@ public void addHeader(String name, String value) {
   public void setTimeout(int connectTimeout, int readTimeout) throws IOException {
     requestConfig
         .setConnectTimeout(Timeout.of(connectTimeout, TimeUnit.MILLISECONDS))
+        .setConnectionRequestTimeout(connectTimeout, TimeUnit.MILLISECONDS)
         // ResponseTimeout behaves the same as 4.x's SocketTimeout
         .setResponseTimeout(Timeout.of(readTimeout, TimeUnit.MILLISECONDS));
   }

google-http-client-apache-v5/src/main/java/com/google/api/client/http/apache/v5/Apache5HttpTransport.java

@@ -33,7 +33,6 @@
 import org.apache.hc.client5.http.classic.methods.HttpTrace;
 import org.apache.hc.client5.http.classic.methods.HttpUriRequestBase;
 import org.apache.hc.client5.http.config.ConnectionConfig;
-import org.apache.hc.client5.http.config.RequestConfig;
 import org.apache.hc.client5.http.impl.classic.HttpClientBuilder;
 import org.apache.hc.client5.http.impl.classic.HttpClients;
 import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager;
@@ -61,9 +60,6 @@ public final class Apache5HttpTransport extends HttpTransport {
   /** Apache HTTP client. */
   private final HttpClient httpClient;
 
-  /** The default request config for the client. */
-  private final RequestConfig defaultRequestConfig;
-
   /** If the HTTP client uses mTLS channel. */
   private final boolean isMtls;
 
@@ -85,29 +81,8 @@ public Apache5HttpTransport() {
    * @param httpClient Apache HTTP client to use
    */
   public Apache5HttpTransport(HttpClient httpClient) {
-    this(httpClient, false);
-  }
-
-  /**
-   * {@link Beta} <br>
-   * Constructor that allows an alternative CLoseable Apache HTTP client to be used.
-   *
-   * <p>If you choose to provide your own Apache HttpClient implementation, be sure that
-   *
-   * <ul>
-   *   <li>HTTP version is set to 1.1.
-   *   <li>Retries are disabled (google-http-client handles retries).
-   *   <li>Redirects are disabled (google-http-client handles retries).
-   * </ul>
-   *
-   * @param httpClient Apache HTTP client to use
-   * @param isMtls If the HTTP client is mutual TLS
-   */
-  @Beta
-  public Apache5HttpTransport(HttpClient httpClient, boolean isMtls) {
     this.httpClient = httpClient;
-    this.defaultRequestConfig = null;
-    this.isMtls = isMtls;
+    this.isMtls = false;
   }
 
   /**
@@ -123,14 +98,11 @@ public Apache5HttpTransport(HttpClient httpClient, boolean isMtls) {
    * </ul>
    *
    * @param httpClient Apache HTTP client to use
-   * @param defaultRequestConfig The default request config for the client
    * @param isMtls If the HTTP client is mutual TLS
    */
   @Beta
-  public Apache5HttpTransport(
-      HttpClient httpClient, RequestConfig defaultRequestConfig, boolean isMtls) {
+  public Apache5HttpTransport(HttpClient httpClient, boolean isMtls) {
     this.httpClient = httpClient;
-    this.defaultRequestConfig = defaultRequestConfig;
     this.isMtls = isMtls;
   }
 
@@ -174,10 +146,6 @@ public static HttpClient newDefaultHttpClient() {
    *       properties</a>.
    * </ul>
    *
-   * <p>NOTE: The {@code ConnectionRequestTimeout} in the {@code RequestConfig} is not honored when
-   * set via the builder. Instead, the request config should be passed in the {@link
-   * #Apache5HttpTransport(HttpClient, RequestConfig, boolean)} constructor.
-   *
    * @return new instance of the Apache HTTP client builder
    */
   public static HttpClientBuilder newDefaultHttpClientBuilder() {
@@ -225,7 +193,7 @@ protected Apache5HttpRequest buildRequest(String method, String url) {
     } else {
       requestBase = new HttpUriRequestBase(Preconditions.checkNotNull(method), URI.create(url));
     }
-    return new Apache5HttpRequest(httpClient, requestBase, defaultRequestConfig);
+    return new Apache5HttpRequest(httpClient, requestBase);
   }
 
   /**

google-http-client-apache-v5/src/test/java/com/google/api/client/http/apache/v5/Apache5HttpRequestTest.java

@@ -26,16 +26,19 @@
 import java.io.InputStream;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 import org.apache.hc.client5.http.classic.methods.HttpPost;
 import org.apache.hc.client5.http.classic.methods.HttpUriRequestBase;
+import org.apache.hc.client5.http.config.RequestConfig;
 import org.apache.hc.core5.http.ClassicHttpRequest;
 import org.apache.hc.core5.http.ClassicHttpResponse;
 import org.apache.hc.core5.http.ContentType;
 import org.apache.hc.core5.http.HttpEntity;
 import org.apache.hc.core5.http.HttpHost;
 import org.apache.hc.core5.http.io.entity.BasicHttpEntity;
 import org.apache.hc.core5.http.protocol.HttpContext;
+import org.apache.hc.core5.util.Timeout;
 import org.junit.Test;
 
 public class Apache5HttpRequestTest {
@@ -128,4 +131,26 @@ public ClassicHttpResponse executeOpen(
     response.getContent().close();
     assertEquals(1, closedResponseCounter.get());
   }
+
+  @Test
+  public void testSetTimeout() throws Exception {
+    HttpUriRequestBase base = new HttpPost("http://www.google.com");
+    Apache5HttpRequest request =
+        new Apache5HttpRequest(
+            new MockHttpClient() {
+              @Override
+              public ClassicHttpResponse executeOpen(
+                  HttpHost target, ClassicHttpRequest request, HttpContext context) {
+                return new MockClassicHttpResponse();
+              }
+            },
+            base);
+    request.setTimeout(100, 200);
+    request.execute();
+
+    RequestConfig config = base.getConfig();
+    assertEquals(Timeout.of(100, TimeUnit.MILLISECONDS), config.getConnectTimeout());
+    assertEquals(Timeout.of(200, TimeUnit.MILLISECONDS), config.getResponseTimeout());
+    assertEquals(Timeout.of(100, TimeUnit.MILLISECONDS), config.getConnectionRequestTimeout());
+  }
 }

google-http-client-apache-v5/src/test/java/com/google/api/client/http/apache/v5/Apache5HttpTransportTest.java

@@ -28,21 +28,14 @@
 import com.google.api.client.util.ByteArrayStreamingContent;
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
-import java.util.concurrent.CountDownLatch;
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
-import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 import org.apache.hc.client5.http.ConnectTimeoutException;
 import org.apache.hc.client5.http.HttpHostConnectException;
 import org.apache.hc.client5.http.classic.HttpClient;
-import org.apache.hc.client5.http.config.RequestConfig;
 import org.apache.hc.client5.http.impl.classic.HttpClients;
-import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager;
 import org.apache.hc.core5.http.ClassicHttpRequest;
 import org.apache.hc.core5.http.ClassicHttpResponse;
-import org.apache.hc.core5.http.ConnectionRequestTimeoutException;
 import org.apache.hc.core5.http.ContentType;
 import org.apache.hc.core5.http.EntityDetails;
 import org.apache.hc.core5.http.Header;
@@ -215,69 +208,6 @@ public void testConnectTimeout() {
     }
   }
 
-  @Test(timeout = 5000)
-  public void testConnectionRequestTimeoutFromDefaultRequestConfig() throws Exception {
-    final CountDownLatch latch = new CountDownLatch(1);
-    final HttpRequestHandler handler =
-        new HttpRequestHandler() {
-          @Override
-          public void handle(
-              ClassicHttpRequest request, ClassicHttpResponse response, HttpContext context)
-              throws HttpException, IOException {
-            try {
-              latch.await(); // Wait for the signal to proceed
-            } catch (InterruptedException e) {
-              Thread.currentThread().interrupt();
-            }
-            response.setCode(HttpStatus.SC_OK);
-          }
-        };
-
-    try (FakeServer server = new FakeServer(handler)) {
-      PoolingHttpClientConnectionManager connectionManager =
-          new PoolingHttpClientConnectionManager();
-      connectionManager.setMaxTotal(1); // Only one connection in the pool
-
-      RequestConfig requestConfig =
-          RequestConfig.custom().setConnectionRequestTimeout(1, TimeUnit.SECONDS).build();
-
-      HttpClient httpClient =
-          Apache5HttpTransport.newDefaultHttpClientBuilder()
-              .setConnectionManager(connectionManager)
-              .setDefaultRequestConfig(requestConfig)
-              .build();
-
-      HttpTransport transport = new Apache5HttpTransport(httpClient, requestConfig, false);
-      final GenericUrl url = new GenericUrl("http://localhost:" + server.getPort());
-
-      ExecutorService executor = Executors.newFixedThreadPool(2);
-
-      // First request takes the only connection
-      executor.submit(
-          () -> {
-            try {
-              transport.createRequestFactory().buildGetRequest(url).execute();
-            } catch (IOException e) {
-              // This request might fail if the test finishes before it completes, which is fine.
-            }
-          });
-
-      // Give the first request time to acquire the connection
-      Thread.sleep(100);
-
-      // Second request should time out waiting for a connection
-      try {
-        transport.createRequestFactory().buildGetRequest(url).execute();
-        fail("Should have thrown ConnectionRequestTimeoutException");
-      } catch (ConnectionRequestTimeoutException e) {
-        // Expected
-      } finally {
-        latch.countDown(); // Allow the first request to complete
-        executor.shutdownNow();
-      }
-    }
-  }
-
   private static class FakeServer implements AutoCloseable {
     private final HttpServer server;