[RHCLOUD-43623] Fix event external Id management #3872
Conversation
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
Contributor
Reviewer's GuideAdds support for persisting and exposing an external UUID identifier for Events, wiring it from Kafka message IDs into the Event model, database, and event log API, and updating tests accordingly. Sequence diagram for Kafka message to Event persistence with externalIdsequenceDiagram
actor Producer
participant Kafka
participant EventConsumer
participant Event
participant EventRepository
participant Database
Producer->>Kafka: Send message(payload, messageId)
Kafka->>EventConsumer: Deliver message(payload, messageId)
EventConsumer->>Event: new Event()
EventConsumer->>Event: setExternalId(messageId)
EventConsumer->>Event: setId(UUID.randomUUID())
EventConsumer->>Event: setHasAuthorizationCriterion(extract(event))
EventConsumer->>EventRepository: create(event)
EventRepository->>Database: INSERT Event(id, external_id, ...)
Database-->>EventRepository: Insert OK
EventRepository-->>EventConsumer: Persisted Event
EventConsumer-->>Kafka: Ack message
Sequence diagram for exposing externalId via EventResourcesequenceDiagram
actor User
participant EventResource
participant EventRepository
participant Database
participant EventLogEntry
User->>EventResource: GET /events
EventResource->>EventRepository: findEvents(criteria)
EventRepository->>Database: SELECT * FROM event
Database-->>EventRepository: Event rows
EventRepository-->>EventResource: List of Event
loop For each Event
EventResource->>EventLogEntry: new EventLogEntry()
EventResource->>EventLogEntry: setId(event.id)
EventResource->>EventLogEntry: setExternalId(event.externalId)
EventResource->>EventLogEntry: setCreated(event.created)
end
EventResource-->>User: Page of EventLogEntry (includes externalId)
ER diagram for updated event table with external_iderDiagram
EVENT {
uuid id PK
uuid external_id
timestamp created
text payload
uuid event_type_id
}
EVENT ||--o{ EVENT_TYPE : has_type
Class diagram for updated Event and EventLogEntry modelsclassDiagram
class Event {
+UUID id
+UUID externalId
+OffsetDateTime created
+String payload
+EventType eventType
+EventWrapper eventWrapper
+void setId(UUID id)
+UUID getId()
+void setExternalId(UUID externalId)
+UUID getExternalId()
+LocalDateTime getCreated()
}
class EventLogEntry {
+UUID id
+UUID externalId
+LocalDateTime created
+String bundle
+String application
+String eventTypeDisplayName
+String accountId
+String orgId
+List~EventLogEntryAction~ actions
+UUID getExternalId()
+void setExternalId(UUID externalId)
}
class EventLogEntryAction {
+UUID id
+String status
+String endpointType
+String details
}
EventLogEntry --> "*" EventLogEntryAction
Class diagram for updated EventConsumer process logicclassDiagram
class EventConsumer {
+void process(Message message)
}
class Message {
+String payload
+UUID id
}
class EventRepository {
+void create(Event event)
}
class RecipientsAuthorizationCriterionExtractor {
+Object extract(Event event)
}
class Event {
+UUID id
+UUID externalId
+void setId(UUID id)
+UUID getId()
+void setExternalId(UUID externalId)
+UUID getExternalId()
+void setHasAuthorizationCriterion(boolean hasCriterion)
}
EventConsumer --> Message
EventConsumer --> EventRepository
EventConsumer --> RecipientsAuthorizationCriterionExtractor
EventConsumer --> Event
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
EventConsumer.process, consider only settingexternalIdwhen amessageIdis actually present to avoid storing meaningless null external IDs and to leave room for potential producer-specified external IDs in the future. - The new
externalIdfield is exposed onEventLogEntryand will be serialized even when null; if clients don’t need to see a null field, consider annotating it with@JsonInclude(JsonInclude.Include.NON_NULL)(or equivalent configuration) to avoid noisy responses.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `EventConsumer.process`, consider only setting `externalId` when a `messageId` is actually present to avoid storing meaningless null external IDs and to leave room for potential producer-specified external IDs in the future.
- The new `externalId` field is exposed on `EventLogEntry` and will be serialized even when null; if clients don’t need to see a null field, consider annotating it with `@JsonInclude(JsonInclude.Include.NON_NULL)` (or equivalent configuration) to avoid noisy responses.
## Individual Comments
### Comment 1
<location> `common/src/main/java/com/redhat/cloud/notifications/models/Event.java:90-94` </location>
<code_context>
@NotNull
private UUID id;
+ private UUID externalId;
+
@NotNull
</code_context>
<issue_to_address>
**issue (bug_risk):** Align JPA column name for `externalId` with the DB migration to avoid mapping issues.
The migration adds a column `external_id`, but this field will rely on the default JPA naming strategy. If `externalId` is not automatically mapped to `external_id` in the current config, it may not be persisted/loaded correctly. Please explicitly map it, e.g. `@Column(name = "external_id")`, to ensure it matches the new schema.
</issue_to_address>
### Comment 2
<location> `engine/src/main/java/com/redhat/cloud/notifications/events/EventConsumer.java:300-301` </location>
<code_context>
- // NOTIF-499 If there is no ID provided whatsoever we create one.
- event.setId(Objects.requireNonNullElseGet(messageId, UUID::randomUUID));
- }
+ event.setExternalId(messageId);
+ event.setId(UUID.randomUUID());
+
eventRepository.create(event);
</code_context>
<issue_to_address>
**question (bug_risk):** Revisiting the behavior change where `event.id` is no longer derived from the message ID.
`event.id` is now always a random UUID and `messageId` is only stored in `externalId`, whereas previously `event.id` reused `messageId` when present. This is a behavioral change that may break consumers that depended on `event.id` matching the message ID (e.g., idempotency, correlation, or cross-service references). Please verify all such consumers and either keep the previous `event.id` behavior or update them to use `externalId` instead.
</issue_to_address>
### Comment 3
<location> `engine/src/test/java/com/redhat/cloud/notifications/events/EventConsumerTest.java:167-173` </location>
<code_context>
);
- verifyExactlyOneProcessing(eventType, payload, action, true);
+ final Event processedEvent = verifyExactlyOneProcessing(eventType, payload, action, true);
verifySeverity(action, false);
verify(kafkaMessageDeduplicator, times(1)).isNew(messageId);
// TODO eventDeduplicator will be called once when kafkaMessageDeduplicator is removed.
verify(eventDeduplicator, never()).isNew(any(Event.class));
+ assertEquals(messageId, processedEvent.getExternalId());
+ assertNotEquals(messageId, processedEvent.getId());
}
</code_context>
<issue_to_address>
**suggestion (testing):** Add a complementary test to cover the case where no messageId header is present and verify externalId is null
In that test, also assert that `id` is non-null while `externalId` remains null. This guards against future changes that might unintentionally populate `externalId` when the header is absent.
Suggested implementation:
```java
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.mockito.AdditionalMatchers.or;
import static org.mockito.ArgumentMatchers.any;
```
```java
final Event processedEvent = verifyExactlyOneProcessing(eventType, payload, action, true);
verifySeverity(action, false);
verify(kafkaMessageDeduplicator, times(1)).isNew(messageId);
// TODO eventDeduplicator will be called once when kafkaMessageDeduplicator is removed.
verify(eventDeduplicator, never()).isNew(any(Event.class));
assertEquals(messageId, processedEvent.getExternalId());
assertNotEquals(messageId, processedEvent.getId());
}
@Test
void shouldNotPopulateExternalIdWhenMessageIdHeaderIsAbsent() {
final Event processedEvent = verifyExactlyOneProcessing(eventType, payload, action, true);
// When no messageId header is present, we should not populate externalId,
// but we must still generate a non-null internal id.
assertNotNull(processedEvent.getId());
assertNull(processedEvent.getExternalId());
}
```
This new test assumes that the setup for `verifyExactlyOneProcessing(eventType, payload, action, true)` in this test runs the consumer in a scenario where the Kafka record **does not** include the `messageId` header by default. If that is not currently the case, you will need to:
1. Mirror the setup from the existing “messageId present” test but build a Kafka record (or equivalent input) **without** the `messageId` header.
2. Invoke the same code path as in the existing test to obtain the `Event processedEvent`.
3. Replace the body of `shouldNotPopulateExternalIdWhenMessageIdHeaderIsAbsent` with that setup plus the final assertions on `processedEvent.getId()` (non-null) and `processedEvent.getExternalId()` (null).
</issue_to_address>
### Comment 4
<location> `backend/src/test/java/com/redhat/cloud/notifications/routers/handlers/event/EventResourceTest.java:211-214` </location>
<code_context>
+ createEvent(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, bundle2, app2, eventType2, NOW, PAYLOAD, true, null);
- Event event3 = createEvent(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, bundle2, app2, eventType2, NOW.minusDays(2L));
+ Event event3 = createEvent(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, bundle2, app2, eventType2, NOW.minusDays(2L), PAYLOAD, false, UUID.randomUUID());
Event event4 = createEvent(OTHER_ACCOUNT_ID, OTHER_ORG_ID, bundle2, app2, eventType2, NOW.minusDays(10L));
Endpoint endpoint1 = resourceHelpers.createEndpoint(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, WEBHOOK);
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting the externalId value through the getEvents API response, not only on the in-memory Event entity
Since getEvents now exposes externalId via EventLogEntry and assertSameEvent validates it, please also assert that the externalId returned for event3 matches the value you set. Using a fixed UUID (instead of a random one) will let you verify end‑to‑end propagation of externalId through the router layer in this test.
Suggested implementation:
```java
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
```
```java
// the following event will be ignored because isKesselChecksOnEventLogEnabled is set to false by default
createEvent(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, bundle2, app2, eventType2, NOW, PAYLOAD, true, null);
UUID externalId = UUID.fromString("123e4567-e89b-12d3-a456-426614174000");
Event event3 = createEvent(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, bundle2, app2, eventType2, NOW.minusDays(2L), PAYLOAD, false, externalId);
Event event4 = createEvent(OTHER_ACCOUNT_ID, OTHER_ORG_ID, bundle2, app2, eventType2, NOW.minusDays(10L));
```
```java
assertNull(event2.getExternalId());
assertNotNull(event3.getExternalId());
assertEquals(externalId, event3.getExternalId());
```
To fully implement your comment, you should also:
1. Locate the part of this test method (below the `/* Test #1` comment) where the code invokes the `getEvents` API and validates `event3`, likely via a helper like `assertSameEvent(event3, someEventLogEntry)` or equivalent.
2. After obtaining the `EventLogEntry` corresponding to `event3` (let's call it `eventLogEntry3`), add an explicit assertion to verify end‑to‑end propagation of `externalId`, for example:
```java
assertEquals(externalId, eventLogEntry3.getExternalId());
```
If `assertSameEvent(event3, eventLogEntry3)` already checks `externalId`, you can either:
* Keep the explicit assertion for clarity that you are verifying the specific value you set, or
* Rely on `assertSameEvent` if the team prefers deduplicated assertions.
3. Ensure that the variable name you use for the `EventLogEntry` in this assertion matches whatever is already in the test (e.g., `eventLogEntry`, `resultEvent3`, etc.).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
engine/src/main/java/com/redhat/cloud/notifications/events/EventConsumer.java
Show resolved
Hide resolved
Contributor
Author
|
/retest |
2 similar comments
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
g-duval
force-pushed
the
RHCLOUD-43623_fixExternalIdManagement
branch
from
f681e08 to
85e351d
Compare
g-duval
force-pushed
the
RHCLOUD-43623_fixExternalIdManagement
branch
from
ae343e5 to
ba209fb
Compare
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.
Why?
Summary by Sourcery
Track and expose an immutable external ID for events alongside the internal event ID, and ensure it flows from message consumption through persistence to the event log API.
New Features:
Bug Fixes:
Enhancements: