From f16474d5854951549e6d6a01a3a28e9d91973cd5 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Tue, 6 Jan 2026 17:27:02 +0100 Subject: [PATCH] Reliably resolve overloaded write methods in PropertyDescriptorUtils Prior to this commit, the algorithm behind determineBasicProperties() in PropertyDescriptorUtils did not reliably resolve the correct write method when one candidate write method had a parameter type that was a subtype of another candidate write method whose parameter type was an exact match for the resolved read method's return type. In other words, the algorithm always resolved the candidate write method with the most specific parameter type (similar to covariant return types) which is not necessarily the resolved read method's return type. To address that, this commit ensures that determineBasicProperties() always selects an exact match for the write method whenever possible. As an added bonus, determineBasicProperties() no longer invokes BasicPropertyDescriptor.getWriteMethod(), which avoids triggering the resolution algorithm multiple times (when multiple write method candidates exist), resulting in lazy resolution of the write method the first time client code invokes getWriteMethod(). Closes gh-36113 --- .../beans/PropertyDescriptorUtils.java | 53 ++++++++++--------- ...escriptorUtilsPropertyResolutionTests.java | 10 +--- 2 files changed, 30 insertions(+), 33 deletions(-) diff --git a/spring-beans/src/main/java/org/springframework/beans/PropertyDescriptorUtils.java b/spring-beans/src/main/java/org/springframework/beans/PropertyDescriptorUtils.java index 16b92c0a44e..7fb8ac07181 100644 --- a/spring-beans/src/main/java/org/springframework/beans/PropertyDescriptorUtils.java +++ b/spring-beans/src/main/java/org/springframework/beans/PropertyDescriptorUtils.java @@ -91,14 +91,7 @@ abstract class PropertyDescriptorUtils { BasicPropertyDescriptor pd = pdMap.get(propertyName); if (pd != null) { if (setter) { - Method writeMethod = pd.getWriteMethod(); - if (writeMethod == null || - writeMethod.getParameterTypes()[0].isAssignableFrom(method.getParameterTypes()[0])) { - pd.setWriteMethod(method); - } - else { - pd.addWriteMethod(method); - } + pd.addWriteMethod(method); } else { Method readMethod = pd.getReadMethod(); @@ -270,7 +263,7 @@ abstract class PropertyDescriptorUtils { private @Nullable Method writeMethod; - private final List alternativeWriteMethods = new ArrayList<>(); + private final List candidateWriteMethods = new ArrayList<>(); public BasicPropertyDescriptor(String propertyName, Class beanClass, @Nullable Method readMethod, @Nullable Method writeMethod) throws IntrospectionException { @@ -294,34 +287,46 @@ abstract class PropertyDescriptorUtils { this.writeMethod = writeMethod; } - public void addWriteMethod(Method writeMethod) { + void addWriteMethod(Method writeMethod) { + // Since setWriteMethod() is invoked from the PropertyDescriptor(String, Method, Method) + // constructor, this.writeMethod may be non-null. if (this.writeMethod != null) { - this.alternativeWriteMethods.add(this.writeMethod); + this.candidateWriteMethods.add(this.writeMethod); this.writeMethod = null; } - this.alternativeWriteMethods.add(writeMethod); + this.candidateWriteMethods.add(writeMethod); } @Override public @Nullable Method getWriteMethod() { - if (this.writeMethod == null && !this.alternativeWriteMethods.isEmpty()) { - if (this.readMethod == null) { - this.writeMethod = this.alternativeWriteMethods.get(0); + if (this.writeMethod == null && !this.candidateWriteMethods.isEmpty()) { + if (this.readMethod == null || this.candidateWriteMethods.size() == 1) { + this.writeMethod = this.candidateWriteMethods.get(0); } else { - for (Method method : this.alternativeWriteMethods) { - // Check subtype match first. - if (this.readMethod.getReturnType().isAssignableFrom(method.getParameterTypes()[0])) { + Class resolvedReadType = + ResolvableType.forMethodReturnType(this.readMethod, this.beanClass).toClass(); + for (Method method : this.candidateWriteMethods) { + // 1) Check for an exact match against the resolved types. + Class resolvedWriteType = + ResolvableType.forMethodParameter(method, 0, this.beanClass).toClass(); + if (resolvedReadType.equals(resolvedWriteType)) { this.writeMethod = method; break; } - // Check exact match against resolved generic parameter type as a fallback. - if (!(method.getGenericParameterTypes()[0] instanceof Class)) { - Class resolvedParameterType = - ResolvableType.forMethodParameter(method, 0, this.beanClass).toClass(); - if (this.readMethod.getReturnType().equals(resolvedParameterType)) { + + // 2) Check if the candidate write method's parameter type is compatible with + // the read method's return type. + Class parameterType = method.getParameterTypes()[0]; + if (this.readMethod.getReturnType().isAssignableFrom(parameterType)) { + // If we haven't yet found a compatible write method, or if the current + // candidate's parameter type is a subtype of the previous candidate's + // parameter type, track the current candidate as the write method. + if (this.writeMethod == null || + this.writeMethod.getParameterTypes()[0].isAssignableFrom(parameterType)) { this.writeMethod = method; - break; + // We do not "break" here, since we need to compare the current candidate + // with all remaining candidates. } } } diff --git a/spring-beans/src/test/java/org/springframework/beans/PropertyDescriptorUtilsPropertyResolutionTests.java b/spring-beans/src/test/java/org/springframework/beans/PropertyDescriptorUtilsPropertyResolutionTests.java index dbfc08e3a40..11d772dedb1 100644 --- a/spring-beans/src/test/java/org/springframework/beans/PropertyDescriptorUtilsPropertyResolutionTests.java +++ b/spring-beans/src/test/java/org/springframework/beans/PropertyDescriptorUtilsPropertyResolutionTests.java @@ -387,15 +387,7 @@ class PropertyDescriptorUtilsPropertyResolutionTests { void resolvePropertiesWithUnresolvedGenericsInSubclassWithOverloadedSetter() { var pdMap = resolver.resolve(PersonWithOverloadedSetter.class); - Class writeType = Number.class; - if (resolver instanceof BasicPropertiesResolver) { - // TODO: PropertyDescriptorUtils currently incorrectly resolves setId(Integer) - // as the write method instead of setId(Number) (where Number is the - // unresolved generic for Long). - writeType = Integer.class; - } - - assertReadAndWriteMethodsForClassAndId(pdMap, Number.class, writeType); + assertReadAndWriteMethodsForClassAndId(pdMap, Number.class, Number.class); }