Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add determine appid+cluster namespace num limit logic #5228

Merged
merged 8 commits into from
Oct 6, 2024

Conversation

youngzil
Copy link
Contributor

@youngzil youngzil commented Sep 10, 2024

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+cluster
Added a new parameter namespace.num.limit.white ,Namespace num limit whitelist, The purpose is to allow some special appids to skip the namespace num limit

Because 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:

  • Read the Contributing Guide before making this pull request.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit tests to verify the code.
  • Run mvn clean test to make sure this pull request doesn't break anything.
  • Update the CHANGES log.

Summary by CodeRabbit

  • New Features

    • Introduced configurable limits on the number of namespaces, with options to enable or disable the feature.
    • Added functionality to count namespaces associated with specific application IDs and cluster names.
    • Implemented validation in the namespace saving process to enforce limits based on application IDs and whitelist settings.
  • Documentation

    • Added detailed instructions in the user guide for configuring namespace limits and whitelisting options.

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Sep 10, 2024
Copy link
Contributor

coderabbitai bot commented Sep 10, 2024

Walkthrough

The changes introduce a new feature for managing namespace limits within the Apollo configuration system. This includes the addition of methods in BizConfig for retrieving namespace limits and a whitelist, a new method in NamespaceRepository to count namespaces by application ID and cluster name, and modifications to NamespaceService to enforce these limits. The documentation has also been updated to provide guidance on these new configurations.

Changes

Files Change Summary
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java Added methods for managing namespace limits and defined constants for default values.
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/repository/NamespaceRepository.java Introduced a new method to count namespaces by application ID and cluster name.
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/NamespaceService.java Added a dependency on BizConfig and modified the save method to enforce namespace limits based on application IDs.
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java Added new test cases to validate namespace limit functionality.
docs/en/portal/apollo-user-guide.md Updated documentation to include new system parameters for namespace limits and whitelist configurations.
docs/zh/portal/apollo-user-guide.md Updated Chinese documentation to reflect new features and configuration instructions.

Possibly related PRs

Suggested labels

lgtm

Poem

🐇 In the garden of code, changes bloom bright,
New limits on namespaces, a wonderful sight!
With BizConfig guiding, we count and we check,
Ensuring our namespaces don’t cause a wreck.
So hop with delight, as we code and we play,
For better configurations are here to stay! 🌼


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@nobodyiam
Copy link
Member

  1. I'm unsure if this feature is common.
  2. Enabling it by default could be problematic for users who prefer not to use it.

@youngzil
Copy link
Contributor Author

  1. I'm unsure if this feature is common.
  2. Enabling it by default could be problematic for users who prefer not to use it.

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

@youngzil
Copy link
Contributor Author

#5227:
Function: Limit the number of items under the same namespace
Function reason:
Apollo release will generate a large field record in the release table. Configurations is a longtext type field that stores all configurations under the same namespace.
If there are too many items under the same namespace, it may cause two problems:

  1. When modifying configurations in batches, if the newly added or modified configurations are in the first few lines, the number of all configuration lines will change, and there will be a large-scale insert or update item table operation (there are abuse scenarios in actual business, with thousands of configurations under one namespace)
  2. Each small amount of configuration modification will cause the release table to generate a large field record. Too many records will affect the query performance of the release table.

@youngzil
Copy link
Contributor Author

#5228:
Function: Limit the number of namespaces under the same appid
Reasons:

  1. In actual use, some appids have too many namespaces, which may even cause the UI page to time out and fail to load
  2. Some special services, such as middleware services such as apigateway, MQ, Redis, DB, etc., need to create many namespaces to uniformly manage the configuration of different appids, so this function has a whitelist restriction function

@youngzil
Copy link
Contributor Author

  1. I'm unsure if this feature is common.
  2. Enabling it by default could be problematic for users who prefer not to use it.

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

@nobodyiam
Copy link
Member

Thank you for the background information. Here are some suggestions:

  1. Ensure features are disabled by default.
  2. Update the CHANGES.md and documentation to guide users on enabling and configuring the features.
  3. Address code style issues; refer to CONTRIBUTING for guidance.
  4. Consider adding appropriate unit tests.

1. Added switch control function on and off
2. Add unit tests and usage documentation
3. Update the CHANGES.md
4. Function and code style optimization
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Sep 29, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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_NUM with 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 of false ensuring 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() and namespaceNumLimitWhite() methods correctly implement the namespace limit and whitelist functionality as described in the PR objectives.

Points of note:

  1. namespaceNumLimit() uses checkInt() to ensure the limit is within a valid range.
  2. namespaceNumLimitWhite() correctly uses Sets.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:

  1. The title mentions "配置项数量限制" (configuration item number limit), but the content discusses "命名空间数量上限" (namespace number limit). Consider aligning the title with the content for consistency.

  2. The explanation of the namespace.num.limit.enabled parameter could be more explicit about its default value.

  3. 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.

  4. There's an extra newline at the end of the section that could be removed for consistency with other sections.

Consider applying these changes:

  1. Update the section title:
-## 6.4 单个命名空间下的配置项数量限制
+## 6.4 单个应用集群下的命名空间数量限制
  1. Clarify the default value:
-2. 进入 `管理员工具 - 系统参数 - ConfigDB 配置管理` 页面新增或修改 `namespace.num.limit.enabled` 配置项为true/false 即可开启/关闭此功能,默认关闭
+2. 进入 `管理员工具 - 系统参数 - ConfigDB 配置管理` 页面新增或修改 `namespace.num.limit.enabled` 配置项。设置为true开启功能,设置为false关闭功能。此功能默认关闭(即默认值为false)。
  1. Update image links to use absolute URLs for consistency:
-![item-num-limit-enabled](../../../doc/images/namespace-num-limit-enabled.png)
+![item-num-limit-enabled](https://cdn.jsdelivr.net/gh/apolloconfig/apollo@master/doc/images/namespace-num-limit-enabled.png)

-![item-num-limit](../../../doc/images/namespace-num-limit.png)
+![item-num-limit](https://cdn.jsdelivr.net/gh/apolloconfig/apollo@master/doc/images/namespace-num-limit.png)

-![item-num-limit](../../../doc/images/namespace-num-limit-white.png)
+![item-num-limit](https://cdn.jsdelivr.net/gh/apolloconfig/apollo@master/doc/images/namespace-num-limit-white.png)
  1. 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 validation

Currently, the test only checks that a ServiceException is 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 countByAppIdAndClusterName method could impact performance with large datasets.

Suggestion:

Ensure there are appropriate indexes on the appId and clusterName columns to optimize the count query.


360-360: Enhance error message clarity.

The ServiceException message 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

📥 Commits

Files that changed from the base of the PR and between 4d29c26 and 2195d79.

⛔ Files ignored due to path filters (3)
  • doc/images/namespace-num-limit-enabled.png is excluded by !**/*.png
  • doc/images/namespace-num-limit-white.png is excluded by !**/*.png
  • doc/images/namespace-num-limit.png is 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.java successfully 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:

  1. The feature is disabled by default, ensuring backward compatibility.
  2. The limit and whitelist are configurable, providing flexibility for different use cases.
  3. The implementation uses appropriate data structures and validation.

Suggestions for improvement:

  1. Add Javadoc comments to the new methods to improve API documentation.
  2. 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: Test testNamespaceNumLimit() correctly validates namespace limit enforcement

The test method properly configures the bizConfig to enable the namespace limit and sets the limit to 2. It verifies that saving a namespace beyond the limit throws a ServiceException, and it confirms the total count of namespaces remains at the limit.


184-211: Test testNamespaceNumLimitFalse() accurately checks behavior when limit is disabled

The 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: Test testNamespaceNumLimitWhite() properly verifies whitelist functionality

The test correctly sets up the bizConfig with the namespace limit enabled and adds the testApp to 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 @Mock and @Autowired annotations

Mixing @Mock and @Autowired may lead to unintended behavior. Since bizConfig is a mock, ensure that it is properly injected into namespaceService. Also, consider using @InjectMocks to create the namespaceService instance with the mocked bizConfig.

Run the following script to check if bizConfig is correctly injected:

apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/NamespaceService.java (4)

19-19: Added import for BizConfig.

The import statement for BizConfig is correctly added to support the new namespace limit feature.


72-72: Declared bizConfig member variable.

The bizConfig variable is properly declared as private final, ensuring immutability after construction.


86-87: Updated constructor to include BizConfig.

The constructor now includes BizConfig as a parameter, which is necessary for injecting configuration settings.


100-100: Assigned bizConfig in constructor.

The bizConfig instance is correctly assigned to the class member variable, ensuring it is available throughout the class.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 assertThrows instead 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, and testNamespaceNumLimitWhite) 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:

  1. Consider adding a test case for an app that is not whitelisted when the limit is enforced, to contrast with the whitelisted case.

  2. Ensure consistent use of mocking across all tests, avoiding the use of ReflectionTestUtils as suggested in previous comments.

  3. Consider parameterizing some tests to cover edge cases, such as testing with different limit values.

  4. 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

📥 Commits

Files that changed from the base of the PR and between 2195d79 and a946ad1.

📒 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 @MockBean for bizConfig is 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

@youngzil
Copy link
Contributor Author

youngzil commented Oct 2, 2024

@nobodyiam I can't reply to messages in #5227
image

Reply to #5227 (comment)
The minimum value of 5 is set based on the values ​​of item.key.length.limit and item.value.length.limit. Considering that actual use will not set a value smaller than 5

Reply to #5227 (comment)
countByNamespaceIdAndKeyIsNotEmpty or countByNamespaceIdAndKeyIsNotNullAndKeyNotEmpty will result in the following exception:
IsEmpty / IsNotEmpty can only be used on collection properties!

CHANGES.md Outdated Show resolved Hide resolved
Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Oct 6, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 tests

The additions to NamespaceServiceIntegrationTest successfully implement tests for the new namespace limit functionality. The new tests cover various scenarios:

  1. Enforced namespace limit
  2. Disabled namespace limit
  3. 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 @MockBean for BizConfig allows 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

📥 Commits

Files that changed from the base of the PR and between a946ad1 and 2ccd935.

📒 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 tests

The 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 apps

The testNamespaceNumLimitWhite method effectively verifies the behavior for whitelisted apps when the namespace limit is enforced. The test setup, including the mocking of BizConfig and 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.

Comment on lines +151 to +180
@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);

}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

Comment on lines +182 to +204
@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);

}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

@nobodyiam
Copy link
Member

@nobodyiam I can't reply to messages in #5227 image

Reply to #5227 (comment) The minimum value of 5 is set based on the values ​​of item.key.length.limit and item.value.length.limit. Considering that actual use will not set a value smaller than 5

Reply to #5227 (comment) countByNamespaceIdAndKeyIsNotEmpty or countByNamespaceIdAndKeyIsNotNullAndKeyNotEmpty will result in the following exception: IsEmpty / IsNotEmpty can only be used on collection properties!

@youngzil It was locked after being closed, but I've now unlocked it, so we can continue our discussion in that thread.

@nobodyiam nobodyiam merged commit 94c28af into apolloconfig:master Oct 6, 2024
7 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 6, 2024
@nobodyiam nobodyiam added this to the 2.4.0 milestone Oct 6, 2024
@youngzil youngzil deleted the feature/namespace-num-limit branch October 7, 2024 09:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants