From 4d239e3feb2cfb841ba53ccc7024695638c5f8bc Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Thu, 29 Jul 2021 09:09:23 +0200 Subject: [PATCH 1/2] Prepare issue branch. --- pom.xml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pom.xml b/pom.xml index 9001e043d4..968192ea3f 100644 --- a/pom.xml +++ b/pom.xml @@ -1,11 +1,13 @@ - + 4.0.0 org.springframework.data spring-data-commons - 2.6.0-SNAPSHOT + 2.6.0-2418-SNAPSHOT Spring Data Core From d24a37d5ecaeb41215e01407d179efa286e1d0ee Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Thu, 29 Jul 2021 11:46:31 +0200 Subject: [PATCH 2/2] 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 --- .../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 32e640065c..20297a9240 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 @@ */ interface PathInformation { + /** + * The root property owner type. + * + * @return + * @since 2.5.4 + */ + 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 df50fdc907..2d74c8720d 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 @@ * * @author Oliver Gierke * @author Christoph Strobl + * @author Mark Paluch * @since 1.13 */ class PropertyPathInformation implements PathInformation { @@ -71,6 +72,15 @@ private static PropertyPathInformation of(PropertyPath path) { 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 @@ public boolean equals(Object o) { 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 @@ public boolean equals(Object o) { */ @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 61ffc0cdb7..670fea9676 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 @@ */ public class QuerydslBindings { + // pathSpecs key format: . private final Map> pathSpecs; private final Map, PathAndBinding> typeSpecs; private final Set allowList; @@ -203,7 +204,7 @@ public , T> Optional> getBin 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 @@ Optional> getExistingPath(PathInformation path) { 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 @@ PathInformation getPropertyPath(String path, TypeInformation type) { 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 @@ PathInformation getPropertyPath(String path, TypeInformation type) { } } + /** + * 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 void all(MultiValueBinding binding) { } 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 @@ protected void registerBinding(PathAndBinding binding) { 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 d89776897f..67a0f2da49 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 @@ * {@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 @@ public static QuerydslPathInformation of(Path path) { /* * (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 @@ public Class getLeafType() { /* * (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 @@ public Class getLeafParentType() { /* * (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 @@ public String getLeafProperty() { /* * (non-Javadoc) - * @see org.springframework.data.querydsl.binding.MappedPath#getLeafPropertyDescriptor() + * @see org.springframework.data.querydsl.binding.PathInformation#getLeafPropertyDescriptor() */ @Nullable @Override @@ -89,7 +99,7 @@ public PropertyDescriptor getLeafPropertyDescriptor() { /* * (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 @@ public boolean equals(Object o) { 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 @@ public boolean equals(Object o) { */ @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 6788b44efc..358d5888d1 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 4f80f1b3e0..c9416bcb9b 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 @@ public SpecialUser(String firstname, String lastname, Address address) { } } -@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 0000000000..71a61c1fde --- /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 0000000000..593205fbd7 --- /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 7ae7a48df2..8c09af0b55 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.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 @@ void returnsEmptyOptionalIfNoBindingRegisteredForPath() { 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 @@ void returnsRegisteredBindingForSimplePath() { } @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 b8132fbed2..29fc4c546e 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.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 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 @@ void getPredicateShouldReturnEmptyWhenPropertiesAreEmpty() { .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 @@ void buildsPredicateForBindingUsingNestedDowncast() { 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