Copilot commented on code in PR #17285:
URL: https://github.com/apache/pinot/pull/17285#discussion_r2572952442
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/tls/RenewableTlsUtilsTest.java:
##########
@@ -346,24 +347,41 @@ private TlsConfig createTlsConfig() {
return tlsConfig;
}
+ private void waitForTlsMaterialChange(SSLFactory sslFactory, PrivateKey
privateKey, Certificate certForPrivateKey,
+ X509Certificate acceptedIssuerForCert)
+ throws InterruptedException {
+ long deadline = System.currentTimeMillis() + SSL_FACTORY_RELOAD_TIMEOUT_MS;
+ while (System.currentTimeMillis() < deadline) {
+ X509ExtendedKeyManager keyManager =
sslFactory.getKeyManager().orElseThrow();
+ X509ExtendedTrustManager trustManager =
sslFactory.getTrustManager().orElseThrow();
+ boolean keyChanged =
!privateKey.equals(keyManager.getPrivateKey(KEY_NAME_ALIAS));
+ boolean certChanged =
!certForPrivateKey.equals(keyManager.getCertificateChain(KEY_NAME_ALIAS)[0]);
+ boolean issuerChanged =
!acceptedIssuerForCert.equals(trustManager.getAcceptedIssuers()[0]);
Review Comment:
Potential `NullPointerException` if `getPrivateKey`, `getCertificateChain`,
or `getAcceptedIssuers` returns null or an empty array. Add null/empty checks
before accessing array elements and comparing values to prevent test failures.
```suggestion
X509Certificate[] acceptedIssuers = trustManager.getAcceptedIssuers();
if (acceptedIssuers == null || acceptedIssuers.length == 0) {
fail("TrustManager.getAcceptedIssuers() returned null or empty
array");
}
boolean issuerChanged =
!acceptedIssuerForCert.equals(acceptedIssuers[0]);
```
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/tls/RenewableTlsUtilsTest.java:
##########
@@ -109,8 +111,6 @@ private static void
copyResourceFilesToTempFolder(Map<String, String> srcAndDest
Path destinationPath = Paths.get(DEFAULT_TEST_TLS_DIR,
entry.getValue());
// Use Files.copy to copy the file to the destination folder
Files.copy(resourceStream, destinationPath,
StandardCopyOption.REPLACE_EXISTING);
- } catch (IOException e) {
- e.printStackTrace(); // Handle the exception as needed
}
Review Comment:
Removed catch block without addressing exception handling. The `Files.copy`
operation can throw `IOException`, but the method signature doesn't declare
this exception. Either add `throws IOException` to the method signature or
restore exception handling.
--
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]
