Skip to content

Commit 3cfb7b5

Browse files
ppkarwaszppkarwasz
committed
Apply suggestions from review of #2374 
1 parent 760bafa commit 3cfb7b5

File tree

8 files changed

+183
-89
lines changed

8 files changed

+183
-89
lines changed

‎log4j-api-test/src/main/java/org/apache/logging/log4j/test/TestLogger.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -75,18 +75,18 @@ protected void log(
7575
sb.append(level.toString());
7676
sb.append(' ');
7777
if (location != null) {
78-
sb.append(location.toString());
78+
sb.append(location);
7979
sb.append(' ');
8080
}
8181
sb.append(message.getFormattedMessage());
8282
final Map<String, String> mdc = ThreadContext.getImmutableContext();
83-
if (mdc.size() > 0) {
83+
if (!mdc.isEmpty()) {
8484
sb.append(' ');
85-
sb.append(mdc.toString());
85+
sb.append(mdc);
8686
sb.append(' ');
8787
}
8888
final Object[] params = message.getParameters();
89-
Throwable t;
89+
final Throwable t;
9090
if (throwable == null
9191
&& params != null
9292
&& params.length > 0
@@ -99,7 +99,7 @@ protected void log(
9999
sb.append(' ');
100100
final ByteArrayOutputStream baos = new ByteArrayOutputStream();
101101
t.printStackTrace(new PrintStream(baos));
102-
sb.append(baos.toString());
102+
sb.append(baos);
103103
}
104104
list.add(sb.toString());
105105
// System.out.println(sb.toString());

‎log4j-api-test/src/test/java/org/apache/logging/log4j/TestProvider.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,6 @@
2424
*/
2525
public class TestProvider extends Provider {
2626
public TestProvider() {
27-
super(10, "2.6.0", TestLoggerContextFactory.class);
27+
super(5, CURRENT_VERSION, TestLoggerContextFactory.class);
2828
}
2929
}

‎log4j-api-test/src/test/java/org/apache/logging/log4j/spi/ThreadContextMapTest.java‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@
2929

3030
class ThreadContextMapTest {
3131

32+
private static final String KEY = "key";
33+
3234
static Stream<ThreadContextMap> defaultMaps() {
3335
return Stream.of(
3436
new DefaultThreadContextMap(),
@@ -49,26 +51,24 @@ static Stream<ThreadContextMap> inheritableMaps() {
4951
@ParameterizedTest
5052
@MethodSource("defaultMaps")
5153
void threadLocalNotInheritableByDefault(final ThreadContextMap contextMap) {
52-
contextMap.put("key", "threadLocalNotInheritableByDefault");
53-
final ExecutorService executorService = Executors.newSingleThreadExecutor();
54-
try {
55-
assertThat(executorService.submit(() -> contextMap.get("key")))
56-
.succeedsWithin(Duration.ofSeconds(1))
57-
.isEqualTo(null);
58-
} finally {
59-
executorService.shutdown();
60-
}
54+
contextMap.put(KEY, "threadLocalNotInheritableByDefault");
55+
verifyThreadContextValueFromANewThread(contextMap, null);
6156
}
6257

6358
@ParameterizedTest
6459
@MethodSource("inheritableMaps")
6560
void threadLocalInheritableIfConfigured(final ThreadContextMap contextMap) {
66-
contextMap.put("key", "threadLocalInheritableIfConfigured");
61+
contextMap.put(KEY, "threadLocalInheritableIfConfigured");
62+
verifyThreadContextValueFromANewThread(contextMap, "threadLocalInheritableIfConfigured");
63+
}
64+
65+
private static void verifyThreadContextValueFromANewThread(
66+
final ThreadContextMap contextMap, final String expected) {
6767
final ExecutorService executorService = Executors.newSingleThreadExecutor();
6868
try {
69-
assertThat(executorService.submit(() -> contextMap.get("key")))
69+
assertThat(executorService.submit(() -> contextMap.get(KEY)))
7070
.succeedsWithin(Duration.ofSeconds(1))
71-
.isEqualTo("threadLocalInheritableIfConfigured");
71+
.isEqualTo(expected);
7272
} finally {
7373
executorService.shutdown();
7474
}

‎log4j-api-test/src/test/java/org/apache/logging/log4j/util/ProviderUtilTest.java‎

Lines changed: 111 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,49 +18,141 @@
1818

1919
import static org.assertj.core.api.Assertions.assertThat;
2020

21+
import java.util.Arrays;
22+
import java.util.Collection;
23+
import java.util.Collections;
2124
import java.util.Properties;
25+
import java.util.regex.Pattern;
26+
import java.util.stream.Stream;
2227
import org.apache.logging.log4j.TestProvider;
28+
import org.apache.logging.log4j.spi.Provider;
29+
import org.apache.logging.log4j.test.TestLogger;
2330
import org.apache.logging.log4j.test.TestLoggerContextFactory;
31+
import org.junit.jupiter.api.BeforeEach;
2432
import org.junit.jupiter.api.Test;
2533
import org.junit.jupiter.api.parallel.Execution;
2634
import org.junit.jupiter.api.parallel.ExecutionMode;
35+
import org.junit.jupiter.params.ParameterizedTest;
36+
import org.junit.jupiter.params.provider.Arguments;
37+
import org.junit.jupiter.params.provider.MethodSource;
2738

2839
@Execution(ExecutionMode.CONCURRENT)
2940
class ProviderUtilTest {
3041

31-
/*
32-
* Force initialization of ProviderUtil#PROVIDERS
33-
*/
34-
static {
35-
ProviderUtil.lazyInit();
42+
private static final Pattern ERROR_OR_WARNING = Pattern.compile(" (ERROR|WARN) .*", Pattern.DOTALL);
43+
44+
private static final Provider LOCAL_PROVIDER = new LocalProvider();
45+
private static final Provider TEST_PROVIDER = new TestProvider();
46+
47+
private static final Collection<Provider> NO_PROVIDERS = Collections.emptyList();
48+
private static final Collection<Provider> ONE_PROVIDER = Collections.singleton(TEST_PROVIDER);
49+
private static final Collection<Provider> TWO_PROVIDERS = Arrays.asList(LOCAL_PROVIDER, TEST_PROVIDER);
50+
51+
private TestLogger statusLogger;
52+
53+
@BeforeEach
54+
void setup() {
55+
statusLogger = new TestLogger();
56+
}
57+
58+
@Test
59+
void should_have_a_fallback_provider() {
60+
final PropertiesUtil properties = new PropertiesUtil(new Properties());
61+
assertThat(ProviderUtil.selectProvider(properties, NO_PROVIDERS, statusLogger))
62+
.as("check selected provider")
63+
.isNotNull();
64+
// An error for the absence of providers
65+
assertHasErrorOrWarning(statusLogger);
66+
}
67+
68+
@Test
69+
void should_be_silent_with_a_single_provider() {
70+
final PropertiesUtil properties = new PropertiesUtil(new Properties());
71+
assertThat(ProviderUtil.selectProvider(properties, ONE_PROVIDER, statusLogger))
72+
.as("check selected provider")
73+
.isSameAs(TEST_PROVIDER);
74+
assertNoErrorsOrWarnings(statusLogger);
3675
}
3776

3877
@Test
3978
void should_select_provider_with_highest_priority() {
4079
final PropertiesUtil properties = new PropertiesUtil(new Properties());
41-
assertThat(ProviderUtil.selectProvider(properties))
80+
assertThat(ProviderUtil.selectProvider(properties, TWO_PROVIDERS, statusLogger))
4281
.as("check selected provider")
43-
.isInstanceOf(TestProvider.class);
82+
.isSameAs(TEST_PROVIDER);
83+
// A warning for the presence of multiple providers
84+
assertHasErrorOrWarning(statusLogger);
4485
}
4586

4687
@Test
4788
void should_recognize_log4j_provider_property() {
4889
final Properties map = new Properties();
4990
map.setProperty("log4j.provider", LocalProvider.class.getName());
5091
final PropertiesUtil properties = new PropertiesUtil(map);
51-
assertThat(ProviderUtil.selectProvider(properties))
92+
assertThat(ProviderUtil.selectProvider(properties, TWO_PROVIDERS, statusLogger))
5293
.as("check selected provider")
5394
.isInstanceOf(LocalProvider.class);
95+
assertNoErrorsOrWarnings(statusLogger);
5496
}
5597

98+
/**
99+
* Can be removed in the future.
100+
*/
56101
@Test
57102
void should_recognize_log4j_factory_property() {
58103
final Properties map = new Properties();
59104
map.setProperty("log4j2.loggerContextFactory", LocalLoggerContextFactory.class.getName());
60105
final PropertiesUtil properties = new PropertiesUtil(map);
61-
assertThat(ProviderUtil.selectProvider(properties).getLoggerContextFactory())
106+
assertThat(ProviderUtil.selectProvider(properties, TWO_PROVIDERS, statusLogger)
107+
.getLoggerContextFactory())
62108
.as("check selected logger context factory")
63109
.isInstanceOf(LocalLoggerContextFactory.class);
110+
// Deprecation warning
111+
assertHasErrorOrWarning(statusLogger);
112+
}
113+
114+
/**
115+
* Can be removed in the future.
116+
*/
117+
@Test
118+
void log4j_provider_property_has_priority() {
119+
final Properties map = new Properties();
120+
map.setProperty("log4j.provider", LocalProvider.class.getName());
121+
map.setProperty("log4j2.loggerContextFactory", TestLoggerContextFactory.class.getName());
122+
final PropertiesUtil properties = new PropertiesUtil(map);
123+
assertThat(ProviderUtil.selectProvider(properties, TWO_PROVIDERS, statusLogger))
124+
.as("check selected provider")
125+
.isInstanceOf(LocalProvider.class);
126+
// Warning
127+
assertHasErrorOrWarning(statusLogger);
128+
}
129+
130+
static Stream<Arguments> incorrect_configuration_do_not_throw() {
131+
return Stream.of(
132+
Arguments.of("java.lang.String", null),
133+
Arguments.of("non.existent.Provider", null),
134+
// logger context factory without a matching provider
135+
Arguments.of(null, "org.apache.logging.log4j.util.ProviderUtilTest$LocalLoggerContextFactory"),
136+
Arguments.of(null, "java.lang.String"),
137+
Arguments.of(null, "non.existent.LoggerContextFactory"));
138+
}
139+
140+
@ParameterizedTest
141+
@MethodSource
142+
void incorrect_configuration_do_not_throw(final String provider, final String contextFactory) {
143+
final Properties map = new Properties();
144+
if (provider != null) {
145+
map.setProperty("log4j.provider", provider);
146+
}
147+
if (contextFactory != null) {
148+
map.setProperty("log4j2.loggerContextFactory", contextFactory);
149+
}
150+
final PropertiesUtil properties = new PropertiesUtil(map);
151+
assertThat(ProviderUtil.selectProvider(properties, ONE_PROVIDER, statusLogger))
152+
.as("check selected provider")
153+
.isNotNull();
154+
// Warnings will be present
155+
assertHasErrorOrWarning(statusLogger);
64156
}
65157

66158
public static class LocalLoggerContextFactory extends TestLoggerContextFactory {}
@@ -73,4 +165,14 @@ public LocalProvider() {
73165
super(0, CURRENT_VERSION, LocalLoggerContextFactory.class);
74166
}
75167
}
168+
169+
private void assertHasErrorOrWarning(final TestLogger statusLogger) {
170+
assertThat(statusLogger.getEntries()).as("check StatusLogger entries").anySatisfy(entry -> assertThat(entry)
171+
.matches(ERROR_OR_WARNING));
172+
}
173+
174+
private void assertNoErrorsOrWarnings(final TestLogger statusLogger) {
175+
assertThat(statusLogger.getEntries()).as("check StatusLogger entries").allSatisfy(entry -> assertThat(entry)
176+
.doesNotMatch(ERROR_OR_WARNING));
177+
}
76178
}

‎log4j-api/src/main/java/org/apache/logging/log4j/spi/DefaultThreadContextMap.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ public DefaultThreadContextMap() {
4848
}
4949

5050
/**
51-
* @deprecated Since 2.24.0 use the default constructor or {@link NoOpThreadContextMap} instead.
51+
* @deprecated Since 2.24.0. See {@link Provider#getThreadContextMap()} on how to obtain a no-op map.
5252
*/
5353
@Deprecated
5454
public DefaultThreadContextMap(final boolean useMap) {

‎log4j-api/src/main/java/org/apache/logging/log4j/spi/NoOpThreadContextMap.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,10 @@
2424
* {@code disableThreadContext} is {@code true}. This implementation does nothing.
2525
*
2626
* @since 2.7
27+
* @deprecated since 2.24.0. Return the {@value Provider#NO_OP_CONTEXT_MAP} constant in
28+
* {@link Provider#getThreadContextMap()} instead.
2729
*/
30+
@Deprecated
2831
public class NoOpThreadContextMap implements ThreadContextMap {
2932
@Override
3033
public void clear() {}

0 commit comments

Comments
 (0)