Skip to content

Conversation

@iggzq
Copy link
Contributor

@iggzq iggzq commented Sep 8, 2025

What is the purpose of the change

FIX #13777: Fix improper exception handling and other minor issues

Brief changelog

  • Fix the incorrect member variable name SPRING_JXM_ENABLED in the NacosBootstrap class. The correct name should be SPRING_JMX_ENABLED.
  • Fix the log message in the request method of the GrpcRequestAcceptor class. The incorrect message "un registered" has been corrected to "unregistered".
  • Refactor the isNamespaceExist method in the NamespaceOperationService class into two separate methods: namespaceExists and validateNamespaceNotExists. This resolves the issue of improper exception catching in other classes caused by misuse of the original method.
  • Update the isNamespaceExist method in the NamespaceValidationRequestFilter class to adapt to the new logic for checking namespace existence.
  • Update the checkNamespaceIdExist method in the NamespaceInnerHandler class to align with the new namespace existence checking logic.
  • Modify test cases in NamespaceInnerHandlerTest and NamespaceValidationRequestFilterTest to reflect the updated namespace existence checking logic.

iggzq and others added 3 commits September 8, 2025 16:51
- 修改"un registered"为"unregistered",规范日志信息表述
- 修改错误的变量定义
- NamespaceOperationService将检查命名空间是否存在方法抽离出来,存在返回true,不存在返回false,使代码更加清晰。
- 修正一些因此修改产生变化的代码逻辑
@CLAassistant
Copy link

CLAassistant commented Sep 8, 2025

CLA assistant check
All committers have signed the CLA.

@iggzq iggzq changed the title [issue #13777] 修复乱用异常捕获处理问题和一些其他编写问题 [issue #13777] Fix improper exception handling and some other basic coding issues Sep 8, 2025
- 优化了 NamespaceOperationService 类中的namespaceExists方法异常打印内容
- 改进了 NamespaceValidationRequestFilter 中的命名空间存在性检查逻辑,将原先逻辑删除,在filter方法里直接调用namespaceOperationService.namespaceExists方法即可
@Service
public class NamespaceOperationService {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please dont change the indent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, the code indentation has been reverted to its original state.

- 调整代码缩进和空格,与原来保持一致
- 调整代码缩进和空格,与原来保持一致
KomachiSion
KomachiSion previously approved these changes Sep 9, 2025
@KomachiSion KomachiSion closed this Sep 9, 2025
@KomachiSion KomachiSion reopened this Sep 9, 2025
@github-actions
Copy link

github-actions bot commented Sep 9, 2025

Thanks for your this PR. 🙏
Please check again for your PR changes whether contains any usage/api/configuration change such as Add new API , Add new configuration, Change default value of configuration.
If so, please add or update documents(markdown type) in docs/next/ for repository nacos-group/nacos-group.github.io


感谢您提交的PR。 🙏
请再次查看您的PR内容,确认是否包含任何使用方式/API/配置参数的变更,如:新增API新增配置参数修改默认配置等操作。
如果是,请确保在提交之前,在仓库nacos-group/nacos-group.github.io中的docs/next/目录下添加或更新文档(markdown格式)。

@wuyfee
Copy link

wuyfee commented Sep 9, 2025

$\color{red}{FAILURE}$
DETAILS
✅ - docker: success
❌ - deploy (standalone & cluster & standalone_auth): failure
❌ - e2e-java-test (standalone & cluster & standalone_auth): skipped
❌ - e2e-go-test (standalone & cluster): skipped
❌ - e2e-cpp-test (standalone & cluster): skipped
❌ - e2e-csharp-test (standalone & cluster): skipped
❌ - e2e-nodejs-test (standalone & cluster): skipped
❌ - e2e-python-test (standalone & cluster): skipped
✅ - clean (standalone & cluster & standalone_auth): success

iggzq and others added 2 commits September 9, 2025 18:21
- 重构 namespaceExists 方法,try-catch包裹默认namespace判断逻辑
- 将原先在NamespaceValidationRequestFilterTest类中的在校验命名空间时报错测试,改到NamespaceOperationServiceTest,因为现在异常处理和NamespaceValidationRequest类无关,和NamespaceOperationService类有关
- 在改到NamespaceOperationServiceTest类中增加异常处理测试用例,验证数据库查询失败时的返回结果
- 在NamespaceOperationServiceTest类中添加默认命名空间和非默认命名空间存在性验证的测试用例
@iggzq
Copy link
Contributor Author

iggzq commented Sep 9, 2025

The Maven test error in GitHub Actions has been resolved. The main issue was that the code logic for verifying the existence of namespaces has changed, and the original test method could no longer meet the testing requirements.

@KomachiSion
Copy link
Collaborator

@iggzq unit test can't pass. Please fix.

@wuyfee
Copy link

wuyfee commented Sep 10, 2025

$\color{red}{FAILURE}$
DETAILS
✅ - docker: success
❌ - deploy (standalone & cluster & standalone_auth): failure
❌ - e2e-java-test (standalone & cluster & standalone_auth): skipped
❌ - e2e-go-test (standalone & cluster): skipped
❌ - e2e-cpp-test (standalone & cluster): skipped
❌ - e2e-csharp-test (standalone & cluster): skipped
❌ - e2e-nodejs-test (standalone & cluster): skipped
❌ - e2e-python-test (standalone & cluster): skipped
✅ - clean (standalone & cluster & standalone_auth): success

iggzq and others added 2 commits September 10, 2025 13:45
- 删除了 checkNamespaceIdExistWithException 测试方法。因为判断命名空间是否存在的逻辑已经发生改变,namespaceInnerHandler.checkNamespaceIdExist此方法无需再捕获NacosApiException异常来设置true或false,所以此测试方法可以直接删除
@iggzq
Copy link
Contributor Author

iggzq commented Sep 10, 2025

@iggzq unit test can't pass. Please fix.

Now it should be completely resolved:

  1. The issue was that previously, the namespaceInnerHandler.checkNamespaceIdExist method relied on catching the thrown NacosApiException to indicate whether a namespace existed. That's why the test was added. Now, the namespaceOperationService.namespaceExists method directly catches the exception and returns a boolean value (true or false), making the original test obsolete and safe to remove.
  2. Running mvn clean install -DskipITs locally passes all tests.

@KomachiSion KomachiSion merged commit 10cb933 into alibaba:develop Sep 11, 2025
3 checks passed
@KomachiSion
Copy link
Collaborator

PR merged, @iggzq Would you mind provider an dingtalk number to invite you join Nacos contributor group.

@KomachiSion KomachiSion added kind/bug Category issues or prs related to bug. kind/code quality area/Nacos Core and removed kind/bug Category issues or prs related to bug. labels Sep 11, 2025
@KomachiSion KomachiSion added this to the 3.1.0 milestone Sep 11, 2025
@iggzq
Copy link
Contributor Author

iggzq commented Sep 11, 2025

PR merged, @iggzq Would you mind provider an dingtalk number to invite you join Nacos contributor group.

ok,my dingtalk number is gaozongqi1024

@wuyfee
Copy link

wuyfee commented Sep 11, 2025

$\color{red}{FAILURE}$
DETAILS
✅ - docker: success
❌ - deploy (standalone & cluster & standalone_auth): failure
❌ - e2e-java-test (standalone & cluster & standalone_auth): skipped
❌ - e2e-go-test (standalone & cluster): skipped
❌ - e2e-cpp-test (standalone & cluster): skipped
❌ - e2e-csharp-test (standalone & cluster): skipped
❌ - e2e-nodejs-test (standalone & cluster): skipped
❌ - e2e-python-test (standalone & cluster): skipped
✅ - clean (standalone & cluster & standalone_auth): success

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

【codeReview】命名空间部分源码存在反直觉设计,异常滥用问题

4 participants