Skip to content

Commit 267c5f1

Browse files
JWT007JWT007
committed
Update StringMatchFilter builder API (apache#3509)
* Guard against NPEs * added private constructor to builder * added getText() accessor * add JVerify nullability annotations * added detailed javadoc with implementation details * optimized AbstractLifeCycle and AbstractFilter "equalsImpl" implementations * added changelog
1 parent be94cd1 commit 267c5f1

File tree

8 files changed

+627
-150
lines changed

8 files changed

+627
-150
lines changed

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

+103
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright ownership.
5+
* The ASF licenses this file to you under the Apache License, Version 2.0
6+
* (the "License"); you may not use this file except in compliance with
7+
* the License. You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
package org.apache.logging.log4j.core.filter;
18+
19+
import static org.junit.jupiter.api.Assertions.assertEquals;
20+
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
21+
import static org.junit.jupiter.api.Assertions.assertNotNull;
22+
import static org.junit.jupiter.api.Assertions.assertNull;
23+
24+
import org.apache.logging.log4j.core.Filter;
25+
import org.apache.logging.log4j.core.config.Configuration;
26+
import org.apache.logging.log4j.core.test.junit.LoggerContextSource;
27+
import org.junit.jupiter.api.Assertions;
28+
import org.junit.jupiter.api.Test;
29+
30+
/**
31+
* Unit tests for {@link StringMatchFilter}.
32+
*/
33+
class StringMatchFilterTest {
34+
35+
/**
36+
* Test the normal valid programmatic instantiation of a {@link StringMatchFilter} via its builder.
37+
*/
38+
@Test
39+
void testFilterBuilderOK() {
40+
StringMatchFilter.Builder stringMatchFilterBuilder = StringMatchFilter.newBuilder();
41+
stringMatchFilterBuilder.setText("foo");
42+
StringMatchFilter stringMatchFilter = stringMatchFilterBuilder.build();
43+
assertNotNull(stringMatchFilter, "The filter should not be null.");
44+
assertEquals("foo", stringMatchFilter.getText());
45+
}
46+
47+
/**
48+
* Test that if no match-string is set on the builder, the '{@link StringMatchFilter.Builder#build()}' returns
49+
* {@code null}.
50+
*/
51+
@Test
52+
void testFilterBuilderFailsWithNullText() {
53+
StringMatchFilter.Builder stringMatchFilterBuilder = StringMatchFilter.newBuilder();
54+
Assertions.assertNull(stringMatchFilterBuilder.build());
55+
}
56+
57+
/**
58+
* Test that if a {@code null} string is set as a match-pattern, an {@code IllegalArgumentExeption} is thrown.
59+
*/
60+
@Test
61+
@SuppressWarnings({"DataFlowIssue" // invalid null parameter explicitly being tested
62+
})
63+
void testFilterBuilderFailsWithExceptionOnNullText() {
64+
StringMatchFilter.Builder stringMatchFilterBuilder = StringMatchFilter.newBuilder();
65+
Assertions.assertThrows(IllegalArgumentException.class, () -> stringMatchFilterBuilder.setText(null));
66+
}
67+
68+
/**
69+
* Test that if an empty ({@code ""}) string is set as a match-pattern, an {@code IllegalArgumentException} is thrown.
70+
*/
71+
@Test
72+
void testFilterBuilderFailsWithExceptionOnEmptyText() {
73+
StringMatchFilter.Builder stringMatchFilterBuilder = StringMatchFilter.newBuilder();
74+
Assertions.assertThrows(IllegalArgumentException.class, () -> stringMatchFilterBuilder.setText(""));
75+
}
76+
77+
/**
78+
* Test that if a {@link StringMatchFilter} is specified with a 'text' attribute it is correctly instantiated.
79+
*
80+
* @param configuration the configuration
81+
*/
82+
@Test
83+
@LoggerContextSource("log4j2-stringmatchfilter-3153-ok.xml")
84+
void testConfigurationWithTextPOS(final Configuration configuration) {
85+
final Filter filter = configuration.getFilter();
86+
assertNotNull(filter, "The filter should not be null.");
87+
assertInstanceOf(
88+
StringMatchFilter.class, filter, "Expected a StringMatchFilter, but got: " + filter.getClass());
89+
assertEquals("FooBar", ((StringMatchFilter) filter).getText());
90+
}
91+
92+
/**
93+
* Test that if a {@link StringMatchFilter} is specified without a 'text' attribute it is not instantiated.
94+
*
95+
* @param configuration the configuration
96+
*/
97+
@Test
98+
@LoggerContextSource("log4j2-stringmatchfilter-3153-nok.xml")
99+
void testConfigurationWithTextNEG(final Configuration configuration) {
100+
final Filter filter = configuration.getFilter();
101+
assertNull(filter, "The filter should be null.");
102+
}
103+
}

‎log4j-core-test/src/test/resources/log4j2-stringmatchfilter-3153-nok.xml

+23
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<!--
3+
~ Licensed to the Apache Software Foundation (ASF) under one or more
4+
~ contributor license agreements. See the NOTICE file distributed with
5+
~ this work for additional information regarding copyright ownership.
6+
~ The ASF licenses this file to you under the Apache License, Version 2.0
7+
~ (the "License"); you may not use this file except in compliance with
8+
~ the License. You may obtain a copy of the License at
9+
~
10+
~ http://www.apache.org/licenses/LICENSE-2.0
11+
~
12+
~ Unless required by applicable law or agreed to in writing, software
13+
~ distributed under the License is distributed on an "AS IS" BASIS,
14+
~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
~ See the License for the specific language governing permissions and
16+
~ limitations under the License.
17+
-->
18+
<Configuration status="warn">
19+
<StringMatchFfilter/> <!-- this typo is intentional! -->
20+
<Loggers>
21+
<Root/> <!-- one logger to avoid test warning about no defined loggers -->
22+
</Loggers>
23+
</Configuration>

‎log4j-core-test/src/test/resources/log4j2-stringmatchfilter-3153-ok.xml

+23
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<!--
3+
~ Licensed to the Apache Software Foundation (ASF) under one or more
4+
~ contributor license agreements. See the NOTICE file distributed with
5+
~ this work for additional information regarding copyright ownership.
6+
~ The ASF licenses this file to you under the Apache License, Version 2.0
7+
~ (the "License"); you may not use this file except in compliance with
8+
~ the License. You may obtain a copy of the License at
9+
~
10+
~ http://www.apache.org/licenses/LICENSE-2.0
11+
~
12+
~ Unless required by applicable law or agreed to in writing, software
13+
~ distributed under the License is distributed on an "AS IS" BASIS,
14+
~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
~ See the License for the specific language governing permissions and
16+
~ limitations under the License.
17+
-->
18+
<Configuration status="warn">
19+
<StringMatchFilter text="FooBar"/>
20+
<Loggers>
21+
<Root/> <!-- one logger to avoid test warning about no defined loggers -->
22+
</Loggers>
23+
</Configuration>

‎log4j-core/src/main/java/org/apache/logging/log4j/core/AbstractLifeCycle.java

+14-11
Original file line numberDiff line numberDiff line change
@@ -47,21 +47,24 @@ protected static org.apache.logging.log4j.Logger getStatusLogger() {
4747

4848
private volatile LifeCycle.State state = LifeCycle.State.INITIALIZED;
4949

50-
protected boolean equalsImpl(final Object obj) {
51-
if (this == obj) {
50+
/**
51+
* Indicates whether some other object is "equal to" this one.
52+
* @param other the other object to compare to
53+
* @return {@code true} if this object is the same as the obj argument; {@code false} otherwise.
54+
*/
55+
@SuppressWarnings("BooleanMethodIsAlwaysInverted")
56+
protected boolean equalsImpl(final Object other) {
57+
// identity check - fast exit
58+
if (this == other) {
5259
return true;
5360
}
54-
if (obj == null) {
55-
return false;
56-
}
57-
if (getClass() != obj.getClass()) {
61+
// exact class check
62+
if (other == null || this.getClass() != other.getClass()) {
5863
return false;
5964
}
60-
final LifeCycle other = (LifeCycle) obj;
61-
if (state != other.getState()) {
62-
return false;
63-
}
64-
return true;
65+
// field check
66+
final AbstractLifeCycle that = (AbstractLifeCycle) other;
67+
return (this.state == that.state);
6568
}
6669

6770
@Override

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

+18-14
Original file line numberDiff line numberDiff line change
@@ -110,25 +110,29 @@ protected AbstractFilter(final Result onMatch, final Result onMismatch) {
110110
this.onMismatch = onMismatch == null ? Result.DENY : onMismatch;
111111
}
112112

113+
/**
114+
* Constructor which obtains its parameterization from the given builder.
115+
* @param builder the builder
116+
*/
117+
protected AbstractFilter(final AbstractFilterBuilder<?> builder) {
118+
this(builder.getOnMatch(), builder.getOnMismatch());
119+
}
120+
121+
/** {@inheritDoc} */
113122
@Override
114-
protected boolean equalsImpl(final Object obj) {
115-
if (this == obj) {
123+
@SuppressWarnings("BooleanMethodIsAlwaysInverted")
124+
protected boolean equalsImpl(final Object other) {
125+
// identity check - fast exit
126+
if (this == other) {
116127
return true;
117128
}
118-
if (!super.equalsImpl(obj)) {
119-
return false;
120-
}
121-
if (getClass() != obj.getClass()) {
122-
return false;
123-
}
124-
final AbstractFilter other = (AbstractFilter) obj;
125-
if (onMatch != other.onMatch) {
126-
return false;
127-
}
128-
if (onMismatch != other.onMismatch) {
129+
// type check and superclass
130+
if (other == null || this.getClass() != other.getClass()) {
129131
return false;
130132
}
131-
return true;
133+
// field equality
134+
final AbstractFilter that = (AbstractFilter) other;
135+
return super.equalsImpl(that) && this.onMatch == that.onMatch && this.onMismatch == that.onMismatch;
132136
}
133137

134138
/**

0 commit comments

Comments
 (0)