diff --git a/src/main/java/org/ohdsi/webapi/mvc/GlobalExceptionHandler.java b/src/main/java/org/ohdsi/webapi/mvc/GlobalExceptionHandler.java index 1899a3cef..dc4bfddf6 100644 --- a/src/main/java/org/ohdsi/webapi/mvc/GlobalExceptionHandler.java +++ b/src/main/java/org/ohdsi/webapi/mvc/GlobalExceptionHandler.java @@ -63,15 +63,37 @@ public ResponseEntity handleDatabaseConnectionException(CannotGetJdbcCon public ResponseEntity 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. + * + *

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 handleResourceNotFoundException(Exception ex /** * Handle bad request exceptions */ - @ExceptionHandler(ConceptNotExistException.class) + @ExceptionHandler({ConceptNotExistException.class, BadRequestAtlasException.class}) public ResponseEntity handleBadRequestException(Exception ex) { logException(ex); ex.setStackTrace(new StackTraceElement[0]); diff --git a/src/main/java/org/ohdsi/webapi/tag/TagService.java b/src/main/java/org/ohdsi/webapi/tag/TagService.java index e96cc32ae..62fe7717b 100644 --- a/src/main/java/org/ohdsi/webapi/tag/TagService.java +++ b/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 groupIds = tag.getGroups().stream() .map(Tag::getId) .collect(Collectors.toList()); List groups = findByIdIn(groupIds); - boolean allowCustom = groups.stream() - .filter(Tag::isAllowCustom) - .count() == groups.size(); + List 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)); diff --git a/src/test/java/org/ohdsi/webapi/mvc/GlobalExceptionHandlerTest.java b/src/test/java/org/ohdsi/webapi/mvc/GlobalExceptionHandlerTest.java new file mode 100644 index 000000000..e89a3dffb --- /dev/null +++ b/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 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 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 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 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 response = handler.handleDataIntegrityViolation(ex); + + assertEquals(HttpStatus.CONFLICT, response.getStatusCode()); + assertTrue(response.getBody().message().startsWith("Violation of UNIQUE KEY")); + } +}