Copilot commented on code in PR #6296:
URL: https://github.com/apache/shenyu/pull/6296#discussion_r2791587231
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-request-transformer/src/main/java/org/apache/shenyu/plugin/ai/transformer/request/AiRequestTransformerPlugin.java:
##########
@@ -218,6 +221,28 @@ private static Mono<ServerWebExchange> convertHeader(final
ServerWebExchange exc
return Mono.just(exchange);
}
+ private static Mono<ServerWebExchange> rewriteRequestPath(final
ServerWebExchange exchange, final String aiResponse) {
+ String newPath = extractRequestPathFromAiResponse(aiResponse);
+ if (Objects.isNull(newPath) || newPath.isEmpty()) {
+ return Mono.just(exchange);
+ }
+
+ ServerHttpRequest originalRequest = exchange.getRequest();
+ URI originalUri = originalRequest.getURI();
+
+ URI newUri = originalUri.resolve(newPath);
+
+ ServerHttpRequest newRequest = originalRequest.mutate()
+ .uri(newUri)
+ .build();
+
+ ServerWebExchange newExchange = exchange.mutate()
+ .request(newRequest)
+ .build();
+
+ return Mono.just(newExchange);
Review Comment:
The extracted path from the AI response is used directly to rewrite the
request URI without any validation or sanitization. This could introduce
security vulnerabilities if the AI generates malicious paths. Consider adding
validation to ensure the extracted path is safe, such as checking for path
traversal attempts (e.g., "../"), ensuring it starts with "/", and potentially
restricting the allowed path patterns. Additionally, consider logging the path
rewrite operation for security auditing purposes.
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-request-transformer/src/main/java/org/apache/shenyu/plugin/ai/transformer/request/AiRequestTransformerPlugin.java:
##########
@@ -253,6 +278,24 @@ static HttpHeaders extractHeadersFromAiResponse(final
String aiResponse) {
return headers;
}
+ private static String extractRequestPathFromAiResponse(final String
aiResponse) {
+ if (Objects.isNull(aiResponse) || aiResponse.isEmpty()) {
+ return null;
+ }
+ String[] lines = aiResponse.split("\r?\n");
+ if (lines.length == 0) {
+ return null;
+ }
+
+ String startLine = lines[0].trim();
+ String[] parts = startLine.split(" ");
+ if (parts.length < 2) {
+ return null;
+ }
+
Review Comment:
The extractRequestPathFromAiResponse method assumes the first line of
aiResponse contains an HTTP request line (e.g., "GET /path HTTP/1.1"), but
based on the test cases (AiRequestTransformerPluginTest.java:120,128) and the
extractHeadersFromAiResponse method (line 259), the aiResponse format appears
to be an HTTP response starting with "HTTP/1.1 200 OK". This creates a format
mismatch - the method expects to extract parts[1] from a request line format,
but will actually receive a response line where parts[1] would be the status
code (e.g., "200"), not a path. This will result in incorrect path extraction.
The method should be updated to handle the actual HTTP response format or the
expected format should be clarified.
```suggestion
String startLine = lines[0].trim();
if (startLine.isEmpty()) {
return null;
}
// If this looks like an HTTP response status line (e.g., "HTTP/1.1
200 OK"),
// there is no request path to extract.
if (startLine.startsWith("HTTP/")) {
return null;
}
// Otherwise, expect a request line: "METHOD /path HTTP/1.1"
String[] parts = startLine.split("\\s+");
if (parts.length < 2) {
return null;
}
// Only treat it as a request line if the first token is an HTTP
method.
String method = parts[0];
if (!method.matches("GET|POST|PUT|DELETE|PATCH|OPTIONS|HEAD")) {
return null;
}
```
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-request-transformer/src/main/java/org/apache/shenyu/plugin/ai/transformer/request/AiRequestTransformerPlugin.java:
##########
@@ -218,6 +221,28 @@ private static Mono<ServerWebExchange> convertHeader(final
ServerWebExchange exc
return Mono.just(exchange);
}
+ private static Mono<ServerWebExchange> rewriteRequestPath(final
ServerWebExchange exchange, final String aiResponse) {
+ String newPath = extractRequestPathFromAiResponse(aiResponse);
+ if (Objects.isNull(newPath) || newPath.isEmpty()) {
+ return Mono.just(exchange);
+ }
+
+ ServerHttpRequest originalRequest = exchange.getRequest();
+ URI originalUri = originalRequest.getURI();
+
+ URI newUri = originalUri.resolve(newPath);
Review Comment:
The URI.resolve() method may not behave as expected for path rewriting.
According to RFC 2396, resolve() is designed for relative URI resolution and
treats paths starting with "/" as absolute paths that replace the entire path
of the base URI, which may not preserve the scheme and authority correctly.
Consider using UriUtils.createUri(scheme, authority, path) from the existing
codebase (similar to FallbackHandler.java:71) to ensure consistent URI
construction that properly preserves the scheme and authority while replacing
only the path.
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-request-transformer/src/main/java/org/apache/shenyu/plugin/ai/transformer/request/AiRequestTransformerPlugin.java:
##########
@@ -115,15 +116,17 @@ protected Mono<Void> doExecute(final ServerWebExchange
exchange, final ShenyuPlu
AiRequestTransformerTemplate aiRequestTransformerTemplate = new
AiRequestTransformerTemplate(aiRequestTransformerConfig.getContent(),
exchange.getRequest());
ChatClient finalClient = client;
+
return aiRequestTransformerTemplate.assembleMessage()
- .flatMap(message -> Mono.fromCallable(() ->
finalClient.prompt().user(message).call().content())
- .subscribeOn(Schedulers.boundedElastic())
+ .flatMap(message ->
finalClient.prompt().user(message).stream().content()
+ .collectList()
+ .map(list -> String.join("", list))
.flatMap(aiResponse -> convertHeader(exchange,
aiResponse)
.flatMap(serverWebExchange ->
convertBody(serverWebExchange, messageReaders, aiResponse))
- .flatMap(chain::execute)
+ .flatMap(serverWebExchange ->
rewriteRequestPath(serverWebExchange, aiResponse))
+ .flatMap(chain::execute)
)
);
Review Comment:
The async refactoring in doExecute method lacks error handling for potential
failures in the AI streaming operation. Unlike the AiResponseTransformerPlugin
which has onErrorResume at line 371-375 to handle errors and provide fallback
behavior, this implementation has no error handling. If the AI call fails
(e.g., network issues, API errors, stream errors), the entire request will fail
without any fallback. Consider adding onErrorResume to handle errors
gracefully, potentially logging the error and either proceeding with the
original request unchanged or returning an appropriate error response.
```suggestion
)
.onErrorResume(throwable -> {
LOG.error("AI request transformation failed, proceeding
without AI transformation", throwable);
return chain.execute(exchange);
});
```
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-request-transformer/src/main/java/org/apache/shenyu/plugin/ai/transformer/request/AiRequestTransformerPlugin.java:
##########
@@ -253,6 +278,24 @@ static HttpHeaders extractHeadersFromAiResponse(final
String aiResponse) {
return headers;
}
+ private static String extractRequestPathFromAiResponse(final String
aiResponse) {
+ if (Objects.isNull(aiResponse) || aiResponse.isEmpty()) {
+ return null;
+ }
+ String[] lines = aiResponse.split("\r?\n");
+ if (lines.length == 0) {
+ return null;
+ }
+
+ String startLine = lines[0].trim();
+ String[] parts = startLine.split(" ");
+ if (parts.length < 2) {
+ return null;
+ }
+
+ return parts[1];
+ }
Review Comment:
The new path rewriting functionality (rewriteRequestPath and
extractRequestPathFromAiResponse methods) lacks test coverage. The existing
test file AiRequestTransformerPluginTest.java has tests for convertBodyJson and
extractHeadersFromAiResponse, but no tests for the new path rewriting feature.
Consider adding unit tests that verify path extraction from AI responses and
the path rewriting behavior for various scenarios including edge cases like
empty paths, malformed responses, and paths with query parameters.
--
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]
