From dc65ceb7769d8b3028a920a14b71b7a8eeecc881 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 14 Aug 2026 09:53:53 +0800 Subject: [PATCH] Return 409/400 instead of 500 when tag creation is rejected Creating a tag returned a 500 with no usable message in two situations, both of which are ordinary user error and should be reported as such. The constraint-violation handler assumed the cause chain was exactly two levels deep, that the inner cause carried a message, and that the message contained a PostgreSQL "Detail: " section. Any of those assumptions failing threw a NullPointerException out of the handler itself, so Spring reported a 500 rather than the 409 the handler was written to return. When the section was simply absent, indexOf returned -1 and substring silently dropped the first seven characters of the message instead. This matters for tags because there is a unique index on lower(name) across the whole tags table, which holds tag groups as well, so a name that collides with any existing tag or group lands in this handler. Walk the cause chain defensively, only strip the "Detail: " prefix when it is actually present, and fall back to the most specific cause. Separately, rejecting a tag whose group does not allow custom tags threw IllegalArgumentException, which fell through to the generic handler and became a 500 reading "An exception occurred: java.lang.IllegalArgumentException". Throw BadRequestAtlasException instead and register it on the existing bad-request handler, which already treated it as a 400 when it arrived wrapped in an UndeclaredThrowableException. The message now names the groups that refused the tag, since the client cannot otherwise tell which of the selected groups was the problem. Creating a tag with no groups at all NPEd on the same path and is now a 400 too. Reported in OHDSI/Atlas3#211. --- .../webapi/mvc/GlobalExceptionHandler.java | 30 +++++- .../java/org/ohdsi/webapi/tag/TagService.java | 19 +++- .../mvc/GlobalExceptionHandlerTest.java | 96 +++++++++++++++++++ 3 files changed, 136 insertions(+), 9 deletions(-) create mode 100644 src/test/java/org/ohdsi/webapi/mvc/GlobalExceptionHandlerTest.java 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")); + } +}