feat(biz): Added the value length limit function for AppId-level configuration items #5264
Conversation
dosubot
bot
added
the
size:L
WalkthroughThe changes introduced in this pull request encompass enhancements to the Apollo Configuration Center, including the addition of a value length limit function for AppId-level configuration items. Updates include improved server configuration links, REST template client connection pooling, and global search capabilities for administrators. Several fixes address issues with duplicate comments and blank lines. The modifications also enhance the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (6)
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemServiceTest.java (2)
87-110: Consider adding positive test cases for namespace value length limits.While the error case is well tested, consider adding a positive test case where the value length is within the limit to ensure the validation logic works correctly in both scenarios.
Example addition:
@Test public void testSaveItemWithValidNamespaceValueLength() { long namespaceId = 1L; String itemValue = "test-demo"; Map<Long, Integer> namespaceValueLengthOverride = new HashMap<>(); namespaceValueLengthOverride.put(namespaceId, itemValue.length() + 1); when(bizConfig.namespaceValueLengthLimitOverride()).thenReturn(namespaceValueLengthOverride); when(bizConfig.itemKeyLengthLimit()).thenReturn(100); Item item = createTestItem(namespaceId, itemValue); Item savedItem = itemService2.save(item); Assert.assertEquals(itemValue, savedItem.getValue()); }
115-139: Consider adding edge cases for AppId value length limits.The current test covers the basic error case. Consider adding tests for:
- Edge case where value length equals the limit
- Case where both namespace and AppId limits are configured
Example addition:
@Test public void testSaveItemWithExactAppIdValueLength() { String appId = "testApp"; long namespaceId = 1L; String itemValue = "test-demo"; Map<String, Integer> appIdValueLengthOverride = new HashMap<>(); appIdValueLengthOverride.put(appId, itemValue.length()); when(bizConfig.appIdValueLengthLimitOverride()).thenReturn(appIdValueLengthOverride); when(bizConfig.itemKeyLengthLimit()).thenReturn(100); Item item = createTestItem(namespaceId, itemValue); Item savedItem = itemService2.save(item); Assert.assertEquals(itemValue, savedItem.getValue()); }apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemService.java (2)
Line range hint
238-242: Fix incorrect namespace parameter in parent limit calculation.There's a bug in the parent limit calculation. The method is passing
grayNamespacetogetItemValueLengthLimitinstead ofparentNamespace, which could result in incorrect limit calculation for gray namespaces.Apply this fix:
- int parentLimit = getItemValueLengthLimit(grayNamespace); + int parentLimit = getItemValueLengthLimit(parentNamespace);
260-271: LGTM! Well-structured hierarchical configuration.The implementation follows a clear precedence order (namespace > appId > default) and includes proper null checks. This aligns well with the PR objectives of supporting both namespace and AppId-level configurations.
Consider documenting this precedence order in the configuration documentation to help users understand how the limits are applied.
For future scalability, consider:
- Extracting the configuration hierarchy logic into a separate strategy class if more levels are added in the future
- Adding metrics/logging to track which configuration level is being used, helping administrators understand the actual usage patterns
docs/zh/deployment/distributed-deployment-guide.md (1)
1486-1488: Add language specifier to the code block.The code block should specify the language for proper syntax highlighting.
Apply this change:
-``` +```json appid.value.length.limit.override = {appId-demo1:200,aappId-demo2:300}<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 1486-1486: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> </blockquote></details> <details> <summary>docs/en/deployment/distributed-deployment-guide.md (1)</summary><blockquote> `1545-1547`: **Add language specification to the code block** The code block should specify the language for better syntax highlighting and documentation consistency. ```diff -``` +```json {appId-demo1:200,aappId-demo2:300}<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 1545-1545: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Files that changed from the base of the PR and between ced6383de19b7cf4e37ab68ab8babcde7a9cbbba and 6ab0caa721b58e9a4539e87802313ceaa3a94bd4. </details> <details> <summary>📒 Files selected for processing (7)</summary> * CHANGES.md (1 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/service/ItemService.java (3 hunks) * apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/config/BizConfigTest.java (3 hunks) * apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemServiceTest.java (3 hunks) * docs/en/deployment/distributed-deployment-guide.md (1 hunks) * docs/zh/deployment/distributed-deployment-guide.md (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> <details> <summary>docs/en/deployment/distributed-deployment-guide.md</summary><blockquote> 1545-1545: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> <details> <summary>docs/zh/deployment/distributed-deployment-guide.md</summary><blockquote> 1486-1486: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </blockquote></details> </details> <details> <summary>🔇 Additional comments (11)</summary><blockquote> <details> <summary>CHANGES.md (1)</summary><blockquote> `18-18`: **LGTM! The changelog entry is well-formatted and accurately describes the feature.** The entry follows the established format and clearly communicates the addition of the AppId-level value length limit function, properly linking to PR #5264. </blockquote></details> <details> <summary>apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/config/BizConfigTest.java (2)</summary><blockquote> `21-22`: **LGTM: Import statements are appropriate.** The added imports support the new test functionality for JSON parsing and map operations. --- `112-143`: **LGTM: Comprehensive test coverage for AppId value length limit override.** The test method effectively covers essential scenarios including null config, empty JSON, invalid JSON, and both single and multiple AppId overrides. </blockquote></details> <details> <summary>apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemServiceTest.java (2)</summary><blockquote> `43-58`: **LGTM! Clean setup with proper dependency injection.** The setup properly initializes all required dependencies and creates a separate ItemService instance for testing the new configuration features. --- `61-65`: **Verify the namespaceId change impact.** The namespaceId has been changed from 3 to 1. Please ensure this aligns with the test data in the new `namespace-test.sql` script. <details> <summary>✅ Verification successful</summary> **The namespaceId change to 1 is correct and verified** The change from namespaceId 3 to 1 is valid as the test data in `namespace-test.sql` shows: - A namespace with ID 1 is explicitly created: `INSERT INTO "Namespace" (`Id`, `AppId`,...) VALUES(1,'testApp',...)` - This namespace is referenced in both test files: - `namespace-test.sql` has an item with `NamespaceId=1` - `item-test.sql` has a test item with `NamespaceId=1` </details> <details> <summary>🏁 Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Check if namespace with ID 1 exists in the test SQL rg -A 5 "INSERT.*INTO.*namespace.*1[,)]" apollo-biz/src/test/resources/sql/namespace-test.sqlLength of output: 95
Script:
#!/bin/bash # Let's first check if the file exists and see its content cat apollo-biz/src/test/resources/sql/namespace-test.sql # Also check item-test.sql as it's used together cat apollo-biz/src/test/resources/sql/item-test.sql # Let's also search for any namespace with ID 1 in all SQL files rg "INSERT.*namespace.*1[,)]" apollo-biz/src/test/resources/sql/Length of output: 4078
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemService.java (1)
Line range hint
221-233: LGTM! Verify gray namespace handling.The changes correctly support the new AppId-level configuration while maintaining existing gray namespace functionality. The code properly retrieves the value length limit using the new hierarchical approach.
Let's verify the gray namespace handling:
✅ Verification successful
Gray namespace handling is correctly implemented
The verification confirms that:
- The gray namespace pattern
[0-9]{14}-[a-zA-Z0-9]{16}is uniquely defined in ItemService and consistently used- The gray namespace value length limit logic is properly handled through
getGrayNamespaceItemValueLengthLimitwhich:
- Checks for parent namespace limits
- Returns the higher limit between gray and parent namespace
- The implementation aligns with the gray release functionality in the portal module
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any other gray namespace patterns or special cases in the codebase # that might need similar handling # Search for cluster pattern usage rg -A 3 'clusterPattern' --type java # Search for gray namespace related code rg -A 5 'isGray|grayNamespace|GrayNamespace' --type javaLength of output: 7319
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (2)
61-63: LGTM: Type reference declaration follows established patternsThe new type reference for AppId value length override is properly implemented using TypeToken for type safety, consistent with existing patterns in the codebase.
112-132: Verify configuration override precedence and integrationThe implementation addresses the requirements from issue #5254, but we should verify:
- The interaction between AppId and Namespace level overrides
- The configuration loading and refresh behavior
- The error scenarios and edge cases
Let's verify the configuration usage:
✅ Verification successful
Let's examine the test coverage and implementation details more thoroughly.
Configuration override implementation is properly integrated and tested
The implementation is well-structured with:
- Correct precedence handling in ItemService - namespace-level overrides take precedence over app-level overrides
- Comprehensive test coverage including:
- Empty/invalid configuration scenarios
- Single and multiple override entries
- JSON parsing error handling
- Integration with ItemService validation logic
The code successfully addresses the requirements from issue #5254 with proper validation, error handling, and precedence rules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of these configuration methods echo "Searching for configuration usage..." rg -l "appIdValueLengthLimitOverride|namespaceValueLengthLimitOverride" --type java # Search for test coverage echo "Checking test coverage..." rg -l "appid\.value\.length\.limit\.override|namespace\.value\.length\.limit\.override" --type java --glob "*Test.java"Length of output: 759
Script:
#!/bin/bash # Check ItemService implementation for usage context echo "Checking ItemService implementation..." rg -A 10 "appIdValueLengthLimitOverride|namespaceValueLengthLimitOverride" apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemService.java # Check test implementations echo "Checking test implementations..." rg -A 10 "appIdValueLengthLimitOverride|namespaceValueLengthLimitOverride" apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/config/BizConfigTest.java rg -A 10 "appIdValueLengthLimitOverride|namespaceValueLengthLimitOverride" apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemServiceTest.javaLength of output: 3879
docs/zh/deployment/distributed-deployment-guide.md (2)
1484-1492: LGTM: Clear documentation for AppId-level value length limit configuration.The documentation clearly explains:
- The purpose and scope of
appid.value.length.limit.override- The JSON format for configuration
- The inheritance behavior for new namespaces
🧰 Tools
🪛 Markdownlint
1486-1486: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
1493-1495: LGTM: Clear documentation for Namespace-level value length limit configuration.The documentation effectively describes:
- The purpose of
namespace.value.length.limit.override- The override behavior for both global and AppId-level configurations
- The JSON format using namespace IDs
docs/en/deployment/distributed-deployment-guide.md (1)
1542-1554: LGTM! Clear and well-structured documentation for the new configuration items.The documentation clearly explains:
- The purpose and behavior of the new configuration items
- The inheritance relationship between different configuration levels
- Provides clear examples for better understanding
🧰 Tools
🪛 Markdownlint
1545-1545: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/config/BizConfigTest.java
Outdated
Show resolved
Hide resolved
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemServiceTest.java
Outdated
Show resolved
Hide resolved
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java
Outdated
Show resolved
Hide resolved
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemServiceTest.java (2)
78-97: Add positive test case for namespace value length limit.The current test only covers the error case when the value exceeds the limit. Consider adding a test case where the value length is within the limit to ensure the override works correctly in both scenarios.
Would you like me to help generate the additional test case?
99-120: Add positive test case and edge cases for AppId value length limit.While the error case is covered, consider adding:
- A positive test case where the value length is within the limit
- Edge cases testing the interaction between namespace and AppId-level limits
Would you like me to help generate these additional test cases?
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (1)
258-271: Consider enhancing type safety in parseOverrideConfigThe helper method is well-implemented with proper error handling, filtering, and immutability. However, the type casting could be made more robust.
Consider this enhancement:
private <K, V> Map<K, V> parseOverrideConfig(String configValue, Type typeReference) { Map<K, V> result = Maps.newHashMap(); if (!Strings.isNullOrEmpty(configValue)) { try { Map<K, V> parsed = GSON.fromJson(configValue, typeReference); result = parsed.entrySet().stream() - .filter(e -> e.getValue() != null && ((Integer) e.getValue()) > 0) + .filter(e -> e.getValue() != null && + (e.getValue() instanceof Integer) && + ((Integer) e.getValue()) > 0) .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); } catch (Exception e) { logger.error("Invalid override config value: {}", configValue, e); } } return Collections.unmodifiableMap(result); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (5 hunks)
- apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/config/BizConfigTest.java (3 hunks)
- apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemServiceTest.java (4 hunks)
🔇 Additional comments (6)
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/ItemServiceTest.java (3)
Line range hint
19-58: LGTM! Well-structured test setup.The setup properly initializes both the real and mocked dependencies needed for testing the new value length limit functionality.
157-166: LGTM! Well-implemented helper method.The
createItemhelper method effectively reduces code duplication and improves test maintainability, addressing the previous review feedback.
Line range hint
1-167: Verify complete test coverage across the codebase.Let's ensure all related components have appropriate test coverage.
✅ Verification successful
Test coverage is comprehensive and properly implemented
The test coverage for value length limit functionality is complete and well-structured across the codebase:
BizConfigTestthoroughly tests the AppId value length limit override functionality with:
- Empty/null configuration cases
- Invalid JSON handling
- Single and multiple AppId overrides
- Edge cases (zero and negative values)
ItemServiceTestcovers both namespace-level and AppId-level value length limit validations- The implementation in
ItemServicecorrectly cascades through namespace override, AppId override, and default limit checksThe test coverage adequately verifies all aspects of the value length limit feature, including configuration parsing, validation logic, and error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other test files that might need updates for the new functionality # Look for test files that might need coverage for the new AppId-level configuration echo "Searching for related test files..." fd -e java -e groovy -e scala test --exec rg -l "BizConfig|ItemService|namespace\.value\.length\.limit|appid\.value\.length\.limit" # Look for configuration test files echo "Searching for configuration test files..." fd -e properties -e yaml -e xml test --exec rg -l "value\.length\.limit"Length of output: 3199
Script:
#!/bin/bash # Let's check the BizConfig and ItemService test coverage more specifically # Check BizConfigTest for value length limit configurations echo "Checking BizConfigTest coverage:" rg -A 10 "namespaceValueLengthLimitOverride|appIdValueLengthLimitOverride" ./apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/config/BizConfigTest.java # Check ItemService implementations for value length limit validations echo -e "\nChecking ItemService implementations:" rg -A 5 "namespaceValueLengthLimitOverride|appIdValueLengthLimitOverride" ./apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/ItemService.java # Check for any other potential test files with value length validations echo -e "\nChecking other potential test files:" rg -A 5 "value.*length.*limit" ./apollo-biz/src/test/java/Length of output: 7357
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (3)
34-35: LGTM: Logger and type reference additions are well-structuredThe logger implementation and type reference follow the established patterns in the codebase.
Also applies to: 41-42, 65-67
204-204: LGTM: Consistent use of parseOverrideConfigThe refactoring to use the common helper method improves code maintainability.
116-124: LGTM: Override configuration methods align with requirementsThe implementation properly supports both AppId and Namespace level value length limit configurations as requested in issue #5254. The methods maintain consistency with the existing configuration patterns while adding the new AppId-level functionality.
Let's verify the configuration key names are used consistently across the codebase:
✅ Verification successful
Configuration keys are used consistently across the codebase
The verification confirms that both configuration keys are used consistently:
appid.value.length.limit.overrideis properly documented in deployment guides (both English and Chinese) with correct format examplesnamespace.value.length.limit.overrideis also well documented with proper usage examples- Both keys are correctly used in the implementation (BizConfig.java) and have corresponding test coverage (BizConfigTest.java)
- The documentation clearly explains the override hierarchy between these configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to the configuration keys echo "Searching for appid.value.length.limit.override usage:" rg "appid\.value\.length\.limit\.override" echo -e "\nSearching for namespace.value.length.limit.override usage:" rg "namespace\.value\.length\.limit\.override"Length of output: 3947
| @Test | ||
| public void testAppIdValueLengthLimitOverride() { | ||
| when(environment.getProperty("appid.value.length.limit.override")).thenReturn(null); | ||
| Map<String, Integer> result = bizConfig.appIdValueLengthLimitOverride(); | ||
| assertTrue(result.isEmpty()); | ||
|
|
||
| String input = "{}"; | ||
| when(environment.getProperty("appid.value.length.limit.override")).thenReturn(input); | ||
| result = bizConfig.appIdValueLengthLimitOverride(); | ||
| assertTrue(result.isEmpty()); | ||
|
|
||
| input = "invalid json"; | ||
| when(environment.getProperty("appid.value.length.limit.override")).thenReturn(input); | ||
| result = bizConfig.appIdValueLengthLimitOverride(); | ||
| assertTrue(result.isEmpty()); | ||
|
|
||
| input = "{'appid1':555}"; | ||
| when(environment.getProperty("appid.value.length.limit.override")).thenReturn(input); | ||
| int overrideValue = bizConfig.appIdValueLengthLimitOverride().get("appid1"); | ||
| assertEquals(1, bizConfig.appIdValueLengthLimitOverride().size()); | ||
| assertEquals(555, overrideValue); | ||
|
|
||
| input = "{'appid1':555,'appid2':666}"; | ||
| when(environment.getProperty("appid.value.length.limit.override")).thenReturn(input); | ||
| overrideValue = bizConfig.appIdValueLengthLimitOverride().get("appid2"); | ||
| assertEquals(2, bizConfig.appIdValueLengthLimitOverride().size()); | ||
| assertEquals(666, overrideValue); | ||
|
|
||
| input = "{'appid1':555,'appid2':666,'appid3':0,'appid4':-1}"; | ||
| when(environment.getProperty("appid.value.length.limit.override")).thenReturn(input); | ||
| overrideValue = bizConfig.appIdValueLengthLimitOverride().get("appid2"); | ||
| assertEquals(2, bizConfig.appIdValueLengthLimitOverride().size()); | ||
| assertEquals(666, overrideValue); | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test organization and documentation.
The test method is handling multiple scenarios which makes it harder to understand and maintain. Consider:
- Breaking down into separate focused test methods with clear @DisplayName annotations
- Adding Javadoc to document test scenarios
- Using constants for test values
- Using more realistic AppId examples
Example refactor:
@DisplayName("AppId value length limit override tests")
class AppIdValueLengthLimitTests {
private static final String GATEWAY_APP_ID = "api-gateway";
private static final int GATEWAY_LENGTH_LIMIT = 102400;
@Test
@DisplayName("Should return empty map for null config")
void nullConfigReturnsEmptyMap() {
when(environment.getProperty("appid.value.length.limit.override")).thenReturn(null);
assertTrue(bizConfig.appIdValueLengthLimitOverride().isEmpty());
}
@Test
@DisplayName("Should handle valid single entry")
void validSingleEntry() {
String input = String.format("{'%s':%d}", GATEWAY_APP_ID, GATEWAY_LENGTH_LIMIT);
when(environment.getProperty("appid.value.length.limit.override")).thenReturn(input);
Map<String, Integer> result = bizConfig.appIdValueLengthLimitOverride();
assertEquals(1, result.size());
assertEquals(GATEWAY_LENGTH_LIMIT, result.get(GATEWAY_APP_ID).intValue());
}
// Add more focused test methods...
}
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/config/BizConfigTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/config/BizConfigTest.java (1)
110-150: Good test coverage, but could be improved for maintainability.The test method thoroughly covers the basic scenarios including null config, empty JSON, invalid JSON, and valid entries. However, consider improving maintainability by:
- Breaking down into focused test methods
- Using constants for test values
- Adding method documentation
- Using more realistic AppId examples (e.g., "api-gateway" instead of "appid1")
Example refactor:
private static final String GATEWAY_APP_ID = "api-gateway"; private static final int GATEWAY_LENGTH_LIMIT = 102400; /** * Tests the AppId value length limit override functionality */ @Test @DisplayName("Should handle null configuration") public void testAppIdValueLengthLimitOverride_NullConfig() { when(environment.getProperty("appid.value.length.limit.override")).thenReturn(null); assertTrue(bizConfig.appIdValueLengthLimitOverride().isEmpty()); } @Test @DisplayName("Should handle valid configuration") public void testAppIdValueLengthLimitOverride_ValidConfig() { String input = String.format("{'%s':%d}", GATEWAY_APP_ID, GATEWAY_LENGTH_LIMIT); when(environment.getProperty("appid.value.length.limit.override")).thenReturn(input); Map<String, Integer> result = bizConfig.appIdValueLengthLimitOverride(); assertEquals(1, result.size()); assertEquals(GATEWAY_LENGTH_LIMIT, result.get(GATEWAY_APP_ID).intValue()); } // Additional test methods for other scenarios...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/config/BizConfigTest.java (3 hunks)
🔇 Additional comments (1)
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/config/BizConfigTest.java (1)
21-21: LGTM!The Map import is required for the new test method.
| input = "{'appid1':555,'appid2':666,'appid3':0,'appid4':-1}"; | ||
| when(environment.getProperty("appid.value.length.limit.override")).thenReturn(input); | ||
| result = bizConfig.appIdValueLengthLimitOverride(); | ||
|
|
||
| assertTrue(result.containsKey("appid1")); | ||
| assertTrue(result.containsKey("appid2")); | ||
| assertFalse(result.containsKey("appid3")); | ||
| assertFalse(result.containsKey("appid4")); | ||
| assertEquals(2, result.size()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tests for boundary values.
While the test verifies filtering of invalid values (0, -1), it should also test boundary conditions:
@Test
@DisplayName("Should handle boundary values")
public void testAppIdValueLengthLimitOverride_Boundaries() {
String input = String.format(
"{'%s':1,'%s':%d,'%s':%d}",
"min-app", "max-app", Integer.MAX_VALUE,
"invalid-app", Integer.MIN_VALUE
);
when(environment.getProperty("appid.value.length.limit.override")).thenReturn(input);
Map<String, Integer> result = bizConfig.appIdValueLengthLimitOverride();
assertTrue(result.containsKey("min-app"));
assertTrue(result.containsKey("max-app"));
assertFalse(result.containsKey("invalid-app"));
assertEquals(Integer.MAX_VALUE, result.get("max-app").intValue());
assertEquals(1, result.get("min-app").intValue());
}
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java
Outdated
Show resolved
Hide resolved
# Conflicts: # CHANGES.md
…es and add value validity verification
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (1)
258-273: Consider enhancing type safety for numeric values.The implementation is robust with good error handling and immutability. However, since this is primarily used for numeric limits, consider adding type bounds to prevent potential ClassCastException.
- private <K, V> Map<K, V> parseOverrideConfig(String configValue, Type typeReference, Predicate<V> valueFilter) { + private <K, V extends Number> Map<K, V> parseOverrideConfig(String configValue, Type typeReference, Predicate<V> valueFilter) {docs/zh/deployment/distributed-deployment-guide.md (1)
1486-1488: Add language specifier to the code block.The code block should specify a language for better syntax highlighting.
-``` +```json appid.value.length.limit.override = {"appId-demo1":200,"appId-demo2":300}<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary> 1486-1486: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </blockquote></details> <details> <summary>docs/en/deployment/distributed-deployment-guide.md (1)</summary><blockquote> `1545-1547`: **Add language specifier to the code block and use quotation marks in the example.** The code block should specify a language and the JSON values should be quoted for better clarity. ```diff -``` +```json -appid.value.length.limit.override = {"appId-demo1":200,"appId-demo2":300} +appid.value.length.limit.override = {"appId-demo1": "200", "appId-demo2": "300"}🧰 Tools
🪛 Markdownlint
1545-1545: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
CHANGES.md(1 hunks)apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java(5 hunks)docs/en/deployment/distributed-deployment-guide.md(1 hunks)docs/zh/deployment/distributed-deployment-guide.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGES.md
🧰 Additional context used
🪛 Markdownlint
docs/en/deployment/distributed-deployment-guide.md
1545-1545: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/zh/deployment/distributed-deployment-guide.md
1486-1486: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (7)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (4)
33-35: LGTM: Logger and imports are appropriate.
The new imports and logger field are correctly added to support the new functionality.
Also applies to: 41-42
65-67: LGTM: Type reference is properly defined.
The new type reference follows the existing pattern and correctly defines the type for AppId value length overrides.
204-204: LGTM: Good reuse of common parsing logic.
The existing method has been updated to use the new parsing logic, reducing code duplication.
116-124: LGTM: Configuration override methods are well implemented.
The methods correctly implement the AppId and Namespace level value length limit overrides. Let's verify the configuration keys are documented.
✅ Verification successful
Both configuration keys are properly documented
The documentation in distributed-deployment-guide.md thoroughly covers both configuration keys:
appid.value.length.limit.override: Documented with format, purpose, and inheritance behaviornamespace.value.length.limit.override: Documented with format, purpose, and override precedence
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the new configuration keys are documented
rg -l "appid\.value\.length\.limit\.override" --type md
Length of output: 159
Script:
#!/bin/bash
# Check content of documentation files to verify if both configuration keys are properly documented
rg "appid\.value\.length\.limit\.override|namespace\.value\.length\.limit\.override" docs/en/deployment/distributed-deployment-guide.md -A 2 -B 2
Length of output: 1690
docs/zh/deployment/distributed-deployment-guide.md (1)
1484-1495: Documentation looks good!
The new sections clearly explain the configuration options for value length limits at both AppId and Namespace levels. The documentation:
- Clearly describes the purpose and format of each configuration
- Provides good examples
- Explains the inheritance and override behavior
🧰 Tools
🪛 Markdownlint
1486-1486: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/en/deployment/distributed-deployment-guide.md (2)
1542-1551: LGTM! Clear documentation for appId-level value length limit override.
The documentation clearly explains:
- Purpose of the configuration
- JSON format structure
- Inheritance behavior for new namespaces
🧰 Tools
🪛 Markdownlint
1545-1545: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1552-1554: LGTM! Clear documentation for namespace-level value length limit override.
The documentation clearly explains:
- Purpose of the configuration
- Override behavior for both item.value.length.limit and appid.value.length.limit.override
- JSON format structure using namespace IDs
What's the purpose of this PR
Which issue(s) this PR fixes:
Fixes #5254
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
Release Notes for Apollo Version 2.4.0
New Features
Bug Fixes
Documentation