Improvements on the logger message factory namespacing (#2936)

vy · vy · commit 602bf88d2c75 · 2024-09-17T21:06:48.000+02:00
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java b/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java
@@ -95,7 +95,7 @@ public Object getExternalContext() {
 
     @Override
     public ExtendedLogger getLogger(final String name) {
-        return getLogger(name, null);
+        return getLogger(name, DEFAULT_MESSAGE_FACTORY);
     }
 
     @Override
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java b/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java
@@ -259,26 +259,29 @@ public boolean hasLogger(final String name, final Class<? extends MessageFactory
     }
 
     /**
-     * Registers the provided logger using the given name – <b>message factory parameter is ignored</b> and the one from the logger will be used instead.
+     * Registers the provided logger.
+     * <b>Logger name and message factory parameters are ignored</b>, those will be obtained from the logger instead.
      *
-     * @param name a logger name
+     * @param name ignored – kept for backward compatibility
      * @param messageFactory ignored – kept for backward compatibility
      * @param logger a logger instance
      * @deprecated As of version {@code 2.25.0}, planned to be removed!
      * Use {@link #computeIfAbsent(String, MessageFactory, BiFunction)} instead.
      */
     @Deprecated
-    public void putIfAbsent(final String name, @Nullable final MessageFactory messageFactory, final T logger) {
+    public void putIfAbsent(
+            @Nullable final String name, @Nullable final MessageFactory messageFactory, final T logger) {
 
         // Check arguments
-        requireNonNull(name, "name");
         requireNonNull(logger, "logger");
 
         // Insert the logger
         writeLock.lock();
         try {
+            final String loggerName = logger.getName();
             final Map<MessageFactory, WeakReference<T>> loggerRefByMessageFactory =
-                    loggerRefByMessageFactoryByName.computeIfAbsent(name, this::createLoggerRefByMessageFactoryMap);
+                    loggerRefByMessageFactoryByName.computeIfAbsent(
+                            loggerName, this::createLoggerRefByMessageFactoryMap);
             final MessageFactory loggerMessageFactory = logger.getMessageFactory();
             final WeakReference<T> loggerRef = loggerRefByMessageFactory.get(loggerMessageFactory);
             if (loggerRef == null || loggerRef.get() == null) {
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/LoggerContextTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/LoggerContextTest.java
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.logging.log4j.core;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
+
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.message.MessageFactory2;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
+
+class LoggerContextTest {
+
+    @Test
+    void newInstance_should_honor_name_and_message_factory(final TestInfo testInfo) {
+        final String testName = testInfo.getDisplayName();
+        try (final LoggerContext loggerContext = new LoggerContext(testName)) {
+            final String loggerName = testName + "-loggerName";
+            final MessageFactory2 messageFactory = mock(MessageFactory2.class);
+            final Logger logger = loggerContext.newInstance(loggerContext, loggerName, messageFactory);
+            assertThat(logger.getName()).isEqualTo(loggerName);
+            assertThat((MessageFactory) logger.getMessageFactory()).isSameAs(messageFactory);
+        }
+    }
+}
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/async/AsyncLoggerContextTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/async/AsyncLoggerContextTest.java
@@ -16,23 +16,30 @@
  */
 package org.apache.logging.log4j.core.async;
 
-import static org.junit.Assert.assertTrue;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
 
 import org.apache.logging.log4j.Logger;
-import org.apache.logging.log4j.core.LoggerContext;
-import org.apache.logging.log4j.core.test.CoreLoggerContexts;
 import org.apache.logging.log4j.core.test.categories.AsyncLoggers;
-import org.junit.Test;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.message.MessageFactory2;
 import org.junit.experimental.categories.Category;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
 
 @Category(AsyncLoggers.class)
-public class AsyncLoggerContextTest {
+class AsyncLoggerContextTest {
 
     @Test
-    public void testNewInstanceReturnsAsyncLogger() {
-        final Logger logger = new AsyncLoggerContext("a").newInstance(new LoggerContext("a"), "a", null);
-        assertTrue(logger instanceof AsyncLogger);
-
-        CoreLoggerContexts.stopLoggerContext(); // stop async thread
+    void verify_newInstance(final TestInfo testInfo) {
+        final String testName = testInfo.getDisplayName();
+        try (final AsyncLoggerContext loggerContext = new AsyncLoggerContext(testName)) {
+            final String loggerName = testName + "-loggerName";
+            final MessageFactory2 messageFactory = mock(MessageFactory2.class);
+            final Logger logger = loggerContext.newInstance(loggerContext, loggerName, messageFactory);
+            assertThat(logger).isInstanceOf(AsyncLogger.class);
+            assertThat(logger.getName()).isEqualTo(loggerName);
+            assertThat((MessageFactory) logger.getMessageFactory()).isSameAs(messageFactory);
+        }
     }
 }
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java
@@ -54,7 +54,6 @@
 import org.apache.logging.log4j.spi.LoggerRegistry;
 import org.apache.logging.log4j.spi.Terminable;
 import org.apache.logging.log4j.spi.ThreadContextMapFactory;
-import org.apache.logging.log4j.status.StatusLogger;
 import org.apache.logging.log4j.util.PropertiesUtil;
 import org.jspecify.annotations.Nullable;
 
@@ -506,7 +505,7 @@ public Object getExternalContext() {
      */
     @Override
     public Logger getLogger(final String name) {
-        return getLogger(name, null);
+        return getLogger(name, DEFAULT_MESSAGE_FACTORY);
     }
 
     /**
@@ -815,19 +814,7 @@ private void initApiModule() {
     }
 
     private Logger newInstance(final String name, final MessageFactory messageFactory) {
-        final Logger logger = newInstance(this, name, messageFactory);
-        final MessageFactory loggerMessageFactory = logger.getMessageFactory();
-        if (!loggerMessageFactory.equals(messageFactory)) {
-            StatusLogger.getLogger()
-                    .error(
-                            "Newly created logger with name `{}` and message factory `{}` was actually requested to be created with a different message factory: `{}`.\n"
-                                    + "This generally hints a problem.\n"
-                                    + "Please report this using the Log4j project issue tracker.",
-                            name,
-                            loggerMessageFactory,
-                            messageFactory);
-        }
-        return logger;
+        return newInstance(this, name, messageFactory);
     }
 
     // LOG4J2-151: changed visibility from private to protected
diff --git a/log4j-taglib/src/main/java/org/apache/logging/log4j/taglib/Log4jTaglibLoggerContext.java b/log4j-taglib/src/main/java/org/apache/logging/log4j/taglib/Log4jTaglibLoggerContext.java
@@ -66,7 +66,7 @@ public Object getExternalContext() {
 
     @Override
     public Log4jTaglibLogger getLogger(final String name) {
-        return getLogger(name, null);
+        return getLogger(name, DEFAULT_MESSAGE_FACTORY);
     }
 
     @Override
@@ -84,7 +84,7 @@ private Log4jTaglibLogger createLogger(final String name, @Nullable final Messag
 
     @Override
     public boolean hasLogger(final String name) {
-        return loggerRegistry.hasLogger(name);
+        return loggerRegistry.hasLogger(name, DEFAULT_MESSAGE_FACTORY);
     }
 
     @Override
diff --git a/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLoggerContext.java b/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLoggerContext.java
@@ -45,7 +45,7 @@ public Object getExternalContext() {
 
     @Override
     public ExtendedLogger getLogger(final String name) {
-        return getLogger(name, null);
+        return getLogger(name, DEFAULT_MESSAGE_FACTORY);
     }
 
     @Override
diff --git a/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLoggerContext.java b/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLoggerContext.java
@@ -38,7 +38,7 @@ public Object getExternalContext() {
 
     @Override
     public ExtendedLogger getLogger(final String name) {
-        return getLogger(name, null);
+        return getLogger(name, DEFAULT_MESSAGE_FACTORY);
     }
 
     @Override

Original file line number	Diff line number	Diff line change
`@@ -95,7 +95,7 @@ public Object getExternalContext() {`
`95`	`95`
`96`	`96`	`@Override`
`97`	`97`	`public ExtendedLogger getLogger(final String name) {`
`98`		`- return getLogger(name, null);`
	`98`	`+ return getLogger(name, DEFAULT_MESSAGE_FACTORY);`
`99`	`99`	`}`
`100`	`100`
`101`	`101`	`@Override`
Original file line number	Diff line number	Diff line change
`@@ -66,7 +66,7 @@ public Object getExternalContext() {`
`66`	`66`
`67`	`67`	`@Override`
`68`	`68`	`public Log4jTaglibLogger getLogger(final String name) {`
`69`		`- return getLogger(name, null);`
	`69`	`+ return getLogger(name, DEFAULT_MESSAGE_FACTORY);`
`70`	`70`	`}`
`71`	`71`
`72`	`72`	`@Override`
`@@ -84,7 +84,7 @@ private Log4jTaglibLogger createLogger(final String name, @Nullable final Messag`
`84`	`84`
`85`	`85`	`@Override`
`86`	`86`	`public boolean hasLogger(final String name) {`
`87`		`- return loggerRegistry.hasLogger(name);`
	`87`	`+ return loggerRegistry.hasLogger(name, DEFAULT_MESSAGE_FACTORY);`
`88`	`88`	`}`
`89`	`89`
`90`	`90`	`@Override`
Original file line number	Diff line number	Diff line change
`@@ -45,7 +45,7 @@ public Object getExternalContext() {`
`45`	`45`
`46`	`46`	`@Override`
`47`	`47`	`public ExtendedLogger getLogger(final String name) {`
`48`		`- return getLogger(name, null);`
	`48`	`+ return getLogger(name, DEFAULT_MESSAGE_FACTORY);`
`49`	`49`	`}`
`50`	`50`
`51`	`51`	`@Override`
Original file line number	Diff line number	Diff line change
`@@ -38,7 +38,7 @@ public Object getExternalContext() {`
`38`	`38`
`39`	`39`	`@Override`
`40`	`40`	`public ExtendedLogger getLogger(final String name) {`
`41`		`- return getLogger(name, null);`
	`41`	`+ return getLogger(name, DEFAULT_MESSAGE_FACTORY);`
`42`	`42`	`}`
`43`	`43`
`44`	`44`	`@Override`