Skip to content

Commit

Permalink
Fix reflection exceptions caused by having identically named fallback…
Browse files Browse the repository at this point in the history 
…/blockHandler with different parameter types (#3395)
  • Loading branch information
@dowenliu-xyz
dowenliu-xyz authored May 29, 2024
1 parent bb813dd commit 568e2df
Show file tree
Hide file tree
Showing 9 changed files with 180 additions and 51 deletions.
8 changes: 7 additions & 1 deletion ...src/main/java/com/alibaba/csp/sentinel/demo/annotation/aop/controller/DemoController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,20 @@ public class DemoController {
private TestService service;

@GetMapping("/foo")
public String apiFoo(@RequestParam(required = false) Long t) throws Exception {
public String apiFoo(@RequestParam(required = false) Long t) {
if (t == null) {
t = System.currentTimeMillis();
}
service.test();
return service.hello(t);
}

@GetMapping("/bar")
public String apiBar(@RequestParam(required = false) String t) {
service.test();
return service.hello(t);
}

@GetMapping("/baz/{name}")
public String apiBaz(@PathVariable("name") String name) {
return service.helloAnother(name);
Expand Down
2 changes: 2 additions & 0 deletions ...g-aop/src/main/java/com/alibaba/csp/sentinel/demo/annotation/aop/service/TestService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,7 @@ public interface TestService {

String hello(long s);

String hello(String s);

String helloAnother(String name);
}
17 changes: 15 additions & 2 deletions ...p/src/main/java/com/alibaba/csp/sentinel/demo/annotation/aop/service/TestServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
package com.alibaba.csp.sentinel.demo.annotation.aop.service;

import com.alibaba.csp.sentinel.annotation.SentinelResource;
import com.alibaba.csp.sentinel.slots.block.BlockException;

import org.springframework.stereotype.Service;

/**
Expand All @@ -41,6 +39,15 @@ public String hello(long s) {
return String.format("Hello at %d", s);
}

@Override
@SentinelResource(value = "helloStr", fallback = "helloFallback")
public String hello(String s) {
if (s == null || s.trim().isEmpty()) {
throw new IllegalArgumentException("unknown");
}
return String.format("Hello, %s", s);
}

@Override
@SentinelResource(value = "helloAnother", defaultFallback = "defaultFallback",
exceptionsToIgnore = {IllegalStateException.class})
Expand All @@ -60,6 +67,12 @@ public String helloFallback(long s, Throwable ex) {
return "Oops, error occurred at " + s;
}

private String helloFallback(String ignored, Throwable e) {
// Do some log here.
e.printStackTrace();
return "Hello, stranger";
}

public String defaultFallback() {
System.out.println("Go to default fallback");
return "default_fallback";
Expand Down
23 changes: 11 additions & 12 deletions .../main/java/com/alibaba/csp/sentinel/annotation/aspectj/AbstractSentinelAspectSupport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
*
* @author Eric Zhao
* @author zhaoyuguang
* @author dowenliu-xyz([email protected])
*/
public abstract class AbstractSentinelAspectSupport {

Expand Down Expand Up @@ -177,12 +178,13 @@ private Method extractFallbackMethod(ProceedingJoinPoint pjp, String fallbackNam
}
boolean mustStatic = locationClass != null && locationClass.length >= 1;
Class<?> clazz = mustStatic ? locationClass[0] : pjp.getTarget().getClass();
MethodWrapper m = ResourceMetadataRegistry.lookupFallback(clazz, fallbackName);
Method originMethod = resolveMethod(pjp);
MethodWrapper m = ResourceMetadataRegistry.lookupFallback(clazz, fallbackName, originMethod.getParameterTypes());
if (m == null) {
// First time, resolve the fallback.
Method method = resolveFallbackInternal(pjp, fallbackName, clazz, mustStatic);
Method method = resolveFallbackInternal(originMethod, fallbackName, clazz, mustStatic);
// Cache the method instance.
ResourceMetadataRegistry.updateFallbackFor(clazz, fallbackName, method);
ResourceMetadataRegistry.updateFallbackFor(clazz, fallbackName, originMethod.getParameterTypes(), method);
return method;
}
if (!m.isPresent()) {
Expand Down Expand Up @@ -232,9 +234,7 @@ private Method extractDefaultFallbackMethod(ProceedingJoinPoint pjp, String defa
return m.getMethod();
}

private Method resolveFallbackInternal(ProceedingJoinPoint pjp, /*@NonNull*/ String name, Class<?> clazz,
boolean mustStatic) {
Method originMethod = resolveMethod(pjp);
private Method resolveFallbackInternal(Method originMethod, String name, Class<?> clazz, boolean mustStatic) {
// Fallback function allows two kinds of parameter list.
Class<?>[] defaultParamTypes = originMethod.getParameterTypes();
Class<?>[] paramTypesWithException = Arrays.copyOf(defaultParamTypes, defaultParamTypes.length + 1);
Expand All @@ -261,12 +261,13 @@ private Method extractBlockHandlerMethod(ProceedingJoinPoint pjp, String name, C
// By default current class.
clazz = pjp.getTarget().getClass();
}
MethodWrapper m = ResourceMetadataRegistry.lookupBlockHandler(clazz, name);
Method originMethod = resolveMethod(pjp);
MethodWrapper m = ResourceMetadataRegistry.lookupBlockHandler(clazz, name, originMethod.getParameterTypes());
if (m == null) {
// First time, resolve the block handler.
Method method = resolveBlockHandlerInternal(pjp, name, clazz, mustStatic);
Method method = resolveBlockHandlerInternal(originMethod, name, clazz, mustStatic);
// Cache the method instance.
ResourceMetadataRegistry.updateBlockHandlerFor(clazz, name, method);
ResourceMetadataRegistry.updateBlockHandlerFor(clazz, name, originMethod.getParameterTypes(), method);
return method;
}
if (!m.isPresent()) {
Expand All @@ -275,9 +276,7 @@ private Method extractBlockHandlerMethod(ProceedingJoinPoint pjp, String name, C
return m.getMethod();
}

private Method resolveBlockHandlerInternal(ProceedingJoinPoint pjp, /*@NonNull*/ String name, Class<?> clazz,
boolean mustStatic) {
Method originMethod = resolveMethod(pjp);
private Method resolveBlockHandlerInternal(Method originMethod, String name, Class<?> clazz, boolean mustStatic) {
Class<?>[] originList = originMethod.getParameterTypes();
Class<?>[] parameterTypes = Arrays.copyOf(originList, originList.length + 1);
parameterTypes[parameterTypes.length - 1] = BlockException.class;
Expand Down
52 changes: 52 additions & 0 deletions ...j/src/main/java/com/alibaba/csp/sentinel/annotation/aspectj/ResourceMetadataRegistry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,59 +16,111 @@
package com.alibaba.csp.sentinel.annotation.aspectj;

import java.lang.reflect.Method;
import java.util.Arrays;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.stream.Collectors;

import com.alibaba.csp.sentinel.util.StringUtil;

/**
* Registry for resource configuration metadata (e.g. fallback method)
*
* @author Eric Zhao
* @author dowenliu-xyz([email protected])
*/
final class ResourceMetadataRegistry {

private static final Map<String, MethodWrapper> FALLBACK_MAP = new ConcurrentHashMap<>();
private static final Map<String, MethodWrapper> DEFAULT_FALLBACK_MAP = new ConcurrentHashMap<>();
private static final Map<String, MethodWrapper> BLOCK_HANDLER_MAP = new ConcurrentHashMap<>();

/**
* @deprecated use {@link #lookupFallback(Class, String, Class[])}
*/
@Deprecated
static MethodWrapper lookupFallback(Class<?> clazz, String name) {
return FALLBACK_MAP.get(getKey(clazz, name));
}

static MethodWrapper lookupFallback(Class<?> clazz, String name, Class<?>[] parameterTypes) {
return FALLBACK_MAP.get(getKey(clazz, name, parameterTypes));
}

static MethodWrapper lookupDefaultFallback(Class<?> clazz, String name) {
return DEFAULT_FALLBACK_MAP.get(getKey(clazz, name));
}

/**
* @deprecated use {@link #lookupBlockHandler(Class, String, Class[])}
*/
@Deprecated
static MethodWrapper lookupBlockHandler(Class<?> clazz, String name) {
return BLOCK_HANDLER_MAP.get(getKey(clazz, name));
}

static MethodWrapper lookupBlockHandler(Class<?> clazz, String name, Class<?>[] parameterTypes) {
return BLOCK_HANDLER_MAP.get(getKey(clazz, name, parameterTypes));
}

/**
* @deprecated use {@link #updateFallbackFor(Class, String, Class[], Method)}
*/
@Deprecated
static void updateFallbackFor(Class<?> clazz, String name, Method method) {
if (clazz == null || StringUtil.isBlank(name)) {
throw new IllegalArgumentException("Bad argument");
}
FALLBACK_MAP.put(getKey(clazz, name), MethodWrapper.wrap(method));
}

static void updateFallbackFor(Class<?> clazz, String handlerName, Class<?>[] parameterTypes, Method handlerMethod) {
if (clazz == null || StringUtil.isBlank(handlerName)) {
throw new IllegalArgumentException("Bad argument");
}
FALLBACK_MAP.put(getKey(clazz, handlerName, parameterTypes), MethodWrapper.wrap(handlerMethod));
}

static void updateDefaultFallbackFor(Class<?> clazz, String name, Method method) {
if (clazz == null || StringUtil.isBlank(name)) {
throw new IllegalArgumentException("Bad argument");
}
DEFAULT_FALLBACK_MAP.put(getKey(clazz, name), MethodWrapper.wrap(method));
}


/**
* @deprecated use {@link #updateBlockHandlerFor(Class, String, Class[], Method)}
*/
@Deprecated
static void updateBlockHandlerFor(Class<?> clazz, String name, Method method) {
if (clazz == null || StringUtil.isBlank(name)) {
throw new IllegalArgumentException("Bad argument");
}
BLOCK_HANDLER_MAP.put(getKey(clazz, name), MethodWrapper.wrap(method));
}

static void updateBlockHandlerFor(Class<?> clazz, String handlerName, Class<?>[] parameterTypes,
Method handlerMethod) {
if (clazz == null || StringUtil.isBlank(handlerName)) {
throw new IllegalArgumentException("Bad argument");
}
BLOCK_HANDLER_MAP.put(getKey(clazz, handlerName, parameterTypes), MethodWrapper.wrap(handlerMethod));
}

private static String getKey(Class<?> clazz, String name) {
return String.format("%s:%s", clazz.getCanonicalName(), name);
}

private static String getKey(Class<?> clazz, String name, Class<?>[] parameterTypes) {
return String.format(
"%s:%s;%s",
clazz.getCanonicalName(),
name,
Arrays.stream(parameterTypes).map(Class::getCanonicalName).collect(Collectors.joining(","))
);
}

/**
* Only for internal test.
*/
Expand Down
27 changes: 15 additions & 12 deletions ...c/test/java/com/alibaba/csp/sentinel/annotation/aspectj/ResourceMetadataRegistryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

/**
* @author Eric Zhao
* @author dowenliu-xyz([email protected])
*/
public class ResourceMetadataRegistryTest {

Expand All @@ -45,14 +46,15 @@ public void tearDown() throws Exception {
public void testUpdateThenLookupFallback() {
Class<?> clazz = FooService.class;
String methodName = "someMethodFallback";
Class<?>[] parameterTypes = new Class<?>[]{String.class, int.class};
Method method = clazz.getMethods()[0];
assertThat(ResourceMetadataRegistry.lookupFallback(clazz, methodName)).isNull();
assertThat(ResourceMetadataRegistry.lookupFallback(clazz, methodName, parameterTypes)).isNull();

ResourceMetadataRegistry.updateFallbackFor(clazz, methodName, null);
assertThat(ResourceMetadataRegistry.lookupFallback(clazz, methodName).isPresent()).isFalse();
ResourceMetadataRegistry.updateFallbackFor(clazz, methodName, parameterTypes, null);
assertThat(ResourceMetadataRegistry.lookupFallback(clazz, methodName, parameterTypes).isPresent()).isFalse();

ResourceMetadataRegistry.updateFallbackFor(clazz, methodName, method);
MethodWrapper wrapper = ResourceMetadataRegistry.lookupFallback(clazz, methodName);
ResourceMetadataRegistry.updateFallbackFor(clazz, methodName, parameterTypes, method);
MethodWrapper wrapper = ResourceMetadataRegistry.lookupFallback(clazz, methodName, parameterTypes);
assertThat(wrapper.isPresent()).isTrue();
assertThat(wrapper.getMethod()).isSameAs(method);
}
Expand All @@ -61,25 +63,26 @@ public void testUpdateThenLookupFallback() {
public void testUpdateThenLookupBlockHandler() {
Class<?> clazz = FooService.class;
String methodName = "someMethodBlockHand;er";
Class<?>[] parameterTypes = new Class<?>[]{String.class, int.class};
Method method = clazz.getMethods()[1];
assertThat(ResourceMetadataRegistry.lookupBlockHandler(clazz, methodName)).isNull();
assertThat(ResourceMetadataRegistry.lookupBlockHandler(clazz, methodName, parameterTypes)).isNull();

ResourceMetadataRegistry.updateBlockHandlerFor(clazz, methodName, null);
assertThat(ResourceMetadataRegistry.lookupBlockHandler(clazz, methodName).isPresent()).isFalse();
ResourceMetadataRegistry.updateBlockHandlerFor(clazz, methodName, parameterTypes, null);
assertThat(ResourceMetadataRegistry.lookupBlockHandler(clazz, methodName, parameterTypes).isPresent()).isFalse();

ResourceMetadataRegistry.updateBlockHandlerFor(clazz, methodName, method);
MethodWrapper wrapper = ResourceMetadataRegistry.lookupBlockHandler(clazz, methodName);
ResourceMetadataRegistry.updateBlockHandlerFor(clazz, methodName, parameterTypes, method);
MethodWrapper wrapper = ResourceMetadataRegistry.lookupBlockHandler(clazz, methodName, parameterTypes);
assertThat(wrapper.isPresent()).isTrue();
assertThat(wrapper.getMethod()).isSameAs(method);
}

@Test(expected = IllegalArgumentException.class)
public void testUpdateBlockHandlerBadArgument() {
ResourceMetadataRegistry.updateBlockHandlerFor(null, "sxs", String.class.getMethods()[0]);
ResourceMetadataRegistry.updateBlockHandlerFor(null, "sxs", new Class<?>[]{}, String.class.getMethods()[0]);
}

@Test(expected = IllegalArgumentException.class)
public void testUpdateFallbackBadArgument() {
ResourceMetadataRegistry.updateBlockHandlerFor(String.class, "", String.class.getMethods()[0]);
ResourceMetadataRegistry.updateBlockHandlerFor(String.class, "", new Class[0], String.class.getMethods()[0]);
}
}
23 changes: 11 additions & 12 deletions ...m/alibaba/csp/sentinel/annotation/cdi/interceptor/AbstractSentinelInterceptorSupport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
*
* @author Eric Zhao
* @author seasidesky
* @author dowenliu-xyz([email protected])
*/
public abstract class AbstractSentinelInterceptorSupport {

Expand Down Expand Up @@ -170,12 +171,13 @@ private Method extractFallbackMethod(InvocationContext ctx, String fallbackName,
}
boolean mustStatic = locationClass != null && locationClass.length >= 1;
Class<?> clazz = mustStatic ? locationClass[0] : ctx.getTarget().getClass();
MethodWrapper m = ResourceMetadataRegistry.lookupFallback(clazz, fallbackName);
Method originMethod = resolveMethod(ctx);
MethodWrapper m = ResourceMetadataRegistry.lookupFallback(clazz, fallbackName, originMethod.getParameterTypes());
if (m == null) {
// First time, resolve the fallback.
Method method = resolveFallbackInternal(ctx, fallbackName, clazz, mustStatic);
Method method = resolveFallbackInternal(originMethod, fallbackName, clazz, mustStatic);
// Cache the method instance.
ResourceMetadataRegistry.updateFallbackFor(clazz, fallbackName, method);
ResourceMetadataRegistry.updateFallbackFor(clazz, fallbackName, originMethod.getParameterTypes(), method);
return method;
}
if (!m.isPresent()) {
Expand Down Expand Up @@ -225,9 +227,7 @@ private Method extractDefaultFallbackMethod(InvocationContext ctx, String defaul
return m.getMethod();
}

private Method resolveFallbackInternal(InvocationContext ctx, /*@NonNull*/ String name, Class<?> clazz,
boolean mustStatic) {
Method originMethod = resolveMethod(ctx);
private Method resolveFallbackInternal(Method originMethod, String name, Class<?> clazz, boolean mustStatic) {
// Fallback function allows two kinds of parameter list.
Class<?>[] defaultParamTypes = originMethod.getParameterTypes();
Class<?>[] paramTypesWithException = Arrays.copyOf(defaultParamTypes, defaultParamTypes.length + 1);
Expand All @@ -254,12 +254,13 @@ private Method extractBlockHandlerMethod(InvocationContext ctx, String name, Cla
// By default current class.
clazz = ctx.getTarget().getClass();
}
MethodWrapper m = ResourceMetadataRegistry.lookupBlockHandler(clazz, name);
Method originMethod = resolveMethod(ctx);
MethodWrapper m = ResourceMetadataRegistry.lookupBlockHandler(clazz, name, originMethod.getParameterTypes());
if (m == null) {
// First time, resolve the block handler.
Method method = resolveBlockHandlerInternal(ctx, name, clazz, mustStatic);
Method method = resolveBlockHandlerInternal(originMethod, name, clazz, mustStatic);
// Cache the method instance.
ResourceMetadataRegistry.updateBlockHandlerFor(clazz, name, method);
ResourceMetadataRegistry.updateBlockHandlerFor(clazz, name, originMethod.getParameterTypes(), method);
return method;
}
if (!m.isPresent()) {
Expand All @@ -268,9 +269,7 @@ private Method extractBlockHandlerMethod(InvocationContext ctx, String name, Cla
return m.getMethod();
}

private Method resolveBlockHandlerInternal(InvocationContext ctx, /*@NonNull*/ String name, Class<?> clazz,
boolean mustStatic) {
Method originMethod = resolveMethod(ctx);
private Method resolveBlockHandlerInternal(Method originMethod, String name, Class<?> clazz, boolean mustStatic) {
Class<?>[] originList = originMethod.getParameterTypes();
Class<?>[] parameterTypes = Arrays.copyOf(originList, originList.length + 1);
parameterTypes[parameterTypes.length - 1] = BlockException.class;
Expand Down
Loading

0 comments on commit 568e2df

Please sign in to comment.