Copilot commented on code in PR #7904:
URL: https://github.com/apache/incubator-seata/pull/7904#discussion_r2647179510
##########
discovery/seata-discovery-raft/src/main/java/org/apache/seata/discovery/registry/raft/RaftRegistryServiceImpl.java:
##########
@@ -498,12 +495,12 @@ private static void acquireClusterMetaData(String
clusterName, String group) thr
Map<String, String> param = new HashMap<>();
param.put("group", group);
String response = null;
- try (CloseableHttpResponse httpResponse =
+ try (Response httpResponse =
HttpClientUtil.doGet("http://"; + tcAddress +
"/metadata/v1/cluster", param, header, 1000)) {
if (httpResponse != null) {
- int statusCode =
httpResponse.getStatusLine().getStatusCode();
+ int statusCode = httpResponse.code();
if (statusCode == HttpStatus.SC_OK) {
- response =
EntityUtils.toString(httpResponse.getEntity(), StandardCharsets.UTF_8);
+ response = httpResponse.body().string();
Review Comment:
Potential NullPointerException when calling response.body().string(). The
OkHttp Response.body() method can return null. While the response object itself
is checked for null, the body should also be checked before calling string() to
avoid NPE.
```suggestion
if (httpResponse.body() != null) {
response = httpResponse.body().string();
} else {
LOGGER.warn("Received HTTP 200 from {} but
response body is null for group: {}", tcAddress, group);
}
```
##########
discovery/seata-discovery-namingserver/src/main/java/org/apache/seata/discovery/registry/namingserver/NamingserverRegistryServiceImpl.java:
##########
@@ -576,11 +572,11 @@ private static void refreshToken(String
namingServerAddress) throws RetryableExc
Map<String, String> header = new HashMap<>();
header.put(HTTP.CONTENT_TYPE,
ContentType.APPLICATION_JSON.getMimeType());
String response = null;
- try (CloseableHttpResponse httpResponse =
+ try (Response httpResponse =
HttpClientUtil.doPost("http://"; + namingServerAddress +
"/api/v1/auth/login", param, header, 1000)) {
if (httpResponse != null) {
- if (httpResponse.getStatusLine().getStatusCode() ==
HttpStatus.SC_OK) {
- response = EntityUtils.toString(httpResponse.getEntity(),
StandardCharsets.UTF_8);
+ if (httpResponse.code() == HttpStatus.SC_OK) {
Review Comment:
Potential NullPointerException when calling response.body().string(). The
OkHttp Response.body() method can return null. While the response object itself
is checked for null, the body should also be checked before calling string() to
avoid NPE.
```suggestion
if (httpResponse.code() == HttpStatus.SC_OK) {
if (httpResponse.body() == null) {
throw new RetryableException("Empty response body
from naming server auth login.");
}
```
##########
common/src/main/java/org/apache/seata/common/util/HttpClientUtil.java:
##########
@@ -305,6 +219,24 @@ private static OkHttpClient
createHttp2ClientWithTimeout(int timeoutSeconds) {
.build());
}
+ private static Request buildHttp1Request(
+ String url, Map<String, String> headers, RequestBody requestBody,
String method) {
+ Headers.Builder headerBuilder = new Headers.Builder();
+ if (headers != null) {
+ headers.forEach(headerBuilder::add);
+ }
+
+ Request.Builder requestBuilder = new
Request.Builder().url(url).headers(headerBuilder.build());
+
+ if ("POST".equals(method) && requestBody != null) {
+ requestBuilder.post(requestBody);
+ } else if ("GET".equals(method)) {
+ requestBuilder.get();
+ }
+
+ return requestBuilder.build();
Review Comment:
When the method is "POST" but requestBody is null, the request builder
doesn't call post() and falls through to build() which creates a GET request by
default in OkHttp. This can happen when doPost is called with a null body
parameter. The method should either provide an empty RequestBody for POST
requests or throw an exception to prevent silent behavior change.
##########
common/src/main/java/org/apache/seata/common/util/HttpClientUtil.java:
##########
@@ -323,6 +255,32 @@ private static Request buildHttp2Request(
return requestBuilder.build();
}
+ private static String buildUrlWithParams(String url, Map<String, String>
params) {
+ if (params == null || params.isEmpty()) {
+ return url;
+ }
+ StringBuilder urlBuilder = new StringBuilder(url);
+ boolean first = !url.contains("?");
+ for (Map.Entry<String, String> entry : params.entrySet()) {
+ if (first) {
+ urlBuilder.append("?");
+ first = false;
+ } else {
+ urlBuilder.append("&");
+ }
+ try {
+ urlBuilder
+ .append(URLEncoder.encode(entry.getKey(),
StandardCharsets.UTF_8.toString()))
+ .append("=")
+ .append(URLEncoder.encode(entry.getValue(),
StandardCharsets.UTF_8.toString()));
Review Comment:
The URLEncoder.encode() method with String parameter is deprecated as of
Java 10. Use URLEncoder.encode(String, Charset) or URLEncoder.encode(String,
String) without calling toString() on the Charset. The current code is
unnecessarily converting StandardCharsets.UTF_8 to a String.
```suggestion
.append(URLEncoder.encode(entry.getKey(),
StandardCharsets.UTF_8.name()))
.append("=")
.append(URLEncoder.encode(entry.getValue(),
StandardCharsets.UTF_8.name()));
```
##########
discovery/seata-discovery-namingserver/src/main/java/org/apache/seata/discovery/registry/namingserver/NamingserverRegistryServiceImpl.java:
##########
@@ -438,12 +434,12 @@ public List<InetSocketAddress> refreshGroup(String
vGroup) throws IOException, R
if (StringUtils.isNotBlank(jwtToken)) {
header.put(AUTHORIZATION_HEADER, jwtToken);
}
- try (CloseableHttpResponse response = HttpClientUtil.doGet(url,
paraMap, header, 3000)) {
- if (response == null || response.getStatusLine().getStatusCode()
!= HttpStatus.SC_OK) {
+ try (Response response = HttpClientUtil.doGet(url, paraMap, header,
3000)) {
+ if (response == null || response.code() != HttpStatus.SC_OK) {
throw new NamingRegistryException("cannot lookup server list
in vgroup: " + vGroup + ", http code: "
- + response.getStatusLine().getStatusCode());
+ + (response != null ? response.code() : -1));
}
- String jsonResponse = EntityUtils.toString(response.getEntity(),
StandardCharsets.UTF_8);
+ String jsonResponse = response.body().string();
Review Comment:
Potential NullPointerException when calling response.body().string(). The
OkHttp Response.body() method can return null. While the response object itself
is checked for null, the body should also be checked before calling string() to
avoid NPE.
##########
discovery/seata-discovery-raft/src/main/java/org/apache/seata/discovery/registry/raft/RaftRegistryServiceImpl.java:
##########
@@ -544,11 +541,11 @@ private static void refreshToken(String tcAddress) throws
RetryableException {
Map<String, String> header = new HashMap<>();
header.put(HTTP.CONTENT_TYPE,
ContentType.APPLICATION_JSON.getMimeType());
String response = null;
- try (CloseableHttpResponse httpResponse =
+ try (Response httpResponse =
HttpClientUtil.doPost("http://"; + tcAddress +
"/api/v1/auth/login", param, header, 1000)) {
if (httpResponse != null) {
- if (httpResponse.getStatusLine().getStatusCode() ==
HttpStatus.SC_OK) {
- response = EntityUtils.toString(httpResponse.getEntity(),
StandardCharsets.UTF_8);
+ if (httpResponse.code() == HttpStatus.SC_OK) {
+ response = httpResponse.body().string();
Review Comment:
Potential NullPointerException when calling response.body().string(). The
OkHttp Response.body() method can return null. While the response object itself
is checked for null, the body should also be checked before calling string() to
avoid NPE.
```suggestion
okhttp3.ResponseBody responseBody = httpResponse.body();
if (responseBody == null) {
// authorized failed,throw exception to kill process
throw new AuthenticationFailedException(
"Authentication failed! empty response body,
please check the server configuration.");
}
response = responseBody.string();
```
##########
common/src/main/java/org/apache/seata/common/util/HttpClientUtil.java:
##########
@@ -295,6 +200,15 @@ private static RequestBody createRequestBody(Map<String,
String> params, String
}
}
+ private static OkHttpClient createHttp1ClientWithTimeout(int
timeoutSeconds) {
+ return HTTP_CLIENT_MAP.computeIfAbsent(timeoutSeconds, k -> new
OkHttpClient.Builder()
+ // Use HTTP/1.1 (default protocol, no need to specify)
+ .connectTimeout(timeoutSeconds, TimeUnit.SECONDS)
+ .readTimeout(timeoutSeconds, TimeUnit.SECONDS)
+ .writeTimeout(timeoutSeconds, TimeUnit.SECONDS)
+ .build());
+ }
Review Comment:
The timeout parameter is being interpreted as seconds, but all callers are
passing values in milliseconds (e.g., 1000, 3000, 30000). This creates
extremely long timeouts that don't match the original Apache HttpClient
behavior. The parameter should either be converted from milliseconds to
seconds, or the TimeUnit should be changed to MILLISECONDS to maintain backward
compatibility with the original API.
##########
common/pom.xml:
##########
@@ -43,11 +43,11 @@
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
</dependency>
- <dependency>
+ <!--<dependency>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpclient</artifactId>
<scope>provided</scope>
- </dependency>
+ </dependency>-->
Review Comment:
The httpclient dependency is commented out but should be completely removed
if it's no longer needed. Leaving commented-out code in production can cause
confusion about whether it should be restored or removed permanently. Consider
removing it entirely or documenting why it's being kept.
##########
common/src/main/java/org/apache/seata/common/util/HttpClientUtil.java:
##########
@@ -107,130 +93,49 @@ public class HttpClientUtil {
}
// post request
- public static CloseableHttpResponse doPost(
- String url, Map<String, String> params, Map<String, String>
header, int timeout) throws IOException {
+ public static Response doPost(String url, Map<String, String> params,
Map<String, String> header, int timeout)
+ throws IOException {
try {
- URIBuilder builder = new URIBuilder(url);
- URI uri = builder.build();
- HttpPost httpPost = new HttpPost(uri);
- String contentType = "";
- if (header != null) {
- header.forEach(httpPost::addHeader);
- contentType = header.get("Content-Type");
- }
- if (StringUtils.isNotBlank(contentType)) {
- if
(ContentType.APPLICATION_FORM_URLENCODED.getMimeType().equals(contentType)) {
- List<NameValuePair> nameValuePairs = new ArrayList<>();
- params.forEach((k, v) -> {
- nameValuePairs.add(new BasicNameValuePair(k, v));
- });
- String requestBody =
URLEncodedUtils.format(nameValuePairs, StandardCharsets.UTF_8);
- StringEntity stringEntity = new StringEntity(requestBody,
ContentType.APPLICATION_FORM_URLENCODED);
- httpPost.setEntity(stringEntity);
- } else if
(ContentType.APPLICATION_JSON.getMimeType().equals(contentType)) {
- String requestBody =
OBJECT_MAPPER.writeValueAsString(params);
- StringEntity stringEntity = new StringEntity(requestBody,
ContentType.APPLICATION_JSON);
- httpPost.setEntity(stringEntity);
- }
- }
- CloseableHttpClient client =
HTTP_CLIENT_MAP.computeIfAbsent(timeout, k -> HttpClients.custom()
-
.setConnectionManager(POOLING_HTTP_CLIENT_CONNECTION_MANAGER)
- .setDefaultRequestConfig(RequestConfig.custom()
- .setConnectionRequestTimeout(timeout)
- .setSocketTimeout(timeout)
- .setConnectTimeout(timeout)
- .build())
- .build());
- return client.execute(httpPost);
- } catch (URISyntaxException | ClientProtocolException e) {
+ String contentType = header != null ? header.get("Content-Type") :
"";
+ RequestBody requestBody = createRequestBody(params, contentType);
+ Request request = buildHttp1Request(url, header, requestBody,
"POST");
+ OkHttpClient client = createHttp1ClientWithTimeout(timeout);
+ return client.newCall(request).execute();
+ } catch (JsonProcessingException e) {
LOGGER.error(e.getMessage(), e);
+ throw new IOException("Failed to create request body", e);
}
- return null;
}
// post request
- public static CloseableHttpResponse doPost(String url, String body,
Map<String, String> header, int timeout)
- throws IOException {
- try {
- URIBuilder builder = new URIBuilder(url);
- URI uri = builder.build();
- HttpPost httpPost = new HttpPost(uri);
- String contentType = "";
- if (header != null) {
- header.forEach(httpPost::addHeader);
- contentType = header.get("Content-Type");
- }
- if (StringUtils.isNotBlank(contentType)) {
- if
(ContentType.APPLICATION_JSON.getMimeType().equals(contentType)) {
- StringEntity stringEntity = new StringEntity(body,
ContentType.APPLICATION_JSON);
- httpPost.setEntity(stringEntity);
- }
- }
- CloseableHttpClient client =
HTTP_CLIENT_MAP.computeIfAbsent(timeout, k -> HttpClients.custom()
-
.setConnectionManager(POOLING_HTTP_CLIENT_CONNECTION_MANAGER)
- .setDefaultRequestConfig(RequestConfig.custom()
- .setConnectionRequestTimeout(timeout)
- .setSocketTimeout(timeout)
- .setConnectTimeout(timeout)
- .build())
- .build());
- return client.execute(httpPost);
- } catch (URISyntaxException | ClientProtocolException e) {
- LOGGER.error(e.getMessage(), e);
- }
- return null;
+ public static Response doPost(String url, String body, Map<String, String>
header, int timeout) throws IOException {
+ String contentType = header != null ? header.get("Content-Type") : "";
+ MediaType mediaType = StringUtils.isNotBlank(contentType) ?
MediaType.parse(contentType) : MEDIA_TYPE_JSON;
+ RequestBody requestBody = RequestBody.create(body, mediaType);
Review Comment:
Calling RequestBody.create() with a null body string will cause a
NullPointerException in OkHttp. The method should handle null body parameter by
either creating an empty request body or throwing an appropriate exception with
a clear error message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
