errors/2 advices

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.
  *
  * <h2>Body shape</h2>
- * 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 <em>in isolation</em>, not
+ * against the shape — so the local catches go in the same change, and the objection with them.
+ *
+ * <p>{@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<ResponseError<BadRequestError>> 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());
     }
 }

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.
  *
- * <p>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.
+ * <p>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<ResponseError<ConflictError>> 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.");
     }
 }

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.
+ *
+ * <p>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 <em>success-shaped</em> 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.
+ *
+ * <p>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());
+    }
+}

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.
+ *
+ * <p>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());
+    }
+}

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.
+     *
+     * <p>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<Map<String, String>> legacyFields) {
+        List<FieldProblem> fields = new ArrayList<>();
+        if (legacyFields != null) {
+            for (Map<String, String> 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.
      *

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.
+ *
+ * <p>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.
+ *
+ * <p>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());
+    }
+}

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 -----------------------------------------------