Skip to content

Fix SQL logging and add catch-all exception handler (continuation w #11988) #12004

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

Closed
nishantharkut wants to merge 3 commits into from
Closed

Fix SQL logging and add catch-all exception handler (continuation w #11988) #12004

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a0551a
Fix SQL logging and add catch-all exception handler
nishantharkut Mar 3, 2026
cc87ac4
Merge branch 'master' into fix/11987-logging-and-catch-all
nishantharkut Mar 3, 2026
1cba704
Extend ResponseEntityExceptionHandler to preserve Spring MVC 4xx hand…
nishantharkut Mar 3, 2026
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
18 changes: 15 additions & 3 deletions src/main/java/org/cbioportal/application/rest/error/GlobalExceptionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import org.cbioportal.legacy.service.exception.SampleNotFoundException;
import org.cbioportal.legacy.service.exception.StudyNotFoundException;
import org.cbioportal.legacy.service.exception.TokenNotFoundException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.TypeMismatchException;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
Expand All @@ -36,12 +38,14 @@
import org.springframework.web.bind.MissingServletRequestParameterException;
import org.springframework.web.bind.annotation.ControllerAdvice;
import org.springframework.web.bind.annotation.ExceptionHandler;
import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler;

// TODO
// - consider extending extends ResponseEntityExceptionHandler
// - check controllers for not catching exceptions themselves
@ControllerAdvice({"org.cbioportal.legacy.web", "org.cbioportal.application.rest.vcolumnstore"})
public class GlobalExceptionHandler {
public class GlobalExceptionHandler extends ResponseEntityExceptionHandler {

private static final Logger LOG = LoggerFactory.getLogger(GlobalExceptionHandler.class);

@ExceptionHandler(UnsupportedOperationException.class)
public ResponseEntity<ErrorResponse> handleUnsupportedOperation() {
Expand Down Expand Up @@ -242,7 +246,7 @@ public ResponseEntity<ErrorResponse> handleResourceDefinitionNotFound(

@ExceptionHandler(BadSqlGrammarException.class)
public ResponseEntity<ErrorResponse> handleBadSqlGrammar(BadSqlGrammarException ex) {
ex.printStackTrace(); // we still want this to show up in the logs
LOG.error("SQL grammar exception", ex);
return new ResponseEntity<>(
new ErrorResponse(
"SQL exception. If you are a maintainer of this instance, see logs for details."),
Expand All @@ -253,4 +257,12 @@ public ResponseEntity<ErrorResponse> handleBadSqlGrammar(BadSqlGrammarException
public ResponseEntity<ErrorResponse> handleNoSuchElementException(NoSuchElementException ex) {
return new ResponseEntity<>(new ErrorResponse(ex.getMessage()), HttpStatus.NOT_FOUND);
}

@ExceptionHandler(Exception.class)
public ResponseEntity<ErrorResponse> handleUncaughtException(Exception ex) {
LOG.error("Unhandled exception", ex);
return new ResponseEntity<>(
new ErrorResponse("An unexpected error occurred. Please contact your administrator."),
HttpStatus.INTERNAL_SERVER_ERROR);
Comment on lines +261 to +266
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

@ExceptionHandler(Exception.class) will also intercept Spring MVC framework exceptions (e.g., HttpRequestMethodNotSupportedException, HttpMediaTypeNotSupportedException, etc.) that would otherwise be translated to correct 4xx/405 responses by Spring’s default resolvers. With this catch-all in place, those cases may start returning a generic 500 ErrorResponse instead. Consider narrowing the handler to RuntimeException (to avoid overriding framework 4xx handling) and/or adding explicit handlers for common Spring MVC exceptions with appropriate HTTP statuses, or extending ResponseEntityExceptionHandler to preserve the default mappings while still returning JSON.

Copilot uses AI. Check for mistakes.
}
}
40 changes: 40 additions & 0 deletions src/test/java/org/cbioportal/application/rest/error/GlobalExceptionHandlerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package org.cbioportal.application.rest.error;

import static org.assertj.core.api.Assertions.assertThat;

import java.sql.SQLException;
import org.junit.Test;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.jdbc.BadSqlGrammarException;

public class GlobalExceptionHandlerTests {

private final GlobalExceptionHandler handler = new GlobalExceptionHandler();

@Test
public void handleBadSqlGrammarReturnsInternalServerError() {
BadSqlGrammarException ex =
new BadSqlGrammarException("task", "SELECT 1", new SQLException("bad sql"));

ResponseEntity<ErrorResponse> response = handler.handleBadSqlGrammar(ex);

assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR);
assertThat(response.getBody()).isNotNull();
assertThat(response.getBody().getMessage())
.isEqualTo(
"SQL exception. If you are a maintainer of this instance, see logs for details.");
}

@Test
public void handleUncaughtExceptionReturnsInternalServerError() {
RuntimeException ex = new RuntimeException("something unexpected");

ResponseEntity<ErrorResponse> response = handler.handleUncaughtException(ex);

assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR);
assertThat(response.getBody()).isNotNull();
assertThat(response.getBody().getMessage())
.isEqualTo("An unexpected error occurred. Please contact your administrator.");
}
}