feat : handle exceptional scenario #1642
Conversation
WalkthroughThis pull request introduces significant changes to a PostgreSQL notification listener application, focusing on enhancing test configuration and coverage reporting, as well as restructuring packages and improving validation. Key modifications include the relocation of Changes
Possibly related PRs
Suggested labels
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/config/NotificationProperties.java (1)
12-18: Consider adding @SInCE tag to the documentation.The documentation is clear and comprehensive. Consider adding a
@sincetag to track when these properties were introduced./* * Configuration properties for PostgreSQL notification listener. * * @param channelName Name of the notification channel to listen to * @param listenerEnabled Flag to enable/disable the notification listener * @param connectionPoolSize Size of the R2DBC connection pool (1-100) + * @since 1.0.0 */r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationController.java (1)
27-30: Add a custom error message for validation.While the regex pattern
[a-zA-Z0-9_]+is a good choice for validating channel names, consider adding a custom message to provide clear feedback when validation fails.- @PathVariable @Pattern(regexp = "[a-zA-Z0-9_]+") String channel) { + @PathVariable @Pattern( + regexp = "[a-zA-Z0-9_]+", + message = "Channel name must contain only letters, numbers, and underscores" + ) String channel) {r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java (2)
49-50: Consider making rate limiting parameters configurable.The rate limit (100) and delay (1 second) are hardcoded. Consider moving these to NotificationProperties for better configurability.
+ // In NotificationProperties.java + private final int rateLimitSize = 100; + private final Duration delayDuration = Duration.ofSeconds(1); // In NotificationListener.java - .limitRate(100) - .delayElements(Duration.ofSeconds(1)) + .limitRate(notificationProperties.rateLimitSize()) + .delayElements(notificationProperties.delayDuration())
42-42: Consider extracting retry configuration to avoid duplication.The retry configuration (3 attempts, 1s backoff) is duplicated. Consider extracting it to a constant or configuration property.
+ private static final Retry RETRY_STRATEGY = Retry.backoff(3, Duration.ofSeconds(1)); - .retryWhen(Retry.backoff(3, Duration.ofSeconds(1))) + .retryWhen(RETRY_STRATEGY) - .retryWhen(Retry.backoff(3, Duration.ofSeconds(1))) + .retryWhen(RETRY_STRATEGY)Also applies to: 63-63
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/NotificationRequest.java (1)
Line range hint
6-8: Consider PostgreSQL-specific channel name requirements.While the current validation is secure, it might be too restrictive for PostgreSQL channel names. PostgreSQL allows additional characters in identifiers.
Consider:
- Updating the pattern to match PostgreSQL identifier rules
- Adding a custom error message
- Removing redundant
@NotBlankfor channel as the pattern already prevents empty stringspublic record NotificationRequest( - @NotBlank @Pattern(regexp = "[a-zA-Z0-9_]+") String channel, + @Pattern( + regexp = "^[a-zA-Z_][a-zA-Z0-9_$]*$", + message = "Channel name must start with a letter or underscore and contain only letters, numbers, underscores, or $ characters" + ) String channel, @NotBlank String message) {}r2dbc/boot-r2dbc-notify-listen/pom.xml (3)
129-149: Good Git information tracking setup!Consider adding these useful Git properties for better deployment tracking:
git.commit.timegit.build.time<includeOnlyProperties> <includeOnlyProperty>^git.branch$</includeOnlyProperty> <includeOnlyProperty>^git.commit.id.abbrev$</includeOnlyProperty> <includeOnlyProperty>^git.commit.user.name$</includeOnlyProperty> <includeOnlyProperty>^git.commit.message.full$</includeOnlyProperty> + <includeOnlyProperty>^git.commit.time$</includeOnlyProperty> + <includeOnlyProperty>^git.build.time$</includeOnlyProperty> </includeOnlyProperties>
150-191: Well-organized test execution setup!Consider enabling parallel test execution to reduce build time:
<configuration> <runOrder>alphabetical</runOrder> <reportsDirectory>${junit.utReportFolder}</reportsDirectory> + <parallel>classes</parallel> + <threadCount>4</threadCount> <excludes>
194-263: Robust code coverage configuration!Consider adding additional coverage metrics for more comprehensive quality assurance:
<limits> <limit> <counter>COMPLEXITY</counter> <value>COVEREDRATIO</value> <minimum>${jacoco.minimum.coverage}</minimum> </limit> + <limit> + <counter>LINE</counter> + <value>COVEREDRATIO</value> + <minimum>${jacoco.minimum.coverage}</minimum> + </limit> + <limit> + <counter>BRANCH</counter> + <value>COVEREDRATIO</value> + <minimum>${jacoco.minimum.coverage}</minimum> + </limit> </limits>r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/Notifier.java (3)
29-29: Adjust log level for successful operations to reduce log noise.Using
log.infofor each successful operation might clutter the logs, especially in a production environment. Consider changing the log level todebugto reduce verbosity.Apply this diff to adjust the log level:
- .doOnSuccess(postgresqlResult -> log.info(postgresqlResult.toString())) + .doOnSuccess(postgresqlResult -> log.debug(postgresqlResult.toString()))
30-39: Improve code readability by extracting the notification sending logic into a separate method.Extracting the inner logic into a dedicated method enhances readability and makes the code easier to maintain.
Example:
private Mono<ServerResponse> sendNotification(NotificationRequest notificationRequest) { log.debug("Channel {} registered, sending notification", notificationRequest.channel()); return databaseClient .sql("SELECT pg_notify(:channel, :message)") .bind("channel", notificationRequest.channel()) .bind("message", notificationRequest.message()) .fetch() .rowsUpdated() .flatMap(rowsUpdated -> ServerResponse.ok().bodyValue(rowsUpdated)) .onErrorResume(error -> { log.error("Failed to send notification", error); return ServerResponse.status(500).bodyValue("Failed to send notification"); }); }Then update the code:
return req.bodyToMono(NotificationRequest.class) .flatMap(notificationRequest -> notificationListener .listenTo(notificationRequest.channel()) .doOnSuccess(postgresqlResult -> log.debug(postgresqlResult.toString())) - .then(Mono.defer(() -> { - log.debug("Channel {} registered, sending notification", notificationRequest.channel()); - return databaseClient - .sql("SELECT pg_notify(:channel, :message)") - .bind("channel", notificationRequest.channel()) - .bind("message", notificationRequest.message()) - .fetch() - .rowsUpdated() - .flatMap(rowsUpdated -> ServerResponse.ok().bodyValue(rowsUpdated)) - .onErrorResume(error -> { - log.error("Failed to send notification", error); - return ServerResponse.status(500).bodyValue("Failed to send notification"); - }); - }))) + .then(Mono.defer(() -> sendNotification(notificationRequest)))) .onErrorResume(error -> { log.error("Failed to process notification request", error); return ServerResponse.status(500).bodyValue("Failed to process notification request"); });
40-40: Remove redundantdoOnErrorto prevent double logging of errors.Since errors are handled in
onErrorResume, thedoOnErrormight lead to logging the same error twice.Apply this diff to remove the redundant error logging:
- .doOnError(error -> log.error("Failed to send notification", error));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
r2dbc/boot-r2dbc-notify-listen/pom.xml(2 hunks)r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/NotifyListenPGApplication.java(1 hunks)r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/config/NotificationProperties.java(2 hunks)r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationController.java(1 hunks)r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java(2 hunks)r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/NotificationRequest.java(1 hunks)r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/Notifier.java(2 hunks)r2dbc/boot-r2dbc-notify-listen/src/test/java/com/example/demo/ApplicationIntTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- r2dbc/boot-r2dbc-notify-listen/src/test/java/com/example/demo/ApplicationIntTest.java
- r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/NotifyListenPGApplication.java
🧰 Additional context used
📓 Learnings (3)
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/config/NotificationProperties.java (1)
Learnt from: rajadilipkolli
PR: rajadilipkolli/my-spring-boot-experiments#1639
File: r2dbc/boot-r2dbc-notify-listen/src/main/resources/application.properties:1-19
Timestamp: 2025-01-12T11:22:52.010Z
Learning: The NotificationListener in Spring Boot applications can be configured using properties:
- `app.notification.channel-name`: Specifies the default channel to listen to
- `app.notification.listener.enabled`: Controls whether the listener is enabled
- `app.notification.connection-pool-size`: Controls the R2DBC connection pool size
The configuration is bound using @ConfigurationProperties and @EnableConfigurationProperties annotations.
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/Notifier.java (1)
Learnt from: rajadilipkolli
PR: rajadilipkolli/my-spring-boot-experiments#1639
File: r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/Notifier.java:45-45
Timestamp: 2025-01-12T11:55:38.018Z
Learning: In PostgreSQL NOTIFY/LISTEN implementation, always ensure that a channel is being listened to before sending notifications to it. Use `listenTo` method to register the channel if it's not already being listened to.
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java (1)
Learnt from: rajadilipkolli
PR: rajadilipkolli/my-spring-boot-experiments#0
File: :0-0
Timestamp: 2025-01-12T10:45:47.987Z
Learning: The NotificationListener service in Spring Boot R2DBC applications should handle PostgreSQL notifications by:
1. Using PostgresqlConnection for low-level notification handling
2. Publishing notifications as both Spring events and reactive streams
3. Managing connection lifecycle with @PostConstruct and @PreDestroy
4. Providing methods to listen to specific channels and get notifications as Flux
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (18)
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/config/NotificationProperties.java (4)
1-1: LGTM! Package move improves code organization.Moving
NotificationPropertiesto theconfigpackage better aligns with Spring Boot conventions for configuration classes.
10-11: LGTM! Proper configuration and validation setup.The
@ConfigurationPropertiesprefix matches the expected configuration structure, and@Validatedensures property validation.
19-20: LGTM! Comprehensive validation constraints.The validation constraints are well-chosen:
@NotBlankensures a non-empty channel name@NotNullfor explicit listener enablement@Min(1) @Max(100)provides reasonable bounds for the connection pool
1-1: Verify package move impact.Let's verify that all references to
NotificationPropertieshave been updated to use the new package.✅ Verification successful
Package move verified successfully
All references to
NotificationPropertiesare using the new package pathcom.example.demo.config. No outdated references found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old package rg "com\.example\.demo\.listener\.NotificationProperties" # Search for references to the new package to confirm updates rg "com\.example\.demo\.config\.NotificationProperties"Length of output: 417
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationController.java (3)
3-5: LGTM! Good addition of validation support.The addition of validation dependencies and
@Validatedannotation properly enables method parameter validation, which aligns well with handling exceptional scenarios.Also applies to: 12-13
22-24: LGTM! Good encapsulation.The package-private visibility for
streamNotificationsaligns with the encapsulation approach used in the class.
13-17: Verify the impact of reduced visibility.The change from public to package-private visibility for both the class and constructor is good for encapsulation. However, please ensure this doesn't break any existing usage from other packages.
✅ Verification successful
Visibility change is safe and recommended ✅
The reduced visibility is perfectly fine because:
- The class is managed by Spring's component scanning (via @RestController)
- No direct instantiation or inheritance is found in the codebase
- REST endpoints remain publicly accessible regardless of class visibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any usage of NotificationController from other packages rg -l "new NotificationController\(" --type java rg -l "extends NotificationController" --type java rg -l "@Autowired.*NotificationController" --type javaLength of output: 160
Script:
#!/bin/bash # Search for any reference to NotificationController rg "NotificationController" --type java -l # Check for component scanning configurations rg "@ComponentScan|@SpringBootApplication" --type java -l # Look for the actual file to see Spring annotations fd "NotificationController.java" --type f --exec cat {}Length of output: 5483
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java (4)
3-3: LGTM! Good package organization.Moving NotificationProperties to a dedicated config package improves the separation of concerns.
23-35: Great improvements to variable naming and sink configuration!
- Descriptive variable names improve code readability
- Using multicast sink with backpressure is the correct choice for handling multiple subscribers
41-45: Good addition of retry mechanism with exponential backoff.The retry mechanism improves resilience against transient failures.
67-72: Good improvement in error handling and return type.Returning PostgresqlResult provides more information to callers, and the error logging helps with debugging.
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/NotificationRequest.java (2)
4-4: LGTM! Appropriate import for validation.The import of
Patternfrom jakarta validation constraints is correct and follows modern standards.
7-7: Verify existing channel names compatibility.Let's check if existing channel names in the codebase comply with the new validation pattern.
✅ Verification successful
Channel name validation pattern is compatible with existing usage.
The new validation pattern
[a-zA-Z0-9_]+aligns with:
- Current example usage ("events")
- Default configuration values
- PostgreSQL identifier naming best practices
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for channel name usage in the codebase echo "Searching for channel name declarations and usages..." rg -g '!*.{class,jar}' -l 'channel.*=.*["'\''].*["'\'']' . # Search for test cases covering channel name validation echo "Searching for validation test coverage..." rg -g '*Test*.java' -l 'channel.*invalid|@Pattern|ValidationException' .Length of output: 345
Script:
#!/bin/bash # Search for notification/channel related code echo "Searching for notification and channel related code..." rg -g '!*.{class,jar}' -l 'NOTIFY|LISTEN.*channel|setChannel|getChannel' . # Search for notification related test files echo "Searching for notification related test files..." fd -g '*Test*.java' --exec grep -l "Notification" {} \; # Search for validation related test files echo "Searching for validation test files..." fd -g '*Test*.java' --exec grep -l "@Valid|javax.validation|jakarta.validation" {} \;Length of output: 875
Script:
#!/bin/bash # Check content of notification related files echo "Content of NotificationListener.java:" cat ./r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java echo -e "\nContent of ContainerConfig.java:" cat ./r2dbc/boot-r2dbc-notify-listen/src/test/java/com/example/demo/common/ContainerConfig.java echo -e "\nContent of README.md:" cat ./r2dbc/boot-r2dbc-notify-listen/README.mdLength of output: 8325
r2dbc/boot-r2dbc-notify-listen/pom.xml (3)
20-29: Well-structured test configuration properties!Good choice setting the minimum coverage threshold to 80%. This provides a solid baseline for maintaining code quality while being achievable.
96-127: Comprehensive code formatting configuration!Good setup with Spotless plugin:
- Palantir Java formatter for consistent Java code style
- SQL formatting for database scripts
- Automated import optimization
Line range hint
7-8: Verify Spring Boot milestone version stabilityYou're using Spring Boot 3.4.1 which is a milestone version. Ensure it's stable enough for your needs or consider using the latest stable version.
Run this script to check the latest stable version:
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/Notifier.java (2)
20-22: Good integration ofNotificationListenerin the constructor.Including
NotificationListenerin the constructor ensures that the notifier can register channels before sending notifications, which aligns with best practices for PostgreSQL NOTIFY/LISTEN implementations.
28-29: Verify thatlistenTohandles empty or invalid channel names gracefully.Ensure that the
listenTomethod inNotificationListenerproperly handles cases where the channel name might be empty or invalid, to prevent unexpected errors.Run the following script to check for input validation in the
listenTomethod:✅ Verification successful
The implementation has sufficient error handling despite lacking explicit validation
The
listenTomethod relies on PostgreSQL's built-in channel name validation and includes comprehensive error handling:
- Errors are caught and logged via
doOnError- Retry mechanism with backoff is implemented
- Multiple logging points help track issues
While explicit validation could be added as an enhancement, the current implementation is safe and gracefully handles invalid channel names through database-level validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that NotificationListener.listenTo() validates channel names. # Search for validation logic in the listenTo method. ast-grep --pattern $'public Mono<$_> listenTo(String channel) { if (channel == null || channel.isEmpty()) { $$$ } $$$ }' --lang javaLength of output: 153
Script:
#!/bin/bash # Find the listenTo method implementation ast-grep --pattern 'listenTo(String $$$) { $$$ }' # Also search for any usage patterns to understand how it's called rg "listenTo\(" -A 5Length of output: 3403
...c/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java
Outdated
Show resolved
Hide resolved
| .bind("channel", notificationRequest.channel()) | ||
| .bind("message", notificationRequest.message()) |
There was a problem hiding this comment.
Validate input parameters to prevent SQL injection.
While parameter binding helps prevent SQL injection, it's important to ensure that the channel and message inputs are validated or sanitized to avoid any potential security risks.
Consider adding validation logic to check the channel and message contents before processing.
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/Notifier.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
r2dbc/boot-r2dbc-notify-listen/src/test/java/com/example/demo/ApplicationIntTest.java (2)
12-13: Rename test method to reflect its purposeThe method name
contextLoadsis too generic and doesn't describe what's being tested. Consider renaming it to something more specific likeshouldSuccessfullyNotifyOnValidChannel.
19-20: Document test data significanceConsider adding a comment explaining why the values "junitChannel" and "junitMessage" were chosen and what the expected behavior is.
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java (2)
44-44: Review the handling ofPostgresqlResultin subscriptionIn the
initialize()method, the subscription tolistenTo()logs thepostgresqlResultusinglog.debug(postgresqlResult.toString()). Consider whether logging thepostgresqlResultis necessary, as it may not provide meaningful information for debugging. If the result does not need to be processed, you might adjust the subscription to ignore the result or log more relevant details.
50-50: Reevaluate the use ofdelayElementsto reduce latencyThe use of
.delayElements(Duration.ofSeconds(1))introduces a 1-second delay between processing each notification. This may cause unnecessary latency in handling notifications. If rate limiting is not required, consider removing this delay to improve the responsiveness of the application.Apply this diff to remove the delay:
- .delayElements(Duration.ofSeconds(1))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationController.java(1 hunks)r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java(2 hunks)r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/NotificationRequest.java(1 hunks)r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/Notifier.java(2 hunks)r2dbc/boot-r2dbc-notify-listen/src/test/java/com/example/demo/ApplicationIntTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/NotificationRequest.java
- r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationController.java
🧰 Additional context used
📓 Learnings (2)
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java (1)
Learnt from: rajadilipkolli
PR: rajadilipkolli/my-spring-boot-experiments#0
File: :0-0
Timestamp: 2025-01-12T10:45:47.987Z
Learning: The NotificationListener service in Spring Boot R2DBC applications should handle PostgreSQL notifications by:
1. Using PostgresqlConnection for low-level notification handling
2. Publishing notifications as both Spring events and reactive streams
3. Managing connection lifecycle with @PostConstruct and @PreDestroy
4. Providing methods to listen to specific channels and get notifications as Flux
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/Notifier.java (1)
Learnt from: rajadilipkolli
PR: rajadilipkolli/my-spring-boot-experiments#1639
File: r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/Notifier.java:45-45
Timestamp: 2025-01-12T11:55:38.018Z
Learning: In PostgreSQL NOTIFY/LISTEN implementation, always ensure that a channel is being listened to before sending notifications to it. Use `listenTo` method to register the channel if it's not already being listened to.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (9)
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/Notifier.java (4)
3-5: LGTM! Well-structured class-level changes.The addition of proper logging and NotificationListener integration follows best practices and aligns with the requirement to ensure channels are being listened to before sending notifications.
Also applies to: 15-15, 17-18
20-22: LGTM! Clean constructor implementation.The constructor properly implements dependency injection for both required services.
26-35: LGTM! Robust implementation with proper error handling.The changes effectively:
- Ensure channel registration before sending notifications
- Handle errors appropriately with status 500 responses
- Use proper logging levels
37-46: Add input validation for channel and message parameters.While parameter binding helps prevent SQL injection, it's important to validate the input parameters before processing.
Let's verify if input validation has been added elsewhere:
#!/bin/bash # Search for validation annotations or checks on NotificationRequest ast-grep --pattern 'class NotificationRequest { $$$ @$_($$$) $_($$$) channel() { $$$ } $$$ }'r2dbc/boot-r2dbc-notify-listen/src/test/java/com/example/demo/ApplicationIntTest.java (2)
1-10: Well-structured test class with appropriate inheritance!The class is properly organized and follows testing best practices by extending
AbstractIntegrationTest.
12-25: Verify validation implementationLet's verify the validation constraint implementation and error handling.
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java (3)
35-35: Ensure proper backpressure handling withmulticastsinkBy switching to a
multicastsink with.onBackpressureBuffer(), all subscribers will receive the same sequence of notifications, and backpressure is handled by buffering. Make sure that this aligns with the application's requirements and that the buffer memory consumption is acceptable under load. If there's a potential for high throughput, consider specifying a buffer size or using strategies like.onBackpressureLatest().Would you like to verify the backpressure strategy to prevent potential memory issues under high load?
58-60: Handle emission failures innotificationSinkmore robustlyWhen
notificationSink.tryEmitNext(notificationEvent)fails, the current implementation logs the error. Consider implementing additional handling for emission failures. Depending on the application's requirements, you might:
- Retry emitting the notification.
- Analyze the cause of the failure and take corrective action.
- Notify an alerting system if critical notifications could be lost.
Would you like assistance in implementing a more robust error handling strategy for emission failures?
85-98: Great job replacing blocking calls with non-blocking alternativesThe previous use of
.block()in thecleanup()method has been effectively replaced with a non-blocking approach using.subscribe(). This aligns well with reactive programming best practices and improves the application's responsiveness during shutdown.
r2dbc/boot-r2dbc-notify-listen/src/test/java/com/example/demo/ApplicationIntTest.java
Outdated
Show resolved
Hide resolved
| return notificationListener.getNotificationStream(); | ||
| } | ||
|
|
||
| @GetMapping(path = "/notifications/{channel}", produces = MediaType.TEXT_EVENT_STREAM_VALUE) | ||
| public Flux<NotificationEvent> streamChannelNotifications(@PathVariable String channel) { | ||
| Flux<NotificationEvent> streamChannelNotifications( |
There was a problem hiding this comment.
@coderabbitai, please help me to write IntegrationTest for channel junitChannel of this api to confirm that it has received message send in contextLoads test of ApplicationIntTest
There was a problem hiding this comment.
@rajadilipkolli Here's the integration test for verifying message reception on the junitChannel:
@Test
void shouldReceiveNotificationOnChannel() {
// Create notification request
NotificationRequest notificationRequest = new NotificationRequest("junitChannel", "Hello from test!");
// Subscribe to channel notifications and verify message reception
webTestClient.get()
.uri("/notifications/junitChannel")
.accept(MediaType.TEXT_EVENT_STREAM)
.exchange()
.expectStatus().isOk()
.returnResult(NotificationEvent.class)
.getResponseBody()
.as(StepVerifier::create)
.then(() -> {
// Send notification after subscribing
webTestClient.post()
.uri("/api/notify")
.contentType(MediaType.APPLICATION_JSON)
.bodyValue(notificationRequest)
.exchange()
.expectStatus().isOk();
})
.assertNext(event -> {
assertThat(event.channel()).isEqualTo("junitChannel");
assertThat(event.message()).isEqualTo("Hello from test!");
})
.thenCancel() // Cancel the subscription after verification
.verify(Duration.ofSeconds(5));
}This test:
- Creates a notification request for the
junitChannel - Subscribes to the channel's notifications using WebTestClient
- Sends a notification through the
/api/notifyendpoint - Verifies that the correct notification is received
- Cancels the subscription after verification
The test uses StepVerifier to handle the reactive stream and includes a timeout to prevent hanging in case of issues.
There was a problem hiding this comment.
@coderabbitai, above code is throwing below error, please fix
java.lang.IllegalStateException: Timeout on blocking read for 5000000000 NANOSECONDS
at reactor.core.publisher.BlockingSingleSubscriber.blockingGet(BlockingSingleSubscriber.java:129)
at reactor.core.publisher.Mono.block(Mono.java:1807)
at org.springframework.test.web.reactive.server.DefaultWebTestClient$DefaultRequestBodyUriSpec.exchange(DefaultWebTestClient.java:369)
at com.example.demo.ApplicationIntTest.shouldReceiveNotificationOnChannel(ApplicationIntTest.java:57)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
Caused by: java.util.concurrent.TimeoutException: Timeout on blocking read for 5000000000 NANOSECONDS
... 7 more
There was a problem hiding this comment.
@rajadilipkolli Here's the fixed version of the test:
@Test
void shouldReceiveNotificationOnChannel() {
// Create notification request
NotificationRequest notificationRequest = new NotificationRequest("junitChannel", "Hello from test!");
// Start listening to notifications
FluxExchangeResult<NotificationEvent> result = webTestClient.get()
.uri("/notifications/junitChannel")
.accept(MediaType.TEXT_EVENT_STREAM)
.exchange()
.expectStatus().isOk()
.returnResult(NotificationEvent.class);
// Send notification
webTestClient.post()
.uri("/api/notify")
.contentType(MediaType.APPLICATION_JSON)
.bodyValue(notificationRequest)
.exchange()
.expectStatus().isOk()
.expectBody(Long.class)
.isEqualTo(1L);
// Verify the notification is received
StepVerifier.create(result.getResponseBody().take(1))
.assertNext(event -> {
assertThat(event.channel()).isEqualTo("junitChannel");
assertThat(event.message()).isEqualTo("Hello from test!");
})
.verifyComplete();
}Key changes made:
- Separated the subscription and notification sending operations
- Used
FluxExchangeResultto handle the streaming response - Added
take(1)to limit the stream to just the first event - Removed the nested notification sending from within StepVerifier
- Added proper verification of the notification response
This should resolve the timeout issue while maintaining the test's effectiveness.
There was a problem hiding this comment.
Still receiving below error, should any change be made in main code to fix this?
java.lang.IllegalStateException: Timeout on blocking read for 5000000000 NANOSECONDS
at reactor.core.publisher.BlockingSingleSubscriber.blockingGet(BlockingSingleSubscriber.java:129)
at reactor.core.publisher.Mono.block(Mono.java:1807)
at org.springframework.test.web.reactive.server.DefaultWebTestClient$DefaultRequestBodyUriSpec.exchange(DefaultWebTestClient.java:369)
at com.example.demo.ApplicationIntTest.shouldReceiveNotificationOnChannel(ApplicationIntTest.java:57)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
Caused by: java.util.concurrent.TimeoutException: Timeout on blocking read for 5000000000 NANOSECONDS
... 7 more
There was a problem hiding this comment.
@rajadilipkolli Let's modify both the controller and test code to fix this issue:
- First, update the
NotificationControllerto ensure channel readiness:
@GetMapping(path = "/notifications/{channel}", produces = MediaType.TEXT_EVENT_STREAM_VALUE)
Flux<NotificationEvent> streamChannelNotifications(
@PathVariable
@Pattern(
regexp = "[a-zA-Z0-9_]+",
message = "Channel name must contain only letters, numbers, and underscores")
String channel) {
return notificationListener.listenTo(channel)
.doOnSuccess(result -> log.debug("Successfully subscribed to channel: {}", channel))
.thenReturn(channel)
.flatMapMany(ch -> notificationListener.getNotificationStream(ch));
}- And here's the updated test:
@Test
void shouldReceiveNotificationOnChannel() {
// Create notification request
NotificationRequest notificationRequest = new NotificationRequest("junitChannel", "Hello from test!");
// Start listening to notifications
FluxExchangeResult<NotificationEvent> result = webTestClient.get()
.uri("/notifications/junitChannel")
.accept(MediaType.TEXT_EVENT_STREAM)
.exchange()
.expectStatus().isOk()
.returnResult(NotificationEvent.class);
// Wait for channel subscription to be established
try {
Thread.sleep(1000);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
// Send notification
webTestClient.post()
.uri("/api/notify")
.contentType(MediaType.APPLICATION_JSON)
.bodyValue(notificationRequest)
.exchange()
.expectStatus().isOk()
.expectBody(Long.class)
.isEqualTo(1L);
// Verify the notification is received with increased timeout
StepVerifier.create(result.getResponseBody().take(1))
.assertNext(event -> {
assertThat(event.channel()).isEqualTo("junitChannel");
assertThat(event.message()).isEqualTo("Hello from test!");
})
.verifyComplete();
}The changes:
- Added debug logging in the controller to track channel subscription
- Added a small delay after subscription to ensure the channel is ready
- Used
flatMapManyinstead ofthenManyfor better stream handling - Made the subscription chain more explicit with
thenReturn
These changes should resolve the timeout issue by ensuring proper channel initialization before sending notifications.
✏️ Learnings added
Learnt from: rajadilipkolli
PR: rajadilipkolli/my-spring-boot-experiments#1642
File: r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationController.java:27-27
Timestamp: 2025-01-12T13:47:18.377Z
Learning: When testing PostgreSQL NOTIFY/LISTEN functionality in Spring WebFlux, ensure proper channel initialization by adding a small delay between channel subscription and notification sending to prevent timeout issues.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@coderabbitai, can you once review full code and suggest the fix. debug statements are printed as changed above. Please fix the issue.
2025-01-12T19:21:56.140+05:30 INFO 25452 --- [notify-listen-postgresql] [actor-tcp-nio-1] c.e.demo.listener.NotificationListener : Listening on channel: junitChannel
2025-01-12T19:21:56.140+05:30 DEBUG 25452 --- [notify-listen-postgresql] [actor-tcp-nio-1] c.e.d.listener.NotificationController : Successfully subscribed to channel: junitChannel
java.lang.IllegalStateException: Timeout on blocking read for 5000000000 NANOSECONDS
at reactor.core.publisher.BlockingSingleSubscriber.blockingGet(BlockingSingleSubscriber.java:129)
at reactor.core.publisher.Mono.block(Mono.java:1807)
at org.springframework.test.web.reactive.server.DefaultWebTestClient$DefaultRequestBodyUriSpec.exchange(DefaultWebTestClient.java:369)
at com.example.demo.ApplicationIntTest.shouldReceiveNotificationOnChannel(ApplicationIntTest.java:59)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
Caused by: java.util.concurrent.TimeoutException: Timeout on blocking read for 5000000000 NANOSECONDS
... 7 more
There was a problem hiding this comment.
@rajadilipkolli Let's fix this by modifying the Notifier class to avoid duplicate channel subscription. Here's the updated code:
@Component
@Slf4j
public class Notifier {
private final DatabaseClient databaseClient;
public Notifier(DatabaseClient databaseClient) {
this.databaseClient = databaseClient;
}
public Mono<ServerResponse> notifyData(NotificationRequest notificationRequest) {
log.debug("Sending notification on channel: {} with message: {}",
notificationRequest.channel(), notificationRequest.message());
return sendNotification(notificationRequest)
.flatMap(rowsUpdated -> ServerResponse.ok()
.bodyValue(rowsUpdated))
.onErrorResume(e -> {
log.error("Error sending notification: {}", e.getMessage(), e);
return ServerResponse.status(HttpStatus.INTERNAL_SERVER_ERROR)
.bodyValue("Error sending notification: " + e.getMessage());
});
}
private Mono<Long> sendNotification(NotificationRequest notificationRequest) {
return databaseClient.sql("SELECT pg_notify($1, $2)")
.bind(0, notificationRequest.channel())
.bind(1, notificationRequest.message())
.fetch()
.rowsUpdated();
}
}Key changes:
- Removed the NotificationListener dependency
- Removed the channel listening step before sending notification
- Simplified the notification sending process
The test should now work as expected since:
- Controller handles channel subscription
- Notifier only sends notifications without trying to listen
- No more deadlock in the notification flow
(｀･ω･´)ゞ Thoughtfully suggesting this architectural improvement!
✏️ Learnings added
Learnt from: rajadilipkolli
PR: rajadilipkolli/my-spring-boot-experiments#1642
File: r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationController.java:27-27
Timestamp: 2025-01-12T13:55:34.238Z
Learning: When implementing PostgreSQL NOTIFY/LISTEN pattern in Spring WebFlux, separate the concerns:
1. NotificationController/Listener should handle channel subscriptions
2. Notifier should only send notifications without attempting to listen
This prevents deadlocks and timeout issues in integration tests.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
...c/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
r2dbc/boot-r2dbc-notify-listen/src/main/resources/application.properties (1)
9-10: LGTM! Good addition for enhanced error handling.Enabling WebFlux problem details is a good practice as it provides standardized error responses following RFC 7807, which helps clients better understand and handle error scenarios.
This configuration will ensure that your error responses include:
- A type URI identifying the error
- A title describing the error
- HTTP status
- A detailed error message
- Instance information
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java (2)
41-61: Consider making retry and rate limit parameters configurable.The retry mechanism and rate limiting are good additions, but consider moving the magic numbers to configuration:
- Retry attempts (3)
- Backoff duration (1 second)
- Rate limit (100)
@PostConstruct public void initialize() { listenTo(notificationProperties.channelName()) - .retryWhen(Retry.backoff(3, Duration.ofSeconds(1))) + .retryWhen(Retry.backoff( + notificationProperties.maxRetries(), + notificationProperties.backoffDuration())) .subscribe(null, error -> log.error("Fatal error in channel subscription", error)); postgresqlConnection .getNotifications() - .limitRate(100) + .limitRate(notificationProperties.rateLimit())
82-95: Consider enhancing cleanup robustness.While the cleanup process is improved, consider these enhancements:
- Add timeout to prevent hanging during shutdown
- Handle case where UNLISTEN fails but connection close succeeds
@PreDestroy public void cleanup() { notificationSink.tryEmitComplete(); Mono.from(postgresqlConnection .createStatement("UNLISTEN " + notificationProperties.channelName()) .execute()) .then(postgresqlConnection.close()) + .timeout(Duration.ofSeconds(5)) + .onErrorResume(error -> { + log.error("Error during UNLISTEN, attempting to close connection", error); + return postgresqlConnection.close(); + }) .doOnError(error -> log.error("Error closing connection", error)) .doOnSuccess(unused -> log.info("Notification listener connection closed")) .retryWhen(Retry.backoff(3, Duration.ofSeconds(1))) .subscribe( unused -> log.info("Notification listener cleanup completed"), error -> log.error("Failed to cleanup notification listener", error)); }r2dbc/boot-r2dbc-notify-listen/src/test/java/com/example/demo/ApplicationIntTest.java (2)
18-31: Enhance test coverage by verifying notification deliveryWhile the test verifies the API response, it doesn't confirm if the notification was actually delivered. Consider combining this with a notification listener to verify end-to-end functionality.
You can use the approach shown in the disabled test
shouldReceiveNotificationOnChannelas a reference.
33-51: Add more validation test casesThe test covers invalid channel name with special characters, but consider adding tests for:
- Empty channel name
- Empty message
- Null values
Example test case for empty channel:
@Test void shouldRejectEmptyChannelName() { this.webTestClient .post() .uri("/api/notify") .accept(MediaType.APPLICATION_JSON) .contentType(MediaType.APPLICATION_JSON) .bodyValue(new NotificationRequest("", "message")) .exchange() .expectStatus() .isBadRequest() .expectBody(ProblemDetail.class) .value(problemDetail -> { assertThat(problemDetail.getStatus()).isEqualTo(400); assertThat(problemDetail.getDetail()).isNotEmpty(); }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java(2 hunks)r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/Notifier.java(2 hunks)r2dbc/boot-r2dbc-notify-listen/src/main/resources/application.properties(1 hunks)r2dbc/boot-r2dbc-notify-listen/src/test/java/com/example/demo/ApplicationIntTest.java(1 hunks)
🧰 Additional context used
📓 Learnings (2)
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java (6)
Learnt from: rajadilipkolli
PR: rajadilipkolli/my-spring-boot-experiments#0
File: :0-0
Timestamp: 2025-01-12T10:45:47.987Z
Learning: The NotificationListener service in Spring Boot R2DBC applications should handle PostgreSQL notifications by:
1. Using PostgresqlConnection for low-level notification handling
2. Publishing notifications as both Spring events and reactive streams
3. Managing connection lifecycle with @PostConstruct and @PreDestroy
4. Providing methods to listen to specific channels and get notifications as Flux
Learnt from: rajadilipkolli
PR: rajadilipkolli/my-spring-boot-experiments#1642
File: r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationController.java:27-27
Timestamp: 2025-01-12T13:47:18.533Z
Learning: When testing PostgreSQL NOTIFY/LISTEN functionality in Spring WebFlux, ensure proper channel initialization by adding a small delay between channel subscription and notification sending to prevent timeout issues.
Learnt from: rajadilipkolli
PR: rajadilipkolli/my-spring-boot-experiments#1642
File: r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java:67-67
Timestamp: 2025-01-12T13:49:28.393Z
Learning: In Spring WebFlux applications with Bean Validation, handle ConstraintViolationException globally using @RestControllerAdvice to convert validation errors into HTTP 400 responses with problem details. Keep validation at the controller level using annotations like @Pattern and @NotBlank, rather than implementing it in the service layer.
Learnt from: rajadilipkolli
PR: rajadilipkolli/my-spring-boot-experiments#1642
File: r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java:67-67
Timestamp: 2025-01-12T13:45:58.565Z
Learning: In Spring WebFlux applications, use @RestControllerAdvice instead of @ControllerAdvice for exception handling, and ensure the handler methods return Mono/Flux to maintain the reactive flow. Additionally, use @ResponseStatus to explicitly define the HTTP status code.
Learnt from: rajadilipkolli
PR: rajadilipkolli/my-spring-boot-experiments#1642
File: r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java:67-67
Timestamp: 2025-01-12T13:38:22.906Z
Learning: In reactive applications, to handle HTTP 400 (Bad Request) responses, create a custom exception and handle it using a global exception handler (@ControllerAdvice) instead of using ResponseStatusException directly in the service layer.
Learnt from: rajadilipkolli
PR: rajadilipkolli/my-spring-boot-experiments#1642
File: r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java:67-67
Timestamp: 2025-01-12T13:34:29.877Z
Learning: In the NotificationListener class, the channel name must be validated to contain only letters, numbers, and underscores (pattern: [a-zA-Z0-9_]+). Invalid channel names should result in a BadRequest (400) response.
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/Notifier.java (2)
Learnt from: rajadilipkolli
PR: rajadilipkolli/my-spring-boot-experiments#1642
File: r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java:67-67
Timestamp: 2025-01-12T13:49:28.393Z
Learning: In Spring WebFlux applications with Bean Validation, handle ConstraintViolationException globally using @RestControllerAdvice to convert validation errors into HTTP 400 responses with problem details. Keep validation at the controller level using annotations like @Pattern and @NotBlank, rather than implementing it in the service layer.
Learnt from: rajadilipkolli
PR: rajadilipkolli/my-spring-boot-experiments#1639
File: r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/Notifier.java:45-45
Timestamp: 2025-01-12T11:55:38.018Z
Learning: In PostgreSQL NOTIFY/LISTEN implementation, always ensure that a channel is being listened to before sending notifications to it. Use `listenTo` method to register the channel if it's not already being listened to.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/listener/NotificationListener.java (3)
3-16: LGTM! Clean import organization.The relocation of
NotificationPropertiesto the config package improves separation of concerns, and the new imports support the enhanced functionality.
23-35: Great improvements to variable naming and sink configuration!The changes enhance code clarity and scalability:
- Descriptive variable names (
postgresqlConnection,notificationProperties)- Using multicast sink allows multiple subscribers to receive notifications
64-69: LGTM! Clean implementation of the listen operation.The method correctly returns the PostgreSQL result and includes proper error logging. As per the learnings, channel name validation is handled at the controller level.
r2dbc/boot-r2dbc-notify-listen/src/test/java/com/example/demo/ApplicationIntTest.java (1)
53-92: Clarify why the test is disabledThe test appears to be well-structured for verifying end-to-end notification flow. Please add a comment explaining why it's disabled or enable it if it's ready for use.
r2dbc/boot-r2dbc-notify-listen/src/main/java/com/example/demo/notifier/Notifier.java (2)
23-30: LGTM! Good dependency injection and logging setup.The addition of NotificationListener aligns with the best practice of ensuring channels are listened to before sending notifications.
65-72: LGTM! Well-structured notification logic.The method follows good practices:
- Uses parameterized queries for SQL injection prevention
- Includes appropriate debug logging
- Clean separation of concerns
| return req.bodyToMono(NotificationRequest.class) | ||
| .flatMap(notificationRequest -> { | ||
| Validator validator = | ||
| Validation.buildDefaultValidatorFactory().getValidator(); | ||
| Set<ConstraintViolation<NotificationRequest>> violations = validator.validate(notificationRequest); | ||
| if (!violations.isEmpty()) { | ||
| String errorMessage = violations.stream() | ||
| .map(ConstraintViolation::getMessage) | ||
| .collect(Collectors.joining(", ")); | ||
| ProblemDetail problem = ProblemDetail.forStatusAndDetail(HttpStatus.BAD_REQUEST, errorMessage); | ||
| return ServerResponse.badRequest() | ||
| .contentType(MediaType.APPLICATION_PROBLEM_JSON) | ||
| .bodyValue(problem); | ||
| } | ||
| return notificationListener | ||
| .listenTo(notificationRequest.channel()) | ||
| .doOnSuccess(postgresqlResult -> log.debug(postgresqlResult.toString())) | ||
| .then(Mono.defer(() -> sendNotification(notificationRequest) | ||
| .flatMap(rowsUpdated -> ServerResponse.ok().bodyValue(rowsUpdated)) | ||
| .onErrorResume(e -> { | ||
| log.error("Error sending notification: {}", e.getMessage(), e); | ||
| return ServerResponse.status(HttpStatus.INTERNAL_SERVER_ERROR) | ||
| .bodyValue("Error sending notification: " + e.getMessage()); | ||
| }))); | ||
| }) | ||
| .onErrorResume(error -> { | ||
| log.error("Failed to process notification request", error); | ||
| return ServerResponse.status(500).bodyValue("Failed to process notification request"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move validation to controller layer using annotations
The current validation implementation in the service layer goes against the application's established pattern. Consider moving the validation to the controller layer using annotations on the NotificationRequest class.
- Add validation annotations to NotificationRequest:
public record NotificationRequest(
@NotBlank(message = "Channel name is required")
@Pattern(regexp = "[a-zA-Z0-9_]+", message = "Channel name must contain only letters, numbers, and underscores")
String channel,
@NotBlank(message = "Message is required")
String message
) {}- Remove manual validation from the service layer and let Spring's validation handle it through the
@Validannotation in the controller.
Summary by CodeRabbit
Chores
New Features
Bug Fixes
Documentation