Merged
Conversation
✅ Deploy Preview for kcloud-platform-iot ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Reviewer's Guide本次 PR 优化了认证域和基础设施层中的异常处理与日志记录，尤其是围绕用户创建和登录日志记录的部分，并通过从认证流程中移除受检异常来简化方法签名。 带精细化异常处理的 OAuth2 认证流程时序图sequenceDiagram
actor Client
participant OAuth2UsernamePasswordAuthenticationProvider
participant AbstractOAuth2AuthenticationProvider
participant OAuth2AuthenticationProcessor
participant AuthA
participant LoginLogConvertor
participant AddressUtils
participant DomainEventPublisher
Client->>OAuth2UsernamePasswordAuthenticationProvider: authenticate
OAuth2UsernamePasswordAuthenticationProvider->>AbstractOAuth2AuthenticationProvider: getPrincipal(request)
AbstractOAuth2AuthenticationProvider->>AuthA: createUsernamePasswordAuth
AuthA-->>AbstractOAuth2AuthenticationProvider: AuthA
AbstractOAuth2AuthenticationProvider->>OAuth2AuthenticationProcessor: authentication(authA, request)
alt authentication success
OAuth2AuthenticationProcessor->>AuthA: getUserE
OAuth2AuthenticationProcessor-->>AbstractOAuth2AuthenticationProvider: UsernamePasswordAuthenticationToken
AbstractOAuth2AuthenticationProvider-->>OAuth2UsernamePasswordAuthenticationProvider: Authentication
OAuth2UsernamePasswordAuthenticationProvider-->>Client: Authentication success
OAuth2AuthenticationProcessor->>DomainEventPublisher: publish(LoginEvent OK)
else authentication GlobalException
OAuth2AuthenticationProcessor->>LoginLogConvertor: toDomainEvent(request, authA, GlobalException)
LoginLogConvertor->>AddressUtils: getRealAddress(ip)
AddressUtils-->>LoginLogConvertor: address or throws InetAddressException or IOException or InterruptedException
alt address resolution failure
LoginLogConvertor-->>OAuth2AuthenticationProcessor: throws IllegalArgumentException
OAuth2AuthenticationProcessor->>DomainEventPublisher: publish(LoginEvent null)
OAuth2AuthenticationProcessor-->>AbstractOAuth2AuthenticationProvider: GlobalException rethrown
else address resolution success
LoginLogConvertor-->>OAuth2AuthenticationProcessor: LoginEvent FAIL
OAuth2AuthenticationProcessor->>DomainEventPublisher: publish(LoginEvent FAIL)
OAuth2AuthenticationProcessor-->>AbstractOAuth2AuthenticationProvider: GlobalException rethrown
end
AbstractOAuth2AuthenticationProvider-->>OAuth2UsernamePasswordAuthenticationProvider: propagate failure
OAuth2UsernamePasswordAuthenticationProvider-->>Client: Authentication failure
end
带内部异常处理的 AuthA 用户创建时序图sequenceDiagram
participant Caller
participant AuthA
participant RSAUtils
participant AESUtils
Caller->>AuthA: createUsernamePasswordAuth
AuthA->>AuthA: getUserVByUsernamePasswordAuth
AuthA->>RSAUtils: decryptByPrivateKey(usernameParam)
RSAUtils-->>AuthA: username
AuthA->>RSAUtils: decryptByPrivateKey(passwordParam)
RSAUtils-->>AuthA: password
AuthA->>AESUtils: encrypt(username)
AESUtils-->>AuthA: encryptedUsername
AuthA-->>AuthA: build UserV
AuthA-->>Caller: AuthA with UserV
note over AuthA: 在 getUserVByUsernamePasswordAuth 内部出现任意 Exception 时
AuthA-->>Caller: throws IllegalArgumentException
更新后的认证域与基础设施异常处理类图classDiagram
class AuthA {
<<Entity>>
+GrantType grantType
+CaptchaV captchaV
+UserV userV
+AuthA createUsernamePasswordAuth()
+AuthA createMobileAuth()
+AuthA createMailAuth()
+AuthA createAuthorizationCodeAuth()
+AuthA createTestAuth()
-UserV getUserVByUsernamePasswordAuth()
-UserV getUserVByAuthorizationCodeAuth()
-UserV getUserVByMobileAuth()
-UserV getUserVByMailAuth()
-UserV getUserVByTestAuth()
-AuthA create()
}
class OAuth2AuthenticationProcessor {
-DomainEventPublisher kafkaDomainEventPublisher
+UsernamePasswordAuthenticationToken authentication(AuthA authA, HttpServletRequest request)
}
class AbstractOAuth2AuthenticationProvider {
-OAuth2AuthenticationProcessor authenticationProcessor
+Authentication authentication(Authentication authentication, HttpServletRequest request)
+UsernamePasswordAuthenticationToken authentication(AuthA authA, HttpServletRequest request)
<<abstract>>
}
class OAuth2UsernamePasswordAuthenticationProvider {
+boolean supports(Class authentication)
+Authentication getPrincipal(HttpServletRequest request)
}
class OAuth2MobileAuthenticationProvider {
+boolean supports(Class authentication)
+Authentication getPrincipal(HttpServletRequest request)
}
class OAuth2MailAuthenticationProvider {
+boolean supports(Class authentication)
+Authentication getPrincipal(HttpServletRequest request)
}
class OAuth2TestAuthenticationProvider {
+boolean supports(Class authentication)
+Authentication getPrincipal(HttpServletRequest request)
}
class LoginLogConvertor {
+LoginLogCO toClientObject(LoginEvent loginEvent)
+LoginEvent toDomainEvent(HttpServletRequest request, AuthA authA, GlobalException gex)
}
class UserDetailsServiceImpl {
+UserDetails loadUserByUsername(String username)
}
class DomainEventPublisher {
+void publish(LoginEvent event)
}
class LoginEvent {
+Long id
+String username
+String ip
+String address
+String browser
+String os
+int status
+String errorMessage
+int type
+Long loginTime
+Long tenantId
+Long creator
+Long deptId
}
class UserV {
+String username()
+String password()
+String tenantCode()
+String mail()
+String mobile()
}
class UserE {
+Long getId()
+Long getTenantId()
+Long getDeptId()
}
class GlobalException {
+String getMsg()
}
class HttpServletRequest
class Authentication
class UsernamePasswordAuthenticationToken
OAuth2UsernamePasswordAuthenticationProvider --|> AbstractOAuth2AuthenticationProvider
OAuth2MobileAuthenticationProvider --|> AbstractOAuth2AuthenticationProvider
OAuth2MailAuthenticationProvider --|> AbstractOAuth2AuthenticationProvider
OAuth2TestAuthenticationProvider --|> AbstractOAuth2AuthenticationProvider
AbstractOAuth2AuthenticationProvider --> OAuth2AuthenticationProcessor
OAuth2AuthenticationProcessor --> AuthA
OAuth2AuthenticationProcessor --> DomainEventPublisher
OAuth2AuthenticationProcessor --> LoginLogConvertor
LoginLogConvertor --> AuthA
LoginLogConvertor --> LoginEvent
UserDetailsServiceImpl --> OAuth2AuthenticationProcessor
UserDetailsServiceImpl --> AuthA
UserDetailsServiceImpl --> UserV
AuthA --> UserV
AuthA --> UserE
LoginEvent --> LoginLogConvertor
GlobalException --> LoginLogConvertor
HttpServletRequest --> LoginLogConvertor
HttpServletRequest --> OAuth2AuthenticationProcessor
Authentication --> AbstractOAuth2AuthenticationProvider
UsernamePasswordAuthenticationToken --> AbstractOAuth2AuthenticationProvider
文件级变更
提示与命令与 Sourcery 交互
自定义你的体验访问你的 控制面板 来：
获取帮助Original review guide in EnglishReviewer's GuideThis PR refines exception handling and logging in the auth domain and infrastructure layers, especially around user creation and login logging, while simplifying method signatures by removing checked exceptions from authentication flows. Sequence diagram for OAuth2 authentication flow with refined exception handlingsequenceDiagram
actor Client
participant OAuth2UsernamePasswordAuthenticationProvider
participant AbstractOAuth2AuthenticationProvider
participant OAuth2AuthenticationProcessor
participant AuthA
participant LoginLogConvertor
participant AddressUtils
participant DomainEventPublisher
Client->>OAuth2UsernamePasswordAuthenticationProvider: authenticate
OAuth2UsernamePasswordAuthenticationProvider->>AbstractOAuth2AuthenticationProvider: getPrincipal(request)
AbstractOAuth2AuthenticationProvider->>AuthA: createUsernamePasswordAuth
AuthA-->>AbstractOAuth2AuthenticationProvider: AuthA
AbstractOAuth2AuthenticationProvider->>OAuth2AuthenticationProcessor: authentication(authA, request)
alt authentication success
OAuth2AuthenticationProcessor->>AuthA: getUserE
OAuth2AuthenticationProcessor-->>AbstractOAuth2AuthenticationProvider: UsernamePasswordAuthenticationToken
AbstractOAuth2AuthenticationProvider-->>OAuth2UsernamePasswordAuthenticationProvider: Authentication
OAuth2UsernamePasswordAuthenticationProvider-->>Client: Authentication success
OAuth2AuthenticationProcessor->>DomainEventPublisher: publish(LoginEvent OK)
else authentication GlobalException
OAuth2AuthenticationProcessor->>LoginLogConvertor: toDomainEvent(request, authA, GlobalException)
LoginLogConvertor->>AddressUtils: getRealAddress(ip)
AddressUtils-->>LoginLogConvertor: address or throws InetAddressException or IOException or InterruptedException
alt address resolution failure
LoginLogConvertor-->>OAuth2AuthenticationProcessor: throws IllegalArgumentException
OAuth2AuthenticationProcessor->>DomainEventPublisher: publish(LoginEvent null)
OAuth2AuthenticationProcessor-->>AbstractOAuth2AuthenticationProvider: GlobalException rethrown
else address resolution success
LoginLogConvertor-->>OAuth2AuthenticationProcessor: LoginEvent FAIL
OAuth2AuthenticationProcessor->>DomainEventPublisher: publish(LoginEvent FAIL)
OAuth2AuthenticationProcessor-->>AbstractOAuth2AuthenticationProvider: GlobalException rethrown
end
AbstractOAuth2AuthenticationProvider-->>OAuth2UsernamePasswordAuthenticationProvider: propagate failure
OAuth2UsernamePasswordAuthenticationProvider-->>Client: Authentication failure
end
Sequence diagram for AuthA user creation with internal exception handlingsequenceDiagram
participant Caller
participant AuthA
participant RSAUtils
participant AESUtils
Caller->>AuthA: createUsernamePasswordAuth
AuthA->>AuthA: getUserVByUsernamePasswordAuth
AuthA->>RSAUtils: decryptByPrivateKey(usernameParam)
RSAUtils-->>AuthA: username
AuthA->>RSAUtils: decryptByPrivateKey(passwordParam)
RSAUtils-->>AuthA: password
AuthA->>AESUtils: encrypt(username)
AESUtils-->>AuthA: encryptedUsername
AuthA-->>AuthA: build UserV
AuthA-->>Caller: AuthA with UserV
note over AuthA: On any Exception inside getUserVByUsernamePasswordAuth
AuthA-->>Caller: throws IllegalArgumentException
Updated class diagram for auth domain and infrastructure exception handlingclassDiagram
class AuthA {
<<Entity>>
+GrantType grantType
+CaptchaV captchaV
+UserV userV
+AuthA createUsernamePasswordAuth()
+AuthA createMobileAuth()
+AuthA createMailAuth()
+AuthA createAuthorizationCodeAuth()
+AuthA createTestAuth()
-UserV getUserVByUsernamePasswordAuth()
-UserV getUserVByAuthorizationCodeAuth()
-UserV getUserVByMobileAuth()
-UserV getUserVByMailAuth()
-UserV getUserVByTestAuth()
-AuthA create()
}
class OAuth2AuthenticationProcessor {
-DomainEventPublisher kafkaDomainEventPublisher
+UsernamePasswordAuthenticationToken authentication(AuthA authA, HttpServletRequest request)
}
class AbstractOAuth2AuthenticationProvider {
-OAuth2AuthenticationProcessor authenticationProcessor
+Authentication authentication(Authentication authentication, HttpServletRequest request)
+UsernamePasswordAuthenticationToken authentication(AuthA authA, HttpServletRequest request)
<<abstract>>
}
class OAuth2UsernamePasswordAuthenticationProvider {
+boolean supports(Class authentication)
+Authentication getPrincipal(HttpServletRequest request)
}
class OAuth2MobileAuthenticationProvider {
+boolean supports(Class authentication)
+Authentication getPrincipal(HttpServletRequest request)
}
class OAuth2MailAuthenticationProvider {
+boolean supports(Class authentication)
+Authentication getPrincipal(HttpServletRequest request)
}
class OAuth2TestAuthenticationProvider {
+boolean supports(Class authentication)
+Authentication getPrincipal(HttpServletRequest request)
}
class LoginLogConvertor {
+LoginLogCO toClientObject(LoginEvent loginEvent)
+LoginEvent toDomainEvent(HttpServletRequest request, AuthA authA, GlobalException gex)
}
class UserDetailsServiceImpl {
+UserDetails loadUserByUsername(String username)
}
class DomainEventPublisher {
+void publish(LoginEvent event)
}
class LoginEvent {
+Long id
+String username
+String ip
+String address
+String browser
+String os
+int status
+String errorMessage
+int type
+Long loginTime
+Long tenantId
+Long creator
+Long deptId
}
class UserV {
+String username()
+String password()
+String tenantCode()
+String mail()
+String mobile()
}
class UserE {
+Long getId()
+Long getTenantId()
+Long getDeptId()
}
class GlobalException {
+String getMsg()
}
class HttpServletRequest
class Authentication
class UsernamePasswordAuthenticationToken
OAuth2UsernamePasswordAuthenticationProvider --|> AbstractOAuth2AuthenticationProvider
OAuth2MobileAuthenticationProvider --|> AbstractOAuth2AuthenticationProvider
OAuth2MailAuthenticationProvider --|> AbstractOAuth2AuthenticationProvider
OAuth2TestAuthenticationProvider --|> AbstractOAuth2AuthenticationProvider
AbstractOAuth2AuthenticationProvider --> OAuth2AuthenticationProcessor
OAuth2AuthenticationProcessor --> AuthA
OAuth2AuthenticationProcessor --> DomainEventPublisher
OAuth2AuthenticationProcessor --> LoginLogConvertor
LoginLogConvertor --> AuthA
LoginLogConvertor --> LoginEvent
UserDetailsServiceImpl --> OAuth2AuthenticationProcessor
UserDetailsServiceImpl --> AuthA
UserDetailsServiceImpl --> UserV
AuthA --> UserV
AuthA --> UserE
LoginEvent --> LoginLogConvertor
GlobalException --> LoginLogConvertor
HttpServletRequest --> LoginLogConvertor
HttpServletRequest --> OAuth2AuthenticationProcessor
Authentication --> AbstractOAuth2AuthenticationProvider
UsernamePasswordAuthenticationToken --> AbstractOAuth2AuthenticationProvider
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoRefactor exception handling and improve error logging in authentication module
WalkthroughsDescription• Remove checked exceptions from method signatures across authentication flow • Add proper exception handling with try-catch blocks and logging • Refactor exception handling in UserDetailsServiceImpl to remove unnecessary wrapping • Add @Slf4j annotation for logging in AuthA and LoginLogConvertor classes • Remove unused @Component annotation from OAuth2AuthenticationFailureHandler Diagramflowchart LR
A["Method Signatures"] -->|Remove throws Exception| B["Clean API"]
C["Exception Handling"] -->|Add try-catch blocks| D["Internal Error Management"]
E["Logging"] -->|Add @Slf4j annotation| F["Better Observability"]
B --> G["Improved Code Quality"]
D --> G
F --> G
File Changes1. laokou-service/laokou-auth/laokou-auth-domain/src/main/java/org/laokou/auth/model/AuthA.java
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
WalkthroughThis PR systematically removes Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.16.4)laokou-service/laokou-auth/laokou-auth-domain/src/main/java/org/laokou/auth/model/AuthA.java┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m laokou-service/laokou-auth/laokou-auth-infrastructure/src/main/java/org/laokou/auth/config/authentication/AbstractOAuth2AuthenticationProvider.java┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m laokou-service/laokou-auth/laokou-auth-infrastructure/src/main/java/org/laokou/auth/config/authentication/OAuth2AuthenticationProcessor.java┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了两个问题，并给出了一些整体性的反馈：
- 在
AuthA中新的异常处理逻辑（例如getUserVByUsernamePasswordAuth、getUserVByMailAuth等），建议保留原始异常原因，而不是仅使用ex.getMessage()抛出IllegalArgumentException，这样调用方可以更准确地检查底层异常及其堆栈信息。 LoginLogConvertor.toDomainEvent方法现在在地址解析失败时抛出IllegalArgumentException；你可能会希望在这里记录错误日志，并回退到一个默认地址，这样临时性的 IP 查询问题就不会中断登录流程/事件创建。- 对于加解密以及 IO/IP 查询路径中的失败，使用
IllegalArgumentException在语义上略显误导；可以考虑使用更贴近业务语义的运行时异常（或者复用GlobalException/BizException），以更好地表达这些失败的性质。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new exception handling in `AuthA` (e.g., `getUserVByUsernamePasswordAuth`, `getUserVByMailAuth`, etc.), consider preserving the original cause instead of throwing `IllegalArgumentException` with only `ex.getMessage()`, so callers can inspect the underlying exception and stack trace more accurately.
- The `LoginLogConvertor.toDomainEvent` method now throws `IllegalArgumentException` when address resolution fails; you might want to log the error and fall back to a default address instead, so a transient IP lookup issue doesn’t break the login flow/event creation.
- Using `IllegalArgumentException` for failures in cryptographic/decryption and IO/IP lookup paths is a bit misleading semantically; consider using a more domain-appropriate runtime exception (or reusing `GlobalException`/`BizException`) to better express the nature of these failures.
## Individual Comments
### Comment 1
<location path="laokou-service/laokou-auth/laokou-auth-domain/src/main/java/org/laokou/auth/model/AuthA.java" line_range="471-473" />
<code_context>
- .mail(AESUtils.encrypt(this.captchaV.uuid()))
- .mobile(StringConstants.EMPTY)
- .build();
+ private UserV getUserVByAuthorizationCodeAuth() {
+ try {
+ String username = getParameterValue(Constants.USERNAME);
+ String password = getParameterValue(Constants.PASSWORD);
+ String tenantCode = getParameterValue(Constants.TENANT_CODE);
+ return UserV.builder()
+ .username(AESUtils.encrypt(username))
+ .password(password)
+ .tenantCode(tenantCode)
+ .mail(StringConstants.EMPTY)
+ .mobile(StringConstants.EMPTY)
+ .build();
+ }
+ catch (Exception ex) {
+ log.error("getUserVByAuthorizationCodeAuth error: {}", ex.getMessage(), ex);
+ throw new IllegalArgumentException(ex.getMessage());
</code_context>
<issue_to_address>
**suggestion (bug_risk):** InterruptedException (and other checked errors) are treated as IllegalArgumentException, and the thread is not re-interrupted.
This method wraps all `Exception`s in `IllegalArgumentException(ex.getMessage())`, losing the original cause and any interrupted status. If `InterruptedException` (or other non-programming errors) can be thrown, the current thread should be re-interrupted and the cause preserved. Consider calling `Thread.currentThread().interrupt();` before rethrowing and using a more suitable runtime exception that wraps the original cause (e.g., `new RuntimeException("Failed to build UserV for authorization code auth", ex)`).
```suggestion
catch (InterruptedException ex) {
Thread.currentThread().interrupt();
log.error("getUserVByAuthorizationCodeAuth interrupted: {}", ex.getMessage(), ex);
throw new RuntimeException("Failed to build UserV for authorization code auth", ex);
}
catch (Exception ex) {
log.error("getUserVByAuthorizationCodeAuth error: {}", ex.getMessage(), ex);
throw new RuntimeException("Failed to build UserV for authorization code auth", ex);
```
</issue_to_address>
### Comment 2
<location path="laokou-service/laokou-auth/laokou-auth-infrastructure/src/main/java/org/laokou/auth/convertor/LoginLogConvertor.java" line_range="106-115" />
<code_context>
+ public static LoginEvent toDomainEvent(HttpServletRequest request, AuthA authA, GlobalException gex) {
</code_context>
<issue_to_address>
**issue (bug_risk):** `toDomainEvent` now throws `IllegalArgumentException` on infrastructure issues, which can misclassify failures and drop interruption status.
Wrapping `InetAddressException`, `IOException`, and `InterruptedException` in `IllegalArgumentException(ex.getMessage())` both mislabels infrastructure/IO failures as argument issues and drops the original cause. It also swallows `InterruptedException` without restoring the interrupt flag.
Consider instead:
- Preserving the cause, e.g. `throw new RuntimeException("Failed to build LoginEvent", ex);` or a domain-specific exception.
- If interruption matters here, call `Thread.currentThread().interrupt();` before rethrowing.
This preserves logging while giving callers clearer error signals and respecting cooperative cancellation.
</issue_to_address>Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- In the new exception handling in
AuthA(e.g.,getUserVByUsernamePasswordAuth,getUserVByMailAuth, etc.), consider preserving the original cause instead of throwingIllegalArgumentExceptionwith onlyex.getMessage(), so callers can inspect the underlying exception and stack trace more accurately. - The
LoginLogConvertor.toDomainEventmethod now throwsIllegalArgumentExceptionwhen address resolution fails; you might want to log the error and fall back to a default address instead, so a transient IP lookup issue doesn’t break the login flow/event creation. - Using
IllegalArgumentExceptionfor failures in cryptographic/decryption and IO/IP lookup paths is a bit misleading semantically; consider using a more domain-appropriate runtime exception (or reusingGlobalException/BizException) to better express the nature of these failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new exception handling in `AuthA` (e.g., `getUserVByUsernamePasswordAuth`, `getUserVByMailAuth`, etc.), consider preserving the original cause instead of throwing `IllegalArgumentException` with only `ex.getMessage()`, so callers can inspect the underlying exception and stack trace more accurately.
- The `LoginLogConvertor.toDomainEvent` method now throws `IllegalArgumentException` when address resolution fails; you might want to log the error and fall back to a default address instead, so a transient IP lookup issue doesn’t break the login flow/event creation.
- Using `IllegalArgumentException` for failures in cryptographic/decryption and IO/IP lookup paths is a bit misleading semantically; consider using a more domain-appropriate runtime exception (or reusing `GlobalException`/`BizException`) to better express the nature of these failures.
## Individual Comments
### Comment 1
<location path="laokou-service/laokou-auth/laokou-auth-domain/src/main/java/org/laokou/auth/model/AuthA.java" line_range="471-473" />
<code_context>
- .mail(AESUtils.encrypt(this.captchaV.uuid()))
- .mobile(StringConstants.EMPTY)
- .build();
+ private UserV getUserVByAuthorizationCodeAuth() {
+ try {
+ String username = getParameterValue(Constants.USERNAME);
+ String password = getParameterValue(Constants.PASSWORD);
+ String tenantCode = getParameterValue(Constants.TENANT_CODE);
+ return UserV.builder()
+ .username(AESUtils.encrypt(username))
+ .password(password)
+ .tenantCode(tenantCode)
+ .mail(StringConstants.EMPTY)
+ .mobile(StringConstants.EMPTY)
+ .build();
+ }
+ catch (Exception ex) {
+ log.error("getUserVByAuthorizationCodeAuth error: {}", ex.getMessage(), ex);
+ throw new IllegalArgumentException(ex.getMessage());
</code_context>
<issue_to_address>
**suggestion (bug_risk):** InterruptedException (and other checked errors) are treated as IllegalArgumentException, and the thread is not re-interrupted.
This method wraps all `Exception`s in `IllegalArgumentException(ex.getMessage())`, losing the original cause and any interrupted status. If `InterruptedException` (or other non-programming errors) can be thrown, the current thread should be re-interrupted and the cause preserved. Consider calling `Thread.currentThread().interrupt();` before rethrowing and using a more suitable runtime exception that wraps the original cause (e.g., `new RuntimeException("Failed to build UserV for authorization code auth", ex)`).
```suggestion
catch (InterruptedException ex) {
Thread.currentThread().interrupt();
log.error("getUserVByAuthorizationCodeAuth interrupted: {}", ex.getMessage(), ex);
throw new RuntimeException("Failed to build UserV for authorization code auth", ex);
}
catch (Exception ex) {
log.error("getUserVByAuthorizationCodeAuth error: {}", ex.getMessage(), ex);
throw new RuntimeException("Failed to build UserV for authorization code auth", ex);
```
</issue_to_address>
### Comment 2
<location path="laokou-service/laokou-auth/laokou-auth-infrastructure/src/main/java/org/laokou/auth/convertor/LoginLogConvertor.java" line_range="106-115" />
<code_context>
+ public static LoginEvent toDomainEvent(HttpServletRequest request, AuthA authA, GlobalException gex) {
</code_context>
<issue_to_address>
**issue (bug_risk):** `toDomainEvent` now throws `IllegalArgumentException` on infrastructure issues, which can misclassify failures and drop interruption status.
Wrapping `InetAddressException`, `IOException`, and `InterruptedException` in `IllegalArgumentException(ex.getMessage())` both mislabels infrastructure/IO failures as argument issues and drops the original cause. It also swallows `InterruptedException` without restoring the interrupt flag.
Consider instead:
- Preserving the cause, e.g. `throw new RuntimeException("Failed to build LoginEvent", ex);` or a domain-specific exception.
- If interruption matters here, call `Thread.currentThread().interrupt();` before rethrowing.
This preserves logging while giving callers clearer error signals and respecting cooperative cancellation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+471
to
+473
| catch (Exception ex) { | ||
| log.error("getUserVByAuthorizationCodeAuth error: {}", ex.getMessage(), ex); | ||
| throw new IllegalArgumentException(ex.getMessage()); |
There was a problem hiding this comment.
suggestion (bug_risk): InterruptedException（以及其他受检异常）被当作 IllegalArgumentException 处理，而且线程不会被重新设置中断标记。
该方法将所有 Exception 都包装成 IllegalArgumentException(ex.getMessage())，从而丢失了原始异常原因以及任何中断状态。如果这里可能抛出 InterruptedException（或其他非编程错误），则应在重新抛出之前重新设置当前线程的中断标记，并保留原始异常作为 cause。可以考虑在重新抛出前调用 Thread.currentThread().interrupt();，并使用更合适的运行时异常来包装原始异常（例如：new RuntimeException("Failed to build UserV for authorization code auth", ex)）。
Suggested change
| catch (Exception ex) { | |
| log.error("getUserVByAuthorizationCodeAuth error: {}", ex.getMessage(), ex); | |
| throw new IllegalArgumentException(ex.getMessage()); | |
| catch (InterruptedException ex) { | |
| Thread.currentThread().interrupt(); | |
| log.error("getUserVByAuthorizationCodeAuth interrupted: {}", ex.getMessage(), ex); | |
| throw new RuntimeException("Failed to build UserV for authorization code auth", ex); | |
| } | |
| catch (Exception ex) { | |
| log.error("getUserVByAuthorizationCodeAuth error: {}", ex.getMessage(), ex); | |
| throw new RuntimeException("Failed to build UserV for authorization code auth", ex); |
Original comment in English
suggestion (bug_risk): InterruptedException (and other checked errors) are treated as IllegalArgumentException, and the thread is not re-interrupted.
This method wraps all Exceptions in IllegalArgumentException(ex.getMessage()), losing the original cause and any interrupted status. If InterruptedException (or other non-programming errors) can be thrown, the current thread should be re-interrupted and the cause preserved. Consider calling Thread.currentThread().interrupt(); before rethrowing and using a more suitable runtime exception that wraps the original cause (e.g., new RuntimeException("Failed to build UserV for authorization code auth", ex)).
Suggested change
| catch (Exception ex) { | |
| log.error("getUserVByAuthorizationCodeAuth error: {}", ex.getMessage(), ex); | |
| throw new IllegalArgumentException(ex.getMessage()); | |
| catch (InterruptedException ex) { | |
| Thread.currentThread().interrupt(); | |
| log.error("getUserVByAuthorizationCodeAuth interrupted: {}", ex.getMessage(), ex); | |
| throw new RuntimeException("Failed to build UserV for authorization code auth", ex); | |
| } | |
| catch (Exception ex) { | |
| log.error("getUserVByAuthorizationCodeAuth error: {}", ex.getMessage(), ex); | |
| throw new RuntimeException("Failed to build UserV for authorization code auth", ex); |
Comment on lines
+106
to
+115
| public static LoginEvent toDomainEvent(HttpServletRequest request, AuthA authA, GlobalException gex) { | ||
| try { | ||
| Capabilities capabilities = RequestUtils.getCapabilities(request); | ||
| String ip = IpUtils.getIpAddr(request); | ||
| int status = LoginStatus.OK.getCode(); | ||
| UserE userE = authA.getUserE(); | ||
| Optional<UserE> optional = Optional.ofNullable(userE); | ||
| Long creator = optional.map(UserE::getId).orElse(null); | ||
| Long tenantId = optional.map(UserE::getTenantId).orElse(null); | ||
| Long deptId = optional.map(UserE::getDeptId).orElse(null); |
There was a problem hiding this comment.
issue (bug_risk): toDomainEvent 现在在出现基础设施层问题时抛出 IllegalArgumentException，这可能会导致错误被错误分类，并且丢失中断状态。
将 InetAddressException、IOException 和 InterruptedException 包装成 IllegalArgumentException(ex.getMessage())，既把基础设施/IO 失败误标记为参数错误，又丢失了原始异常原因。同时还吞掉了 InterruptedException，而没有恢复中断标记。
可以考虑：
- 保留原始 cause，例如
throw new RuntimeException("Failed to build LoginEvent", ex);，或者使用一个领域特定的异常； - 如果这里的中断语义很重要，在重新抛出前调用
Thread.currentThread().interrupt();。
这样既能保留日志，又能为调用方提供更清晰的错误信号，并遵守协作式取消的约定。
Original comment in English
issue (bug_risk): toDomainEvent now throws IllegalArgumentException on infrastructure issues, which can misclassify failures and drop interruption status.
Wrapping InetAddressException, IOException, and InterruptedException in IllegalArgumentException(ex.getMessage()) both mislabels infrastructure/IO failures as argument issues and drops the original cause. It also swallows InterruptedException without restoring the interrupt flag.
Consider instead:
- Preserving the cause, e.g.
throw new RuntimeException("Failed to build LoginEvent", ex);or a domain-specific exception. - If interruption matters here, call
Thread.currentThread().interrupt();before rethrowing.
This preserves logging while giving callers clearer error signals and respecting cooperative cancellation.
|
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Code Review by Qodo
1. GlobalException masked as IAE
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+435
to
+504
| private UserV getUserVByUsernamePasswordAuth() { | ||
| try { | ||
| String username = RSAUtils.decryptByPrivateKey(getParameterValue(Constants.USERNAME)); | ||
| String password = RSAUtils.decryptByPrivateKey(getParameterValue(Constants.PASSWORD)); | ||
| String tenantCode = getParameterValue(Constants.TENANT_CODE); | ||
| return UserV.builder() | ||
| .username(AESUtils.encrypt(username)) | ||
| .password(password) | ||
| .tenantCode(tenantCode) | ||
| .mail(StringConstants.EMPTY) | ||
| .mobile(StringConstants.EMPTY) | ||
| .build(); | ||
| } | ||
| catch (Exception ex) { | ||
| log.error("getUserVByUsernamePasswordAuth error: {}", ex.getMessage(), ex); | ||
| throw new IllegalArgumentException(ex.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| private UserV getUserVByTestAuth() throws Exception { | ||
| private UserV getUserVByTestAuth() { | ||
| return getUserVByAuthorizationCodeAuth(); | ||
| } | ||
|
|
||
| private UserV getUserVByAuthorizationCodeAuth() throws Exception { | ||
| String username = getParameterValue(Constants.USERNAME); | ||
| String password = getParameterValue(Constants.PASSWORD); | ||
| String tenantCode = getParameterValue(Constants.TENANT_CODE); | ||
| return UserV.builder() | ||
| .username(AESUtils.encrypt(username)) | ||
| .password(password) | ||
| .tenantCode(tenantCode) | ||
| .mail(StringConstants.EMPTY) | ||
| .mobile(StringConstants.EMPTY) | ||
| .build(); | ||
| } | ||
|
|
||
| private UserV getUserVByMobileAuth() throws Exception { | ||
| String tenantCode = getParameterValue(Constants.TENANT_CODE); | ||
| return UserV.builder() | ||
| .username(StringConstants.EMPTY) | ||
| .tenantCode(tenantCode) | ||
| .mail(StringConstants.EMPTY) | ||
| .mobile(AESUtils.encrypt(this.captchaV.uuid())) | ||
| .build(); | ||
| } | ||
|
|
||
| private UserV getUserVByMailAuth() throws Exception { | ||
| String tenantCode = getParameterValue(Constants.TENANT_CODE); | ||
| return UserV.builder() | ||
| .username(StringConstants.EMPTY) | ||
| .tenantCode(tenantCode) | ||
| .mail(AESUtils.encrypt(this.captchaV.uuid())) | ||
| .mobile(StringConstants.EMPTY) | ||
| .build(); | ||
| private UserV getUserVByAuthorizationCodeAuth() { | ||
| try { | ||
| String username = getParameterValue(Constants.USERNAME); | ||
| String password = getParameterValue(Constants.PASSWORD); | ||
| String tenantCode = getParameterValue(Constants.TENANT_CODE); | ||
| return UserV.builder() | ||
| .username(AESUtils.encrypt(username)) | ||
| .password(password) | ||
| .tenantCode(tenantCode) | ||
| .mail(StringConstants.EMPTY) | ||
| .mobile(StringConstants.EMPTY) | ||
| .build(); | ||
| } | ||
| catch (Exception ex) { | ||
| log.error("getUserVByAuthorizationCodeAuth error: {}", ex.getMessage(), ex); | ||
| throw new IllegalArgumentException(ex.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| private UserV getUserVByMobileAuth() { | ||
| try { | ||
| return UserV.builder() | ||
| .username(StringConstants.EMPTY) | ||
| .tenantCode(getParameterValue(Constants.TENANT_CODE)) | ||
| .mail(StringConstants.EMPTY) | ||
| .mobile(AESUtils.encrypt(this.captchaV.uuid())) | ||
| .build(); | ||
| } | ||
| catch (Exception ex) { | ||
| log.error("getUserVByMobileAuth error: {}", ex.getMessage(), ex); | ||
| throw new IllegalArgumentException(ex); | ||
| } | ||
| } | ||
|
|
||
| private UserV getUserVByMailAuth() { | ||
| try { | ||
| return UserV.builder() | ||
| .username(StringConstants.EMPTY) | ||
| .tenantCode(getParameterValue(Constants.TENANT_CODE)) | ||
| .mail(AESUtils.encrypt(this.captchaV.uuid())) | ||
| .mobile(StringConstants.EMPTY) | ||
| .build(); | ||
| } | ||
| catch (Exception ex) { | ||
| log.error("getUserVByMailAuth error: {}", ex.getMessage(), ex); | ||
| throw new IllegalArgumentException(ex.getMessage()); | ||
| } |
There was a problem hiding this comment.
1. Globalexception masked as iae 🐞 Bug ✓ Correctness
AuthA now catches exceptions from RSA/AES operations and rethrows IllegalArgumentException, which converts GlobalException/SystemException into a non-GlobalException and bypasses OAuth2 error translation. As a result, malformed/invalid encrypted credentials can surface as generic RuntimeException/500 instead of a structured OAuth2/GlobalException response.
Agent Prompt
## Issue description
`AuthA` converts `GlobalException`/`SystemException` (e.g., from `RSAUtils.decryptByPrivateKey`) into `IllegalArgumentException`. This breaks the downstream exception translation that relies on `GlobalException` and can turn normal auth failures into generic 500s.
## Issue Context
- `RSAUtils.decryptByPrivateKey(...)` throws `SystemException` (a `GlobalException`) on decrypt failure.
- `AbstractOAuth2AuthenticationProvider.authenticate(...)` only converts `GlobalException` into an `OAuth2AuthenticationException`; other exceptions are wrapped into a `RuntimeException`.
## Fix Focus Areas
- Ensure `GlobalException` is rethrown unchanged (do not wrap).
- For non-Global exceptions, wrap into a `SystemException`/`BizException` with an appropriate code/message and preserve the cause (e.g., `new SystemException(code, msg, ex)`), rather than `IllegalArgumentException(ex.getMessage())`.
- Keep the original exception as the cause to preserve stack traces.
### Focus lines
- laokou-service/laokou-auth/laokou-auth-domain/src/main/java/org/laokou/auth/model/AuthA.java[435-504]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+106
to
140
| public static LoginEvent toDomainEvent(HttpServletRequest request, AuthA authA, GlobalException gex) { | ||
| try { | ||
| Capabilities capabilities = RequestUtils.getCapabilities(request); | ||
| String ip = IpUtils.getIpAddr(request); | ||
| int status = LoginStatus.OK.getCode(); | ||
| UserE userE = authA.getUserE(); | ||
| Optional<UserE> optional = Optional.ofNullable(userE); | ||
| Long creator = optional.map(UserE::getId).orElse(null); | ||
| Long tenantId = optional.map(UserE::getTenantId).orElse(null); | ||
| Long deptId = optional.map(UserE::getDeptId).orElse(null); | ||
| String errorMessage = StringConstants.EMPTY; | ||
| if (ObjectUtils.isNotNull(gex)) { | ||
| status = LoginStatus.FAIL.getCode(); | ||
| errorMessage = gex.getMsg(); | ||
| } | ||
| return LoginEvent.builder() | ||
| .id(authA.getId()) | ||
| .username(authA.getLoginName()) | ||
| .ip(ip) | ||
| .address(AddressUtils.getRealAddress(ip)) | ||
| .browser(capabilities.getBrowser()) | ||
| .os(capabilities.getPlatform()) | ||
| .status(status) | ||
| .errorMessage(errorMessage) | ||
| .type(authA.getGrantType().getCode()) | ||
| .loginTime(authA.getCreateTime()) | ||
| .tenantId(tenantId) | ||
| .creator(creator) | ||
| .deptId(deptId) | ||
| .build(); | ||
| } | ||
| catch (InetAddressException | IOException | InterruptedException ex) { | ||
| log.error("错误信息：{}", ex.getMessage(), ex); | ||
| throw new IllegalArgumentException(ex.getMessage()); | ||
| } |
There was a problem hiding this comment.
2. Login depends on ip lookup 🐞 Bug ⛯ Reliability
LoginLogConvertor.toDomainEvent now rethrows IllegalArgumentException on address/IP lookup failures (including InterruptedException), and OAuth2AuthenticationProcessor calls it on the success path before returning the authentication token. A transient geo/IP lookup failure can therefore abort an otherwise successful login and the InterruptedException handling clears the interrupt flag.
Agent Prompt
## Issue description
`LoginLogConvertor.toDomainEvent(...)` throws `IllegalArgumentException` when IP->address resolution fails, and it catches `InterruptedException` without restoring interrupt status. Because the auth processor calls `toDomainEvent` on the success path, a transient lookup failure can break successful authentication.
## Issue Context
- `OAuth2AuthenticationProcessor.authentication(...)` calls `LoginLogConvertor.toDomainEvent(request, authA, null)` before returning the token.
- `AddressUtils.getRealAddress(...)` declares `throws InetAddressException, IOException, InterruptedException`.
## Fix Focus Areas
- Do not throw from `toDomainEvent` for address/capabilities lookup failures; instead:
- Log the failure
- Use safe defaults (e.g., empty/"unknown" address/browser/os)
- Still return a `LoginEvent`
- If `InterruptedException` is caught, call `Thread.currentThread().interrupt()` before continuing/returning.
- Preserve causes if you must rethrow, and prefer a domain-specific `GlobalException` rather than `IllegalArgumentException`.
### Focus lines
- laokou-service/laokou-auth/laokou-auth-infrastructure/src/main/java/org/laokou/auth/convertor/LoginLogConvertor.java[106-140]
- laokou-service/laokou-auth/laokou-auth-infrastructure/src/main/java/org/laokou/auth/config/authentication/OAuth2AuthenticationProcessor.java[48-69]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5910 +/- ##
============================================
- Coverage 58.29% 58.27% -0.03%
- Complexity 1145 1147 +2
============================================
Files 270 270
Lines 5364 5385 +21
Branches 339 339
============================================
+ Hits 3127 3138 +11
- Misses 2061 2071 +10
Partials 176 176 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 by Sourcery
通过在内部处理异常并进行日志记录，同时简化认证流程，改进认证领域和基础设施层的错误处理。
增强内容：
IllegalArgumentException来封装异常。UserDetailsServiceImpl，将异常处理下沉到更底层，并直接从认证结果中返回用户详情。Original summary in English
Summary by Sourcery
Improve auth domain and infrastructure error handling by internalizing exceptions and logging while simplifying authentication flows.
Enhancements:
Summary by CodeRabbit
Refactor
Chores