From 9be5a95948bfc6d974f5f39dd945a2d335bfe9b4 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Tue, 18 Oct 2022 18:02:38 +0100 Subject: [PATCH 1/4] Support binding to Map as target value Before this commit, we supported binding to a higher level object, or a scalar, or a List of either, but not to a Map of either, which requires similar support as what we have for a List, i.e. creating the target Map and populating it in a recursive manner. This commit adds that support. Closes gh-449 --- .../graphql/data/GraphQlArgumentBinder.java | 44 +++++++++++++++--- .../data/GraphQlArgumentBinderTests.java | 45 +++++++++++++++++++ 2 files changed, 82 insertions(+), 7 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 b99aef13..ca306179 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 @@ -71,7 +71,7 @@ public class GraphQlArgumentBinder { private final BindingErrorProcessor bindingErrorProcessor = new DefaultBindingErrorProcessor(); - private List> dataBinderInitializers = new ArrayList<>(); + private final List> dataBinderInitializers = new ArrayList<>(); public GraphQlArgumentBinder() { @@ -160,7 +160,7 @@ public class GraphQlArgumentBinder { // From Map if (rawValue instanceof Map) { - Object target = createValue((Map) rawValue, targetClass, bindingResult, segments); + Object target = createValue((Map) rawValue, targetType, bindingResult, segments); return wrapAsOptionalIfNecessary(target, targetType); } @@ -204,6 +204,7 @@ public class GraphQlArgumentBinder { return Collections.emptyList(); } + ResolvableType elementType = collectionType.asCollection().getGeneric(0); Class elementClass = collectionType.asCollection().getGeneric(0).resolve(); if (elementClass == null) { bindingResult.rejectValue(toArgumentPath(segments), "unknownElementType", "Unknown element type"); @@ -218,7 +219,7 @@ public class GraphQlArgumentBinder { collection.add((T) rawValue); } else if (rawValue instanceof Map) { - collection.add((T) createValueOrNull((Map) rawValue, elementClass, bindingResult, segments)); + collection.add((T) createValueOrNull((Map) rawValue, elementType, bindingResult, segments)); } else { collection.add((T) convertValue(rawValue, elementClass, bindingResult, segments)); @@ -230,7 +231,7 @@ public class GraphQlArgumentBinder { @Nullable private Object createValueOrNull( - Map rawMap, Class targetType, BindingResult result, Stack segments) { + Map rawMap, ResolvableType targetType, BindingResult result, Stack segments) { try { return createValue(rawMap, targetType, result, segments); @@ -242,11 +243,40 @@ public class GraphQlArgumentBinder { @SuppressWarnings("unchecked") private Object createValue( - Map rawMap, Class targetType, BindingResult bindingResult, + Map rawMap, ResolvableType targetType, BindingResult bindingResult, Stack segments) throws BindException { + Class targetClass = targetType.resolve(); + Assert.notNull(targetClass, "Unknown target class"); + + if (Map.class.isAssignableFrom(targetClass)) { + ResolvableType valueType = targetType.asMap().getGeneric(1); + Class valueClass = valueType.resolve(); + if (valueClass == null) { + bindingResult.rejectValue(toArgumentPath(segments), "unknownMapValueType", "Unknown Map value type"); + return Collections.emptyMap(); + } + Map map = CollectionFactory.createMap(targetClass, rawMap.size()); + for (Map.Entry entry : rawMap.entrySet()) { + Object rawValue = entry.getValue(); + segments.push("[" + entry.getKey() + "]"); + if (rawValue == null || valueType.isAssignableFrom(rawValue.getClass())) { + map.put(entry.getKey(), entry.getValue()); + } + else if (rawValue instanceof Map) { + map.put(entry.getKey(), createValueOrNull( + (Map) rawValue, valueType, bindingResult, segments)); + } + else { + map.put(entry.getKey(), convertValue(rawValue, valueClass, bindingResult, segments)); + } + segments.pop(); + } + return map; + } + Object target; - Constructor ctor = BeanUtils.getResolvableConstructor(targetType); + Constructor ctor = BeanUtils.getResolvableConstructor(targetClass); // Default constructor + data binding via properties @@ -293,7 +323,7 @@ public class GraphQlArgumentBinder { } else if (rawValue instanceof Map) { boolean isOptional = (paramTypes[i] == Optional.class); - Class type = (isOptional ? methodParam.nestedIfOptional().getNestedParameterType() : paramTypes[i]); + ResolvableType type = ResolvableType.forMethodParameter(methodParam.nestedIfOptional()); Object value = createValueOrNull((Map) rawValue, type, bindingResult, segments); args[i] = (isOptional ? Optional.ofNullable(value) : value); } 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 60bd142c..dead81ad 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 @@ -280,6 +280,37 @@ class GraphQlArgumentBinderTests { }); } + @Test + void primaryConstructorWithMapArgument() throws Exception { + + Object result = this.binder.bind( + environment( + "{\"key\":{" + + "\"map\":{" + + "\"item1\":{" + + "\"name\":\"Jason\"," + + "\"age\":\"21\"" + + "}," + + "\"item2\":{" + + "\"name\":\"James\"," + + "\"age\":\"22\"" + + "}" + + "}}}"), + "key", + ResolvableType.forClass(PrimaryConstructorItemMapBean.class)); + + assertThat(result).isNotNull().isInstanceOf(PrimaryConstructorItemMapBean.class); + Map map = ((PrimaryConstructorItemMapBean) result).getMap(); + + Item item1 = map.get("item1"); + assertThat(item1.getName()).isEqualTo("Jason"); + assertThat(item1.getAge()).isEqualTo(21); + + Item item2 = map.get("item2"); + assertThat(item2.getName()).isEqualTo("James"); + assertThat(item2.getAge()).isEqualTo(22); + } + @Test // gh-447 @SuppressWarnings("unchecked") void primaryConstructorWithGenericObject() throws Exception { @@ -428,6 +459,20 @@ class GraphQlArgumentBinderTests { } + static class PrimaryConstructorItemMapBean { + + private final Map map; + + public PrimaryConstructorItemMapBean(Map map) { + this.map = map; + } + + public Map getMap() { + return this.map; + } + } + + @SuppressWarnings("OptionalUsedAsFieldOrParameterType") static class PrimaryConstructorOptionalItemBean { From bc8ee162f9f477333ba4241ccd67b5352a5ccd61 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Tue, 18 Oct 2022 18:23:05 +0100 Subject: [PATCH 2/4] Minor refactoring in GraphQlArgumentBinder See gh-449 --- .../graphql/data/GraphQlArgumentBinder.java | 234 +++++++++--------- 1 file changed, 114 insertions(+), 120 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 ca306179..16163bc2 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 @@ -149,7 +149,7 @@ public class GraphQlArgumentBinder { if (isApproximableCollectionType(rawValue)) { segments.push(argumentName); - return createCollection((Collection) rawValue, targetType, bindingResult, segments); + return bindCollection((Collection) rawValue, targetType, bindingResult, segments); } if (targetClass == Optional.class) { @@ -160,7 +160,7 @@ public class GraphQlArgumentBinder { // From Map if (rawValue instanceof Map) { - Object target = createValue((Map) rawValue, targetType, bindingResult, segments); + Object target = bindMap((Map) rawValue, targetType, bindingResult, segments); return wrapAsOptionalIfNecessary(target, targetType); } @@ -195,7 +195,7 @@ public class GraphQlArgumentBinder { } @SuppressWarnings({"ConstantConditions", "unchecked"}) - private Collection createCollection( + private Collection bindCollection( Collection rawCollection, ResolvableType collectionType, BindingResult bindingResult, Stack segments) { @@ -219,7 +219,7 @@ public class GraphQlArgumentBinder { collection.add((T) rawValue); } else if (rawValue instanceof Map) { - collection.add((T) createValueOrNull((Map) rawValue, elementType, bindingResult, segments)); + collection.add((T) bindMap((Map) rawValue, elementType, bindingResult, segments)); } else { collection.add((T) convertValue(rawValue, elementClass, bindingResult, segments)); @@ -230,134 +230,128 @@ public class GraphQlArgumentBinder { } @Nullable - private Object createValueOrNull( - Map rawMap, ResolvableType targetType, BindingResult result, Stack segments) { + @SuppressWarnings("unchecked") + private Object bindMap( + Map rawMap, ResolvableType targetType, BindingResult bindingResult, + Stack segments) { try { - return createValue(rawMap, targetType, result, segments); + Class targetClass = targetType.resolve(); + Assert.notNull(targetClass, "Unknown target class"); + + if (Map.class.isAssignableFrom(targetClass)) { + ResolvableType valueType = targetType.asMap().getGeneric(1); + Class valueClass = valueType.resolve(); + if (valueClass == null) { + bindingResult.rejectValue(toArgumentPath(segments), "unknownMapValueType", "Unknown Map value type"); + return Collections.emptyMap(); + } + Map map = CollectionFactory.createMap(targetClass, rawMap.size()); + for (Map.Entry entry : rawMap.entrySet()) { + Object rawValue = entry.getValue(); + segments.push("[" + entry.getKey() + "]"); + if (rawValue == null || valueType.isAssignableFrom(rawValue.getClass())) { + map.put(entry.getKey(), entry.getValue()); + } + else if (rawValue instanceof Map) { + map.put(entry.getKey(), bindMap( + (Map) rawValue, valueType, bindingResult, segments)); + } + else { + map.put(entry.getKey(), convertValue(rawValue, valueClass, bindingResult, segments)); + } + segments.pop(); + } + return map; + } + + Object target; + Constructor ctor = BeanUtils.getResolvableConstructor(targetClass); + + // Default constructor + data binding via properties + + if (ctor.getParameterCount() == 0) { + target = BeanUtils.instantiateClass(ctor); + DataBinder dataBinder = new DataBinder(target); + initDataBinder(dataBinder); + dataBinder.getBindingResult().setNestedPath(toArgumentPath(segments)); + dataBinder.setConversionService(getConversionService()); + dataBinder.bind(toPropertyValues(rawMap)); + + if (dataBinder.getBindingResult().hasErrors()) { + addDataBinderErrors(dataBinder, bindingResult, segments); + throw new BindException(bindingResult); + } + + return target; + } + + // Data class constructor + + if (!segments.isEmpty()) { + segments.push("."); + } + + String[] paramNames = BeanUtils.getParameterNames(ctor); + Class[] paramTypes = ctor.getParameterTypes(); + Object[] args = new Object[paramTypes.length]; + + for (int i = 0; i < paramNames.length; i++) { + String paramName = paramNames[i]; + Object rawValue = rawMap.get(paramName); + segments.push(paramName); + MethodParameter methodParam = new MethodParameter(ctor, i); + if (rawValue == null && methodParam.isOptional()) { + args[i] = (paramTypes[i] == Optional.class ? Optional.empty() : null); + } + else if (paramTypes[i] == Object.class) { + args[i] = rawValue; + } + else if (isApproximableCollectionType(rawValue)) { + ResolvableType elementType = ResolvableType.forMethodParameter(methodParam); + args[i] = bindCollection((Collection) rawValue, elementType, bindingResult, segments); + } + else if (rawValue instanceof Map) { + boolean isOptional = (paramTypes[i] == Optional.class); + ResolvableType type = ResolvableType.forMethodParameter(methodParam.nestedIfOptional()); + Object value = bindMap((Map) rawValue, type, bindingResult, segments); + args[i] = (isOptional ? Optional.ofNullable(value) : value); + } + else { + args[i] = convertValue(rawValue, paramTypes[i], new TypeDescriptor(methodParam), bindingResult, segments); + } + segments.pop(); + } + + if (segments.size() > 1) { + segments.pop(); + } + + try { + return BeanUtils.instantiateClass(ctor, args); + } + catch (BeanInstantiationException ex) { + // Swallow if we had binding errors, it's as far as we could go + checkBindingResult(bindingResult); + throw ex; + } } catch (BindException ex) { return null; } } - @SuppressWarnings("unchecked") - private Object createValue( - Map rawMap, ResolvableType targetType, BindingResult bindingResult, - Stack segments) throws BindException { - - Class targetClass = targetType.resolve(); - Assert.notNull(targetClass, "Unknown target class"); - - if (Map.class.isAssignableFrom(targetClass)) { - ResolvableType valueType = targetType.asMap().getGeneric(1); - Class valueClass = valueType.resolve(); - if (valueClass == null) { - bindingResult.rejectValue(toArgumentPath(segments), "unknownMapValueType", "Unknown Map value type"); - return Collections.emptyMap(); - } - Map map = CollectionFactory.createMap(targetClass, rawMap.size()); - for (Map.Entry entry : rawMap.entrySet()) { - Object rawValue = entry.getValue(); - segments.push("[" + entry.getKey() + "]"); - if (rawValue == null || valueType.isAssignableFrom(rawValue.getClass())) { - map.put(entry.getKey(), entry.getValue()); - } - else if (rawValue instanceof Map) { - map.put(entry.getKey(), createValueOrNull( - (Map) rawValue, valueType, bindingResult, segments)); - } - else { - map.put(entry.getKey(), convertValue(rawValue, valueClass, bindingResult, segments)); - } - segments.pop(); - } - return map; - } - - Object target; - Constructor ctor = BeanUtils.getResolvableConstructor(targetClass); - - // Default constructor + data binding via properties - - if (ctor.getParameterCount() == 0) { - target = BeanUtils.instantiateClass(ctor); - DataBinder dataBinder = new DataBinder(target); - initDataBinder(dataBinder); - dataBinder.getBindingResult().setNestedPath(toArgumentPath(segments)); - dataBinder.setConversionService(getConversionService()); - dataBinder.bind(initBindValues(rawMap)); - - if (dataBinder.getBindingResult().hasErrors()) { - addErrors(dataBinder, bindingResult, segments); - throw new BindException(bindingResult); - } - - return target; - } - - // Data class constructor - - if (!segments.isEmpty()) { - segments.push("."); - } - - String[] paramNames = BeanUtils.getParameterNames(ctor); - Class[] paramTypes = ctor.getParameterTypes(); - Object[] args = new Object[paramTypes.length]; - - for (int i = 0; i < paramNames.length; i++) { - String paramName = paramNames[i]; - Object rawValue = rawMap.get(paramName); - segments.push(paramName); - MethodParameter methodParam = new MethodParameter(ctor, i); - if (rawValue == null && methodParam.isOptional()) { - args[i] = (paramTypes[i] == Optional.class ? Optional.empty() : null); - } - else if (paramTypes[i] == Object.class) { - args[i] = rawValue; - } - else if (isApproximableCollectionType(rawValue)) { - ResolvableType elementType = ResolvableType.forMethodParameter(methodParam); - args[i] = createCollection((Collection) rawValue, elementType, bindingResult, segments); - } - else if (rawValue instanceof Map) { - boolean isOptional = (paramTypes[i] == Optional.class); - ResolvableType type = ResolvableType.forMethodParameter(methodParam.nestedIfOptional()); - Object value = createValueOrNull((Map) rawValue, type, bindingResult, segments); - args[i] = (isOptional ? Optional.ofNullable(value) : value); - } - else { - args[i] = convertValue(rawValue, paramTypes[i], new TypeDescriptor(methodParam), bindingResult, segments); - } - segments.pop(); - } - - if (segments.size() > 1) { - segments.pop(); - } - - try { - return BeanUtils.instantiateClass(ctor, args); - } - catch (BeanInstantiationException ex) { - // Swallow if we had binding errors, it's as far as we could go - checkBindingResult(bindingResult); - throw ex; - } - } - - private MutablePropertyValues initBindValues(Map rawMap) { + private static MutablePropertyValues toPropertyValues(Map rawMap) { MutablePropertyValues mpvs = new MutablePropertyValues(); Stack segments = new Stack<>(); for (String key : rawMap.keySet()) { - addBindValues(mpvs, key, rawMap.get(key), segments); + addPropertyValue(mpvs, key, rawMap.get(key), segments); } return mpvs; } @SuppressWarnings("unchecked") - private void addBindValues(MutablePropertyValues mpvs, String name, Object value, Stack segments) { + private static void addPropertyValue(MutablePropertyValues mpvs, String name, Object value, Stack segments) { if (value instanceof List) { List items = (List) value; if (items.isEmpty()) { @@ -367,7 +361,7 @@ public class GraphQlArgumentBinder { } else { for (int i = 0; i < items.size(); i++) { - addBindValues(mpvs, name + "[" + i + "]", items.get(i), segments); + addPropertyValue(mpvs, name + "[" + i + "]", items.get(i), segments); } } } @@ -375,7 +369,7 @@ public class GraphQlArgumentBinder { segments.push(name + "."); Map map = (Map) value; for (String key : map.keySet()) { - addBindValues(mpvs, key, map.get(key), segments); + addPropertyValue(mpvs, key, map.get(key), segments); } segments.pop(); } @@ -386,7 +380,7 @@ public class GraphQlArgumentBinder { } } - private String toArgumentPath(Stack path) { + private static String toArgumentPath(Stack path) { StringBuilder sb = new StringBuilder(); path.forEach(sb::append); return sb.toString(); @@ -415,7 +409,7 @@ public class GraphQlArgumentBinder { return null; } - private void addErrors(DataBinder binder, BindingResult bindingResult, Stack segments) { + private static void addDataBinderErrors(DataBinder binder, BindingResult bindingResult, Stack segments) { String path = (!segments.isEmpty() ? toArgumentPath(segments) + "." : ""); binder.getBindingResult().getFieldErrors().forEach(error -> bindingResult.addError( new FieldError(bindingResult.getObjectName(), path + error.getField(), From d38ffeae6b1f8ff405c0835b05f888bb7ce8c65e Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Wed, 19 Oct 2022 07:16:22 +0100 Subject: [PATCH 3/4] Consistent raw value handling in GraphQlArgumentBinder This commit ensures a single method is used to bind raw values, whether those a top level argument value, or a value nested within a collection or map. See gh-449 --- .../graphql/data/GraphQlArgumentBinder.java | 299 ++++++++---------- .../data/GraphQlArgumentBinderTests.java | 16 +- 2 files changed, 133 insertions(+), 182 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 16163bc2..6abf540d 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 @@ -34,7 +34,6 @@ import org.springframework.beans.MutablePropertyValues; import org.springframework.beans.SimpleTypeConverter; import org.springframework.beans.TypeMismatchException; import org.springframework.core.CollectionFactory; -import org.springframework.core.MethodParameter; import org.springframework.core.ResolvableType; import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.TypeDescriptor; @@ -113,7 +112,7 @@ public class GraphQlArgumentBinder { * Bind a single argument, or the full arguments map, onto an object of the * given target type. * @param environment for access to the arguments - * @param argumentName the name of the argument to bind, or {@code null} to + * @param name the name of the argument to bind, or {@code null} to * use the full arguments map * @param targetType the type of Object to create * @return the created Object, possibly {@code null} @@ -124,58 +123,32 @@ public class GraphQlArgumentBinder { * is the argument path where the issue occurred. */ @Nullable - @SuppressWarnings("unchecked") public Object bind( - DataFetchingEnvironment environment, @Nullable String argumentName, ResolvableType targetType) + DataFetchingEnvironment environment, @Nullable String name, ResolvableType targetType) throws BindException { - Object rawValue = (argumentName != null ? - environment.getArgument(argumentName) : environment.getArguments()); + Object rawValue = (name != null ? + environment.getArgument(name) : environment.getArguments()); - if (rawValue == null) { - return wrapAsOptionalIfNecessary(null, targetType); + DataBinder binder = new DataBinder(null, name != null ? ("Arguments[" + name + "]") : "Arguments"); + initDataBinder(binder); + BindingResult bindingResult = binder.getBindingResult(); + + Stack segments = new Stack<>(); + if (name != null) { + segments.push(name); } Class targetClass = targetType.resolve(); Assert.notNull(targetClass, "Could not determine target type from " + targetType); - DataBinder binder = new DataBinder(null, argumentName != null ? argumentName : "arguments"); - initDataBinder(binder); - BindingResult bindingResult = binder.getBindingResult(); - Stack segments = new Stack<>(); + Object targetValue = bindRawValue(rawValue, targetType, targetClass, bindingResult, segments); - try { - // From Collection - - if (isApproximableCollectionType(rawValue)) { - segments.push(argumentName); - return bindCollection((Collection) rawValue, targetType, bindingResult, segments); - } - - if (targetClass == Optional.class) { - targetClass = targetType.getNested(2).resolve(); - Assert.notNull(targetClass, "Could not determine Optional type from " + targetType); - } - - // From Map - - if (rawValue instanceof Map) { - Object target = bindMap((Map) rawValue, targetType, bindingResult, segments); - return wrapAsOptionalIfNecessary(target, targetType); - } - - // From Scalar - - if (targetClass.isInstance(rawValue)) { - return wrapAsOptionalIfNecessary(rawValue, targetType); - } - - Object target = convertValue(rawValue, targetClass, bindingResult, segments); - return wrapAsOptionalIfNecessary(target, targetType); - } - finally { - checkBindingResult(bindingResult); + if (bindingResult.hasErrors()) { + throw new BindException(bindingResult); } + + return targetValue; } private void initDataBinder(DataBinder binder) { @@ -183,23 +156,45 @@ public class GraphQlArgumentBinder { this.dataBinderInitializers.forEach(initializer -> initializer.accept(binder)); } - @Nullable - private Object wrapAsOptionalIfNecessary(@Nullable Object value, ResolvableType type) { - return (type.resolve(Object.class).equals(Optional.class) ? Optional.ofNullable(value) : value); - } - - private boolean isApproximableCollectionType(@Nullable Object rawValue) { - return (rawValue != null && - (CollectionFactory.isApproximableCollectionType(rawValue.getClass()) || - rawValue instanceof List)); // it may be SingletonList - } - @SuppressWarnings({"ConstantConditions", "unchecked"}) - private Collection bindCollection( + @Nullable + private Object bindRawValue( + Object rawValue, ResolvableType targetValueType, Class targetValueClass, + BindingResult bindingResult, Stack segments) { + + boolean isOptional = targetValueClass == Optional.class; + + if (isOptional) { + targetValueType = targetValueType.getNested(2); + targetValueClass = targetValueType.resolve(); + } + + Object targetValue; + if (rawValue == null || targetValueClass == Object.class) { + targetValue = rawValue; + } + else if (rawValue instanceof Collection) { + targetValue = bindCollection((Collection) rawValue, targetValueType, bindingResult, segments); + } + else if (rawValue instanceof Map) { + targetValue = bindMap((Map) rawValue, targetValueType, bindingResult, segments); + } + else { + targetValue = (targetValueClass.isAssignableFrom(rawValue.getClass()) ? + rawValue : convertValue(rawValue, targetValueClass, bindingResult, segments)); + } + + return (isOptional ? Optional.ofNullable(targetValue) : targetValue); + } + + private Collection bindCollection( Collection rawCollection, ResolvableType collectionType, BindingResult bindingResult, Stack segments) { - if (!Collection.class.isAssignableFrom(collectionType.resolve())) { + Class collectionClass = collectionType.resolve(); + Assert.notNull(collectionClass, "Unknown Collection class"); + + if (!Collection.class.isAssignableFrom(collectionClass)) { bindingResult.rejectValue(toArgumentPath(segments), "typeMismatch", "Expected collection: " + collectionType); return Collections.emptyList(); } @@ -211,133 +206,95 @@ public class GraphQlArgumentBinder { return Collections.emptyList(); } - Collection collection = CollectionFactory.createCollection(collectionType.getRawClass(), elementClass, rawCollection.size()); - int i = 0; + Collection collection = + CollectionFactory.createCollection(collectionClass, elementClass, rawCollection.size()); + + int index = 0; for (Object rawValue : rawCollection) { - segments.push("[" + i++ + "]"); - if (rawValue == null || elementClass.isAssignableFrom(rawValue.getClass())) { - collection.add((T) rawValue); - } - else if (rawValue instanceof Map) { - collection.add((T) bindMap((Map) rawValue, elementType, bindingResult, segments)); - } - else { - collection.add((T) convertValue(rawValue, elementClass, bindingResult, segments)); - } + segments.push("[" + index++ + "]"); + collection.add(bindRawValue(rawValue, elementType, elementClass, bindingResult, segments)); segments.pop(); } return collection; } @Nullable - @SuppressWarnings("unchecked") private Object bindMap( Map rawMap, ResolvableType targetType, BindingResult bindingResult, Stack segments) { - try { - Class targetClass = targetType.resolve(); - Assert.notNull(targetClass, "Unknown target class"); + Class targetClass = targetType.resolve(); + Assert.notNull(targetClass, "Unknown target class"); - if (Map.class.isAssignableFrom(targetClass)) { - ResolvableType valueType = targetType.asMap().getGeneric(1); - Class valueClass = valueType.resolve(); - if (valueClass == null) { - bindingResult.rejectValue(toArgumentPath(segments), "unknownMapValueType", "Unknown Map value type"); - return Collections.emptyMap(); - } - Map map = CollectionFactory.createMap(targetClass, rawMap.size()); - for (Map.Entry entry : rawMap.entrySet()) { - Object rawValue = entry.getValue(); - segments.push("[" + entry.getKey() + "]"); - if (rawValue == null || valueType.isAssignableFrom(rawValue.getClass())) { - map.put(entry.getKey(), entry.getValue()); - } - else if (rawValue instanceof Map) { - map.put(entry.getKey(), bindMap( - (Map) rawValue, valueType, bindingResult, segments)); - } - else { - map.put(entry.getKey(), convertValue(rawValue, valueClass, bindingResult, segments)); - } - segments.pop(); - } - return map; + if (Map.class.isAssignableFrom(targetClass)) { + ResolvableType valueType = targetType.asMap().getGeneric(1); + Class valueClass = valueType.resolve(); + if (valueClass == null) { + bindingResult.rejectValue(toArgumentPath(segments), "unknownMapValueType", "Unknown Map value type"); + return Collections.emptyMap(); } - - Object target; - Constructor ctor = BeanUtils.getResolvableConstructor(targetClass); - - // Default constructor + data binding via properties - - if (ctor.getParameterCount() == 0) { - target = BeanUtils.instantiateClass(ctor); - DataBinder dataBinder = new DataBinder(target); - initDataBinder(dataBinder); - dataBinder.getBindingResult().setNestedPath(toArgumentPath(segments)); - dataBinder.setConversionService(getConversionService()); - dataBinder.bind(toPropertyValues(rawMap)); - - if (dataBinder.getBindingResult().hasErrors()) { - addDataBinderErrors(dataBinder, bindingResult, segments); - throw new BindException(bindingResult); - } - - return target; - } - - // Data class constructor - - if (!segments.isEmpty()) { - segments.push("."); - } - - String[] paramNames = BeanUtils.getParameterNames(ctor); - Class[] paramTypes = ctor.getParameterTypes(); - Object[] args = new Object[paramTypes.length]; - - for (int i = 0; i < paramNames.length; i++) { - String paramName = paramNames[i]; - Object rawValue = rawMap.get(paramName); - segments.push(paramName); - MethodParameter methodParam = new MethodParameter(ctor, i); - if (rawValue == null && methodParam.isOptional()) { - args[i] = (paramTypes[i] == Optional.class ? Optional.empty() : null); - } - else if (paramTypes[i] == Object.class) { - args[i] = rawValue; - } - else if (isApproximableCollectionType(rawValue)) { - ResolvableType elementType = ResolvableType.forMethodParameter(methodParam); - args[i] = bindCollection((Collection) rawValue, elementType, bindingResult, segments); - } - else if (rawValue instanceof Map) { - boolean isOptional = (paramTypes[i] == Optional.class); - ResolvableType type = ResolvableType.forMethodParameter(methodParam.nestedIfOptional()); - Object value = bindMap((Map) rawValue, type, bindingResult, segments); - args[i] = (isOptional ? Optional.ofNullable(value) : value); - } - else { - args[i] = convertValue(rawValue, paramTypes[i], new TypeDescriptor(methodParam), bindingResult, segments); - } + Map map = CollectionFactory.createMap(targetClass, rawMap.size()); + 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(); } - - if (segments.size() > 1) { - segments.pop(); - } - - try { - return BeanUtils.instantiateClass(ctor, args); - } - catch (BeanInstantiationException ex) { - // Swallow if we had binding errors, it's as far as we could go - checkBindingResult(bindingResult); - throw ex; - } + return map; } - catch (BindException ex) { - return null; + + Object target; + Constructor constructor = BeanUtils.getResolvableConstructor(targetClass); + + // Default constructor + data binding via properties + + if (constructor.getParameterCount() == 0) { + target = BeanUtils.instantiateClass(constructor); + DataBinder dataBinder = new DataBinder(target); + initDataBinder(dataBinder); + dataBinder.getBindingResult().setNestedPath(toArgumentPath(segments)); + dataBinder.setConversionService(getConversionService()); + dataBinder.bind(toPropertyValues(rawMap)); + + if (dataBinder.getBindingResult().hasErrors()) { + copyBindingErrors(dataBinder, bindingResult, segments); + return null; + } + + return target; + } + + // Data class constructor + + if (!segments.isEmpty()) { + segments.push("."); + } + + String[] paramNames = BeanUtils.getParameterNames(constructor); + Class[] paramTypes = constructor.getParameterTypes(); + Object[] args = new Object[paramTypes.length]; + + 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(); + } + + try { + return BeanUtils.instantiateClass(constructor, args); + } + catch (BeanInstantiationException ex) { + // Ignore if we had binding errors already + if (bindingResult.hasErrors()) { + return null; + } + throw ex; } } @@ -409,7 +366,7 @@ public class GraphQlArgumentBinder { return null; } - private static void addDataBinderErrors(DataBinder binder, BindingResult bindingResult, Stack segments) { + private static void copyBindingErrors(DataBinder binder, BindingResult bindingResult, Stack segments) { String path = (!segments.isEmpty() ? toArgumentPath(segments) + "." : ""); binder.getBindingResult().getFieldErrors().forEach(error -> bindingResult.addError( new FieldError(bindingResult.getObjectName(), path + error.getField(), @@ -417,10 +374,4 @@ public class GraphQlArgumentBinder { error.getArguments(), error.getDefaultMessage()))); } - private void checkBindingResult(BindingResult bindingResult) throws BindException { - if (bindingResult.hasErrors()) { - throw new BindException(bindingResult); - } - } - } 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 dead81ad..6f575324 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 @@ -120,8 +120,8 @@ class GraphQlArgumentBinderTests { .extracting(ex -> ((BindException) ex).getFieldErrors()) .satisfies(errors -> { assertThat(errors).hasSize(1); - assertThat(errors.get(0).getObjectName()).isEqualTo("key"); - assertThat(errors.get(0).getField()).isEqualTo("age"); + assertThat(errors.get(0).getObjectName()).isEqualTo("Arguments[key]"); + assertThat(errors.get(0).getField()).isEqualTo("key.age"); assertThat(errors.get(0).getRejectedValue()).isEqualTo("invalid"); }); } @@ -246,12 +246,12 @@ class GraphQlArgumentBinderTests { .satisfies(errors -> { assertThat(errors).hasSize(2); - assertThat(errors.get(0).getObjectName()).isEqualTo("key"); - assertThat(errors.get(0).getField()).isEqualTo("age"); + assertThat(errors.get(0).getObjectName()).isEqualTo("Arguments[key]"); + assertThat(errors.get(0).getField()).isEqualTo("key.age"); assertThat(errors.get(0).getRejectedValue()).isEqualTo("invalid"); - assertThat(errors.get(1).getObjectName()).isEqualTo("key"); - assertThat(errors.get(1).getField()).isEqualTo("item.age"); + assertThat(errors.get(0).getObjectName()).isEqualTo("Arguments[key]"); + assertThat(errors.get(1).getField()).isEqualTo("key.item.age"); assertThat(errors.get(1).getRejectedValue()).isEqualTo("invalid"); }); } @@ -272,8 +272,8 @@ class GraphQlArgumentBinderTests { assertThat(errors).hasSize(2); for (int i = 0; i < errors.size(); i++) { FieldError error = errors.get(i); - assertThat(error.getObjectName()).isEqualTo("key"); - assertThat(error.getField()).isEqualTo("items[" + i + "].age"); + assertThat(error.getObjectName()).isEqualTo("Arguments[key]"); + assertThat(error.getField()).isEqualTo("key.items[" + i + "].age"); assertThat(error.getRejectedValue()).isEqualTo("invalid"); assertThat(error.getDefaultMessage()).startsWith("Failed to convert property value"); } From 11fab7dafd3a924d0fed290a2a84064c849f1838 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Wed, 19 Oct 2022 09:07:36 +0100 Subject: [PATCH 4/4] Polishing See gh-449 --- .../graphql/data/GraphQlArgumentBinder.java | 179 +++++++++--------- 1 file changed, 86 insertions(+), 93 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 6abf540d..654e1618 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 @@ -38,7 +38,6 @@ 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.Assert; import org.springframework.validation.BindException; import org.springframework.validation.BindingErrorProcessor; import org.springframework.validation.BindingResult; @@ -65,6 +64,7 @@ public class GraphQlArgumentBinder { */ private static final int DEFAULT_AUTO_GROW_COLLECTION_LIMIT = 1024; + @Nullable private final SimpleTypeConverter typeConverter; @@ -139,10 +139,8 @@ public class GraphQlArgumentBinder { segments.push(name); } - Class targetClass = targetType.resolve(); - Assert.notNull(targetClass, "Could not determine target type from " + targetType); - - Object targetValue = bindRawValue(rawValue, targetType, targetClass, bindingResult, segments); + Object targetValue = bindRawValue( + rawValue, targetType, targetType.resolve(Object.class), bindingResult, segments); if (bindingResult.hasErrors()) { throw new BindException(bindingResult); @@ -159,51 +157,43 @@ public class GraphQlArgumentBinder { @SuppressWarnings({"ConstantConditions", "unchecked"}) @Nullable private Object bindRawValue( - Object rawValue, ResolvableType targetValueType, Class targetValueClass, + Object rawValue, ResolvableType targetType, Class targetClass, BindingResult bindingResult, Stack segments) { - boolean isOptional = targetValueClass == Optional.class; + boolean isOptional = (targetClass == Optional.class); if (isOptional) { - targetValueType = targetValueType.getNested(2); - targetValueClass = targetValueType.resolve(); + targetType = targetType.getNested(2); + targetClass = targetType.resolve(); } - Object targetValue; - if (rawValue == null || targetValueClass == Object.class) { - targetValue = rawValue; + Object value; + if (rawValue == null || targetClass == Object.class) { + value = rawValue; } else if (rawValue instanceof Collection) { - targetValue = bindCollection((Collection) rawValue, targetValueType, bindingResult, segments); + value = bindCollection((Collection) rawValue, targetType, targetClass, bindingResult, segments); } else if (rawValue instanceof Map) { - targetValue = bindMap((Map) rawValue, targetValueType, bindingResult, segments); + value = bindMap((Map) rawValue, targetType, targetClass, bindingResult, segments); } else { - targetValue = (targetValueClass.isAssignableFrom(rawValue.getClass()) ? - rawValue : convertValue(rawValue, targetValueClass, bindingResult, segments)); + value = (targetClass.isAssignableFrom(rawValue.getClass()) ? + rawValue : convertValue(rawValue, targetClass, bindingResult, segments)); } - return (isOptional ? Optional.ofNullable(targetValue) : targetValue); + return (isOptional ? Optional.ofNullable(value) : value); } private Collection bindCollection( - Collection rawCollection, ResolvableType collectionType, + Collection rawCollection, ResolvableType collectionType, Class collectionClass, BindingResult bindingResult, Stack segments) { - Class collectionClass = collectionType.resolve(); - Assert.notNull(collectionClass, "Unknown Collection class"); - - if (!Collection.class.isAssignableFrom(collectionClass)) { - bindingResult.rejectValue(toArgumentPath(segments), "typeMismatch", "Expected collection: " + collectionType); - return Collections.emptyList(); - } - ResolvableType elementType = collectionType.asCollection().getGeneric(0); Class elementClass = collectionType.asCollection().getGeneric(0).resolve(); if (elementClass == null) { - bindingResult.rejectValue(toArgumentPath(segments), "unknownElementType", "Unknown element type"); - return Collections.emptyList(); + bindingResult.rejectValue(toArgumentPath(segments), "unknownTargetType", "Unknown target type"); + return Collections.emptyList(); // Keep going, report as many errors as we can } Collection collection = @@ -215,58 +205,80 @@ public class GraphQlArgumentBinder { collection.add(bindRawValue(rawValue, elementType, elementClass, bindingResult, segments)); segments.pop(); } + 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, BindingResult bindingResult, - Stack segments) { - - Class targetClass = targetType.resolve(); - Assert.notNull(targetClass, "Unknown target class"); + Map rawMap, ResolvableType targetType, Class targetClass, + BindingResult bindingResult, Stack segments) { if (Map.class.isAssignableFrom(targetClass)) { - ResolvableType valueType = targetType.asMap().getGeneric(1); - Class valueClass = valueType.resolve(); - if (valueClass == null) { - bindingResult.rejectValue(toArgumentPath(segments), "unknownMapValueType", "Unknown Map value type"); - return Collections.emptyMap(); - } - Map map = CollectionFactory.createMap(targetClass, rawMap.size()); - 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(); - } - return map; + return bindMapToMap(rawMap, targetType, bindingResult, segments, targetClass); } - Object target; Constructor constructor = BeanUtils.getResolvableConstructor(targetClass); - - // Default constructor + data binding via properties - - if (constructor.getParameterCount() == 0) { - target = BeanUtils.instantiateClass(constructor); - DataBinder dataBinder = new DataBinder(target); - initDataBinder(dataBinder); - dataBinder.getBindingResult().setNestedPath(toArgumentPath(segments)); - dataBinder.setConversionService(getConversionService()); - dataBinder.bind(toPropertyValues(rawMap)); - - if (dataBinder.getBindingResult().hasErrors()) { - copyBindingErrors(dataBinder, bindingResult, segments); - return null; - } - - return target; + if (constructor.getParameterCount() > 0) { + return bindMapToObjectViaConstructor(rawMap, constructor, bindingResult, segments); } - // Data class constructor + Object target = BeanUtils.instantiateClass(constructor); + DataBinder dataBinder = new DataBinder(target); + initDataBinder(dataBinder); + dataBinder.getBindingResult().setNestedPath(toArgumentPath(segments)); + dataBinder.setConversionService(getConversionService()); + dataBinder.bind(createPropertyValues(rawMap)); - if (!segments.isEmpty()) { + 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; + } + + return target; + } + + private Map bindMapToMap( + Map rawMap, ResolvableType targetType, BindingResult bindingResult, + Stack segments, Class targetClass) { + + ResolvableType valueType = targetType.asMap().getGeneric(1); + Class valueClass = valueType.resolve(); + if (valueClass == null) { + bindingResult.rejectValue(toArgumentPath(segments), "unknownTargetType", "Unknown target type"); + return Collections.emptyMap(); // Keep going, report as many errors as we can + } + + Map map = CollectionFactory.createMap(targetClass, rawMap.size()); + + 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(); + } + + return map; + } + + @Nullable + private Object bindMapToObjectViaConstructor( + Map rawMap, Constructor constructor, BindingResult bindingResult, + Stack segments) { + + if (segments.size() > 0) { segments.push("."); } @@ -290,7 +302,7 @@ public class GraphQlArgumentBinder { return BeanUtils.instantiateClass(constructor, args); } catch (BeanInstantiationException ex) { - // Ignore if we had binding errors already + // Ignore: we had binding errors to begin with if (bindingResult.hasErrors()) { return null; } @@ -298,7 +310,7 @@ public class GraphQlArgumentBinder { } } - private static MutablePropertyValues toPropertyValues(Map rawMap) { + private static MutablePropertyValues createPropertyValues(Map rawMap) { MutablePropertyValues mpvs = new MutablePropertyValues(); Stack segments = new Stack<>(); for (String key : rawMap.keySet()) { @@ -337,25 +349,14 @@ public class GraphQlArgumentBinder { } } - private static String toArgumentPath(Stack path) { - StringBuilder sb = new StringBuilder(); - path.forEach(sb::append); - return sb.toString(); - } - @SuppressWarnings("unchecked") @Nullable - private T convertValue(@Nullable Object rawValue, Class type, BindingResult result, Stack segments) { - return (T) convertValue(rawValue, type, TypeDescriptor.valueOf(type), result, segments); - } - - @Nullable - private Object convertValue( - @Nullable Object rawValue, Class type, TypeDescriptor descriptor, - BindingResult bindingResult, Stack segments) { + private T convertValue( + @Nullable Object rawValue, Class type, BindingResult bindingResult, Stack segments) { + Object value = null; try { - return getTypeConverter().convertIfNecessary(rawValue, type, descriptor); + value = getTypeConverter().convertIfNecessary(rawValue, (Class) type, TypeDescriptor.valueOf(type)); } catch (TypeMismatchException ex) { String name = toArgumentPath(segments); @@ -363,15 +364,7 @@ public class GraphQlArgumentBinder { bindingResult.recordFieldValue(name, type, rawValue); this.bindingErrorProcessor.processPropertyAccessException(ex, bindingResult); } - return null; - } - - private static void copyBindingErrors(DataBinder binder, BindingResult bindingResult, Stack segments) { - String path = (!segments.isEmpty() ? toArgumentPath(segments) + "." : ""); - binder.getBindingResult().getFieldErrors().forEach(error -> bindingResult.addError( - new FieldError(bindingResult.getObjectName(), path + error.getField(), - error.getRejectedValue(), error.isBindingFailure(), error.getCodes(), - error.getArguments(), error.getDefaultMessage()))); + return (T) value; } }