Return 409/400 instead of 500 when tag creation is rejected

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

@@ -63,15 +63,37 @@ public ResponseEntity<String> handleDatabaseConnectionException(CannotGetJdbcCon
     public ResponseEntity<ErrorMessage> handleDataIntegrityViolation(DataIntegrityViolationException ex) {
         logException(ex);
 
-        String cause = ex.getCause().getCause().getMessage();
-        cause = cause.substring(cause.indexOf(DETAIL) + DETAIL.length());
-        RuntimeException sanitizedException = new RuntimeException(cause);
+        RuntimeException sanitizedException = new RuntimeException(describeConstraintViolation(ex));
         sanitizedException.setStackTrace(new StackTraceElement[0]);
 
         ErrorMessage errorMessage = new ErrorMessage(sanitizedException);
         return ResponseEntity.status(HttpStatus.CONFLICT).body(errorMessage);
     }
 
+    /**
+     * Pull a human-readable reason out of a constraint violation.
+     *
+     * <p>The cause chain is not guaranteed to be two levels deep, nor to carry a
+     * message, and only PostgreSQL appends a "Detail: " section. Assuming any of
+     * that threw a NullPointerException (or silently truncated the message by
+     * seven characters when {@code indexOf} returned -1) from inside the handler,
+     * which Spring then reported as a 500 instead of the intended 409.
+     */
+    private static String describeConstraintViolation(DataIntegrityViolationException ex) {
+        for (Throwable t = ex; t != null; t = t.getCause()) {
+            String message = t.getMessage();
+            if (message == null) {
+                continue;
+            }
+            int detailIndex = message.indexOf(DETAIL);
+            if (detailIndex >= 0) {
+                return message.substring(detailIndex + DETAIL.length());
+            }
+        }
+        String message = ex.getMostSpecificCause().getMessage();
+        return message != null ? message : "The request conflicts with existing data.";
+    }
+
     /**
      * Handle authorization/permission exceptions
      */
@@ -111,7 +133,7 @@ public ResponseEntity<ErrorMessage> handleResourceNotFoundException(Exception ex
     /**
      * Handle bad request exceptions
      */
-    @ExceptionHandler(ConceptNotExistException.class)
+    @ExceptionHandler({ConceptNotExistException.class, BadRequestAtlasException.class})
     public ResponseEntity<ErrorMessage> handleBadRequestException(Exception ex) {
         logException(ex);
         ex.setStackTrace(new StackTraceElement[0]);

src/main/java/org/ohdsi/webapi/tag/TagService.java

@@ -1,6 +1,7 @@
 package org.ohdsi.webapi.tag;
 
 import org.apache.commons.lang3.StringUtils;
+import org.ohdsi.webapi.exception.BadRequestAtlasException;
 import org.ohdsi.webapi.security.authz.AuthorizationService;
 import org.ohdsi.webapi.service.AbstractDaoService;
 import org.ohdsi.webapi.tag.domain.Tag;
@@ -72,16 +73,24 @@ public TagDTO create(@RequestBody TagDTO dto) {
 
     public Tag create(Tag tag) {
         tag.setType(TagType.CUSTOM);
+        if (tag.getGroups() == null || tag.getGroups().isEmpty()) {
+            throw new BadRequestAtlasException("A tag must be assigned to at least one tag group.");
+        }
         List<Integer> groupIds = tag.getGroups().stream()
                 .map(Tag::getId)
                 .collect(Collectors.toList());
         List<Tag> groups = findByIdIn(groupIds);
-        boolean allowCustom = groups.stream()
-                .filter(Tag::isAllowCustom)
-                .count() == groups.size();
+        List<String> rejectingGroups = groups.stream()
+                .filter(group -> !group.isAllowCustom())
+                .map(Tag::getName)
+                .collect(Collectors.toList());
 
-        if (!allowCustom) {
-            throw new IllegalArgumentException("Tag can be added only to groups that allows to do it");
+        if (!rejectingGroups.isEmpty()) {
+            // Naming the groups matters: the client cannot otherwise tell which
+            // of the selected groups refused the tag.
+            throw new BadRequestAtlasException(
+                    "Tags cannot be added to these groups because they do not allow custom tags: "
+                            + String.join(", ", rejectingGroups));
         }
 
         tag.setGroups(new HashSet<>(groups));

src/test/java/org/ohdsi/webapi/mvc/GlobalExceptionHandlerTest.java

@@ -0,0 +1,96 @@
+package org.ohdsi.webapi.mvc;
+
+import org.junit.Test;
+import org.springframework.context.ApplicationEventPublisher;
+import org.springframework.dao.DataIntegrityViolationException;
+import org.springframework.http.HttpStatus;
+import org.springframework.http.ResponseEntity;
+import org.springframework.test.util.ReflectionTestUtils;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * The constraint-violation branch used to assume a two-level cause chain, a
+ * non-null message on it, and a PostgreSQL "Detail: " section. Creating a tag
+ * whose name collides with an existing tag or tag group hit that branch and
+ * threw a NullPointerException out of the handler, so the client saw a 500
+ * instead of the intended 409 (OHDSI/Atlas3#211).
+ */
+public class GlobalExceptionHandlerTest {
+
+    private final GlobalExceptionHandler handler = newHandler();
+
+    private static GlobalExceptionHandler newHandler() {
+        GlobalExceptionHandler handler = new GlobalExceptionHandler();
+        ApplicationEventPublisher noopPublisher = event -> { };
+        ReflectionTestUtils.setField(handler, "eventPublisher", noopPublisher);
+        return handler;
+    }
+
+    @Test
+    public void returnsConflictAndTheDetailSectionWhenPostgresSuppliesOne() {
+        DataIntegrityViolationException ex = new DataIntegrityViolationException(
+                "could not execute statement",
+                new RuntimeException("constraint violation",
+                        new RuntimeException("ERROR: duplicate key value violates unique constraint "
+                                + "\"tags_name_idx\"\n  Detail: Key (lower(name))=(my tag) already exists.")));
+
+        ResponseEntity<GlobalExceptionHandler.ErrorMessage> response = handler.handleDataIntegrityViolation(ex);
+
+        assertEquals(HttpStatus.CONFLICT, response.getStatusCode());
+        assertEquals("Key (lower(name))=(my tag) already exists.", response.getBody().message());
+    }
+
+    @Test
+    public void returnsConflictWhenTheViolationHasNoCauseAtAll() {
+        DataIntegrityViolationException ex =
+                new DataIntegrityViolationException("could not execute statement");
+
+        ResponseEntity<GlobalExceptionHandler.ErrorMessage> response = handler.handleDataIntegrityViolation(ex);
+
+        assertEquals(HttpStatus.CONFLICT, response.getStatusCode());
+        assertEquals("could not execute statement", response.getBody().message());
+    }
+
+    @Test
+    public void returnsConflictWhenTheCauseChainIsOnlyOneLevelDeep() {
+        DataIntegrityViolationException ex = new DataIntegrityViolationException(
+                "could not execute statement", new RuntimeException("unique constraint violated"));
+
+        ResponseEntity<GlobalExceptionHandler.ErrorMessage> response = handler.handleDataIntegrityViolation(ex);
+
+        assertEquals(HttpStatus.CONFLICT, response.getStatusCode());
+        assertEquals("unique constraint violated", response.getBody().message());
+    }
+
+    @Test
+    public void returnsConflictWhenTheCauseCarriesNoMessage() {
+        DataIntegrityViolationException ex = new DataIntegrityViolationException(
+                "could not execute statement", new RuntimeException(new RuntimeException()));
+
+        ResponseEntity<GlobalExceptionHandler.ErrorMessage> response = handler.handleDataIntegrityViolation(ex);
+
+        assertEquals(HttpStatus.CONFLICT, response.getStatusCode());
+        assertNotNull(response.getBody().message());
+    }
+
+    /**
+     * Databases other than PostgreSQL do not emit a "Detail: " section. The old
+     * code fed indexOf's -1 straight into substring and silently dropped the
+     * first seven characters of the message.
+     */
+    @Test
+    public void doesNotTruncateMessagesThatHaveNoDetailSection() {
+        DataIntegrityViolationException ex = new DataIntegrityViolationException(
+                "could not execute statement",
+                new RuntimeException(new RuntimeException(
+                        "Violation of UNIQUE KEY constraint 'tags_name_idx'.")));
+
+        ResponseEntity<GlobalExceptionHandler.ErrorMessage> response = handler.handleDataIntegrityViolation(ex);
+
+        assertEquals(HttpStatus.CONFLICT, response.getStatusCode());
+        assertTrue(response.getBody().message().startsWith("Violation of UNIQUE KEY"));
+    }
+}