Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat : adds dynamic sort #1571

Merged
merged 5 commits into from
Dec 17, 2024
Merged

feat : adds dynamic sort #1571

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7dccc33
feat : adds dynamic sort
rajadilipkolli Dec 17, 2024
c0266d5
feat : open for extension, closed for modification
rajadilipkolli Dec 17, 2024
409992a
implements code review comments
rajadilipkolli Dec 17, 2024
326ec24
enhance logging
rajadilipkolli Dec 17, 2024
ab911f3
removes wild card import
rajadilipkolli Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions ...-pagination/src/main/java/com/example/keysetpagination/config/GlobalExceptionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -40,6 +41,26 @@ ProblemDetail onException(MethodArgumentNotValidException methodArgumentNotValid
return problemDetail;
}

@ExceptionHandler(ConstraintViolationException.class)
ProblemDetail onException(ConstraintViolationException constraintViolationException) {
List<ApiValidationError> apiValidationErrors = constraintViolationException.getConstraintViolations().stream()
.map(constraintViolation -> new ApiValidationError(
constraintViolation.getRootBeanClass().getSimpleName(),
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.setType(URI.create("https://api.boot-data-window-pagination.com/errors/validation"));
problemDetail.setProperty("errorCategory", "Validation");
problemDetail.setProperty("timestamp", Instant.now());
problemDetail.setProperty("violations", apiValidationErrors);
return problemDetail;
}

@ExceptionHandler(Exception.class)
ProblemDetail onException(Exception exception) {
if (exception instanceof ResourceNotFoundException resourceNotFoundException) {
Expand Down
10 changes: 10 additions & 0 deletions ...dow-pagination/src/main/java/com/example/keysetpagination/model/query/SearchCriteria.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import jakarta.validation.constraints.NotNull;
import jakarta.validation.constraints.Size;
import java.util.List;
import java.util.StringJoiner;

public class SearchCriteria {

Expand Down Expand Up @@ -48,4 +49,13 @@ public List<String> getValues() {
public void setValues(List<String> values) {
this.values = values;
}

@Override
public String toString() {
return new StringJoiner(", ", SearchCriteria.class.getSimpleName() + "[", "]")
.add("queryOperator=" + queryOperator)
.add("field='" + field + "'")
.add("values=" + values)
.toString();
}
}
44 changes: 44 additions & 0 deletions ...ndow-pagination/src/main/java/com/example/keysetpagination/model/query/SearchRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package com.example.keysetpagination.model.query;

import java.util.ArrayList;
import java.util.List;

public class SearchRequest {

private List<SearchCriteria> searchCriteriaList;

private List<SortRequest> sortRequests;

public SearchRequest() {
this.searchCriteriaList = new ArrayList<>();
this.sortRequests = new ArrayList<>();
}

public SearchRequest(List<SearchCriteria> searchCriteriaList, List<SortRequest> sortRequests) {
this.searchCriteriaList = searchCriteriaList != null ? searchCriteriaList : new ArrayList<>();
this.sortRequests = sortRequests != null ? sortRequests : new ArrayList<>();
}

public List<SearchCriteria> getSearchCriteriaList() {
return searchCriteriaList;
}

public SearchRequest setSearchCriteriaList(List<SearchCriteria> searchCriteriaList) {
this.searchCriteriaList = searchCriteriaList;
return this;
}

public List<SortRequest> getSortRequests() {
return sortRequests;
}

public SearchRequest setSortRequests(List<SortRequest> sortRequests) {
this.sortRequests = sortRequests;
return this;
}
rajadilipkolli marked this conversation as resolved.
Show resolved Hide resolved

@Override
public String toString() {
return "SearchRequest{" + "searchCriteria=" + searchCriteriaList + ", sortRequests=" + sortRequests + '}';
}
}
42 changes: 42 additions & 0 deletions ...window-pagination/src/main/java/com/example/keysetpagination/model/query/SortRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package com.example.keysetpagination.model.query;

import java.util.Objects;

public class SortRequest {

private String field;
private String direction;

Comment on lines +7 to +9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation for sort direction field

The direction field should be constrained to valid sort directions to prevent runtime errors.

Consider using an enum and adding validation:

-    private String direction;
+    private Direction direction;
+
+    public enum Direction {
+        ASC, DESC;
+        
+        public static Direction fromString(String value) {
+            try {
+                return Direction.valueOf(value.toUpperCase());
+            } catch (IllegalArgumentException e) {
+                throw new IllegalArgumentException("Invalid sort direction. Use ASC or DESC");
+            }
+        }
+    }

Committable suggestion skipped: line range outside the PR's diff.

public String getField() {
return field;
}

public void setField(String field) {
this.field = field;
}

public String getDirection() {
return direction;
}

public void setDirection(String direction) {
this.direction = direction;
}

@Override
public String toString() {
return String.format("SortRequest{field='%s', direction='%s'}", field, 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);
}
}
35 changes: 27 additions & 8 deletions ...-window-pagination/src/main/java/com/example/keysetpagination/services/AnimalService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,16 @@
import com.example.keysetpagination.exception.AnimalNotFoundException;
import com.example.keysetpagination.mapper.AnimalMapper;
import com.example.keysetpagination.model.query.FindAnimalsQuery;
import com.example.keysetpagination.model.query.SearchCriteria;
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;
import com.example.keysetpagination.repositories.AnimalRepository;
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;
Expand All @@ -21,11 +23,14 @@
import org.springframework.data.jpa.domain.Specification;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
import org.springframework.util.CollectionUtils;

@Service
@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<Animal> animalEntitySpecification;
Expand Down Expand Up @@ -60,22 +65,36 @@
return PageRequest.of(pageNo, findAnimalsQuery.pageSize(), sort);
}

public Window<AnimalResponse> searchAnimals(List<SearchCriteria> searchCriteriaList, int pageSize, Long scrollId) {
public Window<AnimalResponse> searchAnimals(SearchRequest searchRequest, int pageSize, Long scrollId) {

Specification<Animal> specification =
animalEntitySpecification.specificationBuilder(searchCriteriaList, Animal.class);
animalEntitySpecification.specificationBuilder(searchRequest.getSearchCriteriaList(), Animal.class);

// Create initial ScrollPosition or continue from the given scrollId
ScrollPosition position = scrollId == null
? ScrollPosition.keyset()
: ScrollPosition.of(Collections.singletonMap("id", scrollId), ScrollPosition.Direction.FORWARD);

// Parse and create sort orders
List<Sort.Order> 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();
Comment on lines +79 to +90
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider adding field name validation and sanitization.

While the sort implementation effectively uses Java 8+ features, it could benefit from additional security measures:

  1. Validate sort fields against a whitelist of allowed entity properties
  2. Sanitize field names to prevent SQL injection

Here's a suggested implementation:

private static final Set<String> ALLOWED_SORT_FIELDS = 
    Set.of("id", "name", "species", "age"); // add all valid fields

private List<Sort.Order> createSortOrders(List<SortRequest> sortRequests) {
    if (CollectionUtils.isEmpty(sortRequests)) {
        return Collections.singletonList(new Sort.Order(Sort.Direction.ASC, "id"));
    }
    
    return sortRequests.stream()
            .map(sortRequest -> {
                String field = sortRequest.getField();
                if (!ALLOWED_SORT_FIELDS.contains(field)) {
                    throw new IllegalArgumentException("Invalid sort field: " + field);
                }
                
                Sort.Direction direction = "desc"
                        .equalsIgnoreCase(Optional.ofNullable(sortRequest.getDirection())
                                .orElse("asc"))
                        ? Sort.Direction.DESC
                        : Sort.Direction.ASC;
                return new Sort.Order(direction, field);
            })
            .toList();
}


log.debug(
"Executing search with criteria: {} and sort orders: {}",
searchRequest.getSearchCriteriaList(),
orders);
Comment on lines +92 to +95

Check notice

Code scanning / SonarCloud

Logging should not be vulnerable to injection attacks Low

Change this code to not log user-controlled data. See more on SonarQube Cloud
return animalRepository
.findAll(
specification,
PageRequest.of(0, pageSize, Sort.by(Sort.Order.asc("id"))),
position,
Animal.class)
.findAll(specification, PageRequest.of(0, pageSize, Sort.by(orders)), position, Animal.class)
.map(animalMapper::toResponse);
}

Expand Down
16 changes: 9 additions & 7 deletions ...gination/src/main/java/com/example/keysetpagination/web/controllers/AnimalController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,21 @@

import com.example.keysetpagination.exception.AnimalNotFoundException;
import com.example.keysetpagination.model.query.FindAnimalsQuery;
import com.example.keysetpagination.model.query.SearchCriteria;
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;
import com.example.keysetpagination.services.AnimalService;
import com.example.keysetpagination.utils.AppConstants;
import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.Parameter;
import io.swagger.v3.oas.annotations.enums.ParameterIn;
import io.swagger.v3.oas.annotations.responses.ApiResponse;
import io.swagger.v3.oas.annotations.responses.ApiResponses;
import jakarta.validation.Valid;
import jakarta.validation.constraints.Max;
import jakarta.validation.constraints.Min;
import java.net.URI;
import java.util.List;
import org.springframework.data.domain.Window;
import org.springframework.http.ResponseEntity;
import org.springframework.validation.annotation.Validated;
Expand All @@ -38,7 +38,7 @@ class AnimalController {

private final AnimalService animalService;

public AnimalController(AnimalService animalService) {
AnimalController(AnimalService animalService) {
this.animalService = animalService;
}

Expand All @@ -65,15 +65,17 @@ PagedResult<AnimalResponse> getAllAnimals(
})
@PostMapping("/search")
public Window<AnimalResponse> searchAnimals(
@Parameter(description = "Number of items per page (max 100)")
@Parameter(description = "Number of items per page (max 100)", in = ParameterIn.QUERY)
rajadilipkolli marked this conversation as resolved.
Show resolved Hide resolved
@RequestParam(defaultValue = "10")
@Min(1)
@Max(100)
int pageSize,
@Parameter(description = "Scroll ID for pagination") @RequestParam(required = false) Long scrollId,
@RequestBody @Valid List<SearchCriteria> searchCriteria) {
@Parameter(description = "Scroll ID for pagination", in = ParameterIn.QUERY) @RequestParam(required = false)
Long scrollId,
@io.swagger.v3.oas.annotations.parameters.RequestBody(required = true) @RequestBody @Valid
SearchRequest searchRequest) {

return animalService.searchAnimals(searchCriteria, pageSize, scrollId);
return animalService.searchAnimals(searchRequest, pageSize, scrollId);
}

@GetMapping("/{id}")
Expand Down
Loading
Loading