Report a rejected argument as 400 with its reason, not an opaque 500

src/main/java/org/ohdsi/webapi/mvc/GlobalExceptionHandler.java

@@ -141,6 +141,46 @@ public ResponseEntity<ErrorMessage> handleBadRequestException(Exception ex) {
         return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(errorMessage);
     }
 
+    /**
+     * Handle rejected arguments.
+     *
+     * <p>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.
+     *
+     * <p>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<ErrorMessage> handleIllegalArgument(IllegalArgumentException ex) {
+        logException(ex);
+        return badRequest(ex);
+    }
+
+    private static ResponseEntity<ErrorMessage> 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<ErrorMessage> 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;

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<GlobalExceptionHandler.ErrorMessage> 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<GlobalExceptionHandler.ErrorMessage> 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<GlobalExceptionHandler.ErrorMessage> 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<GlobalExceptionHandler.ErrorMessage> response = handler.handleUndeclaredThrowable(wrapped);
+
+        assertEquals(HttpStatus.INTERNAL_SERVER_ERROR, response.getStatusCode());
+        assertNotNull(response.getBody());
+        assertTrue(response.getBody().message().contains("java.lang.IllegalStateException"));
+    }
 }