feat: add determine single namespace item num limit logic #5227
Conversation
dosubot
bot
added
the
size:M
WalkthroughThe changes introduced involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ItemController
participant ItemService
participant BizConfig
User->>ItemController: createItem()
ItemController->>BizConfig: get itemNumLimit()
BizConfig-->>ItemController: return limit
ItemController->>ItemService: findNonEmptyItemCount(namespaceId)
ItemService-->>ItemController: return count
ItemController->>ItemController: validate count against limit
ItemController-->>User: BadRequestException if limit exceeded
ItemController-->>User: Item created successfully if valid
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
|
Duplicated with #5228 |
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: 4
🧹 Outside diff range and nitpick comments (14)
CHANGES.md (1)
11-11: LGTM! Consider adding a brief description of the feature's purpose.The addition to the changelog accurately reflects the new feature for limiting the number of items under a single namespace. The format is consistent with other entries, and the link to the pull request is correctly included.
To provide more context for users, consider expanding the changelog entry slightly. For example:
-* [Feature added limit the number of items under a single namespace](https://github.com/apolloconfig/apollo/pull/5228) +* [Feature: Added limit on the number of items under a single namespace to improve system stability](https://github.com/apolloconfig/apollo/pull/5228)This minor enhancement would give users a quick understanding of why this feature was added without needing to refer to the pull request directly.
apollo-biz/src/test/resources/sql/itemset-test.sql (2)
17-17: LGTM: Namespace INSERT statement is correct. Consider improving readability.The INSERT statement for the Namespace table is correctly formatted and contains appropriate test data.
For improved readability, consider formatting the INSERT statement with each field on a new line:
INSERT INTO "Namespace" ( `Id`, `AppId`, `ClusterName`, `NamespaceName`, `IsDeleted`, `DataChange_CreatedBy`, `DataChange_LastModifiedBy` ) VALUES ( 1, 'testApp', 'default', 'application', 0, 'apollo', 'apollo' );
19-25: LGTM: Item INSERT statements are correct. Consider improving consistency.The INSERT statements for the Item table are correctly formatted and contain appropriate test data with varying types.
For consistency with SQL conventions and improved readability, consider the following changes:
- Use single quotes for string literals consistently (currently, "Key", "Type", and "Value" use double quotes).
- Align the VALUES for better readability.
Here's an example of how it could look:
INSERT INTO `Item` (`Id`, `NamespaceId`, `Key`, `Type`, `Value`, `Comment`, `LineNum`) VALUES (9901, 1, 'k1', 0, 'v1', '', 1), (9902, 1, 'k2', 2, 'v2', '', 2), (9903, 1, 'k3', 0, 'v3', '', 3), (9904, 1, 'k4', 0, 'v4', '', 4), (9905, 1, 'k5', 0, 'v5', '', 5);apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/repository/ItemRepository.java (1)
67-68: LGTM! Consider adding a comment and verifying the return type.The new method
countByNamespaceIdAndFilterKeyEmptyis well-implemented and aligns with the PR objectives. It correctly counts items by namespaceId while excluding items with empty keys.Some suggestions for improvement:
- Add a comment explaining why items with empty keys are excluded from the count.
- Verify if
intis sufficient for the expected count range, or iflongwould be more appropriate for future-proofing.- Ensure that a test case is added for this new method.
Consider adding a comment above the method:
/** * Counts the number of items in a namespace, excluding items with empty keys. * This is used to enforce item count limits within a namespace. * * @param namespaceId the ID of the namespace * @return the count of items with non-empty keys in the specified namespace */ @Query("select count(*) from Item where namespaceId = :namespaceId and key <>''") int countByNamespaceIdAndFilterKeyEmpty(@Param("namespaceId") long namespaceId);apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemSetServiceTest.java (4)
32-56: Consider enhancing test assertions.The test successfully verifies the item count after the update. However, it would be more robust to also assert the content of the added items. This ensures not only the correct number of items but also their integrity.
Consider adding assertions to verify the content of the newly added items. For example:
List<Item> items = itemService.findItems(namespace.getId()); Assert.assertTrue(items.stream().anyMatch(item -> item.getKey().equals("k6") && item.getValue().equals("v6"))); Assert.assertTrue(items.stream().anyMatch(item -> item.getKey().equals("k7") && item.getValue().equals("v7")));
58-87: Enhance test to verify item state after failed update.The test correctly verifies that a
BadRequestExceptionis thrown and that the item count remains at 5. However, it would be more comprehensive to also verify that the state of the items remains unchanged after the failed update.Consider adding assertions to verify that the items' state remains unchanged after the failed update. For example:
List<Item> items = itemService.findItems(namespace.getId()); Assert.assertEquals(5, items.size()); Assert.assertTrue(items.stream().anyMatch(item -> item.getId().equals(9901L) && item.getValue().equals(item9901.getValue()))); Assert.assertTrue(items.stream().anyMatch(item -> item.getId().equals(9902L))); Assert.assertFalse(items.stream().anyMatch(item -> item.getKey().equals("k6") || item.getKey().equals("k7")));
89-116: Enhance test to verify item state after successful update.The test correctly verifies that the update succeeds and the item count remains at 5. However, it would be more comprehensive to also verify the state of the items after the successful update.
Consider adding assertions to verify the state of the items after the successful update. For example:
List<Item> items = itemService.findItems(namespace.getId()); Assert.assertEquals(5, items.size()); Assert.assertTrue(items.stream().anyMatch(item -> item.getId().equals(9901L) && item.getValue().equals(item9901.getValue() + " update"))); Assert.assertFalse(items.stream().anyMatch(item -> item.getId().equals(9902L))); Assert.assertTrue(items.stream().anyMatch(item -> item.getKey().equals("k6") && item.getValue().equals("v6")));
1-126: Overall, well-structured and comprehensive test suite.The test file covers the main scenarios for the
ItemSetService, including cases with and without item number limits. The use of SQL scripts for setup and cleanup is commendable.To further enhance the test suite:
- Consider adding more detailed assertions about the state of items after updates in each test method, as suggested in previous comments.
- It might be beneficial to add a test case that checks the behavior when the item count is exactly at the limit (e.g., 5 items with a limit of 5).
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemSetService.java (1)
72-80: LGTM: Item number limit logic implemented correctly.The new logic to enforce the item number limit is well-implemented and consistent with the PR objectives. It correctly considers existing items, newly created items, and deleted items in the calculation.
Suggestions for minor improvements:
- Consider extracting
bizConfig.itemNumLimit()to a constant for better readability.- The error message could include the namespace details (appId, clusterName, namespaceName) for more specific information.
Here's a suggested improvement for the error message:
- throw new BadRequestException("current namespace item count=[" + itemCount + "], single namespace max allow count=[ " + bizConfig.itemNumLimit() + "]"); + throw new BadRequestException(String.format("Namespace %s.%s.%s: current item count=[%d], max allowed count=[%d]", appId, clusterName, namespaceName, itemCount, bizConfig.itemNumLimit()));apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (2)
105-107: LGTM: New method to check if item number limit is enabled.The
isItemNumLimitEnabled()method is well-implemented and consistent with the class's style. It provides a clear way to determine if the item number limit feature is enabled.Consider adding a brief Javadoc comment to explain the purpose of this method and the configuration property it uses. For example:
/** * Checks if the item number limit feature is enabled. * @return true if enabled, false otherwise */ public boolean isItemNumLimitEnabled() { return getBooleanProperty("item.num.limit.enabled", false); }
109-112: LGTM: New method to retrieve the item number limit.The
itemNumLimit()method is well-implemented and consistent with the class's style. It provides a clear way to retrieve the configured item number limit while ensuring the value is within a reasonable range.Consider adding a brief Javadoc comment to explain the purpose of this method, the configuration property it uses, and the valid range of values. For example:
/** * Retrieves the configured item number limit. * @return the item number limit (between 5 and Integer.MAX_VALUE) */ public int itemNumLimit() { int limit = getIntProperty("item.num.limit", DEFAULT_MAX_ITEM_NUM); return checkInt(limit, 5, Integer.MAX_VALUE, DEFAULT_MAX_ITEM_NUM); }apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemService.java (1)
138-140: Approve the new method with suggestions for improvement.The new
findNonEmptyItemCountmethod is a good addition that aligns with the PR objectives. However, consider the following suggestions:
- Add JavaDoc comments to clarify the method's purpose and its relation to the
item.num.limitfeature.- Consider the performance implications of this count operation, especially for large namespaces. You might want to add a comment about potential optimization if needed in the future.
Here's a suggested improvement with added documentation:
/** * Counts the number of non-empty items in a given namespace. * This method is used to support the item.num.limit feature for limiting * the number of configuration items within the same namespace. * * @param namespaceId the ID of the namespace * @return the count of non-empty items in the namespace * @see BizConfig#itemNumLimit() for the actual limit value */ public int findNonEmptyItemCount(long namespaceId) { // TODO: Consider adding caching or other optimization if this becomes a performance bottleneck return itemRepository.countByNamespaceIdAndFilterKeyEmpty(namespaceId); }docs/zh/portal/apollo-user-guide.md (1)
485-501: New section on configuration item limit looks good, with a minor suggestion.The new section 6.4 introduces a useful feature for limiting the number of configuration items in a single namespace. The content is clear and provides necessary information about the feature, its default state, and how to configure it. However, I have a small suggestion to improve consistency and clarity:
Consider adding a brief explanation of why this feature might be useful (e.g., to prevent performance issues or to encourage better organization of configurations).
The image links are using relative paths, which is good for maintainability. However, ensure that these images are actually present in the specified locations (
../../../doc/images/).Consider adding a brief explanation of the feature's benefits, such as:
## 6.4 单个命名空间下的配置项数量限制 -从2.4.0版本开始，apollo-portal提供了限制单个命名空间下的配置项数量的功能，此功能默认关闭，需要配置系统 `item.num.limit.enabled` 开启，同时提供了系统参数`item.num.limit`来动态配置单个Namespace下的item数量上限值 +从2.4.0版本开始，apollo-portal提供了限制单个命名空间下的配置项数量的功能。此功能有助于防止性能问题并鼓励更好地组织配置。此功能默认关闭，需要配置系统 `item.num.limit.enabled` 开启，同时提供了系统参数`item.num.limit`来动态配置单个Namespace下的item数量上限值docs/en/portal/apollo-user-guide.md (1)
512-527: New feature documentation looks good, with a few minor suggestions.The new section 6.4 introduces the feature for limiting the number of configuration items in a single namespace. The content is clear and informative. However, I have a few suggestions for improvement:
Consider adding a brief explanation of why this feature might be useful (e.g., to prevent performance issues or to encourage better organization of configurations).
The image links are using relative paths, which might not work depending on how the documentation is hosted. Consider using absolute URLs to ensure the images are always accessible.
It might be helpful to mention what happens when a user tries to add a configuration item that would exceed the limit.
Here's a suggested improvement for the opening paragraph:
-Starting from version 2.4.0, apollo-portal provides the function of limiting the number of configuration items in a single namespace. This function is disabled by default and needs to be enabled by configuring the system `item.num.limit.enabled`. At the same time, the system parameter `item.num.limit` is provided to dynamically configure the upper limit of the number of items in a single Namespace. +Starting from version 2.4.0, apollo-portal provides a feature to limit the number of configuration items in a single namespace. This feature helps prevent performance issues and encourages better organization of configurations. It is disabled by default and can be enabled by setting the system parameter `item.num.limit.enabled` to true. Additionally, the system parameter `item.num.limit` allows you to dynamically set the maximum number of items allowed in a single Namespace.This revision provides more context about the feature's purpose and slightly improves the wording for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
doc/images/item-num-limit-enabled.pngis excluded by!**/*.pngdoc/images/item-num-limit.pngis excluded by!**/*.png
📒 Files selected for processing (10)
- CHANGES.md (1 hunks)
- apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java (3 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (2 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/repository/ItemRepository.java (1 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemService.java (1 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemSetService.java (5 hunks)
- apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemSetServiceTest.java (1 hunks)
- apollo-biz/src/test/resources/sql/itemset-test.sql (1 hunks)
- docs/en/portal/apollo-user-guide.md (1 hunks)
- docs/zh/portal/apollo-user-guide.md (2 hunks)
🔇 Additional comments (13)
apollo-biz/src/test/resources/sql/itemset-test.sql (2)
1-16: LGTM: Copyright and license information is correct and up-to-date.The copyright notice and Apache License 2.0 information are properly included and formatted.
1-25: Overall assessment: The test data initialization file is well-structured and appropriate.This new SQL file for test data initialization is well-crafted and serves its purpose effectively. It includes proper copyright and license information, and correctly inserts test data for both the Namespace and Item tables. The data provided seems appropriate for testing various scenarios, including different item types.
Minor suggestions for improving readability and consistency have been made in previous comments, but these are not critical issues. The file is approved for use in its current form.
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemSetServiceTest.java (2)
1-30: LGTM: Imports and class structure are well-organized.The imports, class declaration, and dependency injections are appropriately set up for an integration test. The use of
@MockforBizConfigand@Autowiredfor the services is correct.
119-124: LGTM: Helper method is well-implemented.The
buildNormalItemmethod is concise and correctly constructs anItemDTOobject with all the necessary fields.apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemSetService.java (4)
19-19: LGTM: Import statement for BizConfig added.The new import for
BizConfigis correctly placed and consistent with the new dependency added to the class.
43-43: LGTM: BizConfig dependency added correctly.The new
bizConfigfield is properly declared and initialized in the constructor. The changes are consistent with the existing code style and conventions.Also applies to: 49-50, 55-55
102-102: LGTM: Improved code formatting.The added indentation improves code readability and is consistent with the surrounding code style.
Line range hint
1-160: Overall assessment: Well-implemented feature with minor suggestions.The implementation of the item number limit feature is consistent with the PR objectives and well-integrated into the existing code. The changes are logically sound and follow good coding practices. Consider the minor suggestions for improvement, but otherwise, this implementation is ready for integration.
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (2)
40-40: LGTM: New constant for default maximum item number.The addition of
DEFAULT_MAX_ITEM_NUMis appropriate and consistent with the existing code style. Its value of 1000 seems reasonable as a default maximum number of items.
40-40: Summary: New item number limit feature well-integrated.The changes introduce a new feature for limiting the number of items, which aligns well with the PR objectives. The implementation is consistent with the existing code style and patterns in the
BizConfigclass. The new methodsisItemNumLimitEnabled()anditemNumLimit()provide a clear and flexible way to manage this new feature.To ensure these changes are properly utilized, please run the following script to check for usage of these new methods in the codebase:
This will help verify that the new methods are being used appropriately in the codebase and that the new configuration properties are referenced where needed.
Also applies to: 105-112
✅ Verification successful
Verification Successful: New item number limit feature is properly utilized.
The shell script results confirm that the new methods
isItemNumLimitEnabled()anditemNumLimit()are consistently used within the service and controller layers. Additionally, the new configuration propertiesitem.num.limit.enabledanditem.num.limitare appropriately referenced in theBizConfigclass and relevant controllers. This indicates that the new item number limit feature is well-integrated and actively utilized across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of new item number limit methods echo "Checking for usage of isItemNumLimitEnabled():" rg --type java "isItemNumLimitEnabled\(\)" echo "Checking for usage of itemNumLimit():" rg --type java "itemNumLimit\(\)" echo "Checking for references to the new configuration properties:" rg --type java "item\.num\.limit\.enabled" rg --type java "item\.num\.limit"Length of output: 3088
docs/zh/portal/apollo-user-guide.md (1)
485-501: Overall, the new section enhances the user guide effectively.The addition of section 6.4 about limiting the number of configuration items in a single namespace is a valuable update to the Apollo user guide. It clearly explains a new feature introduced in version 2.4.0, providing users with information on its purpose, default state, and configuration process. The use of images to illustrate the configuration steps is particularly helpful.
This update maintains the document's existing structure and style, ensuring consistency with the rest of the user guide. It provides administrators with the necessary information to understand and implement this new feature, which can help in managing large-scale configurations more effectively.
apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java (2)
20-20: Import ofBizConfigis appropriateThe addition of the import statement for
BizConfigis necessary for accessing configuration settings related to the item number limit.
62-69: Injection ofBizConfigvia constructor aligns with best practicesInjecting
BizConfigthrough the constructor ensures that configuration properties are available within the controller and adheres to dependency injection principles.
...service/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java
Outdated
Show resolved
Hide resolved
| if (bizConfig.isItemNumLimitEnabled()) { | ||
| int itemCount = itemService.findNonEmptyItemCount(entity.getNamespaceId()); | ||
| if (itemCount >= bizConfig.itemNumLimit()) { | ||
| throw new BadRequestException("current namespace item count=[" + itemCount + "], single namespace max allow count=[ " + bizConfig.itemNumLimit() + "]"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Encapsulate item limit logic within ItemService
To maintain separation of concerns, consider moving the item count limit check into the ItemService. This ensures that all item creation operations adhere to the limit, promoting consistency across the application.
...service/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java
Outdated
Show resolved
Hide resolved
# Conflicts: # CHANGES.md
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docs/zh/portal/apollo-user-guide.md (1)
485-498: New section on configuration item limit looks good, with a minor suggestion.The new section 6.4 about limiting the number of configuration items in a single namespace is a valuable addition to the user guide. It clearly explains the feature, its availability from version 2.4.0, and how to enable and configure it. The information is presented in a consistent manner with the rest of the document.
A few observations and suggestions:
- The content is well-structured and easy to understand.
- The instructions for enabling and configuring the feature are clear and concise.
- The use of images to illustrate the configuration process is helpful.
One minor suggestion:
Consider adding a brief explanation of why this feature might be useful or important. For example, you could add a sentence like:
从2.4.0版本开始，apollo-portal提供了限制单个命名空间下的配置项数量的功能，此功能默认关闭，需要配置系统 `item.num.limit.enabled` 开启，同时提供了系统参数`item.num.limit`来动态配置单个Namespace下的item数量上限值 + 这个功能可以帮助管理员控制配置的复杂性，防止单个命名空间变得过于庞大和难以维护。This addition would provide context for users about the purpose and benefits of this new feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
doc/images/item-num-limit-enabled.pngis excluded by!**/*.pngdoc/images/item-num-limit.pngis excluded by!**/*.png
📒 Files selected for processing (10)
- CHANGES.md (1 hunks)
- apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java (3 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (2 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/repository/ItemRepository.java (1 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemService.java (1 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemSetService.java (5 hunks)
- apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemSetServiceTest.java (1 hunks)
- apollo-biz/src/test/resources/sql/itemset-test.sql (1 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 (9)
- CHANGES.md
- apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/repository/ItemRepository.java
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemService.java
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemSetService.java
- apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemSetServiceTest.java
- apollo-biz/src/test/resources/sql/itemset-test.sql
- docs/en/portal/apollo-user-guide.md
🔇 Additional comments (1)
docs/zh/portal/apollo-user-guide.md (1)
485-498: Overall, the addition of section 6.4 enhances the user guide.The new section on limiting the number of configuration items in a single namespace is a valuable addition to the Apollo user guide. It's well-integrated with the existing content and provides clear instructions for users. No other significant changes were observed in the rest of the document.
...service/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java
Outdated
Show resolved
Hide resolved
...service/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java
Outdated
Show resolved
Hide resolved
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java
Show resolved
Hide resolved
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/repository/ItemRepository.java
Show resolved
Hide resolved
# Conflicts: # CHANGES.md # apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java # docs/en/portal/apollo-user-guide.md # docs/zh/portal/apollo-user-guide.md
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (16)
CHANGES.md (2)
14-14: Improve wording and add context
- Consider adopting the more concise wording suggested in the previous review:
* [Feature add limit for items count per namespace](https://github.com/apolloconfig/apollo/pull/5228)
- To provide more context, consider expanding the description:
* [Feature add limit for items count per namespace to improve stability during configuration updates](https://github.com/apolloconfig/apollo/pull/5228)This change would make the entry more informative and align it better with the PR objectives.
Line range hint
6-6: Remove duplicate entryThere appears to be a duplicate entry for this feature. Line 6 reads:
* [Feature: Add limit and whitelist for namespace count per appid+cluster](https://github.com/apolloconfig/apollo/pull/5228)This seems to refer to the same feature as line 14. Consider removing one of these entries to avoid redundancy. If they refer to different aspects of the same feature, consider combining them into a single, more comprehensive entry.
Also applies to: 14-14
apollo-biz/src/test/resources/sql/itemset-test.sql (2)
17-17: LGTM: Namespace insert statement is correct. Consider formatting for readability.The INSERT statement for the Namespace table is correctly structured and provides appropriate test data. However, for improved readability, consider formatting the statement across multiple lines.
Here's a suggested format:
INSERT INTO "Namespace" ( `Id`, `AppId`, `ClusterName`, `NamespaceName`, `IsDeleted`, `DataChange_CreatedBy`, `DataChange_LastModifiedBy` ) VALUES ( 1, 'testApp', 'default', 'application', 0, 'apollo', 'apollo' );
19-25: LGTM: Item insert statement is well-structured. Consider type consistency.The INSERT statement for the Item table is correctly formatted for multiple row insertion and provides a good variety of test data. The use of sequential keys and line numbers is appropriate for testing.
For consistency, consider using the same type (0) for all items unless testing different types is specifically required. If different types are intentional, it might be helpful to add a comment explaining the significance of type 2 for 'k2'.
Example comment:
-- Type 2 for 'k2' represents a special case in the systemapollo-biz/src/main/java/com/ctrip/framework/apollo/biz/repository/ItemRepository.java (1)
67-68: Suggest renaming the method for clarityThe current method name
countByNamespaceIdAndFilterKeyEmptydoesn't accurately reflect its functionality. It suggests filtering empty keys, but it actually counts non-empty keys. Consider renaming it to better describe its purpose.Suggested name:
int countByNamespaceIdAndKeyNotEmpty(@Param("namespaceId") long namespaceId);This name more accurately describes the method's behavior of counting items with non-empty keys for a given namespaceId.
Regarding the implementation:
- The query correctly counts items with non-empty keys for the given namespaceId.
- This approach successfully addresses the limitation with IsEmpty/IsNotEmpty on non-collection properties mentioned in previous comments.
- The implementation is efficient and follows Spring Data JPA conventions.
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemSetServiceTest.java (4)
48-72: LGTM! Consider adding a comment explaining the initial item count.The test method effectively verifies the behavior when item number limits are disabled. It correctly sets up the mock configuration and asserts the expected outcome.
Consider adding a comment explaining that there are initially 5 items in the database (as set up by the SQL script), which is why the final count is 7 after adding 2 more items. This would improve the test's readability and make it easier for other developers to understand the scenario.
74-103: LGTM! Consider adding an assertion for the exception message.The test method effectively verifies the behavior when item number limits are enabled. It correctly sets up the mock configuration, attempts to exceed the limit, and asserts both the exception type and the final item count.
Consider adding an assertion for the exception message to ensure it provides the expected information about exceeding the item limit. This can be done by capturing the exception message and using
Assert.assertTrue(e.getMessage().contains("expected error message")).
105-132: LGTM! Consider improving the test method name and adding a descriptive comment.The test method effectively verifies the behavior when item number limits are enabled and operations are within the limit. It correctly sets up the mock configuration and asserts the expected outcome.
- Consider renaming the test method to better describe the specific scenario it's testing, e.g.,
testUpdateSetWithinItemNumLimit.- Add a comment at the beginning of the method explaining the scenario: updating one item, deleting one item, and adding one new item, which should keep the total count at 5 and not exceed the limit.
1-142: Overall, excellent test coverage and structure. Consider adding a test for edge cases.The
ItemSetServiceTestclass provides comprehensive coverage for the new item number limit feature. The tests are well-structured, use appropriate mocking and database setup techniques, and cover both enabled and disabled scenarios for the feature.To further improve the test suite, consider adding a test case for an edge case scenario, such as:
- Attempting to update the set when the current item count is exactly at the limit.
- Testing with a very large item number limit to ensure there are no integer overflow issues.
docs/zh/portal/apollo-user-guide.md (1)
508-515: Setup instructions are clear, but consider adding more details.The instructions for setting up the feature are straightforward. However, it might be helpful to add:
- The purpose or benefits of enabling this feature.
- Any potential impacts or considerations when setting the limit.
docs/en/portal/apollo-user-guide.md (5)
535-547: Consider adding more context to the new configuration option.The introduction of the namespace limit feature is clear, but it would be helpful to provide more context on why this limit might be necessary (e.g., performance considerations, resource management). Additionally, consider explaining the potential impact of enabling this feature on existing projects.
538-547: Improve the formatting and clarity of the setting instructions.The setting instructions are clear, but the formatting could be improved for better readability. Consider using a numbered list for the steps and adding more whitespace between them. Also, it might be helpful to mention what values are acceptable for the
namespace.num.limit.enabledparameter (e.g., "true" to enable, "false" to disable).
535-550: Improve formatting and provide more context for the new feature.
The section title is in Chinese while the rest of the document is in English. Consider translating it to "6.4 Limit on the number of configuration items under a single namespace" for consistency.
The formatting of the images is inconsistent. There are commented-out image links and then actual image links. Standardize the image formatting throughout the document.
Provide more context on why limiting the number of configuration items might be necessary (e.g., performance reasons, maintainability).
Include information on the default value for
item.num.limitif there is one.Consider adding a note on how this limit affects existing namespaces that may already exceed the configured limit.
Line range hint
550-589: Enhance security recommendations with more emphasis and detailed guidance.The security best practices provided are valuable, but consider the following enhancements:
Add a strong opening statement emphasizing the critical importance of security in configuration management systems.
For each recommendation, provide a brief explanation of the potential risks if the recommendation is not followed.
In the authentication section, consider mentioning the importance of regular password rotations and strong password policies if using Apollo's built-in authentication.
In the authorization section, emphasize the principle of least privilege and suggest regular audits of user permissions.
For system access, consider adding a recommendation for network segmentation to further isolate Apollo components.
Add a subsection on logging and monitoring, emphasizing the importance of tracking configuration changes and detecting unusual access patterns.
Consider adding a recommendation for regular security assessments or penetration testing of the Apollo deployment.
These additions would provide a more comprehensive security guide for Apollo administrators.
Line range hint
1-589: Overall document review: Enhance consistency and clarityThis comprehensive guide provides valuable information on using Apollo, from basic usage to advanced features and security best practices. To further improve the document, consider the following suggestions:
Consistency in language: Ensure all section titles and content are in English for a fully English version of the document.
Formatting: Standardize the formatting throughout the document, especially for image links and code blocks.
Version information: At the beginning of the document, clearly state the Apollo version this guide is based on, and consider adding notes for version-specific features.
Navigation: Add a table of contents at the beginning of the document for easier navigation, especially given its length and the variety of topics covered.
Examples: Where possible, provide more real-world examples or use cases to illustrate the practical application of features, especially for advanced topics like grayscale releases.
Troubleshooting: Consider adding a troubleshooting section that addresses common issues users might encounter and how to resolve them.
Related documentation: At the end of each major section, consider adding links to related documentation or API references for users who need more detailed information.
These enhancements would improve the overall user experience and make the document even more valuable as a comprehensive guide to Apollo.
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemSetService.java (1)
78-78: Standardize formatting in exception messageThere's an inconsistent space within the brackets in the exception message. For clarity and consistency, adjust the spacing as follows:
-throw new BadRequestException("current namespace item count=[" + itemCount + "], single namespace max allow count=[ " + bizConfig.itemNumLimit() + "]"); +throw new BadRequestException("current namespace item count=[" + itemCount + "], single namespace max allow count=[" + bizConfig.itemNumLimit() + "]");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
doc/images/item-num-limit-enabled.pngis excluded by!**/*.pngdoc/images/item-num-limit.pngis excluded by!**/*.png
📒 Files selected for processing (10)
- CHANGES.md (1 hunks)
- apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java (3 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (2 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/repository/ItemRepository.java (1 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemService.java (1 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemSetService.java (5 hunks)
- apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemSetServiceTest.java (1 hunks)
- apollo-biz/src/test/resources/sql/itemset-test.sql (1 hunks)
- docs/en/portal/apollo-user-guide.md (1 hunks)
- docs/zh/portal/apollo-user-guide.md (1 hunks)
🧰 Additional context used
📓 Learnings (1)
apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java (1)
Learnt from: nobodyiam PR: apolloconfig/apollo#5227 File: apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java:83-90 Timestamp: 2024-10-01T07:42:47.878Z Learning: In cases where the item count limit is a soft limit, non-atomic checks in the `create` method are acceptable.
🔇 Additional comments (12)
apollo-biz/src/test/resources/sql/itemset-test.sql (1)
1-16: LGTM: Copyright and license header is correct and up-to-date.The copyright notice and Apache License 2.0 header are properly included and formatted. The copyright year (2024) is current, which is appropriate for a newly added file.
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemSetServiceTest.java (1)
135-140: LGTM! The helper method is well-implemented.The
buildNormalItemmethod is a concise and effective helper for creatingItemDTOobjects in the test cases. It's used consistently throughout the test class, which promotes code reuse and readability.apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (4)
44-45: LGTM: New constant for default max item number.The addition of
DEFAULT_MAX_ITEM_NUMwith a value of 1000 is appropriate. It follows the existing naming convention and provides a reasonable default limit for the maximum number of items.
122-124: LGTM: New method to check if item number limit is enabled.The
isItemNumLimitEnabled()method is well-implemented. It follows the existing pattern for feature toggles in the class and defaults to false, ensuring backward compatibility.
126-129: Consider documenting the minimum value choice.The
itemNumLimit()method is well-implemented and consistent with other similar methods. However, the choice of 5 as the minimum value might benefit from some explanation.Consider adding a comment explaining the rationale behind the minimum value of 5, as discussed in the previous review. This will help future maintainers understand the decision.
public int itemNumLimit() { int limit = getIntProperty("item.num.limit", DEFAULT_MAX_ITEM_NUM); + // Minimum value of 5 is chosen based on the typical minimum values for item key and value length limits return checkInt(limit, 5, Integer.MAX_VALUE, DEFAULT_MAX_ITEM_NUM); }
44-45: Overall changes align well with PR objectives.The additions to the
BizConfigclass successfully implement the item number limit feature as described in the PR objectives. The new constant and methods are consistent with existing patterns in the class and provide the necessary functionality to enable/disable the feature and set the limit.The implementation allows for easy configuration and toggling of the feature, which aligns well with the goal of improving system stability during configuration updates.
Also applies to: 122-129
docs/zh/portal/apollo-user-guide.md (1)
505-507: New feature introduction looks good.The introduction of the new feature for limiting the number of configuration items in a single namespace is clear and concise. It provides the version information and explains the default state of the feature.
apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java (5)
20-20: Import ofBizConfigis correctly addedThe import statement for
BizConfigis properly included to support item limit configuration.
62-62: Proper declaration ofBizConfigas a final fieldDeclaring
bizConfigas afinalfield ensures that its reference remains constant, which is good practice for dependency injection.
64-69: Constructor updated to includeBizConfigThe updated constructor now includes
BizConfigand correctly assigns it to the class field.
84-89: Consider moving item limit logic toItemServicefor better encapsulationTo maintain separation of concerns and promote code reuse, consider moving the item count limit check into the
ItemService. This ensures that any operation creating items goes through the same validation logic.
87-87: Improve the clarity of the exception messageEnhancing the exception message will provide users with a clearer understanding of the issue.
| public int findNonEmptyItemCount(long namespaceId) { | ||
| return itemRepository.countByNamespaceIdAndFilterKeyEmpty(namespaceId); | ||
| } |
There was a problem hiding this comment.
Approve new method, but item.num.limit implementation is incomplete.
The new findNonEmptyItemCount method is correctly implemented to count non-empty items in a namespace. However, the item.num.limit feature mentioned in the PR objectives is not fully implemented yet.
To complete the item.num.limit feature:
- Add a method in
BizConfigto retrieve theitem.num.limitvalue. - Implement logic in methods like
saveandupdateto check against this limit before adding or modifying items. - Update relevant controllers to handle cases where the item limit is reached.
Example implementation in the save method:
@Transactional
public Item save(Item entity) {
// Existing checks...
long namespaceId = entity.getNamespaceId();
int currentItemCount = findNonEmptyItemCount(namespaceId);
int itemNumLimit = bizConfig.getItemNumLimit(namespaceId);
if (currentItemCount >= itemNumLimit) {
throw new BadRequestException("Item limit reached for this namespace. Limit: " + itemNumLimit);
}
// Existing save logic...
}Similar checks should be added to other relevant methods.
There was a problem hiding this comment.
Batch modification will result in a large number of queries to the DB
There was a problem hiding this comment.
@youngzil, I understand your concern about the potential performance impact when batch modifications result in numerous database queries. Thank you for bringing this to my attention.
✏️ Learnings added
Learnt from: youngzil
PR: apolloconfig/apollo#5227
File: apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemService.java:138-140
Timestamp: 2024-10-07T09:49:21.175Z
Learning: When implementing item count limits, batch modifications can result in a large number of database queries, leading to performance issues.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
docs/zh/portal/apollo-user-guide.md
Outdated
| [//]: # ( ) | ||
|  | ||
| 3. 进入`管理员工具 - 系统参数 - ConfigDB 配置管理`页面新增或修改`item.num.limit`配置项来配置单个Namespace下的item数量上限值 | ||
|
|
||
| [//]: # ( ) | ||
|  |
There was a problem hiding this comment.
Inconsistent image references need attention.
There are inconsistencies in the image references:
- Line 512-513: Uses a CDN link (commented out) and a relative path.
- Line 516-517: Same issue as above.
Please standardize the image references. If using relative paths, ensure they are correct and consistent throughout the document.
docs/zh/portal/apollo-user-guide.md
Outdated
| ## 6.5 单个命名空间下的配置项数量限制 | ||
| 从2.4.0版本开始，apollo-portal提供了限制单个命名空间下的配置项数量的功能，此功能默认关闭，需要配置系统 `item.num.limit.enabled` 开启，同时提供了系统参数`item.num.limit`来动态配置单个Namespace下的item数量上限值 | ||
|
|
||
| **设置方法：** | ||
| 1. 用超级管理员账号登录到Apollo配置中心的界面 | ||
| 2. 进入`管理员工具 - 系统参数 - ConfigDB 配置管理`页面新增或修改`item.num.limit.enabled`配置项为true/false 即可开启/关闭此功能 | ||
|
|
||
| [//]: # ( ) | ||
|  | ||
| 3. 进入`管理员工具 - 系统参数 - ConfigDB 配置管理`页面新增或修改`item.num.limit`配置项来配置单个Namespace下的item数量上限值 | ||
|
|
||
| [//]: # ( ) | ||
|  | ||
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reorganizing the document structure.
The new section 6.5 is added at the end of section 6. Consider:
- Reviewing the ordering of subsections in section 6 to ensure logical flow.
- Adding a brief introduction at the beginning of section 6 to overview all the "other function configurations".
- Updating the table of contents (if exists) to include this new section.
| if (bizConfig.isItemNumLimitEnabled()) { | ||
| int itemCount = itemService.findNonEmptyItemCount(namespace.getId()); | ||
| int createItemCount = (int) changeSet.getCreateItems().stream().filter(item -> !StringUtils.isEmpty(item.getKey())).count(); | ||
| int deleteItemCount = (int) changeSet.getDeleteItems().stream().filter(item -> !StringUtils.isEmpty(item.getKey())).count(); | ||
| itemCount = itemCount + createItemCount - deleteItemCount; | ||
| if (itemCount > bizConfig.itemNumLimit()) { | ||
| throw new BadRequestException("current namespace item count=[" + itemCount + "], single namespace max allow count=[ " + bizConfig.itemNumLimit() + "]"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Address potential concurrency issues in item count validation
The current logic for enforcing the item number limit may encounter concurrency issues. If multiple requests modify the namespace items simultaneously, the item count retrieved might be outdated by the time the validation occurs, potentially leading to inconsistent states or exceeding the limit unintentionally.
To ensure consistency and prevent race conditions, consider implementing synchronization mechanisms such as database-level locks, optimistic locking, or wrapping this operation within a transaction with an appropriate isolation level.
| int createItemCount = (int) changeSet.getCreateItems().stream().filter(item -> !StringUtils.isEmpty(item.getKey())).count(); | ||
| int deleteItemCount = (int) changeSet.getDeleteItems().stream().filter(item -> !StringUtils.isEmpty(item.getKey())).count(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify variable names for better readability
The variables createItemCount and deleteItemCount represent the number of items to be added and removed. For improved code readability, consider renaming them to newItemCount and removedItemCount, respectively.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
docs/en/portal/apollo-user-guide.md (1)
Line range hint
547-573: LGTM: Comprehensive security best practices added.The expanded security section provides excellent guidance for securing Apollo installations. It covers crucial aspects such as authentication, authorization, and system access control. The recommendations are well-explained and include references to specific features and configurations.
Consider adding a brief introduction to this section emphasizing the importance of security in configuration management systems. This could help reinforce the significance of following these best practices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- CHANGES.md (1 hunks)
- apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java (3 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemSetService.java (5 hunks)
- apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemSetServiceTest.java (1 hunks)
- docs/en/portal/apollo-user-guide.md (1 hunks)
- docs/zh/portal/apollo-user-guide.md (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGES.md
- apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemSetServiceTest.java
- docs/zh/portal/apollo-user-guide.md
🧰 Additional context used
📓 Learnings (2)
apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java (1)
Learnt from: nobodyiam PR: apolloconfig/apollo#5227 File: apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java:83-90 Timestamp: 2024-10-01T07:42:47.878Z Learning: In cases where the item count limit is a soft limit, non-atomic checks in the `create` method are acceptable.apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemSetService.java (1)
Learnt from: youngzil PR: apolloconfig/apollo#5227 File: apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemService.java:138-140 Timestamp: 2024-10-07T09:49:21.339Z Learning: When implementing item count limits, batch modifications can result in a large number of database queries, leading to performance issues.
🔇 Additional comments (11)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemSetService.java (6)
19-19: LGTM: Import statement added for new dependency.The import for
BizConfigis correctly added, which is necessary for the new functionality introduced in this class.
43-43: LGTM: New dependency properly added and initialized.The
BizConfigdependency is correctly added as a field and properly initialized in the constructor. This change is consistent with the new functionality being introduced.Also applies to: 49-50, 55-55
72-80: LGTM: Item count limit validation implemented.The new logic for enforcing item count limits is well-implemented and aligns with the PR objectives. It correctly considers existing, new, and deleted items in the calculation.
However, as noted in a previous review, there's a potential concurrency issue:
The current logic for enforcing the item number limit may encounter concurrency issues. If multiple requests modify the namespace items simultaneously, the item count retrieved might be outdated by the time the validation occurs, potentially leading to inconsistent states or exceeding the limit unintentionally.
To ensure consistency and prevent race conditions, consider implementing synchronization mechanisms such as database-level locks, optimistic locking, or wrapping this operation within a transaction with an appropriate isolation level.
74-75: Consider improving variable names for better readability.The variables
createItemCountanddeleteItemCountrepresent the number of items to be added and removed. For improved code readability, consider renaming them tonewItemCountandremovedItemCount, respectively.
102-102: LGTM: Minor formatting adjustment.The
createCommitmethod call has been moved to a new line. This is a minor formatting change that doesn't affect functionality.
Line range hint
1-164: Summary of review for ItemSetService.javaThe changes successfully implement the item count limit functionality as described in the PR objectives. The new
BizConfigdependency is properly integrated, and theupdateSetmethod now includes logic to enforce item count limits.Key points:
- The implementation is generally sound and meets the PR objectives.
- A potential concurrency issue has been identified and should be addressed.
- Some variable names could be improved for better readability.
Overall, the changes are approved with suggestions for improvements. Please consider addressing the concurrency issue and variable naming to enhance the code quality and reliability.
docs/en/portal/apollo-user-guide.md (4)
534-545: LGTM: New namespace limit feature well-documented.The new section on limiting the number of namespaces is a valuable addition to the user guide. It clearly explains the feature, its purpose, and how to configure it. The inclusion of a whitelist for special cases adds flexibility to the feature.
534-545: LGTM: Configuration item limit feature well-explained.The new section on limiting the number of configuration items in a single namespace is a useful addition to the user guide. It provides clear instructions on how to enable and configure this feature, which can help prevent performance issues related to excessive configuration items.
Line range hint
1-573: LGTM: Well-structured and formatted document.The overall structure and formatting of the user guide are excellent. The use of clear headings, subheadings, and consistent markdown formatting enhances readability and makes it easy for users to navigate the document.
Line range hint
1-573: LGTM: Excellent updates to the Apollo user guide.The additions to the Apollo user guide are comprehensive and well-executed. The new sections on namespace and configuration item limits provide important information about recent features. The expanded security best practices section offers valuable guidance for securing Apollo installations. These updates significantly enhance the usefulness of the guide for Apollo users.
apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java (1)
20-20: EnsureBizConfigis properly injected to avoid runtime exceptionsThe addition of
BizConfigas a dependency inItemControllerrequires that it is correctly managed by Spring's dependency injection. IfBizConfigis not configured as a bean or if the application context is not updated accordingly, this may lead to runtime exceptions due to unsatisfied dependencies.Please verify that
BizConfigis declared as a Spring bean and that all configurations are updated to reflect the constructor change.Run the following script to check for direct instantiations of
ItemControllerthat might be affected:Also applies to: 62-62, 64-69
|
|
||
| if (bizConfig.isItemNumLimitEnabled()) { | ||
| int itemCount = itemService.findNonEmptyItemCount(entity.getNamespaceId()); | ||
| if (itemCount >= bizConfig.itemNumLimit()) { | ||
| throw new BadRequestException("The maximum number of items (" + bizConfig.itemNumLimit() + ") for this namespace has been reached. Current item count is " + itemCount + "."); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider enforcing item count limit in all item creation methods
The item count limit is currently enforced only in the create method. Other methods that create items, such as createComment, do not include this check. This could allow the item count limit to be bypassed through other endpoints, leading to inconsistent enforcement.
To ensure consistent application of the item count limit across the application, consider adding the item count validation in other item creation methods like createComment.
# Conflicts: # CHANGES.md
|
Would you please fix the unit test error? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemSetServiceTest.java (2)
75-106: LGTM: Comprehensive test forupdateSetwith item number limit.This test effectively verifies the behavior of
updateSetwhen item number limits are enabled and exceeded. It properly sets up the test environment, attempts to exceed the limit, and asserts the expected exception and outcome.A minor suggestion for improvement:
Consider using JUnit's
assertThrowsmethod instead of the try-catch block for a more concise and idiomatic way of testing for exceptions. For example:BadRequestException exception = assertThrows(BadRequestException.class, () -> { itemSetService.updateSet(namespace, changeSets); });This approach eliminates the need for the
Assert.fail()call and makes the test more readable.
108-137: LGTM: Good test forupdateSetwith item number limit (non-exceeding case).This test effectively verifies the behavior of
updateSetwhen item number limits are enabled but not exceeded. It properly sets up the test environment, executes the operation, and asserts the expected outcome.Suggestions for improvement:
Instead of using a try-catch block to assert that no exception is thrown, consider removing it entirely. JUnit will automatically fail the test if an unexpected exception is thrown.
To make the test's purpose clearer, you could add an assertion to verify that the operation succeeded. For example:
ItemChangeSets result = itemSetService.updateSet(namespace, changeSets); assertNotNull(result); // Add more specific assertions about the result if needed int size = itemService.findNonEmptyItemCount(namespace.getId()); assertEquals(5, size);These changes would make the test more concise and its intentions clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemSetServiceTest.java (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemSetServiceTest.java (3)
47-73: LGTM: Well-structured test forupdateSetwithout item number limit.This test effectively verifies the behavior of
updateSetwhen item number limits are disabled. It properly sets up the test environment, executes the operation, and asserts the expected outcome. The use of SQL scripts for setup and cleanup is a good practice for maintaining test isolation.
140-145: LGTM: Well-structured helper method.The
buildNormalItemmethod is a good example of a helper method that simplifies the creation of test data. It reduces code duplication and improves readability in the test methods.
1-147: Overall: Well-structured and comprehensive test suite forItemSetService.This test file demonstrates good testing practices:
- Comprehensive coverage of different scenarios (with and without item number limits)
- Effective use of Mockito for controlling
BizConfigbehavior- Use of SQL scripts for test setup and cleanup, ensuring test isolation
Suggestions for overall improvement:
- Consider adding a test for edge cases, such as when the item count is exactly at the limit.
- Add comments explaining the purpose of each test method to improve readability.
- Consider using parameterized tests for scenarios that differ only in input data.
- Ensure consistent naming conventions for test methods (e.g., "should..." or "testThat...").
To ensure comprehensive test coverage, let's verify if there are any untested public methods in the
ItemSetServiceclass:This will help identify any public methods in
ItemSetServicethat might not be covered by the current tests.✅ Verification successful
Verification Complete: All public methods in
ItemSetServiceare adequately tested.The
ItemSetServiceTestclass includes comprehensive tests for theupdateSetmethod under various scenarios, ensuring robust coverage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find public methods in ItemSetService.java echo "Public methods in ItemSetService:" rg "public\s+\w+\s+\w+\s*\([^)]*\)" apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemSetService.java # Find test methods in ItemSetServiceTest.java echo "Test methods in ItemSetServiceTest:" rg "public\s+void\s+test\w+\s*\([^)]*\)" apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemSetServiceTest.javaLength of output: 642
@nobodyiam I have fixed this unit test, please review it again |
What's the purpose of this PR
Added a new parameter
item.num.limit，the purpose is to limit the number of configuration items in the same namespaceIn actual use, when modifying configurations in batches, if the number of configurations exceeds a certain value, it is easy to trigger a timeout or page crash.
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
Bug Fixes
Documentation