solve the problem of duplicate comments and blank lines #5232
Conversation
dosubot
bot
added
the
size:L
WalkthroughThe changes primarily involve enhancing the handling of comment and blank items in the Changes
Assessment against linked issues
📜 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 (
|
|
// TODO |
|
Using the test case in #5000, there is another problem
So I removed this comment or blank line in #5232 |
.../src/main/java/com/ctrip/framework/apollo/portal/component/txtresolver/PropertyResolver.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/ctrip/framework/apollo/portal/component/txtresolver/PropertyResolver.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/ctrip/framework/apollo/portal/component/txtresolver/PropertyResolver.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/ctrip/framework/apollo/portal/component/txtresolver/PropertyResolver.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
CHANGES.md (1)
11-11: LGTM! Consider adding more detail to the changelog entry.The addition to the changelog accurately reflects the main objective of the PR and is consistent with the existing format. Well done!
To provide more context for users, consider expanding the changelog entry slightly. For example:
-* [Fix: The problem of duplicate comments and blank lines](https://github.com/apolloconfig/apollo/pull/5232) +* [Fix: Resolve issues with duplicate comments and blank lines in configuration management](https://github.com/apolloconfig/apollo/pull/5232)This gives users a clearer idea of where the fix applies without needing to click through to the PR.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/txtresolver/PropertyResolver.java (1)
200-200: Maintain consistency in string utility methodsAt line 200~,
Strings.nullToEmpty(line).trim().isEmpty()from Guava'sStringsclass is used. However, sinceStringUtilsfromcom.ctrip.framework.apollo.core.utils.StringUtilsis already imported (line 24~), consider usingStringUtilsfor consistency and to reduce external dependencies. IfStringUtilsprovides a method likeStringUtils.isEmpty(line), it can replace the current implementation for a unified approach.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ItemService.java (1)
234-235: Enhance readability by reordering the equality checkIn the condition
0 == deletedItemDto.getLineNum(), consider switching the order todeletedItemDto.getLineNum() == 0for better readability and consistency with common coding conventions.-int newLineNum = 0 == deletedItemDto.getLineNum() ? lineNum.get() : deletedItemDto.getLineNum(); +int newLineNum = deletedItemDto.getLineNum() == 0 ? lineNum.get() : deletedItemDto.getLineNum();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- CHANGES.md (1 hunks)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/txtresolver/PropertyResolver.java (9 hunks)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ItemService.java (2 hunks)
🔇 Additional comments (5)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/txtresolver/PropertyResolver.java (2)
83-88: Verify the correct mapping of comment and blank itemsThe current logic in handling comment and blank lines assumes a direct correspondence between the new items and the
baseCommentItems/baseBlankItemslists by usingremove(0). If there are additions or deletions of comments or blank lines, this might lead to mismatches or incorrect deletions.Consider reviewing and testing cases where comments or blank lines are added, removed, or modified to ensure the
resolvemethod correctly handles these scenarios. Adding unit tests for these cases will help validate the method's behavior.Also applies to: 93-98
108-108: Ensure method signature changes are reflected across the codebaseThe method
deleteCommentAndBlankItemhas been updated to acceptList<ItemDTO>parameters instead ofMap<Integer, ItemDTO>. Ensure that any calls to this method elsewhere in the codebase have been updated to match the new signature to prevent anyNoSuchMethodErrororMethodNotFoundExceptionat runtime.Run the following script to find usages of
deleteCommentAndBlankItem:#!/bin/bash # Description: Find all usages of deleteCommentAndBlankItem to verify method signature compatibility. # Search for method calls to deleteCommentAndBlankItem rg --type java --pcre2 'deleteCommentAndBlankItem\s*\('apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ItemService.java (3)
53-53: Import statement is necessaryThe import of
java.util.stream.Collectorsis required for the stream operations used in the code.
224-226: Properly filtering and mapping deleted itemsThe code correctly filters out items with empty keys and collects deleted items into a map, ensuring that only relevant items are processed.
242-242: Proper deletion of remaining itemsThe code correctly adds the remaining items in
oldKeyMapItemtochangeSetsas delete items, ensuring that obsolete entries are removed.
.../src/main/java/com/ctrip/framework/apollo/portal/component/txtresolver/PropertyResolver.java
Outdated
Show resolved
Hide resolved
| } else if (!newValue.equals(oldItem.getValue()) || lineCounter != oldItem.getLineNum()) { | ||
| changeSets.addUpdateItem(buildNormalItem(oldItem.getId(), namespaceId, newKey, newValue, oldItem.getComment(), lineCounter)); |
There was a problem hiding this comment.
Prevent potential NullPointerException when comparing string values
In the handleNormalLine method, the condition on line 180~ compares newValue and oldItem.getValue():
} else if (!newValue.equals(oldItem.getValue()) || lineCounter != oldItem.getLineNum()) {If oldItem.getValue() is null, this could result in a NullPointerException. To safely handle possible null values, use StringUtils.equals(newValue, oldItem.getValue()), which handles null comparisons gracefully.
Apply this diff to prevent the potential exception:
-} else if (!newValue.equals(oldItem.getValue()) || lineCounter != oldItem.getLineNum()) {
+} else if (!StringUtils.equals(newValue, oldItem.getValue()) || lineCounter != oldItem.getLineNum()) {| value, oldItem.getComment(), lineNum.get())); | ||
| int newLineNum = 0 == deletedItemDto.getLineNum() ? lineNum.get() : deletedItemDto.getLineNum(); | ||
| changeSets.addCreateItem(buildNormalItem(0L, namespaceId, key, value, deletedItemDto.getComment(), newLineNum)); | ||
| } else if (!oldItem.getValue().equals(value) || lineNum.get() != oldItem.getLineNum()) { |
There was a problem hiding this comment.
Use Objects.equals to prevent potential NullPointerException
If oldItem.getValue() or value could be null, using !oldItem.getValue().equals(value) may cause a NullPointerException. Consider using !Objects.equals(oldItem.getValue(), value) to safely compare values.
Ensure you have imported java.util.Objects.
+import java.util.Objects;
...
} else if (
- !oldItem.getValue().equals(value) || lineNum.get() != oldItem.getLineNum()) {
+ !Objects.equals(oldItem.getValue(), value) || lineNum.get() != oldItem.getLineNum()) {Committable suggestion was skipped due to low confidence.
| //remove comment and blank item map. | ||
| oldKeyMapItem.remove(""); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider filtering out blank keys during map creation
Instead of removing entries with blank keys after creating oldKeyMapItem, consider filtering out items with blank keys when constructing the map. This can improve code clarity and prevent unnecessary entries from being processed.
You can modify the map creation as follows:
-import java.util.HashMap;
+import java.util.HashMap;
+import java.util.stream.Collectors;
Map<String, ItemDTO> oldKeyMapItem = BeanUtils.mapByKey("key", baseItems);
-//remove comment and blank item map.
-oldKeyMapItem.remove("");
+oldKeyMapItem = oldKeyMapItem.entrySet().stream()
+ .filter(entry -> !StringUtils.isEmpty(entry.getKey()))
+ .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));Committable suggestion was skipped due to low confidence.
I mentioned this problem in my reply above, but I don't have a good solution to solve it completely. I submitted some small optimizations for this problem. With a small number of changes, the line num can be kept correct as much as possible. The code is as follows： |
…at' into feature/comment_and_blank_repeat
# Conflicts: # CHANGES.md
What's the purpose of this PR
XXXXX
Which issue(s) this PR fixes:
Fixes #5000
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
Refactor