Skip to content

Commit 71bd2a5

Browse files
dmitrysulmandmitrysulman
committed
Fix value class return handling for suspending AOP methods
Unbox Kotlin value class results returned from proxied suspending methods before returning them to the caller. Spring AOP interceptor chains expose return values as Object, which causes Kotlin value class results to be boxed. For suspending functions, the direct return path expects the unboxed value class representation. Update both CglibAopProxy and JdkDynamicAopProxy to detect value class return types and unbox boxed results after coroutine adaptation. Add tests for suspending methods returning value classes. See #37155 Signed-off-by: Dmitry Sulman <dmitry.sulman@gmail.com>
1 parent 91eb426 commit 71bd2a5

4 files changed

Lines changed: 323 additions & 6 deletions

File tree

‎spring-aop/src/main/java/org/springframework/aop/framework/CglibAopProxy.java‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
package org.springframework.aop.framework;
1818

1919
import java.io.Serializable;
20+
import java.lang.reflect.InvocationTargetException;
2021
import java.lang.reflect.Method;
2122
import java.lang.reflect.Modifier;
2223
import java.lang.reflect.UndeclaredThrowableException;
@@ -421,7 +422,8 @@ private static boolean implementsInterface(Method method, Set<Class<?>> ifcs) {
421422
* Also takes care of the conversion from {@code Mono} to Kotlin Coroutines if needed.
422423
*/
423424
private static @Nullable Object processReturnType(
424-
Object proxy, @Nullable Object target, Method method, Object[] arguments, @Nullable Object returnValue) {
425+
Object proxy, @Nullable Object target, Method method, Object[] arguments, @Nullable Object returnValue) throws
426+
NoSuchMethodException, InvocationTargetException, IllegalAccessException {
425427

426428
// Massage return value if necessary
427429
if (returnValue != null && returnValue == target &&
@@ -436,9 +438,14 @@ private static boolean implementsInterface(Method method, Set<Class<?>> ifcs) {
436438
"Null return value from advice does not match primitive return type for: " + method);
437439
}
438440
if (COROUTINES_REACTOR_PRESENT && KotlinDetector.isSuspendingFunction(method)) {
439-
return COROUTINES_FLOW_CLASS_NAME.equals(new MethodParameter(method, -1).getParameterType().getName()) ?
440-
CoroutinesUtils.asFlow(returnValue) :
441-
CoroutinesUtils.awaitSingleOrNull(returnValue, arguments[arguments.length - 1]);
441+
Class<?> returnParameterType = new MethodParameter(method, -1).getParameterType();
442+
if (COROUTINES_FLOW_CLASS_NAME.equals(returnParameterType.getName())) {
443+
return CoroutinesUtils.asFlow(returnValue);
444+
}
445+
Object awaitResult = CoroutinesUtils.awaitSingleOrNull(returnValue, arguments[arguments.length - 1]);
446+
return KotlinDetector.isInlineClass(returnParameterType) &&
447+
awaitResult != null && KotlinDetector.isInlineClass(awaitResult.getClass()) ?
448+
awaitResult.getClass().getDeclaredMethod("unbox-impl").invoke(awaitResult) : awaitResult;
442449
}
443450
return returnValue;
444451
}

‎spring-aop/src/main/java/org/springframework/aop/framework/JdkDynamicAopProxy.java‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -237,8 +237,14 @@ else if (retVal == null && returnType != void.class && returnType.isPrimitive())
237237
"Null return value from advice does not match primitive return type for: " + method);
238238
}
239239
if (COROUTINES_REACTOR_PRESENT && KotlinDetector.isSuspendingFunction(method)) {
240-
return COROUTINES_FLOW_CLASS_NAME.equals(new MethodParameter(method, -1).getParameterType().getName()) ?
241-
CoroutinesUtils.asFlow(retVal) : CoroutinesUtils.awaitSingleOrNull(retVal, args[args.length - 1]);
240+
Class<?> returnParameterType = new MethodParameter(method, -1).getParameterType();
241+
if (COROUTINES_FLOW_CLASS_NAME.equals(returnParameterType.getName())) {
242+
return CoroutinesUtils.asFlow(retVal);
243+
}
244+
Object awaitResult = CoroutinesUtils.awaitSingleOrNull(retVal, args[args.length - 1]);
245+
return KotlinDetector.isInlineClass(returnParameterType) &&
246+
awaitResult != null && KotlinDetector.isInlineClass(awaitResult.getClass()) ?
247+
awaitResult.getClass().getDeclaredMethod("unbox-impl").invoke(awaitResult) : awaitResult;
242248
}
243249
return retVal;
244250
}

‎spring-aop/src/test/kotlin/org/springframework/aop/framework/CglibAopProxyKotlinTests.kt‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,13 @@
1616

1717
package org.springframework.aop.framework
1818

19+
import kotlinx.coroutines.delay
20+
import org.aopalliance.intercept.MethodInterceptor
1921
import org.assertj.core.api.Assertions.assertThat
2022
import org.assertj.core.api.Assertions.assertThatThrownBy
2123
import org.junit.jupiter.api.Test
2224
import java.time.LocalDateTime
25+
import kotlin.time.Duration.Companion.milliseconds
2326

2427
/**
2528
* Tests for Kotlin support in [CglibAopProxy].
@@ -56,6 +59,95 @@ class CglibAopProxyKotlinTests {
5659
proxyFactory.proxy
5760
}
5861

62+
@Test
63+
suspend fun proxiedSuspendedInvocationValueClass() {
64+
val proxyFactory = ProxyFactory(TestBean())
65+
proxyFactory.addAdvice(MethodInterceptor {
66+
ValueClass("bar")
67+
})
68+
val proxy = proxyFactory.proxy as TestBean
69+
assertThat(proxy.returnValueClass()).isEqualTo(ValueClass("bar"))
70+
}
71+
72+
@Test
73+
suspend fun proxiedSuspendedInvocationValueClassProceed() {
74+
val proxyFactory = ProxyFactory(TestBean())
75+
proxyFactory.addAdvice(MethodInterceptor {
76+
it.proceed()
77+
})
78+
val proxy = proxyFactory.proxy as TestBean
79+
assertThat(proxy.returnValueClass()).isEqualTo(ValueClass("foo"))
80+
}
81+
82+
@Test
83+
suspend fun proxiedSuspendedInvocationNullableValueClass() {
84+
val proxyFactory = ProxyFactory(TestBean())
85+
proxyFactory.addAdvice(MethodInterceptor {
86+
ValueClass("bar")
87+
})
88+
val proxy = proxyFactory.proxy as TestBean
89+
assertThat(proxy.returnNullableValueClass()).isEqualTo(ValueClass("bar"))
90+
}
91+
92+
@Test
93+
suspend fun proxiedSuspendedInvocationNullableValueClassNull() {
94+
val proxyFactory = ProxyFactory(TestBean())
95+
proxyFactory.addAdvice(MethodInterceptor {
96+
null
97+
})
98+
val proxy = proxyFactory.proxy as TestBean
99+
assertThat(proxy.returnNullableValueClass()).isNull()
100+
}
101+
102+
@Test
103+
suspend fun proxiedSuspendedInvocationValueClassNullableValue() {
104+
val proxyFactory = ProxyFactory(TestBean())
105+
proxyFactory.addAdvice(MethodInterceptor {
106+
ValueClassNullableValue("bar")
107+
})
108+
val proxy = proxyFactory.proxy as TestBean
109+
assertThat(proxy.returnValueClassNullableValue()).isEqualTo(ValueClassNullableValue("bar"))
110+
}
111+
112+
@Test
113+
suspend fun proxiedSuspendedInvocationValueClassNullableValueNull() {
114+
val proxyFactory = ProxyFactory(TestBean())
115+
proxyFactory.addAdvice(MethodInterceptor {
116+
ValueClassNullableValue(null)
117+
})
118+
val proxy = proxyFactory.proxy as TestBean
119+
assertThat(proxy.returnValueClassNullableValue()).isEqualTo(ValueClassNullableValue(null))
120+
}
121+
122+
@Test
123+
suspend fun proxiedSuspendedInvocationResult() {
124+
val proxyFactory = ProxyFactory(TestBean())
125+
proxyFactory.addAdvice(MethodInterceptor {
126+
Result.success("bar")
127+
})
128+
val proxy = proxyFactory.proxy as TestBean
129+
assertThat(proxy.returnResult().getOrNull()).isEqualTo("bar")
130+
}
131+
132+
@Test
133+
suspend fun proxiedSuspendedInvocationString() {
134+
val proxyFactory = ProxyFactory(TestBean())
135+
proxyFactory.addAdvice(MethodInterceptor {
136+
"bar"
137+
})
138+
val proxy = proxyFactory.proxy as TestBean
139+
assertThat(proxy.returnString()).isEqualTo("bar")
140+
}
141+
142+
@Test
143+
suspend fun proxiedSuspendedInvocationAnyAdviceReturnValueClass() {
144+
val proxyFactory = ProxyFactory(TestBean())
145+
proxyFactory.addAdvice(MethodInterceptor {
146+
ValueClass("bar")
147+
})
148+
val proxy = proxyFactory.proxy as TestBean
149+
assertThat(proxy.returnAny()).isEqualTo(ValueClass("bar"))
150+
}
59151

60152
open class MyKotlinBean {
61153

@@ -91,4 +183,42 @@ class CglibAopProxyKotlinTests {
91183
val updatedAt: LocalDateTime? = null,
92184
)
93185

186+
@JvmInline
187+
value class ValueClass(val value: String)
188+
189+
@JvmInline
190+
value class ValueClassNullableValue(val value: String?)
191+
192+
open class TestBean {
193+
open suspend fun returnValueClass(): ValueClass {
194+
delay(1000.milliseconds)
195+
return ValueClass("foo")
196+
}
197+
198+
open suspend fun returnNullableValueClass(): ValueClass? {
199+
delay(1000.milliseconds)
200+
return null
201+
}
202+
203+
open suspend fun returnValueClassNullableValue(): ValueClassNullableValue {
204+
delay(1000.milliseconds)
205+
return ValueClassNullableValue(null)
206+
}
207+
208+
open suspend fun returnResult(): Result<String> {
209+
delay(1000.milliseconds)
210+
return Result.success("foo")
211+
}
212+
213+
open suspend fun returnString(): String {
214+
delay(1000.milliseconds)
215+
return "foo"
216+
}
217+
218+
open suspend fun returnAny(): Any {
219+
delay(1000.milliseconds)
220+
return ValueClass("foo")
221+
}
222+
}
223+
94224
}

‎spring-aop/src/test/kotlin/org/springframework/aop/framework/JdkDynamicAopProxyKotlinTests.kt‎

Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,174 @@
1+
/*
2+
* Copyright 2002-present the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.aop.framework
18+
19+
import kotlinx.coroutines.delay
20+
import org.aopalliance.intercept.MethodInterceptor
21+
import org.assertj.core.api.Assertions.assertThat
22+
import org.junit.jupiter.api.Test
23+
import kotlin.time.Duration.Companion.milliseconds
24+
25+
/**
26+
* Tests for Kotlin support in [JdkDynamicAopProxy].
27+
*
28+
* @author Dmitry Sulman
29+
*/
30+
class JdkDynamicAopProxyKotlinTests {
31+
32+
@Test
33+
suspend fun proxiedSuspendedInvocationValueClass() {
34+
val proxyFactory = ProxyFactory(TestBeanImpl())
35+
proxyFactory.addAdvice(MethodInterceptor {
36+
ValueClass("bar")
37+
})
38+
val proxy = proxyFactory.proxy as TestBean
39+
assertThat(proxy.returnValueClass()).isEqualTo(ValueClass("bar"))
40+
}
41+
42+
@Test
43+
suspend fun proxiedSuspendedInvocationValueClassProceed() {
44+
val proxyFactory = ProxyFactory(TestBeanImpl())
45+
proxyFactory.addAdvice(MethodInterceptor {
46+
it.proceed()
47+
})
48+
val proxy = proxyFactory.proxy as TestBean
49+
assertThat(proxy.returnValueClass()).isEqualTo(ValueClass("foo"))
50+
}
51+
52+
@Test
53+
suspend fun proxiedSuspendedInvocationNullableValueClass() {
54+
val proxyFactory = ProxyFactory(TestBeanImpl())
55+
proxyFactory.addAdvice(MethodInterceptor {
56+
ValueClass("bar")
57+
})
58+
val proxy = proxyFactory.proxy as TestBean
59+
assertThat(proxy.returnNullableValueClass()).isEqualTo(ValueClass("bar"))
60+
}
61+
62+
@Test
63+
suspend fun proxiedSuspendedInvocationNullableValueClassNull() {
64+
val proxyFactory = ProxyFactory(TestBeanImpl())
65+
proxyFactory.addAdvice(MethodInterceptor {
66+
null
67+
})
68+
val proxy = proxyFactory.proxy as TestBean
69+
assertThat(proxy.returnNullableValueClass()).isNull()
70+
}
71+
72+
@Test
73+
suspend fun proxiedSuspendedInvocationValueClassNullableValue() {
74+
val proxyFactory = ProxyFactory(TestBeanImpl())
75+
proxyFactory.addAdvice(MethodInterceptor {
76+
ValueClassNullableValue("bar")
77+
})
78+
val proxy = proxyFactory.proxy as TestBean
79+
assertThat(proxy.returnValueClassNullableValue()).isEqualTo(ValueClassNullableValue("bar"))
80+
}
81+
82+
@Test
83+
suspend fun proxiedSuspendedInvocationValueClassNullableValueNull() {
84+
val proxyFactory = ProxyFactory(TestBeanImpl())
85+
proxyFactory.addAdvice(MethodInterceptor {
86+
ValueClassNullableValue(null)
87+
})
88+
val proxy = proxyFactory.proxy as TestBean
89+
assertThat(proxy.returnValueClassNullableValue()).isEqualTo(ValueClassNullableValue(null))
90+
}
91+
92+
@Test
93+
suspend fun proxiedSuspendedInvocationResult() {
94+
val proxyFactory = ProxyFactory(TestBeanImpl())
95+
proxyFactory.addAdvice(MethodInterceptor {
96+
Result.success("bar")
97+
})
98+
val proxy = proxyFactory.proxy as TestBean
99+
assertThat(proxy.returnResult().getOrNull()).isEqualTo("bar")
100+
}
101+
102+
@Test
103+
suspend fun proxiedSuspendedInvocationString() {
104+
val proxyFactory = ProxyFactory(TestBeanImpl())
105+
proxyFactory.addAdvice(MethodInterceptor {
106+
"bar"
107+
})
108+
val proxy = proxyFactory.proxy as TestBean
109+
assertThat(proxy.returnString()).isEqualTo("bar")
110+
}
111+
112+
@Test
113+
suspend fun proxiedSuspendedInvocationAnyAdviceReturnValueClass() {
114+
val proxyFactory = ProxyFactory(TestBeanImpl())
115+
proxyFactory.addAdvice(MethodInterceptor {
116+
ValueClass("bar")
117+
})
118+
val proxy = proxyFactory.proxy as TestBean
119+
assertThat(proxy.returnAny()).isEqualTo(ValueClass("bar"))
120+
}
121+
122+
@JvmInline
123+
value class ValueClass(val value: String)
124+
125+
@JvmInline
126+
value class ValueClassNullableValue(val value: String?)
127+
128+
interface TestBean {
129+
suspend fun returnValueClass(): ValueClass
130+
131+
suspend fun returnNullableValueClass(): ValueClass?
132+
133+
suspend fun returnValueClassNullableValue(): ValueClassNullableValue
134+
135+
suspend fun returnResult(): Result<String>
136+
137+
suspend fun returnString(): String
138+
139+
suspend fun returnAny(): Any
140+
}
141+
142+
class TestBeanImpl : TestBean {
143+
override suspend fun returnValueClass(): ValueClass {
144+
delay(1000.milliseconds)
145+
return ValueClass("foo")
146+
}
147+
148+
override suspend fun returnNullableValueClass(): ValueClass? {
149+
delay(1000.milliseconds)
150+
return null
151+
}
152+
153+
override suspend fun returnValueClassNullableValue(): ValueClassNullableValue {
154+
delay(1000.milliseconds)
155+
return ValueClassNullableValue(null)
156+
}
157+
158+
override suspend fun returnResult(): Result<String> {
159+
delay(1000.milliseconds)
160+
return Result.success("foo")
161+
}
162+
163+
override suspend fun returnString(): String {
164+
delay(1000.milliseconds)
165+
return "foo"
166+
}
167+
168+
override suspend fun returnAny(): Any {
169+
delay(1000.milliseconds)
170+
return ValueClass("foo")
171+
}
172+
}
173+
174+
}

0 commit comments

Comments
 (0)