Polishing in GraphQlArgumentBinder

Author: rstoyanchev

Diff for: spring-graphql/src/main/java/org/springframework/graphql/data/GraphQlArgumentBinder.java

@@ -243,8 +243,8 @@ private Object bindMap(
 		Constructor<?> constructor = BeanUtils.getResolvableConstructor(targetClass);
 
 		Object value = (constructor.getParameterCount() > 0) ?
-				bindMapToObjectViaConstructor(rawMap, constructor, targetType, bindingResult) :
-				bindMapToObjectViaSetters(rawMap, constructor, targetType, bindingResult);
+				bindViaConstructorAndSetters(constructor, rawMap, targetType, bindingResult) :
+				bindViaSetters(constructor, rawMap, targetType, bindingResult);
 
 		bindingResult.popNestedPath();
 
@@ -273,9 +273,8 @@ private Map<?, Object> bindMapToMap(
 	}
 
 	@Nullable
-	private Object bindMapToObjectViaConstructor(
-			Map<String, Object> rawMap, Constructor<?> constructor, ResolvableType ownerType,
-			ArgumentsBindingResult bindingResult) {
+	private Object bindViaConstructorAndSetters(Constructor<?> constructor,
+			Map<String, Object> rawMap, ResolvableType ownerType, ArgumentsBindingResult bindingResult) {
 
 		String[] paramNames = BeanUtils.getParameterNames(constructor);
 		Class<?>[] paramTypes = constructor.getParameterTypes();
@@ -291,13 +290,9 @@ private Object bindMapToObjectViaConstructor(
 					name, rawMap.get(name), !rawMap.containsKey(name), targetType, paramTypes[i], bindingResult);
 		}
 
+		Object target;
 		try {
-			Object target = BeanUtils.instantiateClass(constructor, constructorArguments);
-			// only attempt further properties binding if there were no errors
-			if (!bindingResult.hasErrors()) {
-				bindProperties(rawMap, ownerType, bindingResult, target);
-			}
-			return target;
+			target = BeanUtils.instantiateClass(constructor, constructorArguments);
 		}
 		catch (BeanInstantiationException ex) {
 			// Ignore, if we had binding errors to begin with
@@ -306,18 +301,26 @@ private Object bindMapToObjectViaConstructor(
 			}
 			throw ex;
 		}
+
+		// If no errors, apply setters too
+		if (!bindingResult.hasErrors()) {
+			bindViaSetters(target, rawMap, ownerType, bindingResult);
+		}
+
+		return target;
 	}
 
-	private Object bindMapToObjectViaSetters(
-			Map<String, Object> rawMap, Constructor<?> constructor, ResolvableType ownerType,
-			ArgumentsBindingResult bindingResult) {
+	private Object bindViaSetters(Constructor<?> constructor,
+			Map<String, Object> rawMap, ResolvableType ownerType, ArgumentsBindingResult bindingResult) {
 
 		Object target = BeanUtils.instantiateClass(constructor);
-		bindProperties(rawMap, ownerType, bindingResult, target);
+		bindViaSetters(target, rawMap, ownerType, bindingResult);
 		return target;
 	}
 
-	private void bindProperties(Map<String, Object> rawMap, ResolvableType ownerType, ArgumentsBindingResult bindingResult, Object target) {
+	private void bindViaSetters(Object target,
+			Map<String, Object> rawMap, ResolvableType ownerType, ArgumentsBindingResult bindingResult) {
+
 		BeanWrapper beanWrapper = (this.fallBackOnDirectFieldAccess ?
 				new DirectFieldAccessFallbackBeanWrapper(target) : PropertyAccessorFactory.forBeanPropertyAccess(target));
 
@@ -355,7 +358,6 @@ private void bindProperties(Map<String, Object> rawMap, ResolvableType ownerType
 		}
 	}
 
-
 	@SuppressWarnings("unchecked")
 	@Nullable
 	private <T> T convertValue(

Diff for: spring-graphql/src/test/java/org/springframework/graphql/data/GraphQlArgumentBinderTests.java

@@ -195,14 +195,14 @@ void primaryConstructor() throws Exception {
 		assertThat(result).hasFieldOrPropertyWithValue("name", "test");
 	}
 
-	@Test
+	@Test // gh-1163
 	void mixedConstructorProperties() throws Exception {
 
-		Object result = bind("{\"name\":\"test\", \"age\":30}", ResolvableType.forClass(MixedConstructorPropertiesBean.class));
+		Object result = bind("{\"name\":\"test\", \"age\":30}",
+				ResolvableType.forClass(MixedConstructorPropertiesBean.class));
 
 		assertThat(result).isNotNull().isInstanceOf(MixedConstructorPropertiesBean.class);
-		assertThat(result).hasFieldOrPropertyWithValue("name", "test")
-				.hasFieldOrPropertyWithValue("age", 30);
+		assertThat(result).hasFieldOrPropertyWithValue("name", "test").hasFieldOrPropertyWithValue("age", 30);
 	}
 
 	@Test