From 409992af7c016e4d915da948f4e14696fb2f91c9 Mon Sep 17 00:00:00 2001 From: Raja Kolli Date: Tue, 17 Dec 2024 16:35:26 +0000 Subject: [PATCH] implements code review comments --- .../config/GlobalExceptionHandler.java | 18 ++++++++++ .../model/query/SearchRequest.java | 24 +++++++++++-- .../model/query/SortRequest.java | 19 ++++++++++ .../services/AnimalService.java | 35 ++++++++++--------- .../web/controllers/AnimalControllerIT.java | 4 +-- .../web/controllers/AnimalControllerTest.java | 31 ++++++++++++++++ .../src/test/resources/logback-test.xml | 1 + 7 files changed, 110 insertions(+), 22 deletions(-) diff --git a/jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/config/GlobalExceptionHandler.java b/jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/config/GlobalExceptionHandler.java index 7aa1fd9fc..69ace4428 100644 --- a/jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/config/GlobalExceptionHandler.java +++ b/jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/config/GlobalExceptionHandler.java @@ -1,6 +1,7 @@ package com.example.keysetpagination.config; import com.example.keysetpagination.exception.ResourceNotFoundException; +import jakarta.validation.ConstraintViolationException; import java.net.URI; import java.time.Instant; import java.util.Comparator; @@ -40,6 +41,23 @@ ProblemDetail onException(MethodArgumentNotValidException methodArgumentNotValid return problemDetail; } + @ExceptionHandler(ConstraintViolationException.class) + ProblemDetail onException(ConstraintViolationException constraintViolationException) { + List apiValidationErrors = constraintViolationException.getConstraintViolations().stream() + .map(constraintViolation -> new ApiValidationError( + null, + constraintViolation.getPropertyPath().toString(), + constraintViolation.getInvalidValue(), + constraintViolation.getMessage())) + .sorted(Comparator.comparing(ApiValidationError::field)) + .toList(); + ProblemDetail problemDetail = ProblemDetail.forStatusAndDetail( + HttpStatusCode.valueOf(400), constraintViolationException.getMessage()); + problemDetail.setTitle("Constraint Violation"); + problemDetail.setProperty("violations", apiValidationErrors); + return problemDetail; + } + @ExceptionHandler(Exception.class) ProblemDetail onException(Exception exception) { if (exception instanceof ResourceNotFoundException resourceNotFoundException) { diff --git a/jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/model/query/SearchRequest.java b/jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/model/query/SearchRequest.java index febd2bb5e..8d774cbce 100644 --- a/jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/model/query/SearchRequest.java +++ b/jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/model/query/SearchRequest.java @@ -1,5 +1,6 @@ package com.example.keysetpagination.model.query; +import java.util.ArrayList; import java.util.List; public class SearchRequest { @@ -8,19 +9,36 @@ public class SearchRequest { private List sortRequests; + public SearchRequest() { + this.searchCriteriaList = new ArrayList<>(); + this.sortRequests = new ArrayList<>(); + } + + public SearchRequest(List searchCriteriaList, List sortRequests) { + this.searchCriteriaList = searchCriteriaList != null ? searchCriteriaList : new ArrayList<>(); + this.sortRequests = sortRequests != null ? sortRequests : new ArrayList<>(); + } + public List getSearchCriteriaList() { return searchCriteriaList; } - public void setSearchCriteriaList(List searchCriteriaList) { + public SearchRequest setSearchCriteriaList(List searchCriteriaList) { this.searchCriteriaList = searchCriteriaList; + return this; } - public List getSortDtos() { + public List getSortRequests() { return sortRequests; } - public void setSortDtos(List sortRequests) { + public SearchRequest setSortRequests(List sortRequests) { this.sortRequests = sortRequests; + return this; + } + + @Override + public String toString() { + return "SearchRequest{" + "searchCriteria=" + searchCriteriaList + ", sortRequests=" + sortRequests + '}'; } } diff --git a/jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/model/query/SortRequest.java b/jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/model/query/SortRequest.java index 9db8f1c53..9b818f499 100644 --- a/jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/model/query/SortRequest.java +++ b/jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/model/query/SortRequest.java @@ -1,5 +1,7 @@ package com.example.keysetpagination.model.query; +import java.util.Objects; + public class SortRequest { private String field; @@ -20,4 +22,21 @@ public String getDirection() { public void setDirection(String direction) { this.direction = direction; } + + @Override + public String toString() { + return "SortRequest{field='" + field + "', direction=" + direction + '}'; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof SortRequest that)) return false; + return Objects.equals(field, that.field) && Objects.equals(direction, that.direction); + } + + @Override + public int hashCode() { + return Objects.hash(field, direction); + } } diff --git a/jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/services/AnimalService.java b/jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/services/AnimalService.java index 993ae7a85..23815aa56 100644 --- a/jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/services/AnimalService.java +++ b/jpa/keyset-pagination/boot-data-window-pagination/src/main/java/com/example/keysetpagination/services/AnimalService.java @@ -5,15 +5,15 @@ import com.example.keysetpagination.mapper.AnimalMapper; import com.example.keysetpagination.model.query.FindAnimalsQuery; import com.example.keysetpagination.model.query.SearchRequest; -import com.example.keysetpagination.model.query.SortRequest; import com.example.keysetpagination.model.request.AnimalRequest; import com.example.keysetpagination.model.response.AnimalResponse; import com.example.keysetpagination.model.response.PagedResult; import com.example.keysetpagination.repositories.AnimalRepository; -import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Optional; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.data.domain.Page; import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Pageable; @@ -29,6 +29,8 @@ @Transactional(readOnly = true) public class AnimalService { + private static final Logger log = LoggerFactory.getLogger(AnimalService.class); + private final AnimalRepository animalRepository; private final AnimalMapper animalMapper; private final EntitySpecification animalEntitySpecification; @@ -74,21 +76,20 @@ public Window searchAnimals(SearchRequest searchRequest, int pag : ScrollPosition.of(Collections.singletonMap("id", scrollId), ScrollPosition.Direction.FORWARD); // Parse and create sort orders - List orders = new ArrayList<>(); - - if (!CollectionUtils.isEmpty(searchRequest.getSortDtos())) { - for (SortRequest sortRequest : searchRequest.getSortDtos()) { - Sort.Direction direction = "desc" - .equalsIgnoreCase(Optional.ofNullable(sortRequest.getDirection()) - .orElse("asc")) - ? Sort.Direction.DESC - : Sort.Direction.ASC; - orders.add(new Sort.Order(direction, sortRequest.getField())); - } - } else { - orders.add(new Sort.Order(Sort.Direction.ASC, "id")); - } - + List orders = CollectionUtils.isEmpty(searchRequest.getSortRequests()) + ? Collections.singletonList(new Sort.Order(Sort.Direction.ASC, "id")) + : searchRequest.getSortRequests().stream() + .map(sortRequest -> { + Sort.Direction direction = "desc" + .equalsIgnoreCase(Optional.ofNullable(sortRequest.getDirection()) + .orElse("asc")) + ? Sort.Direction.DESC + : Sort.Direction.ASC; + return new Sort.Order(direction, sortRequest.getField()); + }) + .toList(); + + log.debug("Executing search with specification: {} and sort orders: {}", specification, orders); return animalRepository .findAll(specification, PageRequest.of(0, pageSize, Sort.by(orders)), position, Animal.class) .map(animalMapper::toResponse); diff --git a/jpa/keyset-pagination/boot-data-window-pagination/src/test/java/com/example/keysetpagination/web/controllers/AnimalControllerIT.java b/jpa/keyset-pagination/boot-data-window-pagination/src/test/java/com/example/keysetpagination/web/controllers/AnimalControllerIT.java index d9e3dc7d1..9b82e9641 100644 --- a/jpa/keyset-pagination/boot-data-window-pagination/src/test/java/com/example/keysetpagination/web/controllers/AnimalControllerIT.java +++ b/jpa/keyset-pagination/boot-data-window-pagination/src/test/java/com/example/keysetpagination/web/controllers/AnimalControllerIT.java @@ -434,7 +434,7 @@ void shouldReturnResultForLikeOperator() throws Exception { """)) .andExpect(status().isOk()) .andExpect(jsonPath( - "$.content.size()", is(6))) // "Elephant", "Penguin", "Crocodile", ""Eagle"", "Whale", "Snake" + "$.content.size()", is(6))) // "Elephant", "Penguin", "Crocodile", "Eagle", "Whale", "Snake" .andExpect(jsonPath("$.last", is(true))); } @@ -535,7 +535,7 @@ void shouldReturnResultForBetweenOperator() throws Exception { "field": "id", "values": [ "%d", - "%s" + "%d" ] } ] diff --git a/jpa/keyset-pagination/boot-data-window-pagination/src/test/java/com/example/keysetpagination/web/controllers/AnimalControllerTest.java b/jpa/keyset-pagination/boot-data-window-pagination/src/test/java/com/example/keysetpagination/web/controllers/AnimalControllerTest.java index 30d170685..583327a20 100644 --- a/jpa/keyset-pagination/boot-data-window-pagination/src/test/java/com/example/keysetpagination/web/controllers/AnimalControllerTest.java +++ b/jpa/keyset-pagination/boot-data-window-pagination/src/test/java/com/example/keysetpagination/web/controllers/AnimalControllerTest.java @@ -19,6 +19,7 @@ import com.example.keysetpagination.entities.Animal; import com.example.keysetpagination.exception.AnimalNotFoundException; import com.example.keysetpagination.model.query.FindAnimalsQuery; +import com.example.keysetpagination.model.query.SearchRequest; import com.example.keysetpagination.model.request.AnimalRequest; import com.example.keysetpagination.model.response.AnimalResponse; import com.example.keysetpagination.model.response.PagedResult; @@ -113,6 +114,36 @@ void shouldFetchAllAnimalsWithCustomPageSize() throws Exception { .andExpect(jsonPath("$.hasPrevious", is(true))); } + @Test + void shouldReturnBadRequestWhenPageSizeIsLessThanMin() throws Exception { + SearchRequest searchRequest = new SearchRequest(); + mockMvc.perform(post("/api/animals/search") + .param("pageSize", "0") // Less than minimum value of 1 + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(searchRequest))) + .andExpect(status().isBadRequest()); + } + + @Test + void shouldReturnBadRequestWhenPageSizeExceedsMax() throws Exception { + SearchRequest searchRequest = new SearchRequest(); + mockMvc.perform(post("/api/animals/search") + .param("pageSize", "101") // Exceeds maximum value of 100 + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(searchRequest))) + .andExpect(status().isBadRequest()); + } + + @Test + void shouldReturnOkWhenPageSizeIsWithinValidRange() throws Exception { + SearchRequest searchRequest = new SearchRequest(); + mockMvc.perform(post("/api/animals/search") + .param("pageSize", "50") // Within valid range (1-100) + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(searchRequest))) + .andExpect(status().isOk()); + } + @Test void shouldFindAnimalById() throws Exception { Long animalId = 1L; diff --git a/jpa/keyset-pagination/boot-data-window-pagination/src/test/resources/logback-test.xml b/jpa/keyset-pagination/boot-data-window-pagination/src/test/resources/logback-test.xml index 92589da77..6a5d8ad37 100644 --- a/jpa/keyset-pagination/boot-data-window-pagination/src/test/resources/logback-test.xml +++ b/jpa/keyset-pagination/boot-data-window-pagination/src/test/resources/logback-test.xml @@ -12,4 +12,5 @@ +