Skip to content

[WIP] Replace the usage of commons-beanutils with spring-beans #711

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

Draft
BobDu wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[WIP] Replace the usage of commons-beanutils with spring-beans #711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 11 additions & 5 deletions core/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,6 @@
<version>20110809</version>
<optional>true</optional>
</dependency>
<dependency>
<groupId>commons-beanutils</groupId>
<artifactId>commons-beanutils</artifactId>
<version>1.11.0</version>
</dependency>
<dependency>
<groupId>commons-codec</groupId>
<artifactId>commons-codec</artifactId>
Expand Down Expand Up @@ -105,6 +100,17 @@
<artifactId>json-lib</artifactId>
<version>2.4-jenkins-15</version>
</dependency>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-beans</artifactId>
<version>6.2.10</version>
<exclusions>
<exclusion>
<groupId>org.springframework</groupId>
<artifactId>spring-jcl</artifactId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>jakarta.servlet</groupId>
<artifactId>jakarta.servlet-api</artifactId>
Expand Down
8 changes: 4 additions & 4 deletions core/src/main/java/org/kohsuke/stapler/AcceptHeader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ of this software and associated documentation files (the "Software"), to deal
import java.util.List;
import java.util.Map;
import java.util.stream.Stream;
import org.apache.commons.beanutils.Converter;
import org.springframework.core.convert.converter.Converter;

/**
* Represents the {@code Accept} HTTP header and help server choose the right media type to serve.
Expand Down Expand Up @@ -234,10 +234,10 @@ public String toString() {
}

// this performs databinding for @Header parameter injection
public static class StaplerConverterImpl implements Converter {
public static class StaplerConverterImpl implements Converter<String, Object> {
@Override
public Object convert(Class type, Object value) {
return new AcceptHeader(value.toString());
public Object convert(String source) {
return new AcceptHeader(source);
}
}
}
6 changes: 3 additions & 3 deletions core/src/main/java/org/kohsuke/stapler/AnnotationHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import jakarta.servlet.ServletException;
import java.lang.annotation.Annotation;
import java.lang.reflect.InvocationTargetException;
import org.apache.commons.beanutils.Converter;
import org.springframework.core.convert.converter.Converter;

/**
* Handles stapler parameter annotations by determining what values to inject for a method call.
Expand Down Expand Up @@ -100,12 +100,12 @@ public Object parse(StaplerRequest request, T a, Class type, String parameterNam
* from String.
*/
protected final Object convert(Class targetType, String value) {
Converter converter = Stapler.lookupConverter(targetType);
Converter<String, Object> converter = Stapler.lookupConverter(targetType);
if (converter == null) {
throw new IllegalArgumentException("Unable to convert to " + targetType);
}

return converter.convert(targetType, value);
return converter.convert(value);
}

static Object handle(StaplerRequest2 request, Annotation[] annotations, String parameterName, Class targetType)
Expand Down
44 changes: 24 additions & 20 deletions core/src/main/java/org/kohsuke/stapler/RequestImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,6 @@
import net.sf.json.JSONException;
import net.sf.json.JSONNull;
import net.sf.json.JSONObject;
import org.apache.commons.beanutils.BeanUtils;
import org.apache.commons.beanutils.ConvertUtils;
import org.apache.commons.beanutils.Converter;
import org.apache.commons.beanutils.PropertyUtils;
import org.apache.commons.fileupload2.core.DiskFileItem;
import org.apache.commons.fileupload2.core.DiskFileItemFactory;
import org.apache.commons.fileupload2.core.FileItem;
Expand All @@ -84,6 +80,11 @@
import org.kohsuke.stapler.lang.Klass;
import org.kohsuke.stapler.lang.MethodRef;
import org.kohsuke.stapler.util.IllegalReflectiveAccessLogHandler;
import org.springframework.beans.BeanWrapper;
import org.springframework.beans.BeanWrapperImpl;
import org.springframework.beans.BeansException;
import org.springframework.beans.InvalidPropertyException;
import org.springframework.core.convert.converter.Converter;

/**
* {@link StaplerRequest2} implementation.
Expand Down Expand Up @@ -617,7 +618,7 @@
throw new IllegalArgumentException("Unable to convert to " + types[i]);
}

args[i] = converter.convert(types[i], param);
args[i] = converter.convert(param);
}

return invokeConstructor(c, args);
Expand Down Expand Up @@ -734,7 +735,8 @@
if (last) {
copyProperty(bean, token, value);
} else {
bean = BeanUtils.getProperty(bean, token);
BeanWrapper wrapper = new BeanWrapperImpl(bean);
bean = wrapper.getPropertyValue(token);
}
} catch (IllegalAccessException x) {
throw new IllegalAccessError(x.getMessage());
Expand All @@ -747,8 +749,8 @@
throw (Error) e;
}
throw new RuntimeException(x);
} catch (NoSuchMethodException e) {
// ignore if there's no such property
} catch (Exception e) {
// ignore if there's no such property or other spring bean exceptions
}
}
}
Expand Down Expand Up @@ -936,7 +938,7 @@
throw new IllegalArgumentException("Unable to convert to " + type);
}

return converter.convert(type, o);
return converter.convert(o.toString());
} else { // single value in a collection
Converter converter = Stapler.lookupConverter(l.itemType);
if (converter == null) {
Expand All @@ -946,7 +948,7 @@
throw new IllegalArgumentException("Unable to convert to " + l.itemType);
}
} else {
l.add(converter.convert(l.itemType, o));
l.add(converter.convert(o.toString()));
}
return l.toCollection();
}
Expand Down Expand Up @@ -1146,14 +1148,15 @@
private TypePair getPropertyType(Object bean, String name)
throws IllegalAccessException, InvocationTargetException {
try {
PropertyDescriptor propDescriptor = PropertyUtils.getPropertyDescriptor(bean, name);
BeanWrapper wrapper = new BeanWrapperImpl(bean);
PropertyDescriptor propDescriptor = wrapper.getPropertyDescriptor(name);
if (propDescriptor != null) {
Method m = propDescriptor.getWriteMethod();
if (m != null) {
return new TypePair(m.getGenericParameterTypes()[0], m.getParameterTypes()[0]);
}
}
} catch (NoSuchMethodException e) {
} catch (Exception e) {
// no such property
}

Expand All @@ -1173,9 +1176,10 @@
private static void copyProperty(Object bean, String name, Object value)
throws IllegalAccessException, InvocationTargetException {
PropertyDescriptor propDescriptor;
BeanWrapper wrapper = new BeanWrapperImpl(bean);
try {
propDescriptor = PropertyUtils.getPropertyDescriptor(bean, name);
} catch (NoSuchMethodException e) {
propDescriptor = wrapper.getPropertyDescriptor(name);
} catch (InvalidPropertyException e) {
propDescriptor = null;
}
if (propDescriptor != null && propDescriptor.getWriteMethod() == null) {
Expand All @@ -1184,11 +1188,11 @@
if (propDescriptor != null) {
Converter converter = Stapler.lookupConverter(propDescriptor.getPropertyType());
if (converter != null) {
value = converter.convert(propDescriptor.getPropertyType(), value);
value = converter.convert(value.toString());
}
try {
PropertyUtils.setSimpleProperty(bean, name, value);
} catch (NoSuchMethodException e) {
wrapper.setPropertyValue(name, value);
} catch (BeansException e) {
throw new NoSuchMethodError(e.getMessage());
}
return;
Expand All @@ -1197,12 +1201,12 @@
// try a field
try {
Field field = bean.getClass().getField(name);
Converter converter = ConvertUtils.lookup(field.getType());
Converter converter = Stapler.lookupConverter(field.getType());
if (converter != null) {
value = converter.convert(field.getType(), value);
value = converter.convert(value.toString());
}
field.set(bean, value);
} catch (NoSuchFieldException e) {
} catch (Exception e) {

Check warning on line 1209 in core/src/main/java/org/kohsuke/stapler/RequestImpl.java

View check run for this annotation

ci.jenkins.io / SpotBugs

DE_MIGHT_IGNORE 

NORMAL:
org.kohsuke.stapler.RequestImpl.copyProperty(Object, String, Object) might ignore java.lang.Exception
Raw output 
<p> This method might ignore an exception.&nbsp; In general, exceptions should be handled or reported in some way, or they should be thrown out of the method.</p>

Check warning on line 1209 in core/src/main/java/org/kohsuke/stapler/RequestImpl.java

View check run for this annotation

ci.jenkins.io / SpotBugs

REC_CATCH_EXCEPTION 

NORMAL:
Exception is caught when Exception is not thrown in org.kohsuke.stapler.RequestImpl.copyProperty(Object, String, Object)
Raw output 
<p> This method uses a try-catch block that catches Exception objects, but Exception is not thrown within the try block, and RuntimeException is not explicitly caught. It is a common bug pattern to say try { ... } catch (Exception e) { something } as a shorthand for catching a number of types of exception each of whose catch blocks is identical, but this construct also accidentally catches RuntimeException as well, masking potential bugs. </p> <p>A better approach is to either explicitly catch the specific exceptions that are thrown, or to explicitly catch RuntimeException exception, rethrow it, and then catch all non-Runtime Exceptions, as shown below:</p> <pre><code>try { ... } catch (RuntimeException e) { throw e; } catch (Exception e) { ... deal with all non-runtime exceptions ... } </code></pre>
// no such field
}
}
Expand Down
127 changes: 46 additions & 81 deletions core/src/main/java/org/kohsuke/stapler/Stapler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,28 +57,25 @@
import java.util.Collection;
import java.util.Collections;
import java.util.Date;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.Stack;
import java.util.TimeZone;
import java.util.concurrent.ConcurrentHashMap;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import net.sf.json.JSONObject;
import org.apache.commons.beanutils.ConversionException;
import org.apache.commons.beanutils.ConvertUtils;
import org.apache.commons.beanutils.ConvertUtilsBean;
import org.apache.commons.beanutils.Converter;
import org.apache.commons.beanutils.converters.DoubleConverter;
import org.apache.commons.beanutils.converters.FloatConverter;
import org.apache.commons.beanutils.converters.IntegerConverter;
import org.apache.commons.fileupload2.core.FileItem;
import org.apache.commons.io.IOUtils;
import org.kohsuke.stapler.bind.BoundObjectTable;
import org.springframework.core.convert.converter.Converter;
import org.springframework.core.convert.support.DefaultConversionService;

/**
* Maps an HTTP request to a method call / JSP invocation against a model object
Expand Down Expand Up @@ -1151,21 +1148,31 @@ static String canonicalPath(String path) {
}

/**
* This is the {@link Converter} registry that Stapler uses, primarily
* This is the converter registry that Stapler uses, primarily
* for form-to-JSON binding in {@link StaplerRequest2#bindJSON(Class, JSONObject)}
* and its family of methods.
*/
public static final ConvertUtilsBean CONVERT_UTILS = new ConvertUtilsBean();
public static final Map<Class<?>, Converter<String, Object>> CONVERT_UTILS = new ConcurrentHashMap<>();

public static Converter lookupConverter(Class type) {
Converter c = CONVERT_UTILS.lookup(type);
/**
* Spring's default conversion service for type conversion
*/
static final DefaultConversionService CONVERSION_SERVICE = new DefaultConversionService();

public static Converter<String, Object> lookupConverter(Class type) {
Converter<String, Object> c = CONVERT_UTILS.get(type);
if (c != null) {
return c;
}
// fall back to compatibility behavior
c = ConvertUtils.lookup(type);
if (c != null) {
return c;

// EnumSet conversion is handled by JSON binding, don't convert from string
if (EnumSet.class.isAssignableFrom(type)) {
return null;
}

// check if Spring's conversion service can handle it
if (CONVERSION_SERVICE.canConvert(String.class, type)) {
return source -> CONVERSION_SERVICE.convert(source, type);
}

// look for the associated converter
Expand All @@ -1175,7 +1182,7 @@ public static Converter lookupConverter(Class type) {
}
Class<?> cl = type.getClassLoader().loadClass(type.getName() + "$StaplerConverterImpl");
c = (Converter) cl.getDeclaredConstructor().newInstance();
CONVERT_UTILS.register(c, type);
CONVERT_UTILS.put(type, c);
return c;
} catch (ClassNotFoundException e) {
// fall through
Expand Down Expand Up @@ -1206,77 +1213,35 @@ public static Converter lookupConverter(Class type) {
}
}

// bean utils doesn't check the super type, so converters that apply to multiple types
// need to be handled outside its semantics
if (Enum.class.isAssignableFrom(type)) { // enum
return ENUM_CONVERTER;
}

return null;
}

static {
CONVERT_UTILS.register(
new Converter() {
@Override
public Object convert(Class type, Object value) {
if (value == null) {
return null;
}
try {
return new URL(value.toString());
} catch (MalformedURLException e) {
throw new ConversionException(e);
}
}
},
URL.class);

CONVERT_UTILS.register(
new Converter() {
@Override
public FileItem convert(Class type, Object value) {
if (value == null) {
return null;
}
try {
return Stapler.getCurrentRequest2().getFileItem2(value.toString());
} catch (ServletException | IOException e) {
throw new ConversionException(e);
}
}
},
FileItem.class);

CONVERT_UTILS.register(
new Converter() {
@Override
public org.apache.commons.fileupload.FileItem convert(Class type, Object value) {
if (value == null) {
return null;
}
try {
return org.apache.commons.fileupload.FileItem.fromFileUpload2FileItem(
Stapler.getCurrentRequest2().getFileItem2(value.toString()));
} catch (ServletException | IOException e) {
throw new ConversionException(e);
}
}
},
org.apache.commons.fileupload.FileItem.class);
CONVERT_UTILS.put(URL.class, source -> {
try {
return new URL(source);
} catch (MalformedURLException e) {
throw new RuntimeException(e);
}
});

// mapping for boxed types should map null to null, instead of null to zero.
CONVERT_UTILS.register(new IntegerConverter(null), Integer.class);
CONVERT_UTILS.register(new FloatConverter(null), Float.class);
CONVERT_UTILS.register(new DoubleConverter(null), Double.class);
}
CONVERT_UTILS.put(FileItem.class, source -> {
try {
return Stapler.getCurrentRequest2().getFileItem2(source);
} catch (ServletException | IOException e) {
throw new RuntimeException(e);
}
});

private static final Converter ENUM_CONVERTER = new Converter() {
@Override
public Object convert(Class type, Object value) {
return Enum.valueOf(type, value.toString());
}
};
CONVERT_UTILS.put(org.apache.commons.fileupload.FileItem.class, source -> {
try {
return org.apache.commons.fileupload.FileItem.fromFileUpload2FileItem(
Stapler.getCurrentRequest2().getFileItem2(source));
} catch (ServletException | IOException e) {
throw new RuntimeException(e);
}
});
}

/**
* Escapes HTML/XML unsafe characters for the PCDATA section.
Expand Down
Loading
Loading