feat: add determine appid+cluster namespace num limit logic #5228
Conversation
dosubot
bot
added
the
size:M
WalkthroughThe changes introduce a new feature for managing namespace limits within the Apollo configuration system. This includes the addition of methods in Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
I saw that PR #5227 was closed. #5227 and #5228 are actually two different limitations. In order to keep the functions simple, they are divided into two different PR. The reasons for the following two PRs |
|
#5227:
|
|
#5228:
|
In order to maintain compatibility, the current practice is to set a relatively large default value, but it is still inevitable that the running appid may exceed this limit By adding a switch to control the opening or closing of this function, it can bring better compatibility and greater user selectivity Once the switch is added, I think the default value can be appropriately lowered, or there is a reasonable default value recommended |
|
Thank you for the background information. Here are some suggestions:
|
dosubot
bot
added
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (3)
42-42: LGTM: Default namespace limit constant added.The addition of
DEFAULT_MAX_NAMESPACE_NUMwith a value of 200 is appropriate for implementing the namespace limit feature.Consider adding a comment explaining the rationale behind choosing 200 as the default limit. This would help future maintainers understand the decision-making process.
107-109: LGTM: Method added to check if namespace limit is enabled.The
isNamespaceNumLimitEnabled()method correctly implements a switch for the namespace limit feature, with a default value offalseensuring backward compatibility.Consider adding a Javadoc comment for this method to explain its purpose and the configuration property it uses. This would improve the API documentation.
111-118: LGTM: Methods added for namespace limit and whitelist.The
namespaceNumLimit()andnamespaceNumLimitWhite()methods correctly implement the namespace limit and whitelist functionality as described in the PR objectives.Points of note:
namespaceNumLimit()usescheckInt()to ensure the limit is within a valid range.namespaceNumLimitWhite()correctly usesSets.newHashSet()to create a set from the array.Consider adding Javadoc comments for both methods to explain their purpose, the configuration properties they use, and the return value semantics. This would improve the API documentation and make it easier for users to understand how to configure these features.
docs/zh/portal/apollo-user-guide.md (2)
485-504: New section on namespace number limit looks good, but consider some improvements.The new section "6.4 单个命名空间下的配置项数量限制" (Limit on the number of configuration items under a single namespace) is a valuable addition to the user guide. It clearly explains the new feature introduced in version 2.4.0. However, I have a few suggestions to enhance its clarity and consistency:
The title mentions "配置项数量限制" (configuration item number limit), but the content discusses "命名空间数量上限" (namespace number limit). Consider aligning the title with the content for consistency.
The explanation of the
namespace.num.limit.enabledparameter could be more explicit about its default value.The image links are using relative paths, which is inconsistent with other image links in the document that use absolute URLs. Consider updating these for consistency.
There's an extra newline at the end of the section that could be removed for consistency with other sections.
Consider applying these changes:
- Update the section title:
-## 6.4 单个命名空间下的配置项数量限制 +## 6.4 单个应用集群下的命名空间数量限制
- Clarify the default value:
-2. 进入 `管理员工具 - 系统参数 - ConfigDB 配置管理` 页面新增或修改 `namespace.num.limit.enabled` 配置项为true/false 即可开启/关闭此功能，默认关闭 +2. 进入 `管理员工具 - 系统参数 - ConfigDB 配置管理` 页面新增或修改 `namespace.num.limit.enabled` 配置项。设置为true开启功能，设置为false关闭功能。此功能默认关闭（即默认值为false）。
- Update image links to use absolute URLs for consistency:
- + - + - +
- Remove the extra newline at the end of the section.
Line range hint
1-1: Consider updating the table of contents to include the new section.To improve discoverability of the new namespace number limit feature, consider adding it to the table of contents at the beginning of the document.
Add the following line to the table of contents, under the "六、其它功能配置" section:
* [6.4 单个应用集群下的命名空间数量限制](#64-单个应用集群下的命名空间数量限制)This will help users quickly find information about this new feature.
docs/en/portal/apollo-user-guide.md (1)
514-535: LGTM! Consider improving the English translation slightly.The new section "6.4 单个命名空间下的配置项数量限制" (Configuration item quantity limit under a single namespace) is well-documented and provides clear instructions for setting up the new namespace limit feature introduced in Apollo 2.4.0. The feature seems well-designed, offering flexibility with its configurable limit and whitelist option.
Consider updating the English title to "6.4 Namespace Quantity Limit per AppId+Cluster" for better clarity and consistency with the English documentation style.
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java (1)
175-177: Enhance exception assertion for clearer test validationCurrently, the test only checks that a
ServiceExceptionis thrown, but it does not verify the exception message. To ensure the exception is due to exceeding the namespace limit and not another issue, consider asserting the exception message.Modify the catch block to include an assertion on the exception message:
} catch (Exception e) { Assert.assertTrue(e instanceof ServiceException); + Assert.assertEquals("Namespace number limit exceeded", e.getMessage()); }Please replace
"Namespace number limit exceeded"with the actual expected exception message.apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/NamespaceService.java (2)
358-358: Optimize namespace count query for performance.The
countByAppIdAndClusterNamemethod could impact performance with large datasets.Suggestion:
Ensure there are appropriate indexes on the
appIdandclusterNamecolumns to optimize the count query.
360-360: Enhance error message clarity.The
ServiceExceptionmessage can be made more user-friendly.Suggestion:
Revise the error message for better readability:
- throw new ServiceException("namespace[appId = " + entity.getAppId() + ", cluster= " + entity.getClusterName() + "] nowCount= " + nowCount + ", maxCount =" + bizConfig.namespaceNumLimit()); + throw new ServiceException("The maximum number of namespaces (" + bizConfig.namespaceNumLimit() + ") has been reached for appId '" + entity.getAppId() + "' and cluster '" + entity.getClusterName() + "'. Current count: " + nowCount + ".");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
doc/images/namespace-num-limit-enabled.pngis excluded by!**/*.pngdoc/images/namespace-num-limit-white.pngis excluded by!**/*.pngdoc/images/namespace-num-limit.pngis excluded by!**/*.png
📒 Files selected for processing (6)
- CHANGES.md (1 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (3 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/NamespaceService.java (5 hunks)
- apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java (4 hunks)
- docs/en/portal/apollo-user-guide.md (1 hunks)
- docs/zh/portal/apollo-user-guide.md (2 hunks)
🔇 Additional comments (11)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (2)
24-24: LGTM: Import added for Sets utility.The addition of
import com.google.common.collect.Sets;is appropriate for the new functionality and is correctly placed with other Guava imports.
Line range hint
24-118: Overall implementation looks good, with minor suggestions for improvement.The changes to
BizConfig.javasuccessfully implement the namespace limit and whitelist functionality as described in the PR objectives. The implementation is correct, follows good practices, and addresses the concerns raised in the PR comments, particularly regarding backward compatibility.Key points:
- The feature is disabled by default, ensuring backward compatibility.
- The limit and whitelist are configurable, providing flexibility for different use cases.
- The implementation uses appropriate data structures and validation.
Suggestions for improvement:
- Add Javadoc comments to the new methods to improve API documentation.
- Consider adding a comment explaining the rationale behind the default namespace limit of 200.
These changes enhance the configuration capabilities of Apollo while maintaining flexibility and backward compatibility.
docs/en/portal/apollo-user-guide.md (1)
536-538: Existing security best practices remain relevant and crucial.While there are no changes in this section, it's worth emphasizing the continued importance of the security practices outlined here. These guidelines provide valuable advice for maintaining the security of Apollo as a configuration center.
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java (4)
151-182: TesttestNamespaceNumLimit()correctly validates namespace limit enforcementThe test method properly configures the
bizConfigto enable the namespace limit and sets the limit to 2. It verifies that saving a namespace beyond the limit throws aServiceException, and it confirms the total count of namespaces remains at the limit.
184-211: TesttestNamespaceNumLimitFalse()accurately checks behavior when limit is disabledThe test effectively confirms that when the namespace limit is not enabled, multiple namespaces can be created without throwing exceptions, and the total count reflects all the namespaces saved.
213-242: TesttestNamespaceNumLimitWhite()properly verifies whitelist functionalityThe test correctly sets up the
bizConfigwith the namespace limit enabled and adds thetestAppto the whitelist. It confirms that even with the limit enforced, whitelisted apps can create additional namespaces without exceptions, and the total count includes all namespaces.
74-78: Ensure correct usage of@Mockand@AutowiredannotationsMixing
@Mockand@Autowiredmay lead to unintended behavior. SincebizConfigis a mock, ensure that it is properly injected intonamespaceService. Also, consider using@InjectMocksto create thenamespaceServiceinstance with the mockedbizConfig.Run the following script to check if
bizConfigis correctly injected:apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/NamespaceService.java (4)
19-19: Added import forBizConfig.The import statement for
BizConfigis correctly added to support the new namespace limit feature.
72-72: DeclaredbizConfigmember variable.The
bizConfigvariable is properly declared asprivate final, ensuring immutability after construction.
86-87: Updated constructor to includeBizConfig.The constructor now includes
BizConfigas a parameter, which is necessary for injecting configuration settings.
100-100: AssignedbizConfigin constructor.The
bizConfiginstance is correctly assigned to the class member variable, ensuring it is available throughout the class.
...iz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java
Outdated
Show resolved
Hide resolved
...iz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java
Outdated
Show resolved
Hide resolved
...iz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java
Outdated
Show resolved
Hide resolved
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/NamespaceService.java
Show resolved
Hide resolved
# Conflicts: # CHANGES.md
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/NamespaceService.java
Show resolved
Hide resolved
...iz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java
Outdated
Show resolved
Hide resolved
...iz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java
Outdated
Show resolved
Hide resolved
...iz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java (2)
151-180: LGTM: testNamespaceNumLimit method is well-structured.The test method effectively verifies the namespace number limit functionality. It correctly sets up the mocked configuration, attempts to save namespaces, and checks for the expected exception and final count.
A minor suggestion for improvement:
Consider using
assertThrowsinstead of a try-catch block for a more concise and expressive test:Assert.assertThrows(ServiceException.class, () -> { Namespace namespace2 = new Namespace(); namespace2.setAppId(testApp); namespace2.setClusterName(testCluster); namespace2.setNamespaceName("demo-namespace2"); namespaceService.save(namespace2); });
151-232: Good test coverage for namespace limit feature, with room for minor improvements.The addition of three new test methods (
testNamespaceNumLimit,testNamespaceNumLimitFalse, andtestNamespaceNumLimitWhite) provides comprehensive coverage for the namespace limit feature. These tests effectively verify the behavior when limits are enforced, disabled, and when an app is whitelisted.To further enhance the test suite:
Consider adding a test case for an app that is not whitelisted when the limit is enforced, to contrast with the whitelisted case.
Ensure consistent use of mocking across all tests, avoiding the use of
ReflectionTestUtilsas suggested in previous comments.Consider parameterizing some tests to cover edge cases, such as testing with different limit values.
Add comments to each test method explaining the specific scenario being tested, which will improve readability and maintainability.
These improvements will make the test suite more robust and easier to maintain in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- CHANGES.md (1 hunks)
- apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java (4 hunks)
- docs/en/portal/apollo-user-guide.md (1 hunks)
- docs/zh/portal/apollo-user-guide.md (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGES.md
- docs/en/portal/apollo-user-guide.md
- docs/zh/portal/apollo-user-guide.md
🧰 Additional context used
📓 Learnings (1)
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java (1)
Learnt from: nobodyiam PR: apolloconfig/apollo#5228 File: apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/NamespaceService.java:356-363 Timestamp: 2024-10-01T07:29:12.146Z Learning: In the `NamespaceService#save` method, implementing a soft limit for namespace counts is acceptable, even if there may be potential race conditions due to concurrent access.
🔇 Additional comments (1)
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java (1)
20-20: LGTM: New imports and field declarations are appropriate.The added imports and field declarations are necessary for the new test methods. The use of
@MockBeanforbizConfigis a good practice for mocking configuration in tests.Also applies to: 29-29, 32-32, 35-38, 41-41, 47-47, 53-53, 74-78
...iz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java
Outdated
Show resolved
Hide resolved
...iz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java
Outdated
Show resolved
Hide resolved
|
@nobodyiam I can't reply to messages in #5227 Reply to #5227 (comment) Reply to #5227 (comment) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java (1)
Line range hint
1-231: Overall: Effective implementation of namespace limit testsThe additions to
NamespaceServiceIntegrationTestsuccessfully implement tests for the new namespace limit functionality. The new tests cover various scenarios:
- Enforced namespace limit
- Disabled namespace limit
- Whitelisted apps with enforced limit
These tests align well with the PR objectives and address the concerns raised in the PR comments. The use of
@MockBeanforBizConfigallows for flexible testing of different configurations.The changes are consistent with the existing code style and testing practices. They provide good coverage for the new feature and will help ensure its correct functionality.
Consider adding a test case for an edge case, such as when the number of existing namespaces is exactly at the limit. This would further strengthen the test suite.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- CHANGES.md (1 hunks)
- apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGES.md
🧰 Additional context used
📓 Learnings (1)
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java (1)
Learnt from: nobodyiam PR: apolloconfig/apollo#5228 File: apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/NamespaceService.java:356-363 Timestamp: 2024-10-01T07:29:12.146Z Learning: In the `NamespaceService#save` method, implementing a soft limit for namespace counts is acceptable, even if there may be potential race conditions due to concurrent access.
🔇 Additional comments (2)
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java (2)
20-20: LGTM: Necessary imports and fields added for new testsThe new imports, MockBean for BizConfig, and Autowired NamespaceRepository are appropriate additions for the new test methods. These changes are consistent with the existing code structure and testing practices.
Also applies to: 29-29, 32-32, 35-35, 37-37, 41-41, 47-47, 53-53, 74-78
206-230: LGTM: Well-structured test for whitelisted appsThe
testNamespaceNumLimitWhitemethod effectively verifies the behavior for whitelisted apps when the namespace limit is enforced. The test setup, including the mocking ofBizConfigand the whitelist, is clear and appropriate. The test correctly demonstrates that whitelisted apps can exceed the namespace limit.The structure and assertions in this test are clear and consistent with the other tests in the class. Good job on maintaining consistency and clarity in the test suite.
| @Test | ||
| @Sql(scripts = "/sql/namespace-test.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD) | ||
| @Sql(scripts = "/sql/clean.sql", executionPhase = Sql.ExecutionPhase.AFTER_TEST_METHOD) | ||
| public void testNamespaceNumLimit() { | ||
|
|
||
| when(bizConfig.isNamespaceNumLimitEnabled()).thenReturn(true); | ||
| when(bizConfig.namespaceNumLimit()).thenReturn(2); | ||
|
|
||
| Namespace namespace = new Namespace(); | ||
| namespace.setAppId(testApp); | ||
| namespace.setClusterName(testCluster); | ||
| namespace.setNamespaceName("demo-namespace"); | ||
| namespaceService.save(namespace); | ||
|
|
||
| try { | ||
| Namespace namespace2 = new Namespace(); | ||
| namespace2.setAppId(testApp); | ||
| namespace2.setClusterName(testCluster); | ||
| namespace2.setNamespaceName("demo-namespace2"); | ||
| namespaceService.save(namespace2); | ||
|
|
||
| Assert.fail(); | ||
| } catch (Exception e) { | ||
| Assert.assertTrue(e instanceof ServiceException); | ||
| } | ||
|
|
||
| int nowCount = namespaceRepository.countByAppIdAndClusterName(testApp, testCluster); | ||
| Assert.assertEquals(2, nowCount); | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling in testNamespaceNumLimit
The test effectively verifies the namespace number limit functionality. However, the exception handling can be improved for better clarity and specificity.
Consider using JUnit's assertThrows method to check for the ServiceException. This will make the test more readable and specific about the expected exception. Here's a suggested refactoring:
@Test
@Sql(scripts = "/sql/namespace-test.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD)
@Sql(scripts = "/sql/clean.sql", executionPhase = Sql.ExecutionPhase.AFTER_TEST_METHOD)
public void testNamespaceNumLimit() {
when(bizConfig.isNamespaceNumLimitEnabled()).thenReturn(true);
when(bizConfig.namespaceNumLimit()).thenReturn(2);
Namespace namespace = new Namespace();
namespace.setAppId(testApp);
namespace.setClusterName(testCluster);
namespace.setNamespaceName("demo-namespace");
namespaceService.save(namespace);
Namespace namespace2 = new Namespace();
namespace2.setAppId(testApp);
namespace2.setClusterName(testCluster);
namespace2.setNamespaceName("demo-namespace2");
assertThrows(ServiceException.class, () -> namespaceService.save(namespace2));
int nowCount = namespaceRepository.countByAppIdAndClusterName(testApp, testCluster);
Assert.assertEquals(2, nowCount);
}This refactoring improves readability and makes the test's intention clearer.
| @Test | ||
| @Sql(scripts = "/sql/namespace-test.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD) | ||
| @Sql(scripts = "/sql/clean.sql", executionPhase = Sql.ExecutionPhase.AFTER_TEST_METHOD) | ||
| public void testNamespaceNumLimitFalse() { | ||
|
|
||
| when(bizConfig.namespaceNumLimit()).thenReturn(2); | ||
|
|
||
| Namespace namespace = new Namespace(); | ||
| namespace.setAppId(testApp); | ||
| namespace.setClusterName(testCluster); | ||
| namespace.setNamespaceName("demo-namespace"); | ||
| namespaceService.save(namespace); | ||
|
|
||
| Namespace namespace2 = new Namespace(); | ||
| namespace2.setAppId(testApp); | ||
| namespace2.setClusterName(testCluster); | ||
| namespace2.setNamespaceName("demo-namespace2"); | ||
| namespaceService.save(namespace2); | ||
|
|
||
| int nowCount = namespaceRepository.countByAppIdAndClusterName(testApp, testCluster); | ||
| Assert.assertEquals(3, nowCount); | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test setup in testNamespaceNumLimitFalse
The test effectively verifies the behavior when the namespace limit is not enforced. However, it could be improved by explicitly setting isNamespaceNumLimitEnabled to false for clarity.
Consider adding an explicit mock for isNamespaceNumLimitEnabled(). This will make the test's intention clearer. Here's a suggested improvement:
@Test
@Sql(scripts = "/sql/namespace-test.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD)
@Sql(scripts = "/sql/clean.sql", executionPhase = Sql.ExecutionPhase.AFTER_TEST_METHOD)
public void testNamespaceNumLimitFalse() {
when(bizConfig.isNamespaceNumLimitEnabled()).thenReturn(false);
when(bizConfig.namespaceNumLimit()).thenReturn(2);
Namespace namespace = new Namespace();
namespace.setAppId(testApp);
namespace.setClusterName(testCluster);
namespace.setNamespaceName("demo-namespace");
namespaceService.save(namespace);
Namespace namespace2 = new Namespace();
namespace2.setAppId(testApp);
namespace2.setClusterName(testCluster);
namespace2.setNamespaceName("demo-namespace2");
namespaceService.save(namespace2);
int nowCount = namespaceRepository.countByAppIdAndClusterName(testApp, testCluster);
Assert.assertEquals(3, nowCount);
}This change makes it explicit that the namespace limit is disabled, improving the test's readability and intent.
@youngzil It was locked after being closed, but I've now unlocked it, so we can continue our discussion in that thread. |
What's the purpose of this PR
Added a new parameter
namespace.num.limit，The purpose is to limit the number of namespaces in the same appid+clusterAdded a new parameter
namespace.num.limit.white，Namespace num limit whitelist, The purpose is to allow some special appids to skip the namespace num limitBecause of the openapi interface, some clients abuse the namespace, so there needs to be a limit on the number of namespace
Which issue(s) this PR fixes:
Fixes #
Brief changelog
XXXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
New Features
Documentation