From 664e661783a78d52c82af8663f2f2115222c828b Mon Sep 17 00:00:00 2001 From: Oliver Drotbohm Date: Tue, 10 Dec 2019 14:52:06 +0100 Subject: [PATCH] DATACMNS-1593, DATACMNS-1635 - Overhauled null handling in QuerydslPredicateArgumentResolver. QuerydslPredicateArgumentResolver now properly handles predicate lookups that result in null values. The semantics of a handler method parameter of type of Querydsl's Predicate have been tightened to always see a non-null Predicate by default. Users that want to handle the absence of predicates explicitly can opt into seeing null by annotating the parameter with @Nullable or use Optional. QuerydslPredicateBuilder now consistently returns null in case the original parameter map is entirely empty or consists of only keys with empty value arrays as empty form submissions do. This partially reverts the work of DATACMNS-1168, which moved into the direction of returning a default Predicate value for empty maps in the first place. That however prevents us from producing empty Optionals. Related tickets: DATACMNS-1168. --- .../binding/QuerydslPredicateBuilder.java | 2 +- .../QuerydslPredicateArgumentResolver.java | 33 +++++++-- .../QuerydslPredicateBuilderUnitTests.java | 8 +-- ...dslPredicateArgumentResolverUnitTests.java | 69 +++++++++++++++---- 4 files changed, 86 insertions(+), 26 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 2ceda2ac3..7116ffb0a 100644 --- a/src/main/java/org/springframework/data/querydsl/binding/QuerydslPredicateBuilder.java +++ b/src/main/java/org/springframework/data/querydsl/binding/QuerydslPredicateBuilder.java @@ -91,7 +91,7 @@ public class QuerydslPredicateBuilder { BooleanBuilder builder = new BooleanBuilder(); if (values.isEmpty()) { - return builder; + return builder.getValue(); } for (Entry> entry : values.entrySet()) { 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 f6e6c8ff8..3c72892df 100644 --- a/src/main/java/org/springframework/data/web/querydsl/QuerydslPredicateArgumentResolver.java +++ b/src/main/java/org/springframework/data/web/querydsl/QuerydslPredicateArgumentResolver.java @@ -21,9 +21,11 @@ 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.QuerydslBinderCustomizer; +import org.springframework.data.querydsl.binding.QuerydslBindings; import org.springframework.data.querydsl.binding.QuerydslBindingsFactory; import org.springframework.data.querydsl.binding.QuerydslPredicate; import org.springframework.data.querydsl.binding.QuerydslPredicateBuilder; @@ -38,6 +40,7 @@ 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; /** @@ -50,6 +53,10 @@ import com.querydsl.core.types.Predicate; */ public class QuerydslPredicateArgumentResolver implements HandlerMethodArgumentResolver { + private static final ResolvableType PREDICATE = ResolvableType.forClass(Predicate.class); + private static final ResolvableType OPTIONAL_OF_PREDICATE = ResolvableType.forClassWithGenerics(Optional.class, + PREDICATE); + private final QuerydslBindingsFactory bindingsFactory; private final QuerydslPredicateBuilder predicateBuilder; @@ -74,7 +81,9 @@ public class QuerydslPredicateArgumentResolver implements HandlerMethodArgumentR @Override public boolean supportsParameter(MethodParameter parameter) { - if (Predicate.class.equals(parameter.getParameterType())) { + ResolvableType type = ResolvableType.forMethodParameter(parameter); + + if (PREDICATE.isAssignableFrom(type) || OPTIONAL_OF_PREDICATE.isAssignableFrom(type)) { return true; } @@ -92,7 +101,7 @@ public class QuerydslPredicateArgumentResolver implements HandlerMethodArgumentR */ @Nullable @Override - public Predicate resolveArgument(MethodParameter parameter, @Nullable ModelAndViewContainer mavContainer, + public Object resolveArgument(MethodParameter parameter, @Nullable ModelAndViewContainer mavContainer, NativeWebRequest webRequest, @Nullable WebDataBinderFactory binderFactory) throws Exception { MultiValueMap parameters = new LinkedMultiValueMap<>(); @@ -105,13 +114,23 @@ public class QuerydslPredicateArgumentResolver implements HandlerMethodArgumentR .ofNullable(parameter.getParameterAnnotation(QuerydslPredicate.class)); TypeInformation domainType = extractTypeInfo(parameter).getRequiredActualType(); - Optional>> bindings = annotation// - .map(QuerydslPredicate::bindings)// + Optional>> bindingsAnnotation = annotation // + .map(QuerydslPredicate::bindings) // .map(CastUtils::cast); - return predicateBuilder.getPredicate(domainType, parameters, - bindings.map(it -> bindingsFactory.createBindingsFor(domainType, it)) - .orElseGet(() -> bindingsFactory.createBindingsFor(domainType))); + QuerydslBindings bindings = bindingsAnnotation // + .map(it -> bindingsFactory.createBindingsFor(domainType, it)) // + .orElseGet(() -> bindingsFactory.createBindingsFor(domainType)); + + Predicate result = predicateBuilder.getPredicate(domainType, parameters, bindings); + + if (!parameter.isOptional() && result == null) { + return new BooleanBuilder(); + } + + return OPTIONAL_OF_PREDICATE.isAssignableFrom(ResolvableType.forMethodParameter(parameter)) // + ? Optional.ofNullable(result) // + : result; } /** 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 7339aa04a..c94df53ab 100755 --- a/src/test/java/org/springframework/data/querydsl/binding/QuerydslPredicateBuilderUnitTests.java +++ b/src/test/java/org/springframework/data/querydsl/binding/QuerydslPredicateBuilderUnitTests.java @@ -38,7 +38,6 @@ import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; import com.querydsl.collections.CollQueryFactory; -import com.querydsl.core.BooleanBuilder; import com.querydsl.core.types.Constant; import com.querydsl.core.types.Predicate; @@ -75,9 +74,8 @@ public class QuerydslPredicateBuilderUnitTests { } @Test // DATACMNS-669, DATACMNS-1168 - public void getPredicateShouldReturnEmptyPredicateWhenPropertiesAreEmpty() { - assertThat(builder.getPredicate(ClassTypeInformation.OBJECT, values, DEFAULT_BINDINGS)) - .isEqualTo(new BooleanBuilder()); + public void getPredicateShouldReturnNullWhenPropertiesAreEmpty() { + assertThat(builder.getPredicate(ClassTypeInformation.OBJECT, values, DEFAULT_BINDINGS)).isNull(); } @Test // DATACMNS-669 @@ -87,7 +85,7 @@ public class QuerydslPredicateBuilderUnitTests { Predicate predicate = builder.getPredicate(USER_TYPE, values, DEFAULT_BINDINGS); - assertThat(predicate).isEqualTo((Predicate) QUser.user.firstname.eq("Oliver")); + assertThat(predicate).isEqualTo(QUser.user.firstname.eq("Oliver")); List result = CollQueryFactory.from(QUser.user, Users.USERS).where(predicate).fetchResults().getResults(); diff --git a/src/test/java/org/springframework/data/web/querydsl/QuerydslPredicateArgumentResolverUnitTests.java b/src/test/java/org/springframework/data/web/querydsl/QuerydslPredicateArgumentResolverUnitTests.java index ef53e4188..15154178b 100755 --- a/src/test/java/org/springframework/data/web/querydsl/QuerydslPredicateArgumentResolverUnitTests.java +++ b/src/test/java/org/springframework/data/web/querydsl/QuerydslPredicateArgumentResolverUnitTests.java @@ -22,7 +22,6 @@ import java.util.Optional; import org.junit.Before; import org.junit.Test; - import org.springframework.core.MethodParameter; import org.springframework.data.domain.Page; import org.springframework.data.domain.Pageable; @@ -38,6 +37,7 @@ import org.springframework.data.util.TypeInformation; import org.springframework.hateoas.EntityModel; import org.springframework.http.HttpEntity; import org.springframework.http.ResponseEntity; +import org.springframework.lang.Nullable; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.test.util.ReflectionTestUtils; import org.springframework.web.context.request.ServletWebRequest; @@ -94,7 +94,7 @@ public class QuerydslPredicateArgumentResolverUnitTests { request.addParameter("firstname", "rand"); - Predicate predicate = resolver.resolveArgument(getMethodParameterFor("simpleFind", Predicate.class), null, + Object predicate = resolver.resolveArgument(getMethodParameterFor("simpleFind", Predicate.class), null, new ServletWebRequest(request), null); assertThat(predicate).isEqualTo(QUser.user.firstname.eq("rand")); @@ -106,7 +106,7 @@ public class QuerydslPredicateArgumentResolverUnitTests { request.addParameter("firstname", "rand"); request.addParameter("lastname", "al'thor"); - Predicate predicate = resolver.resolveArgument(getMethodParameterFor("simpleFind", Predicate.class), null, + Object predicate = resolver.resolveArgument(getMethodParameterFor("simpleFind", Predicate.class), null, new ServletWebRequest(request), null); assertThat(predicate).isEqualTo(QUser.user.firstname.eq("rand").and(QUser.user.lastname.eq("al'thor"))); @@ -117,12 +117,12 @@ public class QuerydslPredicateArgumentResolverUnitTests { request.addParameter("address.city", "two rivers"); - Predicate predicate = resolver.resolveArgument(getMethodParameterFor("simpleFind", Predicate.class), null, + Object predicate = resolver.resolveArgument(getMethodParameterFor("simpleFind", Predicate.class), null, new ServletWebRequest(request), null); BooleanExpression eq = QUser.user.address.city.eq("two rivers"); - assertThat(predicate).isEqualTo((Predicate) eq); + assertThat(predicate).isEqualTo(eq); } @Test // DATACMNS-669 @@ -130,10 +130,10 @@ public class QuerydslPredicateArgumentResolverUnitTests { request.addParameter("address.city", "tar valon"); - Predicate predicate = resolver.resolveArgument(getMethodParameterFor("pagedFind", Predicate.class, Pageable.class), + Object predicate = resolver.resolveArgument(getMethodParameterFor("pagedFind", Predicate.class, Pageable.class), null, new ServletWebRequest(request), null); - assertThat(predicate).isEqualTo((Predicate) QUser.user.address.city.eq("tar valon")); + assertThat(predicate).isEqualTo(QUser.user.address.city.eq("tar valon")); } @Test // DATACMNS-669 @@ -142,7 +142,7 @@ public class QuerydslPredicateArgumentResolverUnitTests { request.addParameter("firstname", "egwene"); request.addParameter("lastname", "al'vere"); - Predicate predicate = resolver.resolveArgument(getMethodParameterFor("specificFind", Predicate.class), null, + Object predicate = resolver.resolveArgument(getMethodParameterFor("specificFind", Predicate.class), null, new ServletWebRequest(request), null); assertThat(predicate).isEqualTo( @@ -154,10 +154,10 @@ public class QuerydslPredicateArgumentResolverUnitTests { request.addParameter("inceptionYear", "978"); - Predicate predicate = resolver.resolveArgument(getMethodParameterFor("specificFind", Predicate.class), null, + Object predicate = resolver.resolveArgument(getMethodParameterFor("specificFind", Predicate.class), null, new ServletWebRequest(request), null); - assertThat(predicate).isEqualTo((Predicate) QUser.user.inceptionYear.eq(978L)); + assertThat(predicate).isEqualTo(QUser.user.inceptionYear.eq(978L)); } @Test // DATACMNS-669 @@ -165,10 +165,10 @@ public class QuerydslPredicateArgumentResolverUnitTests { request.addParameter("inceptionYear", new String[] { "978", "998" }); - Predicate predicate = resolver.resolveArgument(getMethodParameterFor("specificFind", Predicate.class), null, + Object predicate = resolver.resolveArgument(getMethodParameterFor("specificFind", Predicate.class), null, new ServletWebRequest(request), null); - assertThat(predicate).isEqualTo((Predicate) QUser.user.inceptionYear.in(978L, 998L)); + assertThat(predicate).isEqualTo(QUser.user.inceptionYear.in(978L, 998L)); } @Test // DATACMNS-669 @@ -190,7 +190,7 @@ public class QuerydslPredicateArgumentResolverUnitTests { TypeInformation type = ReflectionTestUtils.invokeMethod(resolver, "extractTypeInfo", getMethodParameterFor("predicateWithoutAnnotation", Predicate.class)); - assertThat(type).isEqualTo((TypeInformation) ClassTypeInformation.from(User.class)); + assertThat(type).isEqualTo(ClassTypeInformation.from(User.class)); } @Test // DATACMNS-669 @@ -205,6 +205,43 @@ public class QuerydslPredicateArgumentResolverUnitTests { assertThat(extractTypeInfo(getMethodParameterFor("forModelAndView"))).isEqualTo(MODELA_AND_VIEW_TYPE); } + @Test // DATACMNS-1593 + public void returnsEmptyPredicateForEmptyInput() throws Exception { + + MethodParameter parameter = getMethodParameterFor("predicateWithoutAnnotation", Predicate.class); + + request.addParameter("firstname", ""); + + assertThat(resolver.resolveArgument(parameter, null, new ServletWebRequest(request), null)) // + .isNotNull(); + } + + @Test // DATACMNS-1635 + public void forwardsNullValueForNullablePredicate() throws Exception { + + MethodParameter parameter = getMethodParameterFor("nullablePredicateWithoutAnnotation", Predicate.class); + + request.addParameter("firstname", ""); + + assertThat(resolver.resolveArgument(parameter, null, new ServletWebRequest(request), null)).isNull(); + } + + @Test // DATACMNS-1635 + public void returnsOptionalIfDeclared() throws Exception { + + MethodParameter parameter = getMethodParameterFor("optionalPredicateWithoutAnnotation", Optional.class); + + request.addParameter("firstname", ""); + + assertThat(resolver.resolveArgument(parameter, null, new ServletWebRequest(request), null)) // + .isInstanceOfSatisfying(Optional.class, it -> assertThat(it).isEmpty()); + + request.addParameter("lastname", "Matthews"); + + assertThat(resolver.resolveArgument(parameter, null, new ServletWebRequest(request), null)) // + .isInstanceOfSatisfying(Optional.class, it -> assertThat(it).isPresent()); + } + private static MethodParameter getMethodParameterFor(String methodName, Class... args) throws RuntimeException { try { @@ -244,6 +281,12 @@ public class QuerydslPredicateArgumentResolverUnitTests { ModelAndView forModelAndView(); ResponseEntity> forResourceOfUser(); + + // Nullability + + User nullablePredicateWithoutAnnotation(@Nullable Predicate predicate); + + User optionalPredicateWithoutAnnotation(Optional predicate); } public static class SampleRepo implements QuerydslBinderCustomizer {