Cleanup read and write bytes in test & pom improvements#727
Cleanup read and write bytes in test & pom improvements#727
Conversation
method. They were not robust, especially in a Windows OS environment.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @Test | ||
| public void testGetFileBytes_NotRegular_NotReadable() throws IOException { | ||
| try { | ||
| byte[] resultBytes = getFileBytes(resPath, ""); |
Check notice
Code scanning / CodeQL
Unread local variable Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the problem, remove the unused local variable while preserving the side effect of calling getFileBytes. This means we should keep the method invocation but drop the assignment to resultBytes.
The best minimal change is within testGetFileBytes_NotRegular_NotReadable in src/test/java/org/apache/datasketches/common/TestUtilTest.java: replace byte[] resultBytes = getFileBytes(resPath, ""); with just getFileBytes(resPath, "");. This does not alter any test logic (the test still triggers the call that may throw a RuntimeException), but eliminates the unread local variable. No new imports, methods, or other definitions are required.
| @@ -57,7 +57,7 @@ | ||
| @Test | ||
| public void testGetFileBytes_NotRegular_NotReadable() throws IOException { | ||
| try { | ||
| byte[] resultBytes = getFileBytes(resPath, ""); | ||
| getFileBytes(resPath, ""); | ||
| } catch (RuntimeException e) { | ||
| System.out.println(e.toString()); | ||
| } |
| filePath = basePath.resolve(fileName); | ||
| Files.write(filePath, bytes); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("System IO Error writing file: " + filePath.toString() + " " + e); |
Check warning
Code scanning / CodeQL
Dereferenced variable may be null Warning test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 19 hours ago
In general, to fix this type of issue you must ensure that any variable dereferenced in a catch/error path is definitely non-null on all paths, or guard the dereference with an explicit null check or a safe fallback value. Logging or message construction should not itself depend on state that may not have been initialized due to the very failure being reported.
For this specific case, the best fix without changing functionality is to avoid dereferencing filePath when it may be null. Instead of calling filePath.toString() in the error message, we can build the message from values that are always available: basePath and fileName, both of which are validated as non-null at the top of the method. We can simply replace "System IO Error writing file: " + filePath.toString() + " " + e with "System IO Error writing file: " + basePath.resolve(fileName) + " " + e. This uses basePath.resolve(fileName) again purely for string construction; even if that throws, it would be another RuntimeException being thrown from within the catch, which is acceptable and does not reintroduce a null dereference. Alternatively, we could fall back to a literal that indicates an unknown path, but using basePath and fileName preserves nearly identical functionality.
Only the catch block in putFileBytes (lines 135–137) of src/test/java/org/apache/datasketches/common/TestUtil.java needs to change. No new imports or methods are required.
| @@ -133,7 +133,8 @@ | ||
| filePath = basePath.resolve(fileName); | ||
| Files.write(filePath, bytes); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("System IO Error writing file: " + filePath.toString() + " " + e); | ||
| throw new RuntimeException( | ||
| "System IO Error writing file: " + basePath.resolve(fileName) + " " + e); | ||
| } | ||
| } | ||
| } |
leerho
changed the title
1 participant
This replaces a bunch of methods attempting to read file bytes that were not robust, especially in a Windows OS environment.
This is a far simpler and more robust and modern solution that is only one method.