From bb3898add3c624f9e5935bdef24e95fd9bdc5530 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Thu, 29 Jul 2021 11:46:31 +0200 Subject: [PATCH] Consider property path root in `QuerydslBindings`. We now consider a qualified property path when registering and querying `QuerydslBindings` to ensure that paths matching various domain types do not accidentally get applied for a different type than they were registered for. Previously, a binding for `Address.description` would be also applied to `User.description` as only the property path `description` was considered when looking up bindings. Closes: #2418 Original Pull Request: #2422 --- .../querydsl/binding/PathInformation.java | 8 +++ .../binding/PropertyPathInformation.java | 21 ++++++-- .../querydsl/binding/QuerydslBindings.java | 42 +++++++++++++-- .../binding/QuerydslPathInformation.java | 31 +++++++---- .../data/querydsl/Address.java | 1 + .../springframework/data/querydsl/User.java | 5 +- .../data/querydsl/UserWrapper.java | 23 ++++++++ .../PropertyPathInformationUnitTests.java | 53 +++++++++++++++++++ .../binding/QuerydslBindingsUnitTests.java | 25 ++++++++- .../QuerydslPredicateBuilderUnitTests.java | 28 ++++++++-- 10 files changed, 211 insertions(+), 26 deletions(-) create mode 100644 src/test/java/org/springframework/data/querydsl/UserWrapper.java create mode 100644 src/test/java/org/springframework/data/querydsl/binding/PropertyPathInformationUnitTests.java diff --git a/src/main/java/org/springframework/data/querydsl/binding/PathInformation.java b/src/main/java/org/springframework/data/querydsl/binding/PathInformation.java index 32e640065..3a328a3b2 100644 --- a/src/main/java/org/springframework/data/querydsl/binding/PathInformation.java +++ b/src/main/java/org/springframework/data/querydsl/binding/PathInformation.java @@ -31,6 +31,14 @@ import com.querydsl.core.types.Path; */ interface PathInformation { + /** + * The root property owner type. + * + * @return + * @since 2.5.5 + */ + Class getRootParentType(); + /** * The type of the leaf property. * diff --git a/src/main/java/org/springframework/data/querydsl/binding/PropertyPathInformation.java b/src/main/java/org/springframework/data/querydsl/binding/PropertyPathInformation.java index df50fdc90..2d74c8720 100644 --- a/src/main/java/org/springframework/data/querydsl/binding/PropertyPathInformation.java +++ b/src/main/java/org/springframework/data/querydsl/binding/PropertyPathInformation.java @@ -35,6 +35,7 @@ import com.querydsl.core.types.dsl.CollectionPathBase; * * @author Oliver Gierke * @author Christoph Strobl + * @author Mark Paluch * @since 1.13 */ class PropertyPathInformation implements PathInformation { @@ -71,6 +72,15 @@ class PropertyPathInformation implements PathInformation { return new PropertyPathInformation(path); } + /* + * (non-Javadoc) + * @see org.springframework.data.querydsl.binding.PathInformation#getRootParentType() + */ + @Override + public Class getRootParentType() { + return path.getOwningType().getType(); + } + /* * (non-Javadoc) * @see org.springframework.data.querydsl.binding.PathInformation#getLeafType() @@ -162,12 +172,13 @@ class PropertyPathInformation implements PathInformation { return true; } - if (!(o instanceof PropertyPathInformation)) { + if (!(o instanceof PathInformation)) { return false; } - PropertyPathInformation that = (PropertyPathInformation) o; - return ObjectUtils.nullSafeEquals(path, that.path); + PathInformation that = (PathInformation) o; + return ObjectUtils.nullSafeEquals(getRootParentType(), that.getRootParentType()) + && ObjectUtils.nullSafeEquals(toDotPath(), that.toDotPath()); } /* @@ -176,7 +187,9 @@ class PropertyPathInformation implements PathInformation { */ @Override public int hashCode() { - return ObjectUtils.nullSafeHashCode(path); + int result = ObjectUtils.nullSafeHashCode(getRootParentType()); + result = 31 * result + ObjectUtils.nullSafeHashCode(toDotPath()); + return result; } /* diff --git a/src/main/java/org/springframework/data/querydsl/binding/QuerydslBindings.java b/src/main/java/org/springframework/data/querydsl/binding/QuerydslBindings.java index 61ffc0cdb..670fea967 100644 --- a/src/main/java/org/springframework/data/querydsl/binding/QuerydslBindings.java +++ b/src/main/java/org/springframework/data/querydsl/binding/QuerydslBindings.java @@ -65,6 +65,7 @@ import com.querydsl.core.types.Path; */ public class QuerydslBindings { + // pathSpecs key format: . private final Map> pathSpecs; private final Map, PathAndBinding> typeSpecs; private final Set allowList; @@ -203,7 +204,7 @@ public class QuerydslBindings { Assert.notNull(path, "PropertyPath must not be null!"); - PathAndBinding pathAndBinding = (PathAndBinding) pathSpecs.get(path.toDotPath()); + PathAndBinding pathAndBinding = (PathAndBinding) pathSpecs.get(createKey(path)); if (pathAndBinding != null) { @@ -229,7 +230,7 @@ public class QuerydslBindings { Assert.notNull(path, "PropertyPath must not be null!"); - return Optional.ofNullable(pathSpecs.get(path.toDotPath())).flatMap(PathAndBinding::getPath); + return Optional.ofNullable(pathSpecs.get(createKey(path))).flatMap(PathAndBinding::getPath); } /** @@ -249,6 +250,15 @@ public class QuerydslBindings { return null; } + // fully-qualified path lookup + String key = createKey(type, path); + if (pathSpecs.containsKey(key)) { + return pathSpecs.get(key).getPath()// + .map(QuerydslPathInformation::of)// + .orElse(null); + } + + // alias lookup if (pathSpecs.containsKey(path)) { return pathSpecs.get(path).getPath()// .map(QuerydslPathInformation::of)// @@ -263,6 +273,28 @@ public class QuerydslBindings { } } + /** + * Returns the property path key for the given {@link Path}. + * + * @param path can be {@literal null}. + * @return + */ + private static String createKey(Optional> path) { + return path.map(QuerydslPathInformation::of).map(QuerydslBindings::createKey).orElse(""); + } + + private static String createKey(PathInformation path) { + return createKey(path.getRootParentType(), path.toDotPath()); + } + + private static String createKey(TypeInformation type, String path) { + return createKey(type.getType(), path); + } + + private static String createKey(Class type, String path) { + return type.getSimpleName() + "." + path; + } + /** * Checks if a given {@link PropertyPath} should be visible for binding values. * @@ -385,7 +417,7 @@ public class QuerydslBindings { } protected void registerBinding(PathAndBinding binding) { - QuerydslBindings.this.pathSpecs.put(toDotPath(binding.getPath()), binding); + QuerydslBindings.this.pathSpecs.put(createKey(binding.getPath()), binding); } } @@ -455,10 +487,12 @@ public class QuerydslBindings { super.registerBinding(binding); + String dotPath = toDotPath(binding.getPath()); + if (alias != null) { QuerydslBindings.this.pathSpecs.put(alias, binding); QuerydslBindings.this.aliases.add(alias); - QuerydslBindings.this.denyList.add(toDotPath(binding.getPath())); + QuerydslBindings.this.denyList.add(dotPath); } } } diff --git a/src/main/java/org/springframework/data/querydsl/binding/QuerydslPathInformation.java b/src/main/java/org/springframework/data/querydsl/binding/QuerydslPathInformation.java index d89776897..67a0f2da4 100644 --- a/src/main/java/org/springframework/data/querydsl/binding/QuerydslPathInformation.java +++ b/src/main/java/org/springframework/data/querydsl/binding/QuerydslPathInformation.java @@ -29,6 +29,7 @@ import com.querydsl.core.types.Path; * {@link PathInformation} based on a Querydsl {@link Path}. * * @author Oliver Gierke + * @author Mark Paluch * @since 1.13 */ class QuerydslPathInformation implements PathInformation { @@ -45,7 +46,16 @@ class QuerydslPathInformation implements PathInformation { /* * (non-Javadoc) - * @see org.springframework.data.querydsl.binding.MappedPath#getLeafType() + * @see org.springframework.data.querydsl.binding.PathInformation#getRootParentType() + */ + @Override + public Class getRootParentType() { + return path.getRoot().getType(); + } + + /* + * (non-Javadoc) + * @see org.springframework.data.querydsl.binding.PathInformation#getLeafType() */ @Override public Class getLeafType() { @@ -54,7 +64,7 @@ class QuerydslPathInformation implements PathInformation { /* * (non-Javadoc) - * @see org.springframework.data.querydsl.binding.MappedPath#getLeafParentType() + * @see org.springframework.data.querydsl.binding.PathInformation#getLeafParentType() */ @Override public Class getLeafParentType() { @@ -70,7 +80,7 @@ class QuerydslPathInformation implements PathInformation { /* * (non-Javadoc) - * @see org.springframework.data.querydsl.binding.MappedPath#getLeafProperty() + * @see org.springframework.data.querydsl.binding.PathInformation#getLeafProperty() */ @Override public String getLeafProperty() { @@ -79,7 +89,7 @@ class QuerydslPathInformation implements PathInformation { /* * (non-Javadoc) - * @see org.springframework.data.querydsl.binding.MappedPath#getLeafPropertyDescriptor() + * @see org.springframework.data.querydsl.binding.PathInformation#getLeafPropertyDescriptor() */ @Nullable @Override @@ -89,7 +99,7 @@ class QuerydslPathInformation implements PathInformation { /* * (non-Javadoc) - * @see org.springframework.data.querydsl.binding.MappedPath#toDotPath() + * @see org.springframework.data.querydsl.binding.PathInformation#toDotPath() */ @Override public String toDotPath() { @@ -115,12 +125,13 @@ class QuerydslPathInformation implements PathInformation { return true; } - if (!(o instanceof QuerydslPathInformation)) { + if (!(o instanceof PathInformation)) { return false; } - QuerydslPathInformation that = (QuerydslPathInformation) o; - return ObjectUtils.nullSafeEquals(path, that.path); + PathInformation that = (PathInformation) o; + return ObjectUtils.nullSafeEquals(getRootParentType(), that.getRootParentType()) + && ObjectUtils.nullSafeEquals(toDotPath(), that.toDotPath()); } /* @@ -129,7 +140,9 @@ class QuerydslPathInformation implements PathInformation { */ @Override public int hashCode() { - return ObjectUtils.nullSafeHashCode(path); + int result = ObjectUtils.nullSafeHashCode(getRootParentType()); + result = 31 * result + ObjectUtils.nullSafeHashCode(toDotPath()); + return result; } /* diff --git a/src/test/java/org/springframework/data/querydsl/Address.java b/src/test/java/org/springframework/data/querydsl/Address.java index 6788b44ef..358d5888d 100644 --- a/src/test/java/org/springframework/data/querydsl/Address.java +++ b/src/test/java/org/springframework/data/querydsl/Address.java @@ -25,6 +25,7 @@ public class Address { public String street, city; public Double[] lonLat; + public String description; public Address(String street, String city) { this.street = street; diff --git a/src/test/java/org/springframework/data/querydsl/User.java b/src/test/java/org/springframework/data/querydsl/User.java index 4f80f1b3e..c9416bcb9 100644 --- a/src/test/java/org/springframework/data/querydsl/User.java +++ b/src/test/java/org/springframework/data/querydsl/User.java @@ -37,6 +37,7 @@ public class User { public List
addresses; public List nickNames; public Long inceptionYear; + public String description; public User(String firstname, String lastname, Address address) { @@ -56,7 +57,3 @@ class SpecialUser extends User { } } -@QueryEntity -class UserWrapper { - public User user; -} diff --git a/src/test/java/org/springframework/data/querydsl/UserWrapper.java b/src/test/java/org/springframework/data/querydsl/UserWrapper.java new file mode 100644 index 000000000..71a61c1fd --- /dev/null +++ b/src/test/java/org/springframework/data/querydsl/UserWrapper.java @@ -0,0 +1,23 @@ +/* + * Copyright 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.data.querydsl; + +import com.querydsl.core.annotations.QueryEntity; + +@QueryEntity +public final class UserWrapper { + public User user; +} diff --git a/src/test/java/org/springframework/data/querydsl/binding/PropertyPathInformationUnitTests.java b/src/test/java/org/springframework/data/querydsl/binding/PropertyPathInformationUnitTests.java new file mode 100644 index 000000000..593205fbd --- /dev/null +++ b/src/test/java/org/springframework/data/querydsl/binding/PropertyPathInformationUnitTests.java @@ -0,0 +1,53 @@ +/* + * Copyright 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.data.querydsl.binding; + +import static org.assertj.core.api.Assertions.*; + +import org.junit.jupiter.api.Test; + +import org.springframework.data.querydsl.QUser; +import org.springframework.data.querydsl.User; + +/** + * Unit tests for {@link PropertyPathInformation}. + * + * @author Mark Paluch + */ +class PropertyPathInformationUnitTests { + + @Test // GH-2418 + void shouldEqualsCorrectly() { + + PropertyPathInformation information = PropertyPathInformation.of("address.description", User.class); + + QuerydslPathInformation querydslPathInformation = QuerydslPathInformation.of(QUser.user.address.description); + + assertThat(information).isEqualTo(querydslPathInformation); + assertThat(querydslPathInformation).isEqualTo(information); + } + + @Test // GH-2418 + void shouldHashCodeCorrectly() { + + PropertyPathInformation information = PropertyPathInformation.of("address.description", User.class); + + QuerydslPathInformation querydslPathInformation = QuerydslPathInformation.of(QUser.user.address.description); + + assertThat(information).hasSameHashCodeAs(querydslPathInformation); + assertThat(querydslPathInformation).hasSameHashCodeAs(information); + } +} diff --git a/src/test/java/org/springframework/data/querydsl/binding/QuerydslBindingsUnitTests.java b/src/test/java/org/springframework/data/querydsl/binding/QuerydslBindingsUnitTests.java index 7ae7a48df..8c09af0b5 100755 --- a/src/test/java/org/springframework/data/querydsl/binding/QuerydslBindingsUnitTests.java +++ b/src/test/java/org/springframework/data/querydsl/binding/QuerydslBindingsUnitTests.java @@ -23,6 +23,8 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.core.convert.support.DefaultConversionService; +import org.springframework.data.querydsl.Address; +import org.springframework.data.querydsl.QAddress; import org.springframework.data.querydsl.QSpecialUser; import org.springframework.data.querydsl.QUser; import org.springframework.data.querydsl.SimpleEntityPathResolver; @@ -67,6 +69,27 @@ class QuerydslBindingsUnitTests { assertThat(bindings.getBindingForPath(path)).isEmpty(); } + @Test // GH-2418 + void shouldConsiderOwningTypeWhenObtainingBindings() { + + bindings.bind(QUser.user.description).first(CONTAINS_BINDING); + + assertAdapterWithTargetBinding(bindings.getBindingForPath(PropertyPathInformation.of("description", User.class)), + CONTAINS_BINDING); + assertThat(bindings.getBindingForPath(PropertyPathInformation.of("address.description", User.class))).isEmpty(); + assertThat(bindings.getBindingForPath(PropertyPathInformation.of("description", Address.class))).isEmpty(); + } + + @Test // GH-2418 + void shouldConsiderOwningTypeWhenObtainingBindingsWithQuerydslPathInformation() { + + bindings.bind(QUser.user.description).first(CONTAINS_BINDING); + + assertAdapterWithTargetBinding(bindings.getBindingForPath(QuerydslPathInformation.of(QUser.user.description)), + CONTAINS_BINDING); + assertThat(bindings.getBindingForPath(QuerydslPathInformation.of(QAddress.address.description))).isEmpty(); + } + @Test // DATACMNS-669 void returnsRegisteredBindingForSimplePath() { @@ -78,7 +101,7 @@ class QuerydslBindingsUnitTests { } @Test // DATACMNS-669 - void getBindingForPathShouldReturnSpeficicBindingForNestedElementsWhenAvailable() { + void getBindingForPathShouldReturnSpecificBindingForNestedElementsWhenAvailable() { bindings.bind(QUser.user.address.street).first(CONTAINS_BINDING); 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 b8132fbed..29fc4c546 100755 --- a/src/test/java/org/springframework/data/querydsl/binding/QuerydslPredicateBuilderUnitTests.java +++ b/src/test/java/org/springframework/data/querydsl/binding/QuerydslPredicateBuilderUnitTests.java @@ -27,11 +27,14 @@ import org.joda.time.format.DateTimeFormat; import org.joda.time.format.DateTimeFormatter; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; + +import org.springframework.data.querydsl.Address; import org.springframework.data.querydsl.QSpecialUser; import org.springframework.data.querydsl.QUser; import org.springframework.data.querydsl.QUserWrapper; import org.springframework.data.querydsl.SimpleEntityPathResolver; import org.springframework.data.querydsl.User; +import org.springframework.data.querydsl.UserWrapper; import org.springframework.data.querydsl.Users; import org.springframework.data.util.ClassTypeInformation; import org.springframework.data.util.Version; @@ -42,6 +45,7 @@ import org.springframework.util.MultiValueMap; import com.querydsl.collections.CollQueryFactory; import com.querydsl.core.types.Constant; import com.querydsl.core.types.Predicate; +import com.querydsl.core.types.dsl.StringPath; /** * Unit tests for {@link QuerydslPredicateBuilder}. @@ -54,6 +58,7 @@ class QuerydslPredicateBuilderUnitTests { static final ClassTypeInformation USER_TYPE = ClassTypeInformation.from(User.class); static final QuerydslBindings DEFAULT_BINDINGS = new QuerydslBindings(); + static final SingleValueBinding CONTAINS_BINDING = (path, value) -> path.contains(value); QuerydslPredicateBuilder builder; MultiValueMap values; @@ -83,6 +88,21 @@ class QuerydslPredicateBuilderUnitTests { .isTrue(); } + @Test // GH-2418 + void shouldLookupCorrectPath() { + + DEFAULT_BINDINGS.bind(QUser.user.description).first(CONTAINS_BINDING); + + values.add("description", "Linz"); + Predicate predicate = this.builder.getPredicate(ClassTypeInformation.from(User.class), values, DEFAULT_BINDINGS); + + assertThat(predicate).hasToString("contains(user.description,Linz)"); + + predicate = this.builder.getPredicate(ClassTypeInformation.from(Address.class), values, DEFAULT_BINDINGS); + + assertThat(predicate).hasToString("address.description = Linz"); + } + @Test // DATACMNS-669 void resolveArgumentShouldCreateSingleStringParameterPredicateCorrectly() throws Exception { @@ -206,14 +226,14 @@ class QuerydslPredicateBuilderUnitTests { values.add("user.specialProperty", "VALUE"); - QUserWrapper $ = QUserWrapper.userWrapper; + QUserWrapper wrapper = QUserWrapper.userWrapper; QuerydslBindings bindings = new QuerydslBindings(); - bindings.bind($.user.as(QSpecialUser.class).specialProperty)// + bindings.bind(wrapper.user.as(QSpecialUser.class).specialProperty)// .first(QuerydslBindingsUnitTests.ContainsBinding.INSTANCE); - assertThat(builder.getPredicate(USER_TYPE, values, bindings))// - .isEqualTo($.user.as(QSpecialUser.class).specialProperty.contains("VALUE")); + assertThat(builder.getPredicate(ClassTypeInformation.from(UserWrapper.class), values, bindings))// + .isEqualTo(wrapper.user.as(QSpecialUser.class).specialProperty.contains("VALUE")); } @Test // DATACMNS-1443