Skip to content

BeanUtils.setProperty may shouln't record trace log #304

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

BeanUtils.setProperty may shouln't record trace log #304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dc03e5e
the log trace prob
F0otman Oct 15, 2024
d6b0ed8
the log trace prob
F0otman Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 72 additions & 49 deletions src/main/java/org/apache/commons/beanutils2/BeanUtilsBean.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.beans.PropertyDescriptor;
import java.lang.reflect.Array;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -91,7 +92,17 @@ protected BeanUtilsBean initialValue() {
/**
* Logging for this instance
*/
private static final Log LOG = LogFactory.getLog(BeanUtilsBean.class);
private static Log LOG = LogFactory.getLog(BeanUtilsBean.class);

/**
* Reset this bean's log as another log implementation
* <strong>Only use for log test</strong>
*
* @param log log for this BeanUtilsBean
*/
protected static void setLOG(Log log) {
BeanUtilsBean.LOG = log;
}

/**
* Determines the type of a {@code DynaProperty}. Here a special treatment is needed for mapped properties.
Expand Down Expand Up @@ -336,30 +347,8 @@ public void copyProperties(final Object dest, final Object orig) throws IllegalA
public void copyProperty(final Object bean, String name, Object value) throws IllegalAccessException, InvocationTargetException {
// Trace logging (if enabled)
if (LOG.isTraceEnabled()) {
final StringBuilder sb = new StringBuilder(" copyProperty(");
sb.append(bean);
sb.append(", ");
sb.append(name);
sb.append(", ");
if (value == null) {
sb.append("<NULL>");
} else if (value instanceof String) {
sb.append((String) value);
} else if (value instanceof String[]) {
final String[] values = (String[]) value;
sb.append('[');
for (int i = 0; i < values.length; i++) {
if (i > 0) {
sb.append(',');
}
sb.append(values[i]);
}
sb.append(']');
} else {
sb.append(value.toString());
}
sb.append(')');
LOG.trace(sb.toString());
final String logStart = " copyProperty(";
LOG.trace(traceLogRecord(bean, name, value, logStart).toString());
}

// Resolve any nested expression to get the actual target bean
Expand Down Expand Up @@ -771,30 +760,8 @@ public void populate(final Object bean, final Map<String, ? extends Object> prop
public void setProperty(final Object bean, String name, final Object value) throws IllegalAccessException, InvocationTargetException {
// Trace logging (if enabled)
if (LOG.isTraceEnabled()) {
final StringBuilder sb = new StringBuilder(" setProperty(");
sb.append(bean);
sb.append(", ");
sb.append(name);
sb.append(", ");
if (value == null) {
sb.append("<NULL>");
} else if (value instanceof String) {
sb.append((String) value);
} else if (value instanceof String[]) {
final String[] values = (String[]) value;
sb.append('[');
for (int i = 0; i < values.length; i++) {
if (i > 0) {
sb.append(',');
}
sb.append(values[i]);
}
sb.append(']');
} else {
sb.append(value.toString());
}
sb.append(')');
LOG.trace(sb.toString());
final String logStart = " setProperty(";
LOG.trace(traceLogRecord(bean, name, value, logStart).toString());
}

// Resolve any nested expression to get the actual target bean
Expand Down Expand Up @@ -921,4 +888,60 @@ public void setProperty(final Object bean, String name, final Object value) thro
throw new InvocationTargetException(e, "Cannot set " + propName);
}
}

/**
* <p>Build the stringBuilder by using set/copy bean property for log, only
* run when log level is <b>trace</b>. Sequentially fill stringBuilder by
* {@code bean.toString}, property name and property value.</p>
*
* <p><strong>When the bean's toString method is override, hide the detail
* of value.</strong></p>
*
* @param bean Bean on which setting is to be performed
* @param name Property name (can be nested/indexed/mapped/combo)
* @param value Value to be set
* @param logStart the start of log
* @return the value should log as trace
*/
protected static StringBuilder traceLogRecord(Object bean, String name, Object value, String logStart) {
StringBuilder sb = new StringBuilder(logStart);
sb.append(bean);
sb.append(", ");
sb.append(name);
sb.append(", ");
// If bean's toString method has override, do not directly log value
boolean hasToStringOverride = false;
try {
Method toString = bean.getClass().getDeclaredMethod("toString");
if (toString.getDeclaringClass()!= Object.class){
hasToStringOverride = true;
}
} catch (NoSuchMethodException e) {
// If NoSuchMethodException, the bean doesn't override toString, continue
}
if (hasToStringOverride){
// Only mention the value has been set, do not expose detail
return sb.append("has been set)");
}

// normal scene, haven't override toString
if (value == null) {
return sb.append("<NULL>)");
}
if (value instanceof String){
return sb.append((String) value).append(')');
}
if (value instanceof String[]){
final String[] values = (String[]) value;
sb.append('[');
for (int i = 0; i < values.length; i++) {
if (i > 0) {
sb.append(',');
}
sb.append(values[i]);
}
return sb.append("])");
}
return sb.append(value.toString()).append(')');
}
}
26 changes: 2 additions & 24 deletions src/main/java/org/apache/commons/beanutils2/locale/LocaleBeanUtilsBean.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,30 +596,8 @@ public void setProperty(final Object bean, final String name, final Object value
public void setProperty(final Object bean, String name, final Object value, final String pattern) throws IllegalAccessException, InvocationTargetException {
// Trace logging (if enabled)
if (LOG.isTraceEnabled()) {
final StringBuilder sb = new StringBuilder(" setProperty(");
sb.append(bean);
sb.append(", ");
sb.append(name);
sb.append(", ");
if (value == null) {
sb.append("<NULL>");
} else if (value instanceof String) {
sb.append((String) value);
} else if (value instanceof String[]) {
final String[] values = (String[]) value;
sb.append('[');
for (int i = 0; i < values.length; i++) {
if (i > 0) {
sb.append(',');
}
sb.append(values[i]);
}
sb.append(']');
} else {
sb.append(value.toString());
}
sb.append(')');
LOG.trace(sb.toString());
final String logStart = " setProperty(";
LOG.trace(traceLogRecord(bean, name, value, logStart).toString());
}

// Resolve any nested expression to get the actual target bean
Expand Down
166 changes: 162 additions & 4 deletions src/test/java/org/apache/commons/beanutils2/BeanUtilsBeanTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,12 @@
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.util.Calendar;
import java.util.HashMap;
import java.util.Locale;
import java.util.Map;
import java.util.*;

import org.apache.commons.beanutils2.converters.ArrayConverter;
import org.apache.commons.beanutils2.converters.DateConverter;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -1259,10 +1258,169 @@ public void testSetPropertyWriteOnly() throws Exception {

}

/**
* Test copying a new value to a write-only property, make sure the trace level log output correct.
*/
@Test
public void testCopyPropertyLogTrace() throws Exception {
LogSpy logSpy = new LogSpy(LogFactory.getLog(BeanUtilsBean.class));
final BeanUtilsBean beanUtils = new BeanUtilsBean(new ConvertUtilsBean(), new PropertyUtilsBean());
BeanUtilsBean.setLOG(logSpy);
beanUtils.copyProperty(bean, "writeOnlyProperty", "New value");
assertEquals(" copyProperty(" + bean.toString() + ", writeOnlyProperty, New value)", logSpy.getTraceMessages().get(0));
}

/**
* Test copying a null value to a write-only property, make sure the trace level log output correct.
*/
@Test
public void testCopyPropertyLogTraceWithNull() throws Exception {
LogSpy logSpy = new LogSpy(LogFactory.getLog(BeanUtilsBean.class));
final BeanUtilsBean beanUtils = new BeanUtilsBean(new ConvertUtilsBean(), new PropertyUtilsBean());
BeanUtilsBean.setLOG(logSpy);
beanUtils.copyProperty(bean, "writeOnlyProperty", null);
assertEquals(" copyProperty(" + bean.toString() + ", writeOnlyProperty, <NULL>)", logSpy.getTraceMessages().get(0));
}

/**
* Test copying a new value to a write-only property, which the bean to be writen has {@code @Override toString()} method,
* make sure the trace level log output correct.
*/
@Test
public void testCopyPropertyLogTraceWhenBeanOverRidesToString() throws Exception {
final TestBean2 bean = new TestBean2();
LogSpy logSpy = new LogSpy(LogFactory.getLog(BeanUtilsBean.class));
final BeanUtilsBean beanUtils = new BeanUtilsBean(new ConvertUtilsBean(), new PropertyUtilsBean());
BeanUtilsBean.setLOG(logSpy);
beanUtils.copyProperty(bean, "writeOnlyProperty", "New value");
assertEquals(" copyProperty(" + bean.toString() + ", writeOnlyProperty, has been set)", logSpy.getTraceMessages().get(0));
}

/**
* Throw an exception with the specified message.
*/
private void throwException(final String msg) throws Throwable {
throw new Exception(msg);
}

/**
* Build a minimum implement of Log, contains log output
* <strong>Only use for test</strong>
*/
private class LogSpy implements Log {
private final Log originalLog;
private final List<Object> traceMessages = new ArrayList<>();

public List<Object> getTraceMessages() {
return traceMessages;
}

public LogSpy(Log originalLog) {
this.originalLog = originalLog;
}

@Override
public void debug(Object message) {
originalLog.debug(message);
}

// Implement or delegate other methods in the Log interface
@Override
public void debug(Object message, Throwable t) {
originalLog.debug(message, t);
}

@Override
public void error(Object message) {
originalLog.error(message);
}

@Override
public void error(Object message, Throwable t) {
originalLog.error(message, t);
}

@Override
public void fatal(Object message) {
originalLog.fatal(message);
}

@Override
public void fatal(Object message, Throwable t) {
originalLog.fatal(message, t);
}

@Override
public void info(Object message) {
originalLog.info(message);
}

@Override
public void info(Object message, Throwable t) {
originalLog.info(message, t);
}

@Override
public boolean isDebugEnabled() {
return originalLog.isDebugEnabled();
}

@Override
public boolean isErrorEnabled() {
return originalLog.isErrorEnabled();
}

@Override
public boolean isFatalEnabled() {
return originalLog.isFatalEnabled();
}

@Override
public boolean isInfoEnabled() {
return originalLog.isInfoEnabled();
}

@Override
public boolean isTraceEnabled() {
return true;
// return originalLog.isTraceEnabled();
}

@Override
public boolean isWarnEnabled() {
return originalLog.isWarnEnabled();
}

@Override
public void trace(Object message) {
traceMessages.add(message);
originalLog.trace(message);
}

@Override
public void trace(Object message, Throwable t) {
originalLog.trace(message, t);
}

@Override
public void warn(Object message) {
originalLog.warn(message);
}

@Override
public void warn(Object message, Throwable t) {
originalLog.warn(message, t);
}
}

/**
* A testBean has {@code @Override toString()},
* <strong>only use for test</strong>
*/
class TestBean2 extends TestBean {
@Override
public String toString() {
return super.toString();
}
}
}