From 394187a03bd36c89a06294bc263fc442e69b10fa Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Tue, 29 Jun 2021 10:12:34 +0200 Subject: [PATCH] Revise QuerydslPredicateBuilder nullability behavior. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We now no longer return a null Predicate from QuerydslPredicateBuilder.getPredicate(…) if the input values are empty or if the constraints are empty. Instead, we return an empty BooleanBuilder instance to avoid null handling on the calling side. HandlerMethodArgumentsResolvers for QuerydslBindings retain their null/Optional.empty semantics. Closes #2396 --- .../binding/QuerydslPredicateBuilder.java | 32 ++++++++++++++++--- .../QuerydslPredicateArgumentResolver.java | 10 +----- ...rydslPredicateArgumentResolverSupport.java | 15 ++++++++- ...tiveQuerydslPredicateArgumentResolver.java | 14 +++----- .../QuerydslPredicateBuilderUnitTests.java | 8 +++-- 5 files changed, 51 insertions(+), 28 deletions(-) diff --git a/src/main/java/org/springframework/data/querydsl/binding/QuerydslPredicateBuilder.java b/src/main/java/org/springframework/data/querydsl/binding/QuerydslPredicateBuilder.java index 9392f4031..2c0182e28 100644 --- a/src/main/java/org/springframework/data/querydsl/binding/QuerydslPredicateBuilder.java +++ b/src/main/java/org/springframework/data/querydsl/binding/QuerydslPredicateBuilder.java @@ -32,7 +32,6 @@ import org.springframework.core.convert.TypeDescriptor; import org.springframework.data.mapping.PropertyPath; import org.springframework.data.querydsl.EntityPathResolver; import org.springframework.data.util.TypeInformation; -import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.MultiValueMap; import org.springframework.util.StringUtils; @@ -80,9 +79,8 @@ public class QuerydslPredicateBuilder { * @param type the type to create a predicate for. * @param values the values to bind. * @param bindings the {@link QuerydslBindings} for the predicate. - * @return + * @return the {@link Predicate}. */ - @Nullable public Predicate getPredicate(TypeInformation type, MultiValueMap values, QuerydslBindings bindings) { @@ -91,7 +89,7 @@ public class QuerydslPredicateBuilder { BooleanBuilder builder = new BooleanBuilder(); if (values.isEmpty()) { - return builder.getValue(); + return getPredicate(builder); } for (Entry> entry : values.entrySet()) { @@ -118,7 +116,19 @@ public class QuerydslPredicateBuilder { predicate.ifPresent(builder::and); } - return builder.getValue(); + return getPredicate(builder); + } + + /** + * Returns whether the given {@link Predicate} represents an empty predicate instance. + * + * @param predicate + * @return + * @since 2.5.3 + * @see BooleanBuilder + */ + public static boolean isEmpty(Predicate predicate) { + return new BooleanBuilder().equals(predicate); } /** @@ -219,4 +229,16 @@ public class QuerydslPredicateBuilder { private static boolean isSingleElementCollectionWithoutText(List source) { return source.size() == 1 && !StringUtils.hasLength(source.get(0)); } + + /** + * Returns the {@link Predicate} from {@link BooleanBuilder}. + * + * @param builder + * @return + */ + private static Predicate getPredicate(BooleanBuilder builder) { + + Predicate predicate = builder.getValue(); + return predicate == null ? new BooleanBuilder() : predicate; + } } diff --git a/src/main/java/org/springframework/data/web/querydsl/QuerydslPredicateArgumentResolver.java b/src/main/java/org/springframework/data/web/querydsl/QuerydslPredicateArgumentResolver.java index 735746156..f9e7a5772 100644 --- a/src/main/java/org/springframework/data/web/querydsl/QuerydslPredicateArgumentResolver.java +++ b/src/main/java/org/springframework/data/web/querydsl/QuerydslPredicateArgumentResolver.java @@ -21,7 +21,6 @@ import java.util.Map.Entry; import java.util.Optional; import org.springframework.core.MethodParameter; -import org.springframework.core.ResolvableType; import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.support.DefaultConversionService; import org.springframework.data.querydsl.binding.QuerydslBindingsFactory; @@ -33,7 +32,6 @@ import org.springframework.web.context.request.NativeWebRequest; import org.springframework.web.method.support.HandlerMethodArgumentResolver; import org.springframework.web.method.support.ModelAndViewContainer; -import com.querydsl.core.BooleanBuilder; import com.querydsl.core.types.Predicate; /** @@ -84,13 +82,7 @@ public class QuerydslPredicateArgumentResolver extends QuerydslPredicateArgument MultiValueMap queryParameters = getQueryParameters(webRequest); Predicate result = getPredicate(parameter, queryParameters); - if (!parameter.isOptional() && result == null) { - return new BooleanBuilder(); - } - - return OPTIONAL_OF_PREDICATE.isAssignableFrom(ResolvableType.forMethodParameter(parameter)) // - ? Optional.ofNullable(result) // - : result; + return potentiallyConvertMethodParameterValue(parameter, result); } private static MultiValueMap getQueryParameters(NativeWebRequest webRequest) { diff --git a/src/main/java/org/springframework/data/web/querydsl/QuerydslPredicateArgumentResolverSupport.java b/src/main/java/org/springframework/data/web/querydsl/QuerydslPredicateArgumentResolverSupport.java index 1d804f4b7..997c1e90b 100644 --- a/src/main/java/org/springframework/data/web/querydsl/QuerydslPredicateArgumentResolverSupport.java +++ b/src/main/java/org/springframework/data/web/querydsl/QuerydslPredicateArgumentResolverSupport.java @@ -94,7 +94,6 @@ public abstract class QuerydslPredicateArgumentResolverSupport { return false; } - @Nullable Predicate getPredicate(MethodParameter parameter, MultiValueMap queryParameters) { MergedAnnotations annotations = MergedAnnotations.from(parameter.getParameter()); @@ -112,6 +111,20 @@ public abstract class QuerydslPredicateArgumentResolverSupport { return predicateBuilder.getPredicate(domainType, queryParameters, bindings); } + @Nullable + static Object potentiallyConvertMethodParameterValue(MethodParameter parameter, Predicate predicate) { + + if (!parameter.isOptional()) { + return predicate; + } + + if (OPTIONAL_OF_PREDICATE.isAssignableFrom(ResolvableType.forMethodParameter(parameter))) { + return QuerydslPredicateBuilder.isEmpty(predicate) ? Optional.empty() : Optional.of(predicate); + } + + return QuerydslPredicateBuilder.isEmpty(predicate) ? null : predicate; + } + /** * Obtains the domain type information from the given method parameter. Will favor an explicitly registered on through * {@link QuerydslPredicate#root()} but use the actual type of the method's return type as fallback. diff --git a/src/main/java/org/springframework/data/web/querydsl/ReactiveQuerydslPredicateArgumentResolver.java b/src/main/java/org/springframework/data/web/querydsl/ReactiveQuerydslPredicateArgumentResolver.java index b07204520..dd451a234 100644 --- a/src/main/java/org/springframework/data/web/querydsl/ReactiveQuerydslPredicateArgumentResolver.java +++ b/src/main/java/org/springframework/data/web/querydsl/ReactiveQuerydslPredicateArgumentResolver.java @@ -17,12 +17,11 @@ package org.springframework.data.web.querydsl; import java.util.List; import java.util.Map.Entry; -import java.util.Optional; import org.springframework.core.MethodParameter; -import org.springframework.core.ResolvableType; import org.springframework.core.convert.ConversionService; import org.springframework.data.querydsl.binding.QuerydslBindingsFactory; +import org.springframework.lang.Nullable; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; import org.springframework.web.reactive.BindingContext; @@ -30,7 +29,6 @@ import org.springframework.web.reactive.result.method.HandlerMethodArgumentResol import org.springframework.web.reactive.result.method.SyncHandlerMethodArgumentResolver; import org.springframework.web.server.ServerWebExchange; -import com.querydsl.core.BooleanBuilder; import com.querydsl.core.types.Predicate; /** @@ -54,21 +52,17 @@ public class ReactiveQuerydslPredicateArgumentResolver extends QuerydslPredicate * @see org.springframework.web.reactive.result.method.SyncHandlerMethodArgumentResolver(org.springframework.core.MethodParameter, org.springframework.web.reactive.BindingContext, org.springframework.web.server.ServerWebExchange) */ @Override + @Nullable public Object resolveArgumentValue(MethodParameter parameter, BindingContext bindingContext, ServerWebExchange exchange) { MultiValueMap queryParameters = getQueryParameters(exchange); Predicate result = getPredicate(parameter, queryParameters); - if (!parameter.isOptional() && result == null) { - return new BooleanBuilder(); - } - - return OPTIONAL_OF_PREDICATE.isAssignableFrom(ResolvableType.forMethodParameter(parameter)) // - ? Optional.ofNullable(result) // - : result; + return potentiallyConvertMethodParameterValue(parameter, result); } + private static MultiValueMap getQueryParameters(ServerWebExchange exchange) { MultiValueMap queryParams = exchange.getRequest().getQueryParams(); diff --git a/src/test/java/org/springframework/data/querydsl/binding/QuerydslPredicateBuilderUnitTests.java b/src/test/java/org/springframework/data/querydsl/binding/QuerydslPredicateBuilderUnitTests.java index e5b081963..b8132fbed 100755 --- a/src/test/java/org/springframework/data/querydsl/binding/QuerydslPredicateBuilderUnitTests.java +++ b/src/test/java/org/springframework/data/querydsl/binding/QuerydslPredicateBuilderUnitTests.java @@ -77,8 +77,10 @@ class QuerydslPredicateBuilderUnitTests { } @Test // DATACMNS-669, DATACMNS-1168 - void getPredicateShouldReturnNullWhenPropertiesAreEmpty() { - assertThat(builder.getPredicate(ClassTypeInformation.OBJECT, values, DEFAULT_BINDINGS)).isNull(); + void getPredicateShouldReturnEmptyWhenPropertiesAreEmpty() { + assertThat( + QuerydslPredicateBuilder.isEmpty(builder.getPredicate(ClassTypeInformation.OBJECT, values, DEFAULT_BINDINGS))) + .isTrue(); } @Test // DATACMNS-669 @@ -228,6 +230,6 @@ class QuerydslPredicateBuilderUnitTests { values.add("firstname", ""); - assertThat(builder.getPredicate(USER_TYPE, values, DEFAULT_BINDINGS)).isNull(); + assertThat(QuerydslPredicateBuilder.isEmpty(builder.getPredicate(USER_TYPE, values, DEFAULT_BINDINGS))).isTrue(); } }