diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java index 0b8fb34abc0..d41967a44f0 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/support/ReflectivePropertyAccessor.java @@ -149,7 +149,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { if (method != null) { // Treat it like a property... // The readerCache will only contain gettable properties (let's not worry about setters for now). - Property property = new Property(type, method, null); + Property property = new Property(type, method, null, name); TypeDescriptor typeDescriptor = new TypeDescriptor(property); Method methodToInvoke = ClassUtils.getPubliclyAccessibleMethodIfPossible(method, type); this.readerCache.put(cacheKey, new InvokerPair(methodToInvoke, typeDescriptor)); @@ -193,7 +193,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { if (method != null) { // Treat it like a property... // The readerCache will only contain gettable properties (let's not worry about setters for now). - Property property = new Property(type, method, null); + Property property = new Property(type, method, null, name); TypeDescriptor typeDescriptor = new TypeDescriptor(property); methodToInvoke = ClassUtils.getPubliclyAccessibleMethodIfPossible(method, type); invoker = new InvokerPair(methodToInvoke, typeDescriptor); @@ -251,7 +251,7 @@ public class ReflectivePropertyAccessor implements PropertyAccessor { Method method = findSetterForProperty(name, type, target); if (method != null) { // Treat it like a property - Property property = new Property(type, null, method); + Property property = new Property(type, null, method, name); TypeDescriptor typeDescriptor = new TypeDescriptor(property); method = ClassUtils.getPubliclyAccessibleMethodIfPossible(method, type); this.writerCache.put(cacheKey, method); diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/PropertyAccessTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/PropertyAccessTests.java index 2209f56cd09..d730835f728 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/PropertyAccessTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/PropertyAccessTests.java @@ -16,17 +16,25 @@ package org.springframework.expression.spel; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; import java.lang.reflect.Method; import java.time.ZoneId; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.Set; import org.assertj.core.api.ThrowableTypeAssert; +import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.Test; import org.springframework.core.convert.TypeDescriptor; +import org.springframework.core.convert.converter.ConditionalGenericConverter; +import org.springframework.core.convert.support.GenericConversionService; import org.springframework.expression.AccessException; import org.springframework.expression.EvaluationContext; import org.springframework.expression.EvaluationException; @@ -35,8 +43,10 @@ import org.springframework.expression.PropertyAccessor; import org.springframework.expression.TypedValue; import org.springframework.expression.spel.standard.SpelExpression; import org.springframework.expression.spel.standard.SpelExpressionParser; +import org.springframework.expression.spel.support.ReflectivePropertyAccessor; import org.springframework.expression.spel.support.SimpleEvaluationContext; import org.springframework.expression.spel.support.StandardEvaluationContext; +import org.springframework.expression.spel.support.StandardTypeConverter; import org.springframework.expression.spel.testresources.Inventor; import org.springframework.expression.spel.testresources.Person; import org.springframework.expression.spel.testresources.RecordPerson; @@ -238,6 +248,109 @@ class PropertyAccessTests extends AbstractExpressionTests { .extracting(SpelEvaluationException::getMessageCode).isEqualTo(SpelMessage.NOT_ASSIGNABLE); } + @Test // gh-37123 + void propertyReadOnlyWithGenuineRecordAccessorEmbeddingPrefix() { + EvaluationContext context = SimpleEvaluationContext.forReadOnlyDataBinding().build(); + Deal deal = new Deal("100", "urgent"); + + Expression budgetExpr = parser.parseExpression("budget"); + Expression issueExpr = parser.parseExpression("issue"); + + assertThat(budgetExpr.getValue(context, deal)).isEqualTo("100"); + assertThat(issueExpr.getValue(context, deal)).isEqualTo("urgent"); + + // The property name used to look up the record component's backing field (and thus + // its field-only annotations) must exactly match the component name: "budget", not "" + // from Property#resolveName() stripping a "get" that merely appears embedded in the + // name; "issue", not "sue" from stripping an "is" that merely happens to start it. + assertThat(budgetExpr.getValueTypeDescriptor(context, deal).getAnnotation(Marker.class).value()) + .isEqualTo("budget"); + assertThat(issueExpr.getValueTypeDescriptor(context, deal).getAnnotation(Marker.class).value()) + .isEqualTo("issue"); + } + + @Test // gh-37123 + void propertyReadOnlyWithNonRecordDataClassAccessorEmbeddingPrefix() { + EvaluationContext context = SimpleEvaluationContext.forReadOnlyDataBinding().build(); + IslandDataClass target = new IslandDataClass("Bali", "gizmo"); + + Expression islandExpr = parser.parseExpression("island"); + Expression widgetExpr = parser.parseExpression("widget"); + + assertThat(islandExpr.getValue(context, target)).isEqualTo("Bali"); + assertThat(widgetExpr.getValue(context, target)).isEqualTo("gizmo"); + + // Same guarantee as for a genuine java.lang.Record (see above): "island", not "land" + // from stripping an "is" that merely happens to start it; "widget", not "" from + // stripping a "get" that merely appears embedded in the name. But here on a plain, + // hand-written "data class" that is not a record and thus cannot be special-cased + // by type in Property#resolveName(). + assertThat(islandExpr.getValueTypeDescriptor(context, target).getAnnotation(Marker.class).value()) + .isEqualTo("island"); + assertThat(widgetExpr.getValueTypeDescriptor(context, target).getAnnotation(Marker.class).value()) + .isEqualTo("widget"); + } + + @Test // gh-37123 + void readCallSiteWithoutPriorCanReadResolvesCorrectAnnotation() throws AccessException { + // ReflectivePropertyAccessor#read() has its own Property construction call site, + // distinct from the one in canRead(). In the standard SpEL evaluation flow, that + // site is bypassed once canRead() has warmed the cache (and once an optimal + // accessor takes over), so we exercise it directly here with a fresh accessor/cache. + ReflectivePropertyAccessor accessor = new ReflectivePropertyAccessor(); + EvaluationContext context = new StandardEvaluationContext(); + IslandDataClass target = new IslandDataClass("Bali", "gizmo"); + + TypedValue result = accessor.read(context, target, "island"); + + assertThat(result.getValue()).isEqualTo("Bali"); + assertThat(result.getTypeDescriptor().getAnnotation(Marker.class).value()).isEqualTo("island"); + } + + @Test // gh-37123 + void propertyReadWriteWithBooleanJavaBeanGetter() { + EvaluationContext context = SimpleEvaluationContext.forReadWriteDataBinding().build(); + Flag target = new Flag(); + + Expression activeExpr = parser.parseExpression("active"); + assertThat(activeExpr.getValue(context, target)).isEqualTo(false); + + activeExpr.setValue(context, target, true); + assertThat(target.isActive()).isTrue(); + assertThat(activeExpr.getValue(context, target)).isEqualTo(true); + } + + @Test // gh-37123 + void propertyReadResolvesCorrectFieldForAcronymPropertyDespiteSimilarlyNamedField() { + EvaluationContext context = SimpleEvaluationContext.forReadOnlyDataBinding().build(); + UrlHolder target = new UrlHolder(); + target.setURL("https://spring.io"); + + Expression urlExpr = parser.parseExpression("URL"); + assertThat(urlExpr.getValue(context, target)).isEqualTo("https://spring.io"); + + // Property#resolveName() would derive "uRL" (plain uncapitalize of "URL") for this + // getter, which happens to be the exact name of a decoy field on UrlHolder; passing + // the already-verified SpEL identifier "URL" through directly avoids that mismatch. + assertThat(urlExpr.getValueTypeDescriptor(context, target).getAnnotation(Marker.class).value()) + .isEqualTo("right"); + } + + @Test // gh-37123 + void propertyWriteAppliesAnnotationDrivenConversionOnlyForCorrectlyResolvedAcronymField() { + GenericConversionService conversionService = new GenericConversionService(); + conversionService.addConverter(new MarkerAwareStringConverter()); + StandardEvaluationContext context = new StandardEvaluationContext(); + context.setTypeConverter(new StandardTypeConverter(conversionService)); + UrlHolder target = new UrlHolder(); + + parser.parseExpression("URL").setValue(context, target, "https://spring.io"); + + // Only observable if canWrite()'s Property resolves the "URL" field (not the "uRL" + // decoy field) and thus picks up the conversion-triggering @Marker("right") annotation. + assertThat(target.getURL()).isEqualTo("[https://spring.io]"); + } + @Test void propertyReadWrite() { EvaluationContext context = SimpleEvaluationContext.forReadWriteDataBinding().build(); @@ -401,4 +514,97 @@ class PropertyAccessTests extends AbstractExpressionTests { } } + + @Retention(RetentionPolicy.RUNTIME) + @Target(ElementType.FIELD) + private @interface Marker { + + String value(); + } + + + // Intentionally contains a "budget" property with a "get" prefix in its name + // and an "issue" property with an "is" prefix in its name. + private record Deal(@Marker("budget") String budget, @Marker("issue") String issue) { + } + + + // Intentionally contains a "widget" property with a "get" prefix in its name + // and an "island" property with an "is" prefix in its name. + static class IslandDataClass { + + @Marker("island") + private final String island; + + @Marker("widget") + private final String widget; + + IslandDataClass(String island, String widget) { + this.island = island; + this.widget = widget; + } + + // Intentionally begins with an "is" prefix, so to speak. + public String island() { + return this.island; + } + + // Intentionally embeds a "get" prefix, so to speak. + public String widget() { + return this.widget; + } + } + + + static class Flag { + + private boolean active; + + public boolean isActive() { + return this.active; + } + + public void setActive(boolean active) { + this.active = active; + } + } + + + private static class UrlHolder { + + @Marker("wrong") + private String uRL = "decoy"; + + @Marker("right") + private String URL; + + public String getURL() { + return this.URL; + } + + public void setURL(String URL) { + this.URL = URL; + } + } + + + private static class MarkerAwareStringConverter implements ConditionalGenericConverter { + + @Override + public Set getConvertibleTypes() { + return Set.of(new ConvertiblePair(String.class, String.class)); + } + + @Override + public boolean matches(TypeDescriptor sourceType, TypeDescriptor targetType) { + Marker marker = targetType.getAnnotation(Marker.class); + return (marker != null && marker.value().equals("right")); + } + + @Override + public @Nullable Object convert(@Nullable Object source, TypeDescriptor sourceType, TypeDescriptor targetType) { + return "[" + source + "]"; + } + } + }