(wip) feat(optimizer): basic error-code handling across controllers by mkuchenbecker · Pull Request #596 · linkedin/openhouse

mkuchenbecker · 2026-05-22T17:12:20Z

Status

(WIP) — inspection branch on top of mkuchenb/optimizer-2 (PR #531). Addresses the review comment "I think we could add basic error handling with error codes for all the controllers." Open for review before folding into #531.

Summary

Every optimizer endpoint now returns a uniform ApiError body on non-2xx. Reshapes updateOperation to put operationId in the URL (and keeps it in the body for self-describing payloads).

Error body shape

{
  "code": "OPERATION_NOT_FOUND",
  "message": "no operation with id 'abc-123'",
  "path": "/v1/optimizer/operations/abc-123/update"
}

Status code mapping

Scenario	Status	`code`
Operation not found	404	`OPERATION_NOT_FOUND`
Stats not found	404	`STATS_NOT_FOUND`
Bean-validation failure (`@NotNull`/`@NotBlank`)	400	`VALIDATION_ERROR`
Path id vs body operationId mismatch	400	`PATH_BODY_MISMATCH`
Bad enum / bad date / bad int on query param	400	`INVALID_PARAMETER`
Missing required query param (e.g. `limit`)	400	`MISSING_PARAMETER`
Malformed JSON body	400	`MALFORMED_REQUEST`
Anything else	500	`INTERNAL_ERROR`

Path change

POST /v1/optimizer/operations/update (body {operationId, status, ...})
→ POST /v1/optimizer/operations/{id}/update (body still carries operationId, must match path).

URL identifies the resource being acted on, so 404 semantics make sense. Body retains operationId for self-describing payloads (trace/log consumers that may not see the URL).

Changes

New api/error/ApiError.java — {code, message, path} DTO.
New api/error/GlobalExceptionHandler.java — @RestControllerAdvice with one @ExceptionHandler per framework exception. Parses ResponseStatusException.reason as CODE: message so callers can pack endpoint-specific codes.
UpdateOperationRequest: @NotBlank operationId, @NotNull status.
UpsertTableStatsRequest: @NotBlank databaseName, @NotBlank tableName.
All controllers: orElseThrow(ResponseStatusException) replaces bare ResponseEntity.notFound().build(). @Valid on @RequestBody parameters.
TableOperationsController.updateOperation: new path + path/body equality check.
build.gradle: add spring-boot-starter-validation:2.7.8.

Testing Done

Added new tests for the changes made.

./gradlew :services:optimizer:test — all green.

New ControllerErrorHandlingTest (13 MockMvc cases):

updateOperation: not-found, path/body mismatch, missing status, missing operationId, malformed JSON, happy-path sanity.
getTableOperation: not-found.
listOperations: missing limit, bad limit, bad enum.
getTableStats: not-found.
upsertStats: missing required field.
getStatsHistory: bad since format.

🤖 Generated with Claude Code

@RequestParam

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 <noreply@anthropic.com>

@notblank

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 <noreply@anthropic.com>

mkuchenbecker · 2026-05-22T17:28:59Z

+    // 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).


I have no idea what this means or why we would do it.

@notblank

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 <noreply@anthropic.com>

…ng 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 <noreply@anthropic.com>

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 <noreply@anthropic.com>

mkuchenbecker and others added 2 commits May 21, 2026 16:36