Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat : upgrade to springboot 3.4 #1480

Merged
merged 3 commits into from
Nov 1, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions httpClients/boot-restclient/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<parent>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-parent</artifactId>
<version>3.3.5</version>
<version>3.4.0-RC1</version>
<relativePath/> <!-- lookup parent from repository -->
</parent>
<groupId>com.example.restclient</groupId>
Expand Down Expand Up @@ -72,7 +72,7 @@
<configuration>
<java>
<googleJavaFormat>
<version>1.22.0</version>
<version>1.24.0</version>
<style>AOSP</style>
</googleJavaFormat>
<importOrder/>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
package com.example.restclient.bootrestclient.config;

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpRequest;
import org.springframework.http.HttpStatusCode;
import org.springframework.http.client.ClientHttpRequestExecution;
import org.springframework.http.client.ClientHttpRequestInterceptor;
import org.springframework.http.client.ClientHttpResponse;
import org.springframework.stereotype.Component;

@Component
public class ClientLoggerRequestInterceptor implements ClientHttpRequestInterceptor {

private static final Logger log = LoggerFactory.getLogger(ClientLoggerRequestInterceptor.class);

@Override
public ClientHttpResponse intercept(
HttpRequest request, byte[] body, ClientHttpRequestExecution execution)
throws IOException {
logRequest(request, body);
ClientHttpResponse response = execution.execute(request, body);
return logResponse(response);
}

private BufferingClientHttpResponseWrapper logResponse(ClientHttpResponse response)
throws IOException {
log.info(
"============================response begin==========================================");
log.info("Status code : {}", response.getStatusCode());
log.info("Status text : {}", response.getStatusText());
logHeaders(response.getHeaders());
byte[] responseBody = response.getBody().readAllBytes();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure response body stream is not consumed

Calling response.getBody().readAllBytes() consumes the input stream, which may cause issues for downstream processing that expects to read from the response body.

To prevent consuming the response body stream, consider wrapping the response without reading and consuming the stream upfront.

if (responseBody.length > 0) {
log.info("Response body: {}", new String(responseBody, StandardCharsets.UTF_8));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid logging sensitive information in response bodies

Logging the response body can expose sensitive data returned by the server. This poses a security risk if the logs are compromised.

Apply this diff to remove logging of the response body:

-        if (responseBody.length > 0) {
-            log.info("Response body: {}", new String(responseBody, StandardCharsets.UTF_8));
-        }

Committable suggestion skipped: line range outside the PR's diff.

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle potential memory issues with large response bodies

Reading the entire response body into memory using readAllBytes() can lead to OutOfMemoryError if the response is large. This may affect the application's performance and stability.

Consider limiting the size of the response body that is read and logged, or stream the response body instead of loading it entirely into memory.

log.info(
"=======================response end=================================================");
return new BufferingClientHttpResponseWrapper(response, responseBody);
}

private void logRequest(HttpRequest request, byte[] body) {

log.info(
"===========================request begin================================================");
log.info("URI : {}", request.getURI());
log.info("Method : {}", request.getMethod());
logHeaders(request.getHeaders());
if (body.length > 0) {
log.info("Request body: {}", new String(body, StandardCharsets.UTF_8));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid logging sensitive information in request bodies

Logging the request body may expose sensitive data such as credentials or personal information. It's a security risk to log request bodies without sanitization.

Apply this diff to remove logging of the request body:

-        if (body.length > 0) {
-            log.info("Request body: {}", new String(body, StandardCharsets.UTF_8));
-        }

Committable suggestion skipped: line range outside the PR's diff.

}
log.info(
"==========================request end================================================");
}

private void logHeaders(HttpHeaders headers) {
log.info("Headers : ");
headers.forEach((name, values) -> values.forEach(value -> log.info("{}={}", name, value)));
}

private static class BufferingClientHttpResponseWrapper implements ClientHttpResponse {
private final ClientHttpResponse response;
private final byte[] body;

public BufferingClientHttpResponseWrapper(ClientHttpResponse response, byte[] body) {
this.response = response;
this.body = body;
}
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Reuse existing classes for response buffering

Spring provides a BufferingClientHttpResponseWrapper class that can be used instead of implementing a custom wrapper. This reduces code duplication and leverages existing, well-tested functionality.

Consider replacing the custom BufferingClientHttpResponseWrapper with Spring's existing class.

Apply this diff:

-    private static class BufferingClientHttpResponseWrapper implements ClientHttpResponse {
-        // Custom implementation
-    }
+    // Remove custom wrapper and use Spring's BufferingClientHttpResponseWrapper

And modify the logResponse method accordingly.

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai, Spring's BufferingClientHttpResponseWrapper is not a public class, how to replace custom code with Spring's BufferingClientHttpResponseWrapper?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajadilipkolli, thank you for pointing that out. You are correct that Spring's BufferingClientHttpResponseWrapper is not a public class and cannot be used directly. In this case, your custom implementation is appropriate.


✏️ Learnings added
Learnt from: rajadilipkolli
PR: rajadilipkolli/my-spring-boot-experiments#1480
File: httpClients/boot-restclient/src/main/java/com/example/restclient/bootrestclient/config/ClientLoggerRequestInterceptor.java:70-73
Timestamp: 2024-11-01T10:48:43.983Z
Learning: Spring's `BufferingClientHttpResponseWrapper` is not public and cannot be used directly outside of the `org.springframework.http` package. Therefore, custom implementations are necessary when response buffering is needed in `ClientHttpResponse` interceptors.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


@Override
public InputStream getBody() throws IOException {
return new ByteArrayInputStream(body);
}

@Override
public HttpHeaders getHeaders() {
return response.getHeaders();
}

@Override
public HttpStatusCode getStatusCode() throws IOException {
return response.getStatusCode();
}

@Override
public int getRawStatusCode() throws IOException {
return response.getRawStatusCode();
}

@Override
public String getStatusText() throws IOException {
return response.getStatusText();
}

@Override
public void close() {
response.close();
}
}
}
Original file line number Diff line number Diff line change
@@ -1,23 +1,12 @@
package com.example.restclient.bootrestclient.config;

import java.io.IOException;
import java.net.http.HttpClient;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.time.Duration;
import java.util.List;
import lombok.extern.slf4j.Slf4j;
import org.springframework.boot.web.client.RestClientCustomizer;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.http.HttpRequest;
import org.springframework.http.MediaType;
import org.springframework.http.client.BufferingClientHttpRequestFactory;
import org.springframework.http.client.ClientHttpResponse;
import org.springframework.http.client.JdkClientHttpRequestFactory;
import org.springframework.lang.NonNull;
import org.springframework.retry.annotation.EnableRetry;
import org.springframework.util.StreamUtils;
import org.springframework.web.client.RestClient;
import org.springframework.web.util.DefaultUriBuilderFactory;

Expand All @@ -34,7 +23,7 @@ RestClient restClient(RestClient.Builder restClientBuilder) {
@Bean
RestClientCustomizer restClientCustomizer(
ApplicationProperties applicationProperties,
@NonNull BufferingClientHttpRequestFactory bufferingClientHttpRequestFactory) {
ClientLoggerRequestInterceptor clientLoggerRequestInterceptor) {
DefaultUriBuilderFactory factory =
new DefaultUriBuilderFactory(applicationProperties.getExternalCallUrl());
factory.setEncodingMode(DefaultUriBuilderFactory.EncodingMode.URI_COMPONENT);
Expand All @@ -45,64 +34,7 @@ RestClientCustomizer restClientCustomizer(
httpHeaders -> {
httpHeaders.setContentType(MediaType.APPLICATION_JSON);
httpHeaders.setAccept(List.of(MediaType.APPLICATION_JSON));
});
// .requestFactory(bufferingClientHttpRequestFactory)
// .requestInterceptor(
// (request, body, execution) -> {
// logRequest(request, body);
// ClientHttpResponse response = execution.execute(request, body);
// logResponse(response);
// return response;
// });
}

private void logResponse(ClientHttpResponse response) throws IOException {
log.info(
"============================response begin==========================================");
log.info("Status code : {}", response.getStatusCode());
log.info("Status text : {}", response.getStatusText());
log.info("Headers : {}", response.getHeaders());
log.info(
"Response body: {}",
StreamUtils.copyToString(response.getBody(), Charset.defaultCharset()));
log.info(
"=======================response end=================================================");
}

private void logRequest(HttpRequest request, byte[] body) {

log.info(
"===========================request begin================================================");
log.info("URI : {}", request.getURI());
log.info("Method : {}", request.getMethod());
log.info("Headers : {}", request.getHeaders());
log.info("Request body: {}", new String(body, StandardCharsets.UTF_8));
log.info(
"==========================request end================================================");
}

@Bean
HttpClient jdkClient() {
return HttpClient.newBuilder()
.version(HttpClient.Version.HTTP_2)
.connectTimeout(Duration.ofSeconds(30))
.followRedirects(HttpClient.Redirect.NORMAL)
.build();
}

@Bean
JdkClientHttpRequestFactory jdkClientHttpRequestFactory(@NonNull HttpClient jdkClient) {
JdkClientHttpRequestFactory jdkClientHttpRequestFactory =
new JdkClientHttpRequestFactory(jdkClient);
jdkClientHttpRequestFactory.setReadTimeout(Duration.ofSeconds(60));
return jdkClientHttpRequestFactory;
}

// BufferingClientHttpRequestFactory allows us to read the response body multiple times for a
// single request.
@Bean
BufferingClientHttpRequestFactory bufferingClientHttpRequestFactory(
@NonNull JdkClientHttpRequestFactory jdkClientHttpRequestFactory) {
return new BufferingClientHttpRequestFactory(jdkClientHttpRequestFactory);
})
.requestInterceptor(clientLoggerRequestInterceptor);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,7 @@ spring.mvc.problemdetails.enabled=true
spring.threads.virtual.enabled=true

application.external-call-url=https://jsonplaceholder.typicode.com

spring.http.client.factory=jdk
spring.http.client.connect-timeout=PT30S
spring.http.client.read-timeout=PT1M
Loading