From 658af4882c3454c9ae1d58b170ed8f6815590dc6 Mon Sep 17 00:00:00 2001 From: jgjesdal Date: Mon, 14 Sep 2026 10:00:22 +0200 Subject: [PATCH] feat(api): every advice answers with a problem, and validation has one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Converts the two wrapper-shaped advices and adds the three that were missing. Controllers still catch most of these locally, so their bodies are unchanged until the next commit removes those catches. BadRequestExceptionHandler and ConcurrencyExceptionHandler now return ProblemDetail. The former's javadoc argued against exactly this, and the argument was sound as stated: controllers catch BadRequestException locally and return the wrapper, so converting the advice alone gives one exception two shapes. That is an argument against changing it in isolation, not against the shape — the local catches go in the same change. ConflictError.cause = "concurrency" becomes the type .../errors/optimistic-lock. It existed so clients could discriminate without string-matching the message, which is what RFC 9457's type member is for. Three new advices. ConstraintViolationException had none, so seventeen controllers caught it themselves — twenty-six blocks calling BuildErrorResponse, which returns a DataWrapper: a success-shaped envelope used as an error body. DuplicateDataException had none, so fourteen controllers read the status out of the payload with HttpStatusCode.valueOf(error.getCode()). MethodArgumentNotValid had none, which is why adding @Valid to the thirty-six unvalidated bodies was held: without a handler it would have introduced a fourth shape rather than removing shapes. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: jgjesdal --- .../errors/BadRequestExceptionHandler.java | 20 ++++++++---- .../errors/ConcurrencyExceptionHandler.java | 15 +++++---- .../ConstraintViolationExceptionHandler.java | 32 +++++++++++++++++++ .../errors/DuplicateDataExceptionHandler.java | 29 +++++++++++++++++ .../api/controllers/errors/Problems.java | 20 ++++++++++++ ...RequestBodyValidationExceptionHandler.java | 32 +++++++++++++++++++ .../controllers/TimeseriesControllerTest.java | 8 +++-- 7 files changed, 140 insertions(+), 16 deletions(-) create mode 100644 datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/ConstraintViolationExceptionHandler.java create mode 100644 datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/DuplicateDataExceptionHandler.java create mode 100644 datahub-api/src/main/java/ai/intellistream/datahub/api/controllers/errors/RequestBodyValidationExceptionHandler.java 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 -----------------------------------------------