jvz commented on code in PR #2148:
URL: https://github.com/apache/logging-log4j2/pull/2148#discussion_r1439166113
##########
log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/LoggerContextSource.java:
##########
@@ -84,20 +80,4 @@
* Specifies the time unit {@link #timeout()} is measured in.
*/
TimeUnit unit() default TimeUnit.SECONDS;
-
- /**
- * Toggles Log4j1 configuration file compatibility mode for XML and
properties files.
- */
- boolean v1config() default false;
Review Comment:
Extracted to new annotation.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java:
##########
@@ -439,7 +438,7 @@ public boolean stop(final long timeout, final TimeUnit
timeUnit) {
this.setStopping();
String name = getName();
try {
- Server.unregisterLoggerContext(getName()); // LOG4J2-406,
LOG4J2-500
+ Server.unregisterLoggerContext(name); // LOG4J2-406, LOG4J2-500
Review Comment:
Noticed this variable was unused from above, so I fixed.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java:
##########
@@ -775,12 +784,9 @@ protected void setToDefault() {
addAppender(appender);
final LoggerConfig rootLoggerConfig = getRootLogger();
rootLoggerConfig.addAppender(appender, null, null);
-
- final Level defaultLevel = Level.ERROR;
- final String levelName =
-
contextProperties.getStringProperty(Log4jPropertyKey.CONFIG_DEFAULT_LEVEL,
defaultLevel.name());
- final Level level = Level.valueOf(levelName);
- rootLoggerConfig.setLevel(level != null ? level : defaultLevel);
+ final String defaultLevelName =
contextProperties.getStringProperty(Log4jPropertyKey.CONFIG_DEFAULT_LEVEL);
+ final Level defaultLevel = Level.toLevel(defaultLevelName,
Level.ERROR);
Review Comment:
Also noticed from nullability analysis that calling `Level::valueOf` was
incorrect here as it couldn't return `null`.
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/time/ClockFactoryTest.java:
##########
@@ -18,71 +18,47 @@
import static org.assertj.core.api.Assertions.assertThat;
-import org.apache.logging.log4j.core.impl.Log4jPropertyKey;
import org.apache.logging.log4j.core.time.internal.CachedClock;
import org.apache.logging.log4j.core.time.internal.CoarseCachedClock;
import org.apache.logging.log4j.core.time.internal.SystemClock;
-import org.apache.logging.log4j.plugins.di.ConfigurableInstanceFactory;
import org.apache.logging.log4j.plugins.di.DI;
import org.junit.jupiter.api.Test;
-import org.junitpioneer.jupiter.SetSystemProperty;
public class ClockFactoryTest {
- private final ConfigurableInstanceFactory instanceFactory =
DI.createFactory();
+ private final DI.FactoryBuilder builder = DI.builder();
@Test
public void testDefaultIsSystemClock() {
- DI.initializeFactory(instanceFactory);
-
assertThat(instanceFactory.getInstance(Clock.class)).isInstanceOf(SystemClock.class);
+ final Clock clock = builder.build().getInstance(Clock.KEY);
+ assertThat(clock).isInstanceOf(SystemClock.class);
}
@Test
- @SetSystemProperty(key = Log4jPropertyKey.Constant.CONFIG_CLOCK, value =
"SystemClock")
public void testSpecifySystemClockShort() {
- DI.initializeFactory(instanceFactory);
-
assertThat(instanceFactory.getInstance(Clock.class)).isInstanceOf(SystemClock.class);
+ final Clock clock = builder.addInitialBindingFrom(Clock.KEY)
Review Comment:
Test updates are also related to #1977.
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/ShutdownCallbackRegistryTest.java:
##########
@@ -28,20 +28,16 @@
import org.apache.logging.log4j.core.impl.Log4jContextFactory;
import org.apache.logging.log4j.core.selector.ContextSelector;
import org.apache.logging.log4j.core.test.junit.LoggerContextSource;
+import org.apache.logging.log4j.core.test.junit.TestBinding;
import org.apache.logging.log4j.plugins.Singleton;
-import org.apache.logging.log4j.plugins.SingletonFactory;
import org.apache.logging.log4j.status.StatusLogger;
import org.junit.jupiter.api.Test;
+@TestBinding(api = ShutdownCallbackRegistry.class, implementation =
ShutdownCallbackRegistryTest.Registry.class)
Review Comment:
Since all test bindings are registered as initial bindings in the new DSL,
that helps avoid the need for the old `bootstrap` boolean flag in
`@LoggerContextSource`.
##########
log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/LoggerContextSource.java:
##########
@@ -84,20 +80,4 @@
* Specifies the time unit {@link #timeout()} is measured in.
*/
TimeUnit unit() default TimeUnit.SECONDS;
-
- /**
- * Toggles Log4j1 configuration file compatibility mode for XML and
properties files.
- */
- boolean v1config() default false;
-
- /**
- * Determines whether to bootstrap a fresh LoggerContextFactory.
- */
- boolean bootstrap() default false;
-
- /**
- * Overrides the {@link ContextSelector} to use by default. If unset, the
test instance can still
- * define a context selector factory to override the default {@link
ClassLoaderContextSelector}.
- */
- Class<? extends ContextSelector> selector() default ContextSelector.class;
Review Comment:
Extracted to a new annotation.
##########
log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/LoggerContextSource.java:
##########
@@ -84,20 +80,4 @@
* Specifies the time unit {@link #timeout()} is measured in.
*/
TimeUnit unit() default TimeUnit.SECONDS;
-
- /**
- * Toggles Log4j1 configuration file compatibility mode for XML and
properties files.
- */
- boolean v1config() default false;
-
- /**
- * Determines whether to bootstrap a fresh LoggerContextFactory.
- */
- boolean bootstrap() default false;
Review Comment:
No longer relevant; we just bootstrap them all the time depending on where
the annotation is used.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java:
##########
@@ -666,11 +665,12 @@ protected List<Node> processSelect(final Node selectNode,
final PluginType<?> ty
}
protected void doConfigure() {
-
instanceFactory.registerBinding(Binding.from(StringValueResolver.KEY).toInstance(configurationStrSubstitutor));
+ instanceFactory.registerBinding(StringValueResolver.KEY,
this::getConfigurationStrSubstitutor);
processConditionals(rootNode);
preConfigure(rootNode);
configurationScheduler.start();
- if (rootNode.hasChildren() &&
rootNode.getChildren().get(0).getName().equalsIgnoreCase("Properties")) {
+ if (rootNode.hasChildren()
+ &&
"Properties".equalsIgnoreCase(rootNode.getChildren().get(0).getName())) {
Review Comment:
Various flipping around is from nullability analysis from some time where
`Node::getName` is technically nullable.
##########
log4j-plugins/src/main/java/org/apache/logging/log4j/plugins/di/DI.java:
##########
@@ -31,53 +39,143 @@ private DI() {
}
/**
- * Creates a new {@linkplain
#initializeFactory(ConfigurableInstanceFactory) initialized} instance factory.
+ * Creates a new {@linkplain ConfigurableInstanceFactory initialized}
instance factory.
*
* @return the initialized instance factory
*/
public static ConfigurableInstanceFactory createInitializedFactory() {
- final var factory = createFactory();
- initializeFactory(factory);
- return factory;
+ return builder().build();
}
- /**
- * Creates a new instance factory with the provided initial bindings and
subsequently
- * {@linkplain #initializeFactory(ConfigurableInstanceFactory)
initializes} it.
- *
- * @param bindings the bindings to register before initializing the factory
- * @return the initialized instance factory
- */
- public static ConfigurableInstanceFactory createInitializedFactory(final
Binding<?>... bindings) {
- final var factory = createFactory();
- for (final Binding<?> binding : bindings) {
- factory.registerBinding(binding);
+ public static FactoryBuilder builder() {
+ return new FactoryBuilder();
+ }
+
+ public static class FactoryBuilder {
Review Comment:
This file is where a lot of the interesting changes are.
--
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]
