Update DI system with DSL for configuring bindings #2148

jvz · 2024-01-02T05:34:28Z

DI.FactoryBuilder and related builder classes for configuring a ConfigurableInstanceFactory.
Repeatable @TestBinding test annotation for specifying simple bindings in tests.
@LegacyLoggerContextSource test annotation for specifying v1-style configuration files.
Test extension updates to improve compatibility of different annotations.
Various test updates to make use of said test extension updates.
Some nullability updates that got pulled in from my changes in spi-updates.

Signed-off-by: Matt Sicker <[email protected]>

- `DI.FactoryBuilder` and related builder classes for configuring a `ConfigurableInstanceFactory`. - Repeatable `@TestBinding` test annotation for specifying simple bindings in tests. - `@LegacyLoggerContextSource` test annotation for specifying v1-style configuration files. - Test extension updates to improve compatibility of different annotations. Related to apache#2147 Signed-off-by: Matt Sicker <[email protected]>

log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/Log4jExtension.java

jvz · 2024-01-02T05:41:31Z

log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/LoggerContextSource.java

-    /**
-     * Toggles Log4j1 configuration file compatibility mode for XML and properties files.
-     */
-    boolean v1config() default false;


Extracted to new annotation.

jvz · 2024-01-02T05:42:00Z

log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/LoggerContextSource.java

-    /**
-     * Determines whether to bootstrap a fresh LoggerContextFactory.
-     */
-    boolean bootstrap() default false;


No longer relevant; we just bootstrap them all the time depending on where the annotation is used.

jvz · 2024-01-02T05:42:08Z

log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/LoggerContextSource.java

-     * 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;


Extracted to a new annotation.

jvz · 2024-01-02T05:43:37Z

log4j-core-test/src/test/java/org/apache/logging/log4j/core/time/ClockFactoryTest.java

    public void testSpecifySystemClockShort() {
-        DI.initializeFactory(instanceFactory);
-        assertThat(instanceFactory.getInstance(Clock.class)).isInstanceOf(SystemClock.class);
+        final Clock clock = builder.addInitialBindingFrom(Clock.KEY)


Test updates are also related to #1977.

jvz · 2024-01-02T05:44:32Z

...core-test/src/test/java/org/apache/logging/log4j/core/util/ShutdownCallbackRegistryTest.java

 import org.apache.logging.log4j.status.StatusLogger;
 import org.junit.jupiter.api.Test;

+@TestBinding(api = ShutdownCallbackRegistry.class, implementation = ShutdownCallbackRegistryTest.Registry.class)


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/src/main/java/org/apache/logging/log4j/core/LoggerContext.java

jvz · 2024-01-02T05:45:24Z

log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java

        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())) {


Various flipping around is from nullability analysis from some time where Node::getName is technically nullable.

jvz · 2024-01-02T05:46:20Z

log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java

-        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);


Also noticed from nullability analysis that calling Level::valueOf was incorrect here as it couldn't return null.

jvz · 2024-01-02T05:48:01Z

log4j-plugins/src/main/java/org/apache/logging/log4j/plugins/di/DI.java

+        return new FactoryBuilder();
+    }
+
+    public static class FactoryBuilder {


This file is where a lot of the interesting changes are.

ppkarwasz

In general this looks good to me.

I am only concerned with DI changes so late in the 3.x lifecycle. As far as know the DI system is mostly unreviewed/unknown (i.e. except you, no other committer can really say he knows the code). After this PR, can we stabilize it so that people can catch up?

Regarding the nullability annotations, I think this merits a thread on dev@logging. If all committers are not onboard, it is useless to add them.

log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/Log4jExtension.java

ppkarwasz · 2024-01-02T09:10:04Z

log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/Log4jExtension.java

+
+class Log4jExtension implements BeforeAllCallback, BeforeEachCallback, AfterEachCallback {
+    private static final String FQCN = Log4jExtension.class.getName();
+    private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(Log4jExtension.class);


We could use a single Namepace for all our extensions. There is one in ExtensionContextAnchor.

Yeah, I was wondering what you'd think about that. I had originally used ExtensionContextAnchor directly here, but it works with its own namespace, too. Seems like something we can normalize?

log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/TestBindings.java

src/changelog/.3.x.x/2147_add_instance_factory_dsls.xml

src/site/_release-notes/_3.x.x.adoc

jvz · 2024-01-02T16:35:12Z

The "late" DI changes are being made now based because I wasn't trying to indefinitely delay pre-3.0.0 releases. In the lead up to 2.0.0, we accepted API changes up until 2.0.0 was cut (2.0 technically).

This ensures that when multiple test methods define test bindings, said bindings do not clobber one another.

jvz · 2024-01-02T18:38:20Z

log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/LoggingResolvers.java

+/**
+ * Handles parameter resolution of a {@link LoggerContext}.
+ */
+class LoggerContextResolver extends TypeBasedParameterResolver<LoggerContext> {


Since all these ParameterResolver extensions are package-private and only used by this particular file, I combined them all into one file. Less lines wasted on license headers 😂

jvz · 2024-01-02T18:39:40Z

log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/TestBinding.java

+    /**
+     * Specifies the {@linkplain org.apache.logging.log4j.plugins.di.Key#forClass(Class) class to use as a key} for this binding.
+     */
+    Class<?> api();


Will note that some sort of annotation-specific way of specifying a Key here would also be useful, but I haven't had a need for that particular syntax yet as all the bindings needed in tests from this annotation use plain interfaces as the key rather than any particular namespace, name, qualifier, etc.

jvz added 2 commits January 1, 2024 23:22

Extract interface from BindingMap

05879a0

Signed-off-by: Matt Sicker <[email protected]>

jvz added api Affects the public API plugins Affects the plugin system labels Jan 2, 2024

jvz added this to the 3.0.0 milestone Jan 2, 2024

jvz requested a review from ppkarwasz January 2, 2024 05:34

jvz self-assigned this Jan 2, 2024

jvz linked an issue Jan 2, 2024 that may be closed by this pull request

Update DI system with DSL for configuring bindings #2147

Closed

jvz commented Jan 2, 2024

View reviewed changes

log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/Log4jExtension.java Show resolved Hide resolved

jvz commented Jan 2, 2024

View reviewed changes

ppkarwasz approved these changes Jan 2, 2024

View reviewed changes

jvz and others added 6 commits January 2, 2024 11:40

Make TestBinding container annotation inner class

3702f98

Clean up changelog entry for apache#2147

9b49a5d

Combine LoggingResolvers extensions into one file

1f9adf1

Add docs to DI builder DSL

3d49c4c

Ensure Log4jExtension uses copy of builder

891088f

This ensures that when multiple test methods define test bindings, said bindings do not clobber one another.

Merge branch 'main' into issue-2147

d9c623c

jvz commented Jan 2, 2024

View reviewed changes

jvz merged commit c72576d into apache:main Jan 2, 2024

jvz deleted the issue-2147 branch January 2, 2024 19:48

ppkarwasz modified the milestones: 3.0.0, 3.0.0-beta2 Feb 17, 2024

Uh oh!

Update DI system with DSL for configuring bindings #2148

Update DI system with DSL for configuring bindings #2148

Uh oh!

Conversation

jvz commented Jan 2, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ppkarwasz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jvz commented Jan 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!