Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consistent handling of undeclared checked exceptions in CGLIB proxies #32469

Merged
merged 3 commits into from
Apr 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import java.io.Serializable;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.lang.reflect.UndeclaredThrowableException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -54,7 +53,6 @@
import org.springframework.util.ClassUtils;
import org.springframework.util.CollectionUtils;
import org.springframework.util.ObjectUtils;
import org.springframework.util.ReflectionUtils;

/**
* CGLIB-based {@link AopProxy} implementation for the Spring AOP framework.
Expand Down Expand Up @@ -761,27 +759,7 @@ public CglibMethodInvocation(Object proxy, @Nullable Object target, Method metho
@Override
@Nullable
public Object proceed() throws Throwable {
try {
return super.proceed();
}
catch (RuntimeException ex) {
throw ex;
}
catch (Exception ex) {
if (ReflectionUtils.declaresException(getMethod(), ex.getClass()) ||
KotlinDetector.isKotlinType(getMethod().getDeclaringClass())) {
// Propagate original exception if declared on the target method
// (with callers expecting it). Always propagate it for Kotlin code
// since checked exceptions do not have to be explicitly declared there.
throw ex;
}
else {
// Checked exception thrown in the interceptor but not declared on the
// target method signature -> apply an UndeclaredThrowableException,
// aligned with standard JDK dynamic proxy behavior.
throw new UndeclaredThrowableException(ex);
}
}
}
}

Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
/*
* Copyright 2002-2024 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.aop.framework;

import java.io.Serial;
import java.lang.reflect.Proxy;
import java.lang.reflect.UndeclaredThrowableException;
import java.util.Objects;

import org.aopalliance.intercept.MethodInterceptor;
import org.assertj.core.api.WithAssertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.mockito.Mockito;
import org.mockito.stubbing.Answer;

import org.springframework.cglib.proxy.Enhancer;
import org.springframework.lang.Nullable;

import static org.mockito.BDDMockito.doAnswer;
import static org.mockito.BDDMockito.doThrow;
import static org.mockito.BDDMockito.mock;

abstract class ProxyExceptionHandlingTests implements WithAssertions {

private static final RuntimeException uncheckedException = new RuntimeException();
private static final DeclaredCheckedException declaredCheckedException = new DeclaredCheckedException();
private static final UndeclaredCheckedException undeclaredCheckedException = new UndeclaredCheckedException();

protected final MyClass target = mock(MyClass.class);
protected final ProxyFactory proxyFactory = new ProxyFactory(target);

@Nullable
protected MyInterface proxy;
@Nullable
private Throwable throwableSeenByCaller;

static class ObjenesisCglibAopProxyTests extends ProxyExceptionHandlingTests {
@BeforeEach
void beforeEach() {
proxyFactory.setProxyTargetClass(true);
}

@Override
protected void assertProxyType(Object proxy) {
assertThat(Enhancer.isEnhanced(proxy.getClass())).isTrue();
}
}

static class JdkAopProxyTests extends ProxyExceptionHandlingTests {
@Override
protected void assertProxyType(Object proxy) {
assertThat(Proxy.isProxyClass(proxy.getClass())).isTrue();
}
}

protected void assertProxyType(Object proxy) {
}

@BeforeEach
void beforeEach() {
Mockito.clearInvocations(target);
}

@Nested
class WhenThereIsOneInterceptor {

@Nullable
private Throwable throwableSeenByInterceptor;

@BeforeEach
void beforeEach() {
proxyFactory.addAdvice(captureThrowable());

proxy = (MyInterface) proxyFactory.getProxy();

assertProxyType(proxy);
}

@Test
void targetThrowsUndeclaredCheckedException() throws DeclaredCheckedException {
doAnswer(sneakyThrow(undeclaredCheckedException)).when(target).doSomething();

invokeProxy();

assertThat(throwableSeenByInterceptor).isSameAs(undeclaredCheckedException);
assertThat(throwableSeenByCaller)
.isInstanceOf(UndeclaredThrowableException.class)
.hasCauseReference(undeclaredCheckedException);
}

@Test
void targetThrowsDeclaredCheckedException() throws DeclaredCheckedException {
doThrow(declaredCheckedException).when(target).doSomething();

invokeProxy();

assertThat(throwableSeenByInterceptor).isSameAs(declaredCheckedException);
assertThat(throwableSeenByCaller).isSameAs(declaredCheckedException);
}

@Test
void targetThrowsUncheckedException() throws DeclaredCheckedException {
doThrow(uncheckedException).when(target).doSomething();

invokeProxy();

assertThat(throwableSeenByInterceptor).isSameAs(uncheckedException);
assertThat(throwableSeenByCaller).isSameAs(uncheckedException);
}

private MethodInterceptor captureThrowable() {
return invocation -> {
try {
return invocation.proceed();
}
catch (Exception e) {
throwableSeenByInterceptor = e;
throw e;
}
};
}
}

@Nested
class WhenThereAreNoInterceptors {

@BeforeEach
void beforeEach() {
proxy = (MyInterface) proxyFactory.getProxy();

assertProxyType(proxy);
}

@Test
void targetThrowsUndeclaredCheckedException() throws DeclaredCheckedException {
doAnswer(sneakyThrow(undeclaredCheckedException)).when(target).doSomething();

invokeProxy();

assertThat(throwableSeenByCaller)
.isInstanceOf(UndeclaredThrowableException.class)
.hasCauseReference(undeclaredCheckedException);
}

@Test
void targetThrowsDeclaredCheckedException() throws DeclaredCheckedException {
doThrow(declaredCheckedException).when(target).doSomething();

invokeProxy();

assertThat(throwableSeenByCaller).isSameAs(declaredCheckedException);
}

@Test
void targetThrowsUncheckedException() throws DeclaredCheckedException {
doThrow(uncheckedException).when(target).doSomething();

invokeProxy();

assertThat(throwableSeenByCaller).isSameAs(uncheckedException);
}
}

private void invokeProxy() {
throwableSeenByCaller = catchThrowable(() -> Objects.requireNonNull(proxy).doSomething());
}

private Answer<?> sneakyThrow(@SuppressWarnings("SameParameterValue") Throwable throwable) {
return invocation -> {
throw throwable;
};
}

static class MyClass implements MyInterface {
@Override
public void doSomething() throws DeclaredCheckedException {
throw declaredCheckedException;
}
}

protected interface MyInterface {
void doSomething() throws DeclaredCheckedException;
}

protected static class UndeclaredCheckedException extends Exception {
@Serial
private static final long serialVersionUID = -4199611059122356651L;
}

protected static class DeclaredCheckedException extends Exception {
@Serial
private static final long serialVersionUID = 8851375818059230518L;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@

package org.springframework.cglib.core;

import java.lang.reflect.UndeclaredThrowableException;

import org.springframework.cglib.transform.impl.UndeclaredThrowableStrategy;

/**
* CGLIB GeneratorStrategy variant which exposes the application ClassLoader
* as current thread context ClassLoader for the time of class generation.
Expand All @@ -25,11 +29,12 @@
* @author Juergen Hoeller
* @since 5.2
*/
public class ClassLoaderAwareGeneratorStrategy extends DefaultGeneratorStrategy {
public class ClassLoaderAwareGeneratorStrategy extends UndeclaredThrowableStrategy {

private final ClassLoader classLoader;

public ClassLoaderAwareGeneratorStrategy(ClassLoader classLoader) {
super(UndeclaredThrowableException.class);
this.classLoader = classLoader;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import org.springframework.cglib.core.TypeUtils;
import org.springframework.cglib.transform.ClassEmitterTransformer;

@SuppressWarnings({"rawtypes", "unchecked"})
@SuppressWarnings({"rawtypes" })
public class UndeclaredThrowableTransformer extends ClassEmitterTransformer {

private final Type wrapper;
Expand Down Expand Up @@ -55,10 +55,20 @@ public CodeEmitter begin_method(int access, final Signature sig, final Type[] ex
return e;
}
return new CodeEmitter(e) {
private Block handler;
/* init */ {
handler = begin_block();
}
private final boolean isConstructor = Constants.CONSTRUCTOR_NAME.equals(sig.getName());
private Block handler = begin_block();
private boolean calToSuperWasSeen;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo perhaps in cal?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm preparing a revision of this to separate ClassLoaderAwareGeneratorStrategy from an UndeclaredThrowableStrategy delegate, to be committed soon. Noticed this only already, renaming it to callToSuperSeen.


@Override
public void visitMethodInsn(int opcode, String owner, String name, String descriptor, boolean isInterface) {
super.visitMethodInsn(opcode, owner, name, descriptor, isInterface);
if (isConstructor && !calToSuperWasSeen && Constants.CONSTRUCTOR_NAME.equals(name)) {
// we start the entry in the exception table after the call to super
handler = begin_block();
calToSuperWasSeen = true;
}
}

@Override
public void visitMaxs(int maxStack, int maxLocals) {
handler.end();
Expand Down