From 02b16bdc65fdb65b691ca75af532d2fb6671ccd0 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Tue, 21 Sep 2021 09:45:45 +0200 Subject: [PATCH] Polish "Support contructor binding for input arguments" Prior to this commit, only default constructors were supported for instantiating input argument types. This commit adds a new `GraphQlInstantiator` class that manages the instantiation and binding of data fetching environment arguments. Default constructors and primary constructors are now handled. Also, List-like properties were not bound from the data fetching environment arguments to the `MutablePropertyValues` used for binding. This commit ensures that List elements are bound to the property values with array-like property paths. Fixes gh-139 Fixes gh-141 --- build.gradle | 20 +++ spring-graphql/build.gradle | 19 +- .../ArgumentMethodArgumentResolver.java | 81 +-------- .../support/GraphQlArgumentInstantiator.java | 131 ++++++++++++++ .../ArgumentMethodArgumentResolverTests.java | 42 +---- .../GraphQlArgumentInstantiatorTests.java | 163 ++++++++++++++++++ .../annotation/support/KotlinBookInput.kt | 3 - 7 files changed, 333 insertions(+), 126 deletions(-) create mode 100644 spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/GraphQlArgumentInstantiator.java create mode 100644 spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/GraphQlArgumentInstantiatorTests.java delete mode 100644 spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/KotlinBookInput.kt diff --git a/build.gradle b/build.gradle index 6712af56..783982f3 100644 --- a/build.gradle +++ b/build.gradle @@ -1,5 +1,6 @@ plugins { id 'io.spring.dependency-management' version '1.0.11.RELEASE' + id 'org.jetbrains.kotlin.jvm' version '1.5.31' apply false } ext { @@ -35,6 +36,24 @@ configure(moduleProjects) { targetCompatibility = JavaVersion.VERSION_1_8 } + pluginManager.withPlugin("kotlin") { + compileKotlin { + kotlinOptions { + jvmTarget = "1.8" + languageVersion = "1.3" + apiVersion = "1.3" + freeCompilerArgs = ["-Xjsr305=strict", "-Xsuppress-version-warnings", "-Xopt-in=kotlin.RequiresOptIn"] + allWarningsAsErrors = true + } + } + compileTestKotlin { + kotlinOptions { + jvmTarget = "1.8" + freeCompilerArgs = ["-Xjsr305=strict"] + } + } + } + dependencyManagement { imports { mavenBom "com.fasterxml.jackson:jackson-bom:2.12.5" @@ -42,6 +61,7 @@ configure(moduleProjects) { mavenBom "org.springframework:spring-framework-bom:5.3.9" mavenBom "org.springframework.data:spring-data-bom:2021.0.4" mavenBom "org.springframework.security:spring-security-bom:5.5.2" + mavenBom "org.jetbrains.kotlin:kotlin-bom:1.5.31" mavenBom "org.jetbrains.kotlinx:kotlinx-coroutines-bom:1.5.2" mavenBom "org.junit:junit-bom:5.7.2" } diff --git a/spring-graphql/build.gradle b/spring-graphql/build.gradle index 0d5653fe..b4cc4550 100644 --- a/spring-graphql/build.gradle +++ b/spring-graphql/build.gradle @@ -1,8 +1,7 @@ -plugins { - id 'org.jetbrains.kotlin.jvm' version '1.5.31' -} description = "GraphQL Support for Spring Applications" +apply plugin: "kotlin" + dependencies { api 'com.graphql-java:graphql-java' api 'io.projectreactor:reactor-core' @@ -21,6 +20,7 @@ dependencies { compileOnly 'com.google.code.findbugs:jsr305' compileOnly 'org.jetbrains.kotlinx:kotlinx-coroutines-core' + compileOnly 'org.jetbrains.kotlin:kotlin-stdlib' testImplementation 'org.junit.jupiter:junit-jupiter' testImplementation 'org.assertj:assertj-core' @@ -46,16 +46,3 @@ test { events "passed", "skipped", "failed" } } -repositories { - mavenCentral() -} -compileKotlin { - kotlinOptions { - jvmTarget = "1.8" - } -} -compileTestKotlin { - kotlinOptions { - jvmTarget = "1.8" - } -} diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/ArgumentMethodArgumentResolver.java b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/ArgumentMethodArgumentResolver.java index f37ee504..1733a04f 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/ArgumentMethodArgumentResolver.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/ArgumentMethodArgumentResolver.java @@ -15,18 +15,12 @@ */ package org.springframework.graphql.data.method.annotation.support; -import java.lang.reflect.Constructor; import java.util.Collection; -import java.util.Iterator; -import java.util.List; import java.util.Map; import java.util.Optional; -import java.util.Stack; import graphql.schema.DataFetchingEnvironment; -import org.springframework.beans.BeanUtils; -import org.springframework.beans.MutablePropertyValues; import org.springframework.core.CollectionFactory; import org.springframework.core.MethodParameter; import org.springframework.core.convert.TypeDescriptor; @@ -48,6 +42,8 @@ import org.springframework.validation.DataBinder; */ public class ArgumentMethodArgumentResolver implements HandlerMethodArgumentResolver { + private final GraphQlArgumentInstantiator instantiator = new GraphQlArgumentInstantiator(); + @Override public boolean supportsParameter(MethodParameter parameter) { return parameter.getParameterAnnotation(Argument.class) != null; @@ -78,10 +74,7 @@ public class ArgumentMethodArgumentResolver implements HandlerMethodArgumentReso if (annotation.required()) { throw new MissingArgumentException(name, parameter); } - if (parameterType.getType().equals(Optional.class)) { - return Optional.empty(); - } - return null; + returnValue(rawValue, parameterType.getType()); } if (CollectionFactory.isApproximableCollectionType(rawValue.getClass())) { @@ -100,40 +93,17 @@ public class ArgumentMethodArgumentResolver implements HandlerMethodArgumentReso } private Object returnValue(Object value, Class parameterType) { - return (parameterType.equals(Optional.class) ? Optional.of(value) : value); + if (parameterType.equals(Optional.class)) { + return Optional.ofNullable(value); + } + return value; } @SuppressWarnings("unchecked") private Object convert(Object rawValue, Class targetType) { Object target; if (rawValue instanceof Map) { - Constructor ctor = BeanUtils.getResolvableConstructor(targetType); - MutablePropertyValues propertyValues = extractPropertyValues((Map) rawValue); - - if (ctor.getParameterCount() == 0) { - target = BeanUtils.instantiateClass(ctor); - DataBinder dataBinder = new DataBinder(target); - dataBinder.bind(propertyValues); - } else { - // Data class constructor - DataBinder binder = new DataBinder(null); - 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 value = propertyValues.get(paramName); - value = (value instanceof List ? ((List) value).toArray() : value); - MethodParameter methodParam = new MethodParameter(ctor, i); - if (value == null && methodParam.isOptional()) { - args[i] = (methodParam.getParameterType() == Optional.class ? Optional.empty() : null); - } - else { - args[i] = binder.convertIfNecessary(value, paramTypes[i], methodParam); - } - } - target = BeanUtils.instantiateClass(ctor, args); - } + target = this.instantiator.instantiate(targetType, (Map) rawValue); } else if (targetType.isAssignableFrom(rawValue.getClass())) { return returnValue(rawValue, targetType); @@ -148,39 +118,4 @@ public class ArgumentMethodArgumentResolver implements HandlerMethodArgumentReso return target; } - private MutablePropertyValues extractPropertyValues(Map arguments) { - MutablePropertyValues mpvs = new MutablePropertyValues(); - Stack path = new Stack<>(); - visitArgumentMap(arguments, mpvs, path); - return mpvs; - } - - @SuppressWarnings("unchecked") - private void visitArgumentMap(Map arguments, MutablePropertyValues mpvs, Stack path) { - for (String key : arguments.keySet()) { - path.push(key); - Object value = arguments.get(key); - if (value instanceof Map) { - visitArgumentMap((Map) value, mpvs, path); - } - else { - String propertyName = pathToPropertyName(path); - mpvs.add(propertyName, value); - } - path.pop(); - } - } - - private String pathToPropertyName(Stack path) { - StringBuilder sb = new StringBuilder(); - Iterator it = path.iterator(); - while (it.hasNext()) { - sb.append(it.next()); - if (it.hasNext()) { - sb.append("."); - } - } - return sb.toString(); - } - } diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/GraphQlArgumentInstantiator.java b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/GraphQlArgumentInstantiator.java new file mode 100644 index 00000000..b4dd364c --- /dev/null +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/GraphQlArgumentInstantiator.java @@ -0,0 +1,131 @@ +/* + * Copyright 2020-2021 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.graphql.data.method.annotation.support; + +import java.lang.reflect.Constructor; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Stack; + +import org.springframework.beans.BeanUtils; +import org.springframework.beans.MutablePropertyValues; +import org.springframework.core.MethodParameter; +import org.springframework.validation.DataBinder; + +/** + * Instantiate a target type and bind data from + * {@link graphql.schema.DataFetchingEnvironment} arguments. + * + * @author Brian Clozel + */ +class GraphQlArgumentInstantiator { + + /** + * Instantiate the given target type and bind data from + * {@link graphql.schema.DataFetchingEnvironment} arguments. + *

This is considering the default constructor or a primary constructor + * if available. + * + * @param targetType the type of the argument to instantiate + * @param arguments the data fetching environment arguments + * @param the type of the input argument + * @return the instantiated and populated input argument. + * @throws IllegalStateException if there is no suitable constructor. + */ + @SuppressWarnings("unchecked") + public T instantiate(Class targetType, Map arguments) { + Object target; + Constructor ctor = BeanUtils.getResolvableConstructor(targetType); + MutablePropertyValues propertyValues = extractPropertyValues(arguments); + + if (ctor.getParameterCount() == 0) { + target = BeanUtils.instantiateClass(ctor); + DataBinder dataBinder = new DataBinder(target); + dataBinder.bind(propertyValues); + } + else { + // Data class constructor + DataBinder binder = new DataBinder(null); + 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 value = propertyValues.get(paramName); + value = (value instanceof List ? ((List) value).toArray() : value); + MethodParameter methodParam = new MethodParameter(ctor, i); + if (value == null && methodParam.isOptional()) { + args[i] = (methodParam.getParameterType() == Optional.class ? Optional.empty() : null); + } + else { + args[i] = binder.convertIfNecessary(value, paramTypes[i], methodParam); + } + } + target = BeanUtils.instantiateClass(ctor, args); + } + return (T) target; + } + + /** + * Perform a Depth First Search in the given JSON map to collect attribute values + * as {@link MutablePropertyValues} using the full property path as key. + */ + private MutablePropertyValues extractPropertyValues(Map arguments) { + MutablePropertyValues mpvs = new MutablePropertyValues(); + Stack path = new Stack<>(); + visitArgumentMap(arguments, mpvs, path); + return mpvs; + } + + @SuppressWarnings("unchecked") + private void visitArgumentMap(Map arguments, MutablePropertyValues mpvs, Stack path) { + for (String key : arguments.keySet()) { + Object value = arguments.get(key); + if (value instanceof List) { + List items = (List) value; + Map subValues = new HashMap<>(items.size()); + for (int i = 0; i < items.size(); i++) { + subValues.put(key + "[" + i + "]", items.get(i)); + } + visitArgumentMap(subValues, mpvs, path); + } + else if (value instanceof Map) { + path.push(key); + path.push("."); + visitArgumentMap((Map) value, mpvs, path); + path.pop(); + path.pop(); + } + else { + path.push(key); + String propertyName = pathToPropertyName(path); + mpvs.add(propertyName, value); + path.pop(); + } + } + } + + private String pathToPropertyName(Stack path) { + StringBuilder sb = new StringBuilder(); + for (String s : path) { + sb.append(s); + } + return sb.toString(); + } +} diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/ArgumentMethodArgumentResolverTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/ArgumentMethodArgumentResolverTests.java index 53dbac01..e4d32788 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/ArgumentMethodArgumentResolverTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/ArgumentMethodArgumentResolverTests.java @@ -87,15 +87,13 @@ class ArgumentMethodArgumentResolverTests { } @Test - void shouldResolveKotlinBeanArgument() throws Exception { - Method addBook = ClassUtils.getMethod(BookController.class, "ktAddBook", KotlinBookInput.class); - String payload = "{\"bookInput\": { \"name\": \"test name\", \"authorId\": 42} }"; + void shouldResolveDefaultValue() throws Exception { + Method findWithDefault = ClassUtils.getMethod(BookController.class, "findWithDefault", Long.class); + String payload = "{\"name\": \"test\" }"; DataFetchingEnvironment environment = initEnvironment(payload); - MethodParameter methodParameter = getMethodParameter(addBook, 0); + MethodParameter methodParameter = getMethodParameter(findWithDefault, 0); Object result = resolver.resolveArgument(methodParameter, environment); - assertThat(result).isNotNull().isInstanceOf(KotlinBookInput.class); - assertThat((KotlinBookInput) result).hasFieldOrPropertyWithValue("name", "test name") - .hasFieldOrPropertyWithValue("authorId", 42L); + assertThat(result).isNotNull().isInstanceOf(Long.class).isEqualTo(42L); } @Test @@ -110,20 +108,6 @@ class ArgumentMethodArgumentResolverTests { .extracting("name").containsExactly("first", "second"); } - @Test - void shouldResolveNestedJavaBeanArgument() throws Exception { - Method addBookWithNestedAuthor = ClassUtils.getMethod(BookController.class, "addBookWithNestedAuthor", Book.class); - String payload = "{\"book\": { \"name\": \"test name\", \"author\": { \"firstName\": \"Jane\", \"lastName\": \"Spring\"} } }"; - DataFetchingEnvironment environment = initEnvironment(payload); - MethodParameter methodParameter = getMethodParameter(addBookWithNestedAuthor, 0); - Object result = resolver.resolveArgument(methodParameter, environment); - assertThat(result).isNotNull().isInstanceOf(Book.class); - assertThat((Book) result).hasFieldOrPropertyWithValue("name", "test name"); - assertThat(((Book) result).getAuthor()).isNotNull() - .hasFieldOrPropertyWithValue("firstName", "Jane") - .hasFieldOrPropertyWithValue("lastName", "Spring"); - } - private MethodParameter getMethodParameter(Method method, int index) { MethodParameter methodParameter = new MethodParameter(method, index); methodParameter.initParameterNameDiscovery(new DefaultParameterNameDiscoverer()); @@ -136,12 +120,6 @@ class ArgumentMethodArgumentResolverTests { return DataFetchingEnvironmentImpl.newDataFetchingEnvironment().arguments(arguments).build(); } - // BeanWrapper - // ModelAttributeProcessor for constructor based instantiation - - // look at DGS PR for this issue - - @Controller static class BookController { @@ -154,13 +132,13 @@ class ArgumentMethodArgumentResolverTests { return null; } - @MutationMapping - public Book addBook(@Argument BookInput bookInput) { + @QueryMapping + public Book findWithDefault(@Argument(defaultValue = "42") Long id) { return null; } @MutationMapping - public Book ktAddBook(@Argument KotlinBookInput bookInput) { + public Book addBook(@Argument BookInput bookInput) { return null; } @@ -169,10 +147,6 @@ class ArgumentMethodArgumentResolverTests { return null; } - @MutationMapping - public Book addBookWithNestedAuthor(@Argument Book book) { - return null; - } } static class BookInput { diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/GraphQlArgumentInstantiatorTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/GraphQlArgumentInstantiatorTests.java new file mode 100644 index 00000000..25393089 --- /dev/null +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/GraphQlArgumentInstantiatorTests.java @@ -0,0 +1,163 @@ +/* + * Copyright 2020-2021 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.graphql.data.method.annotation.support; + +import java.util.List; +import java.util.Map; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.ObjectMapper; +import graphql.schema.DataFetchingEnvironment; +import graphql.schema.DataFetchingEnvironmentImpl; +import org.junit.jupiter.api.Test; + +import org.springframework.graphql.Book; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Tests for {@link GraphQlArgumentInstantiator} + * + * @author Brian Clozel + */ +class GraphQlArgumentInstantiatorTests { + + private ObjectMapper mapper = new ObjectMapper(); + + private GraphQlArgumentInstantiator instantiator = new GraphQlArgumentInstantiator(); + + @Test + void shouldInstantiateDefaultConstructor() throws Exception { + String payload = "{\"simpleBean\": { \"name\": \"test\"} }"; + DataFetchingEnvironment environment = initEnvironment(payload); + SimpleBean result = instantiator.instantiate(SimpleBean.class, environment.getArgument("simpleBean")); + + assertThat(result).isNotNull().isInstanceOf(SimpleBean.class); + assertThat(result).hasFieldOrPropertyWithValue("name", "test"); + } + + @Test + void shouldInstantiatePrimaryConstructor() throws Exception { + String payload = "{\"constructorBean\": { \"name\": \"test\"} }"; + DataFetchingEnvironment environment = initEnvironment(payload); + ContructorBean result = instantiator.instantiate(ContructorBean.class, environment.getArgument("constructorBean")); + + assertThat(result).isNotNull().isInstanceOf(ContructorBean.class); + assertThat(result).hasFieldOrPropertyWithValue("name", "test"); + } + + @Test + void shouldFailIfNoPrimaryConstructor() throws Exception { + String payload = "{\"noPrimary\": { \"name\": \"test\"} }"; + DataFetchingEnvironment environment = initEnvironment(payload); + assertThatThrownBy(() -> instantiator.instantiate(NoPrimaryConstructor.class, environment.getArgument("noPrimary"))) + .isInstanceOf(IllegalStateException.class).hasMessageContaining("No primary or single public constructor found"); + } + + @Test + void shouldInstantiateNestedBean() throws Exception { + String payload = "{\"book\": { \"name\": \"test name\", \"author\": { \"firstName\": \"Jane\", \"lastName\": \"Spring\"} } }"; + DataFetchingEnvironment environment = initEnvironment(payload); + Book result = instantiator.instantiate(Book.class, environment.getArgument("book")); + + assertThat(result).isNotNull().isInstanceOf(Book.class); + assertThat(result).hasFieldOrPropertyWithValue("name", "test name"); + assertThat(result.getAuthor()).isNotNull() + .hasFieldOrPropertyWithValue("firstName", "Jane") + .hasFieldOrPropertyWithValue("lastName", "Spring"); + } + + @Test + void shouldInstantiateNestedBeanLists() throws Exception { + String payload = "{\"nestedList\": { \"items\": [ {\"name\": \"first\"}, {\"name\": \"second\"}] } }"; + DataFetchingEnvironment environment = initEnvironment(payload); + NestedList result = instantiator.instantiate(NestedList.class, environment.getArgument("nestedList")); + + assertThat(result).isNotNull().isInstanceOf(NestedList.class); + assertThat(result.getItems()).hasSize(2).extracting("name").containsExactly("first", "second"); + } + + private DataFetchingEnvironment initEnvironment(String jsonPayload) throws JsonProcessingException { + Map arguments = this.mapper.readValue(jsonPayload, new TypeReference>() { + }); + return DataFetchingEnvironmentImpl.newDataFetchingEnvironment().arguments(arguments).build(); + } + + static class SimpleBean { + + String name; + + public String getName() { + return this.name; + } + + public void setName(String name) { + this.name = name; + } + } + + static class ContructorBean { + + final String name; + + public ContructorBean(String name) { + this.name = name; + } + + public String getName() { + return this.name; + } + } + + static class NoPrimaryConstructor { + + NoPrimaryConstructor(String name) { + } + + NoPrimaryConstructor(String name, Long id) { + } + } + + static class NestedList { + + List items; + + public List getItems() { + return this.items; + } + + public void setItems(List items) { + this.items = items; + } + } + + static class Item { + + String name; + + public String getName() { + return this.name; + } + + public void setName(String name) { + this.name = name; + } + } + +} \ No newline at end of file diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/KotlinBookInput.kt b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/KotlinBookInput.kt deleted file mode 100644 index 356db4f8..00000000 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/KotlinBookInput.kt +++ /dev/null @@ -1,3 +0,0 @@ -package org.springframework.graphql.data.method.annotation.support; - -data class KotlinBookInput(val name: String, val authorId: Long)