klT45 commented on PR #7916: URL: https://github.com/apache/shenyu/pull/7916#issuecomment-6330790415
Thanks @Aias00 for the detailed review and @wy471x for the extra points. I folded the non-blocking follow-ups into `a0c83949`: 1. **Truncate instead of dropping (Aias00 #1)**: the decorator now keeps the first `maxBytes` and marks the snapshot truncated, so a 1.05 MiB body still gives the model a usable prefix instead of the empty body this PR was fixing. The cap is still a constant; moving it into `AiResponseTransformerHandle` next to `content` needs a config field plus dashboard support, so I left that as a follow-up and can do it in a separate PR if you want it tunable per rule. 2. **Captured bytes vs content type (Aias00 #2, #3; wy471x #1)**: capture now only happens for textual payloads — `null` content type, `text/*`, JSON, `+json`, XML, `+xml`, form-urlencoded — and is skipped entirely when `Content-Encoding` is present. Multipart, binary and gzip payloads are never decoded as UTF-8 mojibake, and their bodies are still forwarded untouched. The remaining case, a downstream plugin that rewrites the body while keeping the original content type (e.g. cryptor request), is documented: the prompt describes the request as the response transformer received it. 3. **`Content-Length` pre-check (Aias00 #5)**: when the declared length already exceeds the cap the capture is skipped, so large uploads are not buffered at all. Peak retention is now ~1× cap and only for chunked bodies. 4. **Log flooding (wy471x #2)**: the truncation warning fires once per JVM, subsequent occurrences log at debug. 5. **Partial-body documentation (Aias00 #4)**: `bodyAsString()` now documents that the value can be a prefix when the upstream answers before the whole request body was forwarded. 6. **Two-argument constructor (Aias00 #6)**: kept but documented as the legacy path for callers/tests outside the plugin chain; the plugin always passes the snapshot. I did not deprecate it here to keep the template tests and any downstream callers compiling — happy to deprecate it in a follow-up. Re-verified locally: module `verify` passes with 65 tests (up from 62), checkstyle 0, RAT 0. New tests cover truncation, binary content type, `Content-Encoding: gzip`, and a declared `Content-Length` above the cap. CI is re-running on `a0c83949`; @Aias00 could you re-approve when it is green? (the push dismissed the earlier approval) -- 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]
