Spring/spring-integration
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

INT-4144: Prevent NPE in the LoggingHandler

Browse Source 
JIRA: https://jira.spring.io/browse/INT-4144
Fixes GH-1936 (https://github.com/spring-projects/spring-integration/issues/1936)

Previously `LoggingHandler` could be used as a standalone object, without any Spring Container initialization.
Even if that doesn't sound reasonable, we should reinstate the logic to avoid breaking changes

* Initialize `expression` and `evaluationContext` during object `<init>` phase
* Some code reformatting to avoid duplicate blocks and cyclomatic complexity

* Simple code formatting
* Additional JavaDocs for `LoggingHandler`

**Cherry-pick to 4.3.x**
This commit is contained in:
Andriy Kryvtsun
2016-10-21 17:13:37 -04:00
committed by Artem Bilan
parent a1a3c2a1ac
commit ee848e9ecf
2 changed files with 95 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files

136
spring-integration-core/src/main/java/org/springframework/integration/handler/LoggingHandler.java
View File

@@ -18,6 +18,7 @@ package org.springframework.integration.handler;
import java.io.PrintWriter; import java.io.PrintWriter;
import java.io.StringWriter; import java.io.StringWriter;
import java.io.Writer;
import org.apache.commons.logging.Log; import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory; import org.apache.commons.logging.LogFactory;
@@ -32,33 +33,36 @@ import org.springframework.util.StringUtils;
/** /**
* MessageHandler implementation that simply logs the Message or its payload depending on the value of the * MessageHandler implementation that simply logs the Message or its payload depending on the value of the
* 'shouldLogFullMessage' property. If logging the payload, and it is assignable to Throwable, it will log the stack * 'shouldLogFullMessage' or SpEL 'logExpression' property.
* If logging the payload, and it is assignable to Throwable, it will log the stack
* trace. By default, it will log the payload only. * trace. By default, it will log the payload only.
* *
* @author Mark Fisher * @author Mark Fisher
* @author Gary Russell * @author Gary Russell
* @author Artem Bilan * @author Artem Bilan
* @author Andriy Kryvtsun
* @since 1.0.1 * @since 1.0.1
*/ */
public class LoggingHandler extends AbstractMessageHandler { public class LoggingHandler extends AbstractMessageHandler {
public enum Level { public enum Level {
FATAL, ERROR, WARN, INFO, DEBUG, TRACE FATAL, ERROR, WARN, INFO, DEBUG, TRACE
} }
private volatile Expression expression;
private volatile boolean expressionSet;
private volatile boolean shouldLogFullMessageSet;
private volatile Level level; private volatile Level level;
private volatile EvaluationContext evaluationContext; private volatile boolean expressionSet;
private volatile Expression expression = EXPRESSION_PARSER.parseExpression("payload");
private volatile EvaluationContext evaluationContext = ExpressionUtils.createStandardEvaluationContext();
private volatile boolean shouldLogFullMessageSet;
private volatile Log messageLogger = this.logger; private volatile Log messageLogger = this.logger;
/** /**
* Create a LoggingHandler with the given log level (case-insensitive). * Create a LoggingHandler with the given log level (case-insensitive).
* <p> * <p>
@@ -68,9 +72,13 @@ public class LoggingHandler extends AbstractMessageHandler {
* @see #LoggingHandler(Level) * @see #LoggingHandler(Level)
*/ */
public LoggingHandler(String level) { public LoggingHandler(String level) {
this(convertLevel(level));
}
private static Level convertLevel(String level) {
Assert.hasText(level, "'level' cannot be empty"); Assert.hasText(level, "'level' cannot be empty");
try { try {
this.level = Level.valueOf(level.toUpperCase()); return Level.valueOf(level.toUpperCase());
} }
catch (IllegalArgumentException e) { catch (IllegalArgumentException e) {
throw new IllegalArgumentException("Invalid log level '" + level throw new IllegalArgumentException("Invalid log level '" + level
@@ -85,8 +93,7 @@ public class LoggingHandler extends AbstractMessageHandler {
* @since 4.3 * @since 4.3
*/ */
public LoggingHandler(Level level) { public LoggingHandler(Level level) {
Assert.notNull(level, "'level' cannot be null"); doSetLevel(level);
this.level = level;
} }
/** /**
@@ -125,6 +132,10 @@ public class LoggingHandler extends AbstractMessageHandler {
* @param level the level. * @param level the level.
*/ */
public void setLevel(Level level) { public void setLevel(Level level) {
doSetLevel(level);
}
private void doSetLevel(Level level) {
Assert.notNull(level, "'level' cannot be null"); Assert.notNull(level, "'level' cannot be null");
this.level = level; this.level = level;
} }
@@ -142,7 +153,7 @@ public class LoggingHandler extends AbstractMessageHandler {
public void setShouldLogFullMessage(boolean shouldLogFullMessage) { public void setShouldLogFullMessage(boolean shouldLogFullMessage) {
Assert.isTrue(!(this.expressionSet), "Cannot set both 'expression' AND 'shouldLogFullMessage' properties"); Assert.isTrue(!(this.expressionSet), "Cannot set both 'expression' AND 'shouldLogFullMessage' properties");
this.shouldLogFullMessageSet = true; this.shouldLogFullMessageSet = true;
this.expression = (shouldLogFullMessage) this.expression = shouldLogFullMessage
? EXPRESSION_PARSER.parseExpression("#root") ? EXPRESSION_PARSER.parseExpression("#root")
: EXPRESSION_PARSER.parseExpression("payload"); : EXPRESSION_PARSER.parseExpression("payload");
} }
@@ -156,66 +167,69 @@ public class LoggingHandler extends AbstractMessageHandler {
protected void onInit() throws Exception { protected void onInit() throws Exception {
super.onInit(); super.onInit();
this.evaluationContext = ExpressionUtils.createStandardEvaluationContext(getBeanFactory()); this.evaluationContext = ExpressionUtils.createStandardEvaluationContext(getBeanFactory());
if (this.expression == null) {
this.expression = EXPRESSION_PARSER.parseExpression("payload");
}
} }
@Override @Override
protected void handleMessageInternal(Message<?> message) throws Exception { protected void handleMessageInternal(Message<?> message) throws Exception {
switch (this.level) { switch (this.level) {
case FATAL: case FATAL:
if (this.messageLogger.isFatalEnabled()) { if (this.messageLogger.isFatalEnabled()) {
this.messageLogger.fatal(createLogMessage(message)); this.messageLogger.fatal(createLogMessage(message));
} }
break; break;
case ERROR: case ERROR:
if (this.messageLogger.isErrorEnabled()) { if (this.messageLogger.isErrorEnabled()) {
this.messageLogger.error(createLogMessage(message)); this.messageLogger.error(createLogMessage(message));
} }
break; break;
case WARN: case WARN:
if (this.messageLogger.isWarnEnabled()) { if (this.messageLogger.isWarnEnabled()) {
this.messageLogger.warn(createLogMessage(message)); this.messageLogger.warn(createLogMessage(message));
} }
break; break;
case INFO: case INFO:
if (this.messageLogger.isInfoEnabled()) { if (this.messageLogger.isInfoEnabled()) {
this.messageLogger.info(createLogMessage(message)); this.messageLogger.info(createLogMessage(message));
} }
break; break;
case DEBUG: case DEBUG:
if (this.messageLogger.isDebugEnabled()) { if (this.messageLogger.isDebugEnabled()) {
this.messageLogger.debug(createLogMessage(message)); this.messageLogger.debug(createLogMessage(message));
} }
break; break;
case TRACE: case TRACE:
if (this.messageLogger.isTraceEnabled()) { if (this.messageLogger.isTraceEnabled()) {
this.messageLogger.trace(createLogMessage(message)); this.messageLogger.trace(createLogMessage(message));
} }
break; break;
default: default:
throw new IllegalStateException("Level '" + this.level + "' is not supported"); throw new IllegalStateException("Level '" + this.level + "' is not supported");
} }
} }
private Object createLogMessage(Message<?> message) { private Object createLogMessage(Message<?> message) {
Object logMessage = this.expression.getValue(this.evaluationContext, message); Object logMessage = this.expression.getValue(this.evaluationContext, message);
if (logMessage instanceof Throwable) { return logMessage instanceof Throwable
StringWriter stringWriter = new StringWriter(); ? createLogMessage((Throwable) logMessage)
if (logMessage instanceof AggregateMessageDeliveryException) { : logMessage;
stringWriter.append(((Throwable) logMessage).getMessage()); }
for (Exception exception : ((AggregateMessageDeliveryException) logMessage).getAggregatedExceptions()) {
exception.printStackTrace(new PrintWriter(stringWriter, true)); private String createLogMessage(Throwable throwable) {
} StringWriter stringWriter = new StringWriter();
if (throwable instanceof AggregateMessageDeliveryException) {
stringWriter.append(throwable.getMessage());
for (Exception exception : ((AggregateMessageDeliveryException) throwable).getAggregatedExceptions()) {
printStackTrace(exception, stringWriter);
} }
else {
((Throwable) logMessage).printStackTrace(new PrintWriter(stringWriter, true));
}
logMessage = stringWriter.toString();
} }
return logMessage; else {
printStackTrace(throwable, stringWriter);
}
return stringWriter.toString();
}
private void printStackTrace(Throwable throwable, Writer writer) {
throwable.printStackTrace(new PrintWriter(writer, true));
} }
} }

22
spring-integration-core/src/test/java/org/springframework/integration/handler/LoggingHandlerTests.java
View File

@@ -29,22 +29,24 @@ import org.apache.commons.logging.Log;
import org.junit.Test; import org.junit.Test;
import org.junit.runner.RunWith; import org.junit.runner.RunWith;
import org.mockito.Mockito; import org.mockito.Mockito;
import org.springframework.beans.DirectFieldAccessor; import org.springframework.beans.DirectFieldAccessor;
import org.springframework.beans.factory.BeanFactory; import org.springframework.beans.factory.BeanFactory;
import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.expression.EvaluationContext; import org.springframework.expression.EvaluationContext;
import org.springframework.expression.Expression; import org.springframework.expression.Expression;
import org.springframework.integration.handler.LoggingHandler.Level;
import org.springframework.integration.support.MessageBuilder;
import org.springframework.messaging.Message; import org.springframework.messaging.Message;
import org.springframework.messaging.MessageChannel; import org.springframework.messaging.MessageChannel;
import org.springframework.integration.handler.LoggingHandler.Level;
import org.springframework.messaging.support.GenericMessage; import org.springframework.messaging.support.GenericMessage;
import org.springframework.integration.support.MessageBuilder;
import org.springframework.test.context.ContextConfiguration; import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
/** /**
* @author Mark Fisher * @author Mark Fisher
* @author Artem Bilan * @author Artem Bilan
* @author Andriy Kryvtsun
* @since 2.0 * @since 2.0
*/ */
@ContextConfiguration @ContextConfiguration
@@ -126,6 +128,22 @@ public class LoggingHandlerTests {
verify(log, times(1)).warn(Mockito.anyString()); verify(log, times(1)).warn(Mockito.anyString());
} }
@Test
public void testUsageWithoutSpringInitialization() {
LoggingHandler loggingHandler = new LoggingHandler("ERROR");
DirectFieldAccessor accessor = new DirectFieldAccessor(loggingHandler);
Log log = (Log) accessor.getPropertyValue("messageLogger");
log = spy(log);
accessor.setPropertyValue("messageLogger", log);
String testPayload = "TEST_PAYLOAD";
Message<String> message = MessageBuilder.withPayload(testPayload).build();
loggingHandler.handleMessage(message);
verify(log).error(testPayload);
}
public static class TestBean { public static class TestBean {
private final String name; private final String name;