ramanathan1504 commented on code in PR #4234:
URL: https://github.com/apache/logging-log4j2/pull/4234#discussion_r4196791005
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManagerTest.java:
##########
@@ -156,6 +162,49 @@ public RolloverDescription rollover(final
RollingFileManager manager) throws Sec
assertEquals(testContent, new
String(Files.readAllBytes(file.toPath()), StandardCharsets.US_ASCII));
}
+ @ParameterizedTest
+ @CsvSource({"bz2, -2", "bz2, 10", "deflate, -2", "deflate, 10"})
Review Comment:
Adding the other two extensions here fails 4 of 4, which is the finding
below in one line.
```suggestion
@CsvSource({"bz2, -2", "bz2, 10", "deflate, -2", "deflate, 10", "gz,
-2", "gz, 10", "zip, -2", "zip, 10"})
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/ZipCompressAction.java:
##########
@@ -108,30 +105,16 @@ public boolean execute() throws IOException {
*/
public static boolean execute(
final File source, final File destination, final boolean
deleteSource, final int level) throws IOException {
- if (source.exists()) {
Review Comment:
`this.level = checkLevel(level)` in the constructor throws during rollover,
so `.zip` loses the event while `.bz2` and `.deflate` now survive. Also, this
`execute` validates after the `exists()` check while `GzCompressAction.execute`
validates before it.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/internal/Bzip2CompressAction.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.logging.log4j.core.appender.rolling.action.internal;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.util.zip.Deflater;
+import
org.apache.commons.compress.compressors.bzip2.BZip2CompressorOutputStream;
+
+/** Compresses a file using BZip2 compression. */
+public final class Bzip2CompressAction extends AbstractCompressAction {
+
+ private final int compressionLevel;
+
+ public Bzip2CompressAction(
+ final File source, final File destination, final boolean
deleteSource, final int compressionLevel) {
+ super(source, destination, deleteSource);
+ this.compressionLevel = compressionLevel;
+ }
+
+ @Override
+ protected void validateCompressionLevel() {
+ final int level = resolveCompressionLevel();
+ if (level < BZip2CompressorOutputStream.MIN_BLOCKSIZE || level >
BZip2CompressorOutputStream.MAX_BLOCKSIZE) {
+ throw new IllegalArgumentException("BZip2 compression level must
be in the range ["
+ + BZip2CompressorOutputStream.MIN_BLOCKSIZE
+ + ", "
+ + BZip2CompressorOutputStream.MAX_BLOCKSIZE
+ + "], got: "
+ + compressionLevel);
+ }
+ }
+
+ @Override
+ protected OutputStream createCompressorOutputStream(final OutputStream
output) throws IOException {
+ return new BZip2CompressorOutputStream(output,
resolveCompressionLevel());
+ }
+
+ private int resolveCompressionLevel() {
+ return compressionLevel == Deflater.DEFAULT_COMPRESSION ||
compressionLevel == Deflater.NO_COMPRESSION
Review Comment:
`compressionLevel="0"` lands on `MAX_BLOCKSIZE` here, so 0 means maximum
compression for `.bz2` while it means no compression for `.zip`, `.gz` and
`.deflate`, and the library default for `.zst`. Is that the reading you want
for the shared attribute?
--
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]
