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..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,11 +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 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; @@ -19,6 +21,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 +32,37 @@ 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, @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, + 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( - 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, String.format("no operation with id %s", id))); } /** Fetch a single operation row by its ID, regardless of status. Returns 404 if not found. */ @@ -55,12 +72,15 @@ public ResponseEntity getTableOperation(@PathVariable String id .getTableOperation(id) .map(TableOperations::fromModel) .map(ResponseEntity::ok) - .orElse(ResponseEntity.notFound().build()); + .orElseThrow( + () -> + new ResponseStatusException( + HttpStatus.NOT_FOUND, String.format("no operation with id %s", 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 +88,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 +97,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..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 @@ -9,6 +9,7 @@ import java.util.Optional; import java.util.stream.Collectors; 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 +18,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 @@ -44,24 +46,29 @@ public ResponseEntity getTableStats(@PathVariable String tableUuid) .getTableStats(tableUuid) .map(TableStats::fromModel) .map(ResponseEntity::ok) - .orElse(ResponseEntity.notFound().build()); + .orElseThrow( + () -> + new ResponseStatusException( + HttpStatus.NOT_FOUND, String.format("no stats for tableUuid %s", 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 +76,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..e7f082b47 100644 --- a/services/optimizer/src/main/resources/application.properties +++ b/services/optimizer/src/main/resources/application.properties @@ -16,7 +16,10 @@ 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 + +# 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 new file mode 100644 index 000000000..b9c8dc3dc --- /dev/null +++ b/services/optimizer/src/test/java/com/linkedin/openhouse/optimizer/api/controller/ControllerErrorHandlingTest.java @@ -0,0 +1,124 @@ +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.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; +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 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 +@ActiveProfiles("test") +@Transactional +class ControllerErrorHandlingTest { + + @Autowired MockMvc mockMvc; + @Autowired TableOperationsRepository operationsRepository; + + @Test + void updateOperation_notFound_returns404() throws Exception { + String id = UUID.randomUUID().toString(); + 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()); + } + + @Test + void updateOperation_pathBodyMismatch_returns400() throws Exception { + String pathId = UUID.randomUUID().toString(); + String bodyId = UUID.randomUUID().toString(); + 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()); + } + + @Test + void updateOperation_missingOperationId_returns400() 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()); + } + + @Test + void updateOperation_missingStatus_returns400() throws Exception { + String id = UUID.randomUUID().toString(); + String body = String.format("{\"operationId\":\"%s\"}", id); + mockMvc + .perform( + post("/v1/optimizer/operations/" + id + "/update") + .contentType(MediaType.APPLICATION_JSON) + .content(body)) + .andExpect(status().isBadRequest()); + } + + @Test + void getTableOperation_notFound_returns404() throws Exception { + String id = UUID.randomUUID().toString(); + mockMvc.perform(get("/v1/optimizer/operations/" + id)).andExpect(status().isNotFound()); + } + + @Test + void getTableStats_notFound_returns404() throws Exception { + String uuid = UUID.randomUUID().toString(); + mockMvc.perform(get("/v1/optimizer/stats/" + uuid)).andExpect(status().isNotFound()); + } + + @Test + void updateOperation_happyPath_returns201() 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(Instant.now()) + .scheduledAt(Instant.now()) + .jobId("job-x") + .build()); + 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().isCreated()) + .andExpect(jsonPath("$.status").value("SUCCESS")); + } +} 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); }