[RHCLOUD-44982] Remove Camel from Slack connector#4286
Open
g-duval wants to merge 1 commit intoRedHatInsights:masterfrom
Open
[RHCLOUD-44982] Remove Camel from Slack connector#4286g-duval wants to merge 1 commit intoRedHatInsights:masterfrom
g-duval wants to merge 1 commit intoRedHatInsights:masterfrom
Conversation
Contributor
Summary by CodeRabbit
WalkthroughReplaces Camel-based routing and exchange models with a handler-based Slack connector: removes Camel classes, updates Maven dependencies to v2 coordinates and Quarkus fault-tolerance, adds a CDI Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Incoming CloudEvent
participant Handler as SlackMessageHandler
participant Validator as Jakarta Validator
participant Template as TemplateService
participant REST as SlackRestClient
participant Result as HandledHttpMessageDetails
Client->>Handler: deliver JsonObject payload
Handler->>Validator: validate SlackNotification
Validator-->>Handler: validation result / violations
alt validation fails
Handler-->>Client: throw ConstraintViolationException
else validation passes
Handler->>Template: renderTemplate(definition, eventData)
Template-->>Handler: rendered string
Handler->>REST: post(webhookUrl, payload)
REST-->>Handler: HTTP Response
Handler->>Result: build HandledHttpMessageDetails(targetUrl, status)
Handler-->>Client: return Result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
SC Environment Impact AssessmentOverall Impact: 🟢 LOW View full reportSummary
Detailed Findings🟢 LOW ImpactEnvironment configuration change detected
Required Actions
This assessment was automatically generated. Please review carefully and consult with the ROSA Core team for critical/high impact changes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
connector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackNotification.java (1)
8-18: Consider adding@RegisterForReflectionfor native image support.The parent class
NotificationToConnectorhas the annotation, but other connector subclasses with public fields (DrawerNotificationToConnector,NotificationToConnectorHttp) explicitly add@RegisterForReflection. SlackNotification's public fields (webhookUrl,eventData,channel) should be similarly registered for Quarkus native builds to ensure JSON deserialization via reflection works correctly.💡 Proposed addition
package com.redhat.cloud.notifications.connector.slack; import com.redhat.cloud.notifications.connector.v2.models.NotificationToConnector; +import io.quarkus.runtime.annotations.RegisterForReflection; import jakarta.validation.constraints.NotBlank; import jakarta.validation.constraints.NotNull; import java.util.Map; +@RegisterForReflection public class SlackNotification extends NotificationToConnector {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackNotification.java` around lines 8 - 18, Add the Quarkus reflection registration to SlackNotification so its public fields (webhookUrl, eventData, channel) are available for native-image JSON deserialization: annotate the SlackNotification class with `@RegisterForReflection` (same as NotificationToConnector and other connector subclasses like DrawerNotificationToConnector and NotificationToConnectorHttp) and import the annotation; ensure the annotation sits on the class declaration so reflection-based frameworks can access those public fields at runtime.connector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackMessageHandlerTest.java (1)
212-235: Assert no outbound call on validation failuresIn these validation tests, add
verifyNoInteractions(...)to ensure fail-fast behavior never reaches template rendering or webhook posting. This guards against regressions where side effects happen before validation completes.Suggested test hardening
import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; @@ void testNullWebhookUrl() { @@ assertThrows(ConstraintViolationException.class, () -> handler.handle(buildIncomingCloudEvent("test-id", "test-type", payload)) ); + verifyNoInteractions(templateService, connectorConfig, webhookRestClient); } @@ void testBlankWebhookUrl() { @@ assertThrows(ConstraintViolationException.class, () -> handler.handle(buildIncomingCloudEvent("test-id", "test-type", payload)) ); + verifyNoInteractions(templateService, connectorConfig, webhookRestClient); } @@ void testNullEventData() { @@ assertThrows(ConstraintViolationException.class, () -> handler.handle(buildIncomingCloudEvent("test-id", "test-type", payload)) ); + verifyNoInteractions(templateService, connectorConfig, webhookRestClient); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackMessageHandlerTest.java` around lines 212 - 235, Add assertions to the validation tests in SlackMessageHandlerTest to ensure no outbound interactions occur when handler.handle(...) throws ConstraintViolationException: after each assertThrows(call to handler.handle(buildIncomingCloudEvent(...))), call verifyNoInteractions(...) on the mocks used for rendering/posting (e.g., the template renderer and the Slack webhook client used by SlackMessageHandler) so the tests assert fail-fast behavior and that methods like the template rendering and webhook posting are never invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@connector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackMessageHandler.java`:
- Around line 47-55: The code in SlackMessageHandler calls
incomingCloudEvent.getData().mapTo(...) without checking for null, causing an
NPE when the CloudEvent payload is missing; add a null guard before mapTo by
checking incomingCloudEvent.getData() (or
incomingCloudEvent.getData().isPresent()/!= null depending on type) and if
absent throw a ConstraintViolationException (or a clear
IllegalArgumentException) with a descriptive message like "Validation failed:
missing cloud event data" so the validation path
(validator.validate(notification) / ConstraintViolationException) is preserved
and no raw NPE occurs.
In
`@connector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackConnectorIntegrationTest.java`:
- Around line 31-54: The test helper buildIncomingPayload creates inconsistent
organization IDs: it sets payload root "org_id" to the literal "12345" while
eventData.orgId uses TestConstants.DEFAULT_ORG_ID; update buildIncomingPayload
so both use the same source (replace the hardcoded "12345" with
TestConstants.DEFAULT_ORG_ID or vice versa) to ensure payload.org_id and
eventData.orgId match and keep tests consistent.
---
Nitpick comments:
In
`@connector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackNotification.java`:
- Around line 8-18: Add the Quarkus reflection registration to SlackNotification
so its public fields (webhookUrl, eventData, channel) are available for
native-image JSON deserialization: annotate the SlackNotification class with
`@RegisterForReflection` (same as NotificationToConnector and other connector
subclasses like DrawerNotificationToConnector and NotificationToConnectorHttp)
and import the annotation; ensure the annotation sits on the class declaration
so reflection-based frameworks can access those public fields at runtime.
In
`@connector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackMessageHandlerTest.java`:
- Around line 212-235: Add assertions to the validation tests in
SlackMessageHandlerTest to ensure no outbound interactions occur when
handler.handle(...) throws ConstraintViolationException: after each
assertThrows(call to handler.handle(buildIncomingCloudEvent(...))), call
verifyNoInteractions(...) on the mocks used for rendering/posting (e.g., the
template renderer and the Slack webhook client used by SlackMessageHandler) so
the tests assert fail-fast behavior and that methods like the template rendering
and webhook posting are never invoked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85206cd8-871e-42cd-979d-d3235b3080b7
📒 Files selected for processing (16)
connector-slack/pom.xmlconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/CamelNotification.javaconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/ExchangeProperty.javaconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackCloudEventDataExtractor.javaconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackExceptionHandler.javaconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackMessageHandler.javaconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackNotification.javaconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackRestClient.javaconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackRouteBuilder.javaconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/config/SlackConnectorConfig.javaconnector-slack/src/main/resources/application.propertiesconnector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackConnectorIntegrationTest.javaconnector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackConnectorRoutesMessageBlocksFormatTest.javaconnector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackConnectorRoutesTest.javaconnector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackMessageHandlerTest.javaconnector-slack/src/test/resources/application.properties
💤 Files with no reviewable changes (6)
- connector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/ExchangeProperty.java
- connector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/CamelNotification.java
- connector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackConnectorRoutesTest.java
- connector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackRouteBuilder.java
- connector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackConnectorRoutesMessageBlocksFormatTest.java
- connector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackCloudEventDataExtractor.java
...-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackMessageHandler.java
Show resolved
Hide resolved
Comment on lines
+31
to
+54
| protected JsonObject buildIncomingPayload(String targetUrl) { | ||
| JsonObject payload = new JsonObject(); | ||
| payload.put("org_id", "12345"); | ||
| payload.put("webhookUrl", targetUrl); | ||
| payload.put("channel", "#test-channel"); | ||
|
|
||
| Map<String, Object> source = new HashMap<>(); | ||
| source.put("event_type", Map.of("display_name", SubscriptionServices.ERRATA_NEW_SUBSCRIPTION_BUGFIX_ERRATA)); | ||
| source.put("application", Map.of("display_name", SubscriptionServices.ERRATA_APP_NAME)); | ||
| source.put("bundle", Map.of("display_name", SubscriptionServices.BUNDLE_NAME)); | ||
|
|
||
| Map<String, Object> eventData = new HashMap<>(); | ||
| eventData.put("bundle", SubscriptionServices.BUNDLE_NAME); | ||
| eventData.put("application", SubscriptionServices.ERRATA_APP_NAME); | ||
| eventData.put("event_type", SubscriptionServices.ERRATA_NEW_SUBSCRIPTION_BUGFIX_ERRATA); | ||
| eventData.put("events", new ArrayList<>()); | ||
| eventData.put("environment", Map.of("url", new ArrayList<>())); | ||
| eventData.put("orgId", TestConstants.DEFAULT_ORG_ID); | ||
| eventData.put("source", source); | ||
|
|
||
| payload.put("eventData", eventData); | ||
|
|
||
| return payload; | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent org_id values between payload root and eventData.
Line 33 sets org_id to "12345" while Line 48 sets eventData.orgId to TestConstants.DEFAULT_ORG_ID. If these values should match (e.g., for validation or logging consistency), consider using the same constant for both.
🔧 Proposed fix
protected JsonObject buildIncomingPayload(String targetUrl) {
JsonObject payload = new JsonObject();
- payload.put("org_id", "12345");
+ payload.put("org_id", TestConstants.DEFAULT_ORG_ID);
payload.put("webhookUrl", targetUrl);
payload.put("channel", "#test-channel");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| protected JsonObject buildIncomingPayload(String targetUrl) { | |
| JsonObject payload = new JsonObject(); | |
| payload.put("org_id", "12345"); | |
| payload.put("webhookUrl", targetUrl); | |
| payload.put("channel", "#test-channel"); | |
| Map<String, Object> source = new HashMap<>(); | |
| source.put("event_type", Map.of("display_name", SubscriptionServices.ERRATA_NEW_SUBSCRIPTION_BUGFIX_ERRATA)); | |
| source.put("application", Map.of("display_name", SubscriptionServices.ERRATA_APP_NAME)); | |
| source.put("bundle", Map.of("display_name", SubscriptionServices.BUNDLE_NAME)); | |
| Map<String, Object> eventData = new HashMap<>(); | |
| eventData.put("bundle", SubscriptionServices.BUNDLE_NAME); | |
| eventData.put("application", SubscriptionServices.ERRATA_APP_NAME); | |
| eventData.put("event_type", SubscriptionServices.ERRATA_NEW_SUBSCRIPTION_BUGFIX_ERRATA); | |
| eventData.put("events", new ArrayList<>()); | |
| eventData.put("environment", Map.of("url", new ArrayList<>())); | |
| eventData.put("orgId", TestConstants.DEFAULT_ORG_ID); | |
| eventData.put("source", source); | |
| payload.put("eventData", eventData); | |
| return payload; | |
| } | |
| protected JsonObject buildIncomingPayload(String targetUrl) { | |
| JsonObject payload = new JsonObject(); | |
| payload.put("org_id", TestConstants.DEFAULT_ORG_ID); | |
| payload.put("webhookUrl", targetUrl); | |
| payload.put("channel", "#test-channel"); | |
| Map<String, Object> source = new HashMap<>(); | |
| source.put("event_type", Map.of("display_name", SubscriptionServices.ERRATA_NEW_SUBSCRIPTION_BUGFIX_ERRATA)); | |
| source.put("application", Map.of("display_name", SubscriptionServices.ERRATA_APP_NAME)); | |
| source.put("bundle", Map.of("display_name", SubscriptionServices.BUNDLE_NAME)); | |
| Map<String, Object> eventData = new HashMap<>(); | |
| eventData.put("bundle", SubscriptionServices.BUNDLE_NAME); | |
| eventData.put("application", SubscriptionServices.ERRATA_APP_NAME); | |
| eventData.put("event_type", SubscriptionServices.ERRATA_NEW_SUBSCRIPTION_BUGFIX_ERRATA); | |
| eventData.put("events", new ArrayList<>()); | |
| eventData.put("environment", Map.of("url", new ArrayList<>())); | |
| eventData.put("orgId", TestConstants.DEFAULT_ORG_ID); | |
| eventData.put("source", source); | |
| payload.put("eventData", eventData); | |
| return payload; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@connector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackConnectorIntegrationTest.java`
around lines 31 - 54, The test helper buildIncomingPayload creates inconsistent
organization IDs: it sets payload root "org_id" to the literal "12345" while
eventData.orgId uses TestConstants.DEFAULT_ORG_ID; update buildIncomingPayload
so both use the same source (replace the hardcoded "12345" with
TestConstants.DEFAULT_ORG_ID or vice versa) to ensure payload.org_id and
eventData.orgId match and keep tests consistent.
g-duval
force-pushed
the
RHCLOUD-44982_migrateSlackConnector
branch
from
50fe3b8 to
d505f82
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
connector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackMessageHandlerTest.java (1)
35-49: Consider adding a test for REST client exceptions.The handler uses
webhookRestClient.post()within a try-with-resources block. While the current tests cover success and error HTTP statuses, there's no test for the scenario wherewebhookRestClient.post()throws an exception (e.g., network timeout, connection refused).If such exceptions should propagate to the caller or be handled by the
SlackExceptionHandler, a test verifying that behavior would improve coverage.Example test for REST client exception
`@Test` void testRestClientException() { Map<String, Object> eventData = new HashMap<>(); eventData.put("bundle", "rhel"); when(connectorConfig.isUseBetaTemplatesEnabled(any(), any(), any(), any())).thenReturn(false); when(templateService.renderTemplate(any(), anyMap())).thenReturn("{}"); when(webhookRestClient.post(anyString(), anyString())) .thenThrow(new ProcessingException("Connection refused")); JsonObject payload = buildPayload("http://localhost/webhook", eventData, null); assertThrows(ProcessingException.class, () -> handler.handle(buildIncomingCloudEvent("test-id", "test-type", payload)) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackMessageHandlerTest.java` around lines 35 - 49, Add a unit test covering the case where webhookRestClient.post(...) throws a runtime client exception: mock connectorConfig.isUseBetaTemplatesEnabled(...) and templateService.renderTemplate(...) as in existing tests, then stub webhookRestClient.post(anyString(), anyString()) to throw a ProcessingException (or the specific client exception used), call handler.handle(...) with a constructed CloudEvent payload (use buildPayload/buildIncomingCloudEvent helpers used in this test class), and assert the expected behavior—either assertThrows(ProcessingException.class, ...) if the exception should propagate from SlackMessageHandler, or verify SlackExceptionHandler handling/metrics calls if the handler is expected to catch it. Ensure the test references SlackMessageHandler and webhookRestClient.post to locate the code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@connector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackMessageHandlerTest.java`:
- Around line 35-49: Add a unit test covering the case where
webhookRestClient.post(...) throws a runtime client exception: mock
connectorConfig.isUseBetaTemplatesEnabled(...) and
templateService.renderTemplate(...) as in existing tests, then stub
webhookRestClient.post(anyString(), anyString()) to throw a ProcessingException
(or the specific client exception used), call handler.handle(...) with a
constructed CloudEvent payload (use buildPayload/buildIncomingCloudEvent helpers
used in this test class), and assert the expected behavior—either
assertThrows(ProcessingException.class, ...) if the exception should propagate
from SlackMessageHandler, or verify SlackExceptionHandler handling/metrics calls
if the handler is expected to catch it. Ensure the test references
SlackMessageHandler and webhookRestClient.post to locate the code under test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb39283f-70ab-4452-8376-fa4daa8090a0
📒 Files selected for processing (16)
connector-slack/pom.xmlconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/CamelNotification.javaconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/ExchangeProperty.javaconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackCloudEventDataExtractor.javaconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackExceptionHandler.javaconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackMessageHandler.javaconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackNotification.javaconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackRestClient.javaconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackRouteBuilder.javaconnector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/config/SlackConnectorConfig.javaconnector-slack/src/main/resources/application.propertiesconnector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackConnectorIntegrationTest.javaconnector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackConnectorRoutesMessageBlocksFormatTest.javaconnector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackConnectorRoutesTest.javaconnector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackMessageHandlerTest.javaconnector-slack/src/test/resources/application.properties
💤 Files with no reviewable changes (6)
- connector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/ExchangeProperty.java
- connector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/CamelNotification.java
- connector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackRouteBuilder.java
- connector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackCloudEventDataExtractor.java
- connector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackConnectorRoutesTest.java
- connector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackConnectorRoutesMessageBlocksFormatTest.java
✅ Files skipped from review due to trivial changes (3)
- connector-slack/src/test/resources/application.properties
- connector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/config/SlackConnectorConfig.java
- connector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackNotification.java
🚧 Files skipped from review as they are similar to previous changes (5)
- connector-slack/src/main/resources/application.properties
- connector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackRestClient.java
- connector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackExceptionHandler.java
- connector-slack/src/test/java/com/redhat/cloud/notifications/connector/slack/SlackConnectorIntegrationTest.java
- connector-slack/src/main/java/com/redhat/cloud/notifications/connector/slack/SlackMessageHandler.java
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Remove Camel from Slack connector