Skip to content

fix(agent-gateway): stop duplicating HTTP tool query arguments in the upstream URI - #7926

Merged
Aias00 merged 1 commit into
apache:masterfrom
hanzhe-one:fix/agent-http-tool-query-duplication
Oct 22, 2026
Merged

Aias00 merged 1 commit into
apache:masterfrom
hanzhe-one:fix/agent-http-tool-query-duplication

Conversation

@hanzhe-one

Copy link
Copy Markdown
Contributor

Fixes #7909.

Problem

A configured HTTP tool with a query argument reaches the upstream with the query twice:

http://127.0.0.1:18080/orders/status?id=A?id=A

AgentHttpToolExecutor put the whole route, query included, into ShenyuContext.realUrl (context.setRealUrl(path)), while the child request URI already carried the same encoded query. RequestUrlUtils.buildRequestUri appends the request query to realUrl, so the ordinary HTTP route concatenated both copies.

Changes

  • AgentHttpToolExecutor now builds the child URI once and stores only target.getRawPath() in realUrl (and path), matching what the other context builders put there (DefaultShenyuContextBuilder, DivideShenyuContextDecorator). The encoded arguments stay on the child request URI, which is the single source of the query.
  • Added shenyu-plugin-context-path as a test-scope dependency so the context-path case can be exercised with the real plugin.

Validation

Regression tests in AgentHttpToolExecutorTest build the final upstream URI with the production RequestUrlUtils.buildRequestUri, the same call URIPlugin makes, instead of only asserting the child request URI:

  • forwardsEncodedQueryArgumentsExactlyOnceThroughTheOrdinaryHttpRoute — GET, id=A&B → http://127.0.0.1:18080/orders/status?id=A%26B.
  • forwardsABodyArgumentWithoutInventingAQuery — POST with argsPosition=body keeps the URI queryless.
  • forwardsAQuerylessRouteWithoutAddingAQuestionMark — no arguments, no trailing ?.
  • keepsTheContextPathRewriteWithoutDuplicatingTheQuery — route /api/orders/status with a context-path rule /api still forwards /orders/status?id=A%26B after the real ContextPathPlugin rewrites realUrl.

Failure before the fix (main code reverted, tests kept):

AgentHttpToolExecutorTest.forwardsEncodedQueryArgumentsExactlyOnceThroughTheOrdinaryHttpRoute
  java.lang.IllegalArgumentException: Invalid character '=' for QUERY_PARAM in "A%26B?id=A%26B"

Passes with the fix. Results:

  • ./mvnw test -pl shenyu-plugin/shenyu-plugin-agent-gateway: 550 tests, 0 failures; 27 errors are all java.io.IOException: Unable to establish loopback connection / SocketException: Invalid argument: connect from tests that open a real local socket (RequestScopedMcpClientTest, ManagedRemoteMcpCatalogTest, one AgentMcpOutputSchemaValidatorTest case). The same failures reproduce on this machine with an unrelated 3-line Selector.open() program, so they are environmental and are left to CI.
  • AgentHttpToolExecutorTest 27/27, AgentHttpToolRoutingTest 13/13, AgentGatewayPluginTest 17/17.
  • checkstyle:check: 0 violations. apache-rat:check: 0 unapproved licenses.

Blast radius

realUrl is read by RequestUrlUtils.buildRequestUri and getRewrittenRawPath, RewritePlugin, and AiRequestTransformerPlugin. Tool calls set path and realUrl to the same value, so the "already rewritten" guard in the AI transformer and the selector/rule matching in getRewrittenRawPath keep seeing the route path, which is what they saw before the query was added to realUrl — the only observable difference is that the duplicated query is gone.

Out of scope

Remote MCP tools and the MCP Server plugin's own ShenyuToolCallback path already strip the query with extractRawPath; they are not touched here.

… upstream URI

AgentHttpToolExecutor stored the route together with its encoded query in
ShenyuContext.realUrl while the child request URI already carried the same
query. RequestUrlUtils.buildRequestUri appends the request query to realUrl,
so every configured GET tool reached its upstream as
/orders/status?id=A?id=A.

Keep only the route path in realUrl (raw path, matching the plugin
contract) and let the child request URI carry the encoded arguments.
Build the child URI once and reuse it for the context fields.

Verified: the new regression test fails on the previous code with
'Invalid character = for QUERY_PARAM in "A%26B?id=A%26B"' and passes with
the fix; POST body arguments and a queryless route stay unchanged.

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the diagnosis is right and the fix is at the right place.

What I verified

  • The duplication was real and had exactly the shape described. RequestUrlUtils.buildRequestUri composes domain + shenyuContext.getRealUrl() and then appends the query of exchange.getRequest().getURI(). Since the child request is built with uri(target) where target already carries the encoded arguments, a realUrl that also carried them produced ...?id=A%26B?id=A%26B. Dropping the query from realUrl is the correct side to fix: the query is owned by the child request URI, so nothing is lost even when the tool template has its own literal query (e.g. /orders/status?fixed=1) — that fragment lives in the child's raw query and is re-appended by buildRequestUri.
  • getPath() → getRawPath() is the right call, not just a cosmetic change. DefaultShenyuContextBuilder.buildDefaultContext does shenyuContext.setPath(requestURI.getRawPath()), so the previous URI.create(path).getPath() made an agent tool call match selectors against a decoded path while ordinary traffic matches against the raw one. Using the same target URI for both setPath and setRealUrl also removes the second URI.create(path).
  • The tests assert against the real RequestUrlUtils rather than a stub, so forwardsEncodedQueryArgumentsExactlyOnceThroughTheOrdinaryHttpRoute proves the byte-level upstream URI (?id=A%26B once, with A&B correctly escaped on the way in) instead of asserting on an intermediate string. keepsTheContextPathRewriteWithoutDuplicatingTheQuery additionally runs the real ContextPathPlugin, which is the plugin that rewrites realUrl and could have re-introduced a query — that is the test that would catch a future regression here.
  • shenyu-plugin-context-path is only needed at test scope for the ContextPathPlugin fixture, and it does not depend back on shenyu-plugin-agent-gateway, so the new edge introduces no reactor cycle.

Non-blocking

  1. setPath moving from decoded to raw is matching-visible for a tool URL whose path contains a percent escape (e.g. /my%20route/x): it now matches the same selectors the ordinary chain would, which is the intent, but none of the new tests pin context.getPath() — they only assert realUrl and the built URI. One assertion on ((ShenyuContext) child.getAttribute(Constants.CONTEXT)).getPath() in the encoded-argument test would lock the behaviour in, and a line in the PR description would flag it as an intentional side effect of the fix.
  2. @BeforeEach installs a mocked ConfigurableApplicationContext via SpringBeanUtils.getInstance().setApplicationContext(context) — a JVM-wide static — and @AfterEach cleans the plugin caches but leaves that context installed. Every test that runs later in this module now sees the mock. You clearly care about isolation here (the cache cleanup is thorough), so capturing the previous context and restoring it would close the last gap.
  3. forwardsABodyArgumentWithoutInventingAQuery and forwardsAQuerylessRouteWithoutAddingAQuestionMark pass on the pre-fix code as well — in neither shape was a query ever produced. They are useful guards, but only the two query-carrying tests actually fail before the fix, which is worth stating in the PR body so a future reader knows which assertions are the regression proof.
  4. realUrl being path-only now depends on the child request URI always carrying the arguments. That holds today because the arguments are encoded into target, and the added comment explains it — good. If the argument placement ever moves out of the URI (headers, body), the coupling breaks silently, so the comment is load-bearing; consider also asserting getRealUrl() has no ? in the encoded-argument test to make that contract explicit.

Note on CI: build (shenyu-integrated-test-http) is currently red on master because of the CryptorResponsePluginTest.testWhenDoesntExistFiled assertion left behind by #7903 — unrelated to this PR.

@Aias00
Aias00 merged commit e36b060 into apache:master Oct 22, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Agent Gateway HTTP tools duplicate query arguments when building the upstream URI

2 participants