more meaningful error messages in EventDtoRequestValidator #8058
Conversation
WalkthroughThe pull request introduces modifications to the event validation and exception handling mechanism in the Green City application. The changes primarily focus on refactoring the Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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
|
| * | ||
| * @param ex the exception | ||
| * @param request the current request | ||
| * @return a {@code ResponseEntity} message |
There was a problem hiding this comment.
There are the same handler for parent class (ValidationException) of EventDtoValidationException, so I think there are not need in this code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
core/src/main/java/greencity/validator/EventDtoRequestValidator.java (2)
42-64: Great use of modern switch expressions and structured error handling! Consider extracting common validation logic.The refactoring improves code readability and error handling. However, there's an opportunity to reduce duplication by extracting the common validation sequence into a private method.
Consider this refactor to reduce duplication:
@Override public boolean isValid(Object value, ConstraintValidatorContext context) { try { switch (value) { case AddEventDtoRequest addEventDtoRequest -> { - validateDateLocations(addEventDtoRequest.getDatesLocations()); - convertToUTC(addEventDtoRequest.getDatesLocations()); - validateEventDateLocations(addEventDtoRequest.getDatesLocations()); - validateTags(addEventDtoRequest.getTags()); + validateEventDto(addEventDtoRequest.getDatesLocations(), addEventDtoRequest.getTags()); } case UpdateEventRequestDto updateEventDto -> { - validateDateLocations(updateEventDto.getDatesLocations()); - convertToUTC(updateEventDto.getDatesLocations()); - validateEventDateLocations(updateEventDto.getDatesLocations()); - validateTags(updateEventDto.getTags()); + validateEventDto(updateEventDto.getDatesLocations(), updateEventDto.getTags()); } default -> { return false; } } return true; } catch (EventDtoValidationException ex) { context.disableDefaultConstraintViolation(); context.buildConstraintViolationWithTemplate(ex.getMessage()).addConstraintViolation(); return false; } } + private <T extends AbstractEventDateLocationDto> void validateEventDto(List<T> datesLocations, List<String> tags) { + validateDateLocations(datesLocations); + convertToUTC(datesLocations); + validateEventDateLocations(datesLocations); + validateTags(tags); + }
Line range hint
89-124: Consider decomposing validateEventDateLocations for better maintainability.The method currently handles multiple validation concerns: duplicate dates, date range validation, location validation, and URL validation. Breaking this into smaller, focused methods would improve readability and maintainability.
Here's a suggested refactor:
private <T extends AbstractEventDateLocationDto> void validateEventDateLocations(List<T> eventDateLocationDtos) { + validateUniqueDates(eventDateLocationDtos); + validateDateRanges(eventDateLocationDtos); + validateLocationDetails(eventDateLocationDtos); + } + + private <T extends AbstractEventDateLocationDto> void validateUniqueDates(List<T> eventDateLocationDtos) { Set<LocalDateTime> startDateSet = new HashSet<>(); Set<LocalDateTime> finishDateSet = new HashSet<>(); for (T eventDateLocationDto : eventDateLocationDtos) { LocalDateTime startDate = eventDateLocationDto.getStartDate().toLocalDateTime(); LocalDateTime finishDate = eventDateLocationDto.getFinishDate().toLocalDateTime(); if (!startDateSet.add(startDate) || !finishDateSet.add(finishDate)) { throw new EventDtoValidationException(ErrorMessage.SAME_EVENT_DATES); } } + } + + private <T extends AbstractEventDateLocationDto> void validateDateRanges(List<T> eventDateLocationDtos) { + for (T eventDateLocationDto : eventDateLocationDtos) { if (eventDateLocationDto.getStartDate().isBefore(ZonedDateTime.now(ZoneOffset.UTC)) || eventDateLocationDto.getStartDate().isAfter(eventDateLocationDto.getFinishDate()) || eventDateLocationDto.getStartDate().isAfter(ZonedDateTime.now(ZoneOffset.UTC) .plusYears(MAX_YEARS_OF_PLANNING))) { throw new EventDtoValidationException(ErrorMessage.EVENT_START_DATE_AFTER_FINISH_DATE_OR_IN_PAST); } + } + } + + private <T extends AbstractEventDateLocationDto> void validateLocationDetails(List<T> eventDateLocationDtos) { + for (T eventDateLocationDto : eventDateLocationDtos) { if (eventDateLocationDto.getOnlineLink() == null && eventDateLocationDto.getCoordinates() == null) { throw new EventDtoValidationException(ErrorMessage.NO_EVENT_LINK_OR_ADDRESS); } if (eventDateLocationDto.getOnlineLink() != null) { isUrlValid(eventDateLocationDto.getOnlineLink()); } } }core/src/test/java/greencity/validator/EventDtoRequestValidatorTest.java (2)
184-185: Consider adding more descriptive assertion messages.The test assertions would be more maintainable with detailed failure messages that explain the expected behavior.
- assertFalse(isValid); + assertFalse(isValid, "Validation should fail for null dates in UpdateEventRequestDto");
Line range hint
44-186: Consider adding tests for additional edge cases.The test suite is comprehensive but could benefit from additional test cases:
- Validation of empty or whitespace-only tags
- Validation of malformed coordinates
- Boundary testing for MAX_YEARS_OF_PLANNING
Example test case to add:
@Test void withEmptyTagsException() { AddEventDtoRequest addEventDtoRequest = ModelUtils.getAddEventDtoRequest(); addEventDtoRequest.setTags(List.of("", " ", "\t")); boolean isValid = validator.isValid(addEventDtoRequest, constraintValidatorContext); assertFalse(isValid, "Validation should fail for empty or whitespace-only tags"); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/greencity/exception/handler/CustomExceptionHandler.java(0 hunks)core/src/main/java/greencity/validator/EventDtoRequestValidator.java(1 hunks)core/src/test/java/greencity/validator/EventDtoRequestValidatorTest.java(4 hunks)
💤 Files with no reviewable changes (1)
- core/src/main/java/greencity/exception/handler/CustomExceptionHandler.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
core/src/test/java/greencity/validator/EventDtoRequestValidatorTest.java (1)
30-42: Well-structured mock setup following best practices!The mock configuration is clean and properly initializes the validation context for testing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
core/src/main/java/greencity/validator/EventDtoRequestValidator.java (2)
42-57: Great use of modern Java features and improved error handling!The switch expression provides a cleaner and more maintainable approach compared to instanceof checks. The error handling with proper context management is well implemented.
Consider extracting error messages to constants for better maintainability:
} catch (EventDtoValidationException ex) { context.disableDefaultConstraintViolation(); - context.buildConstraintViolationWithTemplate(ex.getMessage()).addConstraintViolation(); + String errorMessage = ex.getMessage(); + context.buildConstraintViolationWithTemplate(errorMessage).addConstraintViolation(); return false; }
81-84: Excellent separation of concerns in validation logic!The breakdown into specific validation methods (
validateUniqueDates,validateDateRanges,validateLocationDetails) improves code readability and maintainability. Each method has a clear, single responsibility.Consider using a Set for better performance in date uniqueness validation:
- Set<LocalDateTime> startDateSet = new HashSet<>(); - Set<LocalDateTime> finishDateSet = new HashSet<>(); + Set<LocalDateTime> dateSet = new HashSet<>(); for (T eventDateLocationDto : eventDateLocationDtos) { LocalDateTime startDate = eventDateLocationDto.getStartDate().toLocalDateTime(); LocalDateTime finishDate = eventDateLocationDto.getFinishDate().toLocalDateTime(); - if (!startDateSet.add(startDate) || !finishDateSet.add(finishDate)) { + if (!dateSet.add(startDate) || !dateSet.add(finishDate)) { throw new EventDtoValidationException(ErrorMessage.SAME_EVENT_DATES); } }Also applies to: 86-98, 100-109, 111-121
core/src/test/java/greencity/validator/EventDtoRequestValidatorTest.java (1)
47-48: Well-structured test cases with clear assertions!The test cases provide good coverage of validation scenarios with clear failure messages.
Consider adding
@DisplayNameannotations to make test reports more readable:@Test +@DisplayName("Should fail validation when dates are missing") void withoutDatesException() { AddEventDtoRequest addEventDtoRequest = ModelUtils.getEventDtoWithoutDates(); boolean isValid = validator.isValid(addEventDtoRequest, constraintValidatorContext); assertFalse(isValid, "Validation should fail and return false for empty date locations"); }Also applies to: 54-55, 61-62, 68-69, 75-76, 82-83, 89-90, 96-97
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/greencity/validator/EventDtoRequestValidator.java(4 hunks)core/src/test/java/greencity/validator/EventDtoRequestValidatorTest.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
core/src/main/java/greencity/validator/EventDtoRequestValidator.java (1)
130-135: Well-structured validation workflow!The
validateEventDtomethod provides a clear and organized sequence of validation steps. The generic type parameter makes it reusable for both add and update scenarios.core/src/test/java/greencity/validator/EventDtoRequestValidatorTest.java (1)
30-42: Excellent test setup with proper mocking!The mock setup for
ConstraintValidatorContextand its builder is well-implemented, following testing best practices.
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| MockitoAnnotations.openMocks(this); |
There was a problem hiding this comment.
Ideally, the mocks should be closed, so that Mockito will function properly.
https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/MockitoAnnotations.html
|
#8008
Was before:
Would be after merge:
Summary by CodeRabbit
Summary by CodeRabbit
Refactor
Bug Fixes
EventDtoValidationException, reverting to default error handling.