fix : 修复登陆失败，事务失效问题#317
Conversation
|
|
XiaoSeS
left a comment
XiaoSeS
left a comment
There was a problem hiding this comment.
感谢提交这个修复！整体思路是对的——用独立事务把失败计数和外层登录事务解耦，确实能解决计数不落库的问题。读完代码我整理了一些想法和疑问，希望对你有帮助 🙏
关于根因的小澄清
PR 描述里提到 "Spring self-invocation 导致 @Transactional 失效"——我对照了下原代码，handleFailedLogin(LocalCredential) 是 private 方法且没有 @Transactional 注解，self-invocation 应该不是真实原因。
实际根因是：login() 标注了 @Transactional，当抛出 invalidCredentials()（RuntimeException）时 Spring 默认会回滚整个事务，导致内部 credentialRepository.save(credential) 一并被丢弃。
这不影响修复方案的正确性（REQUIRES_NEW 解决的就是这个问题），但建议在 commit message 里调整一下表述，免得后续维护者被误导。
两个比较担心的点
1. 并发场景下计数可能丢失（lost-update）
新实现是「先查再算再写」三步：
LocalCredential credential = credentialRepository.findById(credentialId)...;
int failedAttempts = credential.getFailedAttempts() + 1;
credentialRepository.updateFailedAttemptsAndLockedUntil(credentialId, failedAttempts, lockedUntil);如果同一账号短时间内被并发尝试（暴破场景就是典型），两个事务可能都读到 failedAttempts=N、都写 N+1，结果只增长了一次。考虑到这段代码的核心目的就是防暴破，这个 race condition 影响会比较直接。
是不是可以考虑用原子自增的 UPDATE 来规避？大致这个意思：
@Modifying
@Query("UPDATE LocalCredential c SET c.failedAttempts = c.failedAttempts + 1, " +
"c.lockedUntil = CASE WHEN c.failedAttempts + 1 >= :threshold AND c.lockedUntil IS NULL " +
" THEN :lockedUntil ELSE c.lockedUntil END " +
"WHERE c.id = :id")
int incrementFailedAttempts(@Param(\"id\") Long id, @Param(\"threshold\") int threshold, @Param(\"lockedUntil\") Instant lockedUntil);2. 锁定时长的判断逻辑变化
原代码：
if (failedAttempts >= MAX_FAILED_ATTEMPTS) {
credential.setLockedUntil(currentTime().plus(LOCK_DURATION));
}新代码加了 lockedUntil == null 这个条件，意味着如果一个账号曾经被锁过（lockedUntil 字段非空，但已过期），后续再失败 5 次也不会续锁。
这是有意的语义调整吗？PR 描述写的是「无功能变化」，所以想跟你确认下。如果是无意改的，建议恢复原行为；如果是有意的，最好补个测试 + 在描述里说明一下。
测试相关
- 原来的
login_afterMaxFailures_setsLockUsingInjectedClock是真实断言锁定时间戳的，新版本改成verify(...)后，阈值判断和锁定时间计算 实际上没有单测覆盖了。建议在这个 PR 里就给LocalAuthFailedService补一组单测（while 在 review 期间补会比开 follow-up 顺一些），覆盖failedAttempts == 4/== 5/> 5、lockedUntil各种状态的边界。 - 这次修复的核心是「外层回滚 + 内层提交」的事务传播语义。建议补一个
@SpringBootTest级别的集成测试触发一次失败登录，提交后从真实 DB 读failed_attempts == 1——这样能真正验证修复有效，而不只是验证方法被调用。
几个小优化（不阻塞）
LocalAuthService第 33-34 行的MAX_FAILED_ATTEMPTS和LOCK_DURATION常量已经没人用了，建议清理掉，否则容易出现两边数值分叉。LocalAuthFailedService构造器后面有 3 个空行、方法体开头有 1 个空行，过 formatter 顺手清一下。@Transactional(... rollbackFor = Exception.class)：方法内只会抛 unchecked，加rollbackFor没有实际作用，可以去掉。- 类的 JavaDoc 建议加一句解释「为什么用
REQUIRES_NEW」——这是这个 service 存在的核心理由，写下来对后续维护者很有用。
总体来说方向是对的，主要建议是处理一下并发问题和确认锁定语义。期待更新 👍
3 participants
Summary
LocalAuthFailedService，使用 @transactional(propagation = REQUIRES_NEW)
在独立事务中执行，确保即使外层登录事务回滚，失败计数和账户锁定仍能正确持久化
dirty checking 依赖外层事务
不经过代理，导致事务注解失效）
Validation
Commands run:
cd server && JDK_JAVA_OPTIONS="-XX:+EnableDynamicAgentLoading" ./mvnw test -pl skillhub-auth -am
-Dtest=LocalAuthServiceTest -Dsurefire.failIfNoSpecifiedTests=false
Tests run: 12, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS
Risk
Notes
不生效，导致失败计数未持久化
涉及文件
┌───────────────────────────────────────────────────────────────┬────────────────────────────────┐
│ 文件 │ 变更 │
├───────────────────────────────────────────────────────────────┼────────────────────────────────┤
│ server/skillhub-auth/src/main/java/.../LocalAuthFailedService │ 新增 — │
│ .java │ 独立事务处理失败登录计数 │
├───────────────────────────────────────────────────────────────┼────────────────────────────────┤
│ │ 注入 │
│ server/skillhub-auth/src/main/java/.../LocalAuthService.java │ LocalAuthFailedService，login │
│ │ 方法中调用替代原内联方法 │
├───────────────────────────────────────────────────────────────┼────────────────────────────────┤
│ server/skillhub-auth/src/main/java/.../LocalCredentialReposit │ 新增 updateFailedAttemptsAndLo │
│ ory.java │ ckedUntil JPQL 方法 │
├───────────────────────────────────────────────────────────────┼────────────────────────────────┤
│ │ 适配新架构：mock LocalAuthFail │
│ server/skillhub-auth/src/test/.../LocalAuthServiceTest.java │ edService，断言改为 verify │
│ │ 调用 │
└───────────────────────────────────────────────────────────────┴────────────────────────────────┘