Skip to content

fix: Update exception handler to use fully qualified class name for A… #239

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
polyglot-k merged 1 commit into from
Aug 7, 2025
Merged

fix: Update exception handler to use fully qualified class name for A… #239

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
4 changes: 2 additions & 2 deletions src/main/java/life/mosu/mosuserver/global/exception/GlobalExceptionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,9 @@ public ResponseEntity<ErrorResponse> handleAuthenticationException(Authenticatio
return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body(response);
}

@ExceptionHandler(AuthenticationException.class)
@ExceptionHandler(life.mosu.mosuserver.global.exception.AuthenticationException.class)
public ResponseEntity<ErrorResponse> handleCustomAuthenticationException(
AuthenticationException ex) {
life.mosu.mosuserver.global.exception.AuthenticationException ex) {
Comment on lines +85 to +87
Copy link

@gemini-code-assist gemini-code-assist bot Aug 7, 2025

Choose a reason for hiding this comment

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

critical

This change correctly wires up the handler for your custom AuthenticationException. However, the handler itself has a critical issue that this change will now expose.

The call to notifyIfNeeded(ex) on line 88 can lead to a NullPointerException. Inside notifyIfNeeded (line 152), the code ex.getCause().toString() is executed without checking if ex.getCause() is null.

Your custom life.mosu.mosuserver.global.exception.AuthenticationException is instantiated without a cause, so ex.getCause() will be null for it. This will crash the exception handler, masking the original error.

This needs to be fixed urgently. The notifyIfNeeded method should be updated to handle null causes gracefully. For example:

// In notifyIfNeeded(Exception ex)
private void notifyIfNeeded(Exception ex) {
    try {
        String cause = ex.getCause() != null ? ex.getCause().toString() : "No cause available";
        DiscordExceptionNotifyEventRequest request = DiscordExceptionNotifyEventRequest.of(
                cause,
                ex.getMessage()
        );
        notifier.send(request);
    } catch (Exception notifyEx) {
        log.error("[Discord Notify Error]", notifyEx);
    }
}

Since this fix is outside the changed lines, please address it in a follow-up PR, but it is critical.

Comment on lines +85 to +87
Copy link

@gemini-code-assist gemini-code-assist bot Aug 7, 2025

Choose a reason for hiding this comment

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

high

While this change is correct, the handler's implementation introduces two maintainability concerns:

  1. Code Duplication: The body of this handler is identical to handleAuthenticationException (lines 73-83). This violates the DRY (Don't Repeat Yourself) principle and makes future changes error-prone.
  2. Lack of Specificity: The handler doesn't use the loginId field from the custom AuthenticationException, missing an opportunity for more detailed error logging/notification.

I recommend refactoring to address both issues. You could extract the common response-building logic into a private helper method. This helper could then check the exception type to include specific details like loginId in notifications.

Addressing this would significantly improve the maintainability and observability of your exception handling. This can be done in a follow-up PR.

notifyIfNeeded(ex);

ErrorResponse response = ErrorResponse.builder()
Expand Down