新增 BTN Peers 活动历史记录协议 #607
Conversation
提升 BTN 版本号
WalkthroughThis pull request introduces several updates across multiple classes in the project. The Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (10)
src/main/java/com/ghostchu/peerbanhelper/btn/ping/BtnPeerHistoryPing.java (1)
14-17: LGTM with suggestions: Fields are well-defined, consider adding documentation.The fields
populateTimeandpeersare appropriately named and use correct types. The@SerializedNameannotations are properly used for JSON serialization.Suggestions for improvement:
- Consider adding JavaDoc comments to describe the purpose of each field.
- For
populateTime, consider usingInstantorZonedDateTimeinstead oflongfor better date-time handling.- For
peers, consider adding a@NonNullannotation if the list should never be null.Example:
/** * The time when this peer history was populated. */ @SerializedName("populate_time") private Instant populateTime; /** * The list of peer histories. */ @SerializedName("peers") @NonNull private List<BtnPeerHistory> peers;src/main/java/com/ghostchu/peerbanhelper/btn/ping/BtnPeerHistory.java (2)
16-39: LGTM: Fields and serialization are well-defined, with a minor suggestion.The fields cover all necessary aspects of peer history, and the use of @SerializedName annotations ensures proper JSON serialization. The naming convention for JSON fields (snake_case) is consistent and follows common practices.
Consider adding @jsonformat annotation to the Timestamp fields to ensure consistent date-time formatting during serialization/deserialization. For example:
@SerializedName("first_time_seen") @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd'T'HH:mm:ss.SSSZ") private Timestamp firstTimeSeen;This will provide explicit control over the date-time format in JSON.
41-57: LGTM: Static factory method is well-implemented, with suggestions for improvement.The
frommethod provides a clean way to create aBtnPeerHistoryinstance from aPeerRecordEntity. The use ofInfoHashUtilfor hashing the torrent identifier is a good practice for privacy and security.Consider the following improvements:
- Add null checks for the input
PeerRecordEntityand its nested objects to prevent NullPointerExceptions:public static BtnPeerHistory from(PeerRecordEntity peer) { if (peer == null || peer.getTorrent() == null) { throw new IllegalArgumentException("PeerRecordEntity or its Torrent cannot be null"); } // ... rest of the method }
- Use Optional for potentially null values:
btnPeer.setPeerFlag(Optional.ofNullable(peer.getLastFlags()).orElse(null));
- Consider using a builder pattern or constructor with parameters instead of setter methods for immutability:
return new BtnPeerHistory( peer.getAddress(), peer.getPeerId(), peer.getClientName(), InfoHashUtil.getHashedIdentifier(peer.getTorrent().getInfoHash()), peer.getTorrent().getSize(), peer.getDownloaded(), peer.getDownloadedOffset(), peer.getUploaded(), peer.getUploadedOffset(), peer.getFirstTimeSeen(), peer.getLastTimeSeen(), Optional.ofNullable(peer.getLastFlags()).orElse(null) );These changes would improve the robustness and maintainability of the code.
src/main/java/com/ghostchu/peerbanhelper/database/dao/impl/PeerRecordDao.java (1)
43-51: LGTM: New method for retrieving pending peer records with pagination.The implementation looks correct and aligns with the PR objectives. It properly filters for pending records and implements pagination.
A minor suggestion for improved readability:
Consider extracting the query condition into a separate method for better readability. For example:
private Where<PeerRecordEntity, Long> getPendingRecordsCondition(Timestamp afterThan) { return queryBuilder().where() .gt("lastSubmitAt", afterThan) .or() .isNull("lastSubmitAt"); } public Page<PeerRecordEntity> getPendingSubmitPeerRecords(Pageable pageable, Timestamp afterThan) throws SQLException { var queryBuilder = getPendingRecordsCondition(afterThan) .queryBuilder() .orderBy("lastTimeSeen", false); return queryByPaging(queryBuilder, pageable); }This separation of concerns makes the main method more concise and the query condition more reusable.
src/main/java/com/ghostchu/peerbanhelper/btn/BtnNetwork.java (1)
Line range hint
1-164: Overall implementation reviewThe changes in this file partially implement the new "submit_histories" protocol as described in the PR objectives. However, there are a few points that need attention:
- The protocol version has been correctly updated.
- A new
peerRecordDaofield has been added, but its usage is not visible in this file.- There are inconsistencies in the implementation of the "submit_histories" ability handling.
To ensure a complete and correct implementation:
- Verify the usage of
peerRecordDaoin related classes.- Correct the inconsistencies in the "submit_histories" ability handling.
- Provide implementation details of the
BtnAbilitySubmitHistoryclass, which should handle the new functionality for submitting cumulative peer activity data.- Consider adding unit tests to verify the correct behavior of the new functionality.
Could you please provide more information on how the cumulative peer activity data is being collected and processed before submission?
src/main/java/com/ghostchu/peerbanhelper/text/Lang.java (1)
Line range hint
1-385: Consider improving enum organization for better maintainabilityWhile the current structure of the
Langenum is functional, consider the following suggestions to enhance readability and maintainability:
- Group related constants together (e.g., all BTN-related constants, all downloader-related constants, etc.).
- Add comments to describe each group of related constants.
- Consider using nested enums for major categories if applicable in your Java version.
Example structure:
public enum Lang { // Error messages ERR_BUILD_NO_INFO_FILE, ERR_CANNOT_LOAD_BUILD_INFO, // ... other error constants // BTN-related messages BTN_SUBMITTING_PEERS, BTN_SUBMITTED_PEERS, BTN_SUBMITTING_HISTORIES, BTN_SUBMITTED_HISTORIES, // ... other BTN constants // Downloader-related messages DOWNLOADER_QB_LOGIN_FAILED, DOWNLOADER_QB_FAILED_REQUEST_TORRENT_LIST, // ... other downloader constants // ... other categories public String getKey() { return name(); } }This organization will make it easier to locate specific constants and maintain the enum as it grows.
src/main/java/com/ghostchu/peerbanhelper/Main.java (1)
278-278: LGTM. Consider using a constant for the BTN-Protocol version.The update to the BTN-Protocol version aligns with the PR objectives. However, to improve maintainability, consider defining the BTN-Protocol version as a constant at the class level. This would make future updates easier and reduce the risk of inconsistencies.
Here's a suggested refactor:
+ private static final String BTN_PROTOCOL_VERSION = "0.0.2"; public static String getUserAgent() { - return "PeerBanHelper/" + meta.getVersion() + " BTN-Protocol/0.0.2"; + return "PeerBanHelper/" + meta.getVersion() + " BTN-Protocol/" + BTN_PROTOCOL_VERSION; }This change would make it easier to update the BTN-Protocol version in the future and ensure consistency if the version is used elsewhere in the codebase.
src/main/resources/lang/en_us/messages.yml (2)
84-85: LGTM! Consider a minor improvement for consistency.The new entries for submitting peer history activities to the BTN network are clear and informative. They follow the existing pattern and style of the file.
For consistency with other BTN-related messages, consider changing "network" to "Network" in both messages:
-BTN_SUBMITTING_HISTORIES: "[BTN Network] Scheduled task submitting {} peers history activities to network, please wait..." +BTN_SUBMITTING_HISTORIES: "[BTN Network] Scheduled task submitting {} peers history activities to Network, please wait..." -BTN_SUBMITTED_HISTORIES: "[BTN Network] Submitted {} peers history activities, thank you for supporting the BTN network!" +BTN_SUBMITTED_HISTORIES: "[BTN Network] Submitted {} peers history activities, thank you for supporting the BTN Network!"This change would align these messages with others like "BTN_SUBMITTED_PEERS" which use "BTN Network" consistently.
Line range hint
1-1000: Consider improving overall consistency in punctuation and capitalization.While reviewing the file, I noticed some minor inconsistencies in punctuation and capitalization across various messages. To improve overall consistency and maintainability, consider:
- Standardizing the use of periods at the end of messages. Either add them to all messages or remove them from all.
- Ensuring consistent capitalization in similar phrases across different messages.
For example:
- Some messages end with periods while others don't: "PeerBanHelper v{} - by PBH-BTN Community, Made with ❤" vs "An internal server error occurred while processing the WebAPI request, please check the console logs"
- Capitalization varies in some cases: "BTN network" vs "BTN Network"
A thorough review and standardization of these aspects would enhance the overall quality and consistency of the messages file.
src/main/java/com/ghostchu/peerbanhelper/btn/ability/BtnAbilitySubmitHistory.java (1)
74-77: Handle exceptions in asynchronous operations correctlyIn the
exceptionallyblock of the asynchronous HTTP request, you log a warning but do not handle the exception further. Ensure that exceptions are properly propagated or handled to avoid silent failures.Consider modifying the
exceptionallyblock to rethrow the exception or handle it appropriately:.exceptionally(e -> { log.warn(tlUI(Lang.BTN_REQUEST_FAILS), e); setLastStatus(false, e.getClass().getName() + ": " + e.getMessage()); + // Propagate the exception if necessary + // throw new CompletionException(e); return null; });Depending on your application's needs, you might want to propagate the exception to ensure that failure scenarios are adequately addressed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- src/main/java/com/ghostchu/peerbanhelper/Main.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/btn/BtnNetwork.java (4 hunks)
- src/main/java/com/ghostchu/peerbanhelper/btn/ability/BtnAbilitySubmitHistory.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/btn/ping/BtnPeerHistory.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/btn/ping/BtnPeerHistoryPing.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/database/dao/impl/PeerRecordDao.java (2 hunks)
- src/main/java/com/ghostchu/peerbanhelper/text/Lang.java (1 hunks)
- src/main/resources/lang/en_us/messages.yml (1 hunks)
- src/main/resources/lang/messages_fallback.yml (1 hunks)
- src/main/resources/lang/zh_cn/messages.yml (1 hunks)
🧰 Additional context used
🔇 Additional comments (16)
src/main/java/com/ghostchu/peerbanhelper/btn/ping/BtnPeerHistoryPing.java (3)
1-9: LGTM: Package declaration and imports are appropriate.The package declaration and imports are well-organized and relevant to the class's functionality. Good job on following Java naming conventions and avoiding unused imports.
10-12: LGTM: Appropriate use of Lombok annotations.The Lombok annotations (@DaTa, @AllArgsConstructor, @NoArgsConstructor) are well-chosen for this data class. They effectively reduce boilerplate code by generating common methods and constructors.
13-13: LGTM: Class declaration is appropriate.The class name
BtnPeerHistoryPingis descriptive and follows Java naming conventions. The public access modifier is suitable for this class.src/main/java/com/ghostchu/peerbanhelper/btn/ping/BtnPeerHistory.java (1)
12-15: LGTM: Class structure and annotations are well-defined.The use of Lombok annotations (@DaTa, @AllArgsConstructor, @NoArgsConstructor) is appropriate for this data class. It reduces boilerplate code while providing necessary methods and constructors.
src/main/java/com/ghostchu/peerbanhelper/database/dao/impl/PeerRecordDao.java (3)
7-8: LGTM: New imports for pagination.The new imports for
PageandPageableare appropriate for implementing pagination in the new method.
Line range hint
1-51: Summary: New method aligns with PR objectives.The changes in this file successfully implement part of the new BTN Peers Activity History Record Protocol as described in the PR objectives. The new
getPendingSubmitPeerRecordsmethod enables efficient retrieval of pending peer records, which is crucial for the new protocol's functionality of submitting cumulative data lists of peers.The implementation uses pagination, which should help in handling large datasets efficiently. This aligns well with the goal of more accurately and efficiently tracking the traffic activities of peers across various clients.
Overall, these changes contribute positively to the PR's objectives without introducing any apparent issues to the existing codebase.
43-51: Verify the implementation ofqueryByPagingmethod.The new
getPendingSubmitPeerRecordsmethod relies on thequeryByPagingmethod, which is likely implemented in the superclass. To ensure correct functionality, please verify thatqueryByPagingproperly handles the query builder and pagination parameters.Run the following script to check the implementation of
queryByPaging:If the
queryByPagingmethod is not found or its implementation doesn't properly handle the query builder and pagination, consider implementing it in this class or updating the superclass implementation.src/main/java/com/ghostchu/peerbanhelper/btn/BtnNetwork.java (1)
34-34: LGTM: Protocol version updateThe update of
PBH_BTN_PROTOCOL_IMPL_VERSIONto 8 aligns with the PR objectives. This change is crucial for maintaining compatibility with the server and supporting the new "submit_histories" protocol.src/main/java/com/ghostchu/peerbanhelper/text/Lang.java (1)
78-79: LGTM: New constants align with PR objectivesThe addition of
BTN_SUBMITTING_HISTORIESandBTN_SUBMITTED_HISTORIESconstants is consistent with the PR objectives of implementing a new "submit_histories" protocol. These constants will likely be used for localization of user interface elements related to the new feature.The naming convention and placement within the enum are appropriate and follow the existing pattern.
src/main/resources/lang/zh_cn/messages.yml (2)
84-85: New messages for BTN network peer history submission look good.The newly added messages for submitting peer activity histories to the BTN network are well-formatted and consistent with the existing localization style. They appropriately use placeholders for dynamic content.
Line range hint
1-83: Overall, the localization file is well-maintained and comprehensive.The existing content in the
messages.ymlfile demonstrates consistent formatting, appropriate use of placeholders, and covers a wide range of application features. The messages provide clear and helpful information to users, including links to documentation where relevant.Some notable points:
- Consistent naming convention using uppercase and underscores for keys.
- Proper use of placeholders (e.g.,
{}) for dynamic content.- Helpful error messages and explanations for various scenarios.
- Inclusion of links to documentation or GitHub issues for complex topics.
No significant issues were found in terms of formatting, content, or potential security risks.
Also applies to: 86-290
src/main/resources/lang/messages_fallback.yml (4)
84-85: New entries for BTN network history submission added.Two new entries have been added for the BTN network history submission process:
BTN_SUBMITTING_HISTORIES: Informs the user that the system is submitting peer activity history records to the BTN network.BTN_SUBMITTED_HISTORIES: Confirms the successful submission of peer activity history records to the BTN network.These additions improve user feedback during the history submission process.
Line range hint
86-90: Improved clarity in configuration-related messages.The following configuration-related entries have been updated:
CONFIG_CHECKING: Now specifies it's a configuration upgrade utility.CONFIG_MIGRATING: Clarifies the migration process from one version to another.CONFIG_EXECUTE_MIGRATE: Indicates the execution of a configuration upgrade script.CONFIG_MIGRATE_FAILED: Provides more detailed information about migration failures.CONFIG_UPGRADED: Confirms successful configuration upgrade to a specific version.These changes enhance user understanding of the configuration update process.
Line range hint
1-474: Enhanced error messages and status notifications throughout the file.Several error messages and status notifications have been improved throughout the file. Some notable examples include:
- More descriptive messages for configuration errors and upgrades.
- Clearer notifications for BTN network operations.
- Improved feedback for IP blacklist rule updates and management.
- Better error reporting for downloader-related issues.
These enhancements provide users with more detailed and contextually relevant information, improving the overall user experience and making troubleshooting easier.
Line range hint
1-474: Overall improvements to localization and user feedback.The changes made to this localization file are positive and enhance the user experience of PeerBanHelper:
- New entries for BTN network history submission provide better user feedback.
- Updated configuration-related messages offer clearer information about the upgrade process.
- Improved error messages and status notifications throughout the file provide more detailed and relevant information to users.
These modifications contribute to better user understanding of the application's processes and potential issues, which should lead to improved usability and easier troubleshooting.
src/main/java/com/ghostchu/peerbanhelper/btn/ability/BtnAbilitySubmitHistory.java (1)
44-44: Ensure the initial delay calculation aligns with scheduling requirementsThe initial delay is calculated as
interval + random value up to randomInitialDelay. This might result in an initial delay longer than expected. Verify that this calculation meets your scheduling needs, and consider whether theintervalshould be included in the initial delay.Run the following script to simulate and verify the initial delay calculations:
Replace
intervalandrandomInitialDelaywith your actual values to see if the initial delays are within the expected range.✅ Verification successful
Initial delay calculation is within the expected range and aligns with scheduling requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initial delay calculation logic. # Set the interval and randomInitialDelay values (replace with actual values) interval=5000 # example interval in milliseconds randomInitialDelay=10000 # example random initial delay in milliseconds # Simulate initial delay calculations for i in {1..5}; do randomDelay=$(shuf -i 0-$randomInitialDelay -n 1) initialDelay=$((interval + randomDelay)) echo "Calculated initial delay: $initialDelay ms (Interval: $interval ms, Random Delay: $randomDelay ms)" doneLength of output: 1567
Script:
#!/bin/bash # Description: Verify the initial delay calculation logic using $RANDOM. # Set the interval and randomInitialDelay values (replace with actual values) interval=5000 # example interval in milliseconds randomInitialDelay=10000 # example random initial delay in milliseconds # Simulate initial delay calculations for i in {1..5}; do randomDelay=$(( RANDOM % (randomInitialDelay + 1) )) initialDelay=$((interval + randomDelay)) echo "Calculated initial delay: $initialDelay ms (Interval: $interval ms, Random Delay: $randomDelay ms)" doneLength of output: 1145
src/main/java/com/ghostchu/peerbanhelper/btn/ability/BtnAbilitySubmitHistory.java
Show resolved
Hide resolved
src/main/java/com/ghostchu/peerbanhelper/btn/ability/BtnAbilitySubmitHistory.java
Outdated
Show resolved
Hide resolved
src/main/java/com/ghostchu/peerbanhelper/btn/ability/BtnAbilitySubmitHistory.java
Outdated
Show resolved
Hide resolved
src/main/java/com/ghostchu/peerbanhelper/btn/ability/BtnAbilitySubmitHistory.java
Show resolved
Hide resolved
本 PR 引入了对 BTN 协议新规范 “提交 Peers 累积数据列表 submit_histories” 的支持。
在过去的版本中，进行累积流量分析依赖于 snapshot 数据，但 snapshot 数据提交较为缓慢，且仅仅能捕获提交瞬间的 Peers 的流量情况。
为了弥补这个缺点，新增了一个 “提交 Peers 累积数据列表” 的协议。实现此协议的 BTN 客户端需要记录 Peers 的会话活动，并持续跟踪会话期间的流量数据改变。
在提交时，自上次提交以来发生的累积数据更新都将被提交到 BTN 服务器。BTN 服务器可根据累积数据跟踪 Peers 的在各个客户端上的流量活动，更加准确和高效的追踪过量下载的恶意 IP 地址。
BTN 网络协议版本号现在更新到 8.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation