diff --git a/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/BadRequestExceptionHandler.java b/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/BadRequestExceptionHandler.java index 9995f06e..7046373d 100644 --- a/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/BadRequestExceptionHandler.java +++ b/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/BadRequestExceptionHandler.java @@ -1,6 +1,7 @@ // SPDX-License-Identifier: AGPL-3.0-or-later package ai.intellistream.datahub.api.controllers.errors; +import org.springframework.http.ProblemDetail; import ai.intellistream.datahub.errors.ResponseError; import lombok.extern.slf4j.Slf4j; import org.springframework.http.HttpStatus; @@ -24,20 +25,25 @@ * rediscover it. An advice is the one place that cannot be forgotten by a new endpoint. * *

Body shape

- * The exception's own {@link ResponseError} payload, so a 400 from {@code /filter} looks like a 400 - * from {@code /create} — the controllers that catch this locally return exactly that. Deliberately - * not the RFC 9457 {@code ProblemDetail} that {@link ObjectNotFoundExceptionHandler} and friends - * use: those cover exceptions with no body of their own, and switching this one would give the same - * exception two shapes depending on which endpoint raised it. + * RFC 9457, like every other advice. This used to return the exception's own {@code ResponseError} + * payload, and the reasoning was sound at the time: controllers catch this locally and return + * exactly that, so converting the advice alone would have given one exception two shapes depending + * on which endpoint raised it. That argument was against changing it in isolation, not + * against the shape — so the local catches go in the same change, and the objection with them. + * + *

{@code message} becomes {@code detail} and {@code fields} becomes the {@code fields} + * extension; {@code code} is dropped because it duplicated the HTTP status it was sent alongside. */ @RestControllerAdvice @Slf4j public class BadRequestExceptionHandler { @ExceptionHandler(BadRequestException.class) - public ResponseEntity> handle(BadRequestException ex) { + public ProblemDetail handle(BadRequestException ex) { BadRequestError error = ex.getError() == null ? null : ex.getError().getError(); log.debug("Rejecting request: {}", error == null ? "no detail" : error.getMessage()); - return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(ex.getError()); + return Problems.badRequest( + error == null ? null : error.getMessage(), + error == null ? null : error.getFields()); } } diff --git a/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/ConcurrencyExceptionHandler.java b/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/ConcurrencyExceptionHandler.java index d5e4ec1c..0603abfc 100644 --- a/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/ConcurrencyExceptionHandler.java +++ b/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/ConcurrencyExceptionHandler.java @@ -1,6 +1,7 @@ // SPDX-License-Identifier: AGPL-3.0-or-later package ai.intellistream.datahub.api.controllers.errors; +import org.springframework.http.ProblemDetail; import ai.intellistream.datahub.errors.ResponseError; import lombok.extern.slf4j.Slf4j; import org.springframework.dao.OptimisticLockingFailureException; @@ -21,20 +22,20 @@ * more specific {@code ObjectOptimisticLockingFailureException}) so any future lock-flavor * Spring Data might throw is also covered. * - *

The response shape is {@link ConflictError} so it shows up as a concrete schema in the - * OpenAPI spec — clients can discriminate on {@code cause = "concurrency"} instead of - * string-matching the message. + *

Clients discriminate on the problem {@code type} — {@code .../errors/optimistic-lock} — + * rather than string-matching the message. That was already the intent: {@code ConflictError} + * carried {@code cause = "concurrency"} for it. A type URI is the RFC 9457 member meant for the + * job, so a lock conflict is now distinguishable from a duplicate one without a bespoke field. */ @RestControllerAdvice @Slf4j public class ConcurrencyExceptionHandler { @ExceptionHandler(OptimisticLockingFailureException.class) - public ResponseEntity> handleOptimisticLock(OptimisticLockingFailureException ex) { + public ProblemDetail handleOptimisticLock(OptimisticLockingFailureException ex) { // Log at info — this is expected under contention and not an operator alert. log.info("Optimistic lock conflict: {}", ex.getMessage()); - return ResponseEntity.status(HttpStatus.CONFLICT).body( - ConflictError.of("The resource was modified or removed by another request. Re-read and retry.") - ); + return Problems.conflict(Problems.OPTIMISTIC_LOCK, + "The resource was modified or removed by another request. Re-read and retry."); } } diff --git a/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/ConstraintViolationExceptionHandler.java b/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/ConstraintViolationExceptionHandler.java new file mode 100644 index 00000000..7681be3c --- /dev/null +++ b/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/ConstraintViolationExceptionHandler.java @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +package ai.intellistream.datahub.api.controllers.errors; + +import jakarta.validation.ConstraintViolationException; +import lombok.extern.slf4j.Slf4j; +import org.springframework.http.ProblemDetail; +import org.springframework.web.bind.annotation.ExceptionHandler; +import org.springframework.web.bind.annotation.RestControllerAdvice; + +/** + * Bean-validation failures raised inside a service, as a 400. + * + *

There was no advice for this, so all seventeen controllers caught it themselves — twenty-six + * {@code catch (ConstraintViolationException)} blocks, each calling {@code BuildErrorResponse} and + * getting back a {@code DataWrapper}: a success-shaped envelope used as an error body. + * Two of them ({@code ResourceController.filter}, {@code DataSetController.filter}) returned + * {@code e.getMessage()} as a bare string instead, so the same failure had two shapes depending on + * which endpoint produced it. + * + *

With this in place those catches are redundant and can go, which is what makes the shape + * uniform rather than merely defined. + */ +@RestControllerAdvice +@Slf4j +public class ConstraintViolationExceptionHandler { + + @ExceptionHandler(ConstraintViolationException.class) + public ProblemDetail handle(ConstraintViolationException ex) { + log.debug("Rejecting request: {} constraint violation(s)", ex.getConstraintViolations().size()); + return Problems.constraintViolation(ex.getConstraintViolations()); + } +} diff --git a/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/DuplicateDataExceptionHandler.java b/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/DuplicateDataExceptionHandler.java new file mode 100644 index 00000000..471b268a --- /dev/null +++ b/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/DuplicateDataExceptionHandler.java @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +package ai.intellistream.datahub.api.controllers.errors; + +import lombok.extern.slf4j.Slf4j; +import org.springframework.http.ProblemDetail; +import org.springframework.web.bind.annotation.ExceptionHandler; +import org.springframework.web.bind.annotation.RestControllerAdvice; + +import java.util.List; + +/** + * A taken external id, as a 409 naming what collided. + * + *

Fourteen controllers caught this themselves and read the status back out of the payload — + * {@code HttpStatusCode.valueOf(dupError.getError().getCode())} — where {@code code} was a field on + * the body defaulting to 409. The status belongs on the response, not inside it. + */ +@RestControllerAdvice +@Slf4j +public class DuplicateDataExceptionHandler { + + @ExceptionHandler(DuplicateDataException.class) + public ProblemDetail handle(DuplicateDataException ex) { + DuplicateError error = ex.getError() == null ? null : ex.getError().getError(); + String detail = error == null ? "Already exists." : error.getMessage(); + log.debug("Rejecting duplicate: {}", detail); + return Problems.duplicate(detail, error == null ? List.of() : error.getDuplicated()); + } +} diff --git a/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/Problems.java b/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/Problems.java index 08128209..271f4e94 100644 --- a/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/Problems.java +++ b/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/Problems.java @@ -59,6 +59,8 @@ public final class Problems { public static final URI DUPLICATE = type("duplicate"); public static final URI CONFLICT = type("conflict"); public static final URI CONSTRAINT_VIOLATION = type("constraint-violation"); + public static final URI OPTIMISTIC_LOCK = type("optimistic-lock"); + public static final URI BAD_REQUEST = type("bad-request"); private Problems() { } @@ -178,6 +180,24 @@ private static Object firstOrList(Object[] arguments) { return arguments.length == 1 ? arguments[0] : Arrays.asList(arguments); } + /** + * A 400 carrying the loose {@code field -> message} pairs the old {@code BadRequestError} used. + * + *

Those entries are not uniform — some are {@code externalId -> "must not be blank"}, others + * {@code "DataSet.Id" -> "5"} — so they become a field and a message and nothing is invented. + * New throw sites should build {@link FieldProblem}s directly and get a code and a rejected + * value with them; this is the bridge for the ones that already exist. + */ + public static ProblemDetail badRequest(String detail, Collection> legacyFields) { + List fields = new ArrayList<>(); + if (legacyFields != null) { + for (Map entry : legacyFields) { + entry.forEach((field, message) -> fields.add(new FieldProblem(field, message, null, null))); + } + } + return withFields(of(HttpStatus.BAD_REQUEST, BAD_REQUEST, "Bad Request", detail), fields); + } + /** * A 409 for something that already exists. * diff --git a/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/RequestBodyValidationExceptionHandler.java b/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/RequestBodyValidationExceptionHandler.java new file mode 100644 index 00000000..224ed145 --- /dev/null +++ b/datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/RequestBodyValidationExceptionHandler.java @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +package ai.intellistream.datahub.api.controllers.errors; + +import lombok.extern.slf4j.Slf4j; +import org.springframework.http.ProblemDetail; +import org.springframework.web.bind.MethodArgumentNotValidException; +import org.springframework.web.bind.annotation.ExceptionHandler; +import org.springframework.web.bind.annotation.RestControllerAdvice; + +/** + * {@code @Valid} failures on a request body, as a 400 — the same shape as one raised in a service. + * + *

Which layer caught a rule is an implementation detail. A caller fixing their request should + * not have to parse one shape when the check ran during binding and another when it ran in a + * service, so this renders exactly what {@link ConstraintViolationExceptionHandler} does. + * + *

This advice is also what makes {@code @Valid} adoptable. Thirty-six handler parameters bind a + * body with no {@code @Valid}, so their constraints never run and {@code DataWrapper}'s batch cap + * is unenforced. Adding the annotation without this handler would have swapped those endpoints' + * error bodies for Spring's default and left the API with a fourth shape, which is why that work + * was held until this existed. + */ +@RestControllerAdvice +@Slf4j +public class RequestBodyValidationExceptionHandler { + + @ExceptionHandler(MethodArgumentNotValidException.class) + public ProblemDetail handle(MethodArgumentNotValidException ex) { + log.debug("Rejecting request body: {} error(s)", ex.getAllErrors().size()); + return Problems.bindingFailure(ex.getAllErrors()); + } +} diff --git a/datahub-api/src/test/java/ai/intellistream/datahub/api/controllers/TimeseriesControllerTest.java b/datahub-api/src/test/java/ai/intellistream/datahub/api/controllers/TimeseriesControllerTest.java index 508e9392..8c54b545 100644 --- a/datahub-api/src/test/java/ai/intellistream/datahub/api/controllers/TimeseriesControllerTest.java +++ b/datahub-api/src/test/java/ai/intellistream/datahub/api/controllers/TimeseriesControllerTest.java @@ -185,8 +185,12 @@ void update_optimisticLockConflict_returns409_viaConcurrencyAdvice() throws Exce .accept(MediaType.APPLICATION_JSON) .content("{\"items\":[{\"externalId\":\"sensor_temp_room_a\"}]}")) .andExpect(status().isConflict()) - .andExpect(jsonPath("$.error.code").value(409)) - .andExpect(jsonPath("$.error.cause").value("concurrency")); + // RFC 9457: the type is the discriminator that ConflictError.cause = "concurrency" + // used to be, and the status lives on the response rather than inside the body. + .andExpect(jsonPath("$.type").value("https://intellistream.ai/errors/optimistic-lock")) + .andExpect(jsonPath("$.title").value("Conflict")) + .andExpect(jsonPath("$.status").value(409)) + .andExpect(jsonPath("$.error").doesNotExist()); } // --- 400: delete blocked by a subscription -----------------------------------------------