diff --git a/src/main/java/org/ohdsi/webapi/mvc/GlobalExceptionHandler.java b/src/main/java/org/ohdsi/webapi/mvc/GlobalExceptionHandler.java index dc4bfddf6..cd56cd108 100644 --- a/src/main/java/org/ohdsi/webapi/mvc/GlobalExceptionHandler.java +++ b/src/main/java/org/ohdsi/webapi/mvc/GlobalExceptionHandler.java @@ -141,6 +141,46 @@ public ResponseEntity handleBadRequestException(Exception ex) { return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(errorMessage); } + /** + * Handle rejected arguments. + * + *

The service layer uses {@link IllegalArgumentException} for conditions + * the caller caused and can act on: a generation, characterization, analysis + * or preset that does not exist. Without a handler these reached the generic + * fallback, which reports 500 and replaces the message with the exception's + * class name, so a request naming a missing generation came back as + * {@code {"message":"An exception occurred: java.lang.IllegalArgumentException"}} + * and the caller could not tell what had been rejected, or that the fault was + * theirs. Reported in OHDSI/Atlas3#291. + * + *

These messages are written by this codebase for exactly this purpose, so + * they are passed through. Everything the service layer does not raise + * deliberately still ends up at the generic handler and is still sanitised. + */ + @ExceptionHandler(IllegalArgumentException.class) + public ResponseEntity handleIllegalArgument(IllegalArgumentException ex) { + logException(ex); + return badRequest(ex); + } + + private static ResponseEntity badRequest(Throwable ex) { + RuntimeException sanitizedException = new RuntimeException(describeRejection(ex)); + sanitizedException.setStackTrace(new StackTraceElement[0]); + return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(new ErrorMessage(sanitizedException)); + } + + /** + * An argument rejection is only worth reporting if it says what was rejected. + * A message-less exception would otherwise produce a null body, which tells + * the caller even less than the class name did. + */ + private static String describeRejection(Throwable ex) { + String message = ex.getMessage(); + return message != null && !message.trim().isEmpty() + ? message + : "Request rejected: " + ex.getClass().getName(); + } + /** * Handle concept not installed exceptions */ @@ -174,9 +214,14 @@ public ResponseEntity handleUndeclaredThrowable(UndeclaredThrowabl status = HttpStatus.BAD_REQUEST; // New exception must be created or direct self-reference exception will be thrown responseException = new RuntimeException(throwable.getMessage()); + } else if (throwable instanceof IllegalArgumentException) { + status = HttpStatus.BAD_REQUEST; + responseException = new RuntimeException(describeRejection(throwable)); } else { status = HttpStatus.INTERNAL_SERVER_ERROR; - responseException = new RuntimeException("An exception occurred: " + ex.getClass().getName()); + // The wrapper's own class name says only that a proxy was involved, + // which is never the exception the caller needs named. + responseException = new RuntimeException("An exception occurred: " + throwable.getClass().getName()); } } else { status = HttpStatus.INTERNAL_SERVER_ERROR; diff --git a/src/test/java/org/ohdsi/webapi/mvc/GlobalExceptionHandlerTest.java b/src/test/java/org/ohdsi/webapi/mvc/GlobalExceptionHandlerTest.java index e89a3dffb..0590297ef 100644 --- a/src/test/java/org/ohdsi/webapi/mvc/GlobalExceptionHandlerTest.java +++ b/src/test/java/org/ohdsi/webapi/mvc/GlobalExceptionHandlerTest.java @@ -3,6 +3,8 @@ import org.junit.Test; import org.springframework.context.ApplicationEventPublisher; import org.springframework.dao.DataIntegrityViolationException; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.UndeclaredThrowableException; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.test.util.ReflectionTestUtils; @@ -93,4 +95,60 @@ public void doesNotTruncateMessagesThatHaveNoDetailSection() { assertEquals(HttpStatus.CONFLICT, response.getStatusCode()); assertTrue(response.getBody().message().startsWith("Violation of UNIQUE KEY")); } + + /** + * The service layer raises IllegalArgumentException for a generation, + * characterization, analysis or preset the caller named but that does not + * exist. With no handler for it these reached the generic fallback, which + * answers 500 and replaces the message with the class name, so the caller + * learned neither what was rejected nor that the fault was theirs + * (OHDSI/Atlas3#291). + */ + @Test + public void reportsARejectedArgumentAsBadRequestAndKeepsItsReason() { + ResponseEntity response = + handler.handleIllegalArgument(new IllegalArgumentException("There is no generation with id = 42.")); + + assertEquals(HttpStatus.BAD_REQUEST, response.getStatusCode()); + assertNotNull(response.getBody()); + assertEquals("There is no generation with id = 42.", response.getBody().message()); + } + + @Test + public void namesTheExceptionWhenARejectedArgumentCarriesNoReason() { + ResponseEntity response = + handler.handleIllegalArgument(new IllegalArgumentException()); + + assertEquals(HttpStatus.BAD_REQUEST, response.getStatusCode()); + assertNotNull(response.getBody()); + assertTrue(response.getBody().message().contains("java.lang.IllegalArgumentException")); + } + + @Test + public void unwrapsARejectedArgumentThatArrivesThroughAProxy() { + UndeclaredThrowableException wrapped = new UndeclaredThrowableException( + new InvocationTargetException(new IllegalArgumentException("Preset analysis with id=7 does not exist"))); + + ResponseEntity response = handler.handleUndeclaredThrowable(wrapped); + + assertEquals(HttpStatus.BAD_REQUEST, response.getStatusCode()); + assertNotNull(response.getBody()); + assertEquals("Preset analysis with id=7 does not exist", response.getBody().message()); + } + + /** + * The wrapper's own class name only says a proxy was involved. Naming the + * exception the caller actually hit is the point of the message. + */ + @Test + public void namesTheUnderlyingExceptionRatherThanTheProxyWrapper() { + UndeclaredThrowableException wrapped = new UndeclaredThrowableException( + new InvocationTargetException(new IllegalStateException("boom"))); + + ResponseEntity response = handler.handleUndeclaredThrowable(wrapped); + + assertEquals(HttpStatus.INTERNAL_SERVER_ERROR, response.getStatusCode()); + assertNotNull(response.getBody()); + assertTrue(response.getBody().message().contains("java.lang.IllegalStateException")); + } }