Skip to content

Commit

Permalink
Merge pull request #987 from codeborne/dont-swallow-exceptions-in-binder
Browse files Browse the repository at this point in the history 
Dont swallow exceptions in binder
  • Loading branch information
@asolntsev
asolntsev authored Jul 2, 2016
2 parents 63cc169 + bc4a2f6 commit 3b40c51
Show file tree
Hide file tree
Showing 9 changed files with 198 additions and 67 deletions.
4 changes: 2 additions & 2 deletions framework/src/play/data/binding/As.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@
Class<? extends TypeBinder<?>> binder() default DEFAULT.class;
Class<? extends TypeUnbinder<?>> unbinder() default DEFAULT.class;

public static final class DEFAULT implements TypeBinder<Object>, TypeUnbinder<Object> {
final class DEFAULT implements TypeBinder<Object>, TypeUnbinder<Object> {
@Override
public Object bind(String name, Annotation[] annotations, String value, Class actualClass, Type genericType) throws Exception {
public Object bind(String name, Annotation[] annotations, String value, Class actualClass, Type genericType) {
throw new UnsupportedOperationException("Not supported.");
}

Expand Down
115 changes: 77 additions & 38 deletions framework/src/play/data/binding/Binder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;
import java.math.BigDecimal;
import java.text.ParseException;
import java.util.*;


Expand Down Expand Up @@ -124,9 +125,9 @@ public static Object bind(RootParamNode parentParamNode, String name, Class<?> c
Method method = methodAndParamInfo.method;
Method defaultMethod = method.getDeclaringClass().getDeclaredMethod(method.getName() + "$default$" + methodAndParamInfo.parameterIndex);
return defaultMethod.invoke(methodAndParamInfo.objectInstance);
} catch (NoSuchMethodException e) {
//
} catch (NoSuchMethodException ignore) {
} catch (Exception e) {
logBindingNormalFailure(paramNode, e);
throw new UnexpectedException(e);
}
}
Expand Down Expand Up @@ -181,7 +182,7 @@ protected static Object internalBind(ParamNode paramNode, Class<?> clazz, Type t
}

if (Map.class.isAssignableFrom(clazz)) {
return bindMap(clazz, type, paramNode, bindingAnnotations);
return bindMap(type, paramNode, bindingAnnotations);
}

if (Collection.class.isAssignableFrom(clazz)) {
Expand All @@ -199,18 +200,38 @@ protected static Object internalBind(ParamNode paramNode, Class<?> clazz, Type t
if (clazz.isArray()) {
return bindArray(clazz, paramNode, bindingAnnotations);
}
if (!paramNode.getAllChildren().isEmpty()) {
return internalBindBean(clazz, paramNode, bindingAnnotations);
}

if (!paramNode.getAllChildren().isEmpty()) {
return internalBindBean(clazz, paramNode, bindingAnnotations);
}

return null; // give up
} catch (NumberFormatException e) {
logBindingNormalFailure(paramNode, e);
addValidationError(paramNode);
} catch (ParseException e) {
logBindingNormalFailure(paramNode, e);
addValidationError(paramNode);
} catch (Exception e) {
Validation.addError(paramNode.getOriginalKey(), "validation.invalid");
// TODO This is bad catch. I would like to remove it in next version.
logBindingUnexpectedFailure(paramNode, e);
addValidationError(paramNode);
}
return MISSING;
}

private static void addValidationError(ParamNode paramNode) {
Validation.addError(paramNode.getOriginalKey(), "validation.invalid");
}

private static void logBindingUnexpectedFailure(ParamNode paramNode, Exception e) {
Logger.error(e, "Failed to bind %s=%s", paramNode.getOriginalKey(), Arrays.toString(paramNode.getValues()));
}

private static void logBindingNormalFailure(ParamNode paramNode, Exception e) {
Logger.debug("Failed to bind %s=%s: %s", paramNode.getOriginalKey(), Arrays.toString(paramNode.getValues()), e);
}

private static Object bindArray(Class<?> clazz, ParamNode paramNode, BindingAnnotations bindingAnnotations) {

Class<?> componentType = clazz.getComponentType();
Expand Down Expand Up @@ -238,7 +259,7 @@ private static Object bindArray(Class<?> clazz, ParamNode paramNode, BindingAnno
try {
Array.set(array, i - invalidItemsCount, directBind(paramNode.getOriginalKey(), bindingAnnotations.annotations, thisValue, componentType, componentType));
} catch (Exception e) {
// bad item..
Logger.debug("Bad item #%s: %s", i, e);
invalidItemsCount++;
}
}
Expand All @@ -252,7 +273,7 @@ private static Object bindArray(Class<?> clazz, ParamNode paramNode, BindingAnno
try {
Array.set(array, i - invalidItemsCount, childValue);
} catch (Exception e) {
// bad item..
Logger.debug("Bad item #%s: %s", i, e);
invalidItemsCount++;
}
}
Expand All @@ -273,12 +294,24 @@ private static Object bindArray(Class<?> clazz, ParamNode paramNode, BindingAnno
return array;
}

private static Object internalBindBean(Class<?> clazz, ParamNode paramNode, BindingAnnotations bindingAnnotations) throws Exception {
Object bean = clazz.newInstance();
private static Object internalBindBean(Class<?> clazz, ParamNode paramNode, BindingAnnotations bindingAnnotations) {
Object bean = createNewInstance(clazz);
internalBindBean(paramNode, bean, bindingAnnotations);
return bean;
}

private static <T> T createNewInstance(Class<T> clazz) {
try {
return clazz.newInstance();
} catch (InstantiationException e) {
Logger.warn("Failed to create instance of %s: %s", clazz.getName(), e);
throw new UnexpectedException(e);
} catch (IllegalAccessException e) {
Logger.warn("Failed to create instance of %s: %s", clazz.getName(), e);
throw new UnexpectedException(e);
}
}

/**
* Invokes the plugins before using the internal bindBean.
*/
Expand All @@ -294,20 +327,21 @@ public static void bindBean(RootParamNode rootParamNode, String name, Object bea

try {
internalBindBean(paramNode, bean, new BindingAnnotations());
} catch (Exception e) {
Validation.addError(paramNode.getOriginalKey(), "validation.invalid");
} catch (NumberFormatException e) {
logBindingNormalFailure(paramNode, e);
addValidationError(paramNode);
}

}

/**
* Does NOT invoke plugins
*/
public static void bindBean(ParamNode paramNode, Object bean, Annotation[] annotations) throws Exception {
public static void bindBean(ParamNode paramNode, Object bean, Annotation[] annotations) {
internalBindBean(paramNode, bean, new BindingAnnotations(annotations));
}

private static void internalBindBean(ParamNode paramNode, Object bean, BindingAnnotations bindingAnnotations) throws Exception {
private static void internalBindBean(ParamNode paramNode, Object bean, BindingAnnotations bindingAnnotations) {

BeanWrapper bw = getBeanWrapper(bean.getClass());
for (BeanWrapper.Property prop : bw.getWrappers()) {
Expand Down Expand Up @@ -336,7 +370,7 @@ private static void internalBindBean(ParamNode paramNode, Object bean, BindingAn
}

@SuppressWarnings("unchecked")
private static Object bindEnum(Class<?> clazz, ParamNode paramNode) throws Exception {
private static Object bindEnum(Class<?> clazz, ParamNode paramNode) {
if (paramNode.getValues() == null) {
return MISSING;
}
Expand All @@ -349,7 +383,7 @@ private static Object bindEnum(Class<?> clazz, ParamNode paramNode) throws Excep
return Enum.valueOf((Class<? extends Enum>) clazz, value);
}

private static Object bindMap(Class<?> clazz, Type type, ParamNode paramNode, BindingAnnotations bindingAnnotations) throws Exception {
private static Object bindMap(Type type, ParamNode paramNode, BindingAnnotations bindingAnnotations) {
Class keyClass = String.class;
Class valueClass = String.class;
if (type instanceof ParameterizedType) {
Expand All @@ -367,16 +401,23 @@ private static Object bindMap(Class<?> clazz, Type type, ParamNode paramNode, Bi
valueObject = null;
}
r.put(keyObject, valueObject);
} catch (Exception e) {
} catch (ParseException e) {
// Just ignore the exception and continue on the next item
logBindingNormalFailure(paramNode, e);
} catch (NumberFormatException e) {
// Just ignore the exception and continue on the next item
logBindingNormalFailure(paramNode, e);
} catch (Exception e) {
// TODO This is bad catch. I would like to remove it in next version.
logBindingUnexpectedFailure(paramNode, e);
}
}

return r;
}

@SuppressWarnings("unchecked")
private static Object bindCollection(Class<?> clazz, Type type, ParamNode paramNode, BindingAnnotations bindingAnnotations) throws Exception {
private static Object bindCollection(Class<?> clazz, Type type, ParamNode paramNode, BindingAnnotations bindingAnnotations) {
if (clazz.isInterface()) {
if (clazz.equals(List.class)) {
clazz = ArrayList.class;
Expand Down Expand Up @@ -412,9 +453,9 @@ private static Object bindCollection(Class<?> clazz, Type type, ParamNode paramN
for (Annotation annotation : bindingAnnotations.annotations) {
if (annotation.annotationType().equals(As.class)) {
As as = ((As) annotation);
final String separator = as.value()[0];
if (separator != null && !separator.isEmpty()){
values = values[0].split(separator);
String separator = as.value()[0];
if (separator != null && !separator.isEmpty()) {
values = values[0].split(separator);
}
}
}
Expand All @@ -424,7 +465,7 @@ private static Object bindCollection(Class<?> clazz, Type type, ParamNode paramN
if (clazz.equals(EnumSet.class)) {
l = EnumSet.noneOf(componentClass);
} else {
l = (Collection) clazz.newInstance();
l = (Collection) createNewInstance(clazz);
}
boolean hasMissing = false;
for (int i = 0; i < values.length; i++) {
Expand All @@ -437,6 +478,7 @@ private static Object bindCollection(Class<?> clazz, Type type, ParamNode paramN
}
} catch (Exception e) {
// Just ignore the exception and continue on the next item
logBindingNormalFailure(paramNode, e); // TODO debug or error?
}
}
if(hasMissing && l.size() == 0){
Expand All @@ -445,7 +487,7 @@ private static Object bindCollection(Class<?> clazz, Type type, ParamNode paramN
return l;
}

Collection r = (Collection) clazz.newInstance();
Collection r = (Collection) createNewInstance(clazz);

if (List.class.isAssignableFrom(clazz)) {
// Must add items at position resolved from each child's key
Expand Down Expand Up @@ -502,9 +544,9 @@ public int compare(String arg0, String arg1) {
* @param value
* @param clazz
* @return The binding object
* @throws Exception
* @throws ParseException
*/
public static Object directBind(String value, Class<?> clazz) throws Exception {
public static Object directBind(String value, Class<?> clazz) throws ParseException {
return directBind(null, value, clazz, null);
}

Expand All @@ -514,9 +556,9 @@ public static Object directBind(String value, Class<?> clazz) throws Exception {
* @param value
* @param clazz
* @return The binding object
* @throws Exception
* @throws ParseException
*/
public static Object directBind(String name, Annotation[] annotations, String value, Class<?> clazz) throws Exception {
public static Object directBind(String name, Annotation[] annotations, String value, Class<?> clazz) throws ParseException {
return directBind(name, annotations, value, clazz, null);
}

Expand All @@ -526,9 +568,9 @@ public static Object directBind(String name, Annotation[] annotations, String va
* @param clazz
* @param type
* @return The binding object
* @throws Exception
* @throws ParseException
*/
public static Object directBind(Annotation[] annotations, String value, Class<?> clazz, Type type) throws Exception {
public static Object directBind(Annotation[] annotations, String value, Class<?> clazz, Type type) throws ParseException {
return directBind(null, annotations, value, clazz, type);
}

Expand All @@ -541,9 +583,8 @@ public static Object directBind(Annotation[] annotations, String value, Class<?>
* @param clazz
* @param type
* @return The binding object
* @throws Exception
*/
public static Object directBind(String name, Annotation[] annotations, String value, Class<?> clazz, Type type) throws Exception {
public static Object directBind(String name, Annotation[] annotations, String value, Class<?> clazz, Type type) throws ParseException {
// calls the direct binding and returns null if no value could be resolved..
Object r = internalDirectBind(name, annotations, value, clazz, type);
if ( r == DIRECTBINDING_NO_RESULT) {
Expand All @@ -555,7 +596,7 @@ public static Object directBind(String name, Annotation[] annotations, String va

// If internalDirectBind was not able to bind it, it returns a special variable instance: DIRECTBIND_MISSING
// Needs this because sometimes we need to know if no value was returned..
private static Object internalDirectBind(String name, Annotation[] annotations, String value, Class<?> clazz, Type type) throws Exception {
private static Object internalDirectBind(String name, Annotation[] annotations, String value, Class<?> clazz, Type type) throws ParseException {
boolean nullOrEmpty = value == null || value.trim().length() == 0;

if (annotations != null) {
Expand All @@ -564,7 +605,7 @@ private static Object internalDirectBind(String name, Annotation[] annotations,
Class<? extends TypeBinder<?>> toInstanciate = ((As) annotation).binder();
if (!(toInstanciate.equals(As.DEFAULT.class))) {
// Instantiate the binder
TypeBinder<?> myInstance = toInstanciate.newInstance();
TypeBinder<?> myInstance = createNewInstance(toInstanciate);
return myInstance.bind(name, annotations, value, clazz, type);
}
}
Expand All @@ -576,7 +617,7 @@ private static Object internalDirectBind(String name, Annotation[] annotations,
if (c.isAnnotationPresent(Global.class)) {
Class<?> forType = (Class) ((ParameterizedType) c.getGenericInterfaces()[0]).getActualTypeArguments()[0];
if (forType.isAssignableFrom(clazz)) {
Object result = c.newInstance().bind(name, annotations, value, clazz, type);
Object result = createNewInstance(c).bind(name, annotations, value, clazz, type);
if (result != null) {
return result;
}
Expand Down Expand Up @@ -687,6 +728,4 @@ private static Object internalDirectBind(String name, Annotation[] annotations,

return DIRECTBINDING_NO_RESULT;
}


}
5 changes: 3 additions & 2 deletions framework/src/play/data/binding/TypeBinder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import java.lang.annotation.Annotation;
import java.lang.reflect.Type;
import java.text.ParseException;

/**
* Supported type for binding. This interface is used to implement custom binders.
Expand All @@ -18,8 +19,8 @@ public interface TypeBinder<T> {
* @param actualClass The class of the object you want to associate the value with
* @param genericType The generic type associated with the object you want to bound the value to
* @return the 'bound' object for example a date object if the value was '12/12/2002'
* @throws Exception
* @throws ParseException if parameter has invalid format (e.g. date)
*/
Object bind(String name, Annotation[] annotations, String value, Class actualClass, Type genericType) throws Exception;
Object bind(String name, Annotation[] annotations, String value, Class actualClass, Type genericType) throws ParseException;

}
22 changes: 9 additions & 13 deletions framework/src/play/data/binding/types/CalendarBinder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,20 @@
public class CalendarBinder implements TypeBinder<Calendar> {

@Override
public Calendar bind(String name, Annotation[] annotations, String value, Class actualClass, Type genericType) throws Exception {
public Calendar bind(String name, Annotation[] annotations, String value, Class actualClass, Type genericType) throws ParseException {
if (value == null || value.trim().length() == 0) {
return null;
}
Calendar cal = Calendar.getInstance(Lang.getLocale());
try {
Date date = AnnotationHelper.getDateAs(annotations, value);
if (date != null) {
cal.setTime(date);
} else {
SimpleDateFormat sdf = new SimpleDateFormat(I18N.getDateFormat());
sdf.setLenient(false);
cal.setTime(sdf.parse(value));
}
} catch (ParseException e) {
throw new IllegalArgumentException("Cannot convert [" + value + "] to a Calendar: " + e.toString());
}

Date date = AnnotationHelper.getDateAs(annotations, value);
if (date != null) {
cal.setTime(date);
} else {
SimpleDateFormat sdf = new SimpleDateFormat(I18N.getDateFormat());
sdf.setLenient(false);
cal.setTime(sdf.parse(value));
}
return cal;
}
}
13 changes: 4 additions & 9 deletions framework/src/play/data/binding/types/DateBinder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public class DateBinder implements TypeBinder<Date> {
public static final String ISO8601 = "'ISO8601:'yyyy-MM-dd'T'HH:mm:ssZ";

@Override
public Date bind(String name, Annotation[] annotations, String value, Class actualClass, Type genericType) throws Exception {
public Date bind(String name, Annotation[] annotations, String value, Class actualClass, Type genericType) throws ParseException {
if (value == null || value.trim().length() == 0) {
return null;
}
Expand All @@ -35,13 +35,8 @@ public Date bind(String name, Annotation[] annotations, String value, Class actu
// Ignore
}

try {
SimpleDateFormat sdf = new SimpleDateFormat(ISO8601);
sdf.setLenient(false);
return sdf.parse(value);
} catch (Exception e) {
throw new IllegalArgumentException("Cannot convert [" + value + "] to a Date: " + e.toString());
}

SimpleDateFormat sdf = new SimpleDateFormat(ISO8601);
sdf.setLenient(false);
return sdf.parse(value);
}
}
Loading

0 comments on commit 3b40c51

Please sign in to comment.