For #13400, Fix naming subscriber get appName is - problem.
#13464
Conversation
|
Thanks for your this PR. 🙏 感谢您提交的PR。 🙏 |
修复appName默认值导致获取异常问题并补充测试用例变更文件
时序图sequenceDiagram
participant Client as GRPC客户端
participant Acceptor as GrpcRequestAcceptor
participant Context as 请求上下文
Client->>Acceptor: 发送请求
Acceptor->>Acceptor: prepareRequestContext()
Acceptor->>Connection: 获取连接元数据appName
alt appName无效（空或"-")
Acceptor->>Request: 读取请求头APP_FILED
Acceptor->>Context: 设置app为请求头值
else appName有效
Acceptor->>Context: 直接使用连接appName
end
Context-->>Acceptor: 完成上下文构建
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
There was a problem hiding this comment.
🔍 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 0 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 0 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 2 | 次要问题，酌情优化。例如：代码格式不规范或注释缺失。 |
总计： 2 个问题
📋 评审意见详情
💡 单文件建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
☕ core/src/test/java/com/alibaba/nacos/core/remote/grpc/GrpcRequestAcceptorTest.java (2 💬)
- 减少测试用例中的重复代码，提取公共方法以提高可维护性 (L376-L414)
- 补充测试用例覆盖appName为默认值且请求头缺失的情况 (L376-L414)
🚀 跨文件建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍 1. 默认应用名称处理逻辑存在架构不一致性风险
新增的DEFAULT_APP_NAME常量在GrpcRequestAcceptor.java中被定义为"-", 但未在其他相关模块中统一使用。当前仅在GrpcRequestAcceptor的prepareRequestContext方法中处理了该默认值逻辑，可能导致以下问题： 1. 其他模块可能仍直接使用"-"字符串进行硬编码，导致维护困难 2. 不同组件对默认应用名称的处理逻辑可能出现不一致 3. 未建立统一的应用名称解析机制，存在重复实现风险 建议将默认应用名称配置集中管理，并建立统一的应用名称解析服务
📌 关键代码：
private static final String DEFAULT_APP_NAME = "-";🔍 2. 应用名称优先级逻辑未在系统层面明确
当connection的appName为默认值时，当前逻辑会从请求头获取应用名称，但未在架构层面定义以下关键规则： 1. 当connection和请求头同时存在有效值时的优先级规则 2. 不同通信协议（如HTTP/REST）间应用名称处理的统一性 3. 客户端/服务端对应用名称格式的契约约定 建议在系统设计文档中明确应用名称的获取策略和优先级规则，并确保各组件实现一致
📌 关键代码：
if (StringUtils.isBlank(app) || StringUtils.equals(DEFAULT_APP_NAME, app)) {
app = request.getHeader(HttpHeaderConsts.APP_FILED, "unknown");
}🔍 3. 测试覆盖未包含关键边界条件
新增的测试用例未覆盖以下重要场景： 1. connection的appName为默认值且请求头也为空时的处理（当前默认返回"unknown"） 2. connection的appName非空且非默认值，但请求头包含有效值时的优先级验证 3. 请求头应用名称字段名称与系统约定不一致时的容错处理 建议补充上述场景的测试用例，并建立参数化测试框架统一验证各种组合情况
📌 关键代码：
// 新增测试用例的实现代码
assertEquals("appNameInConnection", RequestContextHolder.getContext().getBasicContext().getApp());💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@Lingma-Agent 分析这个方法的性能瓶颈并提供优化建议。
-
@Lingma-Agent 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@Lingma-Agent 请总结上述讨论并提出解决方案。
-
@Lingma-Agent 请根据讨论内容生成优化代码。
| @Test | ||
| void testHandleRequestSuccessWithAppName() { | ||
| ApplicationUtils.setStarted(true); | ||
| Mockito.when(requestHandlerRegistry.getByRequestType(Mockito.anyString())).thenReturn(mockHandler); | ||
| Mockito.when(connectionManager.checkValid(Mockito.any())).thenReturn(true); | ||
| String ip = "1.1.1.1"; | ||
| ConnectionMeta connectionMeta = new ConnectionMeta(connectId, ip, ip, 8888, 9848, "GRPC", "", "appNameInConnection", new HashMap<>()); | ||
| Connection connection = new GrpcConnection(connectionMeta, null, null); | ||
| Mockito.when(connectionManager.getConnection(Mockito.any())).thenReturn(connection); | ||
|
|
||
| RequestMeta metadata = new RequestMeta(); | ||
| metadata.setClientIp("127.0.0.1"); | ||
| metadata.setConnectionId(connectId); | ||
| HealthCheckRequest mockRequest = new HealthCheckRequest(); | ||
| Payload payload = GrpcUtils.convert(mockRequest, metadata); | ||
|
|
||
| StreamObserver<Payload> streamObserver = new StreamObserver<Payload>() { | ||
| @Override | ||
| public void onNext(Payload payload) { | ||
| System.out.println("Receive data from server: " + payload); | ||
| Object res = GrpcUtils.parse(payload); | ||
| assertTrue(res instanceof HealthCheckResponse); | ||
| assertEquals("appNameInConnection", RequestContextHolder.getContext().getBasicContext().getApp()); | ||
| } | ||
|
|
||
| @Override | ||
| public void onError(Throwable throwable) { | ||
| fail(throwable.getMessage()); | ||
| } | ||
|
|
||
| @Override | ||
| public void onCompleted() { | ||
| System.out.println("complete"); | ||
| } | ||
| }; | ||
|
|
||
| streamStub.request(payload, streamObserver); | ||
| ApplicationUtils.setStarted(false); | ||
| } |
There was a problem hiding this comment.
减少测试用例中的重复代码，提取公共方法以提高可维护性
🟢 Minor | 🧹 Code Smells
📋 问题详情
测试方法testHandleRequestSuccessWithAppName和testHandleRequestSuccessWithAppNameInHeader存在大量重复代码，包括mock配置、连接和元数据的初始化。重复代码增加了维护成本，当需要修改公共逻辑时需要同时修改多个地方，容易引入错误。
💡 解决方案
将重复的mock配置、连接创建和元数据初始化提取到公共方法中：
+private void setupCommonMocks() {
+ ApplicationUtils.setStarted(true);
+ Mockito.when(requestHandlerRegistry.getByRequestType(Mockito.anyString())).thenReturn(mockHandler);
+ Mockito.when(connectionManager.checkValid(Mockito.any())).thenReturn(true);
+}
+
+private Connection createConnection(String appName) {
+ String ip = "1.1.1.1";
+ ConnectionMeta connectionMeta = new ConnectionMeta(connectId, ip, ip, 8888, 9848, "GRPC", "", appName, new HashMap<>());
+ return new GrpcConnection(connectionMeta, null, null);
+}
+
+private RequestMeta createMetadata() {
+ RequestMeta metadata = new RequestMeta();
+ metadata.setClientIp("127.0.0.1");
+ metadata.setConnectionId(connectId);
+ return metadata;
+}
@Test
void testHandleRequestSuccessWithAppName() {
+ setupCommonMocks();
+ Connection connection = createConnection("appNameInConnection");
+ Mockito.when(connectionManager.getConnection(Mockito.any())).thenReturn(connection);
+
+ RequestMeta metadata = createMetadata();
+ HealthCheckRequest mockRequest = new HealthCheckRequest();
+ Payload payload = GrpcUtils.convert(mockRequest, metadata);
+
+ // 流观察者逻辑保持不变
}
@Test
void testHandleRequestSuccessWithAppNameInHeader() {
+ setupCommonMocks();
+ Connection connection = createConnection("-");
+ Mockito.when(connectionManager.getConnection(Mockito.any())).thenReturn(connection);
+
+ RequestMeta metadata = createMetadata();
+ HealthCheckRequest mockRequest = new HealthCheckRequest();
+ mockRequest.putHeader(HttpHeaderConsts.APP_FILED, "appNameInHeader");
+ Payload payload = GrpcUtils.convert(mockRequest, metadata);
+
+ // 流观察者逻辑保持不变
}您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| @Test | ||
| void testHandleRequestSuccessWithAppName() { | ||
| ApplicationUtils.setStarted(true); | ||
| Mockito.when(requestHandlerRegistry.getByRequestType(Mockito.anyString())).thenReturn(mockHandler); | ||
| Mockito.when(connectionManager.checkValid(Mockito.any())).thenReturn(true); | ||
| String ip = "1.1.1.1"; | ||
| ConnectionMeta connectionMeta = new ConnectionMeta(connectId, ip, ip, 8888, 9848, "GRPC", "", "appNameInConnection", new HashMap<>()); | ||
| Connection connection = new GrpcConnection(connectionMeta, null, null); | ||
| Mockito.when(connectionManager.getConnection(Mockito.any())).thenReturn(connection); | ||
|
|
||
| RequestMeta metadata = new RequestMeta(); | ||
| metadata.setClientIp("127.0.0.1"); | ||
| metadata.setConnectionId(connectId); | ||
| HealthCheckRequest mockRequest = new HealthCheckRequest(); | ||
| Payload payload = GrpcUtils.convert(mockRequest, metadata); | ||
|
|
||
| StreamObserver<Payload> streamObserver = new StreamObserver<Payload>() { | ||
| @Override | ||
| public void onNext(Payload payload) { | ||
| System.out.println("Receive data from server: " + payload); | ||
| Object res = GrpcUtils.parse(payload); | ||
| assertTrue(res instanceof HealthCheckResponse); | ||
| assertEquals("appNameInConnection", RequestContextHolder.getContext().getBasicContext().getApp()); | ||
| } | ||
|
|
||
| @Override | ||
| public void onError(Throwable throwable) { | ||
| fail(throwable.getMessage()); | ||
| } | ||
|
|
||
| @Override | ||
| public void onCompleted() { | ||
| System.out.println("complete"); | ||
| } | ||
| }; | ||
|
|
||
| streamStub.request(payload, streamObserver); | ||
| ApplicationUtils.setStarted(false); | ||
| } |
There was a problem hiding this comment.
补充测试用例覆盖appName为默认值且请求头缺失的情况
🟢 Minor | 🧹 Code Smells
📋 问题详情
当前测试未覆盖当connection的appName为-且请求头未设置APP_FILED时的逻辑。根据代码逻辑，此时应使用默认值unknown，但缺少对应的测试验证。
💡 解决方案
添加新测试用例验证默认情况：
+@Test
+void testHandleRequestWithDefaultAppNameAndMissingHeader() {
+ setupCommonMocks();
+ Connection connection = createConnection("-");
+ Mockito.when(connectionManager.getConnection(Mockito.any())).thenReturn(connection);
+
+ RequestMeta metadata = createMetadata();
+ HealthCheckRequest mockRequest = new HealthCheckRequest();
+ Payload payload = GrpcUtils.convert(mockRequest, metadata);
+
+ StreamObserver<Payload> streamObserver = new StreamObserver<Payload>() {
+ @Override
+ public void onNext(Payload payload) {
+ assertEquals("unknown", RequestContextHolder.getContext().getBasicContext().getApp());
+ }
+ };
+ streamStub.request(payload, streamObserver);
+ ApplicationUtils.setStarted(false);
+}您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #13464 +/- ##
=============================================
- Coverage 70.52% 70.51% -0.02%
+ Complexity 11912 11911 -1
=============================================
Files 1621 1621
Lines 51826 51826
Branches 5210 5210
=============================================
- Hits 36552 36545 -7
- Misses 12826 12831 +5
- Partials 2448 2450 +2
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
…libaba#13464) (cherry picked from commit bd7a1dd)
|
|
Please do not create a Pull Request without creating an issue first.
What is the purpose of the change
Fix #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.