fix: harden login account duplicate-row reads - #6
Merged
Conversation
Phase 36 domain-key duplicate-row hardening. Verification: - [x] `mvn test -Dtest=LoginServiceTest` — 122 tests, 0 failures, 0 errors\n- [x] `mvn test` — 459 tests, 0 failures, 0 errors gate_status: skill
📝 WalkthroughWalkthroughLoginService's getAccount(UUID) method now resolves duplicate player_uuid rows by selecting the account with the minimum non-null ID via a new selectCanonicalAccount helper, instead of returning the first list element. A corresponding test verifies this canonical selection behavior. ChangesCanonical account selection
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/ultikits/plugins/login/service/LoginService.java (1)
572-577: 🗄️ Data Integrity & Integration | 🔵 TrivialInconsistent duplicate-row handling across similar lookup methods.
getAccountByName(unchanged) still returnsaccounts.get(0)without canonical selection, whilegetAccount(UUID)now deterministically picks the lowest-id row. Ifplayer_namecan also have duplicate rows, this method has the same non-determinism the PR is fixing forplayer_uuid.
[medium_effort_and_high_reward]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/ultikits/plugins/login/service/LoginService.java` around lines 572 - 577, The duplicate-row handling in getAccountByName is still non-deterministic because it returns the first list entry without a canonical choice. Update this method to follow the same deterministic pattern as getAccount(UUID) by selecting the lowest-id AccountData when multiple rows match player_name. Keep the lookup in LoginService consistent across both methods so duplicate records resolve the same way everywhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/ultikits/plugins/login/service/LoginService.java`:
- Around line 612-616: The canonical account selection in selectCanonicalAccount
is comparing AccountData.getId() as a string, which can choose the wrong account
for numeric-like ids. Update the ordering to use a numeric or persisted
timestamp/sequence field instead of String::compareTo, and keep the fallback in
selectCanonicalAccount consistent with the intended “oldest row” behavior. Use
the selectCanonicalAccount method and AccountData accessors to locate the
comparison logic.
---
Nitpick comments:
In `@src/main/java/com/ultikits/plugins/login/service/LoginService.java`:
- Around line 572-577: The duplicate-row handling in getAccountByName is still
non-deterministic because it returns the first list entry without a canonical
choice. Update this method to follow the same deterministic pattern as
getAccount(UUID) by selecting the lowest-id AccountData when multiple rows match
player_name. Keep the lookup in LoginService consistent across both methods so
duplicate records resolve the same way everywhere.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00e4136c-2554-459a-9a5f-bc2f2fb4a4ea
📒 Files selected for processing (2)
src/main/java/com/ultikits/plugins/login/service/LoginService.javasrc/test/java/com/ultikits/plugins/login/service/LoginServiceTest.java
Comment on lines
+612
to
616
| private AccountData selectCanonicalAccount(List<AccountData> accounts) { | ||
| return accounts.stream() | ||
| .min(Comparator.comparing(AccountData::getId, Comparator.nullsLast(String::compareTo))) | ||
| .orElse(accounts.get(0)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find AccountData's id field/type and how IDs are generated (auto-increment numeric, UUID, etc.)
ast-grep outline src --items all --type class --match 'AccountData'
rg -n -A5 -B5 'class AccountData' src/main/java
rg -n 'setId\(' src/main/java --type=java -g '!*Test*'Repository: UltiKits/UltiLogin
Length of output: 1324
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## AccountData.java\n'
sed -n '1,220p' src/main/java/com/ultikits/plugins/login/entity/AccountData.java
printf '\n## BaseDataEntity references\n'
rg -n -A4 -B4 'class BaseDataEntity|extends BaseDataEntity|setId\(|getId\(' src/main/java src/test/java --type=java
printf '\n## BaseDataEntity.java if present\n'
fd -a 'BaseDataEntity.java' src/main/javaRepository: UltiKits/UltiLogin
Length of output: 13023
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## LoginService selectCanonicalAccount and nearby logic\n'
sed -n '580,660p' src/main/java/com/ultikits/plugins/login/service/LoginService.java
printf '\n## LoginServiceTest duplicate-account case\n'
sed -n '540,590p' src/test/java/com/ultikits/plugins/login/service/LoginServiceTest.java
printf '\n## Search for any numeric-id or insertion-order assumptions\n'
rg -n 'lowest-id|canonical|insert|earliest|hashCode\(\)|playerUuid.hashCode|order by|Comparator.comparing\(AccountData::getId' src/main/java src/test/java --type=javaRepository: UltiKits/UltiLogin
Length of output: 7341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## LoginService account creation / insert path\n'
sed -n '280,340p' src/main/java/com/ultikits/plugins/login/service/LoginService.java
printf '\n## Any other id assignment in main sources\n'
rg -n 'setId\(' src/main/java --type=java
printf '\n## AccountDataTest id-related assertions\n'
sed -n '170,210p' src/test/java/com/ultikits/plugins/login/entity/AccountDataTest.java
printf '\n## UltiLoginTestHelper sample account id assignment\n'
sed -n '130,170p' src/test/java/com/ultikits/plugins/login/UltiLoginTestHelper.javaRepository: UltiKits/UltiLogin
Length of output: 2208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Files mentioning login_accounts or id column\n'
rg -n 'login_accounts|player_uuid|register_time|id\b' src main . --hidden --glob '!**/target/**' --glob '!**/.git/**'
printf '\n## Candidate schema / migration / docs files\n'
fd -a -e sql -e md -e yml -e yaml -e json -e conf -e properties . .
printf '\n## Search for BaseDataEntity in repository and nearby docs\n'
rg -n 'BaseDataEntity|Table\("login_accounts"\)|`@Table`\("login_accounts"\)' . --hidden --glob '!**/target/**' --glob '!**/.git/**'Repository: UltiKits/UltiLogin
Length of output: 50375
Compare the canonical key numerically, not as a string. selectCanonicalAccount picks the lexicographically smallest id; with ids like String.valueOf(playerUuid.hashCode()), "10" sorts before "9". The duplicate-account test only covers equal-length ids, so it misses this case. If the intent is “oldest row,” sort by a persisted numeric/timestamp field instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/ultikits/plugins/login/service/LoginService.java` around
lines 612 - 616, The canonical account selection in selectCanonicalAccount is
comparing AccountData.getId() as a string, which can choose the wrong account
for numeric-like ids. Update the ordering to use a numeric or persisted
timestamp/sequence field instead of String::compareTo, and keep the fallback in
selectCanonicalAccount consistent with the intended “oldest row” behavior. Use
the selectCanonicalAccount method and AccountData accessors to locate the
comparison logic.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Phase 36 domain-key duplicate-row hardening for
UltiLogin.LoginService.getAccount(UUID)now selects the canonical lowest-idAccountDatarow when duplicateplayer_uuidrows exist, with a regression test covering the duplicate-row ordering case.Changes
src/main/java/com/ultikits/plugins/login/service/LoginService.javasrc/test/java/com/ultikits/plugins/login/service/LoginServiceTest.javaVerification
mvn test -Dtest=LoginServiceTest— 122 tests, 0 failures, 0 errors\n- [x]mvn test— 459 tests, 0 failures, 0 errorsgit diff --checkScope / Non-goals
Summary by CodeRabbit
Bug Fixes
Tests