From a7463df79130e99d3de9172e72b8ded3fecbf98f Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Tue, 17 Mar 2026 15:22:42 +0100 Subject: [PATCH] Consider only instance `with` methods for property access. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We now filter static `with…` methods from wither method introspection to avoid considering a static method a wither method. Previously, we did not check modifiers and so static methods could be considered withers. Closes #3472 --- .../data/mapping/PersistentProperty.java | 44 ++++++++++++-- ...lassGeneratingPropertyAccessorFactory.java | 60 +++++++++---------- .../data/mapping/model/Property.java | 3 +- ...ropertyAccessorFactoryEntityTypeTests.java | 20 ++++++- .../data/mapping/model/PropertyUnitTests.java | 32 ++++++---- 5 files changed, 108 insertions(+), 51 deletions(-) diff --git a/src/main/java/org/springframework/data/mapping/PersistentProperty.java b/src/main/java/org/springframework/data/mapping/PersistentProperty.java index e8afdca29..94aed0885 100644 --- a/src/main/java/org/springframework/data/mapping/PersistentProperty.java +++ b/src/main/java/org/springframework/data/mapping/PersistentProperty.java @@ -86,6 +86,13 @@ public interface PersistentProperty

> { @Nullable Method getGetter(); + /** + * Returns the required getter method to access the property value or throw {@link IllegalStateException} if no getter + * method is available. + * + * @return the getter method to access the property value. + * @throws IllegalStateException if no getter method available. + */ default Method getRequiredGetter() { Method getter = getGetter(); @@ -106,6 +113,14 @@ public interface PersistentProperty

> { @Nullable Method getSetter(); + /** + * Returns the required setter method to set a property value or throw {@link IllegalStateException} if no setter + * method is available. + * + * @return the setter method to set a property value. + * @throws IllegalStateException if no setter method available. + * @see #getSetter() + */ default Method getRequiredSetter() { Method setter = getSetter(); @@ -118,11 +133,11 @@ public interface PersistentProperty

> { } /** - * Returns the with {@link Method} to set a property value on a new object instance. Might return {@literal null} in - * case there is no with available. + * Returns the {@code with} {@link Method} to set a property value on a new object instance. Might return + * {@literal null} in case there is no {@code with} available. *

- * With {@link Method methods} are property-bound instance {@link Method methods} that accept a single argument of the - * property type creating a new object instance. + * {@code with} {@link Method methods} are property-bound instance {@link Method methods} that accept a single + * argument of the property type creating a new object instance. * *

 	 * class Person {
@@ -144,6 +159,14 @@ public interface PersistentProperty

> { @Nullable Method getWither(); + /** + * Returns the required {@code with} {@link Method} to set a property value on a new object instance or throw + * {@link IllegalStateException} if no wither method is available. + * + * @return the with {@link Method} to set a property value on and return a new object instance. + * @throws IllegalStateException if no with {@code with} {@link Method} available. + * @see #getWither() + */ default Method getRequiredWither() { Method wither = getWither(); @@ -155,9 +178,20 @@ public interface PersistentProperty

> { return wither; } + /** + * Returns the backing {@link Field} if available. Might return {@literal null} in case there is no backing field. + * + * @return the backing {@link Field} if available, otherwise {@literal null}. + */ @Nullable Field getField(); + /** + * Returns the required backing {@link Field} or throw {@link IllegalStateException} if no backing field is available. + * + * @return the required backing {@link Field}. + * @throws IllegalStateException if no backing field available. + */ default Field getRequiredField() { Field field = getField(); @@ -182,7 +216,7 @@ public interface PersistentProperty

> { Association

getAssociation(); /** - * Get the {@link Association} of this property. + * Get the requires {@link Association} of this property. * * @return never {@literal null}. * @throws IllegalStateException if not involved in an {@link Association}. diff --git a/src/main/java/org/springframework/data/mapping/model/ClassGeneratingPropertyAccessorFactory.java b/src/main/java/org/springframework/data/mapping/model/ClassGeneratingPropertyAccessorFactory.java index 2f45e5f65..1411b1da4 100644 --- a/src/main/java/org/springframework/data/mapping/model/ClassGeneratingPropertyAccessorFactory.java +++ b/src/main/java/org/springframework/data/mapping/model/ClassGeneratingPropertyAccessorFactory.java @@ -40,6 +40,7 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Function; import org.jspecify.annotations.Nullable; + import org.springframework.asm.ClassWriter; import org.springframework.asm.Label; import org.springframework.asm.MethodVisitor; @@ -1018,11 +1019,7 @@ public class ClassGeneratingPropertyAccessorFactory implements PersistentPropert mv.visitLabel(propertyStackAddress.label); mv.visitFrame(Opcodes.F_SAME, 0, null, 0, null); - if (supportsMutation(property)) { - visitSetProperty0(entity, property, mv, internalClassName); - } else { - mv.visitJumpInsn(GOTO, dfltLabel); - } + visitSetProperty0(entity, property, mv, internalClassName, () -> mv.visitJumpInsn(GOTO, dfltLabel)); } mv.visitLabel(dfltLabel); @@ -1035,7 +1032,7 @@ public class ClassGeneratingPropertyAccessorFactory implements PersistentPropert * called as if the method had the expected signature and not array/varargs. */ private static void visitSetProperty0(PersistentEntity entity, PersistentProperty property, - MethodVisitor mv, String internalClassName) { + MethodVisitor mv, String internalClassName, Runnable onImmutable) { Method setter = property.getSetter(); Method wither = property.getWither(); @@ -1044,12 +1041,12 @@ public class ClassGeneratingPropertyAccessorFactory implements PersistentPropert if (wither != null) { visitWithProperty(entity, property, mv, internalClassName, wither); - } - - if (KotlinDetector.isKotlinType(entity.getType()) && KotlinCopyMethod.hasKotlinCopyMethodFor(property)) { + } else if (KotlinDetector.isKotlinType(entity.getType()) && KotlinCopyMethod.hasKotlinCopyMethodFor(property)) { visitKotlinCopy(entity, property, mv, internalClassName); + } else { + onImmutable.run(); + return; } - } else if (property.usePropertyAccess() && setter != null) { visitSetProperty(entity, property, mv, internalClassName, setter); } else { @@ -1225,30 +1222,29 @@ public class ClassGeneratingPropertyAccessorFactory implements PersistentPropert private static void visitSetField(PersistentEntity entity, PersistentProperty property, MethodVisitor mv, String internalClassName) { - Field field = property.getField(); - if (field != null) { - if (generateSetterMethodHandle(entity, field)) { - // $fieldSetter.invoke(this.bean, object) - mv.visitFieldInsn(GETSTATIC, internalClassName, fieldSetterName(property), - referenceName(JAVA_LANG_INVOKE_METHOD_HANDLE)); - mv.visitVarInsn(ALOAD, 0); - mv.visitFieldInsn(GETFIELD, internalClassName, BEAN_FIELD, getAccessibleTypeReferenceName(entity)); - mv.visitVarInsn(ALOAD, 2); - mv.visitMethodInsn(INVOKEVIRTUAL, JAVA_LANG_INVOKE_METHOD_HANDLE, "invoke", - String.format("(%s%s)V", referenceName(JAVA_LANG_OBJECT), referenceName(JAVA_LANG_OBJECT)), false); - } else { - // this.bean.field - mv.visitVarInsn(ALOAD, 0); - mv.visitFieldInsn(GETFIELD, internalClassName, BEAN_FIELD, getAccessibleTypeReferenceName(entity)); - mv.visitVarInsn(ALOAD, 2); + Field field = property.getRequiredField(); - Class fieldType = field.getType(); + if (generateSetterMethodHandle(entity, field)) { + // $fieldSetter.invoke(this.bean, object) + mv.visitFieldInsn(GETSTATIC, internalClassName, fieldSetterName(property), + referenceName(JAVA_LANG_INVOKE_METHOD_HANDLE)); + mv.visitVarInsn(ALOAD, 0); + mv.visitFieldInsn(GETFIELD, internalClassName, BEAN_FIELD, getAccessibleTypeReferenceName(entity)); + mv.visitVarInsn(ALOAD, 2); + mv.visitMethodInsn(INVOKEVIRTUAL, JAVA_LANG_INVOKE_METHOD_HANDLE, "invoke", + String.format("(%s%s)V", referenceName(JAVA_LANG_OBJECT), referenceName(JAVA_LANG_OBJECT)), false); + } else { + // this.bean.field + mv.visitVarInsn(ALOAD, 0); + mv.visitFieldInsn(GETFIELD, internalClassName, BEAN_FIELD, getAccessibleTypeReferenceName(entity)); + mv.visitVarInsn(ALOAD, 2); - mv.visitTypeInsn(CHECKCAST, Type.getInternalName(autoboxType(fieldType))); - autoboxIfNeeded(autoboxType(fieldType), fieldType, mv); - mv.visitFieldInsn(PUTFIELD, Type.getInternalName(field.getDeclaringClass()), field.getName(), - signatureTypeName(fieldType)); - } + Class fieldType = field.getType(); + + mv.visitTypeInsn(CHECKCAST, Type.getInternalName(autoboxType(fieldType))); + autoboxIfNeeded(autoboxType(fieldType), fieldType, mv); + mv.visitFieldInsn(PUTFIELD, Type.getInternalName(field.getDeclaringClass()), field.getName(), + signatureTypeName(fieldType)); } } diff --git a/src/main/java/org/springframework/data/mapping/model/Property.java b/src/main/java/org/springframework/data/mapping/model/Property.java index c9796aec2..7b7fbdfc0 100644 --- a/src/main/java/org/springframework/data/mapping/model/Property.java +++ b/src/main/java/org/springframework/data/mapping/model/Property.java @@ -19,6 +19,7 @@ import java.beans.FeatureDescriptor; import java.beans.PropertyDescriptor; import java.lang.reflect.Field; import java.lang.reflect.Method; +import java.lang.reflect.Modifier; import java.util.Optional; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; @@ -273,7 +274,7 @@ public class Property { if (owner.isAssignableFrom(owner.getReturnType(it))) { resultHolder.set(it); } - }, it -> isMethodWithSingleParameterOfType(it, methodName, rawType)); + }, it -> isMethodWithSingleParameterOfType(it, methodName, rawType) && !Modifier.isStatic(it.getModifiers())); Method method = resultHolder.get(); return method != null ? Optional.of(method) : Optional.empty(); diff --git a/src/test/java/org/springframework/data/mapping/model/ClassGeneratingPropertyAccessorFactoryEntityTypeTests.java b/src/test/java/org/springframework/data/mapping/model/ClassGeneratingPropertyAccessorFactoryEntityTypeTests.java index c3f650258..711af037a 100755 --- a/src/test/java/org/springframework/data/mapping/model/ClassGeneratingPropertyAccessorFactoryEntityTypeTests.java +++ b/src/test/java/org/springframework/data/mapping/model/ClassGeneratingPropertyAccessorFactoryEntityTypeTests.java @@ -21,6 +21,7 @@ import java.io.Serializable; import java.time.LocalDateTime; import org.junit.jupiter.api.Test; + import org.springframework.data.annotation.Id; import org.springframework.data.mapping.PersistentEntity; import org.springframework.data.mapping.context.SampleMappingContext; @@ -55,7 +56,7 @@ public class ClassGeneratingPropertyAccessorFactoryEntityTypeTests { assertThat(getEntityInformation(Person.class).getId(jonDoe)).isEqualTo(jonDoe.name); } - @Test // #2324 + @Test // GH-2324 void shouldGeneratePropertyAccessorForKotlinClassWithMultipleCopyMethods() { var factory = new ClassGeneratingPropertyAccessorFactory(); @@ -66,6 +67,16 @@ public class ClassGeneratingPropertyAccessorFactoryEntityTypeTests { assertThat(propertyAccessor).isNotNull(); } + @Test // GH-3472 + void shouldIgnoreStaticWithMethod() { + + var factory = new ClassGeneratingPropertyAccessorFactory(); + var propertyAccessor = factory.getPropertyAccessor(mappingContext.getRequiredPersistentEntity(WithStaticWith.class), + new WithStaticWith(null)); + + assertThat(propertyAccessor).isNotNull(); + } + private EntityInformation getEntityInformation(Class type) { PersistentEntity entity = mappingContext.getRequiredPersistentEntity(type); @@ -94,4 +105,11 @@ public class ClassGeneratingPropertyAccessorFactoryEntityTypeTests { this.name = name; } } + + public record WithStaticWith(String string) { + + public static WithStaticWith withString(String string) { + return new WithStaticWith(string); + } + } } diff --git a/src/test/java/org/springframework/data/mapping/model/PropertyUnitTests.java b/src/test/java/org/springframework/data/mapping/model/PropertyUnitTests.java index adbaf893f..31b258e9b 100644 --- a/src/test/java/org/springframework/data/mapping/model/PropertyUnitTests.java +++ b/src/test/java/org/springframework/data/mapping/model/PropertyUnitTests.java @@ -18,6 +18,7 @@ package org.springframework.data.mapping.model; import static org.assertj.core.api.Assertions.*; import org.junit.jupiter.api.Test; + import org.springframework.data.core.TypeInformation; import org.springframework.util.ReflectionUtils; @@ -28,21 +29,18 @@ import org.springframework.util.ReflectionUtils; */ class PropertyUnitTests { - @Test // DATACMNS-1322 + @Test // DATACMNS-1322, GH-3472 void shouldNotFindWitherMethod() { - assertThat(Property - .of(TypeInformation.of(ImmutableType.class), ReflectionUtils.findField(ImmutableType.class, "id")).getWither()) - .isEmpty(); - assertThat( - Property.of(TypeInformation.of(ImmutableType.class), ReflectionUtils.findField(ImmutableType.class, "name")) - .getWither()).isEmpty(); + assertThat(getProperty(ImmutableType.class, "id").getWither()).isEmpty(); + assertThat(getProperty(ImmutableType.class, "name").getWither()).isEmpty(); + assertThat(getProperty(StaticWither.class, "someField").getWither()).isEmpty(); } @Test // DATACMNS-1322 void shouldDiscoverWitherMethod() { - var property = Property.of(TypeInformation.of(WitherType.class), ReflectionUtils.findField(WitherType.class, "id")); + var property = getProperty(WitherType.class, "id"); assertThat(property.getWither()).isPresent().hasValueSatisfying(actual -> { assertThat(actual.getName()).isEqualTo("withId"); @@ -53,8 +51,7 @@ class PropertyUnitTests { @Test // DATACMNS-1421 void shouldDiscoverDerivedWitherMethod() { - var property = Property.of(TypeInformation.of(DerivedWitherClass.class), - ReflectionUtils.findField(DerivedWitherClass.class, "id")); + var property = getProperty(DerivedWitherClass.class, "id"); assertThat(property.getWither()).isPresent().hasValueSatisfying(actual -> { assertThat(actual.getName()).isEqualTo("withId"); @@ -66,12 +63,15 @@ class PropertyUnitTests { @Test // DATACMNS-1421 void shouldNotDiscoverWitherMethodWithIncompatibleReturnType() { - var property = Property.of(TypeInformation.of(AnotherLevel.class), - ReflectionUtils.findField(AnotherLevel.class, "id")); + var property = getProperty(AnotherLevel.class, "id"); assertThat(property.getWither()).isEmpty(); } + private static Property getProperty(Class type, String fieldName) { + return Property.of(TypeInformation.of(type), ReflectionUtils.findField(type, fieldName)); + } + static class ImmutableType { final String id; @@ -166,4 +166,12 @@ class PropertyUnitTests { return new AnotherLevel(id); } } + + public record StaticWither(String someField) { + + public static StaticWither withSomeField(String someField) { + return new StaticWither(someField); + } + } + }