RHCLOUD-46150 - Event table normalization#4289
Open
bonscji1 wants to merge 3 commits intoRedHatInsights:masterfrom
Open
RHCLOUD-46150 - Event table normalization#4289bonscji1 wants to merge 3 commits intoRedHatInsights:masterfrom
bonscji1 wants to merge 3 commits intoRedHatInsights:masterfrom
Conversation
added 2 commits
April 1, 2026 16:51
…ring tests update Assisted-by: Claude Sonnet 4.5 (via Claude Code)
Assisted-by: Claude Sonnet 4.5 (via Claude Code)
Contributor
📝 WalkthroughWalkthroughAdds a "normalized-queries" Unleash toggle in backend and engine configs; repositories conditionally switch between normalized JOIN-based and denormalized field-based HQL; model sort maps made mode-selectable; DB indexes added to support normalized patterns; tests parameterized to exercise both modes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 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)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
SC Environment Impact AssessmentOverall Impact: 🟢 LOW View full reportSummary
Detailed Findings🟢 LOW ImpactFeature flag change detected
Feature flag 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java (1)
364-373:⚠️ Potential issue | 🟠 MajorInvalid date format in cleanup query.
The date literal
'2025-06-20:00:00:00'uses an incorrect format. PostgreSQL expects'2025-06-20 00:00:00'(space, not colon, between date and time portions). This will cause a runtime error when executed.Additionally, hardcoded application IDs and dates suggest this may be a one-off cleanup task that might be better suited for a migration script rather than application code.
🐛 Fix the date format
public void cleanupInventoryEvents(int limit) { String deleteQuery = "delete from event WHERE id in " + - "(select id from event where application_id = '332d6b96-5e91-439d-8345-452acac9a722' AND created > '2025-06-20:00:00:00' limit :limit)"; + "(select id from event where application_id = '332d6b96-5e91-439d-8345-452acac9a722' AND created > '2025-06-20 00:00:00' limit :limit)"; entityManager.createNativeQuery(deleteQuery) .setParameter("limit", limit) .executeUpdate(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java` around lines 364 - 373, The native SQL in cleanupInventoryEvents uses an invalid timestamp literal; update the deleteQuery string in the cleanupInventoryEvents method so the timestamp is formatted as '2025-06-20 00:00:00' (space between date and time) and remove or parameterize the hardcoded application ID and date (pass them as method parameters or bind variables via entityManager.createNativeQuery(...).setParameter(...)) so the query uses placeholders instead of fixed values; keep the call to entityManager.createNativeQuery(...).setParameter("limit", limit).executeUpdate() but add parameters for application_id and cutoff timestamp or move this logic to a migration script as appropriate.
🧹 Nitpick comments (4)
engine/src/test/java/com/redhat/cloud/notifications/db/repositories/EventRepositoryTest.java (1)
109-121: Parity coverage is still weak on projected name fields.Great to run both toggle modes, but current assertions mainly validate counts/date windows. They won’t catch mismatches in
bundle/application/eventTypedisplay-name projections between branches.Also applies to: 128-222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/test/java/com/redhat/cloud/notifications/db/repositories/EventRepositoryTest.java` around lines 109 - 121, The test testGetAll toggles normalized queries but only asserts counts and created timestamps; add explicit equality checks for the projected display-name fields so parity bugs are caught: after calling eventRepository.findEventsToExport(DEFAULT_ORG_ID, null, null) compare each returned Event to the corresponding createdEvents entry (match by id or created timestamp) and assert that bundle display name, application display name, and eventType display name (the projected name fields referenced as bundle/application/eventType) are equal between the createdEvents and result; keep this under the same parameterized test (useNormalizedQueries) so both branches validate the projection values.database/src/main/resources/db/migration/V1.130.0__RHCLOUD-46150_add_indexes_for_normalized_event_queries.sql (1)
1-7: Consider usingCREATE INDEX CONCURRENTLYfor production deployments on write-hot tables.Non-concurrent index creation acquires strong locks that block writes on the
event,applications, andevent_typetables during the index build phase. While this pattern is consistent across the codebase (noCONCURRENTLYsyntax is used in any migration), it poses a risk for hot tables during production deployments.Recommend reviewing whether index creation should use
CONCURRENTmode to allow continued write operations, especially for the multi-column indexes on frequently-accessed tables. This would require ensuring migrations are not wrapped in explicit transactions (configureexecuteInTransaction=falsein Flyway if needed).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/src/main/resources/db/migration/V1.130.0__RHCLOUD-46150_add_indexes_for_normalized_event_queries.sql` around lines 1 - 7, This migration creates several indexes without CONCURRENTLY which will block writes on hot tables; update the CREATE INDEX statements for ix_event_event_type_id, ix_applications_bundle_id, ix_event_type_application_id, ix_bundles_display_name, ix_applications_display_name, ix_event_type_display_name, and ix_event_bundle_id_covering to use CREATE INDEX CONCURRENTLY and ensure the Flyway migration is configured to run without a transaction (executeInTransaction=false) so PostgreSQL can apply concurrent index builds; verify the multi-column covering index ix_event_bundle_id_covering is safe for concurrent creation and adjust ordering or locking if necessary.backend/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java (1)
269-275: Consider using the joined aliasetfor consistency.In the normalized path,
e.eventType.displayNametraverses the relationship implicitly, while an explicitJOIN e.eventType etis already added elsewhere wheneventTypeNameNotEmptyis true. Usinget.displayNameinstead would be more consistent and may help the query optimizer.♻️ Suggested improvement
if (eventTypeNameNotEmpty) { if (useNormalized) { - hql += " AND LOWER(e.eventType.displayName) LIKE :eventTypeDisplayName"; + hql += " AND LOWER(et.displayName) LIKE :eventTypeDisplayName"; } else { hql += " AND LOWER(e.eventTypeDisplayName) LIKE :eventTypeDisplayName"; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java` around lines 269 - 275, The HQL uses e.eventType.displayName when useNormalized is true which implicitly traverses the relationship while a JOIN e.eventType et is already added when eventTypeNameNotEmpty is true; change the condition building to reference the joined alias et.displayName instead of e.eventType.displayName so the query consistently uses the explicit join (look for variables eventTypeNameNotEmpty, useNormalized and the hql string construction in EventRepository.java and replace the implicit path with et.displayName).backend/src/main/java/com/redhat/cloud/notifications/db/repositories/DrawerNotificationRepository.java (1)
140-144: Index optimization lost in normalized mode - verify query performance.The
dn.event.orgId = :orgIdcondition is intentionally skipped in normalized mode, which means the indexix_event_org_id_bundle_id_application_id_event_type_display_namwon't be utilized. Security is maintained viadn.id.orgId = :orgIdin the base WHERE clause, but query performance may differ.Consider monitoring query execution plans after enabling normalized mode to ensure acceptable performance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/redhat/cloud/notifications/db/repositories/DrawerNotificationRepository.java` around lines 140 - 144, The query omits the condition dn.event.orgId = :orgId when useNormalized is true, which prevents use of the ix_event_org_id_bundle_id_application_id_event_type_display_nam index; update the query building in DrawerNotificationRepository so that the orgId filter on the event table (dn.event.orgId = :orgId) is applied regardless of useNormalized (or add a conditional branch that applies it when performance tests show benefit), keep the existing dn.id.orgId = :orgId for security, and then verify via EXPLAIN/EXPLAIN ANALYZE that the index is being used and performance is acceptable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@engine/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java`:
- Around line 46-74: The normalized branch in the findEventsQuery uses joined
entity displayName fields (bundle.displayName, app.displayName, et.displayName)
which breaks parity with the denormalized branch that uses the event snapshot
fields (e.bundleDisplayName, e.applicationDisplayName, e.eventTypeDisplayName);
update the normalized branch so its SELECT NEW projection uses the event
snapshot fields (e.bundleDisplayName, e.applicationDisplayName,
e.eventTypeDisplayName) instead of the joined entity fields to ensure parity
across engineConfig.isNormalizedQueriesEnabled(orgId), keeping the same Event
constructor signature used in the denormalized branch.
---
Outside diff comments:
In
`@backend/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java`:
- Around line 364-373: The native SQL in cleanupInventoryEvents uses an invalid
timestamp literal; update the deleteQuery string in the cleanupInventoryEvents
method so the timestamp is formatted as '2025-06-20 00:00:00' (space between
date and time) and remove or parameterize the hardcoded application ID and date
(pass them as method parameters or bind variables via
entityManager.createNativeQuery(...).setParameter(...)) so the query uses
placeholders instead of fixed values; keep the call to
entityManager.createNativeQuery(...).setParameter("limit",
limit).executeUpdate() but add parameters for application_id and cutoff
timestamp or move this logic to a migration script as appropriate.
---
Nitpick comments:
In
`@backend/src/main/java/com/redhat/cloud/notifications/db/repositories/DrawerNotificationRepository.java`:
- Around line 140-144: The query omits the condition dn.event.orgId = :orgId
when useNormalized is true, which prevents use of the
ix_event_org_id_bundle_id_application_id_event_type_display_nam index; update
the query building in DrawerNotificationRepository so that the orgId filter on
the event table (dn.event.orgId = :orgId) is applied regardless of useNormalized
(or add a conditional branch that applies it when performance tests show
benefit), keep the existing dn.id.orgId = :orgId for security, and then verify
via EXPLAIN/EXPLAIN ANALYZE that the index is being used and performance is
acceptable.
In
`@backend/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java`:
- Around line 269-275: The HQL uses e.eventType.displayName when useNormalized
is true which implicitly traverses the relationship while a JOIN e.eventType et
is already added when eventTypeNameNotEmpty is true; change the condition
building to reference the joined alias et.displayName instead of
e.eventType.displayName so the query consistently uses the explicit join (look
for variables eventTypeNameNotEmpty, useNormalized and the hql string
construction in EventRepository.java and replace the implicit path with
et.displayName).
In
`@database/src/main/resources/db/migration/V1.130.0__RHCLOUD-46150_add_indexes_for_normalized_event_queries.sql`:
- Around line 1-7: This migration creates several indexes without CONCURRENTLY
which will block writes on hot tables; update the CREATE INDEX statements for
ix_event_event_type_id, ix_applications_bundle_id, ix_event_type_application_id,
ix_bundles_display_name, ix_applications_display_name,
ix_event_type_display_name, and ix_event_bundle_id_covering to use CREATE INDEX
CONCURRENTLY and ensure the Flyway migration is configured to run without a
transaction (executeInTransaction=false) so PostgreSQL can apply concurrent
index builds; verify the multi-column covering index ix_event_bundle_id_covering
is safe for concurrent creation and adjust ordering or locking if necessary.
In
`@engine/src/test/java/com/redhat/cloud/notifications/db/repositories/EventRepositoryTest.java`:
- Around line 109-121: The test testGetAll toggles normalized queries but only
asserts counts and created timestamps; add explicit equality checks for the
projected display-name fields so parity bugs are caught: after calling
eventRepository.findEventsToExport(DEFAULT_ORG_ID, null, null) compare each
returned Event to the corresponding createdEvents entry (match by id or created
timestamp) and assert that bundle display name, application display name, and
eventType display name (the projected name fields referenced as
bundle/application/eventType) are equal between the createdEvents and result;
keep this under the same parameterized test (useNormalizedQueries) so both
branches validate the projection values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 975dc69a-a3bd-4d3d-ba36-0a451344ce89
📒 Files selected for processing (11)
backend/src/main/java/com/redhat/cloud/notifications/config/BackendConfig.javabackend/src/main/java/com/redhat/cloud/notifications/db/repositories/DrawerNotificationRepository.javabackend/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.javabackend/src/test/java/com/redhat/cloud/notifications/routers/handlers/drawer/DrawerResourceTest.javabackend/src/test/java/com/redhat/cloud/notifications/routers/handlers/event/EventResourceTest.javacommon/src/main/java/com/redhat/cloud/notifications/models/DrawerNotification.javacommon/src/main/java/com/redhat/cloud/notifications/models/Event.javadatabase/src/main/resources/db/migration/V1.130.0__RHCLOUD-46150_add_indexes_for_normalized_event_queries.sqlengine/src/main/java/com/redhat/cloud/notifications/config/EngineConfig.javaengine/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.javaengine/src/test/java/com/redhat/cloud/notifications/db/repositories/EventRepositoryTest.java
Comment on lines
+46
to
+74
| if (engineConfig.isNormalizedQueriesEnabled(orgId)) { | ||
| findEventsQuery.append( | ||
| "SELECT NEW com.redhat.cloud.notifications.models.Event( " + | ||
| "e.id, " + | ||
| "bundle.displayName, " + | ||
| "app.displayName, " + | ||
| "et.displayName, " + | ||
| "e.created) " + | ||
| "FROM " + | ||
| "Event AS e " + | ||
| "JOIN e.eventType et " + | ||
| "JOIN et.application app " + | ||
| "JOIN app.bundle bundle " + | ||
| "WHERE " + | ||
| "e.orgId = :orgId" | ||
| ); | ||
| } else { | ||
| findEventsQuery.append( | ||
| "SELECT NEW com.redhat.cloud.notifications.models.Event( " + | ||
| "e.id, " + | ||
| "e.bundleDisplayName, " + | ||
| "e.applicationDisplayName, " + | ||
| "e.eventTypeDisplayName, " + | ||
| "e.created) " + | ||
| "FROM " + | ||
| "Event AS e " + | ||
| "WHERE " + | ||
| "e.orgId = :orgId" | ||
| ); |
Contributor
There was a problem hiding this comment.
Normalized projection can break denormalized parity for exported names.
The normalized branch reads display names from joined entities, while denormalized mode reads the event snapshot fields. That can return different values (notably for eventTypeDisplayName, given updateEventDisplayName updates event rows). This violates parity expectations across toggle modes.
💡 Suggested parity-safe projection change
- "bundle.displayName, " +
- "app.displayName, " +
- "et.displayName, " +
+ "e.bundleDisplayName, " +
+ "e.applicationDisplayName, " +
+ "e.eventTypeDisplayName, " +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@engine/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java`
around lines 46 - 74, The normalized branch in the findEventsQuery uses joined
entity displayName fields (bundle.displayName, app.displayName, et.displayName)
which breaks parity with the denormalized branch that uses the event snapshot
fields (e.bundleDisplayName, e.applicationDisplayName, e.eventTypeDisplayName);
update the normalized branch so its SELECT NEW projection uses the event
snapshot fields (e.bundleDisplayName, e.applicationDisplayName,
e.eventTypeDisplayName) instead of the joined entity fields to ensure parity
across engineConfig.isNormalizedQueriesEnabled(orgId), keeping the same Event
constructor signature used in the denormalized branch.
g-duval
reviewed
Apr 7, 2026
Contributor
g-duval
left a comment
g-duval
left a comment
There was a problem hiding this comment.
Great job!
I think we should push the event log queries refactoring a little bit more 😉
| boolean needsEventType = needsApp || sortColumn.startsWith("et."); | ||
|
|
||
| // Add selective JOINs (order matters - FK chain: e → et → app → bundle) | ||
| if (needsEventType) { |
Contributor
There was a problem hiding this comment.
Don't we need to always join those tables to fetch display names?
Contributor
Author
There was a problem hiding this comment.
That is probably my mistake, i used this just for sorting and since you cannot sort on null bundle, not joining mate sense. But as you pointed in the next comment, we need to create event with display names and that will need the joins, even if sorting is not provided. I will fix it.
| if (useNormalized) { | ||
| // Remove DISTINCT to allow ORDER BY with joined columns | ||
| // Deduplication happens naturally since we're fetching by specific event IDs | ||
| hql = "SELECT e FROM Event e " + joinClause + "LEFT JOIN FETCH e.historyEntries he WHERE e.id IN (:eventIds)"; |
Contributor
There was a problem hiding this comment.
Using normalized structure, we need to fetch display names from bundle, application and event type tables.
Then create an Event from scratch, as it is for Export service (engine).
Contributor
Author
There was a problem hiding this comment.
My mistake, since i got the correct events but i forgot to account for needing to adjust the data. I would really like to have EventDTO here, since that is basically what we will be doing. I will fix this.
Contributor
Author
There was a problem hiding this comment.
This should now be fixed in getEventsWithCriterion and getEvents methods.
| if (fetchNotificationHistory) { | ||
| hql = "SELECT DISTINCT e FROM Event e LEFT JOIN FETCH e.historyEntries he WHERE e.id IN (:eventIds)"; | ||
| if (useNormalized) { | ||
| // Remove DISTINCT to allow ORDER BY with joined columns |
Contributor
There was a problem hiding this comment.
Seems weird to me but I'm not a SQL expert.
Did you experimented this locally?
Contributor
Author
There was a problem hiding this comment.
I changed tests to use both approaches to validate parity and this was failing for normalized approach due to distinct with specific postgres error, that is why i changed it to this.
Assisted-by: Claude Sonnet 4.5 (via Claude Code)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java (1)
266-272: Inconsistent alias usage for eventType filter.Lines 249 and 256 use the defined aliases (
bundle.id,app.id) for filtering, but line 268 uses path navigatione.eventType.displayNameinstead of the aliaset.displayName. Using the alias consistently improves clarity and ensures the existing join is utilized.♻️ Proposed fix for consistency
if (eventTypeNameNotEmpty) { if (useNormalized) { - hql += " AND LOWER(e.eventType.displayName) LIKE :eventTypeDisplayName"; + hql += " AND LOWER(et.displayName) LIKE :eventTypeDisplayName"; } else { hql += " AND LOWER(e.eventTypeDisplayName) LIKE :eventTypeDisplayName"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java` around lines 266 - 272, The eventType filter uses path navigation (e.eventType.displayName) instead of the existing join alias (et.displayName); update the conditional that appends the HQL when eventTypeNameNotEmpty (and honoring useNormalized) to reference et.displayName rather than e.eventType.displayName so the existing join on et is used consistently (check the code around eventTypeNameNotEmpty, useNormalized, and the hql string assembly where e.eventType.displayName appears).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java`:
- Around line 53-74: The HQL builds regular JOINs which leave Event.eventType
and its nested associations lazy, causing N+1 loads when you call
event.getEventType().getApplication().getBundle() in the loop; change the join
clause in the method that builds the query (the branch where useNormalized is
true) to use JOIN FETCH for these associations (replace "JOIN e.eventType et
JOIN et.application app JOIN app.bundle bundle" with "JOIN FETCH e.eventType et
JOIN FETCH et.application app JOIN FETCH app.bundle bundle") so the eventType,
application and bundle are eagerly fetched in the same query and no per-event
selects occur.
- Around line 98-139: The normalized branch uses a non-fetching joinClause which
causes N+1 when later accessing lazy associations; update the joinClause used
when useNormalized is true to eager-fetch the related entities (e.g., change
"JOIN e.eventType et JOIN et.application app JOIN app.bundle bundle " to use
JOIN FETCH for those associations: "JOIN FETCH e.eventType et JOIN FETCH
et.application app JOIN FETCH app.bundle bundle "), keeping the existing LEFT
JOIN FETCH e.historyEntries he when fetchNotificationHistory is true and
preserving the deduplication logic (events.stream().distinct()) for the
one-to-many fetch case; ensure these changes are applied only in the
useNormalized path so Event.setBundleDisplayName / setApplicationDisplayName /
setEventTypeDisplayName no longer trigger extra queries.
---
Nitpick comments:
In
`@backend/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java`:
- Around line 266-272: The eventType filter uses path navigation
(e.eventType.displayName) instead of the existing join alias (et.displayName);
update the conditional that appends the HQL when eventTypeNameNotEmpty (and
honoring useNormalized) to reference et.displayName rather than
e.eventType.displayName so the existing join on et is used consistently (check
the code around eventTypeNameNotEmpty, useNormalized, and the hql string
assembly where e.eventType.displayName appears).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aeee5d96-cf9f-4e63-b658-cc2bcac4c851
📒 Files selected for processing (1)
backend/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java
Comment on lines
+53
to
+74
| if (useNormalized) { | ||
| hql += "JOIN e.eventType et JOIN et.application app JOIN app.bundle bundle "; | ||
| } | ||
|
|
||
| hql += "WHERE e.orgId = :orgId"; | ||
|
|
||
| hql = addHqlConditions(hql, bundleIds, appIds, eventTypeDisplayName, startDate, endDate, endpointTypes, compositeEndpointTypes, invocationResults, status, null, Optional.empty(), true); | ||
| hql = addHqlConditions(hql, useNormalized, bundlesNotEmpty, applicationsNotEmpty, eventTypeNameNotEmpty, startDate, endDate, endpointTypes, compositeEndpointTypes, invocationResults, status, null, Optional.empty(), true); | ||
| // we are looking for events with auth criterion only | ||
| hql += " AND e.hasAuthorizationCriterion is true"; | ||
|
|
||
| TypedQuery<Event> typedQuery = entityManager.createQuery(hql, Event.class); | ||
| setQueryParams(typedQuery, orgId, bundleIds, appIds, eventTypeDisplayName, startDate, endDate, endpointTypes, compositeEndpointTypes, invocationResults, status, null, Optional.empty()); | ||
|
|
||
| List<Event> eventsWithAuthorizationCriterion = typedQuery.getResultList(); | ||
|
|
||
| // Populate denormalized display name fields from joined entities | ||
| if (useNormalized && !eventsWithAuthorizationCriterion.isEmpty()) { | ||
| for (Event event : eventsWithAuthorizationCriterion) { | ||
| event.setBundleDisplayName(event.getEventType().getApplication().getBundle().getDisplayName()); | ||
| event.setApplicationDisplayName(event.getEventType().getApplication().getDisplayName()); | ||
| event.setEventTypeDisplayName(event.getEventType().getDisplayName()); | ||
| } |
Contributor
There was a problem hiding this comment.
N+1 query risk due to missing JOIN FETCH.
The query uses regular JOIN but the loop at lines 70-74 navigates through lazy associations (event.getEventType().getApplication().getBundle()). Since Event.eventType is mapped with fetch = LAZY, each call to getEventType() may trigger a separate SELECT query, causing N+1 queries.
Use JOIN FETCH to eagerly initialize the associations:
⚡ Proposed fix to avoid N+1 queries
// Add selective JOINs for normalized approach - only join what we need
if (useNormalized) {
- hql += "JOIN e.eventType et JOIN et.application app JOIN app.bundle bundle ";
+ hql += "JOIN FETCH e.eventType et JOIN FETCH et.application app JOIN FETCH app.bundle bundle ";
}📝 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
| if (useNormalized) { | |
| hql += "JOIN e.eventType et JOIN et.application app JOIN app.bundle bundle "; | |
| } | |
| hql += "WHERE e.orgId = :orgId"; | |
| hql = addHqlConditions(hql, bundleIds, appIds, eventTypeDisplayName, startDate, endDate, endpointTypes, compositeEndpointTypes, invocationResults, status, null, Optional.empty(), true); | |
| hql = addHqlConditions(hql, useNormalized, bundlesNotEmpty, applicationsNotEmpty, eventTypeNameNotEmpty, startDate, endDate, endpointTypes, compositeEndpointTypes, invocationResults, status, null, Optional.empty(), true); | |
| // we are looking for events with auth criterion only | |
| hql += " AND e.hasAuthorizationCriterion is true"; | |
| TypedQuery<Event> typedQuery = entityManager.createQuery(hql, Event.class); | |
| setQueryParams(typedQuery, orgId, bundleIds, appIds, eventTypeDisplayName, startDate, endDate, endpointTypes, compositeEndpointTypes, invocationResults, status, null, Optional.empty()); | |
| List<Event> eventsWithAuthorizationCriterion = typedQuery.getResultList(); | |
| // Populate denormalized display name fields from joined entities | |
| if (useNormalized && !eventsWithAuthorizationCriterion.isEmpty()) { | |
| for (Event event : eventsWithAuthorizationCriterion) { | |
| event.setBundleDisplayName(event.getEventType().getApplication().getBundle().getDisplayName()); | |
| event.setApplicationDisplayName(event.getEventType().getApplication().getDisplayName()); | |
| event.setEventTypeDisplayName(event.getEventType().getDisplayName()); | |
| } | |
| if (useNormalized) { | |
| hql += "JOIN FETCH e.eventType et JOIN FETCH et.application app JOIN FETCH app.bundle bundle "; | |
| } | |
| hql += "WHERE e.orgId = :orgId"; | |
| hql = addHqlConditions(hql, useNormalized, bundlesNotEmpty, applicationsNotEmpty, eventTypeNameNotEmpty, startDate, endDate, endpointTypes, compositeEndpointTypes, invocationResults, status, null, Optional.empty(), true); | |
| // we are looking for events with auth criterion only | |
| hql += " AND e.hasAuthorizationCriterion is true"; | |
| TypedQuery<Event> typedQuery = entityManager.createQuery(hql, Event.class); | |
| setQueryParams(typedQuery, orgId, bundleIds, appIds, eventTypeDisplayName, startDate, endDate, endpointTypes, compositeEndpointTypes, invocationResults, status, null, Optional.empty()); | |
| List<Event> eventsWithAuthorizationCriterion = typedQuery.getResultList(); | |
| // Populate denormalized display name fields from joined entities | |
| if (useNormalized && !eventsWithAuthorizationCriterion.isEmpty()) { | |
| for (Event event : eventsWithAuthorizationCriterion) { | |
| event.setBundleDisplayName(event.getEventType().getApplication().getBundle().getDisplayName()); | |
| event.setApplicationDisplayName(event.getEventType().getApplication().getDisplayName()); | |
| event.setEventTypeDisplayName(event.getEventType().getDisplayName()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java`
around lines 53 - 74, The HQL builds regular JOINs which leave Event.eventType
and its nested associations lazy, causing N+1 loads when you call
event.getEventType().getApplication().getBundle() in the loop; change the join
clause in the method that builds the query (the branch where useNormalized is
true) to use JOIN FETCH for these associations (replace "JOIN e.eventType et
JOIN et.application app JOIN app.bundle bundle" with "JOIN FETCH e.eventType et
JOIN FETCH et.application app JOIN FETCH app.bundle bundle") so the eventType,
application and bundle are eagerly fetched in the same query and no per-event
selects occur.
Comment on lines
+98
to
+139
| if (useNormalized) { | ||
| String joinClause = "JOIN e.eventType et JOIN et.application app JOIN app.bundle bundle "; | ||
|
|
||
| if (fetchNotificationHistory) { | ||
| // Remove DISTINCT to allow ORDER BY with joined columns | ||
| hql = "SELECT e FROM Event e " + joinClause + "LEFT JOIN FETCH e.historyEntries he WHERE e.id IN (:eventIds)"; | ||
| } else { | ||
| hql = "FROM Event e " + joinClause + "WHERE e.id IN (:eventIds)"; | ||
| } | ||
|
|
||
| } else { | ||
| hql = "FROM Event e WHERE e.id IN (:eventIds)"; | ||
| if (fetchNotificationHistory) { | ||
| hql = "SELECT DISTINCT e FROM Event e LEFT JOIN FETCH e.historyEntries he WHERE e.id IN (:eventIds)"; | ||
| } else { | ||
| hql = "FROM Event e WHERE e.id IN (:eventIds)"; | ||
| } | ||
| } | ||
|
|
||
| if (sort.isPresent()) { | ||
| hql += getOrderBy(sort.get()); | ||
| } | ||
|
|
||
| return entityManager.createQuery(hql, Event.class) | ||
| List<Event> events = entityManager.createQuery(hql, Event.class) | ||
| .setParameter("eventIds", eventIds) | ||
| .getResultList(); | ||
|
|
||
| if (useNormalized && !events.isEmpty()) { | ||
| // LEFT JOIN FETCH on one-to-many can create duplicate Event objects | ||
| // Only deduplicate for normalized queries (denormalized already has SQL DISTINCT) | ||
| if (fetchNotificationHistory) { | ||
| events = events.stream() | ||
| .distinct() | ||
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| // Populate denormalized display name fields from joined entities | ||
| for (Event event : events) { | ||
| event.setBundleDisplayName(event.getEventType().getApplication().getBundle().getDisplayName()); | ||
| event.setApplicationDisplayName(event.getEventType().getApplication().getDisplayName()); | ||
| event.setEventTypeDisplayName(event.getEventType().getDisplayName()); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Same N+1 query risk with regular JOIN for normalized path.
The joinClause at line 99 uses regular JOIN but lines 134-138 navigate through lazy associations to populate display names. This will trigger additional queries per event.
⚡ Proposed fix
if (useNormalized) {
- String joinClause = "JOIN e.eventType et JOIN et.application app JOIN app.bundle bundle ";
+ String joinClause = "JOIN FETCH e.eventType et JOIN FETCH et.application app JOIN FETCH app.bundle bundle ";Note: With JOIN FETCH, the deduplication via stream().distinct() at lines 128-130 remains necessary since the LEFT JOIN FETCH e.historyEntries one-to-many relationship can still produce duplicate Event rows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java`
around lines 98 - 139, The normalized branch uses a non-fetching joinClause
which causes N+1 when later accessing lazy associations; update the joinClause
used when useNormalized is true to eager-fetch the related entities (e.g.,
change "JOIN e.eventType et JOIN et.application app JOIN app.bundle bundle " to
use JOIN FETCH for those associations: "JOIN FETCH e.eventType et JOIN FETCH
et.application app JOIN FETCH app.bundle bundle "), keeping the existing LEFT
JOIN FETCH e.historyEntries he when fetchNotificationHistory is true and
preserving the deduplication logic (events.stream().distinct()) for the
one-to-many fetch case; ensure these changes are applied only in the
useNormalized path so Event.setBundleDisplayName / setApplicationDisplayName /
setEventTypeDisplayName no longer trigger extra queries.
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
2 participants
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.
Summary by CodeRabbit
New Features
Database
Tests