Skip to content

fix(openapi-to-mcp): merge query parameters with path suffix - #14166

Open
lordcheng10 wants to merge 2 commits into
apache:masterfrom
lordcheng10:fix/openapi-to-mcp-query-suffix
Open

lordcheng10 wants to merge 2 commits into
apache:masterfrom
lordcheng10:fix/openapi-to-mcp-query-suffix

Conversation

@lordcheng10

Copy link
Copy Markdown
Contributor

Description

openapi-to-mcp currently appends serialized query parameters with an unconditional
?. Some OpenAPI documents, including S3-style APIs, use a query suffix in the path
template. For /{Bucket}?acl with Bucket=b and a declared prefix=logs query
parameter, the tool sends:

/b?acl?prefix=logs

The upstream therefore receives one query parameter named acl?prefix; prefix
is not present under its declared name.

The handler now checks whether the constructed URL already contains a query marker
and uses & in that case. Templates without a query suffix still use ?, so their
behavior is unchanged:

/{Bucket}       -> /b?prefix=logs
/{Bucket}?acl   -> /b?acl&prefix=logs

Which issue(s) this PR fixes:

Fixes #14161

Reproduction and evidence

Verified on APISIX 3.19.0 from master
7577c31733bf0f7067cda6af3153b2a56ec8c6f5. The running container's
apisix/plugins/openapi-to-mcp/tools/handler.lua had SHA-256
2f293159ca3469ec4f997eadd8ce9f668bc6a2b041296d68d50099209230054c, matching the
unmodified master file.

An openapi-to-mcp Route served an operation for /{Bucket}?acl. A JSON-RPC
tools/call with Bucket=b and prefix=logs reached the Node upstream as:

Before:

/b?acl?prefix=logs

After applying the patch:

/b?acl&prefix=logs

Issue 14161 reproduction and fix

Tests

  • Added a regression test covering both the existing no-suffix URL and the
    query-suffix merge.
  • Verified the patched handler loads successfully with LuaJIT.
  • git diff --check passes.
  • The local machine does not have OpenResty, wget, luacheck, Test::Nginx,
    or t::APISIX; therefore make lint and the target .t could not run locally.
    The real-container reproduction above exercises the complete MCP tool-call path.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible

No documentation update is required because this restores the documented OpenAPI
query-parameter behavior without adding configuration.

@lordcheng10

Copy link
Copy Markdown
Contributor Author

PTAL @Baoyuantop @nic-6443 @AlinsRan @ruanweihong-aaa @shreemaan-abhishek. This is a minimal follow-up to the query-suffix behavior covered in #14154, with a real-container before/after reproduction for #14161.

…query-suffix

# Conflicts:
#	t/plugin/openapi-to-mcp.t

@AlinsRan AlinsRan 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

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: openapi-to-mcp appends a second ? to a query-suffixed path template

3 participants