Improve timerange validation for retransmission from view #1916
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bartekdrobczyk
previously approved these changes
Oct 23, 2024
szczygiel-m
reviewed
Oct 30, 2024
| @@ -80,4 +82,11 @@ public OfflineRetransmissionRequest getRequest() { | |||
| public String toString() { | |||
| return "OfflineRetransmissionTask{" + "taskId='" + taskId + '\'' + ", request=" + request + '}'; | |||
| } | |||
|
|
|||
| private static String parseTimestamp(@Nullable Instant startTimestamp) { | |||
There was a problem hiding this comment.
maybe just timestamp as a parameter name since there is passed both startTimestamp and endTimestamp to this function, WDYT?
| import jakarta.validation.ConstraintValidatorContext; | ||
| import pl.allegro.tech.hermes.api.OfflineRetransmissionRequest; | ||
|
|
||
| public class ProperTimeRangePresentForTopicRetransmissionValidator |
There was a problem hiding this comment.
Maybe we can remove Present and Proper from this name? I think that context would rename the same and we would shorten it a little bit 😄
| @Documented | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Target({TYPE}) | ||
| @Constraint(validatedBy = TimeRangeAbsentForViewRetransmissionValidator.class) |
There was a problem hiding this comment.
Maybe it is personal opinion, but I think that we can remove Absent from this name. I am not familiar with the pattern of specifying how the validator should behave, and what it should accept in its name. Thanks to this the name would be shorter and more readable in my opinion.
|
|
||
| import java.time.Instant | ||
|
|
||
| class RetransmissionTimeRangeValidatorTest extends Specification { |
szczygiel-m
approved these changes
Oct 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's changed?
When requesting retransmission from view, the user has already prepared proper events, so they should not provide additional time range for this retransmission. For simplicity of logic, the rules are as follows:
Basic validation for retransmission (proper parameters, timerange validation, empty checks etc.) is done via Jakarta constraint validators. Any other validation related to persistent state in Zookeeper is done later in the retransmission service.
What's new
As a part of the PR, I've also added possibility of attaching link to the offline retransmission from view documentation, visible in retransmission popup.
