lymerin commented on PR #7535: URL: https://github.com/apache/shenyu/pull/7535#issuecomment-6106658348
> Reviewed locally at [5ab0061](https://github.com/apache/shenyu/commit/5ab0061c0a56eb83e006e38a1d91858ff9c3f5ec) (JDK 21). `AiRequestTransformerPluginTest` 10/10 green (including the new parameterized routing test), checkstyle clean. CI on the head commit is green. > > The fix is correct for the main path: with `realUrl == path` the rewritten path is copied to `realUrl`, so `RequestUrlUtils.buildRequestUri` builds the upstream URL from the accepted rewrite, and `REWRITE_URI` still wins. The regression test exercises the real transformer plus `RequestUrlUtils` rather than asserting on the mutated context alone — good. > > One follow-up worth considering: the guard `Objects.equals(context.getRealUrl(), context.getPath())` conflates two different situations. When `ContextPathPlugin` has run, `realUrl` is the context-path-stripped path and `path` is the full path, so they legitimately differ; in that case the rewrite now updates `path` but leaves `realUrl` pointing at the original path, and `buildRequestUri` keeps forwarding to the old upstream path — the same symptom this PR fixes, just behind a context path. Either update `realUrl` there too, or make the intent explicit (comment + test) that a context-path deployment is out of scope. Thanks for pointing this out. I agree that `realUrl != path` does not necessarily indicate an explicit routing override, since `ContextPathPlugin` can also produce this difference. I'd prefer to address the ContextPath interaction in a separate follow-up PR, with dedicated regression tests covering path rewriting and forwarding behavior, rather than expanding the scope of this fix. This PR will remain focused on synchronizing the routing context for the original reproduction scenario while preserving existing forwarding overrides. -- 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]
