A few more improvements + added tests (apache#3086)

Author: jethomas-tsi

log4j-core-test/src/test/java/org/apache/logging/log4j/core/filter/RegexFilterTest.java

@@ -19,9 +19,12 @@
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import org.apache.logging.log4j.Level;
@@ -87,38 +90,131 @@ void testDotAllPattern() throws Exception {
     }
 
     @Test
-    void testNoMsg() throws Exception {
-        final RegexFilter filter = RegexFilter.createFilter(".* test .*", null, false, null, null);
+    void testNoMsg() {
+
+        final RegexFilter filter =
+            RegexFilter.newBuilder()
+                       .setRegex(".* test .*")
+                       .setUseRawMsg(false)
+                       .build();
+
+        assertNotNull(filter);
+
         filter.start();
+
         assertTrue(filter.isStarted());
         assertSame(Filter.Result.DENY, filter.filter(null, Level.DEBUG, null, (Object) null, null));
         assertSame(Filter.Result.DENY, filter.filter(null, Level.DEBUG, null, (Message) null, null));
         assertSame(Filter.Result.DENY, filter.filter(null, Level.DEBUG, null, null, (Object[]) null));
     }
 
     @Test
-    void testParameterizedMsg() throws Exception {
+    void testParameterizedMsg() {
         final String msg = "params {} {}";
         final Object[] params = {"foo", "bar"};
 
         // match against raw message
-        final RegexFilter rawFilter = RegexFilter.createFilter(
-                "params \\{\\} \\{\\}",
-                null,
-                true, // useRawMsg
-                Result.ACCEPT,
-                Result.DENY);
+        final RegexFilter rawFilter =
+            RegexFilter.newBuilder()
+                       .setRegex("params \\{\\} \\{\\}")
+                       .setUseRawMsg(true)
+                       .setOnMatch(Result.ACCEPT)
+                       .setOnMismatch(Result.DENY)
+                       .build();
+
+        assertNotNull(rawFilter);
+
         final Result rawResult = rawFilter.filter(null, null, null, msg, params);
         assertThat(rawResult, equalTo(Result.ACCEPT));
 
         // match against formatted message
-        final RegexFilter fmtFilter = RegexFilter.createFilter(
-                "params foo bar",
-                null,
-                false, // useRawMsg
-                Result.ACCEPT,
-                Result.DENY);
+        final RegexFilter fmtFilter =
+            RegexFilter.newBuilder()
+                       .setRegex("params foo bar")
+                       .setUseRawMsg(false)
+                       .setOnMatch(Result.ACCEPT)
+                       .setOnMismatch(Result.DENY).build();
+
+        assertNotNull(fmtFilter);
+
         final Result fmtResult = fmtFilter.filter(null, null, null, msg, params);
         assertThat(fmtResult, equalTo(Result.ACCEPT));
     }
+
+    /**
+     * A builder with no 'regex' expression should both be invalid and return null on 'build()'.
+     */
+    @Test
+    void testWithValidRegex() {
+
+        final String regex = "^[a-zA-Z0-9_]+$"; // matches alphanumeric with underscores
+
+        final RegexFilter.Builder builder =
+            RegexFilter.newBuilder().setRegex(regex).setUseRawMsg(false).setOnMatch(Result.ACCEPT).setOnMismatch(Result.DENY);
+
+        assertTrue(builder.isValid());
+
+        final RegexFilter filter = builder.build();
+
+        assertNotNull(filter);
+
+        assertEquals(Result.ACCEPT, filter.filter("Hello_123"));
+
+        assertEquals(Result.DENY, filter.filter("Hello@123"));
+
+        assertEquals(regex, filter.getRegex());
+    }
+
+    @Test
+    void testRegexFilterGetters() {
+
+        final String regex = "^[a-zA-Z0-9_]+$"; // matches alphanumeric with underscores
+
+        final RegexFilter filter =
+            RegexFilter.newBuilder()
+                       .setRegex(regex)
+                       .setUseRawMsg(false)
+                       .setOnMatch(Result.ACCEPT)
+                       .setOnMismatch(Result.DENY)
+                       .build();
+
+        assertNotNull(filter);
+
+        assertEquals(regex, filter.getRegex());
+        assertFalse(filter.isUseRawMessage());
+        assertEquals(Result.ACCEPT, filter.getOnMatch());
+        assertEquals(Result.DENY, filter.getOnMismatch());
+        assertNotNull(filter.getPattern());
+        assertEquals(regex, filter.getPattern().pattern());
+    }
+
+    /**
+     * A builder with no 'regex' expression should both be invalid and return null on 'build()'.
+     */
+    @Test
+    void testBuilderWithoutRegexNotValid() {
+
+        final RegexFilter.Builder builder = RegexFilter.newBuilder();
+
+        assertFalse(builder.isValid());
+
+        assertNull(builder.build());
+
+    }
+
+    /**
+     * A builder with an invalid 'regex' expression should return null on 'build()'.
+     */
+    @Test
+    void testBuilderWithInvalidRegexNotValid() {
+
+        final RegexFilter.Builder builder = RegexFilter.newBuilder();
+
+        builder.setRegex("[a-z");
+
+        assertFalse(builder.isValid());
+
+        assertNull(builder.build());
+
+    }
 }

log4j-core/src/main/java/org/apache/logging/log4j/core/filter/RegexFilter.java

@@ -21,6 +21,7 @@
 import java.util.Comparator;
 import java.util.Objects;
 import java.util.regex.Pattern;
+import java.util.regex.PatternSyntaxException;
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.Marker;
 import org.apache.logging.log4j.core.Filter;
@@ -44,9 +45,6 @@
 @Plugin(name = "RegexFilter", category = Node.CATEGORY, elementType = Filter.ELEMENT_TYPE, printObject = true)
 public final class RegexFilter extends AbstractFilter {
 
-    /** The regular-expression. */
-    private final String regex;
-
     /** The pattern compiled from the regular-expression. */
     private final Pattern pattern;
 
@@ -62,13 +60,12 @@ private RegexFilter(final Builder builder) {
 
         super(builder);
 
-        this.regex = builder.regex;
         this.useRawMessage = Boolean.TRUE.equals(builder.useRawMsg);
 
         try {
-            this.pattern = Pattern.compile(regex);
+            this.pattern = Pattern.compile(builder.regex);
         } catch (final Exception ex) {
-            throw new IllegalArgumentException("Unable to compile regular expression: '" + regex + "'.", ex);
+            throw new IllegalArgumentException("Unable to compile regular expression: '" + builder.regex + "'.", ex);
         }
     }
 
@@ -77,7 +74,7 @@ private RegexFilter(final Builder builder) {
      * @return the regular-expression (it may be an empty string but never {@code null})
      */
     public String getRegex() {
-        return this.regex;
+        return this.pattern.pattern();
     }
 
     /**
@@ -136,7 +133,7 @@ public Result filter(final LogEvent event) {
      * @param msg the message
      * @return the filter result
      */
-    private Result filter(final String msg) {
+    public Result filter(final String msg) {
         if (msg == null) {
             return onMismatch;
         }
@@ -181,7 +178,7 @@ private String getMessageTextByType(final Message message) {
 
     @Override
     public String toString() {
-        return "useRawMessage=" + useRawMessage + ", regex=" + regex + ", pattern=" + pattern.toString();
+        return "useRawMessage=" + useRawMessage + ", pattern=" + pattern.toString();
     }
 
     /**
@@ -251,7 +248,11 @@ public Builder setUseRawMsg(final boolean useRawMsg) {
          */
         @Override
         public boolean isValid() {
-            return (regex != null);
+            boolean valid = true;
+            if (!isRegexValid()) {
+                valid = false;
+            }
+            return valid;
         }
 
         /**
@@ -273,6 +274,25 @@ public RegexFilter build() {
                 return null;
             }
         }
+
+        /**
+         * Validates the 'regex' attribute.
+         * <p>
+         *   If the regular-expression is not set, or cannot be compiled to a valid pattern the validation will fail.
+         * </p>
+         * @return {@code true} if the regular-expression is valid; otherwise, {@code false}
+         */
+        private boolean isRegexValid() {
+            if (regex == null) {
+                return false;
+            }
+            try {
+                Pattern.compile(regex);
+            } catch (final PatternSyntaxException ex) {
+                return false;
+            }
+            return true;
+        }
     }
 
     /*
@@ -306,7 +326,7 @@ private RegexFilter(
             final Result onMatch,
             final Result onMismatch) {
         super(onMatch, onMismatch);
-        this.regex = Objects.requireNonNull(regex, "The 'regex' argument must be provided for RegexFilter");
+        Objects.requireNonNull(regex, "The 'regex' argument must be provided for RegexFilter");
         this.patternFlags = patternFlags == null ? new String[0] : patternFlags.clone();
         try {
             int flags = toPatternFlags(this.patternFlags);