feat: support incremental configuration synchronization client #90
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe pull request enhances Apollo's configuration management system by implementing support for incremental configuration updates. Key modifications include the addition of new classes and enums, as well as methods for merging configurations based on changes such as additions, modifications, and deletions. The Changes
Possibly related PRs
Suggested labels
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 (
|
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (10)
apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java (2)
23-28: Enhance class-level documentationThe current documentation has a typo and could be more descriptive. Consider improving it to better explain the purpose and behavior of each sync type.
/** - * This enum represents all the possible Configuration sync from apollo-config + * This enum represents all possible configuration synchronization types supported by Apollo: + * - FULLSYNC: Complete configuration synchronization (default) + * - INCREMENTALSYNC: Partial configuration updates for better performance * * @author jason */
29-29: Consider extracting string values as constantsTo maintain consistency and ease maintenance, consider extracting the string values as public constants.
public enum ConfigSyncType { + public static final String FULL_SYNC_VALUE = "FullSync"; + public static final String INCREMENTAL_SYNC_VALUE = "IncrementalSync"; - FULLSYNC("FullSync"), INCREMENTALSYNC("IncrementalSync"); + FULLSYNC(FULL_SYNC_VALUE), INCREMENTALSYNC(INCREMENTAL_SYNC_VALUE);apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java (3)
22-25: Enhance class documentation for better API understanding.As this is a core DTO class that's part of the public API, consider enhancing the documentation with:
- @SInCE tag for API versioning
- Thread safety guarantees
- Null handling behavior
- Usage examples
/** * Holds the information for a Configuration change. + * <p> + * This class is used to represent individual configuration changes during incremental + * synchronization. It captures the key being modified, its new value, and the type + * of change (add/modify/delete). + * + * <p> + * Thread Safety: Instances of this class are not thread-safe and should be used with + * proper synchronization if shared between threads. + * + * <p> + * Null Handling: The key must not be null. The newValue can be null for DELETE operations. + * + * @since 1.0.0 * @author jason */
62-70: Make toString more defensive.The current implementation is good, but consider making it more defensive against null values.
@Override public String toString() { final StringBuilder sb = new StringBuilder("ConfigChange{"); sb.append(" key='").append(key).append('\''); - sb.append(", newValue='").append(newValue).append('\''); + sb.append(", newValue='").append(newValue == null ? "null" : newValue).append('\''); sb.append(", configurationChangeType=").append(configurationChangeType); sb.append('}'); return sb.toString(); }
71-71: Add equals and hashCode methods.Consider adding equals and hashCode methods as this DTO might be used in collections or comparisons.
+ @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof ConfigurationChange)) return false; + ConfigurationChange that = (ConfigurationChange) o; + return Objects.equals(key, that.key) && + Objects.equals(newValue, that.newValue) && + configurationChangeType == that.configurationChangeType; + } + + @Override + public int hashCode() { + return Objects.hash(key, newValue, configurationChangeType); + }apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java (2)
72-78: Add null safety to gettersConsider adding null safety to prevent NPE when accessing the configuration changes list.
public String getConfigSyncType() { return configSyncType; } public List<ConfigurationChange> getConfigurationChanges() { - return configurationChanges; + return configurationChanges != null ? configurationChanges : Lists.newArrayList(); }
Line range hint
113-126: Update toString to include new fieldsThe toString method should include all fields for complete object representation.
public String toString() { final StringBuilder sb = new StringBuilder("ApolloConfig{"); sb.append("appId='").append(appId).append('\''); sb.append(", cluster='").append(cluster).append('\''); sb.append(", namespaceName='").append(namespaceName).append('\''); sb.append(", configurations=").append(configurations); sb.append(", releaseKey='").append(releaseKey).append('\''); + sb.append(", configSyncType='").append(configSyncType).append('\''); + sb.append(", configurationChanges=").append(configurationChanges); sb.append('}'); return sb.toString(); }apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java (3)
156-197: Enhance test coverage and assertionsWhile the test covers the basic functionality, consider these improvements:
- Add assertions to verify initial state after first sync
- Explicitly assert that deleted key (someKey1) doesn't exist in final config
- Consider testing edge cases (empty changes list, null values)
@Test public void testLoadConfigWithIncrementalSync() throws Exception { String someKey = "someKey"; String someValue = "someValue"; String someKey1 = "someKey1"; String someValue1 = "someKey1"; Map<String, String> configurations = Maps.newHashMap(); configurations.put(someKey, someValue); configurations.put(someKey1, someValue1); ApolloConfig someApolloConfig = assembleApolloConfig(configurations); when(someResponse.getStatusCode()).thenReturn(200); when(someResponse.getBody()).thenReturn(someApolloConfig); RemoteConfigRepository remoteConfigRepository = new RemoteConfigRepository(someNamespace); remoteConfigRepository.sync(); + + // Verify initial state + Properties initialConfig = remoteConfigRepository.getConfig(); + assertEquals(2, initialConfig.size()); + assertEquals(someValue, initialConfig.getProperty(someKey)); + assertEquals(someValue1, initialConfig.getProperty(someKey1)); List<ConfigurationChange> configurationChanges=new ArrayList<>(); String someNewValue = "someNewValue"; configurationChanges.add(new ConfigurationChange(someKey, someNewValue, ConfigurationChangeType.MODIFIED)); configurationChanges.add(new ConfigurationChange(someKey1, null, ConfigurationChangeType.DELETED)); String someKey2 = "someKey2"; String someValue2 = "someValue2"; configurationChanges.add(new ConfigurationChange(someKey2, someValue2, ConfigurationChangeType.ADDED)); ApolloConfig someApolloConfigWithIncrementalSync = assembleApolloConfigWithIncrementalSync(configurationChanges); when(someResponse.getStatusCode()).thenReturn(200); when(someResponse.getBody()).thenReturn(someApolloConfigWithIncrementalSync); remoteConfigRepository.sync(); Properties config = remoteConfigRepository.getConfig(); assertEquals(2, config.size()); assertEquals("someNewValue", config.getProperty("someKey")); assertEquals("someValue2", config.getProperty("someKey2")); + assertNull("Deleted key should not exist", config.getProperty(someKey1)); assertEquals(ConfigSourceType.REMOTE, remoteConfigRepository.getSourceType()); remoteConfigLongPollService.stopLongPollingRefresh(); }
420-430: Fix indentation and add parameter validationThe helper method needs some improvements:
- Fix indentation to match the codebase style
- Add validation for the configurationChanges parameter
private ApolloConfig assembleApolloConfigWithIncrementalSync(List<ConfigurationChange> configurationChanges) { + if (configurationChanges == null) { + throw new IllegalArgumentException("configurationChanges cannot be null"); + } String someAppId = "appId"; String someClusterName = "cluster"; String someReleaseKey = "1"; - ApolloConfig apolloConfig = - new ApolloConfig(someAppId, someClusterName, someNamespace, someReleaseKey); + ApolloConfig apolloConfig = new ApolloConfig(someAppId, someClusterName, someNamespace, someReleaseKey); apolloConfig.setConfigSyncType(ConfigSyncType.INCREMENTALSYNC.getValue()); apolloConfig.setConfigurationChanges(configurationChanges); return apolloConfig; }
Line range hint
156-430: Consider adding tests for error scenarios and edge casesTo ensure robust testing of the incremental sync functionality, consider adding these test cases:
- Test with empty changes list
- Test with invalid change types
- Test with null values in ConfigurationChange objects
- Test concurrent modifications during sync
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java(3 hunks)apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java(4 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java(4 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java(1 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java(1 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigurationChangeType.java(1 hunks)pom.xml(1 hunks)
🔇 Additional comments (8)
apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigurationChangeType.java (1)
23-25: LGTM! The enum implementation is clean and focused.
The enum constants follow standard naming conventions and cover all necessary change types for configuration updates.
apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java (3)
31-35: LGTM!
The constructor and field implementation follow good practices with proper immutability and visibility.
56-61: LGTM!
The getValue implementation is clean and well-documented.
28-62: Verify enum usage across the codebase
Let's ensure this enum is properly integrated with other components, particularly the RemoteConfigRepository mentioned in the AI summary.
✅ Verification successful
Let me analyze the implementation details of RemoteConfigRepository to ensure proper integration.
Let me try a different approach to check the implementation details.
ConfigSyncType is properly integrated across the codebase
The enum is correctly integrated with the core components:
- ApolloConfig DTO properly handles the sync type through getter/setter methods
- RemoteConfigRepository correctly uses the enum for type checking and implements special handling for INCREMENTALSYNC
- The implementation follows a consistent pattern where string values are converted to enum using fromString()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of ConfigSyncType across the codebase
# Check for ConfigSyncType usage
echo "Checking ConfigSyncType usage..."
rg -l "ConfigSyncType"
# Check for string literal usage that might need to use the enum
echo "Checking for potential string literals that should use the enum..."
rg -i "FullSync|IncrementalSync" --type java
Length of output: 2025
Script:
#!/bin/bash
# Check RemoteConfigRepository implementation
echo "Checking RemoteConfigRepository implementation..."
ast-grep --pattern 'class RemoteConfigRepository {
$$$
}'
# Check ApolloConfig implementation
echo "Checking ApolloConfig implementation..."
ast-grep --pattern 'class ApolloConfig {
$$$
}'
Length of output: 311
Script:
#!/bin/bash
# Check how ConfigSyncType is used in RemoteConfigRepository
echo "Checking ConfigSyncType usage in RemoteConfigRepository..."
rg -A 5 -B 5 "ConfigSyncType" apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
# Check how ConfigSyncType is integrated in ApolloConfig
echo "Checking ConfigSyncType integration in ApolloConfig..."
rg -A 5 -B 5 "ConfigSyncType" apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java
Length of output: 2183
apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java (1)
1-71: Verify integration with incremental sync architecture.
As this is a core component of the incremental configuration synchronization feature:
- Consider adding a
getChangeDescription()method to provide human-readable change descriptions for logging - Consider adding a timestamp field to track when the change occurred
- Consider implementing Serializable as DTOs often need serialization
Let's verify the usage patterns:
apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java (1)
39-41: Verify JSON serialization/deserialization
Since this is a DTO class, ensure that the new fields are properly handled during JSON serialization/deserialization, especially the configSyncType enum conversion.
✅ Verification successful
No serialization issues with new fields in ApolloConfig
The ApolloConfig class is a simple POJO without any specific JSON annotations or serialization constraints. The new fields configSyncType and configurationChanges will be automatically handled by the default serialization behavior of JSON libraries (like Gson which is used in the codebase). Since these are standard Java types (String and List), no special serialization handling is required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for JSON serialization configuration or tests
rg -t java "class.*ApolloConfig.*JSON|@JsonFormat|@JsonProperty"
Length of output: 66
Script:
#!/bin/bash
# Search for JSON serialization tests or usage of ApolloConfig with JSON
rg -t java "ApolloConfig.*toJson|fromJson|ObjectMapper.*ApolloConfig" -A 3
# Search for any serialization annotations on the fields
ast-grep --pattern 'class ApolloConfig {
$$$
@$_
private String configSyncType;
$$$
}'
# Search for any test files related to ApolloConfig
fd "ApolloConfig.*Test.java|Test.*ApolloConfig.java"
Length of output: 14218
Script:
#!/bin/bash
# Let's check if ApolloConfig uses any JSON serialization library
rg -t java "@JsonSerialize|@JsonDeserialize|@JsonProperty|@JsonIgnore" apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java
# Check how ApolloConfig is used in tests for serialization
rg -t java "ApolloConfig" apollo-client/src/test/java/com/ctrip/framework/apollo/spring/spi/ApolloConfigRegistrarHelperTest.java apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/importer/PureApolloConfigTest.java -A 2
# Check if there's a custom serializer/deserializer for ApolloConfig
fd "Apollo.*Serializer.java|Apollo.*Deserializer.java"
Length of output: 5085
Script:
#!/bin/bash
# Let's check the base class and any serialization-related code in ApolloConfig
rg -t java "class ApolloConfig|implements.*Serializable" apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java -A 2
# Check if there's any Gson configuration or custom type adapters for ApolloConfig
rg -t java "registerTypeAdapter.*ApolloConfig|GsonBuilder.*ApolloConfig"
# Check if there are any other serialization-related files in apollo-core
fd "Serialization|Gson|Json" apollo-core/src/main/java/
Length of output: 329
pom.xml (1)
63-63: Verify version bump justification
The version jump from 2.2.0-SNAPSHOT to 2.5.0-SNAPSHOT seems significant for a feature addition. According to semantic versioning:
- Major version (2.x.x): Unchanged, appropriate as no breaking changes mentioned
- Minor version (x.5.x): Large jump (+0.3.0) for a single feature
Please clarify if:
- This version bump includes other features/changes not mentioned in the PR
- The incremental sync feature introduces breaking changes
- There are intermediate versions (2.3.0, 2.4.0) in other branches
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java (1)
34-36: LGTM: Required imports added for incremental sync support
The new imports for ConfigSyncType and ConfigurationChangeType enums are correctly added and necessary for the incremental synchronization functionality.
Comment on lines
20
to
22
| /** | ||
| * @author jason | ||
| */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance enum documentation for better API clarity.
As this enum is part of the public API and crucial for the incremental configuration synchronization feature, please enhance the documentation to include:
- Purpose and usage of the enum
- Description of each constant
- Relationship with incremental sync feature
- API version information
/**
+ * Represents the type of change that occurred to a configuration item during
+ * incremental synchronization.
+ *
+ * <p>This enum is used to indicate how a configuration value has changed when
+ * configurations are synchronized incrementally between client and server.</p>
+ *
+ * <p>Possible values:</p>
+ * <ul>
+ * <li>{@code ADDED} - A new configuration key-value pair was added</li>
+ * <li>{@code MODIFIED} - An existing configuration value was updated</li>
+ * <li>{@code DELETED} - An existing configuration key-value pair was removed</li>
+ * </ul>
+ *
* @author jason
+ * @since 2.0.0
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * @author jason | |
| */ | |
| /** | |
| * Represents the type of change that occurred to a configuration item during | |
| * incremental synchronization. | |
| * | |
| * <p>This enum is used to indicate how a configuration value has changed when | |
| * configurations are synchronized incrementally between client and server.</p> | |
| * | |
| * <p>Possible values:</p> | |
| * <ul> | |
| * <li>{@code ADDED} - A new configuration key-value pair was added</li> | |
| * <li>{@code MODIFIED} - An existing configuration value was updated</li> | |
| * <li>{@code DELETED} - An existing configuration key-value pair was removed</li> | |
| * </ul> | |
| * | |
| * @author jason | |
| * @since 2.0.0 | |
| */ |
There was a problem hiding this comment.
It's better to add the suggested description for new classes.
There was a problem hiding this comment.
@nobodyiam, here are some example JavaDoc descriptions you might add to the newly introduced classes. Adapting the style from the suggested description for ConfigurationChangeType, you can follow a similar approach:
Below is illustrative Java code with enhanced JavaDoc. Adjust wording and details as needed.
• ConfigurationChange.java:
/**
* Represents a single configuration change which occurs during incremental synchronization.
*
* <p>This class captures three pieces of information about the change:
* <ul>
* <li>{@code key} - identifies the configuration entry that changed</li>
* <li>{@code newValue} - holds the new or updated value after the change</li>
* <li>{@code configurationChangeType} - describes how the value changed (e.g., ADDED, MODIFIED, DELETED)</li>
* </ul>
*
* <p>By providing these details, clients can more easily handle each change when
* synchronizing partial updates, rather than performing a full configuration load.</p>
*
* @since 2.0.0
*/
public class ConfigurationChange {
// ...
}• ConfigSyncType.java:
/**
* Defines supported synchronization modes for exchanging configuration definitions
* between the Apollo client and the server.
*
* <p>Possible values:</p>
* <ul>
* <li>{@code FULL_SYNC} - Retrieves or refreshes all configuration key-value pairs.</li>
* <li>{@code INCREMENTAL_SYNC} - Applies only the changes that occurred since the last synchronization.</li>
* <li>{@code UNKNOWN} - Represents an unspecified or unrecognized sync type.</li>
* </ul>
*
* <p>This enum is particularly useful in controlling how often or how intensively
* the client polls the server for updates.</p>
*
* @since 2.0.0
*/
public enum ConfigSyncType {
// ...
}• ConfigurationChangeTypeUtils.java:
/**
* Utility methods for converting strings into {@link ConfigurationChangeType} instances.
*
* <p>Used primarily within the Apollo client to parse and handle string representations
* of changes provided by the server during incremental synchronization.</p>
*
* <p>If an unknown or empty string is given, {@code ConfigurationChangeType.UNKNOWN} is returned.</p>
*
* @since 2.0.0
*/
public class ConfigurationChangeTypeUtils {
// ...
}By integrating descriptions like these, you ensure new classes are well-documented and more approachable for contributors and users of the incremental sync feature. Let me know if you need any further adjustments!
Comment on lines
47
to
58
| public static ConfigSyncType fromString(String value) { | ||
| if (StringUtils.isEmpty(value)) { | ||
| return FULLSYNC; | ||
| } | ||
| return Stream.of(ConfigSyncType.values()).filter(type -> type.value.equals(value)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new IllegalArgumentException(value + " can not map enum")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving the fromString implementation
The current implementation has several areas for improvement:
- Case-sensitive comparison could lead to runtime errors
- Stream API might be overkill for just two enum values
- Explicit null handling would be clearer than relying on StringUtils.isEmpty
public static ConfigSyncType fromString(String value) {
+ if (value == null) {
+ return FULLSYNC;
+ }
- if (StringUtils.isEmpty(value)) {
+ if (value.isEmpty()) {
return FULLSYNC;
}
- return Stream.of(ConfigSyncType.values()).filter(type -> type.value.equals(value))
- .findFirst()
- .orElseThrow(() -> new IllegalArgumentException(value + " can not map enum"));
+ for (ConfigSyncType type : values()) {
+ if (type.value.equalsIgnoreCase(value)) {
+ return type;
+ }
+ }
+ throw new IllegalArgumentException("Invalid ConfigSyncType: " + value);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static ConfigSyncType fromString(String value) { | |
| if (StringUtils.isEmpty(value)) { | |
| return FULLSYNC; | |
| } | |
| return Stream.of(ConfigSyncType.values()).filter(type -> type.value.equals(value)) | |
| .findFirst() | |
| .orElseThrow(() -> new IllegalArgumentException(value + " can not map enum")); | |
| } | |
| public static ConfigSyncType fromString(String value) { | |
| if (value == null) { | |
| return FULLSYNC; | |
| } | |
| if (value.isEmpty()) { | |
| return FULLSYNC; | |
| } | |
| for (ConfigSyncType type : values()) { | |
| if (type.value.equalsIgnoreCase(value)) { | |
| return type; | |
| } | |
| } | |
| throw new IllegalArgumentException("Invalid ConfigSyncType: " + value); | |
| } |
Comment on lines
27
to
29
| private final String key; | ||
| private String newValue; | ||
| private ConfigurationChangeType configurationChangeType; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the class immutable and adding null validation.
For better robustness and thread safety:
- Make all fields final
- Add null validation
- Consider using Optional for newValue as it might be null for DELETE operations
public class ConfigurationChange {
private final String key;
- private String newValue;
- private ConfigurationChangeType configurationChangeType;
+ private final String newValue;
+ private final ConfigurationChangeType configurationChangeType;Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
37
to
41
| public ConfigurationChange(String key, String newValue, ConfigurationChangeType configurationChangeType) { | ||
| this.key = key; | ||
| this.newValue = newValue; | ||
| this.configurationChangeType = configurationChangeType; | ||
| } |
There was a problem hiding this comment.
Add parameter validation in constructor.
The constructor should validate its parameters to prevent invalid states:
- key must not be null
- configurationChangeType must not be null
- newValue validation based on change type (can be null only for DELETE)
public ConfigurationChange(String key, String newValue, ConfigurationChangeType configurationChangeType) {
+ if (key == null) {
+ throw new IllegalArgumentException("key must not be null");
+ }
+ if (configurationChangeType == null) {
+ throw new IllegalArgumentException("configurationChangeType must not be null");
+ }
+ if (configurationChangeType != ConfigurationChangeType.DELETE && newValue == null) {
+ throw new IllegalArgumentException("newValue must not be null for non-DELETE changes");
+ }
this.key = key;
this.newValue = newValue;
this.configurationChangeType = configurationChangeType;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public ConfigurationChange(String key, String newValue, ConfigurationChangeType configurationChangeType) { | |
| this.key = key; | |
| this.newValue = newValue; | |
| this.configurationChangeType = configurationChangeType; | |
| } | |
| public ConfigurationChange(String key, String newValue, ConfigurationChangeType configurationChangeType) { | |
| if (key == null) { | |
| throw new IllegalArgumentException("key must not be null"); | |
| } | |
| if (configurationChangeType == null) { | |
| throw new IllegalArgumentException("configurationChangeType must not be null"); | |
| } | |
| if (configurationChangeType != ConfigurationChangeType.DELETE && newValue == null) { | |
| throw new IllegalArgumentException("newValue must not be null for non-DELETE changes"); | |
| } | |
| this.key = key; | |
| this.newValue = newValue; | |
| this.configurationChangeType = configurationChangeType; | |
| } |
Comment on lines
54
to
60
| public void setNewValue(String newValue) { | ||
| this.newValue = newValue; | ||
| } | ||
|
|
||
| public void setConfigurationChangeType(ConfigurationChangeType configurationChangeType) { | ||
| this.configurationChangeType = configurationChangeType; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove setters or add proper validation.
Consider one of these approaches:
- Preferred: Remove setters to make the class immutable
- Alternative: Add proper validation and documentation if mutability is required
If mutability is required, add validation:
+ /**
+ * Sets the new value for this configuration change.
+ * @param newValue the new value (can be null only for DELETE operations)
+ * @throws IllegalArgumentException if newValue is null for non-DELETE changes
+ */
public void setNewValue(String newValue) {
+ if (configurationChangeType != ConfigurationChangeType.DELETE && newValue == null) {
+ throw new IllegalArgumentException("newValue must not be null for non-DELETE changes");
+ }
this.newValue = newValue;
}
+ /**
+ * Sets the type of this configuration change.
+ * @param configurationChangeType the change type (must not be null)
+ * @throws IllegalArgumentException if configurationChangeType is null
+ */
public void setConfigurationChangeType(ConfigurationChangeType configurationChangeType) {
+ if (configurationChangeType == null) {
+ throw new IllegalArgumentException("configurationChangeType must not be null");
+ }
this.configurationChangeType = configurationChangeType;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void setNewValue(String newValue) { | |
| this.newValue = newValue; | |
| } | |
| public void setConfigurationChangeType(ConfigurationChangeType configurationChangeType) { | |
| this.configurationChangeType = configurationChangeType; | |
| } | |
| /** | |
| * Sets the new value for this configuration change. | |
| * @param newValue the new value (can be null only for DELETE operations) | |
| * @throws IllegalArgumentException if newValue is null for non-DELETE changes | |
| */ | |
| public void setNewValue(String newValue) { | |
| if (configurationChangeType != ConfigurationChangeType.DELETE && newValue == null) { | |
| throw new IllegalArgumentException("newValue must not be null for non-DELETE changes"); | |
| } | |
| this.newValue = newValue; | |
| } | |
| /** | |
| * Sets the type of this configuration change. | |
| * @param configurationChangeType the change type (must not be null) | |
| * @throws IllegalArgumentException if configurationChangeType is null | |
| */ | |
| public void setConfigurationChangeType(ConfigurationChangeType configurationChangeType) { | |
| if (configurationChangeType == null) { | |
| throw new IllegalArgumentException("configurationChangeType must not be null"); | |
| } | |
| this.configurationChangeType = configurationChangeType; | |
| } |
apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java
Outdated
Show resolved
Hide resolved
| import com.google.common.collect.Lists; | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Remove unused Guava Lists import
The com.google.common.collect.Lists import is not used anywhere in the class. The class uses List interface for the configurationChanges field but doesn't use any Guava Lists utility methods. This import can be safely removed.
🔗 Analysis chain
Verify usage of Guava Lists import
The com.google.common.collect.Lists import appears to be unused in this class.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Lists usage in this file
rg "Lists\." apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java
Length of output: 94
Script:
#!/bin/bash
# Let's first check the full content of the file to understand the context better
cat apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java
Length of output: 3129
Comment on lines
365
to
403
| private Map<String, String> mergeConfigurations(Map<String, String> configurations,List<ConfigurationChange> configurationChanges) { | ||
| Map<String, String> newConfigurations = Maps.newHashMap(configurations); | ||
| for (ConfigurationChange change : configurationChanges) { | ||
| switch (change.getConfigurationChangeType()) { | ||
| case ADDED: | ||
| case MODIFIED: | ||
| newConfigurations.put(change.getKey(), change.getNewValue()); | ||
| break; | ||
| case DELETED: | ||
| newConfigurations.remove(change.getKey()); | ||
| break; | ||
| default: | ||
| //do nothing | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return newConfigurations; | ||
| } |
There was a problem hiding this comment.
Add null check for configurationChanges in mergeConfigurations method
In the mergeConfigurations method, the configurationChanges parameter may be null, which would result in a NullPointerException during iteration. Please add a null check to handle this case.
Here's the suggested change:
private Map<String, String> mergeConfigurations(Map<String, String> configurations, List<ConfigurationChange> configurationChanges) {
Map<String, String> newConfigurations = Maps.newHashMap(configurations);
+ if (configurationChanges == null) {
+ return newConfigurations;
+ }
for (ConfigurationChange change : configurationChanges) {
switch (change.getConfigurationChangeType()) {
// existing cases
}
}
return newConfigurations;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private Map<String, String> mergeConfigurations(Map<String, String> configurations,List<ConfigurationChange> configurationChanges) { | |
| Map<String, String> newConfigurations = Maps.newHashMap(configurations); | |
| for (ConfigurationChange change : configurationChanges) { | |
| switch (change.getConfigurationChangeType()) { | |
| case ADDED: | |
| case MODIFIED: | |
| newConfigurations.put(change.getKey(), change.getNewValue()); | |
| break; | |
| case DELETED: | |
| newConfigurations.remove(change.getKey()); | |
| break; | |
| default: | |
| //do nothing | |
| break; | |
| } | |
| } | |
| return newConfigurations; | |
| } | |
| private Map<String, String> mergeConfigurations(Map<String, String> configurations,List<ConfigurationChange> configurationChanges) { | |
| Map<String, String> newConfigurations = Maps.newHashMap(configurations); | |
| if (configurationChanges == null) { | |
| return newConfigurations; | |
| } | |
| for (ConfigurationChange change : configurationChanges) { | |
| switch (change.getConfigurationChangeType()) { | |
| case ADDED: | |
| case MODIFIED: | |
| newConfigurations.put(change.getKey(), change.getNewValue()); | |
| break; | |
| case DELETED: | |
| newConfigurations.remove(change.getKey()); | |
| break; | |
| default: | |
| //do nothing | |
| break; | |
| } | |
| } | |
| return newConfigurations; | |
| } |
Comment on lines
253
to
267
| ConfigSyncType configSyncType=ConfigSyncType.fromString(result.getConfigSyncType()); | ||
| if(configSyncType!=null&&configSyncType.equals(ConfigSyncType.INCREMENTALSYNC)){ | ||
| result.setConfigurations(mergeConfigurations(m_configCache.get().getConfigurations(),result.getConfigurationChanges())); | ||
| } |
There was a problem hiding this comment.
Add null checks to prevent potential NullPointerExceptions
The method m_configCache.get() in line 255 may return null, leading to a NullPointerException when calling getConfigurations(). Similarly, result.getConfigurationChanges() might also be null. Please add null checks to ensure robustness.
Consider applying the following changes:
if (configSyncType != null && configSyncType.equals(ConfigSyncType.INCREMENTALSYNC)) {
+ if (m_configCache.get() != null && result.getConfigurationChanges() != null) {
result.setConfigurations(mergeConfigurations(m_configCache.get().getConfigurations(), result.getConfigurationChanges()));
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ConfigSyncType configSyncType=ConfigSyncType.fromString(result.getConfigSyncType()); | |
| if(configSyncType!=null&&configSyncType.equals(ConfigSyncType.INCREMENTALSYNC)){ | |
| result.setConfigurations(mergeConfigurations(m_configCache.get().getConfigurations(),result.getConfigurationChanges())); | |
| } | |
| ConfigSyncType configSyncType=ConfigSyncType.fromString(result.getConfigSyncType()); | |
| if(configSyncType!=null&&configSyncType.equals(ConfigSyncType.INCREMENTALSYNC)){ | |
| if (m_configCache.get() != null && result.getConfigurationChanges() != null) { | |
| result.setConfigurations(mergeConfigurations(m_configCache.get().getConfigurations(),result.getConfigurationChanges())); | |
| } | |
| } |
5 tasks
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java(4 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java(4 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java(1 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java
🔇 Additional comments (3)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (3)
24-26: LGTM: Import statements are appropriate
The new imports for ConfigurationChange and ConfigSyncType are correctly added and necessary for the incremental sync functionality.
250-256:
Add comprehensive null checks and improve error handling
The current implementation has several potential issues:
- Multiple nested null checks make the code hard to read
- Missing error handling for invalid sync type
- Potential NPE when accessing configurations
Consider refactoring the code as follows:
- if(result!=null){
- ConfigSyncType configSyncType=ConfigSyncType.fromString(result.getConfigSyncType());
- if(configSyncType!=null&&configSyncType.equals(ConfigSyncType.INCREMENTALSYNC)){
- ApolloConfig apolloConfig = m_configCache.get()==null?new ApolloConfig():m_configCache.get();
- result.setConfigurations(mergeConfigurations(apolloConfig.getConfigurations(),result.getConfigurationChanges()));
- }
- }
+ if (result == null) {
+ return null;
+ }
+
+ ConfigSyncType configSyncType = ConfigSyncType.fromString(result.getConfigSyncType());
+ if (configSyncType != ConfigSyncType.INCREMENTALSYNC) {
+ return result;
+ }
+
+ ApolloConfig currentConfig = m_configCache.get();
+ Map<String, String> baseConfigurations = currentConfig != null ?
+ currentConfig.getConfigurations() : Collections.emptyMap();
+
+ result.setConfigurations(mergeConfigurations(
+ baseConfigurations,
+ result.getConfigurationChanges()
+ ));
+
+ return result;Likely invalid or redundant comment.
364-391: Verify usage and impact of incremental sync changes
Let's verify the usage of RemoteConfigRepository and potential impact of these changes.
✅ Verification successful
Let me gather more information about the specific usage patterns and test coverage.
Let me check one more aspect to understand the test coverage better.
Based on the gathered information, I can now provide the final response:
Incremental sync changes are properly integrated and tested
The changes to mergeConfigurations method in RemoteConfigRepository are well-contained within the Apollo client's architecture:
- The class is used primarily by
RemoteConfigLongPollServicefor handling configuration updates - Implementation is properly integrated with the
ConfigSyncTypeenum for incremental sync support - Changes are covered by
RemoteConfigRepositoryTestwith appropriate test cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of RemoteConfigRepository and related classes
# Check for RemoteConfigRepository usage
echo "Checking RemoteConfigRepository usage..."
rg -l "RemoteConfigRepository" --type java
# Check for ConfigSyncType usage
echo "Checking ConfigSyncType usage..."
rg -l "ConfigSyncType" --type java
# Check for potential test coverage
echo "Checking test coverage..."
fd -e java -e kt --full-path ".*test.*" | rg -l "RemoteConfigRepository"
Length of output: 1320
Script:
#!/bin/bash
# Check how RemoteConfigRepository is used in DefaultConfigFactory
echo "Checking DefaultConfigFactory implementation..."
ast-grep --pattern 'class DefaultConfigFactory {
$$$
createConfigRepository($$$) {
$$$
}
$$$
}'
# Check test coverage for the mergeConfigurations method
echo "Checking test coverage for mergeConfigurations..."
rg "mergeConfigurations" apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java -A 5
# Check how RemoteConfigRepository is used in RemoteConfigLongPollService
echo "Checking RemoteConfigLongPollService usage..."
rg "RemoteConfigRepository" apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java -A 3
Length of output: 2087
Script:
#!/bin/bash
# Check test implementation for mergeConfigurations
echo "Checking RemoteConfigRepositoryTest implementation..."
ast-grep --pattern 'class RemoteConfigRepositoryTest {
$$$
test$_($$$) {
$$$
mergeConfigurations($$$)
$$$
}
$$$
}'
# Check for any error handling or sync type related tests
echo "Checking sync type related tests..."
rg "ConfigSyncType" apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java -B 2 -A 5
Length of output: 1280
Comment on lines
365
to
403
| private Map<String, String> mergeConfigurations(Map<String, String> configurations,List<ConfigurationChange> configurationChanges) { | ||
| Map<String, String> newConfigurations = new HashMap<>(); | ||
| if(configurations!=null){ | ||
| Maps.newHashMap(configurations); | ||
| } | ||
|
|
||
| if (configurationChanges == null) { | ||
| return newConfigurations; | ||
| } | ||
|
|
||
| for (ConfigurationChange change : configurationChanges) { | ||
| switch (change.getConfigurationChangeType()) { | ||
| case ADDED: | ||
| case MODIFIED: | ||
| newConfigurations.put(change.getKey(), change.getNewValue()); | ||
| break; | ||
| case DELETED: | ||
| newConfigurations.remove(change.getKey()); | ||
| break; | ||
| default: | ||
| //do nothing | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return newConfigurations; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix configuration merge logic and improve robustness
The current implementation has several issues:
- Bug in line 368:
Maps.newHashMap(configurations)result is not assigned - Missing validation for change object attributes
- Switch case structure could be simplified
Apply the following fixes:
private Map<String, String> mergeConfigurations(Map<String, String> configurations, List<ConfigurationChange> configurationChanges) {
- Map<String, String> newConfigurations = new HashMap<>();
- if(configurations!=null){
- Maps.newHashMap(configurations);
- }
+ Map<String, String> newConfigurations = configurations != null ?
+ new HashMap<>(configurations) : new HashMap<>();
if (configurationChanges == null) {
return newConfigurations;
}
for (ConfigurationChange change : configurationChanges) {
+ if (change == null || change.getKey() == null) {
+ continue;
+ }
+
switch (change.getConfigurationChangeType()) {
case ADDED:
case MODIFIED:
- newConfigurations.put(change.getKey(), change.getNewValue());
+ String newValue = change.getNewValue();
+ if (newValue != null) {
+ newConfigurations.put(change.getKey(), newValue);
+ }
break;
case DELETED:
newConfigurations.remove(change.getKey());
break;
- default:
- //do nothing
- break;
}
}
return newConfigurations;
}Additionally, consider adding logging for skipped or invalid changes to help with debugging.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private Map<String, String> mergeConfigurations(Map<String, String> configurations,List<ConfigurationChange> configurationChanges) { | |
| Map<String, String> newConfigurations = new HashMap<>(); | |
| if(configurations!=null){ | |
| Maps.newHashMap(configurations); | |
| } | |
| if (configurationChanges == null) { | |
| return newConfigurations; | |
| } | |
| for (ConfigurationChange change : configurationChanges) { | |
| switch (change.getConfigurationChangeType()) { | |
| case ADDED: | |
| case MODIFIED: | |
| newConfigurations.put(change.getKey(), change.getNewValue()); | |
| break; | |
| case DELETED: | |
| newConfigurations.remove(change.getKey()); | |
| break; | |
| default: | |
| //do nothing | |
| break; | |
| } | |
| } | |
| return newConfigurations; | |
| } | |
| private Map<String, String> mergeConfigurations(Map<String, String> configurations,List<ConfigurationChange> configurationChanges) { | |
| Map<String, String> newConfigurations = configurations != null ? | |
| new HashMap<>(configurations) : new HashMap<>(); | |
| if (configurationChanges == null) { | |
| return newConfigurations; | |
| } | |
| for (ConfigurationChange change : configurationChanges) { | |
| if (change == null || change.getKey() == null) { | |
| continue; | |
| } | |
| switch (change.getConfigurationChangeType()) { | |
| case ADDED: | |
| case MODIFIED: | |
| String newValue = change.getNewValue(); | |
| if (newValue != null) { | |
| newConfigurations.put(change.getKey(), newValue); | |
| } | |
| break; | |
| case DELETED: | |
| newConfigurations.remove(change.getKey()); | |
| break; | |
| } | |
| } | |
| return newConfigurations; | |
| } |
jackie-coming
force-pushed
the
feautre/IncrementalSync
branch
from
ebb31a8 to
f654cb3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (1)
250-267: Simplify the nested conditional logicThe current implementation has deeply nested conditions that make the code harder to read. Consider restructuring for better readability:
- if(result!=null){ - ConfigSyncType configSyncType=ConfigSyncType.fromString(result.getConfigSyncType()); - if(configSyncType!=null&&configSyncType.equals(ConfigSyncType.INCREMENTALSYNC)){ - Map<String, String> previousConfigurations=null; - ApolloConfig previousConfig = m_configCache.get(); - if(previousConfig!=null){ - previousConfigurations=previousConfig.getConfigurations(); - } - result.setConfigurations(mergeConfigurations(previousConfigurations,result.getConfigurationChanges())); - } - } + if (result == null) { + return null; + } + + ConfigSyncType configSyncType = ConfigSyncType.fromString(result.getConfigSyncType()); + if (configSyncType != ConfigSyncType.INCREMENTALSYNC) { + return result; + } + + ApolloConfig previousConfig = m_configCache.get(); + Map<String, String> previousConfigurations = + previousConfig != null ? previousConfig.getConfigurations() : null; + + result.setConfigurations( + mergeConfigurations(previousConfigurations, result.getConfigurationChanges()) + ); + + return result;apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java (2)
156-197: Consider enhancing test coverage for incremental sync.While the test covers basic functionality, consider adding:
- Verification of intermediate state after initial sync
- Assertion for
ConfigSyncTypevalue- Error cases (e.g., invalid change type)
- Edge cases (e.g., modifying non-existent key)
@Test public void testLoadConfigWithIncrementalSync() throws Exception { // ... existing setup ... remoteConfigRepository.sync(); + + // Verify initial state + Properties initialConfig = remoteConfigRepository.getConfig(); + assertEquals(2, initialConfig.size()); + assertEquals(someValue, initialConfig.getProperty(someKey)); + assertEquals(someValue1, initialConfig.getProperty(someKey1)); + + // Verify config sync type + ApolloConfig config = someResponse.getBody(); + assertEquals(ConfigSyncType.INCREMENTALSYNC.getValue(), config.getConfigSyncType()); // ... rest of the test ... } + +@Test(expected = IllegalArgumentException.class) +public void testLoadConfigWithInvalidChangeType() throws Exception { + // Add test for invalid change type +}
199-222: Enhance merge configurations test coverage.Consider adding:
- Verification that the original configuration map remains unmodified (immutability test)
- Test for concurrent modifications
- Test for multiple changes to the same key
@Test public void testMergeConfigurations() throws Exception { // ... existing setup ... + Map<String, String> originalConfig = new HashMap<>(previousConfigurations); Map<String, String> result = remoteConfigRepository.mergeConfigurations(previousConfigurations, configurationChanges); // ... existing assertions ... + + // Verify original config was not modified + assertEquals(originalConfig, previousConfigurations); } + +@Test +public void testMergeConfigurationsWithMultipleChangesToSameKey() throws Exception { + // Add test for multiple changes to same key +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java(4 hunks)apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java(4 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java(4 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java(1 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java(1 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigurationChangeType.java(1 hunks)pom.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigurationChangeType.java
- pom.xml
🔇 Additional comments (4)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (3)
24-26: LGTM: Import statements are appropriate
The new imports for ConfigurationChange and ConfigSyncType are correctly added and necessary for implementing the incremental configuration synchronization feature.
255-263:
Add null checks to prevent potential NPEs
The method chain m_configCache.get().getConfigurations() could throw NullPointerException. Similarly, result.getConfigurationChanges() might be null.
376-403: 🛠️ Refactor suggestion
Add input validation for configuration changes
While the method handles null checks, it should validate individual change objects and their properties before processing:
public Map<String, String> mergeConfigurations(Map<String, String> previousConfigurations,List<ConfigurationChange> configurationChanges) {
Map<String, String> newConfigurations = new HashMap<>();
if(previousConfigurations!=null){
newConfigurations=Maps.newHashMap(previousConfigurations);
}
if (configurationChanges == null) {
return newConfigurations;
}
for (ConfigurationChange change : configurationChanges) {
+ // Validate change object and its properties
+ if (change == null || change.getKey() == null ||
+ change.getConfigurationChangeType() == null) {
+ logger.warn("Invalid configuration change detected, skipping: {}", change);
+ continue;
+ }
+
switch (change.getConfigurationChangeType()) {
case ADDED:
case MODIFIED:
+ if (change.getNewValue() == null) {
+ logger.warn("Null value for key: {}, skipping", change.getKey());
+ continue;
+ }
newConfigurations.put(change.getKey(), change.getNewValue());
break;
case DELETED:
newConfigurations.remove(change.getKey());
break;
default:
- //do nothing
+ logger.warn("Unknown change type: {}", change.getConfigurationChangeType());
break;
}
}
return newConfigurations;
}This change adds:
- Validation for null change objects and their required properties
- Validation for null values in ADDED/MODIFIED cases
- Logging for better debugging and monitoring
Likely invalid or redundant comment.
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java (1)
34-36: LGTM!
The new imports are correctly organized and necessary for the incremental sync functionality.
Comment on lines
223
to
240
| @Test | ||
| public void testMergeConfigurationWithPreviousConfigurationsIsNULL() throws Exception { | ||
| String key1 = "key1"; | ||
| String value1 = "value1"; | ||
|
|
||
| String key3 = "key3"; | ||
| String value3 = "value3"; | ||
|
|
||
| Map<String, String> previousConfigurations = ImmutableMap.of(key1, value1,key3,value3); | ||
|
|
||
|
|
||
| RemoteConfigRepository remoteConfigRepository = new RemoteConfigRepository(someNamespace); | ||
| Map<String, String> result=remoteConfigRepository.mergeConfigurations(previousConfigurations, null); | ||
|
|
||
| assertEquals(2, result.size()); | ||
| assertEquals(value1, result.get(key1)); | ||
| assertEquals(value3, result.get(key3)); | ||
| } |
There was a problem hiding this comment.
Fix test method name and remove unused setup.
The method name suggests testing with null previous configurations, but it actually tests with null changes. Also, there are unused variables in the setup.
-@Test
-public void testMergeConfigurationWithPreviousConfigurationsIsNULL() throws Exception {
+@Test
+public void testMergeConfigurationWithChangesIsNull() throws Exception {
String key1 = "key1";
String value1 = "value1";
-
- String key3 = "key3";
- String value3 = "value3";
-
- Map<String, String> previousConfigurations = ImmutableMap.of(key1, value1,key3,value3);
+ Map<String, String> previousConfigurations = ImmutableMap.of(key1, value1);
RemoteConfigRepository remoteConfigRepository = new RemoteConfigRepository(someNamespace);
Map<String, String> result=remoteConfigRepository.mergeConfigurations(previousConfigurations, null);
- assertEquals(2, result.size());
+ assertEquals(1, result.size());
assertEquals(value1, result.get(key1));
- assertEquals(value3, result.get(key3));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Test | |
| public void testMergeConfigurationWithPreviousConfigurationsIsNULL() throws Exception { | |
| String key1 = "key1"; | |
| String value1 = "value1"; | |
| String key3 = "key3"; | |
| String value3 = "value3"; | |
| Map<String, String> previousConfigurations = ImmutableMap.of(key1, value1,key3,value3); | |
| RemoteConfigRepository remoteConfigRepository = new RemoteConfigRepository(someNamespace); | |
| Map<String, String> result=remoteConfigRepository.mergeConfigurations(previousConfigurations, null); | |
| assertEquals(2, result.size()); | |
| assertEquals(value1, result.get(key1)); | |
| assertEquals(value3, result.get(key3)); | |
| } | |
| @Test | |
| public void testMergeConfigurationWithChangesIsNull() throws Exception { | |
| String key1 = "key1"; | |
| String value1 = "value1"; | |
| Map<String, String> previousConfigurations = ImmutableMap.of(key1, value1); | |
| RemoteConfigRepository remoteConfigRepository = new RemoteConfigRepository(someNamespace); | |
| Map<String, String> result=remoteConfigRepository.mergeConfigurations(previousConfigurations, null); | |
| assertEquals(1, result.size()); | |
| assertEquals(value1, result.get(key1)); | |
| } |
Comment on lines
487
to
497
| private ApolloConfig assembleApolloConfigWithIncrementalSync(List<ConfigurationChange> configurationChanges) { | ||
| String someAppId = "appId"; | ||
| String someClusterName = "cluster"; | ||
| String someReleaseKey = "1"; | ||
| ApolloConfig apolloConfig = | ||
| new ApolloConfig(someAppId, someClusterName, someNamespace, someReleaseKey); | ||
|
|
||
| apolloConfig.setConfigSyncType(ConfigSyncType.INCREMENTALSYNC.getValue()); | ||
| apolloConfig.setConfigurationChanges(configurationChanges); | ||
| return apolloConfig; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in helper methods.
Consider refactoring to reduce duplication between assembleApolloConfig and assembleApolloConfigWithIncrementalSync methods.
-private ApolloConfig assembleApolloConfigWithIncrementalSync(List<ConfigurationChange> configurationChanges) {
- String someAppId = "appId";
- String someClusterName = "cluster";
- String someReleaseKey = "1";
- ApolloConfig apolloConfig =
- new ApolloConfig(someAppId, someClusterName, someNamespace, someReleaseKey);
-
- apolloConfig.setConfigSyncType(ConfigSyncType.INCREMENTALSYNC.getValue());
- apolloConfig.setConfigurationChanges(configurationChanges);
- return apolloConfig;
+private ApolloConfig assembleApolloConfigWithIncrementalSync(List<ConfigurationChange> configurationChanges) {
+ if (configurationChanges == null) {
+ throw new IllegalArgumentException("Configuration changes cannot be null");
+ }
+ ApolloConfig apolloConfig = assembleApolloConfig(Collections.emptyMap());
+ apolloConfig.setConfigSyncType(ConfigSyncType.INCREMENTALSYNC.getValue());
+ apolloConfig.setConfigurationChanges(configurationChanges);
+ return apolloConfig;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private ApolloConfig assembleApolloConfigWithIncrementalSync(List<ConfigurationChange> configurationChanges) { | |
| String someAppId = "appId"; | |
| String someClusterName = "cluster"; | |
| String someReleaseKey = "1"; | |
| ApolloConfig apolloConfig = | |
| new ApolloConfig(someAppId, someClusterName, someNamespace, someReleaseKey); | |
| apolloConfig.setConfigSyncType(ConfigSyncType.INCREMENTALSYNC.getValue()); | |
| apolloConfig.setConfigurationChanges(configurationChanges); | |
| return apolloConfig; | |
| } | |
| private ApolloConfig assembleApolloConfigWithIncrementalSync(List<ConfigurationChange> configurationChanges) { | |
| if (configurationChanges == null) { | |
| throw new IllegalArgumentException("Configuration changes cannot be null"); | |
| } | |
| ApolloConfig apolloConfig = assembleApolloConfig(Collections.emptyMap()); | |
| apolloConfig.setConfigSyncType(ConfigSyncType.INCREMENTALSYNC.getValue()); | |
| apolloConfig.setConfigurationChanges(configurationChanges); | |
| return apolloConfig; | |
| } |
Comment on lines
242
to
264
| @Test | ||
| public void testMergeConfigurationWithChangesIsNULL() throws Exception { | ||
| String key1 = "key1"; | ||
| String value1 = "value1"; | ||
| String anotherValue1 = "anotherValue1"; | ||
|
|
||
| String key3 = "key3"; | ||
| String value3 = "value3"; | ||
|
|
||
| List<ConfigurationChange> configurationChanges=new ArrayList<>(); | ||
| configurationChanges.add(new ConfigurationChange(key1, anotherValue1, ConfigurationChangeType.MODIFIED)); | ||
| String key2 = "key2"; | ||
| String value2 = "value2"; | ||
| configurationChanges.add(new ConfigurationChange(key2, value2, ConfigurationChangeType.ADDED)); | ||
| configurationChanges.add(new ConfigurationChange(key3, null, ConfigurationChangeType.DELETED)); | ||
|
|
||
| RemoteConfigRepository remoteConfigRepository = new RemoteConfigRepository(someNamespace); | ||
| Map<String, String> result=remoteConfigRepository.mergeConfigurations(null, configurationChanges); | ||
|
|
||
| assertEquals(2, result.size()); | ||
| assertEquals(anotherValue1, result.get(key1)); | ||
| assertEquals(value2, result.get(key2)); | ||
| } |
There was a problem hiding this comment.
Fix test method name and simplify test setup.
The method name suggests testing with null changes, but it actually tests with null previous configurations. Also, the test setup can be simplified.
-@Test
-public void testMergeConfigurationWithChangesIsNULL() throws Exception {
+@Test
+public void testMergeConfigurationWithPreviousConfigurationsIsNull() throws Exception {
String key1 = "key1";
- String value1 = "value1";
String anotherValue1 = "anotherValue1";
-
- String key3 = "key3";
- String value3 = "value3";
-
List<ConfigurationChange> configurationChanges=new ArrayList<>();
configurationChanges.add(new ConfigurationChange(key1, anotherValue1, ConfigurationChangeType.MODIFIED));
String key2 = "key2";
String value2 = "value2";
configurationChanges.add(new ConfigurationChange(key2, value2, ConfigurationChangeType.ADDED));
- configurationChanges.add(new ConfigurationChange(key3, null, ConfigurationChangeType.DELETED));
RemoteConfigRepository remoteConfigRepository = new RemoteConfigRepository(someNamespace);
Map<String, String> result=remoteConfigRepository.mergeConfigurations(null, configurationChanges);
assertEquals(2, result.size());
assertEquals(anotherValue1, result.get(key1));
assertEquals(value2, result.get(key2));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Test | |
| public void testMergeConfigurationWithChangesIsNULL() throws Exception { | |
| String key1 = "key1"; | |
| String value1 = "value1"; | |
| String anotherValue1 = "anotherValue1"; | |
| String key3 = "key3"; | |
| String value3 = "value3"; | |
| List<ConfigurationChange> configurationChanges=new ArrayList<>(); | |
| configurationChanges.add(new ConfigurationChange(key1, anotherValue1, ConfigurationChangeType.MODIFIED)); | |
| String key2 = "key2"; | |
| String value2 = "value2"; | |
| configurationChanges.add(new ConfigurationChange(key2, value2, ConfigurationChangeType.ADDED)); | |
| configurationChanges.add(new ConfigurationChange(key3, null, ConfigurationChangeType.DELETED)); | |
| RemoteConfigRepository remoteConfigRepository = new RemoteConfigRepository(someNamespace); | |
| Map<String, String> result=remoteConfigRepository.mergeConfigurations(null, configurationChanges); | |
| assertEquals(2, result.size()); | |
| assertEquals(anotherValue1, result.get(key1)); | |
| assertEquals(value2, result.get(key2)); | |
| } | |
| @Test | |
| public void testMergeConfigurationWithPreviousConfigurationsIsNull() throws Exception { | |
| String key1 = "key1"; | |
| String anotherValue1 = "anotherValue1"; | |
| List<ConfigurationChange> configurationChanges=new ArrayList<>(); | |
| configurationChanges.add(new ConfigurationChange(key1, anotherValue1, ConfigurationChangeType.MODIFIED)); | |
| String key2 = "key2"; | |
| String value2 = "value2"; | |
| configurationChanges.add(new ConfigurationChange(key2, value2, ConfigurationChangeType.ADDED)); | |
| RemoteConfigRepository remoteConfigRepository = new RemoteConfigRepository(someNamespace); | |
| Map<String, String> result=remoteConfigRepository.mergeConfigurations(null, configurationChanges); | |
| assertEquals(2, result.size()); | |
| assertEquals(anotherValue1, result.get(key1)); | |
| assertEquals(value2, result.get(key2)); | |
| } |
nobodyiam
requested changes
Dec 1, 2024
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
Outdated
Show resolved
Hide resolved
| if(result!=null){ | ||
| ConfigSyncType configSyncType=ConfigSyncType.fromString(result.getConfigSyncType()); | ||
|
|
||
| if(configSyncType!=null&&configSyncType.equals(ConfigSyncType.INCREMENTALSYNC)){ |
There was a problem hiding this comment.
According to the fromString logic, configSyncType cannot be null, and we can use == to compare enums.
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
Outdated
Show resolved
Hide resolved
| @@ -354,4 +372,33 @@ private List<ServiceDTO> getConfigServices() { | |||
|
|
|||
| return services; | |||
| } | |||
|
|
|||
| public Map<String, String> mergeConfigurations(Map<String, String> previousConfigurations,List<ConfigurationChange> configurationChanges) { | |||
There was a problem hiding this comment.
Why is the method public? Would package-private be a better choice?
Suggested change
| public Map<String, String> mergeConfigurations(Map<String, String> previousConfigurations,List<ConfigurationChange> configurationChanges) { | |
| Map<String, String> mergeConfigurations(Map<String, String> previousConfigurations,List<ConfigurationChange> configurationChanges) { |
apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java
Show resolved
Hide resolved
apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java
Outdated
Show resolved
Hide resolved
apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java
Show resolved
Hide resolved
…Sync # Conflicts: # pom.xml
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigurationChangeType.java (1)
22-24: Enhance enum documentation for better API clarity.As this enum is part of the public API and crucial for the incremental configuration synchronization feature, please enhance the documentation to include:
- Purpose and usage of the enum
- Description of each constant
- Relationship with incremental sync feature
- API version information
/** + * Represents the type of change that occurred to a configuration item during + * incremental synchronization. + * + * <p>This enum is used to indicate how a configuration value has changed when + * configurations are synchronized incrementally between client and server.</p> + * + * <p>Possible values:</p> + * <ul> + * <li>{@code ADDED} - A new configuration key-value pair was added</li> + * <li>{@code MODIFIED} - An existing configuration value was updated</li> + * <li>{@code DELETED} - An existing configuration key-value pair was removed</li> + * <li>{@code UNKNOWN} - The change type could not be determined</li> + * </ul> + * * @author jason + * @since 2.0.0 */apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java (2)
157-198: Enhance test coverage for edge cases in incremental sync.The test verifies basic incremental sync functionality but could be improved by:
- Testing concurrent modifications
- Testing empty change lists
- Testing invalid change types
Consider adding these test cases:
@Test public void testLoadConfigWithEmptyIncrementalSync() { // Setup initial config // Apply empty change list // Verify config remains unchanged } @Test public void testLoadConfigWithInvalidChangeType() { // Setup initial config // Apply change with invalid type // Verify proper error handling }
224-241: Consolidate null handling test cases.The two test methods for null handling could be combined into a single parameterized test for better maintainability.
Consider refactoring to use a parameterized test:
@ParameterizedTest @MethodSource("nullHandlingTestCases") public void testMergeConfigurationWithNullParameters(Map<String, String> previousConfigurations, List<ConfigurationChange> changes, Map<String, String> expectedResult) { RemoteConfigRepository remoteConfigRepository = new RemoteConfigRepository(someAppId,someNamespace); Map<String, String> result = remoteConfigRepository.mergeConfigurations(previousConfigurations, changes); assertEquals(expectedResult, result); } private static Stream<Arguments> nullHandlingTestCases() { return Stream.of( Arguments.of(createPreviousConfig(), null, createExpectedConfigForNullChanges()), Arguments.of(null, createChanges(), createExpectedConfigForNullPrevious()) ); }Also applies to: 243-265
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java(4 hunks)apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java(4 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java(1 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigurationChangeType.java(1 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigurationChangeTypeUtils.java(1 hunks)pom.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pom.xml
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java
🔇 Additional comments (5)
apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigurationChangeType.java (1)
25-34: LGTM! Well-structured enum implementation.
The implementation is clean and follows best practices:
- Clear, distinct enum values
- Proper validation in
fromStringmethod using Preconditions - Good separation of concerns with
ConfigurationChangeTypeUtils
apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigurationChangeTypeUtils.java (1)
28-49: LGTM! Robust utility implementation.
The implementation is well-designed with:
- Proper null/blank input handling
- Clean string transformation (trim and uppercase)
- Graceful error handling returning UNKNOWN for invalid values
- Clear and complete documentation
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (1)
384-412:
Fix configuration merge logic and improve robustness.
The current implementation has several issues:
- Bug in line 389:
Maps.newHashMap(configurations)result is not assigned - Missing validation for change object attributes
- Switch case structure could be simplified
- Map<String, String> mergeConfigurations(Map<String, String> previousConfigurations,
- List<ConfigurationChange> configurationChanges) {
- Map<String, String> newConfigurations = new HashMap<>();
- if(previousConfigurations!=null){
- newConfigurations=Maps.newHashMap(previousConfigurations);
- }
+ Map<String, String> mergeConfigurations(Map<String, String> previousConfigurations,
+ List<ConfigurationChange> configurationChanges) {
+ Map<String, String> newConfigurations = previousConfigurations != null ?
+ new HashMap<>(previousConfigurations) : new HashMap<>();
if (configurationChanges == null) {
return newConfigurations;
}
for (ConfigurationChange change : configurationChanges) {
+ if (change == null || change.getKey() == null) {
+ continue;
+ }
+
switch (ConfigurationChangeType.fromString(change.getConfigurationChangeType())) {
case ADDED:
case MODIFIED:
- newConfigurations.put(change.getKey(), change.getNewValue());
+ String newValue = change.getNewValue();
+ if (newValue != null) {
+ newConfigurations.put(change.getKey(), newValue);
+ }
break;
case DELETED:
newConfigurations.remove(change.getKey());
break;
- default:
- //do nothing
- break;
}
}
return newConfigurations;
}Likely invalid or redundant comment.
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java (2)
211-215: Use enum constants instead of string literals for change types.
Similar to the previous comment, replace hardcoded strings with enum constants.
483-493: Reduce code duplication in helper methods.
The method duplicates code from assembleApolloConfig. Consider reusing the base method.
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
Outdated
Show resolved
Hide resolved
Comment on lines
179
to
183
| configurationChanges.add(new ConfigurationChange(someKey, someNewValue, "MODIFIED")); | ||
| configurationChanges.add(new ConfigurationChange(someKey1, null, "DELETED")); | ||
| String someKey2 = "someKey2"; | ||
| String someValue2 = "someValue2"; | ||
| configurationChanges.add(new ConfigurationChange(someKey2, someValue2,"ADDED")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use enum constants instead of string literals for change types.
Replace hardcoded strings with enum constants for better type safety and maintainability.
Apply this change:
-configurationChanges.add(new ConfigurationChange(someKey, someNewValue, "MODIFIED"));
-configurationChanges.add(new ConfigurationChange(someKey1, null, "DELETED"));
-configurationChanges.add(new ConfigurationChange(someKey2, someValue2,"ADDED"));
+configurationChanges.add(new ConfigurationChange(someKey, someNewValue, ConfigurationChangeType.MODIFIED.name()));
+configurationChanges.add(new ConfigurationChange(someKey1, null, ConfigurationChangeType.DELETED.name()));
+configurationChanges.add(new ConfigurationChange(someKey2, someValue2, ConfigurationChangeType.ADDED.name()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| configurationChanges.add(new ConfigurationChange(someKey, someNewValue, "MODIFIED")); | |
| configurationChanges.add(new ConfigurationChange(someKey1, null, "DELETED")); | |
| String someKey2 = "someKey2"; | |
| String someValue2 = "someValue2"; | |
| configurationChanges.add(new ConfigurationChange(someKey2, someValue2,"ADDED")); | |
| configurationChanges.add(new ConfigurationChange(someKey, someNewValue, ConfigurationChangeType.MODIFIED.name())); | |
| configurationChanges.add(new ConfigurationChange(someKey1, null, ConfigurationChangeType.DELETED.name())); | |
| String someKey2 = "someKey2"; | |
| String someValue2 = "someValue2"; | |
| configurationChanges.add(new ConfigurationChange(someKey2, someValue2, ConfigurationChangeType.ADDED.name())); |
nobodyiam
requested changes
Dec 14, 2024
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
Outdated
Show resolved
Hide resolved
apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java
Outdated
Show resolved
Hide resolved
apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java
Outdated
Show resolved
Hide resolved
apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (1)
54-55: Replace wildcard import with specific imports.Using wildcard imports makes it harder to track dependencies and can lead to naming conflicts. Consider replacing
java.util.*with specific imports for the classes you need.-import java.util.*; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Properties;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java(4 hunks)apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java(4 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java(1 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java
🔇 Additional comments (2)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (2)
259-274: 🛠️ Refactor suggestion
Simplify the incremental sync logic and fix formatting.
The current implementation has unnecessary nesting and formatting issues:
- Inconsistent spacing around operators and braces
- The outer null check is redundant as
ConfigSyncType.fromStringhandles null - The nested structure makes the code harder to read
- if(result!=null){
- ConfigSyncType configSyncType=ConfigSyncType.fromString(result.getConfigSyncType());
- if (configSyncType == ConfigSyncType.INCREMENTAL_SYNC) {
- ApolloConfig previousConfig = m_configCache.get();
- Map<String, String> previousConfigurations =
- (previousConfig != null) ? previousConfig.getConfigurations() : null;
- result.setConfigurations(mergeConfigurations(previousConfigurations,result.getConfigurationChanges()));
- }
- }
+ if (result != null && ConfigSyncType.fromString(result.getConfigSyncType()) == ConfigSyncType.INCREMENTAL_SYNC) {
+ ApolloConfig previousConfig = m_configCache.get();
+ Map<String, String> previousConfigurations = previousConfig != null ? previousConfig.getConfigurations() : null;
+ result.setConfigurations(mergeConfigurations(previousConfigurations, result.getConfigurationChanges()));
+ }Likely invalid or redundant comment.
384-412: 🛠️ Refactor suggestion
Multiple improvements needed in mergeConfigurations method.
- The method should be package-private as it's only used internally
- The initialization logic can be simplified
- The switch case structure can be improved
- Missing validation for change object attributes
- Map<String, String> mergeConfigurations(Map<String, String> previousConfigurations,
- List<ConfigurationChange> configurationChanges) {
- Map<String, String> newConfigurations = new HashMap<>();
- if(previousConfigurations!=null){
- newConfigurations=Maps.newHashMap(previousConfigurations);
- }
+ Map<String, String> mergeConfigurations(Map<String, String> previousConfigurations,
+ List<ConfigurationChange> configurationChanges) {
+ Map<String, String> newConfigurations = previousConfigurations != null ?
+ new HashMap<>(previousConfigurations) : new HashMap<>();
if (configurationChanges == null) {
return newConfigurations;
}
for (ConfigurationChange change : configurationChanges) {
+ if (change == null || change.getKey() == null) {
+ continue;
+ }
+
- switch (ConfigurationChangeType.fromString(change.getConfigurationChangeType())) {
+ ConfigurationChangeType changeType = ConfigurationChangeType.fromString(change.getConfigurationChangeType());
+ if (changeType == null) {
+ continue;
+ }
+
+ switch (changeType) {
case ADDED:
case MODIFIED:
- newConfigurations.put(change.getKey(), change.getNewValue());
+ String newValue = change.getNewValue();
+ if (newValue != null) {
+ newConfigurations.put(change.getKey(), newValue);
+ }
break;
case DELETED:
newConfigurations.remove(change.getKey());
break;
- default:
- //do nothing
- break;
}
}
return newConfigurations;
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (1)
259-274: 🛠️ Refactor suggestionSimplify the incremental sync logic
The nested structure makes the code harder to read and maintain. The null check for
configSyncTypeis redundant asConfigSyncType.fromStringhandles null values.- if (result != null) { - ConfigSyncType configSyncType = ConfigSyncType.fromString(result.getConfigSyncType()); - if (configSyncType == ConfigSyncType.INCREMENTAL_SYNC) { - ApolloConfig previousConfig = m_configCache.get(); - Map<String, String> previousConfigurations = - (previousConfig != null) ? previousConfig.getConfigurations() : null; - result.setConfigurations( - mergeConfigurations(previousConfigurations, result.getConfigurationChanges())); - } - } + if (result != null && ConfigSyncType.fromString(result.getConfigSyncType()) == ConfigSyncType.INCREMENTAL_SYNC) { + ApolloConfig previousConfig = m_configCache.get(); + Map<String, String> previousConfigurations = previousConfig != null ? previousConfig.getConfigurations() : null; + result.setConfigurations(mergeConfigurations(previousConfigurations, result.getConfigurationChanges())); + }
🧹 Nitpick comments (1)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (1)
54-55: Replace wildcard import with specific importsUsing wildcard imports can lead to naming conflicts and make it harder to track dependencies. Consider replacing
java.util.*with specific imports for the classes you need (e.g.,java.util.Map,java.util.List, etc.).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java(4 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java
🔇 Additional comments (1)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (1)
385-413: 🛠️ Refactor suggestion
Enhance null safety and simplify switch case
The method handles basic null cases but could be improved for better robustness:
- Add null checks for the change object and its key
- Add null check for newValue before putting it in the map
- Remove unnecessary default case in switch statement
Map<String, String> mergeConfigurations(Map<String, String> previousConfigurations,
List<ConfigurationChange> configurationChanges) {
Map<String, String> newConfigurations = new HashMap<>();
if (previousConfigurations != null) {
newConfigurations = Maps.newHashMap(previousConfigurations);
}
if (configurationChanges == null) {
return newConfigurations;
}
for (ConfigurationChange change : configurationChanges) {
+ if (change == null || change.getKey() == null) {
+ continue;
+ }
+
switch (ConfigurationChangeType.fromString(change.getConfigurationChangeType())) {
case ADDED:
case MODIFIED:
- newConfigurations.put(change.getKey(), change.getNewValue());
+ String newValue = change.getNewValue();
+ if (newValue != null) {
+ newConfigurations.put(change.getKey(), newValue);
+ }
break;
case DELETED:
newConfigurations.remove(change.getKey());
break;
- default:
- //do nothing
- break;
}
}
return newConfigurations;
}Likely invalid or redundant comment.
nobodyiam
reviewed
Dec 21, 2024
| * @author jason | ||
| */ | ||
| public enum ConfigSyncType { | ||
| FULL_SYNC("FullSync"), INCREMENTAL_SYNC("IncrementalSync"), UNKNOWN("UnKnown"); |
There was a problem hiding this comment.
Suggested change
| FULL_SYNC("FullSync"), INCREMENTAL_SYNC("IncrementalSync"), UNKNOWN("UnKnown"); | |
| FULL_SYNC("FullSync"), INCREMENTAL_SYNC("IncrementalSync"), UNKNOWN("Unknown"); |
apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
Outdated
Show resolved
Hide resolved
...lo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java
Outdated
Show resolved
Hide resolved
...lo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java
Outdated
Show resolved
Hide resolved
...lo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java
Outdated
Show resolved
Hide resolved
...lo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java (4)
161-203: Improve test robustness and maintainability.
- Use enum constants instead of string literals for change types
- Add test cases for edge cases (null values, empty changes)
Apply these changes for the existing test:
- configurationChanges.add(new ConfigurationChange(someKey, someNewValue, "MODIFIED")); - configurationChanges.add(new ConfigurationChange(someKey1, null, "DELETED")); - configurationChanges.add(new ConfigurationChange(someKey2, someValue2, "ADDED")); + configurationChanges.add(new ConfigurationChange(someKey, someNewValue, ConfigurationChangeType.MODIFIED.name())); + configurationChanges.add(new ConfigurationChange(someKey1, null, ConfigurationChangeType.DELETED.name())); + configurationChanges.add(new ConfigurationChange(someKey2, someValue2, ConfigurationChangeType.ADDED.name()));Consider adding these test cases:
@Test public void testLoadConfigWithIncrementalSyncEdgeCases() { // Test with null values List<ConfigurationChange> changes = new ArrayList<>(); changes.add(new ConfigurationChange(null, "value", ConfigurationChangeType.ADDED.name())); changes.add(new ConfigurationChange("key", null, ConfigurationChangeType.MODIFIED.name())); // Test with empty changes list List<ConfigurationChange> emptyChanges = new ArrayList<>(); // Add assertions }
506-517: Reduce code duplication in helper methods.The method duplicates code from
assembleApolloConfig. Consider reusing the existing method.Apply this change:
-private ApolloConfig assembleApolloConfigWithIncrementalSync(List<ConfigurationChange> configurationChanges) { - String someAppId = "appId"; - String someClusterName = "cluster"; - String someReleaseKey = "1"; - ApolloConfig apolloConfig = - new ApolloConfig(someAppId, someClusterName, someNamespace, someReleaseKey); - - apolloConfig.setConfigSyncType(ConfigSyncType.INCREMENTAL_SYNC.getValue()); - apolloConfig.setConfigurationChanges(configurationChanges); - return apolloConfig; +private ApolloConfig assembleApolloConfigWithIncrementalSync(List<ConfigurationChange> configurationChanges) { + if (configurationChanges == null) { + throw new IllegalArgumentException("Configuration changes cannot be null"); + } + ApolloConfig apolloConfig = assembleApolloConfig(Collections.emptyMap()); + apolloConfig.setConfigSyncType(ConfigSyncType.INCREMENTAL_SYNC.getValue()); + apolloConfig.setConfigurationChanges(configurationChanges); + return apolloConfig; }
232-250: Fix misleading test method name.The method name suggests testing with null previous configurations, but it actually tests with null changes.
Apply this change:
-public void testMergeConfigurationWithPreviousConfigurationsIsNULL() throws Exception { +public void testMergeConfigurationWithChangesIsNull() throws Exception {
252-276: Simplify test implementation.The test has unused variables and can be simplified.
Apply these changes:
@Test public void testMergeConfigurationWithChangesIsNULL() throws Exception { String key1 = "key1"; - String value1 = "value1"; String anotherValue1 = "anotherValue1"; - - String key3 = "key3"; - String value3 = "value3"; - List<ConfigurationChange> configurationChanges = new ArrayList<>(); configurationChanges.add(new ConfigurationChange(key1, anotherValue1, ConfigurationChangeType.MODIFIED.name())); String key2 = "key2"; String value2 = "value2"; configurationChanges.add(new ConfigurationChange(key2, value2, ConfigurationChangeType.ADDED.name())); - configurationChanges.add(new ConfigurationChange(key3, null, ConfigurationChangeType.DELETED.name())); RemoteConfigRepository remoteConfigRepository = new RemoteConfigRepository(someAppId, someNamespace); Map<String, String> result = remoteConfigRepository.mergeConfigurations(null, configurationChanges); assertEquals(2, result.size()); assertEquals(anotherValue1, result.get(key1)); assertEquals(value2, result.get(key2)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java(15 hunks)apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java(15 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java(4 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java(1 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java(1 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigurationChangeTypeUtils.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigurationChangeTypeUtils.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java
🧰 Additional context used
📓 Learnings (1)
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java (1)
Learnt from: nobodyiam
PR: apolloconfig/apollo-java#90
File: apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java:38-40
Timestamp: 2024-12-01T03:00:42.370Z
Learning: In client-server interactions, prefer using String or int types over enums for `configSyncType` to maintain forward compatibility, as new enums from the server could cause errors on the client side.
🔇 Additional comments (2)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (2)
395-423: 🛠️ Refactor suggestion
Improve method access level and null safety.
Several improvements needed:
- Method should be package-private instead of public
- Add null checks for configuration change keys
- Add null checks for new values in ADDED/MODIFIED cases
- Remove unnecessary default case
Apply these changes:
-public Map<String, String> mergeConfigurations(Map<String, String> previousConfigurations,
+Map<String, String> mergeConfigurations(Map<String, String> previousConfigurations,
List<ConfigurationChange> configurationChanges) {
Map<String, String> newConfigurations = new HashMap<>();
if (previousConfigurations != null) {
newConfigurations = Maps.newHashMap(previousConfigurations);
}
if (configurationChanges == null) {
return newConfigurations;
}
for (ConfigurationChange change : configurationChanges) {
+ if (change == null || change.getKey() == null) {
+ continue;
+ }
switch (ConfigurationChangeType.fromString(change.getConfigurationChangeType())) {
case ADDED:
case MODIFIED:
- newConfigurations.put(change.getKey(), change.getNewValue());
+ String newValue = change.getNewValue();
+ if (newValue != null) {
+ newConfigurations.put(change.getKey(), newValue);
+ }
break;
case DELETED:
newConfigurations.remove(change.getKey());
break;
- default:
- //do nothing
- break;
}
}
return newConfigurations;
}Likely invalid or redundant comment.
267-282: 🛠️ Refactor suggestion
Simplify the incremental sync logic and improve code style.
The code can be simplified and made more readable:
- Remove excessive blank lines
- Remove redundant null check for configSyncType
- Simplify nested conditions
Apply this change:
- if (result != null) {
-
- ConfigSyncType configSyncType = ConfigSyncType.fromString(result.getConfigSyncType());
-
- if (configSyncType == ConfigSyncType.INCREMENTAL_SYNC) {
-
- ApolloConfig previousConfig = m_configCache.get();
-
- Map<String, String> previousConfigurations =
- (previousConfig != null) ? previousConfig.getConfigurations() : null;
-
- result.setConfigurations(
- mergeConfigurations(previousConfigurations, result.getConfigurationChanges()));
-
- }
- }
+ if (result != null && ConfigSyncType.fromString(result.getConfigSyncType()) == ConfigSyncType.INCREMENTAL_SYNC) {
+ ApolloConfig previousConfig = m_configCache.get();
+ Map<String, String> previousConfigurations = previousConfig != null ? previousConfig.getConfigurations() : null;
+ result.setConfigurations(mergeConfigurations(previousConfigurations, result.getConfigurationChanges()));
+ }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java (3)
160-202: Enhance test coverage and use enum constants.The test should use enum constants and cover more edge cases.
- Use enum constants instead of string literals:
- configurationChanges.add(new ConfigurationChange(someKey, someNewValue, "MODIFIED")); - configurationChanges.add(new ConfigurationChange(someKey1, null, "DELETED")); - configurationChanges.add(new ConfigurationChange(someKey2, someValue2, "ADDED")); + configurationChanges.add(new ConfigurationChange(someKey, someNewValue, ConfigurationChangeType.MODIFIED.name())); + configurationChanges.add(new ConfigurationChange(someKey1, null, ConfigurationChangeType.DELETED.name())); + configurationChanges.add(new ConfigurationChange(someKey2, someValue2, ConfigurationChangeType.ADDED.name()));
- Add test cases for edge cases:
@Test public void testLoadConfigWithInvalidChangeType() { // Test with invalid change type configurationChanges.add(new ConfigurationChange(someKey, someValue, "INVALID_TYPE")); // Verify the change is ignored } @Test public void testLoadConfigWithNullChange() { // Test with null change object configurationChanges.add(null); // Verify the null change is handled gracefully }
204-275: Improve test organization and reduce duplication.The test methods have redundant setup code and could be better organized.
- Extract common setup:
private static final String KEY1 = "key1"; private static final String VALUE1 = "value1"; private static final String ANOTHER_VALUE1 = "anotherValue1"; private RemoteConfigRepository repository; private List<ConfigurationChange> changes; @Before public void setUp() { // ... existing setup ... repository = new RemoteConfigRepository(someAppId, someNamespace); changes = new ArrayList<>(); } private void addModification(String key, String newValue) { changes.add(new ConfigurationChange(key, newValue, ConfigurationChangeType.MODIFIED.name())); } private void addDeletion(String key) { changes.add(new ConfigurationChange(key, null, ConfigurationChangeType.DELETED.name())); } private void addAddition(String key, String value) { changes.add(new ConfigurationChange(key, value, ConfigurationChangeType.ADDED.name())); }
- Simplify test methods using the helpers:
@Test public void shouldMergeAllChangeTypes() { // Given Map<String, String> previous = ImmutableMap.of(KEY1, VALUE1, "key3", "value3"); // When addModification(KEY1, ANOTHER_VALUE1); addAddition("key2", "value2"); addDeletion("key3"); // Then Map<String, String> result = repository.mergeConfigurations(previous, changes); assertEquals(2, result.size()); assertEquals(ANOTHER_VALUE1, result.get(KEY1)); assertEquals("value2", result.get("key2")); }
494-505: Reduce code duplication in test helper methods.The method duplicates logic from
assembleApolloConfig.Apply this change:
private ApolloConfig assembleApolloConfigWithIncrementalSync( List<ConfigurationChange> configurationChanges) { - String someAppId = "appId"; - String someClusterName = "cluster"; - String someReleaseKey = "1"; - ApolloConfig apolloConfig = - new ApolloConfig(someAppId, someClusterName, someNamespace, someReleaseKey); + ApolloConfig apolloConfig = assembleApolloConfig(Collections.emptyMap()); apolloConfig.setConfigSyncType(ConfigSyncType.INCREMENTAL_SYNC.getValue()); apolloConfig.setConfigurationChanges(configurationChanges); return apolloConfig; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java(5 hunks)apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java(4 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java
🧰 Additional context used
📓 Learnings (1)
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java (1)
Learnt from: nobodyiam
PR: apolloconfig/apollo-java#90
File: apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java:38-40
Timestamp: 2024-12-01T03:00:42.370Z
Learning: In client-server interactions, prefer using String or int types over enums for `configSyncType` to maintain forward compatibility, as new enums from the server could cause errors on the client side.
🔇 Additional comments (3)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (3)
26-29: LGTM!
The new imports are correctly organized and necessary for the incremental sync functionality.
Also applies to: 55-55
262-273: 🛠️ Refactor suggestion
Simplify the incremental sync logic and use String comparison.
Based on the learnings, using String comparison is preferred over enum for configSyncType to maintain forward compatibility. Also, the nested structure can be simplified.
Apply this change:
- if (result != null) {
- ConfigSyncType configSyncType = ConfigSyncType.fromString(result.getConfigSyncType());
- if (configSyncType == ConfigSyncType.INCREMENTAL_SYNC) {
- ApolloConfig previousConfig = m_configCache.get();
- Map<String, String> previousConfigurations =
- (previousConfig != null) ? previousConfig.getConfigurations() : null;
- result.setConfigurations(
- mergeConfigurations(previousConfigurations, result.getConfigurationChanges()));
- }
- }
+ if (result != null && ConfigSyncType.INCREMENTAL_SYNC.getValue().equals(result.getConfigSyncType())) {
+ ApolloConfig previousConfig = m_configCache.get();
+ Map<String, String> previousConfigurations = previousConfig != null ? previousConfig.getConfigurations() : null;
+ result.setConfigurations(mergeConfigurations(previousConfigurations, result.getConfigurationChanges()));
+ }Likely invalid or redundant comment.
384-412: 🛠️ Refactor suggestion
Improve robustness of configuration merging.
The method needs additional validation and should use String comparison for change types.
Apply these changes:
Map<String, String> mergeConfigurations(Map<String, String> previousConfigurations,
List<ConfigurationChange> configurationChanges) {
Map<String, String> newConfigurations = new HashMap<>();
if (previousConfigurations != null) {
newConfigurations = Maps.newHashMap(previousConfigurations);
}
if (configurationChanges == null) {
return newConfigurations;
}
for (ConfigurationChange change : configurationChanges) {
+ if (change == null || change.getKey() == null || change.getConfigurationChangeType() == null) {
+ continue;
+ }
+
+ String changeType = change.getConfigurationChangeType();
- switch (ConfigurationChangeType.fromString(change.getConfigurationChangeType())) {
- case ADDED:
- case MODIFIED:
+ if (ConfigurationChangeType.ADDED.name().equals(changeType)
+ || ConfigurationChangeType.MODIFIED.name().equals(changeType)) {
newConfigurations.put(change.getKey(), change.getNewValue());
- break;
- case DELETED:
+ } else if (ConfigurationChangeType.DELETED.name().equals(changeType)) {
newConfigurations.remove(change.getKey());
- break;
- default:
- //do nothing
- break;
}
}
return newConfigurations;
}Likely invalid or redundant comment.
nobodyiam
reviewed
Dec 22, 2024
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
Show resolved
Hide resolved
apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java
Outdated
Show resolved
Hide resolved
apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java
Outdated
Show resolved
Hide resolved
apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java
Outdated
Show resolved
Hide resolved
apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
Show resolved
Hide resolved
apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java
Outdated
Show resolved
Hide resolved
apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java
Outdated
Show resolved
Hide resolved
apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigurationChangeType.java
Outdated
Show resolved
Hide resolved
...o-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigurationChangeTypeUtils.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Jason Song <[email protected]>
Co-authored-by: Jason Song <[email protected]>
Co-authored-by: Jason Song <[email protected]>
Co-authored-by: Jason Song <[email protected]>
Co-authored-by: Jason Song <[email protected]>
Co-authored-by: Jason Song <[email protected]>
Co-authored-by: Jason Song <[email protected]>
Co-authored-by: Jason Song <[email protected]>
Co-authored-by: Jason Song <[email protected]>
Co-authored-by: Jason Song <[email protected]>
Co-authored-by: Jason Song <[email protected]>
|
I have read the CLA Document and hereby sign the CLA." to sign the CLA |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java (2)
204-229: Consolidate similar test methods and improve test data setup.The test methods have significant duplication in setup and assertions.
Consider using a parameterized test or test helper methods to reduce duplication. Example:
@Test public void testMergeConfigurationsWithDifferentScenarios() { // Test data String key1 = "key1"; String value1 = "value1"; String newValue1 = "newValue1"; String key2 = "key2"; String value2 = "value2"; // Test cases Object[][] testCases = { // desc, previousConfig, changes, expectedSize, expectedValues {"With all changes", ImmutableMap.of(key1, value1), Arrays.asList( new ConfigurationChange(key1, newValue1, ConfigurationChangeType.MODIFIED.name()), new ConfigurationChange(key2, value2, ConfigurationChangeType.ADDED.name()) ), 2, ImmutableMap.of(key1, newValue1, key2, value2) }, {"With null previous", null, Arrays.asList( new ConfigurationChange(key1, value1, ConfigurationChangeType.ADDED.name()) ), 1, ImmutableMap.of(key1, value1) }, {"With null changes", ImmutableMap.of(key1, value1), null, 1, ImmutableMap.of(key1, value1) } }; RemoteConfigRepository repository = new RemoteConfigRepository(someAppId, someNamespace); for (Object[] testCase : testCases) { String desc = (String) testCase[0]; Map<String, String> previousConfig = (Map<String, String>) testCase[1]; List<ConfigurationChange> changes = (List<ConfigurationChange>) testCase[2]; int expectedSize = (int) testCase[3]; Map<String, String> expectedValues = (Map<String, String>) testCase[4]; Map<String, String> result = repository.mergeConfigurations(previousConfig, changes); assertEquals(desc + ": size", expectedSize, result.size()); assertEquals(desc + ": values", expectedValues, result); } }Also applies to: 231-249, 251-275
512-537: Reduce code duplication in helper methods.The helper methods share common setup code.
Consider extracting common code:
+ private ApolloConfig createBaseApolloConfig() { + return new ApolloConfig("appId", "cluster", someNamespace, "1"); + } private ApolloConfig assembleApolloConfigWithIncrementalSync( List<ConfigurationChange> configurationChanges) { - String someAppId = "appId"; - String someClusterName = "cluster"; - String someReleaseKey = "1"; - ApolloConfig apolloConfig = - new ApolloConfig(someAppId, someClusterName, someNamespace, someReleaseKey); + ApolloConfig apolloConfig = createBaseApolloConfig(); apolloConfig.setConfigSyncType(ConfigSyncType.INCREMENTAL_SYNC.getValue()); apolloConfig.setConfigurationChanges(configurationChanges); return apolloConfig; } private ApolloConfig assembleApolloConfigWithUnknownSync( List<ConfigurationChange> configurationChanges) { - String someAppId = "appId"; - String someClusterName = "cluster"; - String someReleaseKey = "1"; - ApolloConfig apolloConfig = - new ApolloConfig(someAppId, someClusterName, someNamespace, someReleaseKey); + ApolloConfig apolloConfig = createBaseApolloConfig(); apolloConfig.setConfigSyncType(ConfigSyncType.UNKNOWN.getValue()); apolloConfig.setConfigurationChanges(configurationChanges); return apolloConfig; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGES.md(1 hunks)apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java(5 hunks)apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java(4 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java(4 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java(1 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java(1 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigurationChangeType.java(1 hunks)apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigurationChangeTypeUtils.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigurationChangeTypeUtils.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigurationChangeType.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ConfigurationChange.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigSyncType.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java
🧰 Additional context used
📓 Learnings (1)
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java (1)
Learnt from: nobodyiam
PR: apolloconfig/apollo-java#90
File: apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java:38-40
Timestamp: 2024-12-01T03:00:42.370Z
Learning: In client-server interactions, prefer using String or int types over enums for `configSyncType` to maintain forward compatibility, as new enums from the server could cause errors on the client side.
🔇 Additional comments (7)
CHANGES.md (1)
14-14: LGTM!
The changelog entry follows the established format and correctly references the PR.
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (3)
26-29: LGTM!
The imports are correctly organized and necessary for the new functionality.
262-278: 🛠️ Refactor suggestion
Simplify the incremental sync logic and improve type safety.
The current implementation has several issues:
- Redundant null checks (ConfigSyncType.fromString handles null)
- Deep nesting makes the code harder to read
- String comparison for sync type is not type-safe
Apply this diff to improve the code:
- if (result != null) {
- ConfigSyncType configSyncType = ConfigSyncType.fromString(result.getConfigSyncType());
- if (configSyncType == ConfigSyncType.INCREMENTAL_SYNC) {
- ApolloConfig previousConfig = m_configCache.get();
- Map<String, String> previousConfigurations =
- (previousConfig != null) ? previousConfig.getConfigurations() : null;
- result.setConfigurations(
- mergeConfigurations(previousConfigurations, result.getConfigurationChanges()));
- } else if (configSyncType == ConfigSyncType.UNKNOWN) {
- String message = String.format(
- "Apollo Config Sync type invalid - configSyncType: %s",
- result.getConfigSyncType());
- throw new ApolloConfigException(message, exception);
- }
- }
+ ConfigSyncType configSyncType = ConfigSyncType.fromString(result.getConfigSyncType());
+ switch (configSyncType) {
+ case INCREMENTAL_SYNC:
+ ApolloConfig previousConfig = m_configCache.get();
+ Map<String, String> previousConfigurations = previousConfig != null ?
+ previousConfig.getConfigurations() : null;
+ result.setConfigurations(
+ mergeConfigurations(previousConfigurations, result.getConfigurationChanges()));
+ break;
+ case UNKNOWN:
+ throw new ApolloConfigException(
+ String.format("Apollo Config Sync type invalid - configSyncType: %s",
+ result.getConfigSyncType()), exception);
+ }Likely invalid or redundant comment.
389-417: 🛠️ Refactor suggestion
Improve the mergeConfigurations method implementation.
The current implementation has several areas for improvement:
- Method visibility should be package-private
- String comparison for change type is not type-safe
- Switch statement can be simplified
Apply this diff to improve the code:
- public Map<String, String> mergeConfigurations(Map<String, String> previousConfigurations,
+ Map<String, String> mergeConfigurations(Map<String, String> previousConfigurations,
List<ConfigurationChange> configurationChanges) {
- Map<String, String> newConfigurations = new HashMap<>();
-
- if (previousConfigurations != null) {
- newConfigurations = Maps.newHashMap(previousConfigurations);
- }
+ Map<String, String> newConfigurations = previousConfigurations != null ?
+ Maps.newHashMap(previousConfigurations) : new HashMap<>();
if (configurationChanges == null) {
return newConfigurations;
}
for (ConfigurationChange change : configurationChanges) {
- switch (ConfigurationChangeType.fromString(change.getConfigurationChangeType())) {
+ if (change == null || change.getKey() == null) {
+ continue;
+ }
+ ConfigurationChangeType changeType = ConfigurationChangeType.fromString(
+ change.getConfigurationChangeType());
+ switch (changeType) {
case ADDED:
case MODIFIED:
- newConfigurations.put(change.getKey(), change.getNewValue());
+ String newValue = change.getNewValue();
+ if (newValue != null) {
+ newConfigurations.put(change.getKey(), newValue);
+ }
break;
case DELETED:
newConfigurations.remove(change.getKey());
break;
- default:
- //do nothing
- break;
}
}
return newConfigurations;
}Likely invalid or redundant comment.
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java (3)
37-39: LGTM!
The imports are correctly organized and necessary for the test cases.
277-293: LGTM!
The test correctly verifies the exception handling for unknown sync type.
160-202: 🛠️ Refactor suggestion
Use enum constants instead of string literals for change types.
The test uses string literals for configuration change types which is not type-safe.
Apply this diff:
- configurationChanges.add(new ConfigurationChange(someKey, someNewValue, "MODIFIED"));
- configurationChanges.add(new ConfigurationChange(someKey1, null, "DELETED"));
- configurationChanges.add(new ConfigurationChange(someKey2, someValue2, "ADDED"));
+ configurationChanges.add(new ConfigurationChange(someKey, someNewValue,
+ ConfigurationChangeType.MODIFIED.name()));
+ configurationChanges.add(new ConfigurationChange(someKey1, null,
+ ConfigurationChangeType.DELETED.name()));
+ configurationChanges.add(new ConfigurationChange(someKey2, someValue2,
+ ConfigurationChangeType.ADDED.name()));Likely invalid or redundant comment.
nobodyiam
reviewed
Dec 23, 2024
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
Outdated
Show resolved
Hide resolved
nobodyiam
reviewed
Dec 23, 2024
apollo-core/src/main/java/com/ctrip/framework/apollo/core/dto/ApolloConfig.java
Outdated
Show resolved
Hide resolved
Apologies for the confusion. The correct comment is: "I have read the CLA Document and I hereby sign the CLA" |
|
I have read the CLA Document and I hereby sign the CLA |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's the purpose of this PR
XXXXX
Which issue(s) this PR fixes:
Fixes #5252
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
Tests
Documentation