Skip to content

Conversation

@relat-ivity
Copy link
Contributor

@relat-ivity relat-ivity commented Oct 17, 2025

🔗 相关问题 / Related Issue

Issue 链接 / Issue Link: #342 👈👈

  • 我已经创建了相关 Issue 并进行了讨论 / I have created and discussed the related issue
  • 这是一个微小的修改(如错别字),不需要 Issue / This is a trivial change (like typo fix) that doesn't need an issue

📋 变更类型 / Type of Change

  • 🐛 Bug 修复 / Bug fix (non-breaking change which fixes an issue)
  • ✨ 新功能 / New feature (non-breaking change which adds functionality)
  • 💥 破坏性变更 / Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📚 文档更新 / Documentation update
  • 🔧 重构 / Refactoring (no functional changes)
  • ⚡ 性能优化 / Performance improvement
  • 📦 依赖升级 / Dependency upgrade (update dependencies to newer versions)
  • 🚀 功能增强 / Feature enhancement (improve existing functionality without breaking changes)
  • 🧹 代码清理 / Code cleanup

📝 变更目的 / Purpose of the Change

根据MCP协议2025-06-18版本新的规范,MCP传输调整为streamable HTTP。并且新增服务器Elicitation与服务器Log通知等新功能。

在原有的FEL中实现的SSE MCP服务器中接入这些新功能需要较大工作量,所以需要接入MCP SDK实现。

📋 主要变更 / Brief Changelog

  • 删除原有的MCP服务器的controller与handler逻辑
  • 在FEL的tool-mcp-server插件中添加0.14.1版本的MCP Java SDK的依赖
  • 新建DefaultMcpStreamableServerTransportProvider类使用FIT的HTTP实现完成Streamable MCP服务器的HTTP传输逻辑
  • DefaulMcpServerBean中构建DefaultMcpStreamableServerTransportProviderMcpSyncServer的Bean
  • DefaultMcpServer类中完成FIT的MCP服务器接口的实现

🧪 验证变更 / Verifying this Change

测试步骤 / Test Steps

  1. 使用npx @modelcontextprotocol/inspector测试服务器功能
  2. 使用Postman测试Inpector测不到的API,例如GET建立SSE

测试覆盖 / Test Coverage

  • 我已经添加了单元测试 / I have added unit tests
  • 所有现有测试都通过 / All existing tests pass
  • 我已经进行了手动测试 / I have performed manual testing

📸 截图 / Screenshots

截图包括MCP Inspector、Postman和服务器logger结果

1. initialize

客户端POST发送initialize,服务端HTTP response回复initialize信息,客户端POST发送notifications/initialized
image
image

2. logging/setLevel

客户端POST发送logging/setLevel,服务端建立SSE发送result,然后关闭SSE连接
image
image

3. GET建立SSE

客户端GET请求建立SSE长连接,客户端通过SSE接收notifications/tools/list_changed通知。关闭GET连接后SSE长连接关闭。
image
image

4. tools/list

添加addTool工具,客户端POST发送tools/list,服务端建立SSE发送result,然后关闭SSE连接
image
image

5. tools/call

调用addTool工具,调中包括Elicitation和服务器Log通知。

  1. 客户端发送POST连接tools/call请求调用addTool工具
  2. 服务端建立SSE发送elicitation/create请求Elicitation
  3. 客户端POST发送Elicitation结果
  4. 服务端SSE发送notifications/message通知服务器Log
  5. 服务端SSE发送工具执行result并关闭SSE
    image image
image

✅ 贡献者检查清单 / Contributor Checklist

请确保你的 Pull Request 符合以下要求 / Please ensure your Pull Request meets the following requirements:

基本要求 / Basic Requirements:

  • 确保有 GitHub Issue 对应这个变更(微小变更如错别字除外)/ Make sure there is a Github issue filed for the change (trivial changes like typos excluded)
  • 你的 Pull Request 只解决一个 Issue,没有包含其他不相关的变更 / Your PR addresses just this issue, without pulling in other changes - one PR resolves one issue
  • PR 中的每个 commit 都有有意义的主题行和描述 / Each commit in the PR has a meaningful subject line and body

代码质量 / Code Quality:

  • 我的代码遵循项目的代码规范 / My code follows the project's coding standards
  • 我已经进行了自我代码审查 / I have performed a self-review of my code
  • 我已经为复杂的代码添加了必要的注释 / I have commented my code, particularly in hard-to-understand areas

测试要求 / Testing Requirements:

  • 我已经编写了必要的单元测试来验证逻辑正确性 / I have written necessary unit-tests to verify the logic correction
  • 当存在跨模块依赖时,我尽量使用了 mock / I have used mocks when cross-module dependencies exist
  • 基础检查通过:mvn -B clean package -Dmaven.test.skip=true,elsa README 中的编译检查 / Basic checks pass
  • 单元测试通过:mvn clean install / Unit tests pass

文档和兼容性 / Documentation and Compatibility:

  • 我已经更新了相应的文档 / I have made corresponding changes to the documentation
  • 如果有破坏性变更,我已经在 PR 描述中详细说明 / If there are breaking changes, I have documented them in detail
  • 我已经考虑了向后兼容性 / I have considered backward compatibility

📋 附加信息 / Additional Notes

由于引入MCP Java SDK中使用SLF4J作为日志,启动时会warning:

SLF4J(W): No SLF4J providers were found.
SLF4J(W): Defaulting to no-operation (NOP) logger implementation
SLF4J(W): See https://www.slf4j.org/codes.html#noProviders for further details.

解决方法:在build/shared文件夹下,添加日志provider的依赖,例如slf4j-apislf4j-simple的jar文件


审查者注意事项 / Reviewer Notes:

@relat-ivity relat-ivity marked this pull request as ready for review October 17, 2025 06:53
@CodeCasterX CodeCasterX added type: enhancement A general enhancement in: fel Issues in FEL(FIT Expression for LLM) modules labels Oct 23, 2025
@CodeCasterX CodeCasterX modified the milestones: 3.5.4, 3.6.0 Oct 23, 2025
@CodeCasterX CodeCasterX linked an issue Oct 23, 2025 that may be closed by this pull request
4 tasks
@CodeCasterX
Copy link
Member

PR #344 代码检视报告

这是一个将 MCP 服务器从自定义实现迁移到官方 MCP Java SDK 的重构 PR。总体来说这是一个积极的改进,但有一些需要注意的地方。

主要变更概述

添加了 921 行,删除了 895 行,涉及 19 个文件

  1. 引入 MCP Java SDK 0.14.0
  2. 删除了所有自定义的 MCP 处理器实现(InitializeHandler, ToolCallHandler 等)
  3. 新增核心实现类
    • DefaultMcpStreamableServerTransportProvider - 788 行新代码,实现 HTTP/SSE 传输
    • DefaultMcpServerBean - Bean 配置类
  4. 重构了 DefaultMcpServer 以集成 SDK

✅ 优点

  1. 符合最新规范 - 遵循 MCP 2025-06-18 协议版本
  2. 功能增强 - 支持新特性如 Elicitation、服务器日志通知
  3. 代码简化 - 删除了大量自定义处理器代码
  4. 测试覆盖 - 有手动测试截图证明功能正常

⚠️ 需要关注的问题

1. 核心问题:连接监控机制的临时性解决方案

DefaultMcpStreamableServerTransportProvider.java:315-338

private void startConnectionMonitoring(String sessionId,
                                    McpStreamableServerSession.McpStreamableServerSessionStream listeningStream,
                                    HttpClassicServerResponse response) {
    // Use a separate thread to periodically check connection status
    Thread monitoringThread = new Thread(() -> {
        try {
            while (!Thread.currentThread().isInterrupted()) {
                Thread.sleep(1000); // Check every second
                
                if (!response.isActive()) {
                    logger.info("[SSE] Connection lost for session, completing emitter to trigger cleanup");
                    listeningStream.close();
                    break;
                }
            }
        } catch (InterruptedException e) {
            // ...
        }
    });
    
    monitoringThread.setDaemon(true);
    monitoringThread.start();
}

问题

  • 使用轮询每秒检查连接状态,资源利用率不高
  • 创建守护线程可能在某些场景下导致资源泄漏
  • 注释明确说这是 "workaround"

建议:在后续 Issue 中优化为事件驱动机制

2. SLF4J 日志警告

PR 描述中提到启动时会出现:

SLF4J(W): No SLF4J providers were found.

建议:需要添加 SLF4J 绑定依赖,比如 logback-classic 或桥接到 FIT 的日志系统

3. 错误处理不够细致

DefaultMcpStreamableServerTransportProvider.java 的多个地方:

catch (Exception e) {
    logger.error("Failed to send message to session {}: {}", session.getId(), e.getMessage());
}

建议

  • 应该记录完整堆栈:logger.error("...", e) 而不是 e.getMessage()
  • 区分不同异常类型进行不同处理

4. 线程安全问题

DefaultMcpServer.java:93-113

public void onToolAdded(String name, String description, Map<String, Object> parameters) {
    // ... 验证逻辑 ...
    
    McpServerFeatures.SyncToolSpecification toolSpecification = ...;
    this.mcpSyncServer.addTool(toolSpecification);  // ← 这里
    
    Tool tool = new Tool();
    // ... 设置属性 ...
    this.tools.put(name, tool);  // ← 和这里
    
    this.toolsChangedObservers.forEach(ToolsChangedObserver::onToolsChanged);
}

问题addTooltools.put 之间如果出现异常,会导致状态不一致

建议

// 先验证,再统一修改状态
McpServerFeatures.SyncToolSpecification toolSpecification = ...;
Tool tool = new Tool();
// ... 设置属性 ...

try {
    this.mcpSyncServer.addTool(toolSpecification);
    this.tools.put(name, tool);
    this.toolsChangedObservers.forEach(ToolsChangedObserver::onToolsChanged);
} catch (Exception e) {
    logger.error("Failed to add tool: {}", name, e);
    // 可能需要回滚
}

5. 类型安全检查不够严谨

DefaultMcpServer.java:93

if (!(parameters.get(TYPE) instanceof String)
        || parameters.get(PROPERTIES) != null && !(parameters.get(PROPERTIES) instanceof Map)
        || parameters.get(REQUIRED) != null && !(parameters.get(REQUIRED) instanceof List)) {
    log.warn("Invalid parameter schema. [toolName={}]", name);
    return;
}

问题

  • 即使类型检查通过,Map 和 List 的泛型参数仍不安全
  • @SuppressWarnings("unchecked") 隐藏了潜在的 ClassCastException

建议:增加运行时验证或使用更强的类型约束

6. 缺少超时配置的可配置性

DefaultMcpServerBean.java:40

.requestTimeout(Duration.ofSeconds(10))

建议:这个值应该从配置文件读取,而不是硬编码

7. 测试覆盖不足

  • PR 只有手动测试,缺少单元测试
  • DefaultMcpServerTest 被修改但没有展示新测试的内容

🔍 代码风格问题

  1. 注释语言不一致 - 有些中文注释,有些英文注释
  2. 日志格式不统一 - 有的用 [POST],有的用 POST
  3. 魔法数字 - Thread.sleep(1000) 应该定义为常量

📋 建议的后续改进

  1. 优先级高

    • 修复 SLF4J 警告
    • 增强错误处理(完整堆栈跟踪)
    • 添加单元测试覆盖
  2. 优先级中

    • 替换连接监控的轮询机制
    • 改进线程安全性
    • 配置项外部化
  3. 优先级低

    • 统一代码风格和注释语言
    • 提取魔法数字为常量

总结

这是一个整体方向正确的重构,成功引入了官方 SDK 并减少了维护负担。但需要在合并前关注:

  1. ✅ 基本功能通过测试
  2. ⚠️ 需要在后续 Issue 中解决已知的临时方案
  3. ⚠️ 建议添加更完善的单元测试
  4. ⚠️ 修复 SLF4J 日志问题

建议:可以合并,但应该立即创建后续 Issue 跟踪上述改进点。

RonnyChan96
RonnyChan96 previously approved these changes Oct 29, 2025
@relat-ivity
Copy link
Contributor Author

relat-ivity commented Oct 30, 2025

代码更改

超时时间可配置

public McpSyncServer mcpSyncServer(FitMcpStreamableServerTransportProvider transportProvider,
        @Value("${mcp.server.request.timeout-seconds}") int requestTimeoutSeconds) 

parameters校验太过于复杂,抽取函数

    private boolean isValidParameterSchema(Map<String, Object> parameters) {
        Object type = parameters.get(TYPE);
        if (!(type instanceof String)) {
            return false;
        }

        Object props = parameters.get(PROPERTIES);
        if (!(props instanceof Map<?, ?> propsMap)) {
            return false;
        }
        if (propsMap.keySet().stream().anyMatch(k -> !(k instanceof String))) {
            return false;
        }

        Object reqs = parameters.get(REQUIRED);
        if (!(reqs instanceof List<?> reqsList)) {
            return false;
        }
        if (reqsList.stream().anyMatch(v -> !(v instanceof String))) {
            return false;
        }

        return true;
    }

工具执行增加异常处理

.callHandler((exchange, request) -> {
    try {
        Map<String, Object> args = request.arguments();
        String result = this.toolExecuteService.execute(name, args);
        return new McpSchema.CallToolResult(result, false);
    } catch (IllegalArgumentException e) {
        log.warn("Invalid arguments for tool execution. [toolName={}, error={}]", name, e.getMessage());
        return new McpSchema.CallToolResult("Error: Invalid arguments - " + e.getMessage(), true);
    } catch (Exception e) {
        log.error("Failed to execute tool. [toolName={}]", name, e);
        return new McpSchema.CallToolResult("Error: Tool execution failed - " + e.getMessage(), true);
    }
})

删除本地Tools保存,避免线程问题

删除tools的存储,getTools()时候调用SDK的api,然后转换为Fit的Tool类

public List<Tool> getTools() {
    return this.mcpSyncServer.listTools().stream()
            .map(this::convertToFelTool)
            .collect(Collectors.toList());
}

删除Server里的ServerSchema相关旧方法

删除getSchema()方法

Test改为非mock的McpServer

由于另存的List<Tool>删除,测试读取getTools()需要用到非mock的server。

@BeforeEach
void setup() {
    this.toolExecuteService = mock(ToolExecuteService.class);
    McpServerConfig config = new McpServerConfig();
    this.mcpSyncServer = config.mcpSyncServer(config.fitMcpStreamableServerTransportProvider(), 10);
}

McpJsonMapper

根据0.14.1版本的MCP SDK,transportProvider里的ObjectMapper更新为McpJsonMapper

以及其他代码规范改动

@relat-ivity
Copy link
Contributor Author

relat-ivity commented Nov 5, 2025

变动记录

从原本的handleGethandlePosthandleDelete类中抽取了以下方法:

  • 验证请求合法的方法:
    • validateGetAcceptHeaders()用于验证GET请求头;
    • validatePostAcceptHeaders()用于验证POST请求头;
    • validateRequestSessionId()用于验证请求的mcp-session-id是否存在,以及是否存在相应Session;
  • 根据请求类型调用handle逻辑的方法:
    • handleReplaySseRequest()用于处理恢复断开SSE(GET消息);
    • handleEstablishSseRequest()用于处理建立SSE(GET消息);
    • handleInitializeRequest()用于处理客户端初始化连接(POST消息);
    • handleJsonRpcResponse()用于处理Elicitation中客户端响应(POST消息);
    • handleJsonRpcNotification()用于处理客户端通知(POST消息);
    • handleJsonRpcRequest()用于处理客户端请求(POST消息)。

测试结果

与原本测试结果相同,本次改动不影响功能实现。

@relat-ivity
Copy link
Contributor Author

代码改动

把原来POST中处理非Initialize的客户端请求逻辑,抽出来成handleJsonRpcMessage方法:
image
该方法会验证并获取Session,然后把消息分流给handleJsonRpcResponse() handleJsonRpcNotification()handleJsonRpcRequest()三种方法:
image

测试验证

与原本测试结果相同,本次改动不影响功能实现。

@CodeCasterX CodeCasterX merged commit c32b6cc into ModelEngine-Group:main Nov 7, 2025
1 check passed
@relat-ivity relat-ivity deleted the mcp-sdk branch November 10, 2025 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in: fel Issues in FEL(FIT Expression for LLM) modules type: enhancement A general enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

使用MCP SDK实现的MCP streamable HTTP服务器

3 participants