Skip to content

fix: harden login account duplicate-row reads #6

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wisdommen merged 1 commit into from
Jul 7, 2026
Merged

fix: harden login account duplicate-row reads #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/main/java/com/ultikits/plugins/login/service/LoginService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.util.Base64;
import java.util.Comparator;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -605,7 +606,13 @@ public AccountData getAccount(UUID playerUuid) {
List<AccountData> accounts = dataOperator.query()
.where("player_uuid").eq(playerUuid.toString())
.list();
return accounts.isEmpty() ? null : accounts.get(0);
return accounts.isEmpty() ? null : selectCanonicalAccount(accounts);
}

private AccountData selectCanonicalAccount(List<AccountData> accounts) {
return accounts.stream()
.min(Comparator.comparing(AccountData::getId, Comparator.nullsLast(String::compareTo)))
.orElse(accounts.get(0));
}
Comment on lines +612 to 616

@coderabbitai coderabbitai Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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/java

Repository: 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=java

Repository: 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.java

Repository: 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.


/**
Expand Down
15 changes: 15 additions & 0 deletions src/test/java/com/ultikits/plugins/login/service/LoginServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,21 @@ void accountExists() {
assertThat(result).isSameAs(expected);
}

@Test
@DisplayName("Should select canonical lowest-id account when duplicate player UUID rows exist")
void duplicatePlayerUuidRowsSelectLowestIdAccount() {
AccountData newer = UltiLoginTestHelper.createSampleAccount(playerUuid, "Newer", "hash2", "salt2");
newer.setId("account-200");
AccountData canonical = UltiLoginTestHelper.createSampleAccount(playerUuid, "Canonical", "hash1", "salt1");
canonical.setId("account-100");
when(mockQuery.list())
.thenReturn(Arrays.asList(newer, canonical));

AccountData result = service.getAccount(playerUuid);

assertThat(result).isSameAs(canonical);
}

@Test
@DisplayName("Should return null when doesn't exist")
void noAccount() {
Expand Down