DHIS2-18568: allow Route response timeout to be customised (#19872)

* feat(DHIS2-18568): allow response timeout to be customised perf: use non-blocking WebClient instead of RestTemplate * fix: persist response timeout tolerate HTTP errors in WebClient test: add test coverage for response timeout and HTTP errors * fix: validate response timeout * fix: solve SonarQube issues * style: format * fix: update to newer patch version of Spring to solve reported security issues * fix: validate Route response time update * fix: handle max body size * refactor: eliminate code smell * refactor: add DB constraint * refactor: rename responseTimeout to responseTimeoutSeconds use int instead of Integer for responseTimeout to remove null checks remove redundant annotations rename references use `Json.object` instead handcrafting JSON in RouteService * fix(security): reduce default to 5s after conversation with @amcgee * perf: stream request and response bodies feat: support binary bodies * refactor: follow-up PR comments * fix: filter response headers * style: reformat * refactor: drop error handing for DataBufferLimitException since it can't happen anymore due to streaming * refactor: drop redundant close on output stream * touch * style: reformat * fix: clean up resources and bump request buffer size * chore: update dependencies to fix reported security issues * fix: limit total transfer time to 5 mins
dhis2 · Feb 17, 2025 · 5f32ce2 · 5f32ce2
1 parent 111fbbd
commit 5f32ce2
Show file tree

Hide file tree

Showing 11 changed files with 429 additions and 120 deletions.
diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/route/Route.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/route/Route.java
@@ -48,6 +48,7 @@
 @EqualsAndHashCode(callSuper = true)
 @Accessors(chain = true)
 public class Route extends BaseIdentifiableObject implements MetadataObject {
+  public static final int DEFAULT_RESPONSE_TIMEOUT_SECONDS = 5;
   public static final String PATH_WILDCARD_SUFFIX = "/**";
 
   @JsonProperty private String description;
@@ -67,6 +68,9 @@ public class Route extends BaseIdentifiableObject implements MetadataObject {
   /** Optional. Required authorities for invoking the route. */
   @JsonProperty private List<String> authorities = new ArrayList<>();
 
+  @JsonProperty(defaultValue = "" + DEFAULT_RESPONSE_TIMEOUT_SECONDS)
+  private int responseTimeoutSeconds = DEFAULT_RESPONSE_TIMEOUT_SECONDS;
+
   /**
    * If the route url ends with /** return true. Otherwise return false.
    *

diff --git a/dhis-2/dhis-services/dhis-service-core/pom.xml b/dhis-2/dhis-services/dhis-service-core/pom.xml
@@ -69,10 +69,6 @@
       <artifactId>dhis-support-sql</artifactId>
     </dependency>
     <!-- Other -->
-    <dependency>
-      <groupId>org.apache.httpcomponents.core5</groupId>
-      <artifactId>httpcore5</artifactId>
-    </dependency>
     <dependency>
       <groupId>org.hisp.dhis.parser</groupId>
       <artifactId>dhis-antlr-expression-parser</artifactId>
@@ -97,6 +93,31 @@
       <groupId>org.springframework.security</groupId>
       <artifactId>spring-security-oauth2-core</artifactId>
     </dependency>
+    <dependency>
+      <groupId>org.springframework</groupId>
+      <artifactId>spring-webflux</artifactId>
+      <version>${spring.version}</version>
+    </dependency>
+    <dependency>
+      <groupId>io.projectreactor</groupId>
+      <artifactId>reactor-core</artifactId>
+      <version>3.6.9</version>
+    </dependency>
+    <dependency>
+      <groupId>io.projectreactor.netty</groupId>
+      <artifactId>reactor-netty-http</artifactId>
+      <version>1.2.2</version>
+    </dependency>
+    <dependency>
+      <groupId>org.reactivestreams</groupId>
+      <artifactId>reactive-streams</artifactId>
+      <version>1.0.4</version>
+    </dependency>
+    <dependency>
+      <groupId>io.netty</groupId>
+      <artifactId>netty-handler</artifactId>
+      <version>4.1.116.Final</version>
+    </dependency>
     <dependency>
       <groupId>com.nimbusds</groupId>
       <artifactId>nimbus-jose-jwt</artifactId>
@@ -197,10 +218,6 @@
       <groupId>com.google.code.gson</groupId>
       <artifactId>gson</artifactId>
     </dependency>
-    <dependency>
-      <groupId>org.apache.httpcomponents.client5</groupId>
-      <artifactId>httpclient5</artifactId>
-    </dependency>
 
     <!-- Apache JClouds -->
 
@@ -290,6 +307,7 @@
       <groupId>org.springframework</groupId>
       <artifactId>spring-webmvc</artifactId>
     </dependency>
+
     <dependency>
       <groupId>org.springframework.security</groupId>
       <artifactId>spring-security-crypto</artifactId>

diff --git a/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/route/RouteService.java b/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/route/RouteService.java
@@ -29,9 +29,10 @@
 
 import static org.hisp.dhis.config.HibernateEncryptionConfig.AES_128_STRING_ENCRYPTOR;
 
+import io.netty.handler.timeout.ReadTimeoutException;
 import jakarta.servlet.http.HttpServletRequest;
 import java.io.IOException;
-import java.nio.charset.StandardCharsets;
+import java.time.Duration;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -45,31 +46,31 @@
 import java.util.function.Function;
 import javax.annotation.Nonnull;
 import javax.annotation.PostConstruct;
-import lombok.Getter;
 import lombok.RequiredArgsConstructor;
-import lombok.Setter;
 import lombok.extern.slf4j.Slf4j;
-import org.apache.hc.client5.http.config.ConnectionConfig;
-import org.apache.hc.client5.http.config.RequestConfig;
-import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager;
-import org.apache.hc.core5.http.io.SocketConfig;
-import org.apache.hc.core5.util.Timeout;
 import org.hibernate.Hibernate;
 import org.hisp.dhis.feedback.BadRequestException;
 import org.hisp.dhis.user.UserDetails;
 import org.jasypt.encryption.pbe.PBEStringCleanablePasswordEncryptor;
-import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Qualifier;
-import org.springframework.http.HttpEntity;
+import org.springframework.core.io.InputStreamResource;
+import org.springframework.core.io.buffer.DataBuffer;
+import org.springframework.core.io.buffer.DataBufferFactory;
+import org.springframework.core.io.buffer.DataBufferUtils;
+import org.springframework.core.io.buffer.DefaultDataBufferFactory;
 import org.springframework.http.HttpHeaders;
 import org.springframework.http.HttpMethod;
+import org.springframework.http.HttpStatus;
 import org.springframework.http.ResponseEntity;
-import org.springframework.http.client.HttpComponentsClientHttpRequestFactory;
+import org.springframework.http.client.reactive.ClientHttpConnector;
 import org.springframework.stereotype.Service;
-import org.springframework.util.StreamUtils;
 import org.springframework.util.StringUtils;
-import org.springframework.web.client.RestTemplate;
+import org.springframework.web.reactive.function.client.WebClient;
+import org.springframework.web.servlet.mvc.method.annotation.StreamingResponseBody;
 import org.springframework.web.util.UriComponentsBuilder;
+import reactor.core.publisher.Flux;
+import reactor.core.publisher.Mono;
+import reactor.netty.http.client.HttpClientRequest;
 
 /**
  * @author Morten Olav Hansen
@@ -85,7 +86,11 @@ public class RouteService {
   @Qualifier(AES_128_STRING_ENCRYPTOR)
   private final PBEStringCleanablePasswordEncryptor encryptor;
 
-  @Autowired @Getter @Setter private RestTemplate restTemplate;
+  private final ClientHttpConnector clientHttpConnector;
+
+  private DataBufferFactory dataBufferFactory;
+
+  private WebClient webClient;
 
   private static final Set<String> ALLOWED_REQUEST_HEADERS =
       Set.of(
@@ -120,29 +125,8 @@ public class RouteService {
 
   @PostConstruct
   public void postConstruct() {
-    // Connect timeout
-    ConnectionConfig connectionConfig =
-        ConnectionConfig.custom().setConnectTimeout(Timeout.ofMilliseconds(5_000)).build();
-
-    // Socket timeout
-    SocketConfig socketConfig =
-        SocketConfig.custom().setSoTimeout(Timeout.ofMilliseconds(10_000)).build();
-
-    // Connection request timeout
-    RequestConfig requestConfig =
-        RequestConfig.custom().setConnectionRequestTimeout(Timeout.ofMilliseconds(1_000)).build();
-
-    PoolingHttpClientConnectionManager connectionManager = new PoolingHttpClientConnectionManager();
-    connectionManager.setDefaultSocketConfig(socketConfig);
-    connectionManager.setDefaultConnectionConfig(connectionConfig);
-
-    org.apache.hc.client5.http.classic.HttpClient httpClient =
-        org.apache.hc.client5.http.impl.classic.HttpClientBuilder.create()
-            .setConnectionManager(connectionManager)
-            .setDefaultRequestConfig(requestConfig)
-            .build();
-
-    restTemplate.setRequestFactory(new HttpComponentsClientHttpRequestFactory(httpClient));
+    webClient = WebClient.builder().clientConnector(clientHttpConnector).build();
+    dataBufferFactory = new DefaultDataBufferFactory();
   }
 
   /**
@@ -185,7 +169,7 @@ public Route getRouteWithDecryptedAuth(@Nonnull String id) {
    * @throws IOException
    * @throws BadRequestException
    */
-  public ResponseEntity<String> execute(
+  public ResponseEntity<StreamingResponseBody> execute(
       Route route, UserDetails userDetails, Optional<String> subPath, HttpServletRequest request)
       throws IOException, BadRequestException {
 
@@ -222,37 +206,79 @@ public ResponseEntity<String> execute(
       uriComponentsBuilder.path(subPath.get());
     }
 
-    String body = StreamUtils.copyToString(request.getInputStream(), StandardCharsets.UTF_8);
-
-    HttpEntity<String> entity = new HttpEntity<>(body, headers);
-
     HttpMethod httpMethod =
         Objects.requireNonNullElse(HttpMethod.valueOf(request.getMethod()), HttpMethod.GET);
     String targetUri = uriComponentsBuilder.toUriString();
 
+    Flux<DataBuffer> requestBodyFlux =
+        DataBufferUtils.read(
+                new InputStreamResource(request.getInputStream()), dataBufferFactory, 8192)
+            .doOnNext(DataBufferUtils.releaseConsumer());
+    WebClient.RequestHeadersSpec<?> requestHeadersSpec =
+        webClient
+            .method(httpMethod)
+            .uri(targetUri)
+            .httpRequest(
+                clientHttpRequest -> {
+                  Object nativeRequest = clientHttpRequest.getNativeRequest();
+                  if (nativeRequest instanceof HttpClientRequest httpClientRequest) {
+                    httpClientRequest.responseTimeout(
+                        Duration.ofSeconds(route.getResponseTimeoutSeconds()));
+                  }
+                })
+            .body(requestBodyFlux, DataBuffer.class);
+
+    for (Map.Entry<String, List<String>> header : headers.entrySet()) {
+      requestHeadersSpec =
+          requestHeadersSpec.header(header.getKey(), header.getValue().toArray(new String[0]));
+    }
+
     log.info(
         "Sending '{}' '{}' with route '{}' ('{}')",
         httpMethod,
         targetUri,
         route.getName(),
         route.getUid());
 
-    ResponseEntity<String> response =
-        restTemplate.exchange(targetUri, httpMethod, entity, String.class);
-
-    HttpHeaders responseHeaders = filterResponseHeaders(response.getHeaders());
+    WebClient.ResponseSpec responseSpec =
+        requestHeadersSpec
+            .retrieve()
+            .onStatus(httpStatusCode -> true, clientResponse -> Mono.empty());
 
-    String responseBody = response.getBody();
+    ResponseEntity<Flux<DataBuffer>> responseEntityFlux =
+        responseSpec
+            .toEntityFlux(DataBuffer.class)
+            .onErrorReturn(
+                throwable -> throwable.getCause() instanceof ReadTimeoutException,
+                new ResponseEntity<>(HttpStatus.GATEWAY_TIMEOUT))
+            .block();
 
     log.info(
         "Request '{}' '{}' responded with status '{}' for route '{}' ('{}')",
         httpMethod,
         targetUri,
-        response.getStatusCode(),
+        responseEntityFlux.getStatusCode(),
         route.getName(),
         route.getUid());
 
-    return new ResponseEntity<>(responseBody, responseHeaders, response.getStatusCode());
+    StreamingResponseBody streamingResponseBody =
+        out -> {
+          if (responseEntityFlux.getBody() != null) {
+            try {
+              Flux<DataBuffer> dataBufferFlux =
+                  DataBufferUtils.write(responseEntityFlux.getBody(), out)
+                      .doOnNext(DataBufferUtils.releaseConsumer());
+              dataBufferFlux.blockLast(Duration.ofMinutes(5));
+            } catch (Exception e) {
+              out.close();
+              throw e;
+            }
+          }
+        };
+    return new ResponseEntity<>(
+        streamingResponseBody,
+        filterResponseHeaders(responseEntityFlux.getHeaders()),
+        responseEntityFlux.getStatusCode());
   }
 
   /**

diff --git a/...services/dhis-service-core/src/main/resources/org/hisp/dhis/route/hibernate/Route.hbm.xml b/...services/dhis-service-core/src/main/resources/org/hisp/dhis/route/hibernate/Route.hbm.xml
@@ -39,5 +39,7 @@
         <!-- Dynamic attribute values -->
         <property name="attributeValues" column="attributevalues" type="jsbAttributeValues"/>
 
+        <property name="responseTimeoutSeconds" type="integer"/>
+
     </class>
 </hibernate-mapping>
diff --git a/...rc/main/resources/org/hisp/dhis/db/migration/2.42/V2_42_36__Add_responsetimeout_route.sql b/...rc/main/resources/org/hisp/dhis/db/migration/2.42/V2_42_36__Add_responsetimeout_route.sql
@@ -0,0 +1 @@
+ALTER TABLE route ADD COLUMN IF NOT EXISTS responsetimeoutseconds INTEGER NOT NULL DEFAULT 5 CHECK (responsetimeoutseconds > 0 AND responsetimeoutseconds <= 60);
diff --git a/...support/dhis-support-system/src/main/java/org/hisp/dhis/support/config/ServiceConfig.java b/...support/dhis-support-system/src/main/java/org/hisp/dhis/support/config/ServiceConfig.java
@@ -29,6 +29,8 @@
 
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
+import org.springframework.http.client.reactive.ClientHttpConnector;
+import org.springframework.http.client.reactive.ReactorClientHttpConnector;
 import org.springframework.web.client.RestTemplate;
 import org.springframework.web.util.UriComponentsBuilder;
 
@@ -42,6 +44,11 @@ public RestTemplate restTemplate() {
     return new RestTemplate();
   }
 
+  @Bean
+  public ClientHttpConnector clientHttpConnector() {
+    return new ReactorClientHttpConnector();
+  }
+
   @Bean
   public UriComponentsBuilder uriComponentsBuilder() {
     return UriComponentsBuilder.newInstance();

diff --git a/dhis-2/dhis-test-web-api/pom.xml b/dhis-2/dhis-test-web-api/pom.xml
@@ -361,6 +361,17 @@
         </exclusion>
       </exclusions>
     </dependency>
+    <dependency>
+      <groupId>org.testcontainers</groupId>
+      <artifactId>testcontainers</artifactId>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
+      <groupId>org.mock-server</groupId>
+      <artifactId>mockserver-client-java</artifactId>
+      <version>5.15.0</version>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
 
   <build>

diff --git a/...s-test-web-api/src/test/java/org/hisp/dhis/test/webapi/ControllerIntegrationTestBase.java b/...s-test-web-api/src/test/java/org/hisp/dhis/test/webapi/ControllerIntegrationTestBase.java
@@ -248,6 +248,13 @@ public String getHeader(String name) {
     }
   }
 
+  protected final MvcResult webRequestWithAsyncMvcResult(MockHttpServletRequestBuilder request) {
+    return exceptionAsFail(
+        () ->
+            mvc.perform(MockMvcRequestBuilders.asyncDispatch(webRequestWithMvcResult(request)))
+                .andReturn());
+  }
+
   protected final MvcResult webRequestWithMvcResult(MockHttpServletRequestBuilder request) {
     return exceptionAsFail(() -> mvc.perform(request.session(session)).andReturn());
   }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		ALTER TABLE route ADD COLUMN IF NOT EXISTS responsetimeoutseconds INTEGER NOT NULL DEFAULT 5 CHECK (responsetimeoutseconds > 0 AND responsetimeoutseconds <= 60);