From 8d80e5f8831212592ba6641030a580f4798869ee Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Thu, 20 Oct 2022 15:31:07 +0100 Subject: [PATCH] Remove DataBinder from GraphQlArgumentBinder DataBinder is not a great fit to bind GraphQL arguments which are essentially a (structured) map of maps and collections, rather than a (flat) map of property values with JavaBean paths. We already navigate recursively the GraphQL arguments map, which corresponds to the target Object structure, creating values, so all we have to do is check for matching bean properties at each level and set them. See gh-516 --- .../graphql/data/GraphQlArgumentBinder.java | 281 +++++++++--------- .../AnnotatedControllerConfigurer.java | 14 +- .../data/GraphQlArgumentBinderTests.java | 36 +-- 3 files changed, 159 insertions(+), 172 deletions(-) diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/GraphQlArgumentBinder.java b/spring-graphql/src/main/java/org/springframework/graphql/data/GraphQlArgumentBinder.java index 654e1618..9a8901a2 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/GraphQlArgumentBinder.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/GraphQlArgumentBinder.java @@ -17,41 +17,51 @@ package org.springframework.graphql.data; import java.lang.reflect.Constructor; -import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.List; import java.util.Map; import java.util.Optional; -import java.util.Stack; import java.util.function.Consumer; import graphql.schema.DataFetchingEnvironment; import org.springframework.beans.BeanInstantiationException; import org.springframework.beans.BeanUtils; -import org.springframework.beans.MutablePropertyValues; +import org.springframework.beans.BeanWrapper; +import org.springframework.beans.NotWritablePropertyException; +import org.springframework.beans.PropertyAccessorFactory; import org.springframework.beans.SimpleTypeConverter; +import org.springframework.beans.TypeConverter; import org.springframework.beans.TypeMismatchException; import org.springframework.core.CollectionFactory; +import org.springframework.core.Conventions; +import org.springframework.core.MethodParameter; import org.springframework.core.ResolvableType; import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.TypeDescriptor; import org.springframework.lang.Nullable; +import org.springframework.util.ClassUtils; +import org.springframework.validation.AbstractBindingResult; import org.springframework.validation.BindException; -import org.springframework.validation.BindingErrorProcessor; -import org.springframework.validation.BindingResult; import org.springframework.validation.DataBinder; -import org.springframework.validation.DefaultBindingErrorProcessor; import org.springframework.validation.FieldError; /** * Bind a GraphQL argument, or the full arguments map, onto a target object. * - *

Binding is performed by mapping argument values to a primary data - * constructor of the target object, or by using a default constructor and - * mapping argument values to its properties. This is applied recursively. + *

Complex objects (non-scalar) are initialized either through the primary + * data constructor where arguments are matched to constructor parameters, or + * through the default constructor where arguments are matched to setter + * property methods. In case objects are related to other objects, binding is + * applied recursively to create nested objects. + * + *

Scalar values are converted to the expected target type through a + * {@link ConversionService}, if provided. + * + *

In case of any errors, when creating objects or converting scalar values, + * a {@link BindException} is raised that contains all errors recorded along + * with the path at which the errors occurred. * * @author Brian Clozel * @author Rossen Stoyanchev @@ -59,19 +69,9 @@ import org.springframework.validation.FieldError; */ public class GraphQlArgumentBinder { - /** - * Use a larger {@link DataBinder#DEFAULT_AUTO_GROW_COLLECTION_LIMIT} for GraphQL use cases - */ - private static final int DEFAULT_AUTO_GROW_COLLECTION_LIMIT = 1024; - - @Nullable private final SimpleTypeConverter typeConverter; - private final BindingErrorProcessor bindingErrorProcessor = new DefaultBindingErrorProcessor(); - - private final List> dataBinderInitializers = new ArrayList<>(); - public GraphQlArgumentBinder() { this(null); @@ -89,22 +89,16 @@ public class GraphQlArgumentBinder { } - private SimpleTypeConverter getTypeConverter() { - return (this.typeConverter != null ? this.typeConverter : new SimpleTypeConverter()); - } - - @Nullable - private ConversionService getConversionService() { - return (this.typeConverter != null ? this.typeConverter.getConversionService() : null); - } - /** - * Add a {@link DataBinder} consumer that initializes the binder instance before the binding process. - * @param dataBinderInitializer the data binder initializer + * Add a {@link DataBinder} consumer that initializes the binder instance + * before the binding process. + * @param consumer the data binder initializer * @since 1.0.1 + * @deprecated this property is deprecated, ignored, and should not be + * necessary as a {@link DataBinder} is no longer used to bind arguments */ - public void addDataBinderInitializer(Consumer dataBinderInitializer) { - this.dataBinderInitializers.add(dataBinderInitializer); + @Deprecated(since = "1.1.0", forRemoval = true) + public void addDataBinderInitializer(Consumer consumer) { } @@ -130,35 +124,37 @@ public class GraphQlArgumentBinder { Object rawValue = (name != null ? environment.getArgument(name) : environment.getArguments()); - DataBinder binder = new DataBinder(null, name != null ? ("Arguments[" + name + "]") : "Arguments"); - initDataBinder(binder); - BindingResult bindingResult = binder.getBindingResult(); + ArgumentsBindingResult bindingResult = new ArgumentsBindingResult(targetType); - Stack segments = new Stack<>(); - if (name != null) { - segments.push(name); - } - - Object targetValue = bindRawValue( - rawValue, targetType, targetType.resolve(Object.class), bindingResult, segments); + Object value = bindRawValue("$", rawValue, targetType, targetType.resolve(Object.class), bindingResult); if (bindingResult.hasErrors()) { throw new BindException(bindingResult); } - return targetValue; - } - - private void initDataBinder(DataBinder binder) { - binder.setAutoGrowCollectionLimit(DEFAULT_AUTO_GROW_COLLECTION_LIMIT); - this.dataBinderInitializers.forEach(initializer -> initializer.accept(binder)); + return value; } + /** + * Bind the raw GraphQL argument value to an Object of the specified type. + * @param name the name of a constructor parameter or a bean property of the + * target Object that is to be initialized from the given raw value; + * {@code "$"} if binding the top level Object; possibly indexed if binding + * to a Collection element or to a Map value. + * @param rawValue the raw argument value (Collection, Map, or scalar) + * @param targetType the type of Object to create + * @param targetClass the resolved class from the targetType + * @param bindingResult for keeping track of the nested path and errors + * @return the target Object instance, possibly {@code null} if the source + * value is {@code null} or if binding failed in which case the result will + * contain errors; nevertheless we keep going to record as many errors as + * we can accumulate + */ @SuppressWarnings({"ConstantConditions", "unchecked"}) @Nullable private Object bindRawValue( - Object rawValue, ResolvableType targetType, Class targetClass, - BindingResult bindingResult, Stack segments) { + String name, @Nullable Object rawValue, ResolvableType targetType, Class targetClass, + ArgumentsBindingResult bindingResult) { boolean isOptional = (targetClass == Optional.class); @@ -172,27 +168,27 @@ public class GraphQlArgumentBinder { value = rawValue; } else if (rawValue instanceof Collection) { - value = bindCollection((Collection) rawValue, targetType, targetClass, bindingResult, segments); + value = bindCollection(name, (Collection) rawValue, targetType, targetClass, bindingResult); } else if (rawValue instanceof Map) { - value = bindMap((Map) rawValue, targetType, targetClass, bindingResult, segments); + value = bindMap(name, (Map) rawValue, targetType, targetClass, bindingResult); } else { value = (targetClass.isAssignableFrom(rawValue.getClass()) ? - rawValue : convertValue(rawValue, targetClass, bindingResult, segments)); + rawValue : convertValue(name, rawValue, targetClass, bindingResult)); } return (isOptional ? Optional.ofNullable(value) : value); } private Collection bindCollection( - Collection rawCollection, ResolvableType collectionType, Class collectionClass, - BindingResult bindingResult, Stack segments) { + String name, Collection rawCollection, ResolvableType collectionType, Class collectionClass, + ArgumentsBindingResult bindingResult) { ResolvableType elementType = collectionType.asCollection().getGeneric(0); Class elementClass = collectionType.asCollection().getGeneric(0).resolve(); if (elementClass == null) { - bindingResult.rejectValue(toArgumentPath(segments), "unknownTargetType", "Unknown target type"); + bindingResult.rejectValue(null, "unknownType", "Unknown Collection element type"); return Collections.emptyList(); // Keep going, report as many errors as we can } @@ -201,63 +197,43 @@ public class GraphQlArgumentBinder { int index = 0; for (Object rawValue : rawCollection) { - segments.push("[" + index++ + "]"); - collection.add(bindRawValue(rawValue, elementType, elementClass, bindingResult, segments)); - segments.pop(); + String indexedName = name + "[" + index++ + "]"; + collection.add(bindRawValue(indexedName, rawValue, elementType, elementClass, bindingResult)); } return collection; } - private static String toArgumentPath(Stack path) { - StringBuilder sb = new StringBuilder(); - path.forEach(sb::append); - return sb.toString(); - } - @Nullable private Object bindMap( - Map rawMap, ResolvableType targetType, Class targetClass, - BindingResult bindingResult, Stack segments) { + String name, Map rawMap, ResolvableType targetType, Class targetClass, + ArgumentsBindingResult bindingResult) { if (Map.class.isAssignableFrom(targetClass)) { - return bindMapToMap(rawMap, targetType, bindingResult, segments, targetClass); + return bindMapToMap(name, rawMap, targetType, targetClass, bindingResult); } + bindingResult.pushNestedPath(name); + Constructor constructor = BeanUtils.getResolvableConstructor(targetClass); - if (constructor.getParameterCount() > 0) { - return bindMapToObjectViaConstructor(rawMap, constructor, bindingResult, segments); - } - Object target = BeanUtils.instantiateClass(constructor); - DataBinder dataBinder = new DataBinder(target); - initDataBinder(dataBinder); - dataBinder.getBindingResult().setNestedPath(toArgumentPath(segments)); - dataBinder.setConversionService(getConversionService()); - dataBinder.bind(createPropertyValues(rawMap)); + Object value = constructor.getParameterCount() > 0 ? + bindMapToObjectViaConstructor(rawMap, constructor, bindingResult) : + bindMapToObjectViaSetters(rawMap, constructor, bindingResult); - if (dataBinder.getBindingResult().hasErrors()) { - String nestedPath = dataBinder.getBindingResult().getNestedPath(); - for (FieldError error : dataBinder.getBindingResult().getFieldErrors()) { - bindingResult.addError( - new FieldError(bindingResult.getObjectName(), nestedPath + error.getField(), - error.getRejectedValue(), error.isBindingFailure(), error.getCodes(), - error.getArguments(), error.getDefaultMessage())); - } - return null; - } + bindingResult.popNestedPath(); - return target; + return value; } private Map bindMapToMap( - Map rawMap, ResolvableType targetType, BindingResult bindingResult, - Stack segments, Class targetClass) { + String name, Map rawMap, ResolvableType targetType, Class targetClass, + ArgumentsBindingResult bindingResult) { ResolvableType valueType = targetType.asMap().getGeneric(1); Class valueClass = valueType.resolve(); if (valueClass == null) { - bindingResult.rejectValue(toArgumentPath(segments), "unknownTargetType", "Unknown target type"); + bindingResult.rejectValue(null, "unknownType", "Unknown Map value type"); return Collections.emptyMap(); // Keep going, report as many errors as we can } @@ -265,9 +241,8 @@ public class GraphQlArgumentBinder { for (Map.Entry entry : rawMap.entrySet()) { String key = entry.getKey(); - segments.push("[" + key + "]"); - map.put(key, bindRawValue(entry.getValue(), valueType, valueClass, bindingResult, segments)); - segments.pop(); + String indexedName = name + "[" + key + "]"; + map.put(key, bindRawValue(indexedName, entry.getValue(), valueType, valueClass, bindingResult)); } return map; @@ -275,12 +250,7 @@ public class GraphQlArgumentBinder { @Nullable private Object bindMapToObjectViaConstructor( - Map rawMap, Constructor constructor, BindingResult bindingResult, - Stack segments) { - - if (segments.size() > 0) { - segments.push("."); - } + Map rawMap, Constructor constructor, ArgumentsBindingResult bindingResult) { String[] paramNames = BeanUtils.getParameterNames(constructor); Class[] paramTypes = constructor.getParameterTypes(); @@ -288,14 +258,8 @@ public class GraphQlArgumentBinder { for (int i = 0; i < paramNames.length; i++) { String name = paramNames[i]; - segments.push(name); ResolvableType paramType = ResolvableType.forConstructorParameter(constructor, i); - args[i] = bindRawValue(rawMap.get(name), paramType, paramTypes[i], bindingResult, segments); - segments.pop(); - } - - if (segments.size() > 1) { - segments.pop(); + args[i] = bindRawValue(name, rawMap.get(name), paramType, paramTypes[i], bindingResult); } try { @@ -310,61 +274,88 @@ public class GraphQlArgumentBinder { } } - private static MutablePropertyValues createPropertyValues(Map rawMap) { - MutablePropertyValues mpvs = new MutablePropertyValues(); - Stack segments = new Stack<>(); - for (String key : rawMap.keySet()) { - addPropertyValue(mpvs, key, rawMap.get(key), segments); - } - return mpvs; - } + private Object bindMapToObjectViaSetters( + Map rawMap, Constructor constructor, ArgumentsBindingResult bindingResult) { - @SuppressWarnings("unchecked") - private static void addPropertyValue(MutablePropertyValues mpvs, String name, Object value, Stack segments) { - if (value instanceof List) { - List items = (List) value; - if (items.isEmpty()) { - segments.push(name); - mpvs.add(toArgumentPath(segments), value); - segments.pop(); + Object target = BeanUtils.instantiateClass(constructor); + BeanWrapper beanWrapper = PropertyAccessorFactory.forBeanPropertyAccess(target); + + for (Map.Entry entry : rawMap.entrySet()) { + String key = entry.getKey(); + TypeDescriptor type = beanWrapper.getPropertyTypeDescriptor(key); + if (type == null) { + // Ignore unknown property + continue; } - else { - for (int i = 0; i < items.size(); i++) { - addPropertyValue(mpvs, name + "[" + i + "]", items.get(i), segments); + Object value = bindRawValue( + key, entry.getValue(), type.getResolvableType(), type.getType(), bindingResult); + try { + if (value != null) { + beanWrapper.setPropertyValue(key, value); } } - } - else if (value instanceof Map) { - segments.push(name + "."); - Map map = (Map) value; - for (String key : map.keySet()) { - addPropertyValue(mpvs, key, map.get(key), segments); + catch (NotWritablePropertyException ex) { + // Ignore unknown property + } + catch (Exception ex) { + bindingResult.rejectValue(value, "invalidPropertyValue", "Failed to set property value"); } - segments.pop(); - } - else { - segments.push(name); - mpvs.add(toArgumentPath(segments), value); - segments.pop(); } + + return target; } @SuppressWarnings("unchecked") @Nullable private T convertValue( - @Nullable Object rawValue, Class type, BindingResult bindingResult, Stack segments) { + String name, @Nullable Object rawValue, Class type, ArgumentsBindingResult bindingResult) { Object value = null; try { - value = getTypeConverter().convertIfNecessary(rawValue, (Class) type, TypeDescriptor.valueOf(type)); + TypeConverter converter = (this.typeConverter != null ? this.typeConverter : new SimpleTypeConverter()); + value = converter.convertIfNecessary(rawValue, (Class) type, TypeDescriptor.valueOf(type)); } catch (TypeMismatchException ex) { - String name = toArgumentPath(segments); - ex.initPropertyName(name); - bindingResult.recordFieldValue(name, type, rawValue); - this.bindingErrorProcessor.processPropertyAccessException(ex, bindingResult); + bindingResult.pushNestedPath(name); + bindingResult.rejectValue(rawValue, ex.getErrorCode(), "Failed to convert argument value"); + bindingResult.popNestedPath(); } return (T) value; } + + /** + * BindingResult without a target Object, only for keeping track of errors + * and their associated, nested paths. + */ + @SuppressWarnings("serial") + private static class ArgumentsBindingResult extends AbstractBindingResult { + + ArgumentsBindingResult(ResolvableType targetType) { + super(initObjectName(targetType)); + } + + private static String initObjectName(ResolvableType targetType) { + return (targetType.getSource() instanceof MethodParameter methodParameter ? + Conventions.getVariableNameForParameter(methodParameter) : + ClassUtils.getShortNameAsProperty(targetType.resolve(Object.class))); + } + + @Override + public Object getTarget() { + return null; + } + + @Override + protected Object getActualFieldValue(String field) { + return null; + } + + public void rejectValue(@Nullable Object rawValue, String code, String defaultMessage) { + addError(new FieldError( + getObjectName(), fixedField(null), rawValue, true, resolveMessageCodes(code), + null, defaultMessage)); + } + } + } diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java index 71c2107b..e9413a7b 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java @@ -127,9 +127,6 @@ public class AnnotatedControllerConfigurer @Nullable private HandlerMethodValidationHelper validationHelper; - @Nullable - private Consumer dataBinderInitializer; - /** * Add a {@code FormatterRegistrar} to customize the {@link ConversionService} @@ -154,11 +151,13 @@ public class AnnotatedControllerConfigurer /** * Configure an initializer that configures the {@link DataBinder} before the binding process. - * @param dataBinderInitializer the data binder initializer + * @param consumer the data binder initializer * @since 1.0.1 + * @deprecated this property is deprecated, ignored, and should not be + * necessary as a {@link DataBinder} is no longer used to bind arguments */ - public void setDataBinderInitializer(@Nullable Consumer dataBinderInitializer) { - this.dataBinderInitializer = dataBinderInitializer; + @Deprecated(since = "1.1.0", forRemoval = true) + public void setDataBinderInitializer(@Nullable Consumer consumer) { } @Override @@ -189,9 +188,6 @@ public class AnnotatedControllerConfigurer } resolvers.addResolver(new ArgumentMapMethodArgumentResolver()); GraphQlArgumentBinder argumentBinder = new GraphQlArgumentBinder(this.conversionService); - if (this.dataBinderInitializer != null) { - argumentBinder.addDataBinderInitializer(this.dataBinderInitializer); - } resolvers.addResolver(new ArgumentMethodArgumentResolver(argumentBinder)); resolvers.addResolver(new ArgumentsMethodArgumentResolver(argumentBinder)); resolvers.addResolver(new ContextValueMethodArgumentResolver()); diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/GraphQlArgumentBinderTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/GraphQlArgumentBinderTests.java index 6f575324..be471924 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/GraphQlArgumentBinderTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/GraphQlArgumentBinderTests.java @@ -117,11 +117,11 @@ class GraphQlArgumentBinderTests { () -> this.binder.bind( environment("{\"key\":{\"name\":\"test\",\"age\":\"invalid\"}}"), "key", ResolvableType.forClass(SimpleBean.class))) - .extracting(ex -> ((BindException) ex).getFieldErrors()) - .satisfies(errors -> { + .satisfies(ex -> { + List errors = ((BindException) ex).getFieldErrors(); assertThat(errors).hasSize(1); - assertThat(errors.get(0).getObjectName()).isEqualTo("Arguments[key]"); - assertThat(errors.get(0).getField()).isEqualTo("key.age"); + assertThat(errors.get(0).getObjectName()).isEqualTo("simpleBean"); + assertThat(errors.get(0).getField()).isEqualTo("$.age"); assertThat(errors.get(0).getRejectedValue()).isEqualTo("invalid"); }); } @@ -242,17 +242,17 @@ class GraphQlArgumentBinderTests { "\"item\":{\"name\":\"Item name\",\"age\":\"invalid\"}}}"), "key", ResolvableType.forClass(PrimaryConstructorItemBean.class))) - .extracting(ex -> ((BindException) ex).getFieldErrors()) - .satisfies(errors -> { - assertThat(errors).hasSize(2); + .satisfies(ex -> { + List fieldErrors = ((BindException) ex).getFieldErrors(); + assertThat(fieldErrors).hasSize(2); - assertThat(errors.get(0).getObjectName()).isEqualTo("Arguments[key]"); - assertThat(errors.get(0).getField()).isEqualTo("key.age"); - assertThat(errors.get(0).getRejectedValue()).isEqualTo("invalid"); + assertThat(fieldErrors.get(0).getObjectName()).isEqualTo("primaryConstructorItemBean"); + assertThat(fieldErrors.get(0).getField()).isEqualTo("$.age"); + assertThat(fieldErrors.get(0).getRejectedValue()).isEqualTo("invalid"); - assertThat(errors.get(0).getObjectName()).isEqualTo("Arguments[key]"); - assertThat(errors.get(1).getField()).isEqualTo("key.item.age"); - assertThat(errors.get(1).getRejectedValue()).isEqualTo("invalid"); + assertThat(fieldErrors.get(1).getObjectName()).isEqualTo("primaryConstructorItemBean"); + assertThat(fieldErrors.get(1).getField()).isEqualTo("$.item.age"); + assertThat(fieldErrors.get(1).getRejectedValue()).isEqualTo("invalid"); }); } @@ -267,15 +267,15 @@ class GraphQlArgumentBinderTests { "{\"name\":\"second\", \"age\":\"invalid\"}]}}"), "key", ResolvableType.forClass(PrimaryConstructorItemListBean.class))) - .extracting(ex -> ((BindException) ex).getFieldErrors()) - .satisfies(errors -> { + .satisfies(ex -> { + List errors = ((BindException) ex).getFieldErrors(); assertThat(errors).hasSize(2); for (int i = 0; i < errors.size(); i++) { FieldError error = errors.get(i); - assertThat(error.getObjectName()).isEqualTo("Arguments[key]"); - assertThat(error.getField()).isEqualTo("key.items[" + i + "].age"); + assertThat(error.getObjectName()).isEqualTo("primaryConstructorItemListBean"); + assertThat(error.getField()).isEqualTo("$.items[" + i + "].age"); assertThat(error.getRejectedValue()).isEqualTo("invalid"); - assertThat(error.getDefaultMessage()).startsWith("Failed to convert property value"); + assertThat(error.getDefaultMessage()).startsWith("Failed to convert argument value"); } }); }