For #13400, Fix naming subscriber get appName is - problem. (#13464)
#13465
Conversation
…libaba#13464) (cherry picked from commit bd7a1dd)
|
Thanks for your this PR. 🙏 感谢您提交的PR。 🙏 |
修复当连接的appName无效时从请求头获取应用名称的逻辑变更文件
时序图sequenceDiagram
participant G as GrpcRequestAcceptor
participant C as ConnectionMeta
participant H as HealthCheckRequest
G->>C: 获取connection的appName
alt appName无效（空或"-")
G->>H: 从请求头获取APP_FILED值
H-->>G: 返回appName
else appName有效
G-->>G: 直接使用connection的appName
end
G->>RequestContext: 设置最终的app值
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
There was a problem hiding this comment.
🔍 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 0 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 0 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 1 | 次要问题，酌情优化。例如：代码格式不规范或注释缺失。 |
总计： 1 个问题
📋 评审意见详情
💡 单文件建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
☕ core/src/test/java/com/alibaba/nacos/core/remote/grpc/GrpcRequestAcceptorTest.java (1 💬)
- 测试用例未覆盖appName为`-`但请求头未设置的情况 (L320)
🚀 跨文件建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍 1. 测试覆盖未全面覆盖所有边界条件
新增的appName处理逻辑未完全覆盖所有边界条件。例如，当连接的appName为'-'且请求头未设置APP_FILED时，应设置为'unknown'，但现有测试未覆盖此情况。系统测试策略需要确保所有条件分支（如connection的appName为'-'但请求头为空）均被验证，以避免潜在逻辑漏洞。
🔍 2. 代码逻辑复杂度增加，建议重构
在GrpcRequestAcceptor的prepareRequestContext方法中，新增的条件判断逻辑（检查appName是否为'-'或空）直接嵌入方法内，可能增加代码复杂度。建议将appName解析逻辑提取为独立方法或工具类，以提高可维护性和一致性，避免未来修改时遗漏条件。
📌 关键代码：
if (StringUtils.isBlank(app) || StringUtils.equals(DEFAULT_APP_NAME, app)) {
app = request.getHeader(HttpHeaderConsts.APP_FILED, "unknown");
}🔍 3. 系统行为变更未充分评估兼容性
改动改变了appName的解析逻辑（原仅检查空值，现扩展至检查'-'），可能影响其他依赖原有行为的组件（如日志模块、权限校验模块）。需确认系统内其他组件是否适配此变更，或是否存在未覆盖的依赖关系。
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@Lingma-Agent 分析这个方法的性能瓶颈并提供优化建议。
-
@Lingma-Agent 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@Lingma-Agent 请总结上述讨论并提出解决方案。
-
@Lingma-Agent 请根据讨论内容生成优化代码。
| @@ -309,6 +317,7 @@ public void onNext(Payload payload) { | |||
| System.out.println("Receive data from server: " + payload); | |||
| Object res = GrpcUtils.parse(payload); | |||
| assertTrue(res instanceof HealthCheckResponse); | |||
| assertEquals("unknown", RequestContextHolder.getContext().getBasicContext().getApp()); | |||
There was a problem hiding this comment.
测试用例未覆盖appName为-但请求头未设置的情况
🟢 Minor | 🧹 Code Smells
📋 问题详情
新增的测试用例testHandleRequestSuccessWithAppNameInHeader验证了当连接的appName为-且请求头存在时的行为，但未测试当连接appName为-且请求头无APP_FIELD字段时的逻辑。该场景应返回默认值unknown。
💡 解决方案
新增测试用例覆盖以下场景：
+ @Test
+ void testHandleRequestSuccessWithDefaultAppName() {
+ ApplicationUtils.setStarted(true);
+ Mockito.when(requestHandlerRegistry.getByRequestType(Mockito.anyString())).thenReturn(mockHandler);
+ Mockito.when(connectionManager.checkValid(Mockito.any())).thenReturn(true);
+ ConnectionMeta connectionMeta = new ConnectionMeta("id", "ip", "ip", 8888, 9848, "GRPC", "", "-", new HashMap<>());
+ Mockito.when(connectionManager.getConnection(Mockito.any())).thenReturn(new GrpcConnection(connectionMeta, null, null));
+
+ RequestMeta metadata = new RequestMeta();
+ HealthCheckRequest mockRequest = new HealthCheckRequest();
+ Payload payload = GrpcUtils.convert(mockRequest, metadata);
+
+ StreamObserver<Payload> observer = new StreamObserver<Payload>() {
+ @Override
+ public void onNext(Payload p) {
+ assertEquals("unknown", RequestContextHolder.getContext().getBasicContext().getApp());
+ }
+ // ... 其他实现
+ };
+ streamStub.request(payload, observer);
+ }您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
(cherry picked from commit bd7a1dd)
Please do not create a Pull Request without creating an issue first.
What is the purpose of the change
Fixes #13400
Brief changelog
XX
Verifying this change
XXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
[ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=trueto make sure basic checks pass. Runmvn clean install -DskipITsto make sure unit-test pass. Runmvn clean test-compile failsafe:integration-testto make sure integration-test pass.