fix: Invalid Parameters handling in Planning History API #260

.github/workflows/pr-check.yml

@@ -0,0 +1,43 @@
+name: PR Check
+
+on:
+  pull_request:
+    branches: [ "**" ]
+
+jobs:
+  spotless:
+    name: Code Style Spotless Check
+    runs-on: ubuntu-latest
+    permissions:
+      contents: read
+    steps:
+      - uses: actions/checkout@v4
+
+      - name: Set up Java
+        uses: actions/setup-java@v4
+        with:
+          java-version: '25'
+          distribution: 'temurin'
+          cache: maven
+
+      - name: Run spotless plugin check
+        run: ./mvnw --no-transfer-progress spotless:check
+
+  verify:
+    name: Maven Build & Test
+    needs: spotless
+    runs-on: ubuntu-latest
+    permissions:
+      contents: read
+    steps:
+      - uses: actions/checkout@v4
+
+      - name: Set up Java
+        uses: actions/setup-java@v4
+        with:
+          java-version: '25'
+          distribution: 'temurin'
+          cache: maven
+
+      - name: Run mvn verify
+        run: ./mvnw --no-transfer-progress verify

src/main/java/com/sivalabs/ft/features/api/GlobalExceptionHandler.java

@@ -4,19 +4,51 @@
 
 import com.sivalabs.ft.features.domain.exceptions.BadRequestException;
 import com.sivalabs.ft.features.domain.exceptions.ResourceNotFoundException;
+import jakarta.validation.ConstraintViolationException;
 import java.time.Instant;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.http.ProblemDetail;
 import org.springframework.http.converter.HttpMessageNotReadableException;
 import org.springframework.web.bind.annotation.ExceptionHandler;
 import org.springframework.web.bind.annotation.RestControllerAdvice;
+import org.springframework.web.method.annotation.HandlerMethodValidationException;
+import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException;
 
 @RestControllerAdvice
 class GlobalExceptionHandler {
 
     private static final Logger log = LoggerFactory.getLogger(GlobalExceptionHandler.class);
 
+    @ExceptionHandler(MethodArgumentTypeMismatchException.class)
+    ProblemDetail handle(MethodArgumentTypeMismatchException e) {
+        log.error("Method argument type mismatch", e);
+        String detail = String.format("Invalid value for parameter '%s'", e.getName());
+        ProblemDetail problemDetail = ProblemDetail.forStatusAndDetail(BAD_REQUEST, detail);
+        problemDetail.setTitle("Invalid Request Parameter");
+        problemDetail.setProperty("timestamp", Instant.now());
+        return problemDetail;
+    }
+
+    @ExceptionHandler(HandlerMethodValidationException.class)
+    ProblemDetail handle(HandlerMethodValidationException e) {
+        log.error("Handler method validation failed", e);
+        ProblemDetail problemDetail =
+                ProblemDetail.forStatusAndDetail(BAD_REQUEST, "Validation failed for request parameters");
+        problemDetail.setTitle("Validation Failed");
+        problemDetail.setProperty("timestamp", Instant.now());
+        return problemDetail;
+    }
+
+    @ExceptionHandler(ConstraintViolationException.class)
+    ProblemDetail handle(ConstraintViolationException e) {
+        log.error("Constraint violation", e);
+        ProblemDetail problemDetail = ProblemDetail.forStatusAndDetail(BAD_REQUEST, e.getMessage());
+        problemDetail.setTitle("Constraint Violation");
+        problemDetail.setProperty("timestamp", Instant.now());
+        return problemDetail;
+    }
+
     @ExceptionHandler(Exception.class)
     ProblemDetail handle(Exception e) {
         log.error("Unhandled exception", e);

src/main/java/com/sivalabs/ft/features/api/controllers/PlanningHistoryController.java

@@ -7,14 +7,17 @@
 import com.sivalabs.ft.features.domain.models.EntityType;
 import io.swagger.v3.oas.annotations.Operation;
 import io.swagger.v3.oas.annotations.tags.Tag;
+import jakarta.validation.constraints.Min;
 import java.time.Instant;
+import org.springframework.validation.annotation.Validated;
 import org.springframework.web.bind.annotation.GetMapping;
 import org.springframework.web.bind.annotation.PathVariable;
 import org.springframework.web.bind.annotation.RequestParam;
 import org.springframework.web.bind.annotation.RestController;
 
 @RestController
 @Tag(name = "Planning History API")
+@Validated
 class PlanningHistoryController {
     private final PlanningHistoryService planningHistoryService;
 
@@ -31,8 +34,8 @@ PagedResult<PlanningHistoryDto> getPlanningHistory(
             @RequestParam(required = false) ChangeType changeType,
             @RequestParam(required = false) Instant dateFrom,
             @RequestParam(required = false) Instant dateTo,
-            @RequestParam(defaultValue = "0") int page,
-            @RequestParam(defaultValue = "20") int size,
+            @RequestParam(defaultValue = "0") @Min(0) int page,
+            @RequestParam(defaultValue = "20") @Min(1) int size,
             @RequestParam(required = false) String sort) {
         return PagedResult.from(planningHistoryService.findHistory(
                 entityType, entityCode, changedBy, changeType, dateFrom, dateTo, page, size, sort));
@@ -42,8 +45,8 @@ PagedResult<PlanningHistoryDto> getPlanningHistory(
     @Operation(summary = "Get planning history for a specific release")
     PagedResult<PlanningHistoryDto> getReleaseHistory(
             @PathVariable String code,
-            @RequestParam(defaultValue = "0") int page,
-            @RequestParam(defaultValue = "20") int size,
+            @RequestParam(defaultValue = "0") @Min(0) int page,
+            @RequestParam(defaultValue = "20") @Min(1) int size,
             @RequestParam(required = false) String sort) {
         return PagedResult.from(
                 planningHistoryService.findHistory(EntityType.RELEASE, code, null, null, null, null, page, size, sort));
@@ -53,8 +56,8 @@ PagedResult<PlanningHistoryDto> getReleaseHistory(
     @Operation(summary = "Get planning history for a specific feature")
     PagedResult<PlanningHistoryDto> getFeatureHistory(
             @PathVariable String code,
-            @RequestParam(defaultValue = "0") int page,
-            @RequestParam(defaultValue = "20") int size,
+            @RequestParam(defaultValue = "0") @Min(0) int page,
+            @RequestParam(defaultValue = "20") @Min(1) int size,
             @RequestParam(required = false) String sort) {
         return PagedResult.from(
                 planningHistoryService.findHistory(EntityType.FEATURE, code, null, null, null, null, page, size, sort));

src/main/java/com/sivalabs/ft/features/domain/PlanningHistoryService.java

@@ -4,13 +4,15 @@
 import com.sivalabs.ft.features.domain.entities.Feature;
 import com.sivalabs.ft.features.domain.entities.PlanningHistory;
 import com.sivalabs.ft.features.domain.entities.Release;
+import com.sivalabs.ft.features.domain.exceptions.BadRequestException;
 import com.sivalabs.ft.features.domain.mappers.PlanningHistoryMapper;
 import com.sivalabs.ft.features.domain.models.ChangeType;
 import com.sivalabs.ft.features.domain.models.EntityType;
 import jakarta.persistence.criteria.Predicate;
 import java.time.Instant;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Set;
 import org.springframework.data.domain.Page;
 import org.springframework.data.domain.PageRequest;
 import org.springframework.data.domain.Pageable;
@@ -23,6 +25,18 @@
 public class PlanningHistoryService {
     private static final int MAX_VALUE_LENGTH = 1000;
     private static final int MAX_RATIONALE_LENGTH = 500;
+    private static final Set<String> VALID_SORT_FIELDS = Set.of(
+            "id",
+            "entityType",
+            "entityId",
+            "entityCode",
+            "changeType",
+            "fieldName",
+            "oldValue",
+            "newValue",
+            "rationale",
+            "changedBy",
+            "changedAt");
 
     private final PlanningHistoryRepository planningHistoryRepository;
     private final PlanningHistoryMapper planningHistoryMapper;
@@ -184,6 +198,9 @@ private Sort parseSort(String sort) {
         }
         String[] parts = sort.split(",");
         String field = parts[0].trim();
+        if (!VALID_SORT_FIELDS.contains(field)) {
+            throw new BadRequestException("Invalid sort field: " + field);
+        }
         Sort.Direction direction =
                 parts.length > 1 && "asc".equalsIgnoreCase(parts[1].trim()) ? Sort.Direction.ASC : Sort.Direction.DESC;
         return Sort.by(direction, field);

src/test/java/com/sivalabs/ft/features/api/controllers/PlanningHistoryControllerTests.java

@@ -230,4 +230,130 @@ void shouldTrackFeatureDeletion() {
                 .extracting(Number::intValue)
                 .satisfies(n -> assertThat(n).isGreaterThanOrEqualTo(1));
     }
+
+    @Test
+    @WithMockOAuth2User(username = "testuser")
+    void shouldReturn400ForInvalidPageNumber() {
+        var result = mvc.get().uri("/api/planning-history?page=-1").exchange();
+
+        assertThat(result.getMvcResult().getResponse().getStatus()).isEqualTo(400);
+    }
+
+    @Test
+    @WithMockOAuth2User(username = "testuser")
+    void shouldReturn400ForInvalidPageSize() {
+        var result = mvc.get().uri("/api/planning-history?size=0").exchange();
+
+        assertThat(result.getMvcResult().getResponse().getStatus()).isEqualTo(400);
+    }
+
+    @Test
+    @WithMockOAuth2User(username = "testuser")
+    void shouldReturn400ForInvalidEntityType() {
+        var result =
+                mvc.get().uri("/api/planning-history?entityType=INVALID_TYPE").exchange();
+
+        assertThat(result.getMvcResult().getResponse().getStatus()).isEqualTo(400);
+    }
+
+    @Test
+    @WithMockOAuth2User(username = "testuser")
+    void shouldReturn400ForInvalidChangeType() {
+        var result =
+                mvc.get().uri("/api/planning-history?changeType=INVALID_CHANGE").exchange();
+
+        assertThat(result.getMvcResult().getResponse().getStatus()).isEqualTo(400);
+    }
+
+    @Test
+    @WithMockOAuth2User(username = "testuser")
+    void shouldReturn400ForInvalidDateFormat() {
+        var result =
+                mvc.get().uri("/api/planning-history?dateFrom=invalid-date").exchange();
+
+        assertThat(result.getMvcResult().getResponse().getStatus()).isEqualTo(400);
+    }
+
+    @Test
+    @WithMockOAuth2User(username = "testuser")
+    void shouldReturn400ForInvalidSortParameter() {
+        var result =
+                mvc.get().uri("/api/planning-history?sort=invalidField,desc").exchange();
+
+        assertThat(result.getMvcResult().getResponse().getStatus()).isEqualTo(400);
+    }
+
+    @Test
+    @WithMockOAuth2User(username = "testuser")
+    void shouldReturn400ForInvalidDateTo() {
+        var result = mvc.get().uri("/api/planning-history?dateTo=invalid-date").exchange();
+
+        assertThat(result.getMvcResult().getResponse().getStatus()).isEqualTo(400);
+    }
+
+    @Test
+    @WithMockOAuth2User(username = "testuser")
+    void shouldReturn400ForNegativePageSize() {
+        var result = mvc.get().uri("/api/planning-history?size=-5").exchange();
+
+        assertThat(result.getMvcResult().getResponse().getStatus()).isEqualTo(400);
+    }
+
+    @Test
+    @WithMockOAuth2User(username = "testuser")
+    void shouldReturn400ForInvalidPageNumberOnFeatureHistory() {
+        var result =
+                mvc.get().uri("/api/features/{code}/history?page=-1", "IDEA-1").exchange();
+
+        assertThat(result.getMvcResult().getResponse().getStatus()).isEqualTo(400);
+    }
+
+    @Test
+    @WithMockOAuth2User(username = "testuser")
+    void shouldReturn400ForInvalidPageSizeOnFeatureHistory() {
+        var result =
+                mvc.get().uri("/api/features/{code}/history?size=0", "IDEA-1").exchange();
+
+        assertThat(result.getMvcResult().getResponse().getStatus()).isEqualTo(400);
+    }
+
+    @Test
+    @WithMockOAuth2User(username = "testuser")
+    void shouldReturn400ForInvalidSortOnFeatureHistory() {
+        var result = mvc.get()
+                .uri("/api/features/{code}/history?sort=invalidField,desc", "IDEA-1")
+                .exchange();
+
+        assertThat(result.getMvcResult().getResponse().getStatus()).isEqualTo(400);
+    }
+
+    @Test
+    @WithMockOAuth2User(username = "testuser")
+    void shouldReturn400ForInvalidPageNumberOnReleaseHistory() {
+        var result = mvc.get()
+                .uri("/api/releases/{code}/history?page=-1", "IDEA-2023.3.8")
+                .exchange();
+
+        assertThat(result.getMvcResult().getResponse().getStatus()).isEqualTo(400);
+    }
+
+    @Test
+    @WithMockOAuth2User(username = "testuser")
+    void shouldReturn400ForInvalidPageSizeOnReleaseHistory() {
+        var result = mvc.get()
+                .uri("/api/releases/{code}/history?size=0", "IDEA-2023.3.8")
+                .exchange();
+
+        assertThat(result.getMvcResult().getResponse().getStatus()).isEqualTo(400);
+    }
+
+    @Test
+    @WithMockOAuth2User(username = "testuser")
+    void shouldReturn400ForInvalidSortOnReleaseHistory() {
+        var result = mvc.get()
+                .uri("/api/releases/{code}/history?sort=invalidField,desc", "IDEA-2023.3.8")
+                .exchange();
+
+        assertThat(result.getMvcResult().getResponse().getStatus()).isEqualTo(400);
+    }
 }