From a89e037dd41b9096271425099701b5011effb804 Mon Sep 17 00:00:00 2001 From: mkuchenbecker Date: Thu, 21 May 2026 16:36:45 -0700 Subject: [PATCH 1/5] feat(optimizer): require limit on list-API endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four list-style endpoints now require a caller-supplied limit. No server-side default, no max — getting the API contract right comes first; bounds can land separately. - TableOperationsController.listTableOperations: @RequestParam int limit - TableStatsController.listTableStats: @RequestParam int limit - TableStatsController.getStatsHistory: drop defaultValue="100" - TableOperationsHistoryController.getHistory: drop defaultValue="100" - OptimizerDataService: listTableOperations / listTableStats gain int limit - OptimizerDataServiceImpl: drop @Value("${optimizer.repo.default-limit}") and the defaultLimit field; thread caller-supplied limit straight to PageRequest.of(0, limit), which cascades to MySQL LIMIT n. - application.properties: remove now-unused optimizer.repo.default-limit. Co-Authored-By: Claude Opus 4.7 --- .../controller/TableOperationsController.java | 10 ++++++---- .../TableOperationsHistoryController.java | 7 +++++-- .../api/controller/TableStatsController.java | 16 +++++++++------- .../optimizer/service/OptimizerDataService.java | 16 ++++++++++------ .../service/OptimizerDataServiceImpl.java | 16 ++++++++-------- .../src/main/resources/application.properties | 2 -- .../service/OptimizerDataServiceImplTest.java | 3 ++- 7 files changed, 40 insertions(+), 30 deletions(-) diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsController.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsController.java index c28002bf7..5db7d31ed 100644 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsController.java +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsController.java @@ -59,8 +59,8 @@ public ResponseEntity getTableOperation(@PathVariable String id } /** - * List operations matching the given filters. All parameters are optional — omit all to return - * every row. + * List operations matching the given filters, capped at {@code limit} rows. Every filter is + * optional; {@code limit} is required so callers always state how much they want back. */ @GetMapping public ResponseEntity> listTableOperations( @@ -68,7 +68,8 @@ public ResponseEntity> listTableOperations( @RequestParam(required = false) OperationStatus status, @RequestParam(required = false) String databaseName, @RequestParam(required = false) String tableName, - @RequestParam(required = false) String tableUuid) { + @RequestParam(required = false) String tableUuid, + @RequestParam int limit) { List result = service .listTableOperations( @@ -76,7 +77,8 @@ public ResponseEntity> listTableOperations( Optional.ofNullable(status).map(OperationStatus::toModel), Optional.ofNullable(databaseName), Optional.ofNullable(tableName), - Optional.ofNullable(tableUuid)) + Optional.ofNullable(tableUuid), + limit) .stream() .map(TableOperations::fromModel) .collect(Collectors.toList()); diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsHistoryController.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsHistoryController.java index 36c422623..9a1b6d303 100644 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsHistoryController.java +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsHistoryController.java @@ -31,10 +31,13 @@ public ResponseEntity appendHistory( .body(TableOperationsHistory.fromModel(service.appendHistory(dto.toModel()))); } - /** Return the most recent history for a table, newest first, up to {@code limit} rows. */ + /** + * Return the most recent history for a table, newest first, capped at {@code limit} rows. {@code + * limit} is required. + */ @GetMapping("/{tableUuid}") public ResponseEntity> getHistory( - @PathVariable String tableUuid, @RequestParam(defaultValue = "100") int limit) { + @PathVariable String tableUuid, @RequestParam int limit) { List result = service.getHistory(tableUuid, limit).stream() .map(TableOperationsHistory::fromModel) diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableStatsController.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableStatsController.java index 7cb745250..049516110 100644 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableStatsController.java +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableStatsController.java @@ -48,20 +48,22 @@ public ResponseEntity getTableStats(@PathVariable String tableUuid) } /** - * List stats rows matching the given filters. All parameters are optional — omit all to return - * every row. + * List stats rows matching the given filters, capped at {@code limit} rows. Every filter is + * optional; {@code limit} is required so callers always state how much they want back. */ @GetMapping public ResponseEntity> listTableStats( @RequestParam(required = false) String databaseName, @RequestParam(required = false) String tableName, - @RequestParam(required = false) String tableUuid) { + @RequestParam(required = false) String tableUuid, + @RequestParam int limit) { List result = service .listTableStats( Optional.ofNullable(databaseName), Optional.ofNullable(tableName), - Optional.ofNullable(tableUuid)) + Optional.ofNullable(tableUuid), + limit) .stream() .map(TableStats::fromModel) .collect(Collectors.toList()); @@ -69,14 +71,14 @@ public ResponseEntity> listTableStats( } /** - * Return per-commit stats history for {@code tableUuid}, newest first. Optionally filter by - * {@code since} (inclusive) and cap at {@code limit} rows. + * Return per-commit stats history for {@code tableUuid}, newest first, capped at {@code limit} + * rows. Optional {@code since} filter (inclusive). {@code limit} is required. */ @GetMapping("/{tableUuid}/history") public ResponseEntity> getStatsHistory( @PathVariable String tableUuid, @RequestParam(required = false) Instant since, - @RequestParam(defaultValue = "100") int limit) { + @RequestParam int limit) { List result = service.getStatsHistory(tableUuid, Optional.ofNullable(since), limit).stream() .map(TableStatsHistory::fromModel) diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/service/OptimizerDataService.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/service/OptimizerDataService.java index 0529d3608..c20ae7bf2 100644 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/service/OptimizerDataService.java +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/service/OptimizerDataService.java @@ -23,15 +23,16 @@ public interface OptimizerDataService { // --- TableOperations --- /** - * List operations matching the given filters. Every parameter is optional — pass {@link - * Optional#empty()} to skip that filter. No filters returns all rows. + * List operations matching the given filters, capped at {@code limit} rows. Every filter + * parameter is optional — pass {@link Optional#empty()} to skip that filter. */ List listTableOperations( Optional operationType, Optional status, Optional databaseName, Optional tableName, - Optional tableUuid); + Optional tableUuid, + int limit); /** * Update an operation by writing a history entry. Looks up the operation row by {@code @@ -60,11 +61,14 @@ List listTableOperations( Optional getTableStats(String tableUuid); /** - * List stats rows matching the given filters. Every parameter is optional — pass {@link - * Optional#empty()} to skip that filter. No filters returns all rows. + * List stats rows matching the given filters, capped at {@code limit} rows. Every filter + * parameter is optional — pass {@link Optional#empty()} to skip that filter. */ List listTableStats( - Optional databaseName, Optional tableName, Optional tableUuid); + Optional databaseName, + Optional tableName, + Optional tableUuid, + int limit); /** * Return per-commit stats history for {@code tableUuid}, newest first. diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/service/OptimizerDataServiceImpl.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/service/OptimizerDataServiceImpl.java index 4f820e1b8..29fd0eeee 100644 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/service/OptimizerDataServiceImpl.java +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/service/OptimizerDataServiceImpl.java @@ -19,7 +19,6 @@ import java.util.UUID; import java.util.stream.Collectors; import lombok.RequiredArgsConstructor; -import org.springframework.beans.factory.annotation.Value; import org.springframework.data.domain.PageRequest; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; @@ -40,9 +39,6 @@ public class OptimizerDataServiceImpl implements OptimizerDataService { private final TableStatsRepository statsRepository; private final TableStatsHistoryRepository statsHistoryRepository; - @Value("${optimizer.repo.default-limit:10000}") - private int defaultLimit; - // --- TableOperations --- @Override @@ -51,7 +47,8 @@ public List listTableOperations( Optional status, Optional databaseName, Optional tableName, - Optional tableUuid) { + Optional tableUuid, + int limit) { return operationsRepository .find( operationType.map(OperationTypeDto::toDb), @@ -61,7 +58,7 @@ public List listTableOperations( tableName, Optional.empty(), Optional.empty(), - PageRequest.of(0, defaultLimit)) + PageRequest.of(0, limit)) .stream() .map(TableOperationDto::fromRow) .collect(Collectors.toList()); @@ -137,8 +134,11 @@ public Optional getTableStats(String tableUuid) { @Override public List listTableStats( - Optional databaseName, Optional tableName, Optional tableUuid) { - return statsRepository.find(databaseName, tableName, tableUuid, PageRequest.of(0, defaultLimit)) + Optional databaseName, + Optional tableName, + Optional tableUuid, + int limit) { + return statsRepository.find(databaseName, tableName, tableUuid, PageRequest.of(0, limit)) .stream() .map(TableStatsDto::fromRow) .collect(Collectors.toList()); diff --git a/services/optimizer/src/main/resources/application.properties b/services/optimizer/src/main/resources/application.properties index 1b7eb1a40..e78745d00 100644 --- a/services/optimizer/src/main/resources/application.properties +++ b/services/optimizer/src/main/resources/application.properties @@ -16,7 +16,5 @@ spring.datasource.username=${OPTIMIZER_DB_USER:oh_user} spring.datasource.password=${OPTIMIZER_DB_PASSWORD:oh_password} spring.datasource.hikari.maximum-pool-size=20 -optimizer.repo.default-limit=10000 - management.endpoints.web.exposure.include=health,prometheus management.endpoint.health.enabled=true diff --git a/services/optimizer/src/test/java/com/linkedin/openhouse/optimizer/service/OptimizerDataServiceImplTest.java b/services/optimizer/src/test/java/com/linkedin/openhouse/optimizer/service/OptimizerDataServiceImplTest.java index 8db14c4d6..2a3c1e676 100644 --- a/services/optimizer/src/test/java/com/linkedin/openhouse/optimizer/service/OptimizerDataServiceImplTest.java +++ b/services/optimizer/src/test/java/com/linkedin/openhouse/optimizer/service/OptimizerDataServiceImplTest.java @@ -165,7 +165,8 @@ void listTableOperations_filtersByOperationTypeAndStatus() { Optional.of(OperationStatusDto.PENDING), Optional.empty(), Optional.empty(), - Optional.empty())) + Optional.empty(), + 100)) .extracting(op -> op.getId()) .containsExactly(pendingId); } From 1e361afc3647c4b4570f828daeba73491ba0647c Mon Sep 17 00:00:00 2001 From: mkuchenbecker Date: Fri, 22 May 2026 10:11:53 -0700 Subject: [PATCH 2/5] feat(optimizer): basic error-code handling across controllers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every optimizer endpoint now returns a uniform ApiError body ({code, message, path}) on non-2xx. Reshape updateOperation so the operationId lives in the URL (and is repeated in the body for self-describing payloads). - api/error/ApiError.java — DTO shape. - api/error/GlobalExceptionHandler.java — @RestControllerAdvice mapping framework exceptions to {VALIDATION_ERROR, INVALID_PARAMETER, MISSING_PARAMETER, MALFORMED_REQUEST, INTERNAL_ERROR}. Reason of ResponseStatusException is parsed as "CODE: message" for endpoint- specific 404s. - Controllers: orElseThrow(ResponseStatusException) replaces bare 404. TableOperationsController moves updateOperation to POST /v1/optimizer/operations/{id}/update; rejects with 400 PATH_BODY_MISMATCH when body.operationId != path.id. - UpdateOperationRequest: @NotBlank operationId, @NotNull status. - UpsertTableStatsRequest: @NotBlank databaseName, tableName. - spring-boot-starter-validation dep added. - New ControllerErrorHandlingTest: 13 MockMvc cases covering 404 / 400 validation / 400 type-mismatch / 400 missing-param / 400 malformed- body / 400 path-body-mismatch + happy-path sanity. Co-Authored-By: Claude Opus 4.7 --- services/optimizer/build.gradle | 1 + .../controller/TableOperationsController.java | 39 +++- .../TableOperationsHistoryController.java | 3 +- .../api/controller/TableStatsController.java | 11 +- .../optimizer/api/error/ApiError.java | 27 +++ .../api/error/GlobalExceptionHandler.java | 102 +++++++++ .../api/spec/UpdateOperationRequest.java | 15 +- .../api/spec/UpsertTableStatsRequest.java | 9 +- .../ControllerErrorHandlingTest.java | 216 ++++++++++++++++++ 9 files changed, 401 insertions(+), 22 deletions(-) create mode 100644 services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/ApiError.java create mode 100644 services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/GlobalExceptionHandler.java create mode 100644 services/optimizer/src/test/java/com/linkedin/openhouse/optimizer/api/controller/ControllerErrorHandlingTest.java diff --git a/services/optimizer/build.gradle b/services/optimizer/build.gradle index c05c7f9c3..c208cf330 100644 --- a/services/optimizer/build.gradle +++ b/services/optimizer/build.gradle @@ -7,6 +7,7 @@ dependencies { implementation 'org.springframework.boot:spring-boot-starter-data-jpa:2.7.8' implementation 'com.vladmihalcea:hibernate-types-55:2.21.1' implementation 'org.springframework.boot:spring-boot-starter-web:2.7.8' + implementation 'org.springframework.boot:spring-boot-starter-validation:2.7.8' implementation 'mysql:mysql-connector-java:8.+' testImplementation 'com.h2database:h2:2.2.224' testImplementation 'org.springframework.boot:spring-boot-starter-test:2.7.8' diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsController.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsController.java index 5db7d31ed..25fd8ab6c 100644 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsController.java +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsController.java @@ -9,6 +9,7 @@ import java.util.List; import java.util.Optional; import java.util.stream.Collectors; +import javax.validation.Valid; import lombok.RequiredArgsConstructor; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; @@ -19,6 +20,7 @@ import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.server.ResponseStatusException; /** REST controller for {@code table_operations}. */ @RestController @@ -29,23 +31,34 @@ public class TableOperationsController { private final OptimizerDataService service; /** - * Report an update to an operation. The body carries the {@code operationId} the caller is - * updating along with its terminal status. The backend looks up the operation row, writes a - * history entry with the operation's table metadata, and returns 201 Created with the history - * row, or 404 if the operation does not exist. + * Report an update to an operation. {@code id} comes from the URL; the body's {@code operationId} + * must match (the controller rejects mismatched requests with 400). The backend looks up the + * operation row, writes a history entry with the operation's table metadata, and returns 201 + * Created with the history row, or 404 if the operation does not exist. */ - @PostMapping("/update") + @PostMapping("/{id}/update") public ResponseEntity updateOperation( - @RequestBody UpdateOperationRequest request) { + @PathVariable String id, @Valid @RequestBody UpdateOperationRequest request) { + if (!id.equals(request.getOperationId())) { + throw new ResponseStatusException( + HttpStatus.BAD_REQUEST, + "PATH_BODY_MISMATCH: operationId in body ('" + + request.getOperationId() + + "') does not match path id ('" + + id + + "')"); + } return service - .updateOperation( - request.getOperationId(), - request.getStatus() == null ? null : request.getStatus().toModel()) + .updateOperation(id, request.getStatus().toModel()) .map( history -> ResponseEntity.status(HttpStatus.CREATED) .body(TableOperationsHistory.fromModel(history))) - .orElse(ResponseEntity.notFound().build()); + .orElseThrow( + () -> + new ResponseStatusException( + HttpStatus.NOT_FOUND, + "OPERATION_NOT_FOUND: no operation with id '" + id + "'")); } /** Fetch a single operation row by its ID, regardless of status. Returns 404 if not found. */ @@ -55,7 +68,11 @@ public ResponseEntity getTableOperation(@PathVariable String id .getTableOperation(id) .map(TableOperations::fromModel) .map(ResponseEntity::ok) - .orElse(ResponseEntity.notFound().build()); + .orElseThrow( + () -> + new ResponseStatusException( + HttpStatus.NOT_FOUND, + "OPERATION_NOT_FOUND: no operation with id '" + id + "'")); } /** diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsHistoryController.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsHistoryController.java index 9a1b6d303..7a457d9cf 100644 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsHistoryController.java +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsHistoryController.java @@ -4,6 +4,7 @@ import com.linkedin.openhouse.optimizer.service.OptimizerDataService; import java.util.List; import java.util.stream.Collectors; +import javax.validation.Valid; import lombok.RequiredArgsConstructor; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; @@ -26,7 +27,7 @@ public class TableOperationsHistoryController { /** Append a completed-job result. Called by the SparkJob after each run (success or failure). */ @PostMapping public ResponseEntity appendHistory( - @RequestBody TableOperationsHistory dto) { + @Valid @RequestBody TableOperationsHistory dto) { return ResponseEntity.status(HttpStatus.CREATED) .body(TableOperationsHistory.fromModel(service.appendHistory(dto.toModel()))); } diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableStatsController.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableStatsController.java index 049516110..976d05e7f 100644 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableStatsController.java +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableStatsController.java @@ -8,7 +8,9 @@ import java.util.List; import java.util.Optional; import java.util.stream.Collectors; +import javax.validation.Valid; import lombok.RequiredArgsConstructor; +import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PathVariable; @@ -17,6 +19,7 @@ import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.server.ResponseStatusException; /** REST controller for managing per-table stats in the optimizer DB. */ @RestController @@ -32,7 +35,7 @@ public class TableStatsController { */ @PutMapping("/{tableUuid}") public ResponseEntity upsertTableStats( - @PathVariable String tableUuid, @RequestBody UpsertTableStatsRequest request) { + @PathVariable String tableUuid, @Valid @RequestBody UpsertTableStatsRequest request) { return ResponseEntity.ok( TableStats.fromModel(service.upsertTableStats(request.toModel(tableUuid)))); } @@ -44,7 +47,11 @@ public ResponseEntity getTableStats(@PathVariable String tableUuid) .getTableStats(tableUuid) .map(TableStats::fromModel) .map(ResponseEntity::ok) - .orElse(ResponseEntity.notFound().build()); + .orElseThrow( + () -> + new ResponseStatusException( + HttpStatus.NOT_FOUND, + "STATS_NOT_FOUND: no stats for tableUuid '" + tableUuid + "'")); } /** diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/ApiError.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/ApiError.java new file mode 100644 index 000000000..9018e1bbe --- /dev/null +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/ApiError.java @@ -0,0 +1,27 @@ +package com.linkedin.openhouse.optimizer.api.error; + +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.Data; +import lombok.NoArgsConstructor; + +/** + * Uniform error response body returned by every optimizer endpoint on a non-2xx status. + * + *

Shape: + * + *

    + *
  • {@code code} — machine-readable identifier (e.g. {@code OPERATION_NOT_FOUND}). + *
  • {@code message} — human-readable explanation. + *
  • {@code path} — the request URI that triggered the error. + *
+ */ +@Data +@Builder +@AllArgsConstructor +@NoArgsConstructor +public class ApiError { + private String code; + private String message; + private String path; +} diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/GlobalExceptionHandler.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/GlobalExceptionHandler.java new file mode 100644 index 000000000..00baf5bd9 --- /dev/null +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/GlobalExceptionHandler.java @@ -0,0 +1,102 @@ +package com.linkedin.openhouse.optimizer.api.error; + +import javax.servlet.http.HttpServletRequest; +import lombok.extern.slf4j.Slf4j; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.http.converter.HttpMessageNotReadableException; +import org.springframework.web.bind.MethodArgumentNotValidException; +import org.springframework.web.bind.MissingServletRequestParameterException; +import org.springframework.web.bind.annotation.ExceptionHandler; +import org.springframework.web.bind.annotation.RestControllerAdvice; +import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException; +import org.springframework.web.server.ResponseStatusException; + +/** + * Maps framework + service exceptions to {@link ApiError} bodies with consistent HTTP status codes + * across every optimizer endpoint. + * + *

Codes used: {@code VALIDATION_ERROR}, {@code INVALID_PARAMETER}, {@code MISSING_PARAMETER}, + * {@code MALFORMED_REQUEST}, {@code OPERATION_NOT_FOUND}, {@code STATS_NOT_FOUND}, {@code + * INTERNAL_ERROR}. Endpoint-specific 404 codes are passed through via {@link + * ResponseStatusException}'s {@code reason} field. + */ +@Slf4j +@RestControllerAdvice +public class GlobalExceptionHandler { + + @ExceptionHandler(MethodArgumentNotValidException.class) + public ResponseEntity handleValidation( + MethodArgumentNotValidException e, HttpServletRequest req) { + String message = + e.getBindingResult().getFieldErrors().stream() + .map(fe -> fe.getField() + ": " + fe.getDefaultMessage()) + .reduce((a, b) -> a + "; " + b) + .orElse(e.getMessage()); + return error(HttpStatus.BAD_REQUEST, "VALIDATION_ERROR", message, req); + } + + @ExceptionHandler(MethodArgumentTypeMismatchException.class) + public ResponseEntity handleTypeMismatch( + MethodArgumentTypeMismatchException e, HttpServletRequest req) { + String type = e.getRequiredType() == null ? "?" : e.getRequiredType().getSimpleName(); + return error( + HttpStatus.BAD_REQUEST, + "INVALID_PARAMETER", + "Parameter '" + + e.getName() + + "' has invalid value '" + + e.getValue() + + "' (expected " + + type + + ")", + req); + } + + @ExceptionHandler(MissingServletRequestParameterException.class) + public ResponseEntity handleMissingParam( + MissingServletRequestParameterException e, HttpServletRequest req) { + return error( + HttpStatus.BAD_REQUEST, + "MISSING_PARAMETER", + "Required parameter '" + e.getParameterName() + "' is missing", + req); + } + + @ExceptionHandler(HttpMessageNotReadableException.class) + public ResponseEntity handleMalformedBody( + HttpMessageNotReadableException e, HttpServletRequest req) { + return error( + HttpStatus.BAD_REQUEST, "MALFORMED_REQUEST", "Request body is missing or malformed", req); + } + + @ExceptionHandler(ResponseStatusException.class) + public ResponseEntity handleResponseStatus( + ResponseStatusException e, HttpServletRequest req) { + HttpStatus status = HttpStatus.resolve(e.getStatus().value()); + if (status == null) { + status = HttpStatus.INTERNAL_SERVER_ERROR; + } + String reason = e.getReason() == null ? status.getReasonPhrase() : e.getReason(); + // Convention: when callers throw ResponseStatusException, they pack a "CODE: human message" + // into the reason. If no colon is present, the whole reason becomes the message and the code + // defaults to the status name (e.g. NOT_FOUND). + int sep = reason.indexOf(':'); + String code = sep > 0 ? reason.substring(0, sep).trim() : status.name(); + String message = sep > 0 ? reason.substring(sep + 1).trim() : reason; + return error(status, code, message, req); + } + + @ExceptionHandler(Exception.class) + public ResponseEntity handleUncaught(Exception e, HttpServletRequest req) { + log.warn("Unhandled exception on {}: {}", req.getRequestURI(), e.toString(), e); + return error( + HttpStatus.INTERNAL_SERVER_ERROR, "INTERNAL_ERROR", "An unexpected error occurred", req); + } + + private static ResponseEntity error( + HttpStatus status, String code, String message, HttpServletRequest req) { + return ResponseEntity.status(status) + .body(ApiError.builder().code(code).message(message).path(req.getRequestURI()).build()); + } +} diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpdateOperationRequest.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpdateOperationRequest.java index a216e9db3..fe5bee516 100644 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpdateOperationRequest.java +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpdateOperationRequest.java @@ -1,5 +1,7 @@ package com.linkedin.openhouse.optimizer.api.spec; +import javax.validation.constraints.NotBlank; +import javax.validation.constraints.NotNull; import lombok.AllArgsConstructor; import lombok.Builder; import lombok.Data; @@ -27,11 +29,16 @@ @AllArgsConstructor public class UpdateOperationRequest { - /** Operation row's UUID — the primary lookup key. */ - private String operationId; + /** + * Operation row's UUID. Required. Must match the {@code {id}} path variable on {@code POST + * /v1/optimizer/operations/{id}/update} — the controller rejects mismatched requests with 400. + * Carrying it in the body keeps the payload self-describing for trace/log consumers that may not + * see the URL. + */ + @NotBlank private String operationId; - /** Terminal outcome for this single operation. */ - private HistoryStatus status; + /** Terminal outcome for this single operation. Required. */ + @NotNull private HistoryStatus status; /** Debug echo: stable table identity the caller believed it was completing. */ private String tableUuid; diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpsertTableStatsRequest.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpsertTableStatsRequest.java index d1b4a5fe2..9d2dadb0e 100644 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpsertTableStatsRequest.java +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpsertTableStatsRequest.java @@ -2,6 +2,7 @@ import java.util.Collections; import java.util.Map; +import javax.validation.constraints.NotBlank; import lombok.AllArgsConstructor; import lombok.Builder; import lombok.Data; @@ -19,11 +20,11 @@ @AllArgsConstructor public class UpsertTableStatsRequest { - /** Denormalized database name for display. */ - private String databaseName; + /** Denormalized database name for display. Required. */ + @NotBlank private String databaseName; - /** Denormalized table name for display. */ - private String tableName; + /** Denormalized table name for display. Required. */ + @NotBlank private String tableName; /** Combined snapshot + delta stats payload from this commit. */ private TableStatsPayload stats; diff --git a/services/optimizer/src/test/java/com/linkedin/openhouse/optimizer/api/controller/ControllerErrorHandlingTest.java b/services/optimizer/src/test/java/com/linkedin/openhouse/optimizer/api/controller/ControllerErrorHandlingTest.java new file mode 100644 index 000000000..97b63b06f --- /dev/null +++ b/services/optimizer/src/test/java/com/linkedin/openhouse/optimizer/api/controller/ControllerErrorHandlingTest.java @@ -0,0 +1,216 @@ +package com.linkedin.openhouse.optimizer.api.controller; + +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.put; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import com.linkedin.openhouse.optimizer.db.OperationType; +import com.linkedin.openhouse.optimizer.db.TableOperationsRow; +import com.linkedin.openhouse.optimizer.repository.TableOperationsRepository; +import java.util.UUID; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.http.MediaType; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.transaction.annotation.Transactional; + +/** + * Exercises the {@code GlobalExceptionHandler} contract across all three controllers — every + * non-2xx response carries an {@link com.linkedin.openhouse.optimizer.api.error.ApiError} body with + * {@code code}, {@code message}, and {@code path}. + */ +@SpringBootTest +@AutoConfigureMockMvc +@ActiveProfiles("test") +@Transactional +class ControllerErrorHandlingTest { + + @Autowired MockMvc mockMvc; + @Autowired TableOperationsRepository operationsRepository; + + // --- /operations/{id}/update --- + + @Test + void updateOperation_notFound_returns404WithCode() throws Exception { + String id = UUID.randomUUID().toString(); + String body = "{\"operationId\":\"" + id + "\",\"status\":\"SUCCESS\"}"; + mockMvc + .perform( + post("/v1/optimizer/operations/" + id + "/update") + .contentType(MediaType.APPLICATION_JSON) + .content(body)) + .andExpect(status().isNotFound()) + .andExpect(jsonPath("$.code").value("OPERATION_NOT_FOUND")) + .andExpect(jsonPath("$.message").value(org.hamcrest.Matchers.containsString(id))) + .andExpect(jsonPath("$.path").value("/v1/optimizer/operations/" + id + "/update")); + } + + @Test + void updateOperation_pathBodyMismatch_returns400() throws Exception { + String pathId = UUID.randomUUID().toString(); + String bodyId = UUID.randomUUID().toString(); + String body = "{\"operationId\":\"" + bodyId + "\",\"status\":\"SUCCESS\"}"; + mockMvc + .perform( + post("/v1/optimizer/operations/" + pathId + "/update") + .contentType(MediaType.APPLICATION_JSON) + .content(body)) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.code").value("PATH_BODY_MISMATCH")); + } + + @Test + void updateOperation_missingStatus_returns400Validation() throws Exception { + String id = UUID.randomUUID().toString(); + String body = "{\"operationId\":\"" + id + "\"}"; + mockMvc + .perform( + post("/v1/optimizer/operations/" + id + "/update") + .contentType(MediaType.APPLICATION_JSON) + .content(body)) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.code").value("VALIDATION_ERROR")) + .andExpect(jsonPath("$.message").value(org.hamcrest.Matchers.containsString("status"))); + } + + @Test + void updateOperation_missingOperationId_returns400Validation() throws Exception { + String pathId = UUID.randomUUID().toString(); + String body = "{\"status\":\"SUCCESS\"}"; + mockMvc + .perform( + post("/v1/optimizer/operations/" + pathId + "/update") + .contentType(MediaType.APPLICATION_JSON) + .content(body)) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.code").value("VALIDATION_ERROR")) + .andExpect( + jsonPath("$.message").value(org.hamcrest.Matchers.containsString("operationId"))); + } + + @Test + void updateOperation_malformedJson_returns400Malformed() throws Exception { + String pathId = UUID.randomUUID().toString(); + mockMvc + .perform( + post("/v1/optimizer/operations/" + pathId + "/update") + .contentType(MediaType.APPLICATION_JSON) + .content("not json")) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.code").value("MALFORMED_REQUEST")); + } + + // --- /operations/{id} --- + + @Test + void getTableOperation_notFound_returns404WithCode() throws Exception { + String id = UUID.randomUUID().toString(); + mockMvc + .perform(get("/v1/optimizer/operations/" + id)) + .andExpect(status().isNotFound()) + .andExpect(jsonPath("$.code").value("OPERATION_NOT_FOUND")) + .andExpect(jsonPath("$.path").value("/v1/optimizer/operations/" + id)); + } + + // --- /operations (list) --- + + @Test + void listOperations_missingLimit_returns400Missing() throws Exception { + mockMvc + .perform(get("/v1/optimizer/operations")) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.code").value("MISSING_PARAMETER")) + .andExpect(jsonPath("$.message").value(org.hamcrest.Matchers.containsString("limit"))); + } + + @Test + void listOperations_badLimit_returns400TypeMismatch() throws Exception { + mockMvc + .perform(get("/v1/optimizer/operations").param("limit", "abc")) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.code").value("INVALID_PARAMETER")) + .andExpect(jsonPath("$.message").value(org.hamcrest.Matchers.containsString("limit"))); + } + + @Test + void listOperations_badEnum_returns400() throws Exception { + mockMvc + .perform(get("/v1/optimizer/operations").param("status", "BOGUS").param("limit", "10")) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.code").value("INVALID_PARAMETER")); + } + + // --- /stats/{tableUuid} --- + + @Test + void getTableStats_notFound_returns404WithCode() throws Exception { + String uuid = UUID.randomUUID().toString(); + mockMvc + .perform(get("/v1/optimizer/stats/" + uuid)) + .andExpect(status().isNotFound()) + .andExpect(jsonPath("$.code").value("STATS_NOT_FOUND")); + } + + // --- /stats (upsert) --- + + @Test + void upsertStats_missingRequiredField_returns400Validation() throws Exception { + String uuid = UUID.randomUUID().toString(); + String body = "{\"tableName\":\"tbl1\"}"; // databaseName missing + mockMvc + .perform( + put("/v1/optimizer/stats/" + uuid) + .contentType(MediaType.APPLICATION_JSON) + .content(body)) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.code").value("VALIDATION_ERROR")) + .andExpect( + jsonPath("$.message").value(org.hamcrest.Matchers.containsString("databaseName"))); + } + + // --- /stats/{tableUuid}/history --- + + @Test + void getStatsHistory_badSince_returns400() throws Exception { + String uuid = UUID.randomUUID().toString(); + mockMvc + .perform( + get("/v1/optimizer/stats/" + uuid + "/history") + .param("since", "not-a-date") + .param("limit", "10")) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.code").value("INVALID_PARAMETER")); + } + + // --- happy path sanity --- + + @Test + void updateOperation_happyPath_stillReturns201() throws Exception { + String id = UUID.randomUUID().toString(); + operationsRepository.save( + TableOperationsRow.builder() + .id(id) + .tableUuid(UUID.randomUUID().toString()) + .databaseName("db1") + .tableName("tbl1") + .operationType(OperationType.ORPHAN_FILES_DELETION) + .status(com.linkedin.openhouse.optimizer.db.OperationStatus.SCHEDULED) + .createdAt(java.time.Instant.now()) + .scheduledAt(java.time.Instant.now()) + .jobId("job-x") + .build()); + String body = "{\"operationId\":\"" + id + "\",\"status\":\"SUCCESS\"}"; + mockMvc + .perform( + post("/v1/optimizer/operations/" + id + "/update") + .contentType(MediaType.APPLICATION_JSON) + .content(body)) + .andExpect(status().isCreated()) + .andExpect(jsonPath("$.status").value("SUCCESS")); + } +} From a37169d8a977fc4faec0212c14e855b0ab07d348 Mon Sep 17 00:00:00 2001 From: mkuchenbecker Date: Fri, 22 May 2026 10:47:01 -0700 Subject: [PATCH 3/5] refactor(optimizer): simplify error handling per PR review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review comments on PR #596 — minimal diff, no bean validation, no per-framework-exception handlers, scoped to the optimizer controllers. - @RestControllerAdvice scoped to the three optimizer controllers via assignableTypes; no longer global. - Two handlers only: ResponseStatusException → ApiError with code = status.name() + reason as message; Exception → 500 INTERNAL_ERROR. Drop MethodArgumentNotValidException, MethodArgumentTypeMismatchException, MissingServletRequestParameterException, HttpMessageNotReadableException — Spring's defaults handle those. - Drop the "CODE: message" reason-parsing convention. - Drop @NotBlank / @NotNull on UpdateOperationRequest and UpsertTableStatsRequest; drop @Valid on controllers; drop spring-boot-starter-validation dep. Validate operationId / status server- side in TableOperationsController.updateOperation — loose-coupling so relaxing required fields later doesn't break wire callers. - String.format throughout; no message concatenation. - ControllerErrorHandlingTest trimmed from 13 cases to 7: only what the controllers actually own (404s, server-side validation on updateOperation, happy-path sanity). Framework-level 4xx left to Spring. Co-Authored-By: Claude Opus 4.7 --- services/optimizer/build.gradle | 1 - .../controller/TableOperationsController.java | 27 ++-- .../TableOperationsHistoryController.java | 3 +- .../api/controller/TableStatsController.java | 6 +- .../api/error/GlobalExceptionHandler.java | 103 ++++--------- .../api/spec/UpdateOperationRequest.java | 13 +- .../api/spec/UpsertTableStatsRequest.java | 9 +- .../ControllerErrorHandlingTest.java | 135 ++++-------------- 8 files changed, 86 insertions(+), 211 deletions(-) diff --git a/services/optimizer/build.gradle b/services/optimizer/build.gradle index c208cf330..c05c7f9c3 100644 --- a/services/optimizer/build.gradle +++ b/services/optimizer/build.gradle @@ -7,7 +7,6 @@ dependencies { implementation 'org.springframework.boot:spring-boot-starter-data-jpa:2.7.8' implementation 'com.vladmihalcea:hibernate-types-55:2.21.1' implementation 'org.springframework.boot:spring-boot-starter-web:2.7.8' - implementation 'org.springframework.boot:spring-boot-starter-validation:2.7.8' implementation 'mysql:mysql-connector-java:8.+' testImplementation 'com.h2database:h2:2.2.224' testImplementation 'org.springframework.boot:spring-boot-starter-test:2.7.8' diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsController.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsController.java index 25fd8ab6c..2f6f62e4b 100644 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsController.java +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsController.java @@ -7,12 +7,13 @@ import com.linkedin.openhouse.optimizer.api.spec.UpdateOperationRequest; import com.linkedin.openhouse.optimizer.service.OptimizerDataService; import java.util.List; +import java.util.Objects; import java.util.Optional; import java.util.stream.Collectors; -import javax.validation.Valid; import lombok.RequiredArgsConstructor; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; +import org.springframework.util.StringUtils; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.PostMapping; @@ -38,15 +39,19 @@ public class TableOperationsController { */ @PostMapping("/{id}/update") public ResponseEntity updateOperation( - @PathVariable String id, @Valid @RequestBody UpdateOperationRequest request) { - if (!id.equals(request.getOperationId())) { + @PathVariable String id, @RequestBody UpdateOperationRequest request) { + if (!StringUtils.hasText(request.getOperationId())) { + throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "operationId is required"); + } + if (!Objects.equals(id, request.getOperationId())) { throw new ResponseStatusException( HttpStatus.BAD_REQUEST, - "PATH_BODY_MISMATCH: operationId in body ('" - + request.getOperationId() - + "') does not match path id ('" - + id - + "')"); + String.format( + "operationId in body (%s) does not match path id (%s)", + request.getOperationId(), id)); + } + if (request.getStatus() == null) { + throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "status is required"); } return service .updateOperation(id, request.getStatus().toModel()) @@ -57,8 +62,7 @@ public ResponseEntity updateOperation( .orElseThrow( () -> new ResponseStatusException( - HttpStatus.NOT_FOUND, - "OPERATION_NOT_FOUND: no operation with id '" + id + "'")); + HttpStatus.NOT_FOUND, String.format("no operation with id %s", id))); } /** Fetch a single operation row by its ID, regardless of status. Returns 404 if not found. */ @@ -71,8 +75,7 @@ public ResponseEntity getTableOperation(@PathVariable String id .orElseThrow( () -> new ResponseStatusException( - HttpStatus.NOT_FOUND, - "OPERATION_NOT_FOUND: no operation with id '" + id + "'")); + HttpStatus.NOT_FOUND, String.format("no operation with id %s", id))); } /** diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsHistoryController.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsHistoryController.java index 7a457d9cf..9a1b6d303 100644 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsHistoryController.java +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableOperationsHistoryController.java @@ -4,7 +4,6 @@ import com.linkedin.openhouse.optimizer.service.OptimizerDataService; import java.util.List; import java.util.stream.Collectors; -import javax.validation.Valid; import lombok.RequiredArgsConstructor; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; @@ -27,7 +26,7 @@ public class TableOperationsHistoryController { /** Append a completed-job result. Called by the SparkJob after each run (success or failure). */ @PostMapping public ResponseEntity appendHistory( - @Valid @RequestBody TableOperationsHistory dto) { + @RequestBody TableOperationsHistory dto) { return ResponseEntity.status(HttpStatus.CREATED) .body(TableOperationsHistory.fromModel(service.appendHistory(dto.toModel()))); } diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableStatsController.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableStatsController.java index 976d05e7f..ca8db4d51 100644 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableStatsController.java +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/controller/TableStatsController.java @@ -8,7 +8,6 @@ import java.util.List; import java.util.Optional; import java.util.stream.Collectors; -import javax.validation.Valid; import lombok.RequiredArgsConstructor; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; @@ -35,7 +34,7 @@ public class TableStatsController { */ @PutMapping("/{tableUuid}") public ResponseEntity upsertTableStats( - @PathVariable String tableUuid, @Valid @RequestBody UpsertTableStatsRequest request) { + @PathVariable String tableUuid, @RequestBody UpsertTableStatsRequest request) { return ResponseEntity.ok( TableStats.fromModel(service.upsertTableStats(request.toModel(tableUuid)))); } @@ -50,8 +49,7 @@ public ResponseEntity getTableStats(@PathVariable String tableUuid) .orElseThrow( () -> new ResponseStatusException( - HttpStatus.NOT_FOUND, - "STATS_NOT_FOUND: no stats for tableUuid '" + tableUuid + "'")); + HttpStatus.NOT_FOUND, String.format("no stats for tableUuid %s", tableUuid))); } /** diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/GlobalExceptionHandler.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/GlobalExceptionHandler.java index 00baf5bd9..d47dd3911 100644 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/GlobalExceptionHandler.java +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/GlobalExceptionHandler.java @@ -1,75 +1,31 @@ package com.linkedin.openhouse.optimizer.api.error; +import com.linkedin.openhouse.optimizer.api.controller.TableOperationsController; +import com.linkedin.openhouse.optimizer.api.controller.TableOperationsHistoryController; +import com.linkedin.openhouse.optimizer.api.controller.TableStatsController; import javax.servlet.http.HttpServletRequest; import lombok.extern.slf4j.Slf4j; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; -import org.springframework.http.converter.HttpMessageNotReadableException; -import org.springframework.web.bind.MethodArgumentNotValidException; -import org.springframework.web.bind.MissingServletRequestParameterException; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.RestControllerAdvice; -import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException; import org.springframework.web.server.ResponseStatusException; /** - * Maps framework + service exceptions to {@link ApiError} bodies with consistent HTTP status codes - * across every optimizer endpoint. - * - *

Codes used: {@code VALIDATION_ERROR}, {@code INVALID_PARAMETER}, {@code MISSING_PARAMETER}, - * {@code MALFORMED_REQUEST}, {@code OPERATION_NOT_FOUND}, {@code STATS_NOT_FOUND}, {@code - * INTERNAL_ERROR}. Endpoint-specific 404 codes are passed through via {@link - * ResponseStatusException}'s {@code reason} field. + * Scoped to the optimizer REST controllers. Two cases only: pass through any {@link + * ResponseStatusException} that a controller threw, and convert any other uncaught exception into a + * 500. Framework-level 4xx responses (missing query param, malformed body, etc.) are left to + * Spring's defaults — this advice intentionally does not blanket every possible exception type. */ @Slf4j -@RestControllerAdvice +@RestControllerAdvice( + assignableTypes = { + TableOperationsController.class, + TableOperationsHistoryController.class, + TableStatsController.class + }) public class GlobalExceptionHandler { - @ExceptionHandler(MethodArgumentNotValidException.class) - public ResponseEntity handleValidation( - MethodArgumentNotValidException e, HttpServletRequest req) { - String message = - e.getBindingResult().getFieldErrors().stream() - .map(fe -> fe.getField() + ": " + fe.getDefaultMessage()) - .reduce((a, b) -> a + "; " + b) - .orElse(e.getMessage()); - return error(HttpStatus.BAD_REQUEST, "VALIDATION_ERROR", message, req); - } - - @ExceptionHandler(MethodArgumentTypeMismatchException.class) - public ResponseEntity handleTypeMismatch( - MethodArgumentTypeMismatchException e, HttpServletRequest req) { - String type = e.getRequiredType() == null ? "?" : e.getRequiredType().getSimpleName(); - return error( - HttpStatus.BAD_REQUEST, - "INVALID_PARAMETER", - "Parameter '" - + e.getName() - + "' has invalid value '" - + e.getValue() - + "' (expected " - + type - + ")", - req); - } - - @ExceptionHandler(MissingServletRequestParameterException.class) - public ResponseEntity handleMissingParam( - MissingServletRequestParameterException e, HttpServletRequest req) { - return error( - HttpStatus.BAD_REQUEST, - "MISSING_PARAMETER", - "Required parameter '" + e.getParameterName() + "' is missing", - req); - } - - @ExceptionHandler(HttpMessageNotReadableException.class) - public ResponseEntity handleMalformedBody( - HttpMessageNotReadableException e, HttpServletRequest req) { - return error( - HttpStatus.BAD_REQUEST, "MALFORMED_REQUEST", "Request body is missing or malformed", req); - } - @ExceptionHandler(ResponseStatusException.class) public ResponseEntity handleResponseStatus( ResponseStatusException e, HttpServletRequest req) { @@ -77,26 +33,25 @@ public ResponseEntity handleResponseStatus( if (status == null) { status = HttpStatus.INTERNAL_SERVER_ERROR; } - String reason = e.getReason() == null ? status.getReasonPhrase() : e.getReason(); - // Convention: when callers throw ResponseStatusException, they pack a "CODE: human message" - // into the reason. If no colon is present, the whole reason becomes the message and the code - // defaults to the status name (e.g. NOT_FOUND). - int sep = reason.indexOf(':'); - String code = sep > 0 ? reason.substring(0, sep).trim() : status.name(); - String message = sep > 0 ? reason.substring(sep + 1).trim() : reason; - return error(status, code, message, req); + String message = e.getReason() == null ? status.getReasonPhrase() : e.getReason(); + return ResponseEntity.status(status) + .body( + ApiError.builder() + .code(status.name()) + .message(message) + .path(req.getRequestURI()) + .build()); } @ExceptionHandler(Exception.class) public ResponseEntity handleUncaught(Exception e, HttpServletRequest req) { - log.warn("Unhandled exception on {}: {}", req.getRequestURI(), e.toString(), e); - return error( - HttpStatus.INTERNAL_SERVER_ERROR, "INTERNAL_ERROR", "An unexpected error occurred", req); - } - - private static ResponseEntity error( - HttpStatus status, String code, String message, HttpServletRequest req) { - return ResponseEntity.status(status) - .body(ApiError.builder().code(code).message(message).path(req.getRequestURI()).build()); + log.warn(String.format("Unhandled exception on %s", req.getRequestURI()), e); + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) + .body( + ApiError.builder() + .code("INTERNAL_ERROR") + .message("An unexpected error occurred") + .path(req.getRequestURI()) + .build()); } } diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpdateOperationRequest.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpdateOperationRequest.java index fe5bee516..fcae718ad 100644 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpdateOperationRequest.java +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpdateOperationRequest.java @@ -1,7 +1,5 @@ package com.linkedin.openhouse.optimizer.api.spec; -import javax.validation.constraints.NotBlank; -import javax.validation.constraints.NotNull; import lombok.AllArgsConstructor; import lombok.Builder; import lombok.Data; @@ -30,15 +28,16 @@ public class UpdateOperationRequest { /** - * Operation row's UUID. Required. Must match the {@code {id}} path variable on {@code POST + * Operation row's UUID. Must match the {@code {id}} path variable on {@code POST * /v1/optimizer/operations/{id}/update} — the controller rejects mismatched requests with 400. * Carrying it in the body keeps the payload self-describing for trace/log consumers that may not - * see the URL. + * see the URL. Validated server-side (no bean-validation annotation) so that future relaxation + * does not break clients on the wire contract. */ - @NotBlank private String operationId; + private String operationId; - /** Terminal outcome for this single operation. Required. */ - @NotNull private HistoryStatus status; + /** Terminal outcome for this single operation. Validated server-side. */ + private HistoryStatus status; /** Debug echo: stable table identity the caller believed it was completing. */ private String tableUuid; diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpsertTableStatsRequest.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpsertTableStatsRequest.java index 9d2dadb0e..d1b4a5fe2 100644 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpsertTableStatsRequest.java +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpsertTableStatsRequest.java @@ -2,7 +2,6 @@ import java.util.Collections; import java.util.Map; -import javax.validation.constraints.NotBlank; import lombok.AllArgsConstructor; import lombok.Builder; import lombok.Data; @@ -20,11 +19,11 @@ @AllArgsConstructor public class UpsertTableStatsRequest { - /** Denormalized database name for display. Required. */ - @NotBlank private String databaseName; + /** Denormalized database name for display. */ + private String databaseName; - /** Denormalized table name for display. Required. */ - @NotBlank private String tableName; + /** Denormalized table name for display. */ + private String tableName; /** Combined snapshot + delta stats payload from this commit. */ private TableStatsPayload stats; diff --git a/services/optimizer/src/test/java/com/linkedin/openhouse/optimizer/api/controller/ControllerErrorHandlingTest.java b/services/optimizer/src/test/java/com/linkedin/openhouse/optimizer/api/controller/ControllerErrorHandlingTest.java index 97b63b06f..59d793441 100644 --- a/services/optimizer/src/test/java/com/linkedin/openhouse/optimizer/api/controller/ControllerErrorHandlingTest.java +++ b/services/optimizer/src/test/java/com/linkedin/openhouse/optimizer/api/controller/ControllerErrorHandlingTest.java @@ -2,13 +2,13 @@ import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; -import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.put; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; import com.linkedin.openhouse.optimizer.db.OperationType; import com.linkedin.openhouse.optimizer.db.TableOperationsRow; import com.linkedin.openhouse.optimizer.repository.TableOperationsRepository; +import java.time.Instant; import java.util.UUID; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; @@ -20,9 +20,9 @@ import org.springframework.transaction.annotation.Transactional; /** - * Exercises the {@code GlobalExceptionHandler} contract across all three controllers — every - * non-2xx response carries an {@link com.linkedin.openhouse.optimizer.api.error.ApiError} body with - * {@code code}, {@code message}, and {@code path}. + * Exercises what the controllers own: server-side validation on {@code updateOperation}, 404 on + * missing rows, and the {@code ApiError} body shape. Framework-level 4xx (missing query param, + * malformed JSON, etc.) is left to Spring's defaults and not asserted here. */ @SpringBootTest @AutoConfigureMockMvc @@ -33,19 +33,17 @@ class ControllerErrorHandlingTest { @Autowired MockMvc mockMvc; @Autowired TableOperationsRepository operationsRepository; - // --- /operations/{id}/update --- - @Test - void updateOperation_notFound_returns404WithCode() throws Exception { + void updateOperation_notFound_returns404WithApiError() throws Exception { String id = UUID.randomUUID().toString(); - String body = "{\"operationId\":\"" + id + "\",\"status\":\"SUCCESS\"}"; + String body = String.format("{\"operationId\":\"%s\",\"status\":\"SUCCESS\"}", id); mockMvc .perform( post("/v1/optimizer/operations/" + id + "/update") .contentType(MediaType.APPLICATION_JSON) .content(body)) .andExpect(status().isNotFound()) - .andExpect(jsonPath("$.code").value("OPERATION_NOT_FOUND")) + .andExpect(jsonPath("$.code").value("NOT_FOUND")) .andExpect(jsonPath("$.message").value(org.hamcrest.Matchers.containsString(id))) .andExpect(jsonPath("$.path").value("/v1/optimizer/operations/" + id + "/update")); } @@ -54,32 +52,21 @@ void updateOperation_notFound_returns404WithCode() throws Exception { void updateOperation_pathBodyMismatch_returns400() throws Exception { String pathId = UUID.randomUUID().toString(); String bodyId = UUID.randomUUID().toString(); - String body = "{\"operationId\":\"" + bodyId + "\",\"status\":\"SUCCESS\"}"; + String body = String.format("{\"operationId\":\"%s\",\"status\":\"SUCCESS\"}", bodyId); mockMvc .perform( post("/v1/optimizer/operations/" + pathId + "/update") .contentType(MediaType.APPLICATION_JSON) .content(body)) .andExpect(status().isBadRequest()) - .andExpect(jsonPath("$.code").value("PATH_BODY_MISMATCH")); - } - - @Test - void updateOperation_missingStatus_returns400Validation() throws Exception { - String id = UUID.randomUUID().toString(); - String body = "{\"operationId\":\"" + id + "\"}"; - mockMvc - .perform( - post("/v1/optimizer/operations/" + id + "/update") - .contentType(MediaType.APPLICATION_JSON) - .content(body)) - .andExpect(status().isBadRequest()) - .andExpect(jsonPath("$.code").value("VALIDATION_ERROR")) - .andExpect(jsonPath("$.message").value(org.hamcrest.Matchers.containsString("status"))); + .andExpect(jsonPath("$.code").value("BAD_REQUEST")) + .andExpect( + jsonPath("$.message") + .value(org.hamcrest.Matchers.containsString("does not match path id"))); } @Test - void updateOperation_missingOperationId_returns400Validation() throws Exception { + void updateOperation_missingOperationId_returns400() throws Exception { String pathId = UUID.randomUUID().toString(); String body = "{\"status\":\"SUCCESS\"}"; mockMvc @@ -88,107 +75,43 @@ void updateOperation_missingOperationId_returns400Validation() throws Exception .contentType(MediaType.APPLICATION_JSON) .content(body)) .andExpect(status().isBadRequest()) - .andExpect(jsonPath("$.code").value("VALIDATION_ERROR")) - .andExpect( - jsonPath("$.message").value(org.hamcrest.Matchers.containsString("operationId"))); + .andExpect(jsonPath("$.code").value("BAD_REQUEST")) + .andExpect(jsonPath("$.message").value("operationId is required")); } @Test - void updateOperation_malformedJson_returns400Malformed() throws Exception { - String pathId = UUID.randomUUID().toString(); + void updateOperation_missingStatus_returns400() throws Exception { + String id = UUID.randomUUID().toString(); + String body = String.format("{\"operationId\":\"%s\"}", id); mockMvc .perform( - post("/v1/optimizer/operations/" + pathId + "/update") + post("/v1/optimizer/operations/" + id + "/update") .contentType(MediaType.APPLICATION_JSON) - .content("not json")) + .content(body)) .andExpect(status().isBadRequest()) - .andExpect(jsonPath("$.code").value("MALFORMED_REQUEST")); + .andExpect(jsonPath("$.code").value("BAD_REQUEST")) + .andExpect(jsonPath("$.message").value("status is required")); } - // --- /operations/{id} --- - @Test - void getTableOperation_notFound_returns404WithCode() throws Exception { + void getTableOperation_notFound_returns404WithApiError() throws Exception { String id = UUID.randomUUID().toString(); mockMvc .perform(get("/v1/optimizer/operations/" + id)) .andExpect(status().isNotFound()) - .andExpect(jsonPath("$.code").value("OPERATION_NOT_FOUND")) + .andExpect(jsonPath("$.code").value("NOT_FOUND")) .andExpect(jsonPath("$.path").value("/v1/optimizer/operations/" + id)); } - // --- /operations (list) --- - - @Test - void listOperations_missingLimit_returns400Missing() throws Exception { - mockMvc - .perform(get("/v1/optimizer/operations")) - .andExpect(status().isBadRequest()) - .andExpect(jsonPath("$.code").value("MISSING_PARAMETER")) - .andExpect(jsonPath("$.message").value(org.hamcrest.Matchers.containsString("limit"))); - } - - @Test - void listOperations_badLimit_returns400TypeMismatch() throws Exception { - mockMvc - .perform(get("/v1/optimizer/operations").param("limit", "abc")) - .andExpect(status().isBadRequest()) - .andExpect(jsonPath("$.code").value("INVALID_PARAMETER")) - .andExpect(jsonPath("$.message").value(org.hamcrest.Matchers.containsString("limit"))); - } - - @Test - void listOperations_badEnum_returns400() throws Exception { - mockMvc - .perform(get("/v1/optimizer/operations").param("status", "BOGUS").param("limit", "10")) - .andExpect(status().isBadRequest()) - .andExpect(jsonPath("$.code").value("INVALID_PARAMETER")); - } - - // --- /stats/{tableUuid} --- - @Test - void getTableStats_notFound_returns404WithCode() throws Exception { + void getTableStats_notFound_returns404WithApiError() throws Exception { String uuid = UUID.randomUUID().toString(); mockMvc .perform(get("/v1/optimizer/stats/" + uuid)) .andExpect(status().isNotFound()) - .andExpect(jsonPath("$.code").value("STATS_NOT_FOUND")); - } - - // --- /stats (upsert) --- - - @Test - void upsertStats_missingRequiredField_returns400Validation() throws Exception { - String uuid = UUID.randomUUID().toString(); - String body = "{\"tableName\":\"tbl1\"}"; // databaseName missing - mockMvc - .perform( - put("/v1/optimizer/stats/" + uuid) - .contentType(MediaType.APPLICATION_JSON) - .content(body)) - .andExpect(status().isBadRequest()) - .andExpect(jsonPath("$.code").value("VALIDATION_ERROR")) - .andExpect( - jsonPath("$.message").value(org.hamcrest.Matchers.containsString("databaseName"))); - } - - // --- /stats/{tableUuid}/history --- - - @Test - void getStatsHistory_badSince_returns400() throws Exception { - String uuid = UUID.randomUUID().toString(); - mockMvc - .perform( - get("/v1/optimizer/stats/" + uuid + "/history") - .param("since", "not-a-date") - .param("limit", "10")) - .andExpect(status().isBadRequest()) - .andExpect(jsonPath("$.code").value("INVALID_PARAMETER")); + .andExpect(jsonPath("$.code").value("NOT_FOUND")); } - // --- happy path sanity --- - @Test void updateOperation_happyPath_stillReturns201() throws Exception { String id = UUID.randomUUID().toString(); @@ -200,11 +123,11 @@ void updateOperation_happyPath_stillReturns201() throws Exception { .tableName("tbl1") .operationType(OperationType.ORPHAN_FILES_DELETION) .status(com.linkedin.openhouse.optimizer.db.OperationStatus.SCHEDULED) - .createdAt(java.time.Instant.now()) - .scheduledAt(java.time.Instant.now()) + .createdAt(Instant.now()) + .scheduledAt(Instant.now()) .jobId("job-x") .build()); - String body = "{\"operationId\":\"" + id + "\",\"status\":\"SUCCESS\"}"; + String body = String.format("{\"operationId\":\"%s\",\"status\":\"SUCCESS\"}", id); mockMvc .perform( post("/v1/optimizer/operations/" + id + "/update") From 6416c9dfce21ed02561d8ab104802eb1b760d043 Mon Sep 17 00:00:00 2001 From: mkuchenbecker Date: Fri, 22 May 2026 11:41:38 -0700 Subject: [PATCH 4/5] refactor(optimizer): drop GlobalExceptionHandler + ApiError; use Spring defaults The custom advice was producing a body shape (ApiError {code, message, path}) that duplicated Spring Boot's default error JSON ({timestamp, status, error, message, path}). The only substantive difference was that Spring Boot 2.7 omits the `message` field by default. Replace the custom advice with a one-line config: server.error.include-message=always Now ResponseStatusException reasons (e.g. "no operation with id X") reach the caller via Spring's default error body, no custom code. - Delete api/error/GlobalExceptionHandler.java - Delete api/error/ApiError.java - application.properties: server.error.include-message=always - ControllerErrorHandlingTest assertions trimmed to status-code-only (MockMvc does not trigger Spring's error-dispatch to BasicErrorController, so body assertions cannot be made in tests even though the body is populated on real HTTP requests). Co-Authored-By: Claude Opus 4.7 --- .../optimizer/api/error/ApiError.java | 27 --------- .../api/error/GlobalExceptionHandler.java | 57 ------------------- .../src/main/resources/application.properties | 5 ++ .../ControllerErrorHandlingTest.java | 47 ++++++--------- 4 files changed, 21 insertions(+), 115 deletions(-) delete mode 100644 services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/ApiError.java delete mode 100644 services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/GlobalExceptionHandler.java diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/ApiError.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/ApiError.java deleted file mode 100644 index 9018e1bbe..000000000 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/ApiError.java +++ /dev/null @@ -1,27 +0,0 @@ -package com.linkedin.openhouse.optimizer.api.error; - -import lombok.AllArgsConstructor; -import lombok.Builder; -import lombok.Data; -import lombok.NoArgsConstructor; - -/** - * Uniform error response body returned by every optimizer endpoint on a non-2xx status. - * - *

Shape: - * - *

    - *
  • {@code code} — machine-readable identifier (e.g. {@code OPERATION_NOT_FOUND}). - *
  • {@code message} — human-readable explanation. - *
  • {@code path} — the request URI that triggered the error. - *
- */ -@Data -@Builder -@AllArgsConstructor -@NoArgsConstructor -public class ApiError { - private String code; - private String message; - private String path; -} diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/GlobalExceptionHandler.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/GlobalExceptionHandler.java deleted file mode 100644 index d47dd3911..000000000 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/error/GlobalExceptionHandler.java +++ /dev/null @@ -1,57 +0,0 @@ -package com.linkedin.openhouse.optimizer.api.error; - -import com.linkedin.openhouse.optimizer.api.controller.TableOperationsController; -import com.linkedin.openhouse.optimizer.api.controller.TableOperationsHistoryController; -import com.linkedin.openhouse.optimizer.api.controller.TableStatsController; -import javax.servlet.http.HttpServletRequest; -import lombok.extern.slf4j.Slf4j; -import org.springframework.http.HttpStatus; -import org.springframework.http.ResponseEntity; -import org.springframework.web.bind.annotation.ExceptionHandler; -import org.springframework.web.bind.annotation.RestControllerAdvice; -import org.springframework.web.server.ResponseStatusException; - -/** - * Scoped to the optimizer REST controllers. Two cases only: pass through any {@link - * ResponseStatusException} that a controller threw, and convert any other uncaught exception into a - * 500. Framework-level 4xx responses (missing query param, malformed body, etc.) are left to - * Spring's defaults — this advice intentionally does not blanket every possible exception type. - */ -@Slf4j -@RestControllerAdvice( - assignableTypes = { - TableOperationsController.class, - TableOperationsHistoryController.class, - TableStatsController.class - }) -public class GlobalExceptionHandler { - - @ExceptionHandler(ResponseStatusException.class) - public ResponseEntity handleResponseStatus( - ResponseStatusException e, HttpServletRequest req) { - HttpStatus status = HttpStatus.resolve(e.getStatus().value()); - if (status == null) { - status = HttpStatus.INTERNAL_SERVER_ERROR; - } - String message = e.getReason() == null ? status.getReasonPhrase() : e.getReason(); - return ResponseEntity.status(status) - .body( - ApiError.builder() - .code(status.name()) - .message(message) - .path(req.getRequestURI()) - .build()); - } - - @ExceptionHandler(Exception.class) - public ResponseEntity handleUncaught(Exception e, HttpServletRequest req) { - log.warn(String.format("Unhandled exception on %s", req.getRequestURI()), e); - return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) - .body( - ApiError.builder() - .code("INTERNAL_ERROR") - .message("An unexpected error occurred") - .path(req.getRequestURI()) - .build()); - } -} diff --git a/services/optimizer/src/main/resources/application.properties b/services/optimizer/src/main/resources/application.properties index e78745d00..e7f082b47 100644 --- a/services/optimizer/src/main/resources/application.properties +++ b/services/optimizer/src/main/resources/application.properties @@ -18,3 +18,8 @@ spring.datasource.hikari.maximum-pool-size=20 management.endpoints.web.exposure.include=health,prometheus management.endpoint.health.enabled=true + +# Include ResponseStatusException.reason in the default error response body. Without this, Spring +# Boot 2.7 omits the `message` field, and the human-readable detail from a thrown +# ResponseStatusException never reaches the caller. +server.error.include-message=always diff --git a/services/optimizer/src/test/java/com/linkedin/openhouse/optimizer/api/controller/ControllerErrorHandlingTest.java b/services/optimizer/src/test/java/com/linkedin/openhouse/optimizer/api/controller/ControllerErrorHandlingTest.java index 59d793441..b9c8dc3dc 100644 --- a/services/optimizer/src/test/java/com/linkedin/openhouse/optimizer/api/controller/ControllerErrorHandlingTest.java +++ b/services/optimizer/src/test/java/com/linkedin/openhouse/optimizer/api/controller/ControllerErrorHandlingTest.java @@ -20,9 +20,12 @@ import org.springframework.transaction.annotation.Transactional; /** - * Exercises what the controllers own: server-side validation on {@code updateOperation}, 404 on - * missing rows, and the {@code ApiError} body shape. Framework-level 4xx (missing query param, - * malformed JSON, etc.) is left to Spring's defaults and not asserted here. + * Exercises what the controllers own: server-side validation on {@code updateOperation} (path/body + * mismatch, missing fields) and 404s on missing rows. Assertions are status-code-only: MockMvc does + * not trigger Spring's error-dispatch to {@code BasicErrorController}, so the response body of a + * {@link org.springframework.web.server.ResponseStatusException} is empty in tests even though it + * is populated in production (with {@code server.error.include-message=always}). Framework-level + * 4xx (missing query param, malformed JSON, etc.) is left to Spring's defaults and not asserted. */ @SpringBootTest @AutoConfigureMockMvc @@ -34,7 +37,7 @@ class ControllerErrorHandlingTest { @Autowired TableOperationsRepository operationsRepository; @Test - void updateOperation_notFound_returns404WithApiError() throws Exception { + void updateOperation_notFound_returns404() throws Exception { String id = UUID.randomUUID().toString(); String body = String.format("{\"operationId\":\"%s\",\"status\":\"SUCCESS\"}", id); mockMvc @@ -42,10 +45,7 @@ void updateOperation_notFound_returns404WithApiError() throws Exception { post("/v1/optimizer/operations/" + id + "/update") .contentType(MediaType.APPLICATION_JSON) .content(body)) - .andExpect(status().isNotFound()) - .andExpect(jsonPath("$.code").value("NOT_FOUND")) - .andExpect(jsonPath("$.message").value(org.hamcrest.Matchers.containsString(id))) - .andExpect(jsonPath("$.path").value("/v1/optimizer/operations/" + id + "/update")); + .andExpect(status().isNotFound()); } @Test @@ -58,11 +58,7 @@ void updateOperation_pathBodyMismatch_returns400() throws Exception { post("/v1/optimizer/operations/" + pathId + "/update") .contentType(MediaType.APPLICATION_JSON) .content(body)) - .andExpect(status().isBadRequest()) - .andExpect(jsonPath("$.code").value("BAD_REQUEST")) - .andExpect( - jsonPath("$.message") - .value(org.hamcrest.Matchers.containsString("does not match path id"))); + .andExpect(status().isBadRequest()); } @Test @@ -74,9 +70,7 @@ void updateOperation_missingOperationId_returns400() throws Exception { post("/v1/optimizer/operations/" + pathId + "/update") .contentType(MediaType.APPLICATION_JSON) .content(body)) - .andExpect(status().isBadRequest()) - .andExpect(jsonPath("$.code").value("BAD_REQUEST")) - .andExpect(jsonPath("$.message").value("operationId is required")); + .andExpect(status().isBadRequest()); } @Test @@ -88,32 +82,23 @@ void updateOperation_missingStatus_returns400() throws Exception { post("/v1/optimizer/operations/" + id + "/update") .contentType(MediaType.APPLICATION_JSON) .content(body)) - .andExpect(status().isBadRequest()) - .andExpect(jsonPath("$.code").value("BAD_REQUEST")) - .andExpect(jsonPath("$.message").value("status is required")); + .andExpect(status().isBadRequest()); } @Test - void getTableOperation_notFound_returns404WithApiError() throws Exception { + void getTableOperation_notFound_returns404() throws Exception { String id = UUID.randomUUID().toString(); - mockMvc - .perform(get("/v1/optimizer/operations/" + id)) - .andExpect(status().isNotFound()) - .andExpect(jsonPath("$.code").value("NOT_FOUND")) - .andExpect(jsonPath("$.path").value("/v1/optimizer/operations/" + id)); + mockMvc.perform(get("/v1/optimizer/operations/" + id)).andExpect(status().isNotFound()); } @Test - void getTableStats_notFound_returns404WithApiError() throws Exception { + void getTableStats_notFound_returns404() throws Exception { String uuid = UUID.randomUUID().toString(); - mockMvc - .perform(get("/v1/optimizer/stats/" + uuid)) - .andExpect(status().isNotFound()) - .andExpect(jsonPath("$.code").value("NOT_FOUND")); + mockMvc.perform(get("/v1/optimizer/stats/" + uuid)).andExpect(status().isNotFound()); } @Test - void updateOperation_happyPath_stillReturns201() throws Exception { + void updateOperation_happyPath_returns201() throws Exception { String id = UUID.randomUUID().toString(); operationsRepository.save( TableOperationsRow.builder() From bbef386ae56acf32ae9d8d31be1a7b50a2720c1c Mon Sep 17 00:00:00 2001 From: mkuchenbecker Date: Fri, 22 May 2026 12:16:57 -0700 Subject: [PATCH 5/5] refactor(optimizer): revert UpdateOperationRequest doc edits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review — no change needed on this file. The path/body validation lives in the controller; the DTO carries the same fields as before with the existing javadoc. Co-Authored-By: Claude Opus 4.7 --- .../optimizer/api/spec/UpdateOperationRequest.java | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpdateOperationRequest.java b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpdateOperationRequest.java index fcae718ad..a216e9db3 100644 --- a/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpdateOperationRequest.java +++ b/services/optimizer/src/main/java/com/linkedin/openhouse/optimizer/api/spec/UpdateOperationRequest.java @@ -27,16 +27,10 @@ @AllArgsConstructor public class UpdateOperationRequest { - /** - * Operation row's UUID. Must match the {@code {id}} path variable on {@code POST - * /v1/optimizer/operations/{id}/update} — the controller rejects mismatched requests with 400. - * Carrying it in the body keeps the payload self-describing for trace/log consumers that may not - * see the URL. Validated server-side (no bean-validation annotation) so that future relaxation - * does not break clients on the wire contract. - */ + /** Operation row's UUID — the primary lookup key. */ private String operationId; - /** Terminal outcome for this single operation. Validated server-side. */ + /** Terminal outcome for this single operation. */ private HistoryStatus status; /** Debug echo: stable table identity the caller believed it was completing. */