Fixed validation of sort parameter in pageable #8072
Conversation
WalkthroughThis pull request introduces significant enhancements to pagination and sorting mechanisms within the Green City application. Key modifications include the addition of a Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
core/src/main/java/greencity/controller/PlaceController.java (2)
Line range hint
454-457: Fix response handling in delete method.The current implementation attempts to set the response status after the method returns, which has no effect. Consider returning a
ResponseEntityinstead.Apply this diff to fix the response handling:
- public void delete(@PathVariable Long id) { - placeService.deleteById(id); - ResponseEntity.status(HttpStatus.OK); + public ResponseEntity<Void> delete(@PathVariable Long id) { + placeService.deleteById(id); + return ResponseEntity.ok().build();
Line range hint
486-491: Enhance error handling for bulk delete operation.The current implementation of parsing comma-separated IDs could throw uncaught exceptions for invalid inputs (e.g., non-numeric values, malformed strings).
Consider adding proper validation and error handling:
public ResponseEntity<Long> bulkDelete( @Parameter(description = "Ids of places separated by a comma \n e.g. 1,2", required = true) @RequestParam String ids) { + if (ids == null || ids.trim().isEmpty()) { + throw new IllegalArgumentException("IDs cannot be empty"); + } + List<Long> idList = new ArrayList<>(); + for (String id : ids.split(",")) { + try { + idList.add(Long.valueOf(id.trim())); + } catch (NumberFormatException e) { + throw new IllegalArgumentException("Invalid ID format: " + id); + } + } - return ResponseEntity.status(HttpStatus.OK).body(placeService.bulkDelete(Arrays.stream(ids.split(",")) - .map(Long::valueOf) - .collect(Collectors.toList()))); + return ResponseEntity.status(HttpStatus.OK).body(placeService.bulkDelete(idList));
🧹 Nitpick comments (24)
core/src/main/java/greencity/controller/PlaceCommentController.java (1)
Line range hint
134-135: Consider using @PathVariable for the delete endpoint.The delete endpoint accepts the
idparameter but doesn't use@PathVariable. Consider updating the endpoint to follow REST conventions:- @DeleteMapping("comments") - public ResponseEntity<Object> delete(Long id) { + @DeleteMapping("comments/{id}") + public ResponseEntity<Object> delete(@PathVariable Long id) {This makes the API more RESTful and consistent with other endpoints in the controller.
service-api/src/main/java/greencity/constant/ErrorMessage.java (1)
152-153: Consider enhancing the DTO class error message for better clarity.The
INVALID_SORTING_VALUEmessage is clear and helpful. However, theINVALID_DTO_CLASSmessage could be more descriptive to help users understand what makes a DTO class invalid.Consider updating the message to be more specific:
- public static final String INVALID_DTO_CLASS = "Invalid DTO class"; + public static final String INVALID_DTO_CLASS = "Invalid DTO class: class must implement Sortable interface";service-api/src/main/java/greencity/enums/SortableFields.java (1)
8-8: Add class-level documentation to explain the enum's purpose and usage.Consider adding Javadoc to describe:
- The purpose of this enum in the sorting validation context
- How it integrates with the
Sortableinterface- Examples of typical usage in DTOs
@RequiredArgsConstructor @Getter +/** + * Defines the set of valid sortable fields used across the application. + * <p> + * This enum is used in conjunction with the {@code Sortable} interface to validate + * sort parameters at the handler layer, preventing invalid sort fields from causing + * SQL errors. + * + * @see greencity.dto.Sortable + */ public enum SortableFields {core/src/main/java/greencity/controller/FriendController.java (2)
Line range hint
229-243: Consider adding validation for filter parameters.The
findAllUsersExceptMainUserAndUsersFriendAndRequestersToMainUsermethod accepts multiple filter parameters. Since we're improving parameter validation, consider adding validation forfilterByFriendsOfFriendsandfilterByCityparameters as well.Example validation approach:
@Validated public ResponseEntity<PageableDto<UserFriendDto>> findAllUsersExceptMainUserAndUsersFriendAndRequestersToMainUser( @Parameter(hidden = true) @PageableDefault Pageable page, @Parameter(hidden = true) @CurrentUser UserVO userVO, @RequestParam(required = false, defaultValue = "") @Size(max = 255) String name, @RequestParam(required = false, defaultValue = "false") boolean filterByFriendsOfFriends, @RequestParam(required = false, defaultValue = "false") boolean filterByCity) {
Line range hint
264-271: Consider validating RecommendedFriendsType parameter.The
findRecommendedFriendsmethod accepts an optionalRecommendedFriendsType. Since this is an enum, consider adding validation to ensure only valid enum values are accepted.Example validation approach:
public ResponseEntity<PageableDto<UserFriendDto>> findRecommendedFriends( @Parameter(hidden = true) Pageable page, @RequestParam(required = false) @Valid RecommendedFriendsType type, @Parameter(hidden = true) @CurrentUser UserVO userVO) {core/src/main/java/greencity/webcontroller/ManagementRatingStatisticsController.java (1)
Line range hint
51-53: Consider using a validated sort factory.Instead of creating the Sort instance directly with
Sort.by("createDate"), consider introducing a factory method that validates the sort fields against theSortableinterface. This would ensure consistent validation across all sorting operations.Example approach:
Sort validatedSort = SortFactory.createValidatedSort("createDate", RatingStatisticsDtoForTables.class); Pageable paging = PageRequest.of(pageable.getPageNumber(), pageable.getPageSize(), validatedSort);core/src/main/java/greencity/handler/CustomPageableHandlerMethodArgumentResolver.java (1)
34-37: Consider addressing possible exceptions gracefully.
Currently, ifsortPageableValidator.validateSortParametersthrows an exception for invalid sort fields, the resolver will propagate an exception. It may be beneficial to add context to the exception message or log it before rethrowing, improving traceability when debugging.service-api/src/main/java/greencity/dto/Sortable.java (1)
5-7: Well-defined contract for sortable fields.
Providing a dedicated interface with agetSortableFields()method helps unify and standardize the notion of sortability across different DTOs. Consider adding brief Javadoc comments explaining its intended usage for improved clarity.service-api/src/main/java/greencity/dto/placecomment/PlaceCommentAdminDto.java (1)
16-28: Implementation looks good! Consider adding documentation.The implementation of
Sortableinterface is clean and appropriate. The choice of sortable fields (ID and CREATED_DATE) aligns well with the DTO's purpose.Consider adding a brief Javadoc comment to
getSortableFields()to document the available sort fields:+ /** + * @return List of sortable fields: id, createdDate + */ @Override public List<String> getSortableFields() {core/src/main/java/greencity/config/PageableConfig.java (2)
13-14: Consider using dependency injection for validators and resolvers.Instead of direct instantiation, consider using dependency injection for better testability and configuration flexibility.
- private final SortPageableValidator sortPageableValidator = new SortPageableValidator(); - private final PageableHandlerMethodArgumentResolver resolver = new PageableHandlerMethodArgumentResolver(); + private final SortPageableValidator sortPageableValidator; + private final PageableHandlerMethodArgumentResolver resolver; + + public PageableConfig(SortPageableValidator sortPageableValidator, + PageableHandlerMethodArgumentResolver resolver) { + this.sortPageableValidator = sortPageableValidator; + this.resolver = resolver; + }
22-25: Consider externalizing the max page size configuration.The max page size is currently hardcoded. Consider moving it to a configuration property for better flexibility across different environments.
+ @Value("${pagination.max-page-size:100}") + private int maxPageSize; CustomPageableHandlerMethodArgumentResolver customResolver = new CustomPageableHandlerMethodArgumentResolver(sortPageableValidator, resolver); - customResolver.setMaxPageSize(100); + customResolver.setMaxPageSize(maxPageSize);service-api/src/main/java/greencity/dto/place/AdminPlaceDto.java (1)
25-25: Implementation of Sortable interface looks good!The sortable fields (ID, NAME, MODIFIED_DATE, IS_FAVORITE) align well with the DTO properties, and using the SortableFields enum ensures type safety.
Consider adding a brief Javadoc comment above the
getSortableFields()method to document the available sort fields for API consumers.Also applies to: 37-44
core/src/main/java/greencity/annotations/ApiPageable.java (1)
25-25: Add documentation for dtoClass parameterThe new
dtoClass()method enhances type safety by requiring a Sortable implementation. However, its purpose and usage should be documented.Add a descriptive comment above the method:
/** * Specifies the DTO class that defines the sortable fields for this pageable endpoint. * The class must implement the Sortable interface. * @return the class of the DTO that defines valid sort fields */ Class<? extends Sortable> dtoClass();service-api/src/main/java/greencity/dto/comment/CommentDto.java (1)
20-20: Comprehensive implementation of sortable fields!The implementation includes a well-thought-out set of sortable fields covering timestamps (CREATED_DATE, MODIFIED_DATE) and engagement metrics (REPLIES, LIKES, DISLIKES).
Consider adding database indexes for frequently sorted fields to optimize query performance, especially for fields like CREATED_DATE and LIKES that are commonly used for sorting comments.
Also applies to: 55-64
service-api/src/main/java/greencity/dto/econews/EcoNewsGenericDto.java (1)
24-24: Add documentation for Sortable interface implementation.Consider adding a class-level Javadoc comment explaining the sorting capabilities and available sort fields for better API documentation.
service-api/src/main/java/greencity/dto/habit/HabitDto.java (1)
46-55: Consider validating sort direction for numeric fields.The implementation looks good, but consider adding validation for sort direction on fields like
LIKES,DISLIKES, andCOMPLEXITY. Some fields might only make sense when sorted in a specific direction.Example implementation:
public boolean isValidSortDirection(String fieldName, Sort.Direction direction) { // Example validation if (fieldName.equals(SortableFields.COMPLEXITY.getFieldName())) { return direction == Sort.Direction.ASC; // Only allow ascending sort for complexity } return true; // Allow both directions for other fields }core/src/test/java/greencity/validator/SortPageableValidatorTest.java (1)
20-28: Add more test cases for valid sort scenarios.Consider adding tests for:
- Multiple sort fields
- Different sort directions (ASC/DESC)
- Case sensitivity handling
Example test case:
@Test void validateSortParametersWithMultipleSortFields() { Sort multiSort = Sort.by( Sort.Order.asc("id"), Sort.Order.desc("rating") ); Pageable pageable = PageRequest.of(0, 2, multiSort); sortPageableValidator.validateSortParameters(pageable, UserFriendDto.class); }core/src/main/java/greencity/validator/SortPageableValidator.java (3)
24-38: Consider optimizing static initialization of valid fields mapThe current implementation creates new instances of DTOs in the static block, which may be inefficient. Consider using static field lists or a builder pattern for initialization.
Example approach:
- private static final Map<Class<? extends Sortable>, List<String>> VALID_FIELDS_MAP = new HashMap<>(); - static { - VALID_FIELDS_MAP.put(AdminPlaceDto.class, new AdminPlaceDto().getSortableFields()); - VALID_FIELDS_MAP.put(CommentDto.class, new CommentDto().getSortableFields()); - // ... other entries - } + private static final Map<Class<? extends Sortable>, List<String>> VALID_FIELDS_MAP = initValidFieldsMap(); + + private static Map<Class<? extends Sortable>, List<String>> initValidFieldsMap() { + Map<Class<? extends Sortable>, List<String>> map = new HashMap<>(); + map.put(AdminPlaceDto.class, List.of("id", "name", "status")); // Define static fields + map.put(CommentDto.class, List.of("id", "text", "createdDate")); + // ... other entries + return Collections.unmodifiableMap(map); + }
40-45: Enhance error message for invalid DTO classThe error message could be more descriptive to help developers identify which DTO class was invalid.
- throw new IllegalArgumentException(ErrorMessage.INVALID_DTO_CLASS); + throw new IllegalArgumentException(String.format(ErrorMessage.INVALID_DTO_CLASS + ": %s", dtoClass.getName()));
47-53: Enhance error reporting for invalid sort propertiesConsider providing more context in the error message to help API consumers understand which sort property was invalid and what the valid options are.
- throw new UnsupportedSortException(ErrorMessage.INVALID_SORTING_VALUE); + throw new UnsupportedSortException(String.format( + "Invalid sort property '%s'. Valid properties are: %s", + property, String.join(", ", validFields)));core/src/test/java/greencity/handler/CustomPageableHandlerMethodArgumentResolverTest.java (1)
49-69: Consider adding more test cases for edge scenariosThe current tests cover the basic scenarios well. Consider adding tests for:
- Multiple sort parameters
- Invalid sort fields
- Empty sort parameters
core/src/main/java/greencity/webcontroller/ManagementFactOfTheDayController.java (2)
53-53: Good enhancement to the API documentation!The addition of
dtoClass = FactOfTheDayDTO.classto@ApiPageableimproves API clarity and enables validation of sort fields. This change aligns well with the PR's objective to prevent SQL errors from invalid sort parameters.Consider adding Swagger documentation to specify which fields are sortable:
@Operation(summary = "Get management page with facts of the day.", description = "Supports sorting by: [list sortable fields here]")
69-69: Consistent implementation of @ApiPageable enhancement!The annotation update maintains consistency with the getAllFacts method and other controllers in the application.
Since this endpoint supports searching, consider adding validation for the search query parameter:
@Parameter(name = "query", description = "Search query for filtering facts", schema = @Schema(type = "string", example = "environment"))service/src/main/java/greencity/service/FriendServiceImpl.java (1)
127-127: Consider renaming the method for better clarity.While the removal of sort validation is appropriate, the method name contains a typo: "UserI" should be "UserId".
Consider renaming the method to improve readability:
- findUserFriendsByUserIAndShowFriendStatusRelatedToCurrentUser + findUserFriendsByUserIdAndShowFriendStatusRelatedToCurrentUser
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
core/src/main/java/greencity/annotations/ApiPageable.java(2 hunks)core/src/main/java/greencity/config/PageableConfig.java(2 hunks)core/src/main/java/greencity/controller/EcoNewsCommentController.java(1 hunks)core/src/main/java/greencity/controller/EcoNewsController.java(1 hunks)core/src/main/java/greencity/controller/EventCommentController.java(1 hunks)core/src/main/java/greencity/controller/FriendController.java(7 hunks)core/src/main/java/greencity/controller/HabitCommentController.java(2 hunks)core/src/main/java/greencity/controller/HabitController.java(4 hunks)core/src/main/java/greencity/controller/PlaceCommentController.java(2 hunks)core/src/main/java/greencity/controller/PlaceController.java(3 hunks)core/src/main/java/greencity/handler/CustomPageableHandlerMethodArgumentResolver.java(1 hunks)core/src/main/java/greencity/validator/SortPageableValidator.java(1 hunks)core/src/main/java/greencity/webcontroller/ManagementFactOfTheDayController.java(2 hunks)core/src/main/java/greencity/webcontroller/ManagementHabitController.java(1 hunks)core/src/main/java/greencity/webcontroller/ManagementRatingCalculationController.java(1 hunks)core/src/main/java/greencity/webcontroller/ManagementRatingStatisticsController.java(1 hunks)core/src/test/java/greencity/handler/CustomPageableHandlerMethodArgumentResolverTest.java(1 hunks)core/src/test/java/greencity/validator/SortPageableValidatorTest.java(1 hunks)service-api/src/main/java/greencity/constant/ErrorMessage.java(1 hunks)service-api/src/main/java/greencity/dto/Sortable.java(1 hunks)service-api/src/main/java/greencity/dto/comment/CommentDto.java(3 hunks)service-api/src/main/java/greencity/dto/econews/EcoNewsGenericDto.java(3 hunks)service-api/src/main/java/greencity/dto/factoftheday/FactOfTheDayDTO.java(2 hunks)service-api/src/main/java/greencity/dto/friends/UserFriendDto.java(3 hunks)service-api/src/main/java/greencity/dto/habit/HabitDto.java(3 hunks)service-api/src/main/java/greencity/dto/habit/HabitManagementDto.java(3 hunks)service-api/src/main/java/greencity/dto/place/AdminPlaceDto.java(3 hunks)service-api/src/main/java/greencity/dto/placecomment/PlaceCommentAdminDto.java(2 hunks)service-api/src/main/java/greencity/dto/ratingstatistics/RatingPointsDto.java(2 hunks)service-api/src/main/java/greencity/dto/ratingstatistics/RatingStatisticsDtoForTables.java(2 hunks)service-api/src/main/java/greencity/enums/SortableFields.java(1 hunks)service/src/main/java/greencity/service/FriendServiceImpl.java(4 hunks)service/src/main/java/greencity/service/PlaceCommentServiceImpl.java(1 hunks)service/src/test/java/greencity/service/FriendServiceImplTest.java(0 hunks)
💤 Files with no reviewable changes (1)
- service/src/test/java/greencity/service/FriendServiceImplTest.java
👮 Files not reviewed due to content moderation or server errors (4)
- core/src/main/java/greencity/controller/HabitCommentController.java
- core/src/main/java/greencity/controller/EventCommentController.java
- core/src/main/java/greencity/controller/EcoNewsCommentController.java
- core/src/main/java/greencity/controller/EcoNewsController.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (38)
core/src/main/java/greencity/controller/PlaceCommentController.java (2)
6-6: LGTM! Clean import addition.The import of
PlaceCommentAdminDtoaligns with the PR's objective of improving sort parameter validation.
101-101: Great enhancement to pagination control!The addition of
dtoClass = PlaceCommentAdminDto.classto@ApiPageableenables proper validation of sort fields at the handler layer, preventing SQL errors from invalid sort parameters. This aligns perfectly with the PR objectives.core/src/main/java/greencity/controller/HabitController.java (1)
100-100: LGTM! Consistent type information for sort validation.The
@ApiPageableannotations correctly specifyHabitDto.classas the DTO class, which aligns with the return types of these paginated endpoints. This change supports proper validation of sort fields.Also applies to: 123-123, 506-506
service-api/src/main/java/greencity/enums/SortableFields.java (2)
1-32: Well-structured enum implementation for sort field validation!Great job on implementing a type-safe approach to sort field validation:
- Using an enum ensures compile-time safety
- Lombok annotations reduce boilerplate code
- Clear mapping between enum constants and their field names
- Comprehensive coverage of sortable fields across different entities
14-16: Consider consolidating similar date field names.There are three similar date-related fields:
CREATE_DATE,CREATED_DATE, andCREATION_DATE. This might lead to confusion or inconsistency in usage. Consider:
- Consolidating these into a single consistent name if they represent the same concept
- Adding comments to clarify the difference if they serve different purposes
Let's check how these fields are used across the codebase:
core/src/main/java/greencity/controller/FriendController.java (1)
168-168: Consistent implementation of @ApiPageable annotation across all pageable endpoints.The addition of
dtoClass = UserFriendDto.classto all pageable endpoints improves API documentation and enables proper sort field validation. This is a good practice that ensures consistency across the controller.However, to complete the validation implementation:
- Ensure that
UserFriendDtoimplements theSortableinterface- Verify that all sortable fields are properly defined in
UserFriendDto.getSortableFields()Also applies to: 197-197, 229-229, 264-264, 292-292, 320-320, 354-354
✅ Verification successful
✅ Sortable implementation verified and complete
The
UserFriendDtocorrectly implements theSortableinterface with proper validation support for the following sort fields:
- id
- name
- rating
- mutualFriends
The
@ApiPageableannotation is properly integrated with the validation system.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify UserFriendDto implements Sortable and defines sortable fields # Check if UserFriendDto implements Sortable echo "Checking if UserFriendDto implements Sortable..." ast-grep --pattern 'class UserFriendDto implements $$$Sortable$$$' # Find getSortableFields implementation echo "Looking for getSortableFields implementation..." ast-grep --pattern 'class UserFriendDto { $$$ getSortableFields() { $$$ } $$$ }'Length of output: 375
Script:
#!/bin/bash # Find UserFriendDto file echo "Searching for UserFriendDto file..." fd UserFriendDto.java # If found, examine its content echo -e "\nChecking file content..." rg -l "class.*UserFriendDto" | xargs cat # Also search for any Sortable interface usage echo -e "\nSearching for Sortable interface usage..." rg "implements.*Sortable" -A 5Length of output: 14828
core/src/main/java/greencity/webcontroller/ManagementRatingStatisticsController.java (2)
46-46: LGTM! Good enhancement to the @ApiPageable annotation.The addition of
dtoClass = RatingStatisticsDtoForTables.classaligns well with the PR's objective to validate sort parameters at the handler layer.
Line range hint
51-53: Verify that 'createDate' is a valid sortable field.The hardcoded sort field
"createDate"should be validated against the sortable fields defined inRatingStatisticsDtoForTables.Let's verify this with the following script:
✅ Verification successful
createDateis a valid sortable fieldThe field is properly defined in RatingStatisticsDtoForTables and is consistently used throughout the codebase for sorting operations, including database queries and UI display. The current usage in the controller is correct and safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the Sortable interface implementation in RatingStatisticsDtoForTables # to confirm if 'createDate' is a valid sortable field ast-grep --pattern 'class RatingStatisticsDtoForTables implements Sortable { $$$ getSortableFields() { $$$ } $$$ }'Length of output: 129
Script:
#!/bin/bash # Find the DTO class file fd RatingStatisticsDtoForTables.java # Search for class definition and Sortable implementation rg -l "class.*RatingStatisticsDtoForTables" # Search for createDate field or property rg "createDate|create_date" -A 2 -B 2Length of output: 65936
service/src/main/java/greencity/service/PlaceCommentServiceImpl.java (1)
123-127: Clean implementation aligns with PR objectives.The removal of the try-catch block is a good decision as it aligns with moving the sort parameter validation to the handler layer. This prevents unnecessary database queries with invalid sort parameters.
Let's verify that the handler layer properly implements the sort parameter validation:
✅ Verification successful
✓ Validation confirmed: Sort parameters are properly validated
The removal of the try-catch block in
PlaceCommentServiceImplis justified as the validation is correctly implemented inCustomPageableHandlerMethodArgumentResolver. This handler validates sort parameters before they reach the service layer, effectively preventing invalid database queries.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the handler/controller layer implementation that validates sort parameters ast-grep --pattern 'class $_ implements $_ { $$$ @$_ $_ getAllComments($_, Pageable $_) { $$$ } $$$ }'Length of output: 118
Script:
#!/bin/bash # Find controller/handler files echo "=== Finding controller files ===" fd -e java "(Controller|Handler)" echo -e "\n=== Searching for getAllComments method ===" rg -l "getAllComments" -g "*.java"Length of output: 8612
Script:
#!/bin/bash echo "=== PlaceCommentController getAllComments implementation ===" rg -A 10 "getAllComments" core/src/main/java/greencity/controller/PlaceCommentController.java echo -e "\n=== Custom Pageable Handler implementation ===" cat core/src/main/java/greencity/handler/CustomPageableHandlerMethodArgumentResolver.javaLength of output: 2696
core/src/main/java/greencity/handler/CustomPageableHandlerMethodArgumentResolver.java (2)
16-17: Well-structured field injection and composition.
Usingprivate finalfields together with constructor injection for bothSortPageableValidatorandPageableHandlerMethodArgumentResolveris a solid approach that enhances testability and maintainability. Great job!
25-33: Clear override of the resolveArgument method.
OverridingresolveArgumentto delegate to the existingPageableHandlerMethodArgumentResolverkeeps existing pagination logic intact while layering in validation. This separation of concerns is commendable for ease of maintenance.service-api/src/main/java/greencity/dto/ratingstatistics/RatingPointsDto.java (1)
22-28: Consistent use of an enum for sortable fields.
Defining the sortable fields through an enum ensures type safety and consistency throughout the application. Initializing them viaList.of(...)is concise. Good approach!service-api/src/main/java/greencity/dto/factoftheday/FactOfTheDayDTO.java (1)
18-31: Clean implementation with appropriate sortable fields!The implementation provides a good selection of sortable fields (id, name, createDate) that would be most useful for fact-of-the-day listings.
service-api/src/main/java/greencity/dto/ratingstatistics/RatingStatisticsDtoForTables.java (1)
20-36: Well-structured implementation with comprehensive sort options.The implementation provides a good range of sortable fields. However, consider the performance implications of sorting by the
ratingfield in large datasets, as sorting by float values might impact query optimization.Could you confirm if there's an index on the
ratingcolumn in the database to support efficient sorting?service-api/src/main/java/greencity/dto/habit/HabitManagementDto.java (1)
22-22: Well-structured implementation of Sortable interface!The chosen sortable fields (ID, COMPLEXITY, DEFAULT_DURATION) are appropriate for habit management and align with the validated fields in the DTO.
Also applies to: 37-43
service-api/src/main/java/greencity/dto/comment/CommentDto.java (1)
Line range hint
1-64: Overall implementation successfully addresses the PR objectivesThe changes effectively prevent SQL errors from invalid sort fields by implementing a type-safe validation mechanism at the handler layer. The solution is:
- Type-safe: Using the Sortable interface and enums
- Maintainable: Each DTO clearly defines its sortable fields
- Efficient: Validates sort parameters before executing database queries
This is a robust solution that improves both system stability and user experience.
service-api/src/main/java/greencity/dto/econews/EcoNewsGenericDto.java (1)
62-71: LGTM! Well-structured implementation of getSortableFields().The implementation correctly uses the SortableFields enum, making it type-safe and maintainable.
service-api/src/main/java/greencity/dto/friends/UserFriendDto.java (1)
54-62: Review security implications of sorting by email.While the implementation is correct, sorting by email might expose sensitive information in certain contexts. Consider:
- Reviewing if email sorting is necessary for the use case
- Ensuring proper access controls are in place
- Logging sort operations on sensitive fields
✅ Verification successful
Email sorting implementation has appropriate security controls
The current implementation is secure as all friend-related endpoints are protected by authentication and proper role-based access control. Email sorting is only available to authenticated users with appropriate roles (USER, ADMIN, MODERATOR, UBS_EMPLOYEE), which follows security best practices for handling potentially sensitive information.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any existing access controls around email field rg -l "UserFriendDto.*email" | xargs rg "@PreAuthorize|@Secured|@RolesAllowed"Length of output: 80
Script:
#!/bin/bash # Find all files containing UserFriendDto echo "=== Files containing UserFriendDto ===" rg -l "UserFriendDto" echo -e "\n=== Controller endpoints using UserFriendDto ===" rg -B 2 -A 2 "UserFriendDto" --type java echo -e "\n=== Security configurations ===" fd -e java -e xml "Security|security"Length of output: 56292
Script:
#!/bin/bash # Check security annotations on FriendController and its endpoints echo "=== Security annotations on FriendController ===" rg -B 2 -A 2 "@(PreAuthorize|Secured|RolesAllowed)" core/src/main/java/greencity/controller/FriendController.java echo -e "\n=== Security configuration for friend endpoints ===" rg -B 2 -A 2 "friend|/user" core/src/main/java/greencity/config/SecurityConfig.javaLength of output: 7042
core/src/test/java/greencity/validator/SortPageableValidatorTest.java (1)
56-61: LGTM! Good use of mock DTO for negative testing.The UnknownDto implementation effectively tests the validation of invalid DTO classes.
core/src/test/java/greencity/handler/CustomPageableHandlerMethodArgumentResolverTest.java (1)
23-48: Well-structured test setup!The test class follows best practices with proper mock initialization and clear separation of dependencies.
core/src/main/java/greencity/webcontroller/ManagementRatingCalculationController.java (1)
41-41: LGTM - Appropriate DTO class specificationThe annotation correctly specifies RatingPointsDto.class for sort validation.
core/src/main/java/greencity/webcontroller/ManagementHabitController.java (1)
59-59: LGTM - Appropriate DTO class specificationThe annotation correctly specifies HabitManagementDto.class for sort validation.
core/src/main/java/greencity/controller/EventCommentController.java (3)
213-213: Consistent implementation of pagination annotationThe addition of
dtoClass = CommentDto.classmaintains consistency with other controllers and strengthens sort parameter validation.
213-213: LGTM! Consistent implementation across controllers.The addition of
dtoClass = CommentDto.classmaintains consistency with other comment controllers and enhances sort parameter validation.
213-213: LGTM! Consistent implementation of sorting validation.The addition of
dtoClass = CommentDto.classmaintains consistency with other controllers and ensures proper validation of sort parameters.core/src/main/java/greencity/controller/EcoNewsCommentController.java (3)
125-125: Standardized pagination annotationThe addition of
dtoClass = CommentDto.classaligns with the project-wide standardization of sort parameter validation.
125-125: LGTM! Maintains consistency in comment controllers.The addition of
dtoClass = CommentDto.classaligns with the implementation in other comment controllers, ensuring consistent sort parameter validation across the application.
125-125: LGTM! Consistent implementation across comment controllers.The addition of
dtoClass = CommentDto.classmaintains consistency with other comment controllers and reinforces the type-safe sorting validation.core/src/main/java/greencity/controller/EcoNewsController.java (5)
Line range hint
195-203: Well-structured parameter handlingThe method demonstrates good practices:
- Optional parameters are properly annotated with
required = false- Clear parameter descriptions in annotations
- Proper null checking for authentication state
194-194: Enhanced pagination with proper DTO specificationThe addition of
dtoClass = EcoNewsGenericDto.classensures proper sort parameter validation for eco news listings.Let's verify that the
EcoNewsGenericDtoclass implements theSortableinterface and defines appropriate sortable fields:✅ Verification successful
✓ Sortable fields properly configured for EcoNewsGenericDto
The DTO correctly implements the
Sortableinterface and defines appropriate sortable fields including id, title, creationDate, likes, countComments, and countOfEcoNews. The implementation ensures proper sort parameter validation for eco news pagination.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for EcoNewsGenericDto class definition and its sortable fields ast-grep --pattern 'class EcoNewsGenericDto implements Sortable { $$$ getSortableFields() { $$$ } $$$ }'Length of output: 118
Script:
#!/bin/bash # Find the DTO file fd -e java "EcoNewsGenericDto" # If found, examine its contents fd -e java "EcoNewsGenericDto" -x cat {}Length of output: 1841
194-194: LGTM! Enhanced sort validation for eco news pagination.The addition of
dtoClass = EcoNewsGenericDto.classproperly specifies the DTO class for sort validation in the eco news context.Let's verify that EcoNewsGenericDto implements the Sortable interface and handles sorting correctly with the optional parameters:
✅ Verification successful
✅ Sorting implementation verified and correctly implemented
The
EcoNewsGenericDtoproperly implements theSortableinterface and includes the necessary validation annotations. The pagination and sorting functionality is consistently implemented across the service layer.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for EcoNewsGenericDto implementation ast-grep --pattern 'class EcoNewsGenericDto implements Sortable { $$$ }' # Search for sort parameter handling in the service layer rg -A 5 "find\(.*Pageable.*tags.*title.*authorId.*favorite.*userEmail"Length of output: 149
Script:
#!/bin/bash # Find EcoNewsGenericDto class definition rg -A 5 "class EcoNewsGenericDto" # Find sorting-related code in eco news context rg -A 5 "class EcoNewsService|interface EcoNewsService" # Look for pageable usage in eco news related files fd -e java -x rg -l "Pageable.*EcoNews" {}Length of output: 3427
Line range hint
195-203: LGTM! Robust handling of optional authentication.The method properly handles cases where the user might not be authenticated by safely extracting the email from the principal:
String userEmail = principal != null ? principal.getName() : null;
194-194: LGTM! Enhanced type safety for eco news sorting.The addition of
dtoClass = EcoNewsGenericDto.classproperly specifies the sortable fields for eco news pagination.Let's verify that EcoNewsGenericDto implements the Sortable interface and properly handles sorting in conjunction with other filter parameters:
✅ Verification successful
✅ Verified: EcoNewsGenericDto properly implements sorting capabilities
The implementation is correct:
- EcoNewsGenericDto implements the Sortable interface
- Sorting fields are properly registered in SortPageableValidator
- Service layer correctly handles pagination and sorting in conjunction with other filter parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for EcoNewsGenericDto implementation ast-grep --pattern 'class EcoNewsGenericDto implements Sortable { $$$ }' # Search for the service implementation to verify filter parameter handling ast-grep --pattern 'find($_, List<String> tags, String title, Long authorId, boolean favorite, String userEmail) { $$$ }'Length of output: 202
Script:
#!/bin/bash # Search for EcoNewsGenericDto class definition with any pattern ast-grep --pattern 'class EcoNewsGenericDto { $$$ }' # Backup search using ripgrep rg "EcoNewsGenericDto" -A 5 # Search for any class implementing Sortable ast-grep --pattern 'implements Sortable { $$$ }' # Search for the service method with a more flexible pattern ast-grep --pattern 'find($$$tags$$$title$$$authorId$$$favorite$$$) { $$$ }'Length of output: 27116
core/src/main/java/greencity/controller/PlaceController.java (1)
249-249: LGTM! The changes enhance sort parameter validation.The addition of
dtoClass = AdminPlaceDto.classto@ApiPageableannotations is a good improvement that enables validation of sort parameters at the handler layer, preventing SQL errors from invalid sort fields.Let's verify that
AdminPlaceDtoimplements theSortableinterface and defines valid sort fields:Also applies to: 339-339, 516-516
✅ Verification successful
✅ Verified: AdminPlaceDto correctly implements sorting validation
The
AdminPlaceDtoproperly implements theSortableinterface and defines valid sort fields (id,name,modifiedDate,isFavorite). The implementation uses an enum-based approach for field names, ensuring type safety and preventing invalid sort parameters.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that AdminPlaceDto implements Sortable and defines sort fields ast-grep --pattern 'class AdminPlaceDto implements Sortable { $$$ }'Length of output: 72
Script:
#!/bin/bash # Find AdminPlaceDto file fd -e java "AdminPlaceDto.java" # Search for AdminPlaceDto class definition rg "class AdminPlaceDto" -A 5 # Search for Sortable interface usage rg "interface Sortable|implements Sortable"Length of output: 2169
Script:
#!/bin/bash # Check Sortable interface definition cat service-api/src/main/java/greencity/dto/Sortable.java # Check complete AdminPlaceDto class cat service-api/src/main/java/greencity/dto/place/AdminPlaceDto.javaLength of output: 1545
core/src/main/java/greencity/webcontroller/ManagementFactOfTheDayController.java (1)
Line range hint
53-79: Verify consistent implementation across controllers.Let's ensure the sort validation is properly implemented for these pageable endpoints.
✅ Verification successful
Sort validation implementation is properly structured and consistent ✅
The pageable endpoints in ManagementFactOfTheDayController follow the established pattern with proper sort validation:
- FactOfTheDayDTO implements the Sortable interface
- SortPageableValidator provides centralized validation
- Comprehensive test coverage ensures reliability
- Consistent with the rest of the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent @ApiPageable usage and sort validation implementation # Find all controllers using @ApiPageable echo "Checking @ApiPageable usage across controllers..." rg -l "ApiPageable" --type java # Look for sort validation implementation echo "\nChecking sort validation implementation..." rg -A 5 "dtoClass\s*=.*class" --type java # Find potential missing validations echo "\nChecking for potential missing validations..." rg "Pageable\s+\w+\s*[,\)]" --type javaLength of output: 59333
Script:
#!/bin/bash # Check the implementation of sort validation echo "Checking SortPageableValidator implementation..." ast-grep --pattern 'class SortPageableValidator { $$$ validateSortParameters($$$) { $$$ } $$$ }' echo "\nChecking CustomPageableHandlerMethodArgumentResolver..." ast-grep --pattern 'class CustomPageableHandlerMethodArgumentResolver { $$$ resolveArgument($$$) { $$$ } $$$ }' echo "\nChecking if FactOfTheDayDTO implements Sortable..." rg "class FactOfTheDayDTO" -A 5 --type javaLength of output: 1317
Script:
#!/bin/bash # Check the validation logic implementation echo "Checking SortPageableValidator implementation details..." rg -A 20 "validateSortParameters.*\{" --type java echo "\nChecking CustomPageableHandlerMethodArgumentResolver integration..." rg -A 20 "resolveArgument.*\{" --type java core/src/main/java/greencity/handler/CustomPageableHandlerMethodArgumentResolver.java echo "\nChecking Sortable interface definition..." rg -A 10 "interface Sortable" --type javaLength of output: 6837
service/src/main/java/greencity/service/FriendServiceImpl.java (3)
154-156: LGTM! Code formatting improves readability.The removal of sort validation and the added line spacing make the code more readable while maintaining the intended functionality.
244-245: LGTM! Consistent with architectural changes.The changes maintain a consistent approach to sort validation removal and follow the established code formatting style.
109-109: LGTM! Removal of sort validation aligns with architectural changes.The removal of sort validation at the service layer is appropriate as this responsibility is being moved to the handler layer, improving separation of concerns.
Let's verify that the sort validation is properly implemented in the handler layer:
✅ Verification successful
✅ Verified: Sort validation is properly implemented in the handler layer
The removal of sort validation from the service layer is justified as the system implements a robust validation mechanism through:
- CustomPageableHandlerMethodArgumentResolver for pagination handling
- SortPageableValidator for field-specific validation
- DTO-based sort field validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the corresponding handler/controller class and verify sort validation rg -l "findUserFriendsByUserId.*Pageable" --type javaLength of output: 181
Script:
#!/bin/bash # Search for friend-related controller/handler classes rg -l "Friend.*Controller|Friend.*Handler|Friend.*Resource" --type java # Also search for specific friend-related endpoint mappings rg "@(GetMapping|PostMapping|RequestMapping).*friend" -i --type javaLength of output: 1723
Script:
#!/bin/bash # Examine the implementation of all-user-friends endpoint in FriendController rg -A 10 -B 2 "@GetMapping.*all-user-friends" core/src/main/java/greencity/controller/FriendController.java # Look for any sort validation or pageable parameter handling ast-grep --pattern 'class FriendController { $$$ getAllUserFriends($$$) { $$$ } $$$ }'Length of output: 890
Script:
#!/bin/bash # Look for ApiPageable annotation implementation rg -l "ApiPageable" --type java # Also check for any PageableHandler or similar validation classes rg -l "Pageable.*Handler|Pageable.*Validator" --type javaLength of output: 5137
Script:
#!/bin/bash # Check the implementation of the pageable validator rg -A 10 "class SortPageableValidator" core/src/main/java/greencity/validator/SortPageableValidator.java # Also check the handler implementation rg -A 10 "class CustomPageableHandlerMethodArgumentResolver" core/src/main/java/greencity/handler/CustomPageableHandlerMethodArgumentResolver.javaLength of output: 1648
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (24)
service-api/src/test/java/greencity/dto/habit/HabitDtoTest.java (1)
9-23: Well-structured test implementation with room for enhancement.The test effectively verifies the sortable fields implementation. Consider these improvements to make it more robust:
- Add verification that the list contains exactly these fields (no extras)
- Consider using assertAll() for grouped assertions
- Add Javadoc to document the test's purpose
Here's a suggested enhancement:
class HabitDtoTest { + /** + * Verifies that HabitDto correctly implements the Sortable interface + * by providing the expected set of sortable fields. + */ @Test void getSortableFields() { HabitDto habitDto = new HabitDto(); List<String> sortableFields = habitDto.getSortableFields(); - assertTrue(sortableFields.contains(SortableFields.ID.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.DEFAULT_DURATION.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.AMOUNT_ACQUIRED_USERS.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.COMPLEXITY.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.LIKES.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.DISLIKES.getFieldName())); + List<String> expectedFields = Arrays.asList( + SortableFields.ID.getFieldName(), + SortableFields.DEFAULT_DURATION.getFieldName(), + SortableFields.AMOUNT_ACQUIRED_USERS.getFieldName(), + SortableFields.COMPLEXITY.getFieldName(), + SortableFields.LIKES.getFieldName(), + SortableFields.DISLIKES.getFieldName() + ); + + assertAll( + () -> assertEquals(expectedFields.size(), sortableFields.size(), "Should have exact number of sortable fields"), + () -> assertTrue(sortableFields.containsAll(expectedFields), "Should contain all expected fields"), + () -> assertTrue(expectedFields.containsAll(sortableFields), "Should not contain extra fields") + ); } }service-api/src/test/java/greencity/dto/place/AdminPlaceDtoTest.java (1)
11-19: Consider making the test more robust with set comparison.While the current implementation works, it could be made more maintainable by using a set comparison approach. This would make the test less brittle when fields are added or removed.
Here's a suggested improvement:
@Test void getSortableFieldsShouldReturnExpectedFieldsTest() { AdminPlaceDto adminPlaceDto = new AdminPlaceDto(); List<String> sortableFields = adminPlaceDto.getSortableFields(); - assertTrue(sortableFields.contains(SortableFields.ID.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.NAME.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.MODIFIED_DATE.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.IS_FAVORITE.getFieldName())); + Set<String> expectedFields = Set.of( + SortableFields.ID.getFieldName(), + SortableFields.NAME.getFieldName(), + SortableFields.MODIFIED_DATE.getFieldName(), + SortableFields.IS_FAVORITE.getFieldName() + ); + assertEquals(expectedFields, new HashSet<>(sortableFields), + "Sortable fields should match exactly"); + assertEquals(expectedFields.size(), sortableFields.size(), + "Should have exactly the expected number of fields"); }This approach:
- Makes the test more maintainable
- Ensures exact field matching
- Verifies the total number of fields
service-api/src/test/java/greencity/dto/ratingstatistics/RatingStatisticsDtoForTablesTest.java (2)
11-20: Consider enhancing test coverage and robustness.The test verifies the presence of required sortable fields, but could be more comprehensive. Consider these improvements:
- Verify the exact size of the returned list to ensure no unexpected fields are present
- Use a single assertion to compare the complete list content
- Add test cases for edge cases (e.g., null values)
Here's a suggested implementation:
@Test void getSortableFields() { RatingStatisticsDtoForTables dto = new RatingStatisticsDtoForTables(); List<String> sortableFields = dto.getSortableFields(); - assertTrue(sortableFields.contains(SortableFields.ID.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.CREATE_DATE.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.EVENT_NAME.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.RATING.getFieldName())); + List<String> expectedFields = Arrays.asList( + SortableFields.ID.getFieldName(), + SortableFields.CREATE_DATE.getFieldName(), + SortableFields.EVENT_NAME.getFieldName(), + SortableFields.RATING.getFieldName() + ); + assertEquals(expectedFields.size(), sortableFields.size(), + "Unexpected number of sortable fields"); + assertEquals(new HashSet<>(expectedFields), new HashSet<>(sortableFields), + "Sortable fields do not match expected fields"); } + @Test + void getSortableFieldsWithNullDto() { + RatingStatisticsDtoForTables dto = null; + assertThrows(NullPointerException.class, () -> dto.getSortableFields()); + }
11-12: Add test method documentation.Consider adding Javadoc to document the test method's purpose and expectations.
+ /** + * Verifies that getSortableFields returns the correct list of sortable fields + * for RatingStatisticsDtoForTables. + * Expected fields: ID, CREATE_DATE, EVENT_NAME, and RATING. + */ @Test void getSortableFields() {service-api/src/test/java/greencity/dto/friends/UserFriendDtoTest.java (1)
62-72: Consider enhancing test coverage with additional assertions.The test effectively verifies the presence of sortable fields. Consider these enhancements to make it more robust:
- Add size verification to ensure no unexpected fields are present
- Consider testing the exact order if it's significant
- Add a negative test case for fields that shouldn't be sortable
Here's an example implementation:
@Test void getSortableFieldsShouldReturnExpectedFieldsTest() { UserFriendDto dto = new UserFriendDto(); List<String> sortableFields = dto.getSortableFields(); + + // Verify exact size + assertEquals(5, sortableFields.size(), "Should have exactly 5 sortable fields"); assertTrue(sortableFields.contains(SortableFields.ID.getFieldName())); assertTrue(sortableFields.contains(SortableFields.NAME.getFieldName())); assertTrue(sortableFields.contains(SortableFields.EMAIL.getFieldName())); assertTrue(sortableFields.contains(SortableFields.RATING.getFieldName())); assertTrue(sortableFields.contains(SortableFields.MUTUAL_FRIENDS.getFieldName())); + + // Verify non-sortable field + assertFalse(sortableFields.contains("profilePicturePath"), + "Profile picture should not be sortable"); }service-api/src/test/java/greencity/dto/habit/HabitManagementDtoTest.java (2)
11-20: Well-structured test following AAA pattern!The test is clear and follows good testing practices. Here are some suggestions to make it even more robust:
@Test void getSortableFields() { HabitManagementDto dto = new HabitManagementDto(); List<String> sortableFields = dto.getSortableFields(); + + // Verify exact size to ensure no unexpected fields + assertEquals(3, sortableFields.size(), "Should have exactly 3 sortable fields"); - assertTrue(sortableFields.contains(SortableFields.ID.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.COMPLEXITY.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.DEFAULT_DURATION.getFieldName())); + assertTrue(sortableFields.contains(SortableFields.ID.getFieldName()), + "ID should be a sortable field"); + assertTrue(sortableFields.contains(SortableFields.COMPLEXITY.getFieldName()), + "COMPLEXITY should be a sortable field"); + assertTrue(sortableFields.contains(SortableFields.DEFAULT_DURATION.getFieldName()), + "DEFAULT_DURATION should be a sortable field"); }
9-21: Consider adding edge cases to strengthen validation testing.To align with the PR's goal of improving sort parameter validation, consider adding these test cases:
@Test void getSortableFieldsShouldNotContainInvalidFields() { HabitManagementDto dto = new HabitManagementDto(); List<String> sortableFields = dto.getSortableFields(); assertFalse(sortableFields.contains("invalid_field"), "Should not contain invalid fields"); assertFalse(sortableFields.contains(""), "Should not contain empty field names"); } @Test void getSortableFieldsShouldReturnImmutableList() { HabitManagementDto dto = new HabitManagementDto(); List<String> sortableFields = dto.getSortableFields(); assertThrows(UnsupportedOperationException.class, () -> sortableFields.add("new_field"), "Returned list should be immutable"); }service-api/src/test/java/greencity/dto/factoftheday/FactOfTheDayDTOTest.java (3)
11-19: Enhance test coverage and assertions for better reliability.The test provides basic validation but could be more comprehensive. Consider these improvements:
- Verify the exact size of the sortable fields list
- Test that invalid fields are not included
- Add descriptive messages to assertions
- Consider using assertAll for better failure reporting
Here's a suggested enhancement:
@Test void getSortableFields() { FactOfTheDayDTO factOfTheDayDTO = new FactOfTheDayDTO(); List<String> sortableFields = factOfTheDayDTO.getSortableFields(); - assertTrue(sortableFields.contains(SortableFields.ID.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.NAME.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.CREATE_DATE.getFieldName())); + assertAll("Sortable fields validation", + () -> assertEquals(3, sortableFields.size(), "Should have exactly 3 sortable fields"), + () -> assertTrue(sortableFields.contains(SortableFields.ID.getFieldName()), + "Should contain ID field"), + () -> assertTrue(sortableFields.contains(SortableFields.NAME.getFieldName()), + "Should contain NAME field"), + () -> assertTrue(sortableFields.contains(SortableFields.CREATE_DATE.getFieldName()), + "Should contain CREATE_DATE field"), + () -> assertFalse(sortableFields.contains("invalid_field"), + "Should not contain invalid fields") + ); }
13-13: Consider using a test fixture or builder pattern.Direct instantiation of
FactOfTheDayDTOmight not be maintainable if the DTO requires specific setup in the future.Consider using
@BeforeEachto set up test fixtures:+private FactOfTheDayDTO factOfTheDayDTO; + +@BeforeEach +void setUp() { + factOfTheDayDTO = new FactOfTheDayDTO(); +} + @Test void getSortableFields() { - FactOfTheDayDTO factOfTheDayDTO = new FactOfTheDayDTO(); List<String> sortableFields = factOfTheDayDTO.getSortableFields(); // ... rest of the test }
11-11: Add test cases for edge cases.Consider adding tests for potential edge cases to ensure robust validation.
Add these test methods:
@Test void getSortableFields_ShouldNeverReturnNull() { FactOfTheDayDTO dto = new FactOfTheDayDTO(); assertNotNull(dto.getSortableFields(), "Sortable fields should never be null"); } @Test void getSortableFields_ShouldReturnUnmodifiableList() { FactOfTheDayDTO dto = new FactOfTheDayDTO(); List<String> sortableFields = dto.getSortableFields(); assertThrows(UnsupportedOperationException.class, () -> sortableFields.add("new_field"), "Sortable fields list should be unmodifiable"); }service-api/src/test/java/greencity/dto/placecomment/PlaceCommentAdminDtoTest.java (2)
9-19: Good foundation, but test coverage could be enhanced.The test provides basic validation of sortable fields. Consider these enhancements to make it more robust:
- Add comprehensive assertions:
@Test void getSortableFields() { PlaceCommentAdminDto placeCommentAdminDto = new PlaceCommentAdminDto(); List<String> sortableFields = placeCommentAdminDto.getSortableFields(); // Verify exact list size assertEquals(2, sortableFields.size()); // Use assertThat for more readable assertions assertThat(sortableFields) .contains( SortableFields.ID.getFieldName(), SortableFields.CREATED_DATE.getFieldName() ); }
- Add negative test cases:
@Test void getSortableFields_shouldNotContainUnsortableFields() { PlaceCommentAdminDto placeCommentAdminDto = new PlaceCommentAdminDto(); List<String> sortableFields = placeCommentAdminDto.getSortableFields(); assertThat(sortableFields) .doesNotContain("unsortableField"); }
1-19: Consider enhancing test coverage for the sorting validation feature.Given that this PR focuses on improving sort parameter validation, consider adding more test cases to cover:
- Integration with
SortPageableValidator- Behavior with invalid sort fields
- Order-sensitive sorting scenarios (if applicable)
This will help ensure the robustness of the sorting validation mechanism.
service-api/src/test/java/greencity/dto/comment/CommentDtoTest.java (2)
11-22: Consider using collection comparison for more maintainable tests.The current approach of individual assertions for each field makes the test verbose and harder to maintain. Consider using collection comparison methods for a more concise and maintainable test.
Here's a suggested refactor using
assertIterableEquals:@Test void getSortableFieldsTest() { CommentDto commentDto = new CommentDto(); - List<String> sortableFields = commentDto.getSortableFields(); - - assertTrue(sortableFields.contains(SortableFields.ID.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.CREATED_DATE.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.CREATED_DATE.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.MODIFIED_DATE.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.REPLIES.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.LIKES.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.DISLIKES.getFieldName())); + List<String> expectedFields = Arrays.asList( + SortableFields.ID.getFieldName(), + SortableFields.CREATED_DATE.getFieldName(), + SortableFields.MODIFIED_DATE.getFieldName(), + SortableFields.REPLIES.getFieldName(), + SortableFields.LIKES.getFieldName(), + SortableFields.DISLIKES.getFieldName() + ); + + assertIterableEquals(expectedFields, commentDto.getSortableFields()); }This approach:
- Makes the test more maintainable
- Ensures exact field count matches
- Provides clearer failure messages
- Reduces code duplication
11-22: Consider adding negative test cases.The current test only verifies the presence of valid fields. Consider adding test cases to verify that invalid fields are not included in the returned list.
Here's a suggested additional test method:
@Test void getSortableFieldsExcludesInvalidFieldsTest() { CommentDto commentDto = new CommentDto(); List<String> sortableFields = commentDto.getSortableFields(); // Verify invalid fields are not present assertFalse(sortableFields.contains("invalid_field")); assertFalse(sortableFields.contains("")); assertFalse(sortableFields.contains(null)); }service-api/src/test/java/greencity/dto/econews/EcoNewsGenericDtoTest.java (1)
12-25: Enhance test coverage with a more comprehensive approach.While the current test verifies the presence of expected fields, it could be more robust and maintainable.
Consider refactoring the test to:
- Verify the exact size of the returned list
- Use a set-based comparison for all expected fields
- Verify that unexpected fields are not present
Here's a suggested implementation:
@Test void getSortableFields() { EcoNewsGenericDto ecoNewsGenericDto = new EcoNewsGenericDto(); List<String> sortableFields = ecoNewsGenericDto.getSortableFields(); - assertTrue(sortableFields.contains(SortableFields.TITLE.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.ID.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.CREATION_DATE.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.ID.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.TITLE.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.CREATION_DATE.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.LIKES.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.COUNT_COMMENTS.getFieldName())); - assertTrue(sortableFields.contains(SortableFields.COUNT_OF_ECO_NEWS.getFieldName())); + Set<String> expectedFields = Set.of( + SortableFields.TITLE.getFieldName(), + SortableFields.ID.getFieldName(), + SortableFields.CREATION_DATE.getFieldName(), + SortableFields.LIKES.getFieldName(), + SortableFields.COUNT_COMMENTS.getFieldName(), + SortableFields.COUNT_OF_ECO_NEWS.getFieldName() + ); + + assertEquals(expectedFields.size(), sortableFields.size(), + "Number of sortable fields doesn't match expected count"); + assertTrue(new HashSet<>(sortableFields).containsAll(expectedFields), + "Not all expected fields are present"); + assertFalse(sortableFields.contains("unexpectedField"), + "Unexpected field found in sortable fields"); }Don't forget to add these imports:
import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import java.util.Set; import java.util.HashSet;core/src/main/java/greencity/handler/CustomPageableHandlerMethodArgumentResolver.java (2)
Line range hint
28-42: Excellent separation of concerns and null safety improvements!The changes effectively:
- Delegate sort handling to a specialized resolver
- Add null safety with
@Nonnullannotations- Maintain single responsibility principle
Consider adding a comment explaining why the sort resolution is delegated, to help future maintainers understand the design decision.
@Override @Nonnull +// Delegates sort resolution to CustomSortHandlerMethodArgumentResolver to validate sort parameters public Pageable resolveArgument(@Nonnull MethodParameter methodParameter,
Line range hint
44-60: Consider enhancing parameter validation for consistency.The method has good error handling but could be improved:
- Add
@Nonnullannotations for consistency with the public method- Validate input parameters to prevent null pointer exceptions
- private int parseParameter(NativeWebRequest webRequest, String param, int defaultValue) { + private int parseParameter(@Nonnull NativeWebRequest webRequest, @Nonnull String param, int defaultValue) { + if (webRequest == null || param == null) { + throw new IllegalArgumentException("WebRequest and param must not be null"); + } String paramValue = webRequest.getParameter(param);service-api/src/test/java/greencity/dto/ratingstatistics/RatingPointsDtoTest.java (1)
11-19: Consider enhancing test coverage with additional assertions and edge cases.While the current test verifies the presence of expected fields, consider these improvements:
- Verify the exact size of the returned list to ensure no unexpected fields
- Add a negative test case to verify a field that shouldn't be sortable
- Consider testing the order of returned fields if it's significant
Here's a suggested enhancement:
@Test void getSortableFields() { RatingPointsDto ratingPointsDto = new RatingPointsDto(); List<String> sortableFields = ratingPointsDto.getSortableFields(); + // Verify exact size + assertEquals(3, sortableFields.size()); + assertTrue(sortableFields.contains(SortableFields.ID.getFieldName())); assertTrue(sortableFields.contains(SortableFields.NAME.getFieldName())); assertTrue(sortableFields.contains(SortableFields.POINTS.getFieldName())); + + // Verify non-sortable field + assertFalse(sortableFields.contains("nonExistentField")); }core/src/main/java/greencity/validator/SortPageableValidator.java (3)
26-40: Consider making the fields list initialization more flexible.Currently, the map is populated by instantiating each DTO class to retrieve its
sortableFields. While this works, using static or precomputed fields might be more efficient and less error-prone. Instantiating the DTO each time can lead to higher memory use, especially if these DTO instances become more complex. A static approach or a registration mechanism would streamline future expansions.
45-47: Include more informative error reporting.When an invalid DTO class is encountered, the exception message might be enhanced to include the unexpected class name. For instance, you could provide details on which DTO class was invalid. This makes debugging easier.
- throw new IllegalArgumentException(ErrorMessage.INVALID_DTO_CLASS); + throw new IllegalArgumentException(ErrorMessage.INVALID_DTO_CLASS + " Provided class: " + dtoClass.getName());
51-52: Provide details about the invalid sort property in the exception.Throwing
UnsupportedSortExceptionwithout specifying the invalid property can slow down troubleshooting. Consider appending the actual property name to the error message for clarity.- throw new UnsupportedSortException(ErrorMessage.INVALID_SORTING_VALUE); + throw new UnsupportedSortException( + ErrorMessage.INVALID_SORTING_VALUE + " Invalid field name: " + property + );core/src/test/java/greencity/handler/CustomSortHandlerMethodArgumentResolverTest.java (1)
98-109: Consider testing invalid sort fields as well.While this test confirms you skip validation on unsorted requests, adding a dedicated negative test for invalid fields (possibly in
SortPageableValidatorTest) would improve confidence. However, this might already be covered in the validator tests. Just something to consider for completeness.core/src/main/java/greencity/config/PageableConfig.java (1)
37-45: Solid integration of custom resolvers with sensible limits.The configuration properly wires together the custom components while maintaining a reasonable page size limit of 100 to prevent resource exhaustion.
Consider externalizing the max page size (100) to a configuration property for easier adjustment across different environments.
core/src/test/java/greencity/handler/CustomPageableHandlerMethodArgumentResolverTest.java (1)
46-67: Good coverage of happy path scenarios.The tests properly verify both default and custom pagination scenarios with sort integration.
Consider adding a test case that verifies the behavior with a non-empty sort parameter to ensure the custom sort resolver is properly integrated.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
core/src/main/java/greencity/annotations/ApiPageable.java(2 hunks)core/src/main/java/greencity/config/PageableConfig.java(1 hunks)core/src/main/java/greencity/controller/HabitController.java(4 hunks)core/src/main/java/greencity/controller/PlaceController.java(3 hunks)core/src/main/java/greencity/handler/CustomPageableHandlerMethodArgumentResolver.java(3 hunks)core/src/main/java/greencity/handler/CustomSortHandlerMethodArgumentResolver.java(1 hunks)core/src/main/java/greencity/validator/SortPageableValidator.java(1 hunks)core/src/test/java/greencity/handler/CustomPageableHandlerMethodArgumentResolverTest.java(7 hunks)core/src/test/java/greencity/handler/CustomSortHandlerMethodArgumentResolverTest.java(1 hunks)core/src/test/java/greencity/validator/SortPageableValidatorTest.java(1 hunks)service-api/src/main/java/greencity/constant/ErrorMessage.java(1 hunks)service-api/src/main/java/greencity/dto/Sortable.java(1 hunks)service-api/src/main/java/greencity/enums/SortableFields.java(1 hunks)service-api/src/test/java/greencity/dto/comment/CommentDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/econews/EcoNewsGenericDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/factoftheday/FactOfTheDayDTOTest.java(1 hunks)service-api/src/test/java/greencity/dto/friends/UserFriendDtoTest.java(2 hunks)service-api/src/test/java/greencity/dto/habit/HabitDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/habit/HabitManagementDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/place/AdminPlaceDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/placecomment/PlaceCommentAdminDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/ratingstatistics/RatingPointsDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/ratingstatistics/RatingStatisticsDtoForTablesTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- service-api/src/main/java/greencity/dto/Sortable.java
- service-api/src/main/java/greencity/enums/SortableFields.java
- core/src/main/java/greencity/annotations/ApiPageable.java
- service-api/src/main/java/greencity/constant/ErrorMessage.java
- core/src/main/java/greencity/controller/HabitController.java
- core/src/main/java/greencity/controller/PlaceController.java
- core/src/test/java/greencity/validator/SortPageableValidatorTest.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (16)
service-api/src/test/java/greencity/dto/place/AdminPlaceDtoTest.java (1)
9-20: Well-structured test with clear naming convention!The test class is well-organized and follows good naming practices. The test method name clearly describes its purpose.
service-api/src/test/java/greencity/dto/friends/UserFriendDtoTest.java (1)
4-10: LGTM! Clean import additions.The new imports are well-organized and necessary for the added test functionality.
service-api/src/test/java/greencity/dto/placecomment/PlaceCommentAdminDtoTest.java (1)
1-8: Clean and well-organized imports!The package structure and imports follow Java best practices, with appropriate static imports for test assertions.
core/src/main/java/greencity/handler/CustomPageableHandlerMethodArgumentResolver.java (2)
1-1: Well-structured package organization!Moving the class to the
handlerpackage better reflects its responsibility as a handler component, improving code organization.
21-25: Good use of constructor injection!The implementation follows Spring best practices by:
- Using constructor injection for required dependencies
- Marking the field as final for immutability
service-api/src/test/java/greencity/dto/ratingstatistics/RatingPointsDtoTest.java (2)
1-8: LGTM! Clean and focused imports.The imports are well-organized and include only what's necessary for the test class.
9-10: Verify test class naming convention alignment.The test class follows standard naming conventions. Let's verify it aligns with other test classes in the project.
✅ Verification successful
Test class naming convention is correct
The class follows the project's established patterns, matching other DTO test classes in both naming convention and method structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check naming patterns of similar test classes # Look for other DTO test classes to ensure consistent naming echo "Checking DTO test class naming patterns..." find . -type f -name "*DtoTest.java" -exec basename {} \; | sort echo -e "\nChecking test method naming patterns in DTO tests..." rg "void \w+\(\)" --type java -g "*DtoTest.java" -A 1Length of output: 6624
core/src/main/java/greencity/handler/CustomSortHandlerMethodArgumentResolver.java (2)
20-24: Excellent constructor-based dependency injection.Using constructor-based injection provides clear dependencies and facilitates testing or mocking. Nice work here!
38-41: Validate necessity of method-level annotation vs. parameter-level annotation.This code retrieves the
ApiPageableannotation from the method, but some teams place paging annotations directly on parameters. Verify that you are always expecting this annotation at the method level in your codebase rather than on parameters.core/src/test/java/greencity/handler/CustomSortHandlerMethodArgumentResolverTest.java (2)
47-59: Good test coverage forsupportsParameter.You've covered distinct scenarios for
Sorttype, non-Sorttype, and null parameter types. This thorough approach ensures robust behavior.
81-96: Excellent validation check with mocks.The test confirms that
validator.validateSortParametersis invoked whenApiPageableexists andSortis sorted. This scenario-based approach effectively validates functional correctness.core/src/main/java/greencity/config/PageableConfig.java (3)
20-30: Well-structured sort handling configuration!Good design choice to compose the custom sort resolver with both the validator and Spring's standard resolver. This maintains Spring's sorting capabilities while adding custom validation.
32-35: Documentation accurately reflects the changes.The updated comment clearly indicates the addition of sort parameter validation functionality.
15-18: LGTM! Clean bean definition for sort validation.The introduction of
SortPageableValidatorbean aligns well with the PR objective to validate sort parameters at the handler layer.Let's verify the validator implementation:
✅ Verification successful
✅ Validator implementation is robust and well-designed
The
SortPageableValidatorimplementation demonstrates excellent practices:
- Type-safe validation through the
Sortableinterface- Clear validation logic with proper error handling
- Extensible design allowing easy addition of new DTOs
- Well-documented code with clear contracts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the validator implementation ast-grep --pattern 'class SortPageableValidator { $$$ validate($$$) { $$$ } $$$ }'Length of output: 96
Script:
#!/bin/bash # Find the validator file fd "SortPageableValidator.java" # If found, display its contents fd "SortPageableValidator.java" --exec cat {}Length of output: 2841
Script:
#!/bin/bash # Verify Sortable interface exists and its contract fd "Sortable.java" --exec cat {} # Check for any classes implementing Sortable that might be missing in the map ast-grep --pattern 'class $_ implements Sortable'Length of output: 400
core/src/test/java/greencity/handler/CustomPageableHandlerMethodArgumentResolverTest.java (2)
31-44: Well-structured test setup with proper mocking.The test class is properly organized with all necessary mocks and initialization.
Line range hint
69-124: Excellent coverage of error scenarios!The test suite thoroughly covers all potential error cases including:
- Negative page/size values
- Invalid format handling
- Size limit validation
This comprehensive error handling aligns perfectly with the PR's objective to prevent invalid parameters from causing server errors.
service-api/src/test/java/greencity/dto/comment/CommentDtoTest.java
Outdated
Show resolved
Hide resolved
service-api/src/test/java/greencity/dto/econews/EcoNewsGenericDtoTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
service-api/src/test/java/greencity/dto/econews/EcoNewsGenericDtoTest.java (2)
25-26: Consider removing redundant size assertion.The
assertEquals(expectedFields, new HashSet<>(sortableFields))already ensures that both sets have the same elements and size. The second assertion is redundant and can be removed to keep the test concise.assertEquals(expectedFields, new HashSet<>(sortableFields)); - assertEquals(expectedFields.size(), sortableFields.size());
17-23: Consider using a test data helper method.To improve maintainability and reusability, consider extracting the expected fields setup into a helper method. This would make it easier to reuse these fields in other test cases and maintain the list of sortable fields in one place.
+ private Set<String> getExpectedSortableFields() { + return Set.of( + SortableFields.ID.getFieldName(), + SortableFields.TITLE.getFieldName(), + SortableFields.CREATION_DATE.getFieldName(), + SortableFields.LIKES.getFieldName(), + SortableFields.COUNT_COMMENTS.getFieldName(), + SortableFields.COUNT_OF_ECO_NEWS.getFieldName()); + } + @Test void getSortableFields() { EcoNewsGenericDto ecoNewsGenericDto = new EcoNewsGenericDto(); List<String> sortableFields = ecoNewsGenericDto.getSortableFields(); - Set<String> expectedFields = Set.of( - SortableFields.ID.getFieldName(), - SortableFields.TITLE.getFieldName(), - SortableFields.CREATION_DATE.getFieldName(), - SortableFields.LIKES.getFieldName(), - SortableFields.COUNT_COMMENTS.getFieldName(), - SortableFields.COUNT_OF_ECO_NEWS.getFieldName()); + Set<String> expectedFields = getExpectedSortableFields();core/src/main/java/greencity/config/PageableConfig.java (1)
15-30: Well-structured bean definitions with clear dependencies!The bean definitions follow Spring best practices and establish a clear dependency chain. Consider adding Javadoc comments to document the purpose and relationships of these beans.
Add documentation like this:
+ /** + * Creates a validator for sorting parameters in pageable requests. + * + * @return A new instance of SortPageableValidator + */ @Bean public SortPageableValidator sortPageableValidator() { return new SortPageableValidator(); }service-api/src/test/java/greencity/dto/place/AdminPlaceDtoTest.java (3)
14-15: Consider adding test documentationWhile the test is straightforward, adding a brief Javadoc comment would improve maintainability by documenting the test's purpose and any specific test conditions.
+ /** + * Verifies that getSortableFields returns the expected set of sortable fields + * defined in the SortableFields enum for AdminPlaceDto. + */ @Test void getSortableFieldsShouldReturnExpectedFieldsTest() {
16-21: Consider using a constant for expected fieldsTo improve maintainability and reusability, consider extracting the expected fields set as a private static final field. This would make it easier to update if the sortable fields change and could be reused in other test methods.
+ private static final Set<String> EXPECTED_SORTABLE_FIELDS = Set.of( + SortableFields.ID.getFieldName(), + SortableFields.NAME.getFieldName(), + SortableFields.MODIFIED_DATE.getFieldName(), + SortableFields.IS_FAVORITE.getFieldName()); + @Test void getSortableFieldsShouldReturnExpectedFieldsTest() { AdminPlaceDto adminPlaceDto = new AdminPlaceDto(); List<String> sortableFields = adminPlaceDto.getSortableFields(); - Set<String> expectedFields = Set.of( - SortableFields.ID.getFieldName(), - SortableFields.NAME.getFieldName(), - SortableFields.MODIFIED_DATE.getFieldName(), - SortableFields.IS_FAVORITE.getFieldName());
22-23: Consider using more specific assertion messagesAdding descriptive messages to assertions would make test failures more informative and easier to debug.
- assertEquals(expectedFields, new HashSet<>(sortableFields)); - assertEquals(expectedFields.size(), sortableFields.size()); + assertEquals(expectedFields, new HashSet<>(sortableFields), + "Sortable fields should match the expected fields"); + assertEquals(expectedFields.size(), sortableFields.size(), + "Number of sortable fields should match expected count");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
core/src/main/java/greencity/config/PageableConfig.java(1 hunks)service-api/src/main/java/greencity/dto/comment/CommentDto.java(3 hunks)service-api/src/main/java/greencity/dto/econews/EcoNewsGenericDto.java(3 hunks)service-api/src/main/java/greencity/dto/factoftheday/FactOfTheDayDTO.java(2 hunks)service-api/src/main/java/greencity/dto/friends/UserFriendDto.java(3 hunks)service-api/src/main/java/greencity/dto/habit/HabitDto.java(3 hunks)service-api/src/main/java/greencity/dto/habit/HabitManagementDto.java(3 hunks)service-api/src/main/java/greencity/dto/place/AdminPlaceDto.java(3 hunks)service-api/src/main/java/greencity/dto/placecomment/PlaceCommentAdminDto.java(2 hunks)service-api/src/main/java/greencity/dto/ratingstatistics/RatingPointsDto.java(2 hunks)service-api/src/main/java/greencity/dto/ratingstatistics/RatingStatisticsDtoForTables.java(2 hunks)service-api/src/test/java/greencity/dto/comment/CommentDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/econews/EcoNewsGenericDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/factoftheday/FactOfTheDayDTOTest.java(1 hunks)service-api/src/test/java/greencity/dto/friends/UserFriendDtoTest.java(2 hunks)service-api/src/test/java/greencity/dto/habit/HabitDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/habit/HabitManagementDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/place/AdminPlaceDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/placecomment/PlaceCommentAdminDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/ratingstatistics/RatingPointsDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/ratingstatistics/RatingStatisticsDtoForTablesTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- service-api/src/test/java/greencity/dto/factoftheday/FactOfTheDayDTOTest.java
- service-api/src/test/java/greencity/dto/ratingstatistics/RatingStatisticsDtoForTablesTest.java
- service-api/src/test/java/greencity/dto/habit/HabitDtoTest.java
- service-api/src/main/java/greencity/dto/place/AdminPlaceDto.java
- service-api/src/main/java/greencity/dto/habit/HabitManagementDto.java
- service-api/src/main/java/greencity/dto/habit/HabitDto.java
- service-api/src/main/java/greencity/dto/comment/CommentDto.java
- service-api/src/main/java/greencity/dto/factoftheday/FactOfTheDayDTO.java
- service-api/src/main/java/greencity/dto/placecomment/PlaceCommentAdminDto.java
- service-api/src/main/java/greencity/dto/ratingstatistics/RatingStatisticsDtoForTables.java
- service-api/src/test/java/greencity/dto/placecomment/PlaceCommentAdminDtoTest.java
- service-api/src/main/java/greencity/dto/friends/UserFriendDto.java
- service-api/src/test/java/greencity/dto/friends/UserFriendDtoTest.java
- service-api/src/main/java/greencity/dto/ratingstatistics/RatingPointsDto.java
- service-api/src/test/java/greencity/dto/comment/CommentDtoTest.java
- service-api/src/test/java/greencity/dto/habit/HabitManagementDtoTest.java
- service-api/src/test/java/greencity/dto/ratingstatistics/RatingPointsDtoTest.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (7)
service-api/src/test/java/greencity/dto/econews/EcoNewsGenericDtoTest.java (1)
1-10: Well-organized imports!Clean and properly organized imports following best practices.
core/src/main/java/greencity/config/PageableConfig.java (2)
3-13: Clean and well-organized imports!The imports are properly structured and include all necessary components for the custom pagination and sorting functionality.
1-44: Implementation aligns well with PR objectives!The changes successfully move sort parameter validation to the handler layer, which will prevent SQL errors and improve error handling. This matches the PR's goal of validating sort parameters before query execution.
Let's verify the integration with the validation components:
✅ Verification successful
Validation chain is properly integrated
The implementation shows a complete validation setup with:
- DTOs implementing the Sortable interface to define valid sort fields
- Controllers using @ApiPageable with appropriate dtoClass parameters
- PageableConfig correctly wiring the validation components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of validation components # Check for the validator implementation echo "Checking for validator implementation..." rg -l "implements.*Sortable" . # Check for usage of the validator in controllers echo "Checking for @ApiPageable usage with dtoClass..." rg "@ApiPageable.*dtoClass"Length of output: 3876
service-api/src/test/java/greencity/dto/place/AdminPlaceDtoTest.java (2)
13-13: Test method name follows best practices!The method name clearly describes the test's purpose and expected outcome, following the convention of "methodUnderTest_scenario_expectedBehavior".
1-25: Verify test coverage for edge casesThe current test covers the happy path, but consider adding tests for potential edge cases:
- Verify the order of returned fields (if order matters)
- Test immutability of the returned list
- Test for null safety
Let me help you verify the test coverage:
service-api/src/main/java/greencity/dto/econews/EcoNewsGenericDto.java (2)
3-5: Clean implementation of the Sortable interface! 👍The implementation aligns well with the PR objectives to improve sort parameter validation. The class structure maintains good separation of concerns by implementing the
Sortableinterface while preserving existing validation constraints.Also applies to: 24-24
62-74: Well-structured implementation with room for optimization 🚀The implementation is clean and uses the
SortableFieldsenum effectively. Consider these improvements:
- Cache the list to avoid recreation on every call:
public class EcoNewsGenericDto implements Sortable { + private static final List<String> SORTABLE_FIELDS = List.of( + SortableFields.ID.getFieldName(), + SortableFields.TITLE.getFieldName(), + SortableFields.CREATION_DATE.getFieldName(), + SortableFields.LIKES.getFieldName(), + SortableFields.COUNT_COMMENTS.getFieldName(), + SortableFields.COUNT_OF_ECO_NEWS.getFieldName()); + @Override public List<String> getSortableFields() { - return List.of( - SortableFields.ID.getFieldName(), - SortableFields.TITLE.getFieldName(), - SortableFields.CREATION_DATE.getFieldName(), - SortableFields.LIKES.getFieldName(), - SortableFields.COUNT_COMMENTS.getFieldName(), - SortableFields.COUNT_OF_ECO_NEWS.getFieldName()); + return SORTABLE_FIELDS; }
- Add documentation about the sorting behavior:
/** * Returns a list of field names that can be used for sorting eco news. * These fields correspond to the database columns and can be used * in ascending or descending order. * * @return an unmodifiable list of sortable field names */ @Override public List<String> getSortableFields() {Also, let's verify that all sortable fields are properly mapped in the database:
service-api/src/test/java/greencity/dto/econews/EcoNewsGenericDtoTest.java
Outdated
Show resolved
Hide resolved
KostashchukIryna
force-pushed
the
bug/8047/fix-sort-parameters-validation
branch
from
68353c5 to
c278a0c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
service-api/src/main/java/greencity/dto/econews/EcoNewsGenericDto.java (1)
24-24: Excellent architectural choice implementing Sortable!The implementation of the Sortable interface provides a clear contract for sorting capabilities and supports the PR's objective of validating sort parameters at the handler layer.
This pattern promotes type safety and makes the sorting capabilities explicit in the API contract.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
core/src/main/java/greencity/config/PageableConfig.java(1 hunks)service-api/src/main/java/greencity/dto/comment/CommentDto.java(3 hunks)service-api/src/main/java/greencity/dto/econews/EcoNewsGenericDto.java(3 hunks)service-api/src/main/java/greencity/dto/factoftheday/FactOfTheDayDTO.java(2 hunks)service-api/src/main/java/greencity/dto/friends/UserFriendDto.java(3 hunks)service-api/src/main/java/greencity/dto/habit/HabitDto.java(3 hunks)service-api/src/main/java/greencity/dto/habit/HabitManagementDto.java(3 hunks)service-api/src/main/java/greencity/dto/place/AdminPlaceDto.java(3 hunks)service-api/src/main/java/greencity/dto/placecomment/PlaceCommentAdminDto.java(2 hunks)service-api/src/main/java/greencity/dto/ratingstatistics/RatingPointsDto.java(2 hunks)service-api/src/main/java/greencity/dto/ratingstatistics/RatingStatisticsDtoForTables.java(2 hunks)service-api/src/test/java/greencity/dto/comment/CommentDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/econews/EcoNewsGenericDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/factoftheday/FactOfTheDayDTOTest.java(1 hunks)service-api/src/test/java/greencity/dto/friends/UserFriendDtoTest.java(2 hunks)service-api/src/test/java/greencity/dto/habit/HabitDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/habit/HabitManagementDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/place/AdminPlaceDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/placecomment/PlaceCommentAdminDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/ratingstatistics/RatingPointsDtoTest.java(1 hunks)service-api/src/test/java/greencity/dto/ratingstatistics/RatingStatisticsDtoForTablesTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (20)
- service-api/src/test/java/greencity/dto/comment/CommentDtoTest.java
- service-api/src/test/java/greencity/dto/ratingstatistics/RatingPointsDtoTest.java
- service-api/src/test/java/greencity/dto/placecomment/PlaceCommentAdminDtoTest.java
- service-api/src/main/java/greencity/dto/place/AdminPlaceDto.java
- service-api/src/test/java/greencity/dto/place/AdminPlaceDtoTest.java
- service-api/src/main/java/greencity/dto/habit/HabitDto.java
- service-api/src/main/java/greencity/dto/ratingstatistics/RatingStatisticsDtoForTables.java
- service-api/src/test/java/greencity/dto/factoftheday/FactOfTheDayDTOTest.java
- service-api/src/test/java/greencity/dto/friends/UserFriendDtoTest.java
- service-api/src/test/java/greencity/dto/ratingstatistics/RatingStatisticsDtoForTablesTest.java
- service-api/src/main/java/greencity/dto/factoftheday/FactOfTheDayDTO.java
- service-api/src/main/java/greencity/dto/friends/UserFriendDto.java
- service-api/src/main/java/greencity/dto/comment/CommentDto.java
- service-api/src/main/java/greencity/dto/ratingstatistics/RatingPointsDto.java
- service-api/src/test/java/greencity/dto/habit/HabitDtoTest.java
- service-api/src/test/java/greencity/dto/habit/HabitManagementDtoTest.java
- service-api/src/main/java/greencity/dto/habit/HabitManagementDto.java
- service-api/src/test/java/greencity/dto/econews/EcoNewsGenericDtoTest.java
- service-api/src/main/java/greencity/dto/placecomment/PlaceCommentAdminDto.java
- core/src/main/java/greencity/config/PageableConfig.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
service-api/src/main/java/greencity/dto/econews/EcoNewsGenericDto.java (2)
3-3: Clean import additions for sorting functionality!The new imports are well-organized and directly support the implementation of sorting capabilities.
Also applies to: 5-5
62-74: Verify field name consistency with DTO properties.The implementation looks good! Let's verify that all enum field names match the actual DTO properties to prevent runtime issues.
Consider caching this immutable list as a static final field since it won't change:
public class EcoNewsGenericDto implements Sortable { + private static final List<String> SORTABLE_FIELDS = List.of( + SortableFields.ID.getFieldName(), + SortableFields.TITLE.getFieldName(), + SortableFields.CREATION_DATE.getFieldName(), + SortableFields.LIKES.getFieldName(), + SortableFields.COUNT_COMMENTS.getFieldName(), + SortableFields.COUNT_OF_ECO_NEWS.getFieldName()); + @Override public List<String> getSortableFields() { - return List.of( - SortableFields.ID.getFieldName(), - SortableFields.TITLE.getFieldName(), - SortableFields.CREATION_DATE.getFieldName(), - SortableFields.LIKES.getFieldName(), - SortableFields.COUNT_COMMENTS.getFieldName(), - SortableFields.COUNT_OF_ECO_NEWS.getFieldName()); + return SORTABLE_FIELDS; }Let's verify the field names match the DTO properties:
✅ Verification successful
Field names verified successfully ✓
All sortable fields match the DTO properties perfectly. The optimization suggestion for caching the immutable list still stands as a good-to-have improvement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that enum field names match actual DTO properties # Get the SortableFields enum values echo "Checking SortableFields enum..." rg -l "enum SortableFields" | xargs cat # Get the field names from the DTO echo "Checking DTO properties..." rg "private \w+ \w+;" service-api/src/main/java/greencity/dto/econews/EcoNewsGenericDto.javaLength of output: 1699
|
GreenCity PR
Issue Link 📋
#8047
Issue summary
Previously, if an invalid sort field was provided, it would result in a SQL error, which would trigger a 500 status code. This approach could lead to unnecessary failures and poor user experience, especially when invalid sorting fields were provided. Or no sorting was allowed and anything with sort parameter would result in 400 code. Or SQL error was catched after executing wrong query which might be unnecessary.
Solution
This PR proposes the validation of sorting parameters in the handler layer.
Also considered using static registry of sortable values, using annotations and reflection or moving this logic to service layer.
Summary by CodeRabbit
Release Notes
New Features
Sortableinterface for defining sortable fields in DTOs.Improvements
Technical Enhancements
CustomPageableHandlerMethodArgumentResolverfor advanced pagination handling.SortPageableValidatorto enforce sorting rules.SortableFieldsenum to standardize sortable field definitions.Bug Fixes