chore(bq jdbc): fix p12 unittest resource access#12294
chore(bq jdbc): fix p12 unittest resource access#12294
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates BigQueryJdbcOAuthUtilityTest to load test resources using getResourceAsStream instead of file paths. While this change improves resource accessibility, the current implementation introduces resource leaks by failing to close the InputStream and uses an unreliable fixed-size buffer for reading stream data. The review feedback recommends using try-with-resources and a more robust reading strategy, such as a ByteArrayOutputStream, to ensure data integrity and proper resource management.
| InputStream stream = BigQueryJdbcOAuthUtilityTest.class.getResourceAsStream("/fake.p12"); | ||
| try { | ||
| PrivateKey pk = | ||
| BigQueryJdbcOAuthUtility.privateKeyFromP12Bytes( | ||
| Files.readAllBytes(Paths.get(resource.toURI())), "notasecret"); | ||
| int size = 16 * 1024; | ||
| byte[] data = new byte[size]; | ||
| stream.read(data, 0, data.length); | ||
| PrivateKey pk = BigQueryJdbcOAuthUtility.privateKeyFromP12Bytes(data, "notasecret"); | ||
| assertNotNull(pk); | ||
| } catch (Exception e) { | ||
| assertTrue(false); |
There was a problem hiding this comment.
This implementation has a couple of issues: it leaks the InputStream and reads from it incorrectly.
- Resource Leak: The
InputStreamis not closed. Using a try-with-resources statement is the standard way to prevent this. - Incorrect Stream Reading: Reading into a fixed-size buffer is unreliable.
read()doesn't guarantee to fill the buffer, and if the file size doesn't match the buffer size, you'll either process incomplete data or data with extra null bytes.
The suggestion below fixes these issues by using a try-with-resources block and reading the entire stream into a ByteArrayOutputStream. It also improves the error handling in the catch block.
You will need to add import java.io.ByteArrayOutputStream;.
try (InputStream stream =
BigQueryJdbcOAuthUtilityTest.class.getResourceAsStream("/fake.p12")) {
assertNotNull("Resource stream for /fake.p12 should not be null", stream);
// In Java 9+ you can use stream.readAllBytes(). For Java 8, this is a common pattern.
java.io.ByteArrayOutputStream baos = new java.io.ByteArrayOutputStream();
byte[] buffer = new byte[4096];
int nRead;
while ((nRead = stream.read(buffer, 0, buffer.length)) != -1) {
baos.write(buffer, 0, nRead);
}
PrivateKey pk =
BigQueryJdbcOAuthUtility.privateKeyFromP12Bytes(baos.toByteArray(), "notasecret");
assertNotNull(pk);
} catch (Exception e) {
fail("Failed to get private key from p12 bytes: " + e.getMessage());
}| InputStream stream = BigQueryJdbcOAuthUtilityTest.class.getResourceAsStream("/fake.p12"); | ||
| try { | ||
| PrivateKey pk = | ||
| BigQueryJdbcOAuthUtility.privateKeyFromP12Bytes( | ||
| Files.readAllBytes(Paths.get(resource.toURI())), "fake"); | ||
| int size = 16 * 1024; | ||
| byte[] data = new byte[size]; | ||
| stream.read(data, 0, data.length); | ||
| PrivateKey pk = BigQueryJdbcOAuthUtility.privateKeyFromP12Bytes(data, "fake"); | ||
| assertNull(pk); | ||
| } catch (Exception e) { | ||
| assertTrue(false); |
There was a problem hiding this comment.
This method has the same issues as testPrivateKeyFromP12Bytes regarding resource handling and stream reading. The InputStream is not closed, and reading into a fixed-size buffer is unreliable.
Please apply a similar fix here using a try-with-resources statement and reading the full stream content. This ensures the test is robust and doesn't leak resources.
try (InputStream stream =
BigQueryJdbcOAuthUtilityTest.class.getResourceAsStream("/fake.p12")) {
assertNotNull("Resource stream for /fake.p12 should not be null", stream);
java.io.ByteArrayOutputStream baos = new java.io.ByteArrayOutputStream();
byte[] buffer = new byte[4096];
int nRead;
while ((nRead = stream.read(buffer, 0, buffer.length)) != -1) {
baos.write(buffer, 0, nRead);
}
PrivateKey pk = BigQueryJdbcOAuthUtility.privateKeyFromP12Bytes(baos.toByteArray(), "fake");
assertNull(pk);
} catch (Exception e) {
fail("Test should not have thrown an exception for wrong password: " + e.getMessage());
}
No description provided.