From e04f0b0debe80c61bcf80abd46224847853d7090 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Fri, 22 Sep 2023 09:34:06 +0200 Subject: [PATCH] =?UTF-8?q?Fallback=20to=20StringUtils.uncapitalize(?= =?UTF-8?q?=E2=80=A6)=20when=20looking=20up=20property=20paths.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Naming restrictions for property paths used in query method names requite capitalization of the first letter regardless whether the property name uses a second-letter uppercase form (zIndex -> ZIndex, qCode -> QCode). In such cases, Introspector.decapitalize(…) shortcuts to non-decapitalization as it checks the second letter casing. This leads to the case that the property name cannot be resolved, assuming proper property naming (getzIndex(), zIndex()). Falling back to StringUtils.uncapitalize() allows catching such properties. Closes: #1851 Original Pull Request: #2940 --- .../data/mapping/PropertyPath.java | 37 +++++++++++++------ .../data/util/TypeDiscoverer.java | 4 +- .../data/mapping/PropertyPathUnitTests.java | 31 ++++++++++++++++ .../query/parser/PartTreeUnitTests.java | 23 ------------ 4 files changed, 58 insertions(+), 37 deletions(-) diff --git a/src/main/java/org/springframework/data/mapping/PropertyPath.java b/src/main/java/org/springframework/data/mapping/PropertyPath.java index debffac2c..70a77bea4 100644 --- a/src/main/java/org/springframework/data/mapping/PropertyPath.java +++ b/src/main/java/org/springframework/data/mapping/PropertyPath.java @@ -50,7 +50,7 @@ public class PropertyPath implements Streamable { private static final Pattern SPLITTER = Pattern.compile("(?:[%s]?([%s]*?[^%s]+))".replaceAll("%s", DELIMITERS)); private static final Pattern SPLITTER_FOR_QUOTED = Pattern.compile("(?:[%s]?([%s]*?[^%s]+))".replaceAll("%s", "\\.")); private static final Pattern NESTED_PROPERTY_PATTERN = Pattern.compile("\\p{Lu}[\\p{Ll}\\p{Nd}]*$"); - private static final Map cache = new ConcurrentReferenceHashMap<>(); + private static final Map cache = new ConcurrentReferenceHashMap<>(); private final TypeInformation owningType; private final String name; @@ -83,19 +83,31 @@ public class PropertyPath implements Streamable { Assert.notNull(owningType, "Owning type must not be null"); Assert.notNull(base, "Previously found properties must not be null"); - String propertyName = Introspector.decapitalize(name); - TypeInformation propertyType = owningType.getProperty(propertyName); + String decapitalized = Introspector.decapitalize(name); + Property property = lookupProperty(owningType, decapitalized); - if (propertyType == null) { - throw new PropertyReferenceException(propertyName, owningType, base); + if (property == null) { + property = lookupProperty(owningType, StringUtils.uncapitalize(name)); + } + + if (property == null) { + throw new PropertyReferenceException(decapitalized, owningType, base); } this.owningType = owningType; - this.typeInformation = propertyType; - this.isCollection = propertyType.isCollectionLike(); - this.name = propertyName; - this.actualTypeInformation = propertyType.getActualType() == null ? propertyType - : propertyType.getRequiredActualType(); + this.name = property.path(); + this.typeInformation = property.type(); + this.isCollection = this.typeInformation.isCollectionLike(); + this.actualTypeInformation = this.typeInformation.getActualType() == null ? this.typeInformation + : this.typeInformation.getRequiredActualType(); + } + + @Nullable + private static Property lookupProperty(TypeInformation owningType, String name) { + + TypeInformation propertyType = owningType.getProperty(name); + + return propertyType != null ? new Property(propertyType, name) : null; } /** @@ -351,7 +363,7 @@ public class PropertyPath implements Streamable { Assert.hasText(source, "Source must not be null or empty"); Assert.notNull(type, "TypeInformation must not be null or empty"); - return cache.computeIfAbsent(new Key(type, source), it -> { + return cache.computeIfAbsent(new Property(type, source), it -> { List iteratorSource = new ArrayList<>(); @@ -487,5 +499,6 @@ public class PropertyPath implements Streamable { return String.format("%s.%s", owningType.getType().getSimpleName(), toDotPath()); } - private record Key(TypeInformation type, String path) {}; + private record Property(TypeInformation type, String path) { + }; } diff --git a/src/main/java/org/springframework/data/util/TypeDiscoverer.java b/src/main/java/org/springframework/data/util/TypeDiscoverer.java index 9a5d5aa2c..332ee51ab 100644 --- a/src/main/java/org/springframework/data/util/TypeDiscoverer.java +++ b/src/main/java/org/springframework/data/util/TypeDiscoverer.java @@ -370,8 +370,8 @@ class TypeDiscoverer implements TypeInformation { var field = ReflectionUtils.findField(rawType, fieldname); return field != null ? Optional.of(TypeInformation.of(ResolvableType.forField(field, resolvableType))) - : Optional.ofNullable(BeanUtils.getPropertyDescriptor(rawType, fieldname)).map(it -> from(it, rawType)) - .map(TypeInformation::of); + : Optional.ofNullable(BeanUtils.getPropertyDescriptor(rawType, fieldname)) + .filter(it -> it.getName().equals(fieldname)).map(it -> from(it, rawType)).map(TypeInformation::of); } private ResolvableType from(PropertyDescriptor descriptor, Class rawType) { diff --git a/src/test/java/org/springframework/data/mapping/PropertyPathUnitTests.java b/src/test/java/org/springframework/data/mapping/PropertyPathUnitTests.java index 39003fe89..db1f30bf4 100755 --- a/src/test/java/org/springframework/data/mapping/PropertyPathUnitTests.java +++ b/src/test/java/org/springframework/data/mapping/PropertyPathUnitTests.java @@ -48,6 +48,16 @@ class PropertyPathUnitTests { assertThat(reference.getOwningType()).isEqualTo(TypeInformation.of(Foo.class)); } + @Test // GH-1851 + void parsesRecordPropertyCorrectly() { + + var reference = PropertyPath.from("userName", MyRecord.class); + + assertThat(reference.hasNext()).isFalse(); + assertThat(reference.toDotPath()).isEqualTo("userName"); + assertThat(reference.getOwningType()).isEqualTo(TypeInformation.of(MyRecord.class)); + } + @Test void parsesPathPropertyCorrectly() { @@ -292,6 +302,15 @@ class PropertyPathUnitTests { assertThat(path.getSegment()).isEqualTo("UUID"); } + @Test // GH-1851 + void findsSecondLetterUpperCaseProperty() { + + assertThat(PropertyPath.from("qCode", Foo.class).toDotPath()).isEqualTo("qCode"); + assertThat(PropertyPath.from("QCode", Foo.class).toDotPath()).isEqualTo("qCode"); + assertThat(PropertyPath.from("zIndex", MyRecord.class).toDotPath()).isEqualTo("zIndex"); + assertThat(PropertyPath.from("ZIndex", MyRecord.class).toDotPath()).isEqualTo("zIndex"); + } + @Test // DATACMNS-257 void findsNestedAllUppercaseProperty() { @@ -427,7 +446,16 @@ class PropertyPathUnitTests { String userName; String _email; String UUID; + String qCode; String var_name_with_underscore; + + public String getqCode() { + return qCode; + } + + public void setqCode(String qCode) { + this.qCode = qCode; + } } private class Bar { @@ -487,4 +515,7 @@ class PropertyPathUnitTests { } private class B {} + + private record MyRecord(String userName, boolean zIndex) { + } } diff --git a/src/test/java/org/springframework/data/repository/query/parser/PartTreeUnitTests.java b/src/test/java/org/springframework/data/repository/query/parser/PartTreeUnitTests.java index 4c046098d..8286051f9 100755 --- a/src/test/java/org/springframework/data/repository/query/parser/PartTreeUnitTests.java +++ b/src/test/java/org/springframework/data/repository/query/parser/PartTreeUnitTests.java @@ -30,7 +30,6 @@ import org.junit.jupiter.api.Test; import org.springframework.data.domain.Limit; import org.springframework.data.domain.Sort; import org.springframework.data.mapping.PropertyPath; -import org.springframework.data.mapping.PropertyReferenceException; import org.springframework.data.repository.query.parser.Part.IgnoreCaseType; import org.springframework.data.repository.query.parser.Part.Type; import org.springframework.data.repository.query.parser.PartTree.OrPart; @@ -618,28 +617,6 @@ class PartTreeUnitTests { assertThat(tree.hasPredicate()).isFalse(); } - /** - * This test does not verify a desired behaviour but documents a limitation. If it starts failing and everything else - * is green, remove the expectation to fail with an exception. - */ - @Test // DATACMNS-1570 - void specialCapitalizationInSubject() { - - assertThatThrownBy(() -> new PartTree("findByZIndex", SpecialCapitalization.class)) - .isInstanceOf(PropertyReferenceException.class); - } - - /** - * This test does not verify a desired behaviour but documents a limitation. If it starts failing and everything else - * is green, remove the expectation to fail with an exception. - */ - @Test // DATACMNS-1570 - void specialCapitalizationInOrderBy() { - - assertThatThrownBy(() -> new PartTree("findByOrderByZIndex", SpecialCapitalization.class)) - .isInstanceOf(PropertyReferenceException.class); - } - @Test // DATACMNS-1570 void allCapsInSubject() {