From 9c53c59d23063b7e27b6e07e18d1f1aa6c99e9aa Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Sun, 19 Mar 2023 16:15:16 +0000 Subject: [PATCH] Refactor SortStrategy For a completely generic SortStrategy we can't provide any concrete support we know neither the input structure nor the Object structure. At best, a nearly empty argument resolver, which is not any better than providing your own custom argument resolver. Hence, SortStrategy is now explicitly based on Spring Data's Sort, which enables us to provide concrete support with a base class. See gh-620 --- .../AnnotatedControllerConfigurer.java | 31 ++--- .../support/SortMethodArgumentResolver.java | 14 ++- .../data/query/AbstractSortStrategy.java | 65 +++++++++++ .../{pagination => query}/SortStrategy.java | 18 +-- .../AnnotatedControllerConfigurerTests.java | 27 ++++- .../support/ArgumentResolverTestSupport.java | 4 + .../SortMethodArgumentResolverTests.java | 106 ++++++++++++++++++ .../SubrangeMethodArgumentResolverTests.java | 6 +- 8 files changed, 228 insertions(+), 43 deletions(-) create mode 100644 spring-graphql/src/main/java/org/springframework/graphql/data/query/AbstractSortStrategy.java rename spring-graphql/src/main/java/org/springframework/graphql/data/{pagination => query}/SortStrategy.java (59%) create mode 100644 spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SortMethodArgumentResolverTests.java 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 2c80c90d..14d6575f 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 @@ -48,6 +48,7 @@ import reactor.core.publisher.Mono; import org.springframework.aop.support.AopUtils; import org.springframework.beans.factory.InitializingBean; +import org.springframework.beans.factory.NoSuchBeanDefinitionException; import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationContextAware; import org.springframework.context.expression.BeanFactoryResolver; @@ -57,7 +58,6 @@ import org.springframework.core.MethodParameter; import org.springframework.core.ResolvableType; import org.springframework.core.annotation.AnnotatedElementUtils; import org.springframework.core.convert.ConversionService; -import org.springframework.expression.BeanResolver; import org.springframework.format.FormatterRegistrar; import org.springframework.format.support.DefaultFormattingConversionService; import org.springframework.format.support.FormattingConversionService; @@ -68,7 +68,7 @@ import org.springframework.graphql.data.method.HandlerMethodArgumentResolverComp import org.springframework.graphql.data.method.annotation.BatchMapping; import org.springframework.graphql.data.method.annotation.SchemaMapping; import org.springframework.graphql.data.pagination.CursorStrategy; -import org.springframework.graphql.data.pagination.SortStrategy; +import org.springframework.graphql.data.query.SortStrategy; import org.springframework.graphql.execution.BatchLoaderRegistry; import org.springframework.graphql.execution.DataFetcherExceptionResolver; import org.springframework.graphql.execution.RuntimeWiringConfigurer; @@ -128,9 +128,6 @@ public class AnnotatedControllerConfigurer implements ApplicationContextAware, I @Nullable private CursorStrategy cursorStrategy; - @Nullable - private SortStrategy sortStrategy; - private final List customArgumentResolvers = new ArrayList<>(8); @Nullable @@ -174,16 +171,6 @@ public class AnnotatedControllerConfigurer implements ApplicationContextAware, I this.cursorStrategy = cursorStrategy; } - /** - * Configure a {@link SortStrategy} to extract sort details for pagination - * requests. This results in {@link SortMethodArgumentResolver} being added - * as a method argument resolver. - * @since 1.2 - */ - public void setSortStrategy(SortStrategy sortStrategy) { - this.sortStrategy = sortStrategy; - } - /** * Add a {@link HandlerMethodArgumentResolver} for custom controller method * arguments. Such custom resolvers are ordered after built-in resolvers @@ -286,13 +273,19 @@ public class AnnotatedControllerConfigurer implements ApplicationContextAware, I if (this.cursorStrategy != null) { resolvers.addResolver(createSubrangeMethodArgumentResolver(this.cursorStrategy)); } - if (this.sortStrategy != null) { - resolvers.addResolver(new SortMethodArgumentResolver(this.sortStrategy)); + if (springDataPresent) { + try { + resolvers.addResolver( + new SortMethodArgumentResolver(obtainApplicationContext().getBean(SortStrategy.class))); + } + catch (NoSuchBeanDefinitionException ex) { + // ignore + } } if (springSecurityPresent) { + ApplicationContext context = obtainApplicationContext(); resolvers.addResolver(new PrincipalMethodArgumentResolver()); - BeanResolver beanResolver = new BeanFactoryResolver(obtainApplicationContext()); - resolvers.addResolver(new AuthenticationPrincipalArgumentResolver(beanResolver)); + resolvers.addResolver(new AuthenticationPrincipalArgumentResolver(new BeanFactoryResolver(context))); } if (KotlinDetector.isKotlinPresent()) { resolvers.addResolver(new ContinuationHandlerMethodArgumentResolver()); diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/SortMethodArgumentResolver.java b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/SortMethodArgumentResolver.java index e84e2305..43d6397c 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/SortMethodArgumentResolver.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/SortMethodArgumentResolver.java @@ -20,23 +20,24 @@ package org.springframework.graphql.data.method.annotation.support; import graphql.schema.DataFetchingEnvironment; import org.springframework.core.MethodParameter; +import org.springframework.data.domain.Sort; import org.springframework.graphql.data.method.HandlerMethodArgumentResolver; -import org.springframework.graphql.data.pagination.SortStrategy; +import org.springframework.graphql.data.query.SortStrategy; import org.springframework.util.Assert; /** - * Resolver for a Sort object decoded with {@link SortStrategy}. + * Resolver for method arguments of type {@link Sort}. * * @author Rossen Stoyanchev * @since 1.2 */ public class SortMethodArgumentResolver implements HandlerMethodArgumentResolver { - private final SortStrategy sortStrategy; + private final SortStrategy sortStrategy; - public SortMethodArgumentResolver(SortStrategy sortStrategy) { + public SortMethodArgumentResolver(SortStrategy sortStrategy) { Assert.notNull(sortStrategy, "SortStrategy is required"); this.sortStrategy = sortStrategy; } @@ -44,12 +45,13 @@ public class SortMethodArgumentResolver implements HandlerMethodArgumentResolver @Override public boolean supportsParameter(MethodParameter parameter) { - return this.sortStrategy.supports(parameter.getParameterType()); + return parameter.getParameterType().equals(Sort.class); } @Override public Object resolveArgument(MethodParameter parameter, DataFetchingEnvironment environment) { - return this.sortStrategy.extract(environment); + Sort sort = this.sortStrategy.extract(environment); + return (sort != null ? sort : Sort.unsorted()); } } diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/query/AbstractSortStrategy.java b/spring-graphql/src/main/java/org/springframework/graphql/data/query/AbstractSortStrategy.java new file mode 100644 index 00000000..80b3067b --- /dev/null +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/query/AbstractSortStrategy.java @@ -0,0 +1,65 @@ +/* + * Copyright 2020-2023 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.query; + + +import java.util.ArrayList; +import java.util.List; + +import graphql.schema.DataFetchingEnvironment; + +import org.springframework.data.domain.Sort; +import org.springframework.lang.Nullable; +import org.springframework.util.ObjectUtils; + +/** + * Convenient base class for a {@link SortStrategy}. Subclasses help to extract + * the list of sort {@link #getProperties(DataFetchingEnvironment) properties} + * and {@link #getDirection(DataFetchingEnvironment) direction}. + * + * @author Rossen Stoyanchev + * @since 1.2 + */ +public abstract class AbstractSortStrategy implements SortStrategy { + + @Override + public Sort extract(DataFetchingEnvironment environment) { + List properties = getProperties(environment); + if (!ObjectUtils.isEmpty(properties)) { + Sort.Direction direction = getDirection(environment); + direction = (direction != null ? direction : Sort.DEFAULT_DIRECTION); + List sortOrders = new ArrayList<>(properties.size()); + for (String property : properties) { + sortOrders.add(new Sort.Order(direction, property)); + } + return Sort.by(sortOrders); + } + return null; + } + + /** + * Return the sort properties to use, or an empty list if there are none. + */ + protected abstract List getProperties(DataFetchingEnvironment environment); + + /** + * Return the sort direction to use, or {@code null}. + */ + @Nullable + protected abstract Sort.Direction getDirection(DataFetchingEnvironment environment); + +} diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/pagination/SortStrategy.java b/spring-graphql/src/main/java/org/springframework/graphql/data/query/SortStrategy.java similarity index 59% rename from spring-graphql/src/main/java/org/springframework/graphql/data/pagination/SortStrategy.java rename to spring-graphql/src/main/java/org/springframework/graphql/data/query/SortStrategy.java index 91c90f1f..9f219969 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/pagination/SortStrategy.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/query/SortStrategy.java @@ -14,32 +14,26 @@ * limitations under the License. */ -package org.springframework.graphql.data.pagination; +package org.springframework.graphql.data.query; import graphql.schema.DataFetchingEnvironment; +import org.springframework.data.domain.Sort; import org.springframework.lang.Nullable; /** - * Strategy to extract sort information from GraphQL request arguments. + * Strategy to extract {@link Sort} details from GraphQL arguments. * * @author Rossen Stoyanchev * @since 1.2 */ -public interface SortStrategy { +public interface SortStrategy { /** - * Whether this strategy supports the given Sort object target type. - */ - boolean supports(Class targetType); - - /** - * Return an Object that contains sort order and direction information. - * @param environment the environment to obtain GraphQL request arguments from - * @return the object with sort details, if present + * Return a {@link Sort} instance initialized from GraphQL arguments, or {@code null}. */ @Nullable - S extract(DataFetchingEnvironment environment); + Sort extract(DataFetchingEnvironment environment); } diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurerTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurerTests.java index c1ad7c79..13f8609a 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurerTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurerTests.java @@ -21,6 +21,7 @@ import org.junit.jupiter.api.Test; import org.springframework.context.support.StaticApplicationContext; import org.springframework.graphql.data.method.HandlerMethodArgumentResolver; +import org.springframework.graphql.data.query.SortStrategy; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; @@ -33,7 +34,6 @@ import static org.mockito.Mockito.mock; */ public class AnnotatedControllerConfigurerTests { - @Test void customArgumentResolvers() { HandlerMethodArgumentResolver customResolver1 = mock(HandlerMethodArgumentResolver.class); @@ -52,4 +52,29 @@ public class AnnotatedControllerConfigurerTests { assertThat(resolvers).element(size -3).isSameAs(customResolver1); } + @Test + void sortArgumentResolver() { + SortStrategy sortStrategy = mock(SortStrategy.class); + + StaticApplicationContext context = new StaticApplicationContext(); + context.registerBean(SortStrategy.class, () -> sortStrategy); + + AnnotatedControllerConfigurer configurer = new AnnotatedControllerConfigurer(); + configurer.setApplicationContext(context); + configurer.afterPropertiesSet(); + + List resolvers = configurer.getArgumentResolvers().getResolvers(); + assertThat(resolvers.stream().filter(r -> r instanceof SortMethodArgumentResolver).findFirst()).isPresent(); + } + + @Test + void sortArgumentResolverStrategyNotPresent() { + AnnotatedControllerConfigurer configurer = new AnnotatedControllerConfigurer(); + configurer.setApplicationContext(new StaticApplicationContext()); + configurer.afterPropertiesSet(); + + List resolvers = configurer.getArgumentResolvers().getResolvers(); + assertThat(resolvers.stream().filter(r -> r instanceof SortMethodArgumentResolver).findFirst()).isNotPresent(); + } + } diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/ArgumentResolverTestSupport.java b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/ArgumentResolverTestSupport.java index 8ad3b76b..61328865 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/ArgumentResolverTestSupport.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/ArgumentResolverTestSupport.java @@ -57,4 +57,8 @@ class ArgumentResolverTestSupport { return DataFetchingEnvironmentImpl.newDataFetchingEnvironment().arguments(arguments).build(); } + protected DataFetchingEnvironment environment(Map arguments) { + return DataFetchingEnvironmentImpl.newDataFetchingEnvironment().arguments(arguments).build(); + } + } \ No newline at end of file diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SortMethodArgumentResolverTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SortMethodArgumentResolverTests.java new file mode 100644 index 00000000..bab6bf57 --- /dev/null +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SortMethodArgumentResolverTests.java @@ -0,0 +1,106 @@ +/* + * Copyright 2002-2023 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.stream.Collectors; + +import graphql.schema.DataFetchingEnvironment; +import org.junit.jupiter.api.Test; + +import org.springframework.core.MethodParameter; +import org.springframework.data.domain.Sort; +import org.springframework.graphql.Book; +import org.springframework.graphql.BookCriteria; +import org.springframework.graphql.data.method.annotation.QueryMapping; +import org.springframework.graphql.data.query.AbstractSortStrategy; +import org.springframework.graphql.data.query.SortStrategy; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Unit tests for {@link SortMethodArgumentResolver}. + * + * @author Rossen Stoyanchev + */ +public class SortMethodArgumentResolverTests extends ArgumentResolverTestSupport { + + private final MethodParameter param = methodParam(BookController.class, "getBooks", Sort.class); + + + @Test + void supports() { + SortMethodArgumentResolver resolver = resolver(new SimpleSortStrategy()); + assertThat(resolver.supportsParameter(this.param)).isTrue(); + + MethodParameter param = methodParam(BookController.class, "getBooksByCriteria", BookCriteria.class); + assertThat(resolver.supportsParameter(param)).isFalse(); + } + + @Test + void resolve() throws Exception { + DataFetchingEnvironment environment = environment(""" + { "sortFields": ["firstName", "lastName", "id"], "sortDirection": "DESC"}" + """); + + Sort sort = (Sort) resolver(new SimpleSortStrategy()).resolveArgument(param, environment); + + assertThat(sort.stream().collect(Collectors.toList())) + .hasSize(3) + .containsExactly( + new Sort.Order(Sort.Direction.DESC, "firstName"), + new Sort.Order(Sort.Direction.DESC, "lastName"), + new Sort.Order(Sort.Direction.DESC, "id")); + } + + private SortMethodArgumentResolver resolver(SortStrategy sortStrategy) { + return new SortMethodArgumentResolver(sortStrategy); + } + + + @SuppressWarnings({"DataFlowIssue", "unused"}) + private static class BookController { + + @QueryMapping + public List getBooks(Sort sort) { + return null; + } + + @QueryMapping + public List getBooksByCriteria(BookCriteria criteria) { + return null; + } + + } + + + private static class SimpleSortStrategy extends AbstractSortStrategy { + + @Override + protected List getProperties(DataFetchingEnvironment environment) { + return environment.getArgument("sortFields"); + } + + @Override + protected Sort.Direction getDirection(DataFetchingEnvironment environment) { + return (environment.containsArgument("sortDirection") ? + Sort.Direction.valueOf(environment.getArgument("sortDirection")) : null); + } + + } + +} diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolverTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolverTests.java index c71bcf3c..656cb13d 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolverTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SubrangeMethodArgumentResolverTests.java @@ -80,12 +80,8 @@ public class SubrangeMethodArgumentResolverTests extends ArgumentResolverTestSup assertThat(subrange.forward()).isEqualTo(forward); } - private static DataFetchingEnvironment environment(Map arguments) { - return DataFetchingEnvironmentImpl.newDataFetchingEnvironment().arguments(arguments).build(); - } - - @SuppressWarnings("unused") + @SuppressWarnings({"unused", "DataFlowIssue"}) @Controller private static class BookController {