Conversation
# Conflicts: # docs/db-schema.md
# Conflicts: # src/main/java/com/epam/aidial/deployment/manager/dao/mapper/PersistenceDeploymentMapper.java # src/main/java/com/epam/aidial/deployment/manager/web/dto/deployment/CreateDeploymentRequestDto.java
Dependency ReviewThe following issues were found:
License Issuessettings.gradle
OpenSSF Scorecard
Scanned Files
|
# Conflicts: # src/test/java/com/epam/aidial/deployment/manager/functional/tests/TopicFunctionalTest.java
Dependency ReviewThe following issues were found:
License Issuessettings.gradle
OpenSSF Scorecard
Scanned Files
|
oleksii-donets
left a comment
There was a problem hiding this comment.
Review: feat: support MCP tool calls
Overall the feature is clean and well-scoped. A few issues to address:
1. Code duplication — callTool duplicates the get() template pattern
McpService.callTool() manually repeats the same steps as the private get() method: get deployment → resolve endpoint → create client → initialize → call → catch. Consider extracting a shared execute helper that both get() and callTool() can use:
private <T> T execute(String deploymentId, Function<McpSyncClient, T> operation) {
var deployment = getDeployment(deploymentId);
var endpointPath = mcpEndpointPathResolver.resolveEndpointPath(deployment);
try (var mcpClient = mcpClientFactory.create(deployment.getUrl(), endpointPath, deployment.getTransport())) {
mcpClient.initialize();
return operation.apply(mcpClient);
} catch (Exception e) {
// unified error handling
}
}Then callTool becomes a one-liner, and get() delegates to execute as well.
2. Potential NPE — ExceptionUtils.getRootCause(e) can return null
From the Apache Commons docs: getRootCause() returns null if the throwable has no cause chain (i.e., it is the root cause). This means ExceptionUtils.getRootCause(e).getMessage() will throw an NPE in that case.
Use ExceptionUtils.getRootCauseMessage(e) instead, or fall back:
var root = ExceptionUtils.getRootCause(e);
String reason = root != null ? root.getMessage() : e.getMessage();3. @Transactional(readOnly = true) inconsistency
The new callTool method has @Transactional(readOnly = true), but the existing listing methods (getTools, getResources, getPrompts) don't. Either all MCP service methods that read deployments should have it, or none should. I'd suggest removing it from callTool to stay consistent with existing methods, unless there's a specific reason it was added.
4. Different error message style
The existing get() method uses a user-friendly message: "Failed to connect to MCP server. Make sure transport '...' and path '...' are correct." The new callTool uses a more technical message with root cause extraction. Consider keeping the error message style consistent — the user-friendly pattern from get() is preferable.
5. Test style inconsistency — JSON path constants
Existing tests in McpControllerTest inline the JSON resource paths directly in ResourceUtils.readResource() calls (e.g. ResourceUtils.readResource("/mcp/mcp_tools_response.json")). The new test extracts them into private static final constants (CALL_TOOL_REQUEST_DTO_JSON_PATH, CALL_TOOL_RESULT_DTO_JSON_PATH) even though each is used only once. Better to keep one style — inline them to match the existing pattern.
6. Minor — test JSON files missing trailing newline
Both call_tool_request_dto.json and call_tool_result_dto.json are missing a trailing newline (\ No newline at end of file).
Dependency ReviewThe following issues were found:
License Issuessettings.gradle
OpenSSF Scorecard
Scanned Files
|
Dependency ReviewThe following issues were found:
License Issuessettings.gradle
OpenSSF Scorecard
Scanned Files
|
Applicable issues
Description of changes
Added new endpoint to call MCP server tools:
POST /api/v1/deployments/mcp/{deploymentId}/call-toolChecklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.