Copilot commented on code in PR #19822:
URL: https://github.com/apache/druid/pull/19822#discussion_r3685098352
##########
extensions-contrib/graphite-emitter/src/test/java/org/apache/druid/emitter/graphite/WhiteListBasedConverterTest.java:
##########
@@ -98,17 +104,17 @@ public void testGetPath(ServiceMetricEvent
serviceMetricEvent, String expectedPa
@Test
public void testWhiteListedStringArrayDimension() throws IOException
{
- File mapFile = File.createTempFile("testing-" + System.nanoTime(),
".json");
- mapFile.deleteOnExit();
-
- try (OutputStream outputStream = new FileOutputStream(mapFile)) {
- IOUtils.copyLarge(
-
getClass().getResourceAsStream("/testWhiteListedStringArrayDimension.json"),
- outputStream
- );
+ final File mapFile = temporaryFolder.newFile("whiteList.json");
+
+ try (
+ InputStream inputStream =
+
WhiteListBasedConverterTest.class.getResourceAsStream("/testWhiteListedStringArrayDimension.json");
+ OutputStream outputStream = new FileOutputStream(mapFile)
+ ) {
+ IOUtils.copyLarge(inputStream, outputStream);
}
Review Comment:
The resource InputStream can be null if the test resource is missing, which
would cause a NullPointerException in IOUtils.copyLarge. Since this code is
already touching resource lookup, fail fast with a clear message by requiring
the resource stream to be non-null.
##########
extensions-contrib/ambari-metrics-emitter/src/test/java/org/apache/druid/emitter/ambari/metrics/WhiteListBasedDruidToTimelineEventConverterTest.java:
##########
@@ -93,29 +99,31 @@ public void testGetName(ServiceMetricEvent
serviceMetricEvent, String expectedPa
@Test
public void testWhiteListedStringArrayDimension() throws IOException
{
- File mapFile = File.createTempFile("testing-" + System.nanoTime(),
".json");
- mapFile.deleteOnExit();
+ final File mapFile = temporaryFolder.newFile("whiteList.json");
- try (OutputStream outputStream = new FileOutputStream(mapFile)) {
- IOUtils.copyLarge(
-
getClass().getResourceAsStream("/testWhiteListedStringArrayDimension.json"),
- outputStream
- );
+ try (
+ InputStream inputStream =
+ WhiteListBasedDruidToTimelineEventConverterTest.class
+
.getResourceAsStream("/testWhiteListedStringArrayDimension.json");
+ OutputStream outputStream = new FileOutputStream(mapFile)
+ ) {
+ IOUtils.copyLarge(inputStream, outputStream);
}
Review Comment:
The resource InputStream can be null if the test resource is missing, which
would cause a NullPointerException in IOUtils.copyLarge. Consider requiring the
resource stream to be non-null so failures are explicit and easier to diagnose.
##########
sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java:
##########
@@ -1540,14 +1541,12 @@ private void outprint(Object post)
public File getResourceAsTemporaryFile(final String resource)
{
final File file = newTempFile("resourceAsTempFile");
- final InputStream stream = getClass().getResourceAsStream(resource);
-
- if (stream == null) {
- throw new RE(StringUtils.format("No such resource [%s]", resource));
- }
-
- try {
- ByteStreams.copy(stream, Files.newOutputStream(file.toPath()));
+ try (InputStream stream =
BaseCalciteQueryTest.class.getResourceAsStream(resource);
+ OutputStream outputStream = Files.newOutputStream(file.toPath())) {
+ if (stream == null) {
+ throw new RE(StringUtils.format("No such resource [%s]", resource));
+ }
+ ByteStreams.copy(stream, outputStream);
}
Review Comment:
The OutputStream is opened before verifying that the resource stream exists.
If the resource is missing, this method throws but still creates an empty temp
file, which is a surprising side effect for a lookup failure. Open the output
stream only after confirming the resource stream is non-null.
--
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]
