Copilot commented on code in PR #16411:
URL: https://github.com/apache/pinot/pull/16411#discussion_r2227650594
##########
pinot-common/src/main/java/org/apache/pinot/common/segment/generation/SegmentGenerationUtils.java:
##########
@@ -225,21 +240,50 @@ public static URI getDirectoryURI(String uriStr)
}
/**
- * Retrieve a URL via GET request, with an optional authorization token.
+ * Retrieves the content of the given URL using a GET request.
+ * Supports HTTPS connections, including those with self-signed certificates.
*
- * @param url target url
- * @param authToken optional auth token, or null
- * @return fetched document
- * @throws IOException on connection problems
+ * @param url The target URL to fetch.
+ * @param authToken Optional authorization token to include in the request
header, or null.
+ * @return The response body as a string.
+ * @throws IOException If an error occurs during the connection or reading
the response.
*/
- private static String fetchUrl(URL url, String authToken)
+ public static String fetchUrl(URL url, String authToken, TlsSpec tlsSpec)
throws IOException {
- URLConnection connection = url.openConnection();
+ try {
+ URLConnection connection = url.openConnection();
+
+ if (connection instanceof HttpsURLConnection) {
+ HttpsURLConnection httpsConn = (HttpsURLConnection) connection;
+
+ if (tlsSpec != null) {
+ TrustManagerFactory tmf = TlsUtils.createTrustManagerFactory(
+ tlsSpec.getTrustStorePath(),
+ tlsSpec.getTrustStorePassword(),
+ tlsSpec.getTrustStoreType());
+
+ SSLContext sslContext = SSLContext.getInstance("TLS");
+ sslContext.init(null, tmf.getTrustManagers(), new SecureRandom());
+
+ httpsConn.setSSLSocketFactory(sslContext.getSocketFactory());
+ httpsConn.setConnectTimeout(tlsSpec.getConnectTimeout());
+ httpsConn.setReadTimeout(tlsSpec.getReadTimeout());
+ }
+ connection = httpsConn;
+ }
+
+ if (StringUtils.isNotBlank(authToken)) {
+ connection.setRequestProperty("Authorization", authToken);
+ }
+
+ if (connection instanceof HttpURLConnection) {
+ ((HttpURLConnection) connection).setRequestMethod("GET");
+ }
- if (StringUtils.isNotBlank(authToken)) {
- connection.setRequestProperty("Authorization", authToken);
+ return IOUtils.toString(connection.getInputStream(),
StandardCharsets.UTF_8);
+ } catch (Exception e) {
+ throw new IOException("Failed to fetch URL: " + url, e);
Review Comment:
Catching generic Exception is too broad. Consider catching specific
exceptions like KeyStoreException, NoSuchAlgorithmException,
KeyManagementException, and IOException separately to provide more targeted
error handling.
```suggestion
} catch (NoSuchAlgorithmException e) {
throw new IOException("Failed to initialize SSL context due to missing
algorithm: " + e.getMessage(), e);
} catch (KeyManagementException e) {
throw new IOException("Failed to initialize SSL context due to key
management issue: " + e.getMessage(), e);
} catch (IOException e) {
throw new IOException("I/O error occurred while fetching URL: " + url,
e);
} catch (RuntimeException e) {
throw new IOException("Unexpected runtime error occurred while
fetching URL: " + url, e);
```
##########
pinot-common/src/main/java/org/apache/pinot/common/segment/generation/SegmentGenerationUtils.java:
##########
@@ -225,21 +240,50 @@ public static URI getDirectoryURI(String uriStr)
}
/**
- * Retrieve a URL via GET request, with an optional authorization token.
+ * Retrieves the content of the given URL using a GET request.
+ * Supports HTTPS connections, including those with self-signed certificates.
*
- * @param url target url
- * @param authToken optional auth token, or null
- * @return fetched document
- * @throws IOException on connection problems
+ * @param url The target URL to fetch.
+ * @param authToken Optional authorization token to include in the request
header, or null.
+ * @return The response body as a string.
+ * @throws IOException If an error occurs during the connection or reading
the response.
Review Comment:
The method visibility changed from private to public without clear
justification. If this method needs to be public for testing or external use,
consider adding proper JavaDoc documentation to explain its public API contract.
##########
pinot-common/src/main/java/org/apache/pinot/common/segment/generation/SegmentGenerationUtils.java:
##########
@@ -225,21 +240,50 @@ public static URI getDirectoryURI(String uriStr)
}
/**
- * Retrieve a URL via GET request, with an optional authorization token.
+ * Retrieves the content of the given URL using a GET request.
+ * Supports HTTPS connections, including those with self-signed certificates.
*
- * @param url target url
- * @param authToken optional auth token, or null
- * @return fetched document
- * @throws IOException on connection problems
+ * @param url The target URL to fetch.
+ * @param authToken Optional authorization token to include in the request
header, or null.
+ * @return The response body as a string.
+ * @throws IOException If an error occurs during the connection or reading
the response.
*/
- private static String fetchUrl(URL url, String authToken)
+ public static String fetchUrl(URL url, String authToken, TlsSpec tlsSpec)
throws IOException {
- URLConnection connection = url.openConnection();
+ try {
+ URLConnection connection = url.openConnection();
+
+ if (connection instanceof HttpsURLConnection) {
+ HttpsURLConnection httpsConn = (HttpsURLConnection) connection;
+
+ if (tlsSpec != null) {
+ TrustManagerFactory tmf = TlsUtils.createTrustManagerFactory(
+ tlsSpec.getTrustStorePath(),
+ tlsSpec.getTrustStorePassword(),
+ tlsSpec.getTrustStoreType());
+
+ SSLContext sslContext = SSLContext.getInstance("TLS");
+ sslContext.init(null, tmf.getTrustManagers(), new SecureRandom());
+
+ httpsConn.setSSLSocketFactory(sslContext.getSocketFactory());
+ httpsConn.setConnectTimeout(tlsSpec.getConnectTimeout());
+ httpsConn.setReadTimeout(tlsSpec.getReadTimeout());
+ }
+ connection = httpsConn;
+ }
+
Review Comment:
[nitpick] The entire method body is wrapped in a try-catch block, making the
control flow less clear. Consider restructuring to handle specific SSL-related
exceptions separately from the general URL connection logic.
```suggestion
URLConnection connection;
try {
connection = url.openConnection();
if (connection instanceof HttpsURLConnection) {
HttpsURLConnection httpsConn = (HttpsURLConnection) connection;
if (tlsSpec != null) {
try {
TrustManagerFactory tmf = TlsUtils.createTrustManagerFactory(
tlsSpec.getTrustStorePath(),
tlsSpec.getTrustStorePassword(),
tlsSpec.getTrustStoreType());
SSLContext sslContext = SSLContext.getInstance("TLS");
sslContext.init(null, tmf.getTrustManagers(), new
SecureRandom());
httpsConn.setSSLSocketFactory(sslContext.getSocketFactory());
httpsConn.setConnectTimeout(tlsSpec.getConnectTimeout());
httpsConn.setReadTimeout(tlsSpec.getReadTimeout());
} catch (GeneralSecurityException e) {
throw new IOException("Failed to configure SSL for URL: " + url,
e);
}
}
connection = httpsConn;
}
} catch (IOException e) {
throw new IOException("Failed to open connection to URL: " + url, e);
}
try {
```
--
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]
