From d2a07d9d0c962d41e89e343a58013ad5ff6f204a Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Fri, 22 Sep 2023 09:19:55 +0200 Subject: [PATCH 1/4] Prepare issue branch. --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 38a742d830..7c3f49b2af 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-commons - 3.3.0-SNAPSHOT + 3.3.x-GH-1851-SNAPSHOT Spring Data Core Core Spring concepts underpinning every Spring Data module. From dca733197016d966f87ef607405e779a8102ed4b Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Fri, 22 Sep 2023 09:34:06 +0200 Subject: [PATCH 2/4] =?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. --- .../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 debffac2cb..70a77bea49 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 static PropertyPath from(String source, TypeInformation type) { 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 String toString() { 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 9a5d5aa2c8..332ee51abf 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 @@ private Optional> getPropertyInformation(String fieldname) { 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 39003fe89a..db1f30bf44 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 @@ void parsesSimplePropertyCorrectly() { 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 @@ void findsAllUppercaseProperty() { 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 @@ private class Foo { 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 @@ private class Category { } 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 4c046098d3..8286051f90 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.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 @@ void emptyTreeDoesNotContainParts() { 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() { From f54deadc1ec453c1c548916da906db8a41adef87 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 8 Nov 2023 11:39:42 +0100 Subject: [PATCH 3/4] Polishing. Fix test naming. Improve equals check by caching resolved generics. --- src/main/java/org/springframework/data/util/TypeDiscoverer.java | 2 +- .../java/org/springframework/data/util/TypeInformation.java | 2 +- .../org/springframework/data/mapping/PropertyPathUnitTests.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/springframework/data/util/TypeDiscoverer.java b/src/main/java/org/springframework/data/util/TypeDiscoverer.java index 332ee51abf..45235cfecd 100644 --- a/src/main/java/org/springframework/data/util/TypeDiscoverer.java +++ b/src/main/java/org/springframework/data/util/TypeDiscoverer.java @@ -74,7 +74,7 @@ protected TypeDiscoverer(ResolvableType type) { .map(ResolvableType::toClass).collect(Collectors.toList())); } - static TypeDiscoverer td(ResolvableType type) { + static TypeDiscoverer ofCached(ResolvableType type) { Assert.notNull(type, "Type must not be null"); diff --git a/src/main/java/org/springframework/data/util/TypeInformation.java b/src/main/java/org/springframework/data/util/TypeInformation.java index 3d49c87e71..2b2ff88f97 100644 --- a/src/main/java/org/springframework/data/util/TypeInformation.java +++ b/src/main/java/org/springframework/data/util/TypeInformation.java @@ -60,7 +60,7 @@ public static TypeInformation of(ResolvableType type) { Assert.notNull(type, "Type must not be null"); return type.hasGenerics() || (type.isArray() && type.getComponentType().hasGenerics()) // - || (type.getType() instanceof TypeVariable) ? TypeDiscoverer.td(type) : ClassTypeInformation.from(type); + || (type.getType() instanceof TypeVariable) ? TypeDiscoverer.ofCached(type) : ClassTypeInformation.from(type); } /** diff --git a/src/test/java/org/springframework/data/mapping/PropertyPathUnitTests.java b/src/test/java/org/springframework/data/mapping/PropertyPathUnitTests.java index db1f30bf44..51996eb364 100755 --- a/src/test/java/org/springframework/data/mapping/PropertyPathUnitTests.java +++ b/src/test/java/org/springframework/data/mapping/PropertyPathUnitTests.java @@ -124,7 +124,7 @@ void handlesArrayCorrectly() { } @Test - void handlesInvalidCollectionCompountTypeProperl() { + void handlesInvalidCollectionCompoundTypeProperly() { try { PropertyPath.from("usersMame", Bar.class); From 0d5a9bc3aaf60cf00061f4dd7a45c382643677d4 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Wed, 8 Nov 2023 10:52:59 +0100 Subject: [PATCH 4/4] Add tests & update documentation. --- .../repositories/query-methods-details.adoc | 29 ++++++++++++++++++- .../data/mapping/PropertyPathUnitTests.java | 21 ++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/main/antora/modules/ROOT/pages/repositories/query-methods-details.adoc b/src/main/antora/modules/ROOT/pages/repositories/query-methods-details.adoc index e417be1bcd..ea2fddb1b7 100644 --- a/src/main/antora/modules/ROOT/pages/repositories/query-methods-details.adoc +++ b/src/main/antora/modules/ROOT/pages/repositories/query-methods-details.adoc @@ -119,7 +119,34 @@ So our method name would be as follows: List findByAddress_ZipCode(ZipCode zipCode); ---- -Because we treat the underscore character as a reserved character, we strongly advise following standard Java naming conventions (that is, not using underscores in property names but using camel case instead). +[NOTE] +==== +Because we treat underscores (`_`) as a reserved character, we strongly advise to follow standard Java naming conventions (that is, not using underscores in property names but applying camel case instead). +==== + +[CAUTION] +==== +.Field Names starting with underscore: +Field names may start with underscores like `String \_name`. +Make sure to preserve the `_` as in `\_name` and use double `_` to split nested paths like `user__name`. + +.Upper Case Field Names: +Field names that are all uppercase can be used as such. +Nested paths if applicable require splitting via `_` as in `USER_name`. + +.Field Names with 2nd uppercase letter: +Field names that consist of a starting lower case letter followed by an uppercase one like `String qCode` can be resolved by starting with two upper case letters as in `QCode`. +Please be aware of potential path ambiguities. + +.Path Ambiguities: +In the following sample the arrangement of properties `qCode` and `q`, with `q` containing a property called `code`, creates an ambiguity for the path `QCode`. +```java +record Container(String qCode, Code q) {} +record Code(String code) {} +``` +Since a direct match on a property is considered first, any potential nested paths will not be considered and the algorithm picks the `qCode` field. +In order to select the `code` field in `q` the underscore notation `Q_Code` is required. +==== [[repositories.collections-and-iterables]] == Repository Methods Returning Collections or Iterables diff --git a/src/test/java/org/springframework/data/mapping/PropertyPathUnitTests.java b/src/test/java/org/springframework/data/mapping/PropertyPathUnitTests.java index 51996eb364..7df3c8729d 100755 --- a/src/test/java/org/springframework/data/mapping/PropertyPathUnitTests.java +++ b/src/test/java/org/springframework/data/mapping/PropertyPathUnitTests.java @@ -309,6 +309,17 @@ void findsSecondLetterUpperCaseProperty() { 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"); + assertThat(PropertyPath.from("_foo.QCode", Sample2.class).toDotPath()).isEqualTo("_foo.qCode"); + assertThat(PropertyPath.from("_fooQCode", Sample2.class).toDotPath()).isEqualTo("_foo.qCode"); + } + + @Test // GH-1851 + void favoursPropertyHitOverNestedPath() { + + assertThat(PropertyPath.from("qCode", NameAmbiguities.class).toDotPath()).isEqualTo("qCode"); + assertThat(PropertyPath.from("QCode", NameAmbiguities.class).toDotPath()).isEqualTo("qCode"); + assertThat(PropertyPath.from("Q_Code", NameAmbiguities.class).toDotPath()).isEqualTo("q.code"); + assertThat(PropertyPath.from("q_code", NameAmbiguities.class).toDotPath()).isEqualTo("q.code"); } @Test // DATACMNS-257 @@ -458,6 +469,16 @@ public void setqCode(String qCode) { } } + private static class NameAmbiguities { + + String qCode; + Code q; + } + + private static class Code { + String code; + } + private class Bar { private FooBar user;