From 45ae984221f0326e3b5395be113e8d80cf392f68 Mon Sep 17 00:00:00 2001 From: Balazs Toth Date: Sun, 2 Dec 2018 18:59:59 +0100 Subject: [PATCH] default methods to Log interface to prevent unnecessary payload creation This change require Java 8 and add default interface methods to simplify the usage of the log levels. No if statement required to prevent log message object creation if those methods used. --- pom.xml | 6 +- .../java/org/apache/commons/logging/Log.java | 80 +++++ .../LogInterfaceDefaultMethodsTestCase.java | 305 ++++++++++++++++++ 3 files changed, 388 insertions(+), 3 deletions(-) create mode 100644 src/test/java/org/apache/commons/logging/LogInterfaceDefaultMethodsTestCase.java diff --git a/pom.xml b/pom.xml index 84e80d832..2de717440 100644 --- a/pom.xml +++ b/pom.xml @@ -32,7 +32,7 @@ under the License. commons-logging commons-logging Apache Commons Logging - 1.2.1-SNAPSHOT + 1.3.0-SNAPSHOT Apache Commons Logging is a thin adapter allowing configurable bridging to other, well known logging systems. http://commons.apache.org/proper/commons-logging/ @@ -530,8 +530,8 @@ under the License. - 1.6 - 1.6 + 1.8 + 1.8 logging org.apache.commons.logging 1.2 diff --git a/src/main/java/org/apache/commons/logging/Log.java b/src/main/java/org/apache/commons/logging/Log.java index be6ffa893..4754fb355 100644 --- a/src/main/java/org/apache/commons/logging/Log.java +++ b/src/main/java/org/apache/commons/logging/Log.java @@ -17,6 +17,8 @@ package org.apache.commons.logging; +import java.util.function.Supplier; + /** * A simple logging interface abstracting logging APIs. In order to be * instantiated successfully by {@link LogFactory}, classes that implement @@ -50,6 +52,11 @@ * } * *

+ * or + *

+ * 

+ *     log.debug(() -> theResult);
+ *
* Configuration of the underlying logging system will generally be done * external to the Logging APIs, through whatever mechanism is supported by * that system. @@ -58,6 +65,79 @@ */ public interface Log { + default void debug(Supplier msgSupplier) { + if (isDebugEnabled()) { + debug(msgSupplier != null ? msgSupplier.get() : null); + } + } + + default void debug(Supplier msgSupplier, Throwable t) { + if (isDebugEnabled()) { + debug(msgSupplier != null ? msgSupplier.get() : null, t); + } + } + + default void error(Supplier msgSupplier) { + if (isErrorEnabled()) { + error(msgSupplier != null ? msgSupplier.get() : null); + } + } + + default void error(Supplier msgSupplier, Throwable t) { + if (isErrorEnabled()) { + error(msgSupplier != null ? msgSupplier.get() : null, t); + } + } + + default void fatal(Supplier msgSupplier) { + if (isFatalEnabled()) { + fatal(msgSupplier != null ? msgSupplier.get() : null); + } + } + + default void fatal(Supplier msgSupplier, Throwable t) { + if (isFatalEnabled()) { + fatal(msgSupplier != null ? msgSupplier.get() : null, t); + } + } + + default void info(Supplier msgSupplier) { + if (isInfoEnabled()) { + info(msgSupplier != null ? msgSupplier.get() : null); + } + } + + default void info(Supplier msgSupplier, Throwable t) { + if (isInfoEnabled()) { + info(msgSupplier != null ? msgSupplier.get() : null, t); + } + } + + default void trace(Supplier msgSupplier) { + if (isTraceEnabled()) { + trace(msgSupplier != null ? msgSupplier.get() : null); + } + } + + default void trace(Supplier msgSupplier, Throwable t) { + if (isTraceEnabled()) { + trace(msgSupplier != null ? msgSupplier.get() : null, t); + } + } + + default void warn(Supplier msgSupplier) { + if (isWarnEnabled()) { + warn(msgSupplier != null ? msgSupplier.get() : null); + } + } + + default void warn(Supplier msgSupplier, Throwable t) { + if (isWarnEnabled()) { + warn(msgSupplier != null ? msgSupplier.get() : null, t); + } + } + + /** * Logs a message with debug log level. * diff --git a/src/test/java/org/apache/commons/logging/LogInterfaceDefaultMethodsTestCase.java b/src/test/java/org/apache/commons/logging/LogInterfaceDefaultMethodsTestCase.java new file mode 100644 index 000000000..c188a94f6 --- /dev/null +++ b/src/test/java/org/apache/commons/logging/LogInterfaceDefaultMethodsTestCase.java @@ -0,0 +1,305 @@ +package org.apache.commons.logging; + +import java.util.EnumSet; +import java.util.Set; +import java.util.UUID; +import java.util.function.BiConsumer; +import java.util.function.Consumer; +import java.util.function.Supplier; + +import org.apache.commons.logging.LogInterfaceDefaultMethodsTestCase.TestImplementation.LogLevel; + +import junit.framework.TestCase; + +/* + * Test for the default methods uses Supplier on the Log interface for avoid unnecessary payload creation + */ +public class LogInterfaceDefaultMethodsTestCase extends TestCase { + static class TestImplementation implements Log { + enum LogLevel { + DEBUG, ERROR, FATAL, INFO, TRACE, WARN + } + private LogLevel lastLoggedLevel = null; + private Object lastLoggedMessage = null; + private Throwable lastLoggedException = null; + + private Set enabledLevels = EnumSet.noneOf(LogLevel.class); + + + LogLevel getLastLoggedLevel() { + return this.lastLoggedLevel; + } + + Object getLastLoggedMessage() { + return this.lastLoggedMessage; + } + + Throwable getLastLoggedException() { + return this.lastLoggedException; + } + + void reset() { + this.recordLogEntry(null, null, null); + this.enabledLevels.clear(); + } + + void enableLogLevel(LogLevel level) { + this.enabledLevels.add(level); + } + + void disableLogLevel(LogLevel level) { + this.enabledLevels.remove(level); + } + + Set getEnabledLevels() { + return enabledLevels; + } + + private void recordLogEntry(LogLevel level, Object message, Throwable t) { + this.lastLoggedLevel = level; + this.lastLoggedMessage = message; + this.lastLoggedException = t; + } + + @Override + public void debug(Object message) { + this.recordLogEntry(LogLevel.DEBUG, message, null); + } + + @Override + public void debug(Object message, Throwable t) { + this.recordLogEntry(LogLevel.DEBUG, message, t); + } + + @Override + public void error(Object message) { + this.recordLogEntry(LogLevel.ERROR, message, null); + } + + @Override + public void error(Object message, Throwable t) { + this.recordLogEntry(LogLevel.ERROR, message, t); + } + + @Override + public void fatal(Object message) { + this.recordLogEntry(LogLevel.FATAL, message, null); + } + + @Override + public void fatal(Object message, Throwable t) { + this.recordLogEntry(LogLevel.FATAL, message, t); + } + + @Override + public void info(Object message) { + this.recordLogEntry(LogLevel.INFO, message, null); + } + + @Override + public void info(Object message, Throwable t) { + this.recordLogEntry(LogLevel.INFO, message, t); + } + + @Override + public void trace(Object message) { + this.recordLogEntry(LogLevel.TRACE, message, null); + } + + @Override + public void trace(Object message, Throwable t) { + this.recordLogEntry(LogLevel.TRACE, message, t); + } + + @Override + public void warn(Object message) { + this.recordLogEntry(LogLevel.WARN, message, null); + } + + @Override + public void warn(Object message, Throwable t) { + this.recordLogEntry(LogLevel.WARN, message, t); + } + + @Override + public boolean isDebugEnabled() { + return this.enabledLevels.contains(LogLevel.DEBUG); + } + + @Override + public boolean isErrorEnabled() { + return this.enabledLevels.contains(LogLevel.ERROR); + } + + @Override + public boolean isFatalEnabled() { + return this.enabledLevels.contains(LogLevel.FATAL); + } + + @Override + public boolean isInfoEnabled() { + return this.enabledLevels.contains(LogLevel.INFO); + } + + @Override + public boolean isTraceEnabled() { + return this.enabledLevels.contains(LogLevel.TRACE); + } + + @Override + public boolean isWarnEnabled() { + return this.enabledLevels.contains(LogLevel.WARN); + } + } + + @SuppressWarnings("serial") + static class TestException extends RuntimeException { + // + } + static class TestObject { + TestObject() { + throw new TestException(); + } + } + + private void testReset(TestImplementation log) { + log.reset(); + assertNull("Reset method must clear the last logged level", log.getLastLoggedLevel()); + assertNull("Reset method must clear the last logged message", log.getLastLoggedMessage()); + assertNull("Reset method must clear the last logged exception", log.getLastLoggedException()); + assertTrue("Reset method must clear the enabled log levels", log.getEnabledLevels().isEmpty()); + } + + private void testLevel(TestImplementation log, + LogLevel level, + Consumer withoutException, + BiConsumer withException, + Consumer> supplierWithoutException, + BiConsumer,Throwable> supplierWithException) { + + // System.out.println("Test default methods on level: "+level.name()); + + Object testMessage = UUID.randomUUID(); + Throwable testException = new TestException(); + + // be sure the normal methods works as expected before test the default methods + withoutException.accept(testMessage); + assertEquals("Dummy implementation should save the level even if the level is diabled ["+level+"]", level, log.getLastLoggedLevel()); + assertEquals("Dummy implementation should save the message even if the level is diabled ["+level+"]", testMessage, log.getLastLoggedMessage()); + assertNull("Dummy implementation should save the exception even if the level is diabled ["+level+"]", log.getLastLoggedException()); + testReset(log); + + withException.accept(testMessage, testException); + assertEquals("Dummy implementation should save the level even if the level is diabled ["+level+"]", level, log.getLastLoggedLevel()); + assertEquals("Dummy implementation should save the message even if the level is diabled ["+level+"]", testMessage, log.getLastLoggedMessage()); + assertEquals("Dummy implementation should save the exception even if the level is diabled ["+level+"]", testException, log.getLastLoggedException()); + testReset(log); + + // debug log level not enabled, message should not appear + supplierWithoutException.accept(() -> testMessage); + assertNull("Default method should prevent to save the level if it is disabled ["+level+"]", log.getLastLoggedLevel()); + assertNull("Default method should prevent to save the message if it is disabled ["+level+"]", log.getLastLoggedMessage()); + assertNull("Default method should prevent to save the exception if it is disabled ["+level+"]", log.getLastLoggedException()); + + supplierWithException.accept(() -> testMessage, testException); + assertNull("Default method should prevent to save the level if it is disabled ["+level+"]", log.getLastLoggedLevel()); + assertNull("Default method should prevent to save the message if it is disabled ["+level+"]", log.getLastLoggedMessage()); + assertNull("Default method should prevent to save the exception if it is disabled ["+level+"]", log.getLastLoggedException()); + + try { + supplierWithoutException.accept(() -> new TestObject()); + } catch (TestException e) { + fail("Supplier should not executed with disabled log level"); + } + + try { + supplierWithException.accept(() -> new TestObject(), testException); + } catch (TestException e) { + fail("Supplier should not executed with disabled log level"); + } + + testReset(log); + + // test default methods with enabled log level + log.enableLogLevel(level); + assertTrue("Requested level must be enabled before the test execution ["+level+"]", log.getEnabledLevels().contains(level)); + supplierWithoutException.accept(() -> testMessage); + assertEquals("Stored log level does not match with the expected", level, log.getLastLoggedLevel()); + assertEquals("Stored log message does not match with the expected", testMessage, log.getLastLoggedMessage()); + assertNull("Stored log exception does not match with the expected", log.getLastLoggedException()); + testReset(log); + + log.enableLogLevel(level); + assertTrue("Requested level must be enabled before the test execution ["+level+"]", log.getEnabledLevels().contains(level)); + supplierWithException.accept(() -> testMessage, testException); + assertEquals("Stored log level does not match with the expected", level, log.getLastLoggedLevel()); + assertEquals("Stored log message does not match with the expected", testMessage, log.getLastLoggedMessage()); + assertEquals("Stored log exception does not match with the expected", testException, log.getLastLoggedException()); + testReset(log); + + // test default methods with enabled log level with NULL supplier + log.enableLogLevel(level); + assertTrue("Requested level must be enabled before the test execution ["+level+"]", log.getEnabledLevels().contains(level)); + supplierWithoutException.accept((Supplier)null); + assertEquals("Stored log level does not match with the expected", level, log.getLastLoggedLevel()); + assertNull("Stored log message does not match with the expected", log.getLastLoggedMessage()); + assertNull("Stored log exception does not match with the expected", log.getLastLoggedException()); + testReset(log); + + log.enableLogLevel(level); + assertTrue("Requested level must be enabled before the test execution ["+level+"]", log.getEnabledLevels().contains(level)); + supplierWithException.accept((Supplier)null, testException); + assertEquals("Stored log level does not match with the expected", level, log.getLastLoggedLevel()); + assertNull("Stored log message does not match with the expected", log.getLastLoggedMessage()); + assertEquals("Stored log exception does not match with the expected", testException, log.getLastLoggedException()); + testReset(log); + + // test default methods with enabled log level with supplier returns null + log.enableLogLevel(level); + assertTrue("Requested level must be enabled before the test execution ["+level+"]", log.getEnabledLevels().contains(level)); + supplierWithoutException.accept(() -> null); + assertEquals("Stored log level does not match with the expected", level, log.getLastLoggedLevel()); + assertNull("Stored log message does not match with the expected", log.getLastLoggedMessage()); + assertNull("Stored log exception does not match with the expected", log.getLastLoggedException()); + testReset(log); + + log.enableLogLevel(level); + assertTrue("Requested level must be enabled before the test execution ["+level+"]", log.getEnabledLevels().contains(level)); + supplierWithException.accept(() -> null, testException); + assertEquals("Stored log level does not match with the expected", level, log.getLastLoggedLevel()); + assertNull("Stored log message does not match with the expected", log.getLastLoggedMessage()); + assertEquals("Stored log exception does not match with the expected", testException, log.getLastLoggedException()); + testReset(log); + + // test default methods with enabled log level and supplier throws exception + log.enableLogLevel(level); + assertTrue("Requested level must be enabled before the test execution ["+level+"]", log.getEnabledLevels().contains(level)); + try { + supplierWithoutException.accept(() -> new TestObject()); + fail("Supplier should executed and throw an exception"); + } catch (TestException e) { + // + } + testReset(log); + + log.enableLogLevel(level); + assertTrue("Requested level must be enabled before the test execution ["+level+"]", log.getEnabledLevels().contains(level)); + try { + supplierWithException.accept(() -> new TestObject(), testException); + fail("Supplier should executed and throw an exception"); + } catch (TestException e) { + // + } + testReset(log); + } + + public void testDefaultMethods() { + Log log = new TestImplementation(); + testLevel((TestImplementation)log, LogLevel.DEBUG, log::debug, log::debug, log::debug, log::debug); + testLevel((TestImplementation)log, LogLevel.ERROR, log::error, log::error, log::error, log::error); + testLevel((TestImplementation)log, LogLevel.FATAL, log::fatal, log::fatal, log::fatal, log::fatal); + testLevel((TestImplementation)log, LogLevel.INFO, log::info, log::info, log::info, log::info); + testLevel((TestImplementation)log, LogLevel.TRACE, log::trace, log::trace, log::trace, log::trace); + testLevel((TestImplementation)log, LogLevel.WARN, log::warn, log::warn, log::warn, log::warn); + } +}