TAP: Add MCP client helper and run_sql_readonly validation#5372
TAP: Add MCP client helper and run_sql_readonly validation#5372wazir-ahmed wants to merge 2 commits intov3.0from
run_sql_readonly validation#5372Conversation
- Add `MCPClient` class for testing ProxySQL's MCP endpoints via HTTP/JSON-RPC - Add TAP test for `run_sql_readonly` query tool validation - Update AI test group configuration Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
Summary of ChangesHello @wazir-ahmed, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the testing framework for ProxySQL's Model Context Protocol (MCP). It introduces a robust C++ client for interacting with MCP endpoints, which is then leveraged to create a comprehensive test for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughAdds an MCP JSON‑RPC client and tests: new C++ MCP client library with libcurl, CLI and Makefile integration, MCP test group registration and proxysql MCP config, and a new TAP C++ test validating run_sql_readonly behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as TAP Test
participant MCP as MCP Server
participant MySQL as MySQL DB
Test->>MCP: MCPClient.check_server()
MCP-->>Test: status/result
Test->>MySQL: prepare test DB/data
MySQL-->>Test: ready
loop per query
Test->>MCP: MCPClient.call_tool("config","run_sql_readonly", args)
MCP->>MySQL: execute SQL (readonly enforcement)
MySQL-->>MCP: result or error
MCP-->>Test: JSON-RPC response (result/error)
Test->>Test: validate MCPResponse (success or expected error)
end
Test->>MySQL: cleanup
MySQL-->>Test: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
wazir-ahmed
changed the title
run_sql_readonly validation test
wazir-ahmed
changed the title
run_sql_readonly validation testrun_sql_readonly validation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a C++ MCPClient for testing ProxySQL's MCP endpoints and adds a new TAP test for run_sql_readonly validation. The overall implementation of the MCPClient is robust, with good error handling and a clean interface. The new test successfully validates the rejection of non-read-only queries.
However, there are several areas for improvement. The new test file mcp_query_run_sql_readonly-t.cpp uses raw pointers and goto for cleanup, which should be replaced with RAII constructs like std::unique_ptr for better resource management and code clarity. A significant portion of the new tests ("Allowed Query Tests") are commented out because they are failing, which suggests the feature or test is incomplete and should be addressed before merging. Additionally, there are some minor issues like hardcoded credentials in a SQL script and inconsistent use of functions for string-to-integer conversion in mcp_client.cpp.
My feedback focuses on improving code quality, maintainability, and robustness.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@test/tap/groups/ai/pre-proxysql.sql`:
- Around line 7-8: The test uses the wrong singular MCP variable names causing
silent misconfiguration: open test/tap/tests/mcp_query_run_sql_readonly-t.cpp,
find where it sets or references "mcp-mysql_host" and "mcp-mysql_port" (look for
setenv/putenv or string refs in the test setup) and change them to the
documented plural names "mcp-mysql_hosts" and "mcp-mysql_ports" so they match
the SQL fixture and doc/MCP/VARIABLES.md; update any related assertions or
helper calls that expect the singular names to use the new plural identifiers.
In `@test/tap/tap/mcp_client.h`:
- Around line 226-228: The doc example incorrectly accesses the private member
result_ via resp.result; update the example to call the public accessor
resp.get_result() instead (so use resp.get_result()["results"]) to match the
actual API and avoid direct member access of result_ (search for the snippet
using resp.result or direct result_ access in the example and replace with
resp.get_result()).
- Around line 44-47: The doc example passes resp.get_error_message() (which
returns const std::string&) to diag with a %s format specifier; update the call
in the error branch to pass resp.get_error_message().c_str() so the %s receives
a const char*, e.g. keep diag("Tool error: %s (code %d)",
resp.get_error_message().c_str(), resp.get_error_code()) using the existing
resp.get_error_code() and diag call sites.
In `@test/tap/tests/mcp_query_run_sql_readonly-t.cpp`:
- Around line 215-227: test_query_allowed is constructing args without the
"schema" key while test_query_rejected includes {"schema","test"}, causing
mismatched behavior when run_sql_readonly expects a schema; update
test_query_allowed to include the same schema argument (e.g., add
{"schema","test"} to the json args passed to
MCPClient::call_tool("query","run_sql_readonly", args)) so both tests use
consistent inputs for validation and routing.
- Line 80: The TAP plan declares 20 tests via plan(20) but only 11 actually run
because the nine "allowed query" tests were commented out; update the test plan
or restore the tests so they match: either change the plan(20) call to plan(11)
to reflect the current active tests, or re-enable the nine commented "allowed
query" tests so the total executed tests matches 20; locate the plan(20)
invocation and the commented-out allowed query test blocks in
test/tap/tests/mcp_query_run_sql_readonly-t.cpp (identify the plan(20) symbol
and the nearby allowed-query test cases) and make a consistent change.
🧹 Nitpick comments (4)
test/tap/tap/command_line.cpp (1)
242-254: Minor inconsistency:TAP_MCP_PORThere vsTAP_MCPPORTinmcp_client.cpp.
CommandLine::getEnv()readsTAP_MCP_PORT(line 244), butMCPClient's constructor fallback (mcp_client.cpp, line 28) readsTAP_MCPPORT(no underscore separator). Since the test createsMCPClientwith the explicit port fromCommandLine, this won't cause a runtime bug, but the inconsistent naming could confuse users who try to set the env var directly.Consider aligning both to use the same env var name.
test/tap/tap/mcp_client.cpp (2)
88-104: Consider callingcurl_global_init/curl_global_cleanup.
curl_easy_init()will auto-callcurl_global_init()on first use, but the libcurl docs warn this auto-initialization is not thread-safe. Since the TAP libraries are now linked into all test binaries (not just MCP tests), any test that happens to initialize curl concurrently could hit undefined behavior.A static init/cleanup (e.g., via a RAII wrapper or an explicit init in
main) would be safer.
369-399:check_serverdoes not add auth token for the"config"endpoint.
call_tooladds anAuthorizationheader if there's a token configured for the endpoint, butcheck_serverbuilds its own request directly without checkingauth_tokens_. If the MCP server requires authentication on the config endpoint, this health check will fail.Consider reusing the auth token logic or delegating to
call_tool.test/tap/tap/mcp_client.h (1)
10-10:using jsonalias in header leaks into all includers.This type alias at namespace scope will propagate to every translation unit that includes this header. For a test-internal library this is likely fine, but if the header is ever included more broadly it could cause name collisions.
- Replace per-endpoint auth token map with single token field - Remove automatic reading of TAP_ADMINHOST/TAP_MCPPORT env vars (users should pass these via CommandLine) - Fix MCP MySQL config variable names (host→hosts, port→ports) - Re-enable allowed query tests with required schema parameter Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/tap/tap/mcp_client.cpp`:
- Around line 355-385: check_server() is missing the Authorization header so
requests fail when auth_token_ is set; update MCPClient::check_server to append
an "Authorization: Bearer <auth_token_>" header when auth_token_ is non-empty
(same pattern used in call_tool), e.g. build headers with curl_slist_append
including Content-Type and the Authorization header, set CURLOPT_HTTPHEADER to
that list before curl_easy_perform, and ensure you still call
curl_slist_free_all(headers) after the request; reference
MCPClient::check_server, auth_token_, and call_tool to mirror the existing auth
header construction and cleanup.
🧹 Nitpick comments (3)
test/tap/tap/mcp_client.h (2)
9-9:usingdirective in header pollutes every includer's namespace.
using json = nlohmann::json;at file scope in a header forces thejsonalias onto every translation unit that includes this file. Consider moving it inside the class or usingnlohmann::jsonexplicitly in the header, and letting.cppfiles define their own alias.For a test-only utility this is low-risk, but worth noting.
171-180: Public setters weaken encapsulation ofMCPResponse.These setters are documented as "for MCPClient use" but are fully public, meaning any consumer can mutate response state. A
friend class MCPClientdeclaration with private setters would enforce this contract at compile time.Given this is test infrastructure, this is a minor concern.
test/tap/tap/mcp_client.cpp (1)
190-271: MCP error type is pre-set before validation, which works but is subtle.Line 194 sets
error_type_toMCPErrorType::MCPas a "default" for the entire MCP validation section, and it's only overridden toNONEon the success path (line 257). This works correctly, but the "guilty until proven innocent" approach could be confusing to future maintainers.A comment at line 194 clarifying the intent (e.g., "Default to MCP error; cleared on success") would help.
|
…g model This commit ports the TAP MCP client test coverage from PR #5372 to the new MCP profiles architecture, replacing legacy direct backend variable assumptions with target/profile-based routing. What was updated: - Reworked test setup to use `mcp_target_profiles` + `mcp_auth_profiles` rather than legacy `mcp-mysql_*` config variables. - Updated MCP request payloads and helper flows to route via `target_id`. - Aligned assertions with profile-backed runtime behavior and removed obsolete expectations tied to static mysql host/user/password globals. Why this was needed: - The previous tests were written before MCP target/profile routing existed. - Without this update, TAP coverage would exercise deprecated paths and miss regressions in the current MCP execution model. Result: - Test suite now validates the same functional intent as PR #5372 but against the current profile-driven MCP routing implementation.
2 participants
MCPClientclass for testing ProxySQL's MCP endpoints via HTTP/JSON-RPCrun_sql_readonlyquery tool validationSummary by CodeRabbit
New Features
Tests
Chores