From 593015198654bdc8906fe41e0faa8f391758caf3 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Sun, 30 Aug 2026 22:32:52 +0800 Subject: [PATCH] Report a rejected argument as 400 with its reason, not an opaque 500 The service layer raises IllegalArgumentException for conditions the caller caused and can act on: a generation, characterization, analysis or preset named in the request that does not exist. CcServiceImpl alone does this in eighteen places, each with a message saying exactly what was not found. None of it reached the caller. IllegalArgumentException had no handler, so it fell to the generic fallback, which answers 500 and replaces the message with the exception's class name. Fetching results for a characterization whose preset analysis no longer resolves came back as {"message":"An exception occurred: java.lang.IllegalArgumentException"} which says neither what was rejected nor that the fault lay with the request. That is what OHDSI/Atlas3#291 ran into: the reporter could not tell whether the generation had failed or only the results retrieval, and had to read the network tab to get even the class name. Handle it as a 400 carrying the message the service layer already wrote, and unwrap it when it arrives through a proxy. These messages exist to be read; everything not raised deliberately still reaches the generic handler and is still sanitised. An exception with no message at all falls back to naming its class, which is no worse than before. Separately, the proxy branch reported the wrapper's own class name rather than the exception inside it, so an unexpected failure through a proxy was always described as UndeclaredThrowableException. Reported in OHDSI/Atlas3#291. --- .../webapi/mvc/GlobalExceptionHandler.java | 47 ++++++++++++++- .../mvc/GlobalExceptionHandlerTest.java | 58 +++++++++++++++++++ 2 files changed, 104 insertions(+), 1 deletion(-) 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")); + } }