GH-909 Resolve auto-message issues with database query, and message ordering. #909
Conversation
… move database table to another class.
WalkthroughThis update makes a few clear changes across the auto message functionality. First, a redundant import in the AutoMessageCommand file was removed and then re-added without altering any method behavior. In the repository class, the old data model using an ignore wrapper has been replaced by a new table class, and related query logic and method signatures were adjusted accordingly. In the service routine, the recursive scheduling has been replaced with a timer-based method, and a documentation annotation was removed. Additionally, a new class has been introduced to represent a database table with a unique ID, and there was a minor comment fix in the messages file. Overall, the changes refine the internal implementations and update the data model without affecting public interfaces. ✨ Finishing Touches
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 (
|
vLuckyyy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/automessage/AutoMessageService.java (1)
56-63: Better scheduling approach!The timer-based scheduling is more reliable than recursive scheduling. However, consider adding error handling for the timer task.
private void tick() { this.scheduler.timerAsync( () -> { if (this.settings.enabled()) { - this.broadcastNextMessage(); + try { + this.broadcastNextMessage(); + } catch (Exception e) { + // Log error and ensure timer continues + this.server.getLogger().warning("Error broadcasting auto message: " + e.getMessage()); + } } }, this.settings.interval(), this.settings.interval()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
eternalcore-core/src/main/java/com/eternalcode/core/feature/automessage/AutoMessageCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/automessage/AutoMessageRepositoryOrmLite.java(3 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/automessage/AutoMessageService.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/automessage/AutoMessageTable.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/automessage/messages/ENAutoMessageMessages.java(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/automessage/messages/ENAutoMessageMessages.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/automessage/AutoMessageCommand.java
🔇 Additional comments (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/automessage/AutoMessageTable.java (1)
7-18: Nice and clean table design!The new table class is well-structured with proper ORM annotations and a clear purpose.
eternalcore-core/src/main/java/com/eternalcode/core/feature/automessage/AutoMessageRepositoryOrmLite.java (2)
33-39: Great fix for the database query issue!The new
where.in()approach is much better than the old loop-based query. This directly fixes the issue mentioned in the PR where player UUIDs weren't being combined correctly.
41-49: Simple and efficient filtering!Good use of streams to filter out ignored players from the online players list.
| this.action( | ||
| AutoMessageTable.class, dao -> { | ||
| Where<AutoMessageTable, Object> where = dao.queryBuilder().where(); | ||
| where.in("unique_id", onlineUniqueIds); |
Faulty Query:
The
forloop addedeq(equality) conditions for eachUUID, but it did not correctly combine them using theORoperator.The
or(onlineUniqueIds.size())method did not work as expected because it did not create a proper logicalORcondition in SQL.UUID1,UUID2,UUID3.UUID2has ignored messages.ignoredIdslist, such as an empty list.UUID2) received messages, even thoughUUID2had ignored them.Correct Behavior (After the Fix):
ignoredIdslist containing onlyUUID2.UUID1andUUID3.UUID2does not receive messages, in accordance with their settings.