From b3fb9f4e31ab7ff7b5ad09ae217d83ff35643509 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Mon, 15 Dec 2025 15:26:46 +0100 Subject: [PATCH] Reliably resolve generic read/write methods in PropertyDescriptorUtils Prior to this commit, the determineBasicProperties() method in PropertyDescriptorUtils did not reliably resolve read/write methods in type hierarchies with generics. This utility method is used by SimpleBeanInfoFactory which is used by BeanUtils and BeanWrapperImpl. Thus, failure to reliably resolve read/write JavaBeans methods resulted in bugs in certain scenarios. For example, BeanUtils.copyProperties() randomly failed to copy certain properties if the write method for the property could not be resolved. To address such issues, this commit revises the implementation of PropertyDescriptorUtils as follows. 1) Read methods with covariant return types are now consistently resolved correctly. 2) If multiple ambiguous write methods are discovered, the algorithm now checks for an exact match against the resolved generic parameter type as a fallback. Closes gh-36019 (cherry picked from commit 4b07edbaeb80b3ed6d0710c47fdc19065d102553) --- .../beans/PropertyDescriptorUtils.java | 25 +- .../springframework/beans/BeanUtilsTests.java | 12 + ...escriptorUtilsPropertyResolutionTests.java | 265 ++++++++++++++++++ 3 files changed, 296 insertions(+), 6 deletions(-) create mode 100644 spring-beans/src/test/java/org/springframework/beans/PropertyDescriptorUtilsPropertyResolutionTests.java 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 8dfc4e53ff7..5cb9b02a72b 100644 --- a/spring-beans/src/main/java/org/springframework/beans/PropertyDescriptorUtils.java +++ b/spring-beans/src/main/java/org/springframework/beans/PropertyDescriptorUtils.java @@ -26,6 +26,7 @@ import java.util.List; import java.util.Map; import java.util.TreeMap; +import org.springframework.core.ResolvableType; import org.springframework.lang.Nullable; import org.springframework.util.ObjectUtils; import org.springframework.util.StringUtils; @@ -35,6 +36,7 @@ import org.springframework.util.StringUtils; * * @author Chris Beams * @author Juergen Hoeller + * @author Sam Brannen */ abstract class PropertyDescriptorUtils { @@ -99,14 +101,13 @@ abstract class PropertyDescriptorUtils { } else { Method readMethod = pd.getReadMethod(); - if (readMethod == null || - (readMethod.getReturnType() == method.getReturnType() && method.getName().startsWith("is"))) { + if (readMethod == null || readMethod.getReturnType().isAssignableFrom(method.getReturnType())) { pd.setReadMethod(method); } } } else { - pd = new BasicPropertyDescriptor(propertyName, (!setter ? method : null), (setter ? method : null)); + pd = new BasicPropertyDescriptor(propertyName, beanClass, (!setter ? method : null), (setter ? method : null)); pdMap.put(propertyName, pd); } } @@ -264,6 +265,8 @@ abstract class PropertyDescriptorUtils { */ private static class BasicPropertyDescriptor extends PropertyDescriptor { + private final Class beanClass; + @Nullable private Method readMethod; @@ -272,10 +275,11 @@ abstract class PropertyDescriptorUtils { private final List alternativeWriteMethods = new ArrayList<>(); - public BasicPropertyDescriptor(String propertyName, @Nullable Method readMethod, @Nullable Method writeMethod) + public BasicPropertyDescriptor(String propertyName, Class beanClass, @Nullable Method readMethod, @Nullable Method writeMethod) throws IntrospectionException { super(propertyName, readMethod, writeMethod); + this.beanClass = beanClass; } @Override @@ -307,14 +311,24 @@ abstract class PropertyDescriptorUtils { public Method getWriteMethod() { if (this.writeMethod == null && !this.alternativeWriteMethods.isEmpty()) { if (this.readMethod == null) { - return this.alternativeWriteMethods.get(0); + this.writeMethod = this.alternativeWriteMethods.get(0); } else { for (Method method : this.alternativeWriteMethods) { + // Check subtype match first. if (this.readMethod.getReturnType().isAssignableFrom(method.getParameterTypes()[0])) { 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)) { + this.writeMethod = method; + break; + } + } } } } @@ -322,5 +336,4 @@ abstract class PropertyDescriptorUtils { } } - } diff --git a/spring-beans/src/test/java/org/springframework/beans/BeanUtilsTests.java b/spring-beans/src/test/java/org/springframework/beans/BeanUtilsTests.java index c8ae06c4d80..34d5903a4aa 100644 --- a/spring-beans/src/test/java/org/springframework/beans/BeanUtilsTests.java +++ b/spring-beans/src/test/java/org/springframework/beans/BeanUtilsTests.java @@ -38,6 +38,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; +import org.springframework.beans.PropertyDescriptorUtilsPropertyResolutionTests.ServiceWithOverriddenGetterAndOverloadedSetter; import org.springframework.beans.factory.BeanFactory; import org.springframework.beans.propertyeditors.CustomDateEditor; import org.springframework.beans.testfixture.beans.DerivedTestBean; @@ -541,6 +542,17 @@ class BeanUtilsTests { assertThat((boolean) target.getFlag1()).isTrue(); assertThat(target.getFlag2()).isTrue(); } + + @Test // gh-36019 + void copyPropertiesHonorsGenericsInTypeHieararchyAndIgnoresOverloadedSetterMethod() { + var source = new ServiceWithOverriddenGetterAndOverloadedSetter(); + var target = new ServiceWithOverriddenGetterAndOverloadedSetter(); + + source.setId(1); + BeanUtils.copyProperties(source, target); + + assertThat(target.getId()).isEqualTo(source.getId()).isEqualTo("1"); + } } diff --git a/spring-beans/src/test/java/org/springframework/beans/PropertyDescriptorUtilsPropertyResolutionTests.java b/spring-beans/src/test/java/org/springframework/beans/PropertyDescriptorUtilsPropertyResolutionTests.java new file mode 100644 index 00000000000..66207423f1b --- /dev/null +++ b/spring-beans/src/test/java/org/springframework/beans/PropertyDescriptorUtilsPropertyResolutionTests.java @@ -0,0 +1,265 @@ +/* + * Copyright 2002-present 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.beans; + +import java.beans.Introspector; +import java.beans.PropertyDescriptor; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.stream.Stream; + +import org.junit.jupiter.api.Named; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.Parameter; +import org.junit.jupiter.params.ParameterizedClass; +import org.junit.jupiter.params.provider.FieldSource; + +import static java.util.stream.Collectors.groupingBy; +import static java.util.stream.Collectors.toList; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assumptions.assumeThat; +import static org.assertj.core.api.SoftAssertions.assertSoftly; +import static org.junit.jupiter.api.Named.named; + +/** + * Unit tests for property descriptor resolution via + * {@link PropertyDescriptorUtils#determineBasicProperties(Class)}. + * + *

Results are compared to the behavior of the standard {@link Introspector}. + * + * @author Sam Brannen + * @since 6.2.16 + */ +@ParameterizedClass(name = "{0}") +@FieldSource("resolvers") +class PropertyDescriptorUtilsPropertyResolutionTests { + + static final List> resolvers = List.of( + named("Basic Properties", new BasicPropertiesResolver()), + named("Standard Properties", new StandardPropertiesResolver())); + + + @Parameter + PropertiesResolver resolver; + + + @Test + void determineBasicPropertiesWithUnresolvedGenericsInInterface() { + var pdMap = resolver.resolve(GenericService.class); + + assertThat(pdMap).containsOnlyKeys("id"); + assertReadAndWriteMethodsForId(pdMap.get("id"), Object.class, Object.class); + } + + @Test + void determineBasicPropertiesWithUnresolvedGenericsInSubInterface() { + // FYI: java.beans.Introspector does not resolve properties for sub-interfaces. + assumeThat(resolver).isNotInstanceOf(StandardPropertiesResolver.class); + + var pdMap = resolver.resolve(SubGenericService.class); + + assertThat(pdMap).containsOnlyKeys("id"); + assertReadAndWriteMethodsForId(pdMap.get("id"), Object.class, Object.class); + } + + @Test + void resolvePropertiesWithUnresolvedGenericsInClass() { + var pdMap = resolver.resolve(BaseService.class); + + assertReadAndWriteMethodsForClassAndId(pdMap, Object.class, Object.class); + } + + @Test // gh-36019 + void resolvePropertiesInSubclassWithOverriddenGetterAndSetter() { + var pdMap = resolver.resolve(ServiceWithOverriddenGetterAndSetter.class); + + assertReadAndWriteMethodsForClassAndId(pdMap, String.class, String.class); + } + + @Test // gh-36019 + void resolvePropertiesWithUnresolvedGenericsInSubclassWithOverloadedSetter() { + var pdMap = resolver.resolve(ServiceWithOverloadedSetter.class); + + assertReadAndWriteMethodsForClassAndId(pdMap, Object.class, Object.class); + } + + @Test // gh-36019 + void resolvePropertiesWithPartiallyUnresolvedGenericsInSubclassWithOverriddenGetter() { + var pdMap = resolver.resolve(ServiceWithOverriddenGetter.class); + + assertReadAndWriteMethodsForClassAndId(pdMap, String.class, Object.class); + } + + @Test // gh-36019 + void resolvePropertiesWithPartiallyUnresolvedGenericsInSubclassWithOverriddenGetterAndOverloadedSetter() { + var pdMap = resolver.resolve(ServiceWithOverriddenGetterAndOverloadedSetter.class); + + assertReadAndWriteMethodsForClassAndId(pdMap, String.class, Object.class); + } + + + private static void assertReadAndWriteMethodsForClassAndId(Map> pdMap, + Class readType, Class writeType) { + + assertThat(pdMap).containsOnlyKeys("class", "id"); + assertReadAndWriteMethodsForClass(pdMap.get("class")); + assertReadAndWriteMethodsForId(pdMap.get("id"), readType, writeType); + } + + private static void assertReadAndWriteMethodsForClass(List pds) { + assertThat(pds).hasSize(1); + var pd = pds.get(0); + assertThat(pd.getName()).isEqualTo("class"); + + var readMethod = pd.getReadMethod(); + assertThat(readMethod.getName()).isEqualTo("getClass"); + assertThat(readMethod.getReturnType()).as("read type").isEqualTo(Class.class); + assertThat(readMethod.getParameterCount()).isZero(); + + assertThat(pd.getWriteMethod()).as("write method").isNull(); + } + + private static void assertReadAndWriteMethodsForId(List pds, Class readType, Class writeType) { + assertThat(pds).hasSize(1); + var pd = pds.get(0); + assertThat(pd.getName()).isEqualTo("id"); + + var readMethod = pd.getReadMethod(); + var writeMethod = pd.getWriteMethod(); + + assertSoftly(softly -> { + softly.assertThat(readMethod.getName()).isEqualTo("getId"); + softly.assertThat(readMethod.getReturnType()).as("read type").isEqualTo(readType); + softly.assertThat(readMethod.getParameterCount()).isZero(); + + softly.assertThat(writeMethod).as("write method").isNotNull(); + if (writeMethod != null) { + softly.assertThat(writeMethod.getName()).isEqualTo("setId"); + softly.assertThat(writeMethod.getReturnType()).isEqualTo(void.class); + softly.assertThat(writeMethod.getParameterCount()).isEqualTo(1); + softly.assertThat(writeMethod.getParameterTypes()[0]).as("write type").isEqualTo(writeType); + } + }); + } + + private static Map> toMap(Stream stream) { + return stream.collect(groupingBy(PropertyDescriptor::getName, toList())); + } + + + interface PropertiesResolver { + + Map> resolve(Class beanClass); + } + + static class BasicPropertiesResolver implements PropertiesResolver { + + @Override + public Map> resolve(Class beanClass) { + try { + var pds = PropertyDescriptorUtils.determineBasicProperties(beanClass); + return toMap(pds.stream()); + } + catch (Exception ex) { + throw new RuntimeException(ex); + } + } + } + + static class StandardPropertiesResolver implements PropertiesResolver { + + @Override + public Map> resolve(Class beanClass) { + try { + var beanInfo = Introspector.getBeanInfo(beanClass); + return toMap(Arrays.stream(beanInfo.getPropertyDescriptors())); + } + catch (Exception ex) { + throw new RuntimeException(ex); + } + } + } + + interface GenericService { + + void setId(T id); + + T getId(); + } + + interface SubGenericService extends GenericService { + } + + static class BaseService { + + private T id; + + public T getId() { + return id; + } + + public void setId(T id) { + this.id = id; + } + } + + static class ServiceWithOverriddenGetterAndSetter extends BaseService + implements SubGenericService { + + @Override + public String getId() { + return super.getId(); + } + + @Override + public void setId(String id) { + super.setId(id); + } + } + + static class ServiceWithOverloadedSetter extends BaseService + implements SubGenericService { + + public void setId(int id) { + setId(String.valueOf(id)); + } + } + + static class ServiceWithOverriddenGetter extends BaseService + implements SubGenericService { + + @Override + public String getId() { + return super.getId(); + } + } + + static class ServiceWithOverriddenGetterAndOverloadedSetter extends BaseService + implements SubGenericService { + + @Override + public String getId() { + return super.getId(); + } + + public void setId(int id) { + setId(String.valueOf(id)); + } + } + +}