Skip to content

Commit 5f87412

Browse files
mp911demp911de
committed
Consider only instance with methods for property access.
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
1 parent 6eeff22 commit 5f87412

5 files changed

Lines changed: 108 additions & 51 deletions

File tree

‎src/main/java/org/springframework/data/mapping/PersistentProperty.java‎

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,13 @@ public interface PersistentProperty<P extends PersistentProperty<P>> {
8686
@Nullable
8787
Method getGetter();
8888

89+
/**
90+
* Returns the required getter method to access the property value or throw {@link IllegalStateException} if no getter
91+
* method is available.
92+
*
93+
* @return the getter method to access the property value.
94+
* @throws IllegalStateException if no getter method available.
95+
*/
8996
default Method getRequiredGetter() {
9097

9198
Method getter = getGetter();
@@ -106,6 +113,14 @@ default Method getRequiredGetter() {
106113
@Nullable
107114
Method getSetter();
108115

116+
/**
117+
* Returns the required setter method to set a property value or throw {@link IllegalStateException} if no setter
118+
* method is available.
119+
*
120+
* @return the setter method to set a property value.
121+
* @throws IllegalStateException if no setter method available.
122+
* @see #getSetter()
123+
*/
109124
default Method getRequiredSetter() {
110125

111126
Method setter = getSetter();
@@ -118,11 +133,11 @@ default Method getRequiredSetter() {
118133
}
119134

120135
/**
121-
* Returns the with {@link Method} to set a property value on a new object instance. Might return {@literal null} in
122-
* case there is no with available.
136+
* Returns the {@code with} {@link Method} to set a property value on a new object instance. Might return
137+
* {@literal null} in case there is no {@code with} available.
123138
* <p>
124-
* With {@link Method methods} are property-bound instance {@link Method methods} that accept a single argument of the
125-
* property type creating a new object instance.
139+
* {@code with} {@link Method methods} are property-bound instance {@link Method methods} that accept a single
140+
* argument of the property type creating a new object instance.
126141
*
127142
* <pre class="code">
128143
* class Person {
@@ -144,6 +159,14 @@ default Method getRequiredSetter() {
144159
@Nullable
145160
Method getWither();
146161

162+
/**
163+
* Returns the required {@code with} {@link Method} to set a property value on a new object instance or throw
164+
* {@link IllegalStateException} if no wither method is available.
165+
*
166+
* @return the with {@link Method} to set a property value on and return a new object instance.
167+
* @throws IllegalStateException if no with {@code with} {@link Method} available.
168+
* @see #getWither()
169+
*/
147170
default Method getRequiredWither() {
148171

149172
Method wither = getWither();
@@ -155,9 +178,20 @@ default Method getRequiredWither() {
155178
return wither;
156179
}
157180

181+
/**
182+
* Returns the backing {@link Field} if available. Might return {@literal null} in case there is no backing field.
183+
*
184+
* @return the backing {@link Field} if available, otherwise {@literal null}.
185+
*/
158186
@Nullable
159187
Field getField();
160188

189+
/**
190+
* Returns the required backing {@link Field} or throw {@link IllegalStateException} if no backing field is available.
191+
*
192+
* @return the required backing {@link Field}.
193+
* @throws IllegalStateException if no backing field available.
194+
*/
161195
default Field getRequiredField() {
162196

163197
Field field = getField();
@@ -182,7 +216,7 @@ default Field getRequiredField() {
182216
Association<P> getAssociation();
183217

184218
/**
185-
* Get the {@link Association} of this property.
219+
* Get the requires {@link Association} of this property.
186220
*
187221
* @return never {@literal null}.
188222
* @throws IllegalStateException if not involved in an {@link Association}.

‎src/main/java/org/springframework/data/mapping/model/ClassGeneratingPropertyAccessorFactory.java‎

Lines changed: 28 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import java.util.function.Function;
4141

4242
import org.jspecify.annotations.Nullable;
43+
4344
import org.springframework.asm.ClassWriter;
4445
import org.springframework.asm.Label;
4546
import org.springframework.asm.MethodVisitor;
@@ -1018,11 +1019,7 @@ private static void visitSetPropertySwitch(PersistentEntity<?, ?> entity,
10181019
mv.visitLabel(propertyStackAddress.label);
10191020
mv.visitFrame(Opcodes.F_SAME, 0, null, 0, null);
10201021

1021-
if (supportsMutation(property)) {
1022-
visitSetProperty0(entity, property, mv, internalClassName);
1023-
} else {
1024-
mv.visitJumpInsn(GOTO, dfltLabel);
1025-
}
1022+
visitSetProperty0(entity, property, mv, internalClassName, () -> mv.visitJumpInsn(GOTO, dfltLabel));
10261023
}
10271024

10281025
mv.visitLabel(dfltLabel);
@@ -1035,7 +1032,7 @@ private static void visitSetPropertySwitch(PersistentEntity<?, ?> entity,
10351032
* called as if the method had the expected signature and not array/varargs.
10361033
*/
10371034
private static void visitSetProperty0(PersistentEntity<?, ?> entity, PersistentProperty<?> property,
1038-
MethodVisitor mv, String internalClassName) {
1035+
MethodVisitor mv, String internalClassName, Runnable onImmutable) {
10391036

10401037
Method setter = property.getSetter();
10411038
Method wither = property.getWither();
@@ -1044,12 +1041,12 @@ private static void visitSetProperty0(PersistentEntity<?, ?> entity, PersistentP
10441041

10451042
if (wither != null) {
10461043
visitWithProperty(entity, property, mv, internalClassName, wither);
1047-
}
1048-
1049-
if (KotlinDetector.isKotlinType(entity.getType()) && KotlinCopyMethod.hasKotlinCopyMethodFor(property)) {
1044+
} else if (KotlinDetector.isKotlinType(entity.getType()) && KotlinCopyMethod.hasKotlinCopyMethodFor(property)) {
10501045
visitKotlinCopy(entity, property, mv, internalClassName);
1046+
} else {
1047+
onImmutable.run();
1048+
return;
10511049
}
1052-
10531050
} else if (property.usePropertyAccess() && setter != null) {
10541051
visitSetProperty(entity, property, mv, internalClassName, setter);
10551052
} else {
@@ -1225,30 +1222,29 @@ private static void visitSetProperty(PersistentEntity<?, ?> entity, PersistentPr
12251222
private static void visitSetField(PersistentEntity<?, ?> entity, PersistentProperty<?> property, MethodVisitor mv,
12261223
String internalClassName) {
12271224

1228-
Field field = property.getField();
1229-
if (field != null) {
1230-
if (generateSetterMethodHandle(entity, field)) {
1231-
// $fieldSetter.invoke(this.bean, object)
1232-
mv.visitFieldInsn(GETSTATIC, internalClassName, fieldSetterName(property),
1233-
referenceName(JAVA_LANG_INVOKE_METHOD_HANDLE));
1234-
mv.visitVarInsn(ALOAD, 0);
1235-
mv.visitFieldInsn(GETFIELD, internalClassName, BEAN_FIELD, getAccessibleTypeReferenceName(entity));
1236-
mv.visitVarInsn(ALOAD, 2);
1237-
mv.visitMethodInsn(INVOKEVIRTUAL, JAVA_LANG_INVOKE_METHOD_HANDLE, "invoke",
1238-
String.format("(%s%s)V", referenceName(JAVA_LANG_OBJECT), referenceName(JAVA_LANG_OBJECT)), false);
1239-
} else {
1240-
// this.bean.field
1241-
mv.visitVarInsn(ALOAD, 0);
1242-
mv.visitFieldInsn(GETFIELD, internalClassName, BEAN_FIELD, getAccessibleTypeReferenceName(entity));
1243-
mv.visitVarInsn(ALOAD, 2);
1225+
Field field = property.getRequiredField();
12441226

1245-
Class<?> fieldType = field.getType();
1227+
if (generateSetterMethodHandle(entity, field)) {
1228+
// $fieldSetter.invoke(this.bean, object)
1229+
mv.visitFieldInsn(GETSTATIC, internalClassName, fieldSetterName(property),
1230+
referenceName(JAVA_LANG_INVOKE_METHOD_HANDLE));
1231+
mv.visitVarInsn(ALOAD, 0);
1232+
mv.visitFieldInsn(GETFIELD, internalClassName, BEAN_FIELD, getAccessibleTypeReferenceName(entity));
1233+
mv.visitVarInsn(ALOAD, 2);
1234+
mv.visitMethodInsn(INVOKEVIRTUAL, JAVA_LANG_INVOKE_METHOD_HANDLE, "invoke",
1235+
String.format("(%s%s)V", referenceName(JAVA_LANG_OBJECT), referenceName(JAVA_LANG_OBJECT)), false);
1236+
} else {
1237+
// this.bean.field
1238+
mv.visitVarInsn(ALOAD, 0);
1239+
mv.visitFieldInsn(GETFIELD, internalClassName, BEAN_FIELD, getAccessibleTypeReferenceName(entity));
1240+
mv.visitVarInsn(ALOAD, 2);
12461241

1247-
mv.visitTypeInsn(CHECKCAST, Type.getInternalName(autoboxType(fieldType)));
1248-
autoboxIfNeeded(autoboxType(fieldType), fieldType, mv);
1249-
mv.visitFieldInsn(PUTFIELD, Type.getInternalName(field.getDeclaringClass()), field.getName(),
1250-
signatureTypeName(fieldType));
1251-
}
1242+
Class<?> fieldType = field.getType();
1243+
1244+
mv.visitTypeInsn(CHECKCAST, Type.getInternalName(autoboxType(fieldType)));
1245+
autoboxIfNeeded(autoboxType(fieldType), fieldType, mv);
1246+
mv.visitFieldInsn(PUTFIELD, Type.getInternalName(field.getDeclaringClass()), field.getName(),
1247+
signatureTypeName(fieldType));
12521248
}
12531249
}
12541250

‎src/main/java/org/springframework/data/mapping/model/Property.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import java.beans.PropertyDescriptor;
2020
import java.lang.reflect.Field;
2121
import java.lang.reflect.Method;
22+
import java.lang.reflect.Modifier;
2223
import java.util.Optional;
2324
import java.util.concurrent.atomic.AtomicReference;
2425
import java.util.function.Function;
@@ -273,7 +274,7 @@ private static Optional<Method> findWither(TypeInformation<?> owner, String prop
273274
if (owner.isAssignableFrom(owner.getReturnType(it))) {
274275
resultHolder.set(it);
275276
}
276-
}, it -> isMethodWithSingleParameterOfType(it, methodName, rawType));
277+
}, it -> isMethodWithSingleParameterOfType(it, methodName, rawType) && !Modifier.isStatic(it.getModifiers()));
277278

278279
Method method = resultHolder.get();
279280
return method != null ? Optional.of(method) : Optional.empty();

‎src/test/java/org/springframework/data/mapping/model/ClassGeneratingPropertyAccessorFactoryEntityTypeTests.java‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import java.time.LocalDateTime;
2222

2323
import org.junit.jupiter.api.Test;
24+
2425
import org.springframework.data.annotation.Id;
2526
import org.springframework.data.mapping.PersistentEntity;
2627
import org.springframework.data.mapping.context.SampleMappingContext;
@@ -55,7 +56,7 @@ void getIdentifierOfClassBasedEntity() {
5556
assertThat(getEntityInformation(Person.class).getId(jonDoe)).isEqualTo(jonDoe.name);
5657
}
5758

58-
@Test // #2324
59+
@Test // GH-2324
5960
void shouldGeneratePropertyAccessorForKotlinClassWithMultipleCopyMethods() {
6061

6162
var factory = new ClassGeneratingPropertyAccessorFactory();
@@ -66,6 +67,16 @@ void shouldGeneratePropertyAccessorForKotlinClassWithMultipleCopyMethods() {
6667
assertThat(propertyAccessor).isNotNull();
6768
}
6869

70+
@Test // GH-3472
71+
void shouldIgnoreStaticWithMethod() {
72+
73+
var factory = new ClassGeneratingPropertyAccessorFactory();
74+
var propertyAccessor = factory.getPropertyAccessor(mappingContext.getRequiredPersistentEntity(WithStaticWith.class),
75+
new WithStaticWith(null));
76+
77+
assertThat(propertyAccessor).isNotNull();
78+
}
79+
6980
private EntityInformation<Object, Serializable> getEntityInformation(Class<?> type) {
7081

7182
PersistentEntity<Object, SamplePersistentProperty> entity = mappingContext.getRequiredPersistentEntity(type);
@@ -94,4 +105,11 @@ static class Person {
94105
this.name = name;
95106
}
96107
}
108+
109+
public record WithStaticWith(String string) {
110+
111+
public static WithStaticWith withString(String string) {
112+
return new WithStaticWith(string);
113+
}
114+
}
97115
}

‎src/test/java/org/springframework/data/mapping/model/PropertyUnitTests.java‎

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import static org.assertj.core.api.Assertions.*;
1919

2020
import org.junit.jupiter.api.Test;
21+
2122
import org.springframework.data.core.TypeInformation;
2223
import org.springframework.util.ReflectionUtils;
2324

@@ -28,21 +29,18 @@
2829
*/
2930
class PropertyUnitTests {
3031

31-
@Test // DATACMNS-1322
32+
@Test // DATACMNS-1322, GH-3472
3233
void shouldNotFindWitherMethod() {
3334

34-
assertThat(Property
35-
.of(TypeInformation.of(ImmutableType.class), ReflectionUtils.findField(ImmutableType.class, "id")).getWither())
36-
.isEmpty();
37-
assertThat(
38-
Property.of(TypeInformation.of(ImmutableType.class), ReflectionUtils.findField(ImmutableType.class, "name"))
39-
.getWither()).isEmpty();
35+
assertThat(getProperty(ImmutableType.class, "id").getWither()).isEmpty();
36+
assertThat(getProperty(ImmutableType.class, "name").getWither()).isEmpty();
37+
assertThat(getProperty(StaticWither.class, "someField").getWither()).isEmpty();
4038
}
4139

4240
@Test // DATACMNS-1322
4341
void shouldDiscoverWitherMethod() {
4442

45-
var property = Property.of(TypeInformation.of(WitherType.class), ReflectionUtils.findField(WitherType.class, "id"));
43+
var property = getProperty(WitherType.class, "id");
4644

4745
assertThat(property.getWither()).isPresent().hasValueSatisfying(actual -> {
4846
assertThat(actual.getName()).isEqualTo("withId");
@@ -53,8 +51,7 @@ void shouldDiscoverWitherMethod() {
5351
@Test // DATACMNS-1421
5452
void shouldDiscoverDerivedWitherMethod() {
5553

56-
var property = Property.of(TypeInformation.of(DerivedWitherClass.class),
57-
ReflectionUtils.findField(DerivedWitherClass.class, "id"));
54+
var property = getProperty(DerivedWitherClass.class, "id");
5855

5956
assertThat(property.getWither()).isPresent().hasValueSatisfying(actual -> {
6057
assertThat(actual.getName()).isEqualTo("withId");
@@ -66,12 +63,15 @@ void shouldDiscoverDerivedWitherMethod() {
6663
@Test // DATACMNS-1421
6764
void shouldNotDiscoverWitherMethodWithIncompatibleReturnType() {
6865

69-
var property = Property.of(TypeInformation.of(AnotherLevel.class),
70-
ReflectionUtils.findField(AnotherLevel.class, "id"));
66+
var property = getProperty(AnotherLevel.class, "id");
7167

7268
assertThat(property.getWither()).isEmpty();
7369
}
7470

71+
private static Property getProperty(Class<?> type, String fieldName) {
72+
return Property.of(TypeInformation.of(type), ReflectionUtils.findField(type, fieldName));
73+
}
74+
7575
static class ImmutableType {
7676

7777
final String id;
@@ -166,4 +166,12 @@ DerivedWitherClass withId(String id) {
166166
return new AnotherLevel(id);
167167
}
168168
}
169+
170+
public record StaticWither(String someField) {
171+
172+
public static StaticWither withSomeField(String someField) {
173+
return new StaticWither(someField);
174+
}
175+
}
176+
169177
}

0 commit comments

Comments
 (0)