feat(mcp): expose schema as a Resource at arcadedb://{database}/schema (#4865)#5280
Conversation
|
Tick the box to add this pull request to the merge queue (same as
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -53 |
🟢 Coverage 94.90% diff coverage · -6.25% coverage variation
Metric Results Coverage variation ✅ -6.25% coverage variation Diff coverage ✅ 94.90% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (6108123) 139797 104095 74.46% Head commit (f3ef2f1) 171976 (+32179) 117315 (+13220) 68.22% (-6.25%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#5280) 196 186 94.90% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Code Review: MCP schema-as-Resource (#5280)Reviewed the full diff. This is a clean, well-motivated change: the A few observations, mostly minor: Minor / discussion
Things I checked and confirmed are fine
Overall: LGTM. The Reviewed with Claude Code. |
There was a problem hiding this comment.
Code Review
This pull request implements the MCP Schema Resource feature by exposing database schemas as MCP resources at arcadedb://{database}/schema over both HTTP and stdio transports. It extracts a transport-neutral MCPDispatcher to unify JSON-RPC routing, refactors GetSchemaTool to share schema-building logic, and introduces MCPResources for resource discovery and retrieval. Feedback suggests adding a defensive null check when retrieving the database in MCPResources.read to prevent a potential NullPointerException if a database is concurrently dropped.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| final Database database = server.getDatabase(databaseName); | ||
| final JSONObject schema = GetSchemaTool.buildSchema(database, databaseName); |
There was a problem hiding this comment.
To prevent potential NullPointerExceptions if the database fails to load or is concurrently dropped between the existence check and the retrieval, add a defensive null check for the database instance. Throwing MCPResourceNotFoundException is more appropriate here as it maps to a -32002 (Resource not found) error rather than a -32603 (Internal error).
| final Database database = server.getDatabase(databaseName); | |
| final JSONObject schema = GetSchemaTool.buildSchema(database, databaseName); | |
| final Database database = server.getDatabase(databaseName); | |
| if (database == null) | |
| throw new MCPResourceNotFoundException("Resource not found: " + uri); | |
| final JSONObject schema = GetSchemaTool.buildSchema(database, databaseName); |
There was a problem hiding this comment.
Holding off on this one - the null check is unreachable in this codebase.
server.getDatabase(name) delegates to getDatabase(name, false, true). That method never returns null: the fast path returns only when db != null && db.isOpen(), and the slow path either reuses an active instance, calls factory.open(), or throws (DatabaseOperationException, or the open failure itself). There is no path that assigns null to db before return db. So a failed or concurrently-dropped load throws rather than returning null - an added null check can never fire.
The concurrent-drop race is genuinely possible (drop between existsDatabase and getDatabase in the same request), but it's already handled: factory.open() throws, and the dispatcher's generic catch (Exception) maps that to -32603, which is what the spec's error table prescribes for "anything else". -32002 is reserved by design for unknown/unauthorized/malformed URIs to prevent using resources/read as a database-existence probe; a transient server-side drop-mid-read is neither a caller error nor a probing vector, so -32603 is the correct signal here.
No code change.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5280 +/- ##
============================================
- Coverage 65.53% 65.52% -0.02%
Complexity 941 941
============================================
Files 1706 1709 +3
Lines 139797 139781 -16
Branches 29937 29943 +6
============================================
- Hits 91620 91593 -27
- Misses 35743 35747 +4
- Partials 12434 12441 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Addressed the review in 576182b. #1 (JSON-RPC #2 (shared static #3 ( CI red (unit/integration/ha-integration/Meterian) is pre-existing on |
Code Review: MCP schema Resource (#4865)Reviewed the Java changes ( Verified
Minor observations (non-blocking)
Test coverageStrong. Overall: LGTM. No blocking issues; the notes above are polish. Thanks for the clean refactor and the honest capability flags ( 🤖 Generated with Claude Code |
|
Addressed the second review. Test coverage gap (the security-relevant one) - fixed in f3ef2f1. New On the four non-blocking observations, none actioned, reasoning below:
|
Review: MCP schema Resource + dispatcher extractionReviewed the main-source changes ( What holds up well
Minor points (non-blocking)
Test coverageStrong and appropriately layered: transport-free I did not run the full LGTM once the release note for the stdio gating change lands. |
Closes #4865. Part of epic #4859 (Wave 1).
What
Exposes each accessible database's schema as an MCP Resource at
arcadedb://{database}/schema, over both the HTTP and stdio transports. A client that supports Resources can now auto-load schema at session start instead of spending a model turn on theget_schematool.get_schemastays, and stays recommended, for clients that do not implement Resources.initializenow advertisescapabilities.resourceswithlistChanged: falseandsubscribe: false. Both flags are honest: no change-notification or subscription machinery is built.The refactor
MCPHttpHandlerandMCPStdioServerwere near-duplicates: each carried its ownTOOLS_LIST, its ownMCP_PROTOCOL_VERSION, its own methodswitch, and its own copies of the JSON-RPC envelope helpers. Adding a second protocol primitive would have paid that duplication twice more.A new
MCPDispatchernow owns the whole JSON-RPC surface (protocol version, tool list,instructions, gating, method routing, envelope shaping) and both transports collapse to thin adapters. Transports own I/O and their own parse errors; the dispatcher owns protocol.Net effect: a whole new protocol primitive lands for net +112 lines of main source, because ~346 lines of transport duplication were deleted.
The schema walk was extracted to a single
GetSchemaTool.buildSchema(Database, String). The tool and the resource both render from it, so the issue's headline criterion (tool output and resource content are identical) is true by construction rather than by coincidence. It cannot drift, because there is only one implementation.Behavior change (stdio transport)
MCPStdioServernow honorsMCPConfiguration.isEnabled()andisUserAllowed(), which it previously ignored, and itsinitializeresponse now includes theinstructionsblock that the HTTP transport already returned. The default path is unaffected:main()setsenabled=trueand authenticates asroot, androotis the default sole entry inallowedUsers. A stdio deployment that had rewrittenallowedUsersto omitrootwill now be refused, which is arguably a latent bug being fixed.This warrants a release note.
Design notes
java.net.URI.URI.getHost()applies hostname syntax rules: it lowercases the authority and returnsnullfor an authority containing an underscore. ArcadeDB database names are case-sensitive and may legally contain_, soarcadedb://My_DB/schemawould silently resolve to nothing underjava.net.URI.resources/readdeliberately departs from the house error convention.MCPToolUtils.resolveDatabaseenumerates the available databases in its error text so the model can self-correct. That is right for tools, where the model supplies the name from memory and can guess wrong. Here, unknown database, unauthorized database, and malformed URI all collapse to one indistinguishable-32002, soresources/readcannot be used to probe which databases exist. Safe because read URIs come fromresources/list, so a legitimate caller never guesses one.listhides,readerrors. WithallowReads=false,resources/listreturns an empty array (a discovery call most clients issue unprompted, so a reads-disabled server stays quiet) whileresources/readfails loudly with-32600.Testing
Tool count stays at 11: this adds a protocol primitive, not a tool.
MCPResourcesTest(8 tests): transport-free calls intoMCPResources;readcontent equalsGetSchemaTool.executeoutput; malformed-URI shapes; thejava.net.URIcase/underscore trap; URI round-trip.MCPServerPluginTest(+8):resourcescapability advertised;resources/listset equals thelist_databasesset;get_schemaoutput equalsresources/readcontent text;-32002for unknown database and malformed URI; unauthorized databases omitted from the list;allowReads=falsebehavior.MCPStdioServerTest(+3):resources/listandresources/readover stdio;initializeadvertisesresourcesand now carriesinstructions.The existing MCP suites are the regression net for the dispatcher extraction, and they pass completely unedited. The test diff against
mainis additions only - zero deletions in any test file - so the old suites remain an untainted control.servermodule green: 601 tests, 0 failuresSequencing
This rewrites
MCPHttpHandler.javaandMCPStdioServer.java, the two files that #4860 (vector_search), #4863 (sample_records), and #4864 (upsert_entity/upsert_relationship) also edit. #4865 should land before those three, with the siblings rebased onto it: each of them adds a tool, which after this refactor means oneTOOLS_LISTline and oneswitcharm instead of two of each.Out of scope
resources/templates/list, subscriptions, any resource other than schema, and the out-of-treearcadedb-docsupdate (tracked separately, consistent with how #4862 handled it). No new configuration flag; default permissions unchanged.🤖 Generated with Claude Code