nikhiln64 commented on code in PR #6897:
URL: https://github.com/apache/shenyu/pull/6897#discussion_r3744138166
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-proxy/src/main/java/org/apache/shenyu/plugin/ai/proxy/enhanced/AiProxyPlugin.java:
##########
@@ -100,18 +101,8 @@ protected Mono<Void> doExecute(
CacheKeyUtils.INST.getKey(
selector.getId(), Constants.DEFAULT_RULE));
- return DataBufferUtils.join(exchange.getRequest().getBody())
+ return DataBufferUtils.join(exchange.getRequest().getBody(),
MAX_REQUEST_BODY_SIZE_BYTES)
Review Comment:
## Context
Nice catch, this is a real fix. Buffering the whole body with the single
argument DataBufferUtils.join before checking readableByteCount meant the
memory was already spent by the time the limit was enforced, and passing the
maxSize into join so it aborts mid stream and onErrorResume maps it to 413 is
the right shape.
▎ Example: The test mocks DataBufferUtils.join statically and matches the
size argument with anyInt(), so it only proves the DataBufferLimitException to
413 mapping and never pins the limit the plugin is responsible for. A
regression that weakened the limit, say passing Integer.MAX_VALUE or a mis
scaled constant, would still pass green.
## Suggestion
An ArgumentCaptor or an eq(MAX_REQUEST_BODY_SIZE_BYTES) matcher so the test
asserts join is called with exactly the 5MB constant would close that gap. If
it is easy, one unmocked test that feeds a real body over the limit through
doExecute and asserts 413 with no downstream call would make the guarantee end
to end.
## NOTE
Not blocking, the production change looks correct as is.
--
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]
