You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@logging.apache.org by rg...@apache.org on 2019/06/29 04:39:07 UTC
[logging-log4j2] 04/04: LOG4J2-2639 - More code review changes
This is an automated email from the ASF dual-hosted git repository.
rgoers pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit 45503f5885a655b10ee870a24cf9f520b5b90833
Author: Ralph Goers <rg...@apache.org>
AuthorDate: Tue Jun 25 21:43:41 2019 -0700
LOG4J2-2639 - More code review changes
---
.../logging/log4j/{internal => }/LogBuilder.java | 29 +++--
.../main/java/org/apache/logging/log4j/Logger.java | 16 +--
.../logging/log4j/internal/DefaultLogBuilder.java | 137 +++++++++------------
.../apache/logging/log4j/spi/AbstractLogger.java | 18 +--
.../java/org/apache/logging/log4j/LoggerTest.java | 6 +-
.../org/apache/logging/log4j/core/LoggerTest.java | 6 +-
6 files changed, 94 insertions(+), 118 deletions(-)
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/internal/LogBuilder.java b/log4j-api/src/main/java/org/apache/logging/log4j/LogBuilder.java
similarity index 68%
rename from log4j-api/src/main/java/org/apache/logging/log4j/internal/LogBuilder.java
rename to log4j-api/src/main/java/org/apache/logging/log4j/LogBuilder.java
index 93b40f8..e18ae40 100644
--- a/log4j-api/src/main/java/org/apache/logging/log4j/internal/LogBuilder.java
+++ b/log4j-api/src/main/java/org/apache/logging/log4j/LogBuilder.java
@@ -14,15 +14,15 @@
* See the license for the specific language governing permissions and
* limitations under the license.
*/
-package org.apache.logging.log4j.internal;
+package org.apache.logging.log4j;
-import org.apache.logging.log4j.Marker;
import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.util.Supplier;
-import java.util.function.Supplier;
/**
- * Interface for constructing log events before logging them.
+ * Interface for constructing log events before logging them. Instances of LogBuilders should only be created
+ * by calling one of the Logger methods that return a LogBuilder.
*/
public interface LogBuilder {
@@ -44,26 +44,25 @@ public interface LogBuilder {
return this;
}
- default LogBuilder withMessage(Message msg) {
- return this;
+ default void log(CharSequence message) {
}
- default LogBuilder withMessage(String msg) {
- return this;
+ default void log(String message) {
}
- default LogBuilder withMessage(Object msg) {
- return this;
+ default void log(String message, Object... params) {
}
- default LogBuilder withParameters(Object... params) {
- return this;
+ default void log(String message, Supplier<?>... params) {
}
- default LogBuilder withParameters(Supplier<Object>... params) {
- return this;
+ default void log(Message message) {
+ }
+
+ default void log(Supplier<Message> messageSupplier) {
}
- default void log() {
+ default void log(Object message) {
+
}
}
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/Logger.java b/log4j-api/src/main/java/org/apache/logging/log4j/Logger.java
index 03e3c42..0d7b090 100644
--- a/log4j-api/src/main/java/org/apache/logging/log4j/Logger.java
+++ b/log4j-api/src/main/java/org/apache/logging/log4j/Logger.java
@@ -16,16 +16,12 @@
*/
package org.apache.logging.log4j;
-import org.apache.logging.log4j.internal.DefaultLogBuilder;
-import org.apache.logging.log4j.internal.LogBuilder;
import org.apache.logging.log4j.message.EntryMessage;
import org.apache.logging.log4j.message.Message;
import org.apache.logging.log4j.message.MessageFactory;
import org.apache.logging.log4j.util.MessageSupplier;
import org.apache.logging.log4j.util.Supplier;
-import java.util.List;
-
/**
* This is the central interface in the log4j package. Most logging operations, except configuration, are done through
* this interface.
@@ -4217,7 +4213,7 @@ public interface Logger {
* @return a LogBuilder.
* @since 3.0
*/
- default LogBuilder trace() {
+ default LogBuilder atTrace() {
return LogBuilder.NOOP;
}
/**
@@ -4225,7 +4221,7 @@ public interface Logger {
* @return a LogBuilder.
* @since 3.0
*/
- default LogBuilder debug() {
+ default LogBuilder atDebug() {
return LogBuilder.NOOP;
}
/**
@@ -4233,7 +4229,7 @@ public interface Logger {
* @return a LogBuilder.
* @since 3.0
*/
- default LogBuilder info() {
+ default LogBuilder atInfo() {
return LogBuilder.NOOP;
}
/**
@@ -4241,7 +4237,7 @@ public interface Logger {
* @return a LogBuilder.
* @since 3.0
*/
- default LogBuilder warn() {
+ default LogBuilder atWarn() {
return LogBuilder.NOOP;
}
/**
@@ -4249,7 +4245,7 @@ public interface Logger {
* @return a LogBuilder.
* @since 3.0
*/
- default LogBuilder error() {
+ default LogBuilder atError() {
return LogBuilder.NOOP;
}
/**
@@ -4257,7 +4253,7 @@ public interface Logger {
* @return a LogBuilder.
* @since 3.0
*/
- default LogBuilder fatal() {
+ default LogBuilder atFatal() {
return LogBuilder.NOOP;
}
/**
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/internal/DefaultLogBuilder.java b/log4j-api/src/main/java/org/apache/logging/log4j/internal/DefaultLogBuilder.java
index 6d1681f..6d97f83 100644
--- a/log4j-api/src/main/java/org/apache/logging/log4j/internal/DefaultLogBuilder.java
+++ b/log4j-api/src/main/java/org/apache/logging/log4j/internal/DefaultLogBuilder.java
@@ -17,17 +17,19 @@
package org.apache.logging.log4j.internal;
import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.LogBuilder;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.Marker;
import org.apache.logging.log4j.message.Message;
import org.apache.logging.log4j.message.SimpleMessage;
import org.apache.logging.log4j.status.StatusLogger;
+import org.apache.logging.log4j.util.LambdaUtil;
import org.apache.logging.log4j.util.StackLocatorUtil;
import org.apache.logging.log4j.util.Supplier;
/**
- * Collects data for a log event and then logs it.
+ * Collects data for a log event and then logs it. This class should be considered private.
*/
public class DefaultLogBuilder implements LogBuilder {
private static Message EMPTY_MESSAGE = new SimpleMessage("");
@@ -39,18 +41,12 @@ public class DefaultLogBuilder implements LogBuilder {
private Marker marker;
private Throwable throwable;
private StackTraceElement location;
- private Object object;
- private Message msg;
- private String textMessage;
- private Supplier<Message> supplier;
- private Object[] parameters;
private volatile boolean inUse;
private long threadId;
public DefaultLogBuilder(Logger logger, Level level) {
this.logger = logger;
this.level = level;
- this.inUse = false;
this.threadId = Thread.currentThread().getId();
this.inUse = true;
}
@@ -61,17 +57,17 @@ public class DefaultLogBuilder implements LogBuilder {
this.threadId = Thread.currentThread().getId();
}
- public LogBuilder setLevel(Level level) {
+ /**
+ * This method should be considered internal. It is used to reset the LogBuilder for a new log message.
+ * @param level The logging level for this event.
+ * @return This LogBuilder instance.
+ */
+ public LogBuilder reset(Level level) {
this.inUse = true;
this.level = level;
this.marker = null;
this.throwable = null;
this.location = null;
- this.object = null;
- this.msg = null;
- this.textMessage = null;
- this.supplier = null;
- this.parameters = null;
return this;
}
@@ -95,93 +91,78 @@ public class DefaultLogBuilder implements LogBuilder {
return this;
}
- public LogBuilder withMessage(String msg) {
- this.textMessage = msg;
- return this;
+ public boolean isInUse() {
+ return inUse;
}
- public LogBuilder withMessage(Object msg) {
- this.object = msg;
- return this;
+ @Override
+ public void log(Message message) {
+ if (isValid()) {
+ logMessage(message);
+ }
}
- public LogBuilder withMessage(Message msg) {
- this.msg = msg;
- return this;
+ @Override
+ public void log(CharSequence message) {
+ if (isValid()) {
+ logMessage(logger.getMessageFactory().newMessage(message));
+ }
}
- public LogBuilder withMessage(Supplier<Message> supplier) {
- this.supplier = supplier;
- return this;
+ @Override
+ public void log(String message) {
+ if (isValid()) {
+ logMessage(logger.getMessageFactory().newMessage(message));
+ }
}
- public LogBuilder withParameters(Object... params) {
- if (params != null && params.length > 0) {
- if (parameters == null) {
- parameters = params;
- } else {
- Object[] prev = parameters;
- int count = parameters.length + params.length;
- parameters = new Object[count];
- System.arraycopy(prev, 0, parameters, 0, prev.length);
- System.arraycopy(params, 0, parameters, prev.length, params.length);
- }
+ @Override
+ public void log(String message, Object... params) {
+ if (isValid()) {
+ logMessage(logger.getMessageFactory().newMessage(message, params));
}
- return this;
}
- @SafeVarargs
- public final LogBuilder withParameters(java.util.function.Supplier<Object>... params) {
- if (params != null && params.length > 0) {
- if (parameters == null) {
- parameters = new Object[params.length];
- for (int i = 0; i < params.length; ++i) {
- parameters[i] = params[i].get();
- }
- } else {
- Object[] prev = parameters;
- int count = parameters.length + params.length;
- parameters = new Object[count];
- System.arraycopy(prev, 0, parameters, 0, prev.length);
- for (int i = 0; i < params.length; ++i) {
- parameters[prev.length + i] = params[i].get();
- }
- }
- }
- return this;
+ @Override
+ public void log(String message, Supplier<?>... params) {
+ if (isValid()) {
+ logMessage(logger.getMessageFactory().newMessage(message, LambdaUtil.getAll(params)));
+ }
}
- public boolean isInUse() {
- return inUse;
+ @Override
+ public void log(Supplier<Message> messageSupplier) {
+ if (isValid()) {
+ logMessage(messageSupplier.get());
+ }
}
- public void log() {
+ @Override
+ public void log(Object message) {
+ if (isValid()) {
+ logMessage(logger.getMessageFactory().newMessage(message));
+ }
+ }
+
+ private void logMessage(Message message) {
+ try {
+ logger.logMessage(level, marker, FQCN, location, message, throwable);
+ } finally {
+ inUse = false;
+ }
+ }
+
+ private boolean isValid() {
if (!inUse) {
LOGGER.warn("Attempt to reuse LogBuilder was ignored. {}",
StackLocatorUtil.getCallerClass(2));
- return;
+ return false ;
}
if (this.threadId != Thread.currentThread().getId()) {
LOGGER.warn("LogBuilder can only be used on the owning thread. {}",
StackLocatorUtil.getCallerClass(2));
+ return false;
}
- try {
- Message message;
- if (msg != null) {
- message = msg;
- } else if (supplier != null) {
- message = supplier.get();
- } else if (object != null) {
- message = logger.getMessageFactory().newMessage(object);
- } else if (textMessage != null) {
- message = logger.getMessageFactory().newMessage(textMessage, parameters);
- } else {
- message = EMPTY_MESSAGE;
- }
- logger.logMessage(level, marker, FQCN, location, message, throwable);
- } finally {
- inUse = false;
- }
-
+ return true;
}
}
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java b/log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java
index 4918c52..02a9f57 100644
--- a/log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java
+++ b/log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java
@@ -21,7 +21,7 @@ import org.apache.logging.log4j.LoggingException;
import org.apache.logging.log4j.Marker;
import org.apache.logging.log4j.MarkerManager;
import org.apache.logging.log4j.internal.DefaultLogBuilder;
-import org.apache.logging.log4j.internal.LogBuilder;
+import org.apache.logging.log4j.LogBuilder;
import org.apache.logging.log4j.message.DefaultFlowMessageFactory;
import org.apache.logging.log4j.message.EntryMessage;
import org.apache.logging.log4j.message.FlowMessageFactory;
@@ -2755,7 +2755,7 @@ public abstract class AbstractLogger implements ExtendedLogger, Serializable {
* @since 3.0
*/
@Override
- public LogBuilder trace() {
+ public LogBuilder atTrace() {
return atLevel(Level.TRACE);
}
/**
@@ -2764,7 +2764,7 @@ public abstract class AbstractLogger implements ExtendedLogger, Serializable {
* @since 3.0
*/
@Override
- public LogBuilder debug() {
+ public LogBuilder atDebug() {
return atLevel(Level.DEBUG);
}
/**
@@ -2773,7 +2773,7 @@ public abstract class AbstractLogger implements ExtendedLogger, Serializable {
* @since 3.0
*/
@Override
- public LogBuilder info() {
+ public LogBuilder atInfo() {
return atLevel(Level.INFO);
}
/**
@@ -2782,7 +2782,7 @@ public abstract class AbstractLogger implements ExtendedLogger, Serializable {
* @since 3.0
*/
@Override
- public LogBuilder warn() {
+ public LogBuilder atWarn() {
return atLevel(Level.WARN);
}
/**
@@ -2791,7 +2791,7 @@ public abstract class AbstractLogger implements ExtendedLogger, Serializable {
* @since 3.0
*/
@Override
- public LogBuilder error() {
+ public LogBuilder atError() {
return atLevel(Level.ERROR);
}
/**
@@ -2800,7 +2800,7 @@ public abstract class AbstractLogger implements ExtendedLogger, Serializable {
* @since 3.0
*/
@Override
- public LogBuilder fatal() {
+ public LogBuilder atFatal() {
return atLevel(Level.FATAL);
}
/**
@@ -2814,7 +2814,7 @@ public abstract class AbstractLogger implements ExtendedLogger, Serializable {
if (builder.isInUse()) {
return new DefaultLogBuilder(this);
}
- return logBuilder.get().setLevel(Level.OFF);
+ return builder.reset(Level.OFF);
}
/**
* Constuct a log event.
@@ -2828,7 +2828,7 @@ public abstract class AbstractLogger implements ExtendedLogger, Serializable {
if (builder.isInUse()) {
return new DefaultLogBuilder(this, level);
}
- return logBuilder.get().setLevel(level);
+ return builder.reset(level);
} else {
return LogBuilder.NOOP;
}
diff --git a/log4j-api/src/test/java/org/apache/logging/log4j/LoggerTest.java b/log4j-api/src/test/java/org/apache/logging/log4j/LoggerTest.java
index 3388515..978c08d 100644
--- a/log4j-api/src/test/java/org/apache/logging/log4j/LoggerTest.java
+++ b/log4j-api/src/test/java/org/apache/logging/log4j/LoggerTest.java
@@ -59,9 +59,9 @@ public class LoggerTest {
@Test
public void builder() {
- logger.debug().withLocation().withMessage("Hello").log();
- logger.error().withMarker(marker).withMessage("Hello {}").withParameters("John").log();
- logger.warn().withMessage(new SimpleMessage("Log4j rocks!")).withThrowable(new Throwable("This is a test")).log();
+ logger.atDebug().withLocation().log("Hello");
+ logger.atError().withMarker(marker).log("Hello {}", "John");
+ logger.atWarn().withThrowable(new Throwable("This is a test")).log((Message) new SimpleMessage("Log4j rocks!"));
assertEquals(3, results.size());
assertThat("Incorrect message 1", results.get(0), equalTo(" DEBUG org.apache.logging.log4j.LoggerTest.builder(LoggerTest.java:62) Hello"));
assertThat("Incorrect message 2", results.get(1), equalTo("test ERROR Hello John"));
diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/LoggerTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/LoggerTest.java
index 59a4465..e385277 100644
--- a/log4j-core/src/test/java/org/apache/logging/log4j/core/LoggerTest.java
+++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/LoggerTest.java
@@ -85,10 +85,10 @@ public class LoggerTest {
@Test
public void builder() {
- logger.atDebug().withLocation().withMessage("Hello").log();
+ logger.atDebug().withLocation().log("Hello");
Marker marker = MarkerManager.getMarker("test");
- logger.atError().withMarker(marker).withMessage("Hello {}").withParameters("John").log();
- logger.atWarn().withMessage(new SimpleMessage("Log4j rocks!")).withThrowable(new Throwable("This is a test")).log();
+ logger.atError().withMarker(marker).log("Hello {}", "John");
+ logger.atWarn().withThrowable(new Throwable("This is a test")).log((Message) new SimpleMessage("Log4j rocks!"));
final List<LogEvent> events = app.getEvents();
assertEventCount(events, 3);
assertEquals("Incorrect location", "org.apache.logging.log4j.core.LoggerTest.builder(LoggerTest.java:88)", events.get(0).getSource().toString());