vy commented on code in PR #4008:
URL: https://github.com/apache/logging-log4j2/pull/4008#discussion_r2650756669
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/RollingRandomAccessFileManager.java:
##########
@@ -174,8 +174,10 @@ public static RollingRandomAccessFileManager
getRollingRandomAccessFileManager(
long size = 0;
long time = System.currentTimeMillis();
RandomAccessFile raf = null;
+ boolean fileExistedBefore = false;
Review Comment:
Can we replace the `file.exists()` on line 184 with `fileExistedBefore` too?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/RollingRandomAccessFileManager.java:
##########
@@ -174,8 +174,10 @@ public static RollingRandomAccessFileManager
getRollingRandomAccessFileManager(
long size = 0;
long time = System.currentTimeMillis();
RandomAccessFile raf = null;
+ boolean fileExistedBefore = false;
if (fileName != null) {
file = new File(name);
+ fileExistedBefore = new File(name).exists();
Review Comment:
Why don't we reuse the `file` here?
```suggestion
fileExistedBefore = file.exists();
```
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingRandomAccessFileManagerTest.java:
##########
@@ -333,4 +334,40 @@ void testRolloverRetainsFileAttributes() throws Exception {
.permissions();
assertEquals(filePermissions, actualFilePermissions);
}
+
+ @Test
+ void testWriteHeaderWhenFileDoesNotExistBefore() throws IOException {
Review Comment:
I see that header write is not tested at all. Would you mind extending this
to adapt the following footprint, please?
@ParameterizedTest
@CsvSource({
"true,true",
"true,false",
"false,true",
"false,false",
})
void testWriteHeaderWhenFileDoesNotExistBefore(final boolean append,
final boolean fileExists) throws Exception {
// ...
}
If you think there are more cases (of header write) to be tested, please
share.
##########
src/changelog/.2.x.x/fix_RollingRandomAccessFileManager_writeHeader.xml:
##########
@@ -0,0 +1,12 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns="https://logging.apache.org/xml/ns";
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="
+ https://logging.apache.org/xml/ns
+ https://logging.apache.org/xml/ns/log4j-changelog-0.xsd";
+ type="fixed">
+ <description format="asciidoc">
+ Fix writeHeader computation in RollingRandomAccessFileManager. The
writeHeader boolean was computed after creating the RandomAccessFile, which
creates the file if it doesn't exist. This made file.exists() always return
true, causing incorrect header write decisions when append=true and the file
didn't exist before.
Review Comment:
```suggestion
Fix header write in `RollingRandomAccessFileManager` that was being
incorrectly skipped if `append=true` and the file didn't exist before.
```
--
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]
