Copilot commented on code in PR #6248:
URL: https://github.com/apache/shenyu/pull/6248#discussion_r2587881939
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-proxy-enhanced/src/main/java/org/apache/shenyu/plugin/ai/proxy/enhanced/AiProxyPlugin.java:
##########
@@ -89,25 +89,30 @@ protected Mono<Void> doExecute(
final ShenyuPluginChain chain,
final SelectorData selector,
final RuleData rule) {
- final AiProxyHandle selectorHandle =
- aiProxyPluginHandler
- .getSelectorCachedHandle()
- .obtainHandle(
- CacheKeyUtils.INST.getKey(
- selector.getId(),
Constants.DEFAULT_RULE));
+ final AiProxyHandle selectorHandle = aiProxyPluginHandler
+ .getSelectorCachedHandle()
+ .obtainHandle(
+ CacheKeyUtils.INST.getKey(
+ selector.getId(), Constants.DEFAULT_RULE));
+
+ long contentLength =
exchange.getRequest().getHeaders().getContentLength();
+ if (contentLength > 5 * 1024 * 1024) {
Review Comment:
The 5MB size limit is hardcoded with a magic number. This should be
extracted to a named constant (e.g., `MAX_REQUEST_BODY_SIZE_BYTES`) or made
configurable through plugin settings to allow different limits per deployment
or selector.
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-proxy-enhanced/src/main/java/org/apache/shenyu/plugin/ai/proxy/enhanced/AiProxyPlugin.java:
##########
@@ -147,22 +153,22 @@ private Mono<Void> handleStreamRequest(
final AiCommonConfig primaryConfig,
final AiProxyHandle selectorHandle) {
final ChatClient mainClient = createMainChatClient(selector.getId(),
primaryConfig);
- final Optional<ChatClient> fallbackClient =
- resolveFallbackClient(primaryConfig, selectorHandle,
selector.getId(), requestBody);
+ final Optional<ChatClient> fallbackClient =
resolveFallbackClient(primaryConfig, selectorHandle,
+ selector.getId(), requestBody);
+ final String prompt = aiProxyConfigService.extractPrompt(requestBody);
final ServerHttpResponse response = exchange.getResponse();
response.getHeaders().setContentType(MediaType.TEXT_EVENT_STREAM);
- final Flux<ChatResponse> chatResponseFlux =
- aiProxyExecutorService.executeStream(mainClient,
fallbackClient, requestBody);
+ final Flux<ChatResponse> chatResponseFlux =
aiProxyExecutorService.executeStream(mainClient, fallbackClient,
+ prompt);
Review Comment:
The prompt extraction happens after fallback client resolution, but the
fallback client resolution also needs the requestBody (line 156-157). This
creates a potential inconsistency: if the request contains a fallbackConfig,
the fallback client is resolved using the full requestBody, but the AI call is
made with the extracted prompt. This means the fallback configuration and the
actual prompt being sent might be from different parts of the request
structure. Consider whether the prompt should be extracted before or whether
both the main and fallback calls should use the same input.
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-proxy-enhanced/src/main/java/org/apache/shenyu/plugin/ai/proxy/enhanced/service/AiProxyConfigService.java:
##########
@@ -121,4 +124,30 @@ private Optional<AiCommonConfig>
extractDynamicFallbackConfig(final String reque
}
return Optional.empty();
}
+
+ /**
+ * Extract prompt from request body if it contains fallback config.
+ *
+ * @param requestBody the request body
+ * @return the extracted prompt or original body
+ */
+ public String extractPrompt(final String requestBody) {
+ if (Objects.isNull(requestBody) || requestBody.isEmpty()) {
+ return requestBody;
+ }
+ try {
+ JsonNode jsonNode = JsonUtils.toJsonNode(requestBody);
+ if (jsonNode.has(FALLBACK_CONFIG)) {
+ if (jsonNode.has("prompt")) {
+ return jsonNode.get("prompt").asText();
+ }
+ if (jsonNode.has("content")) {
+ return jsonNode.get("content").asText();
+ }
+ }
+ } catch (Exception e) {
+ // ignore
+ }
+ return requestBody;
+ }
Review Comment:
The new `extractPrompt` method lacks test coverage. Given that this method
involves JSON parsing with exception handling and conditional logic, tests
should be added to cover scenarios: (1) null/empty input, (2) valid JSON with
fallbackConfig and prompt, (3) valid JSON with fallbackConfig and content, (4)
valid JSON with fallbackConfig but neither prompt nor content, (5) valid JSON
without fallbackConfig, and (6) malformed JSON.
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-proxy-enhanced/src/main/java/org/apache/shenyu/plugin/ai/proxy/enhanced/AiProxyPlugin.java:
##########
@@ -174,15 +180,15 @@ private Mono<Void> handleNonStreamRequest(
final AiCommonConfig primaryConfig,
final AiProxyHandle selectorHandle) {
final ChatClient mainClient = createMainChatClient(selector.getId(),
primaryConfig);
- final Optional<ChatClient> fallbackClient =
- resolveFallbackClient(primaryConfig, selectorHandle,
selector.getId(), requestBody);
+ final Optional<ChatClient> fallbackClient =
resolveFallbackClient(primaryConfig, selectorHandle,
+ selector.getId(), requestBody);
+ final String prompt = aiProxyConfigService.extractPrompt(requestBody);
return aiProxyExecutorService
- .execute(mainClient, fallbackClient, requestBody)
+ .execute(mainClient, fallbackClient, prompt)
Review Comment:
Same issue as the stream request handler: the prompt is extracted from
requestBody after fallback client resolution, creating potential inconsistency
between the fallback configuration source and the prompt being executed.
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-proxy-enhanced/src/main/java/org/apache/shenyu/plugin/ai/proxy/enhanced/cache/ChatClientCache.java:
##########
@@ -30,30 +30,37 @@
* This is ChatClient cache.
*/
public final class ChatClientCache {
-
+
private static final Logger LOG =
LoggerFactory.getLogger(ChatClientCache.class);
-
+
+ private static final int MAX_CACHE_SIZE = 500;
Review Comment:
The MAX_CACHE_SIZE constant is hardcoded to 500 with no configuration
option. This should be externalized to a configuration property to allow
administrators to tune cache size based on their deployment needs and memory
constraints.
```suggestion
private static final int MAX_CACHE_SIZE = getCacheSize();
private static int getCacheSize() {
String value =
System.getProperty("shenyu.plugin.ai.proxy.enhanced.cache.maxSize",
System.getenv("SHENYU_PLUGIN_AI_PROXY_ENHANCED_CACHE_MAXSIZE"));
if (value != null) {
try {
return Integer.parseInt(value);
} catch (NumberFormatException e) {
LoggerFactory.getLogger(ChatClientCache.class)
.warn("[ChatClientCache] Invalid cache size '{}',
using default 500.", value);
}
}
return 500;
}
```
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-proxy-enhanced/src/main/java/org/apache/shenyu/plugin/ai/proxy/enhanced/cache/ChatClientCache.java:
##########
@@ -30,30 +30,37 @@
* This is ChatClient cache.
*/
public final class ChatClientCache {
-
+
private static final Logger LOG =
LoggerFactory.getLogger(ChatClientCache.class);
-
+
+ private static final int MAX_CACHE_SIZE = 500;
+
private final Map<String, ChatClient> chatClientMap = new
ConcurrentHashMap<>();
-
+
/**
* Instantiates a new Chat client cache.
*/
public ChatClientCache() {
}
-
+
/**
* Gets client or compute if absent.
*
- * @param key the key
+ * @param key the key
* @param chatModelSupplier the chat model supplier
* @return the chat client
*/
public ChatClient computeIfAbsent(final String key, final
Supplier<ChatModel> chatModelSupplier) {
+ if (chatClientMap.size() > MAX_CACHE_SIZE) {
+ LOG.warn("[ChatClientCache] Cache size exceeded {}, clearing
all.", MAX_CACHE_SIZE);
+ chatClientMap.clear();
+ }
Review Comment:
The cache eviction strategy has a critical race condition. Multiple
concurrent requests could check the size condition (line 54) simultaneously
before any clear operation completes, leading to unpredictable cache state.
Additionally, clearing the entire cache when the size limit is exceeded is too
aggressive and will cause a thundering herd problem where all subsequent
requests will need to recreate their clients simultaneously. Consider using a
proper LRU cache implementation or at least implement proper synchronization
and evict only the oldest entries.
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-proxy-enhanced/src/main/java/org/apache/shenyu/plugin/ai/proxy/enhanced/AiProxyPlugin.java:
##########
@@ -89,25 +89,30 @@ protected Mono<Void> doExecute(
final ShenyuPluginChain chain,
final SelectorData selector,
final RuleData rule) {
- final AiProxyHandle selectorHandle =
- aiProxyPluginHandler
- .getSelectorCachedHandle()
- .obtainHandle(
- CacheKeyUtils.INST.getKey(
- selector.getId(),
Constants.DEFAULT_RULE));
+ final AiProxyHandle selectorHandle = aiProxyPluginHandler
+ .getSelectorCachedHandle()
+ .obtainHandle(
+ CacheKeyUtils.INST.getKey(
+ selector.getId(), Constants.DEFAULT_RULE));
+
+ long contentLength =
exchange.getRequest().getHeaders().getContentLength();
+ if (contentLength > 5 * 1024 * 1024) {
+ exchange.getResponse().setStatusCode(HttpStatus.PAYLOAD_TOO_LARGE);
+ return exchange.getResponse().setComplete();
+ }
Review Comment:
The request body size limit check only validates the Content-Length header,
which can be spoofed or missing. An attacker could send a request without a
Content-Length header or with a false value to bypass this check. The actual
body size should be validated after reading it at line 106, or a configurable
reactive body size limit should be enforced at the gateway level.
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-proxy-enhanced/src/main/java/org/apache/shenyu/plugin/ai/proxy/enhanced/service/AiProxyConfigService.java:
##########
@@ -121,4 +124,30 @@ private Optional<AiCommonConfig>
extractDynamicFallbackConfig(final String reque
}
return Optional.empty();
}
+
+ /**
+ * Extract prompt from request body if it contains fallback config.
+ *
+ * @param requestBody the request body
+ * @return the extracted prompt or original body
+ */
+ public String extractPrompt(final String requestBody) {
+ if (Objects.isNull(requestBody) || requestBody.isEmpty()) {
+ return requestBody;
+ }
+ try {
+ JsonNode jsonNode = JsonUtils.toJsonNode(requestBody);
+ if (jsonNode.has(FALLBACK_CONFIG)) {
+ if (jsonNode.has("prompt")) {
+ return jsonNode.get("prompt").asText();
+ }
+ if (jsonNode.has("content")) {
+ return jsonNode.get("content").asText();
+ }
+ }
Review Comment:
The extractPrompt method has incorrect logic. It only extracts "prompt" or
"content" fields when a "fallbackConfig" field exists in the JSON, but this
doesn't make semantic sense. The presence of fallbackConfig shouldn't determine
whether to extract the prompt. The method should either always attempt to
extract the prompt from the standard fields, or the logic should be clarified
with proper documentation explaining why prompt extraction depends on
fallbackConfig presence.
```suggestion
if (jsonNode.has("prompt")) {
return jsonNode.get("prompt").asText();
}
if (jsonNode.has("content")) {
return jsonNode.get("content").asText();
}
```
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-proxy-enhanced/src/main/java/org/apache/shenyu/plugin/ai/proxy/enhanced/AiProxyPlugin.java:
##########
@@ -202,36 +208,36 @@ private Optional<ChatClient> resolveFallbackClient(
return createDynamicFallbackClient(cfg);
})
.or(
- () ->
- aiProxyConfigService
-
.resolveAdminFallbackConfig(primaryConfig, selectorHandle)
- .map(adminFallbackConfig -> {
- LOG.info("[AiProxy] use admin
fallback");
- if (LOG.isDebugEnabled()) {
- LOG.debug("[AiProxy] admin
fallback config: {}", adminFallbackConfig);
- }
- return
createAdminFallbackClient(selectorId, adminFallbackConfig);
- }));
+ () -> aiProxyConfigService
+ .resolveAdminFallbackConfig(primaryConfig,
selectorHandle)
+ .map(adminFallbackConfig -> {
+ LOG.info("[AiProxy] use admin fallback");
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("[AiProxy] admin fallback
config: {}", adminFallbackConfig);
+ }
+ return
createAdminFallbackClient(selectorId, adminFallbackConfig);
+ }));
}
private ChatClient createMainChatClient(final String selectorId, final
AiCommonConfig config) {
+ final String cacheKey = selectorId + "_" + config.hashCode();
Review Comment:
Cache invalidation is broken due to key format mismatch. The main client
cache key uses underscore separator (e.g., "selectorId_hashcode") at line 223,
but the ChatClientCache.remove() method at line 72 only removes keys that
either equal the selectorId exactly or start with "selectorId|". This means
main client cache entries will never be properly invalidated when the selector
is updated or removed, leading to memory leaks and stale client usage.
```suggestion
final String cacheKey = selectorId + "|" + config.hashCode();
```
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-proxy-enhanced/src/main/java/org/apache/shenyu/plugin/ai/proxy/enhanced/AiProxyPlugin.java:
##########
@@ -202,36 +208,36 @@ private Optional<ChatClient> resolveFallbackClient(
return createDynamicFallbackClient(cfg);
})
.or(
- () ->
- aiProxyConfigService
-
.resolveAdminFallbackConfig(primaryConfig, selectorHandle)
- .map(adminFallbackConfig -> {
- LOG.info("[AiProxy] use admin
fallback");
- if (LOG.isDebugEnabled()) {
- LOG.debug("[AiProxy] admin
fallback config: {}", adminFallbackConfig);
- }
- return
createAdminFallbackClient(selectorId, adminFallbackConfig);
- }));
+ () -> aiProxyConfigService
+ .resolveAdminFallbackConfig(primaryConfig,
selectorHandle)
+ .map(adminFallbackConfig -> {
+ LOG.info("[AiProxy] use admin fallback");
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("[AiProxy] admin fallback
config: {}", adminFallbackConfig);
+ }
+ return
createAdminFallbackClient(selectorId, adminFallbackConfig);
+ }));
}
private ChatClient createMainChatClient(final String selectorId, final
AiCommonConfig config) {
+ final String cacheKey = selectorId + "_" + config.hashCode();
Review Comment:
Using `config.hashCode()` in the cache key is problematic because it
includes the `apiKey` field in the hash calculation (as seen in
AiCommonConfig.hashCode()). When the apiKey is updated at runtime (e.g., via
proxy key replacement at line 127), the same config object will produce a
different hashCode, causing cache misses for what should be logically the same
configuration. This breaks the caching mechanism and can lead to resource leaks
as old cache entries are never removed. Consider creating a cache key based
only on immutable config fields (provider, model, baseUrl, temperature,
maxTokens, stream) or implement a proper config identifier.
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-proxy-enhanced/src/main/java/org/apache/shenyu/plugin/ai/proxy/enhanced/AiProxyPlugin.java:
##########
@@ -202,36 +208,36 @@ private Optional<ChatClient> resolveFallbackClient(
return createDynamicFallbackClient(cfg);
})
.or(
- () ->
- aiProxyConfigService
-
.resolveAdminFallbackConfig(primaryConfig, selectorHandle)
- .map(adminFallbackConfig -> {
- LOG.info("[AiProxy] use admin
fallback");
- if (LOG.isDebugEnabled()) {
- LOG.debug("[AiProxy] admin
fallback config: {}", adminFallbackConfig);
- }
- return
createAdminFallbackClient(selectorId, adminFallbackConfig);
- }));
+ () -> aiProxyConfigService
+ .resolveAdminFallbackConfig(primaryConfig,
selectorHandle)
+ .map(adminFallbackConfig -> {
+ LOG.info("[AiProxy] use admin fallback");
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("[AiProxy] admin fallback
config: {}", adminFallbackConfig);
+ }
+ return
createAdminFallbackClient(selectorId, adminFallbackConfig);
+ }));
}
private ChatClient createMainChatClient(final String selectorId, final
AiCommonConfig config) {
+ final String cacheKey = selectorId + "_" + config.hashCode();
return chatClientCache.computeIfAbsent(
- selectorId,
+ cacheKey,
() -> {
- LOG.info("Creating and caching main model for selector:
{}", selectorId);
+ LOG.info("Creating and caching main model for selector:
{}, key: {}", selectorId, cacheKey);
return createChatModel(config);
});
}
private ChatClient createAdminFallbackClient(
final String selectorId, final AiCommonConfig fallbackConfig) {
- final String fallbackCacheKey = selectorId + "|adminFallback";
+ final String fallbackCacheKey = selectorId + "|adminFallback_" +
fallbackConfig.hashCode();
Review Comment:
The same hashCode issue applies here. Using `fallbackConfig.hashCode()` in
the cache key will cause cache misses when configuration changes, and the cache
invalidation logic at ChatClientCache.remove() won't properly clean up these
entries because the removal logic only matches prefixes starting with
selectorId (see ChatClientCache line 72), but these keys now use a different
separator ("_" for main and "|adminFallback_" for fallback).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
