Kessel inventory client migration - #4728
Merged
g-duval merged 2 commits intoAug 4, 2026
Merged
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change introduces Kessel inventory streaming, a v2 subscription API, paged event exports, admin-console bundle and behavior-group workflows, Lightwell email handling, and updated email templates. ChangesKessel inventory integration
Backend subscription and event APIs
Admin console
Engine and templates
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant RecipientResolver
participant KesselService
participant KesselInventoryClient
participant OAuth2ClientCredentialsCache
participant KesselInventory
RecipientResolver->>KesselService: lookupSubjects(criterion)
KesselService->>KesselInventoryClient: streamedListSubjects(request)
KesselInventoryClient->>OAuth2ClientCredentialsCache: getCredentials()
KesselInventoryClient->>KesselInventory: StreamedListSubjects(request)
KesselInventory-->>KesselInventoryClient: streamed subject responses
KesselInventoryClient-->>KesselService: response iterator
KesselService-->>RecipientResolver: resolved subject IDs
Possibly related PRs
Suggested labels: Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
|
bonscji1
commented
Jul 20, 2026
| // RBAC/MBOP calls (FetchUsersFromExternalServices) per RecipientSettings in the same request/timeout window -- | ||
| // so the safe target is meaningfully less than 30s, not just-under-30s. No real traffic exercises this path yet | ||
| // (use-kessel toggle is off everywhere), so there's no latency data to size against. Needs sizing guidance. | ||
| @Retry(maxRetries = 3, delay = 100, retryOn = KesselTransientException.class) |
Contributor
Author
There was a problem hiding this comment.
This is problematic, i would appreciate any help.
Contributor
There was a problem hiding this comment.
We will revisit it later, once #4539 will be merged.
bonscji1
marked this pull request as ready for review
July 21, 2026 11:29
![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: 4
🧹 Nitpick comments (1)
recipients-resolver/src/test/java/com/redhat/cloud/notifications/recipients/resolver/kessel/KesselServiceTest.java (1)
63-75: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing coverage for mid-stream partial-consumption before failure.
KesselInventoryClient's Javadoc explicitly calls out that aStatusRuntimeException"can surface mid-stream, after some responses were already consumed," and the caller "owns retrying the whole request/iteration from scratch."throwingIteratoralways fails on the very firsthasNext(), so no test exercises the case where one or moreStreamedListSubjectsResponseitems are already added touserIdsbefore the failure — leaving the "discard partial results and retry from scratch" guarantee unverified.A test could combine a first successful
next()with a subsequenthasNext()throw, then assert the retried call returns only the final, complete result set (not a union with the partial one).Also applies to: 106-118
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@recipients-resolver/src/test/java/com/redhat/cloud/notifications/recipients/resolver/kessel/KesselServiceTest.java` around lines 63 - 75, Extend KesselServiceTest coverage for mid-stream failure by adding an iterator/helper variant that returns at least one StreamedListSubjectsResponse successfully, then throws from a subsequent hasNext() call. Add a test around the KesselInventoryClient retry flow that verifies partial userIds collected before the failure are discarded and the retry returns only the complete result set, not a union with the partial results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@recipients-resolver/pom.xml`:
- Around line 35-45: Add version management for the grpc-netty-shaded dependency
in the recipients-resolver Maven configuration: either define its version
directly or add the matching io.grpc BOM entry in the dependencyManagement
section. Keep the existing Kessel SDK dependency and transport dependency
unchanged apart from ensuring grpc-netty-shaded resolves through managed
versioning.
In
`@recipients-resolver/src/main/java/com/redhat/cloud/notifications/recipients/resolver/kessel/KesselService.java`:
- Around line 34-41: Update the `@Retry` annotation on lookupSubjects to set an
explicit maxDuration with durationUnit using the MicroProfile/SmallRye Fault
Tolerance API version pinned by the project. Cap the total retry window below
the callers’ 30-second REST timeout while preserving the existing maxRetries,
delay, and retryOn settings.
- Around line 44-51: Update the resource ID transformation in the KesselService
subject-processing loop to remove kesselAdditionalDomainName only when it
appears at the beginning of the ID, preserving all later occurrences and leaving
non-prefixed IDs unchanged. Avoid using global substring replacement, and retain
the existing userIds collection behavior.
In
`@recipients-resolver/src/main/java/com/redhat/cloud/notifications/recipients/resolver/kessel/OAuth2ClientCredentialsCache.java`:
- Around line 22-31: Update getCredentials in OAuth2ClientCredentialsCache to
validate the issuer, client ID, and client secret optionals before use and throw
descriptive configuration errors identifying the missing property; avoid direct
bare .get() calls while preserving the existing discovery and credential
construction flow.
---
Nitpick comments:
In
`@recipients-resolver/src/test/java/com/redhat/cloud/notifications/recipients/resolver/kessel/KesselServiceTest.java`:
- Around line 63-75: Extend KesselServiceTest coverage for mid-stream failure by
adding an iterator/helper variant that returns at least one
StreamedListSubjectsResponse successfully, then throws from a subsequent
hasNext() call. Add a test around the KesselInventoryClient retry flow that
verifies partial userIds collected before the failure are discarded and the
retry returns only the complete result set, not a union with the partial
results.
🪄 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: Enterprise
Run ID: cce5cf39-2a2e-4c5c-a316-42d013301054
📒 Files selected for processing (12)
.rhcicd/clowdapp-recipients-resolver.yamlpom.xmlrecipients-resolver/pom.xmlrecipients-resolver/src/main/java/com/redhat/cloud/notifications/recipients/config/RecipientsResolverConfig.javarecipients-resolver/src/main/java/com/redhat/cloud/notifications/recipients/resolver/kessel/KesselInventoryClient.javarecipients-resolver/src/main/java/com/redhat/cloud/notifications/recipients/resolver/kessel/KesselService.javarecipients-resolver/src/main/java/com/redhat/cloud/notifications/recipients/resolver/kessel/KesselTransientException.javarecipients-resolver/src/main/java/com/redhat/cloud/notifications/recipients/resolver/kessel/OAuth2ClientCredentialsCache.javarecipients-resolver/src/main/resources/application.propertiesrecipients-resolver/src/test/java/com/redhat/cloud/notifications/recipients/resolver/kessel/KesselInventoryClientOAuth2Test.javarecipients-resolver/src/test/java/com/redhat/cloud/notifications/recipients/resolver/kessel/KesselInventoryClientTest.javarecipients-resolver/src/test/java/com/redhat/cloud/notifications/recipients/resolver/kessel/KesselServiceTest.java
💤 Files with no reviewable changes (1)
- pom.xml
Comment on lines
+34
to
+41
| // worst-case retry budget (4 x kessel.timeout-ms = 120s) exceeds callers' default 30s REST timeout. | ||
| // Confirmed: none of engine/connector-email/connector-drawer override quarkus.rest-client.recipients-resolver's | ||
| // read-timeout, so all three genuinely get Quarkus's 30s default. Also, this budget isn't competing with an | ||
| // otherwise-empty request: RecipientsResolver.findRecipients() calls this lookup first, then still has to run | ||
| // RBAC/MBOP calls (FetchUsersFromExternalServices) per RecipientSettings in the same request/timeout window -- | ||
| // so the safe target is meaningfully less than 30s, not just-under-30s. No real traffic exercises this path yet | ||
| // (use-kessel toggle is off everywhere), so there's no latency data to size against. Needs sizing guidance. | ||
| @Retry(maxRetries = 3, delay = 100, retryOn = KesselTransientException.class) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
Retry budget can exceed the caller's REST timeout — unresolved from prior review.
The developer's own comment confirms worst-case retry duration (~4 × kessel.timeout-ms ≈ 120s) can exceed the default 30s REST client timeout used by engine/connector-email/connector-drawer. This is exactly the concern flagged in the previous review round ("This is problematic, i would appreciate any help.") and remains unaddressed — @Retry has no upper bound on total elapsed time. If this path is enabled without a fix, a slow/unavailable Kessel backend could tie up request threads well past the caller's timeout, risking thread-pool exhaustion under load.
Consider bounding total retry duration with maxDuration/durationUnit on @Retry (MicroProfile Fault Tolerance) so the whole lookupSubjects call is capped below the 30s caller budget, independent of how kessel.timeout-ms is tuned later.
🩹 Suggested fix: cap total retry duration
+import java.time.temporal.ChronoUnit;- `@Retry`(maxRetries = 3, delay = 100, retryOn = KesselTransientException.class)
+ `@Retry`(maxRetries = 3, delay = 100, maxDuration = 25_000, durationUnit = ChronoUnit.MILLIS, retryOn = KesselTransientException.class)Please confirm maxDuration/durationUnit are the correct attribute names for the SmallRye/MicroProfile Fault Tolerance @Retry version pinned in this project before applying.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@recipients-resolver/src/main/java/com/redhat/cloud/notifications/recipients/resolver/kessel/KesselService.java`
around lines 34 - 41, Update the `@Retry` annotation on lookupSubjects to set an
explicit maxDuration with durationUnit using the MicroProfile/SmallRye Fault
Tolerance API version pinned by the project. Cap the total retry window below
the callers’ 30-second REST timeout while preserving the existing maxRetries,
delay, and retryOn settings.
Collaborator
|
✅ Performance Tests passed
Tests run: InsightsNotificationswebhook_runner · InsightsNotificationsgw_runner · InsightsNotificationsemail_runner |
g-duval
reviewed
Jul 23, 2026
g-duval
left a comment
g-duval
left a comment
Contributor
There was a problem hiding this comment.
Looks good, could you please check if we can avoid the cacheing credentials?
| MeterRegistry meterRegistry; | ||
|
|
||
| @Inject | ||
| OAuth2ClientCredentialsCache oauth2ClientCredentialsCache; |
Contributor
There was a problem hiding this comment.
Why do we need to keep those credentials in cache? can't we handle those in postConstruct method?
Contributor
Author
There was a problem hiding this comment.
This handling is something copied/borrowed from backend. On further discussion with claude, i believe the rationale is that postConstruct only runs once during the bean lifetime, while the current cashe is invalidated and refreshed on: channel init, unheathy channel recovery and UNAUTHENTICATED grpc error which gives us free edge case error handling we would likely have to do manually with postConstruct.
| // RBAC/MBOP calls (FetchUsersFromExternalServices) per RecipientSettings in the same request/timeout window -- | ||
| // so the safe target is meaningfully less than 30s, not just-under-30s. No real traffic exercises this path yet | ||
| // (use-kessel toggle is off everywhere), so there's no latency data to size against. Needs sizing guidance. | ||
| @Retry(maxRetries = 3, delay = 100, retryOn = KesselTransientException.class) |
Contributor
There was a problem hiding this comment.
We will revisit it later, once #4539 will be merged.
Assisted-by: Claude Sonnet 5 (via Claude Code)
Assisted-by: Claude Sonnet 5 (via Claude Code)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
engine/src/main/java/com/redhat/cloud/notifications/exports/transformers/ResultsTransformer.java (1)
26-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that the transformer is single-use.
CSVEventTransformer.finish()closes the underlyingCSVPrinter. A call toaddRecordsafterfinishtherefore fails or produces undefined output. State this restriction in the interface contract, so implementers and callers do not rely on repeatedfinishcalls or post-finishwrites.♻️ Proposed Javadoc addition
/** * Finalizes the transformation and returns the serialized contents built - * from every page previously added via {`@link` `#addRecords`}. + * from every page previously added via {`@link` `#addRecords`}. An + * implementation may release its underlying resources here, so call this + * method exactly once, and do not call {`@link` `#addRecords`} afterwards. * `@return` a {`@link` String} with the transformed contents.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/src/main/java/com/redhat/cloud/notifications/exports/transformers/ResultsTransformer.java` around lines 26 - 33, Update the ResultsTransformer.finish() Javadoc contract to state that the transformer is single-use: callers must not invoke finish more than once or call addRecords after finish, because the transformer is finalized and cannot accept further writes.engine/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java (1)
59-61: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd an upper bound for the event export page size.
findEventsByIdspasses each page ase.id IN (:ids)and sets each UUID as a separate bind parameter. PostgreSQL can fail for statements with more than 65,535 bind parameters, including Hibernate query-plan-cache padding. A misconfigurednotifications.events.export.page-sizeabove that limit is currently caught only at query execution; catch it at the existing fail-fast guard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java` around lines 59 - 61, Update the existing page-size validation in findEventsByIds to reject values above the supported bind-parameter limit, while retaining the current positive-integer check and fail-fast IllegalStateException behavior. Ensure the upper-bound validation accounts for Hibernate query-plan-cache padding rather than allowing page sizes that could exceed PostgreSQL’s parameter limit.backend/src/main/resources/META-INF/resources/internal/assets/index-Ct3h5WTo.js (3)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTreat Biome
noSwitchDeclarationsfindings here as artifacts of minification, not a real scoping bug.Static analysis flags multiple
caseblocks with unscopedvar/letdeclarations around these lines. These come from minified switch statements inside vendored library code (React reconciler internals, lodash, and generated OpenAPI action creators). The minifier does not add block-scoping braces percase, but the transpiled logic is still correct as written; there is no evidence of an actual cross-case variable leak causing wrong behavior, since these are direct outputs of tools (esbuild/rollup) that already resolved scoping correctly at the source level.If lint is expected to pass on this path, exclude generated bundle files from the Biome/ESLint configuration rather than editing the minified output directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/main/resources/META-INF/resources/internal/assets/index-Ct3h5WTo.js` around lines 8 - 9, Treat the Biome noSwitchDeclarations findings in the minified bundle as generated-code artifacts rather than source-level scoping defects. Update the Biome/ESLint configuration to exclude generated bundle files, including this asset, from linting; do not modify the minified React, lodash, or OpenAPI output.Source: Linters/SAST tools
76-77: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffTreat
noUnsafeFinallyfindings here as intentional control flow from immer/zod internals, not a bug to patch.Static analysis flags
return/throwstatements insidefinallyblocks near these lines. This pattern appears in the immerfinalize/scope-management helpers and in zod's safe-parse wrappers, where thefinallyintentionally overrides the try/catch outcome as part of the library's control flow contract. Rewriting this in the bundle would diverge from the upstream library behavior and risks introducing regressions that are invisible until the next rebuild overwrites the change anyway.Exclude this generated path from static analysis instead of editing the minified output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/main/resources/META-INF/resources/internal/assets/index-Ct3h5WTo.js` around lines 76 - 77, Exclude this generated asset from the noUnsafeFinally static-analysis rule instead of modifying its minified contents. Preserve the upstream control flow in the immer finalize/scope helpers and zod safe-parse wrappers, and configure the exclusion at the analyzer or project configuration level for this generated path.Source: Linters/SAST tools
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTreat Biome findings on this line as false positives from minified code, not real defects.
Static analysis flags
noSelfCompareat several offsets in this file. Each flagged comparison is the standardx !== xNaN-detection idiom used internally by lodash (eq/baseIsEqual) and by the immer helpergeon Set/Map contents. This is intentional, well-known library code, not a self-compare bug.Do not "fix" these comparisons in the bundle. If Biome runs against build output on every PR, exclude generated bundle paths from static analysis instead of suppressing the rule globally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/main/resources/META-INF/resources/internal/assets/index-Ct3h5WTo.js` at line 1, The Biome static analysis tool is incorrectly flagging legitimate `x !== x` NaN-detection patterns in the minified bundle as self-compare bugs. Rather than suppressing the noSelfCompare rule globally, configure Biome to exclude the generated bundle file paths (like index-Ct3h5WTo.js and similar build output) from static analysis in the Biome configuration file. This keeps the rule active for source code while preventing false positives in minified output.Source: Linters/SAST tools
engine/src/main/java/com/redhat/cloud/notifications/processors/email/EmailAggregationProcessor.java (1)
168-168: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse one validation provider instance.
EmailAggregationProcessor.processAggregationSync()builds aValidatorFactoryon every Kafka aggregation message. Inject the bean-managed QuarkusValidatorwith@Injectinstead, and use it for the aggregation key validation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/src/main/java/com/redhat/cloud/notifications/processors/email/EmailAggregationProcessor.java` at line 168, In EmailAggregationProcessor.processAggregationSync(), replace the line that builds a new ValidatorFactory using Validation.buildDefaultValidatorFactory().getValidator() with an injected Quarkus Validator bean. Add a field annotated with `@Inject` to declare the validator dependency, then use that injected instance for the aggregation key validation instead of creating a new factory each time the method is called.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@admin-console/src/main/webapp/src/app/App.tsx`:
- Around line 63-72: Update the successful response handling in the
useCreateBundle callback to validate the generated operation payload’s success
status and read its value directly, without checking for a type: 'Bundle'
discriminator. Preserve the existing modal closure, refresh, and navigation
flow, using the returned bundle value’s id for navigate.
In
`@admin-console/src/main/webapp/src/components/SystemBehaviorGroups/BehaviorGroupEventTypesPanel.tsx`:
- Around line 155-178: The handleUpdate callback must handle rejected promises
from onLinkEventType and onUnlinkEventType. Wrap the event-type mutation loop in
try/catch/finally, set saveError in catch, and reset saving in finally so the
Update button can be retried; add a test covering a rejected mutation and
verifying retry remains possible.
- Around line 111-117: Update BehaviorGroupEventTypesPanel’s saved-selection
handling so hasChanges compares checkedIds against a local baseline rather than
the unchanged props.behaviorGroup. After each successful link or unlink
operation, advance that baseline only for the corresponding event type; retain
the prior baseline for failures so mixed-success batches retry only unsuccessful
operations. Ensure a second Update click after a fully successful update
performs no duplicate calls, and add tests covering both repeated updates and
mixed-success batches.
- Around line 42-46: Update the unselected branch in the React.useEffect of
BehaviorGroupEventTypesPanel to reset the pending request state, including
loading and fetch error, when selectedAppId is cleared; preserve the existing
eventTypes and checkedIds resets. Add a regression test covering selection
clearance while the request is pending and verify the spinner stops and obsolete
errors are removed.
In `@admin-console/src/main/webapp/src/pages/BundlePage/BehaviorGroupTable.tsx`:
- Around line 18-39: The function actionsToDropdownValue only extracts
information from actions[0], which causes the editor to represent and submit
only a single action. When a behavior group with multiple actions is edited and
submitted, all actions except the first are lost. Update the editor to either
preserve and support editing all actions in the actions array during edit and
submission, or detect when a behavior group contains multiple actions and reject
or disable editing for those records to prevent data loss.
- Around line 81-99: Update handleLinkEventType and handleUnlinkEventType to
wrap both their mutation and getBehaviorGroups.query calls in try/catch blocks,
returning false whenever either operation throws; preserve the existing refresh
and success behavior when operations complete without errors.
In
`@backend/src/test/java/com/redhat/cloud/notifications/routers/handlers/event/EventResourceTest.java`:
- Around line 1500-1555: The test uses NOW for date-only boundary comparisons in
Cases 5 and 6, which causes timezone-dependent failures if NOW occurs before
04:00 UTC (making eventA fall into a previous date). Define a referenceTime
variable set to a stable past midday value (such as a fixed hour offset like
NOW.minusHours(12L)) and replace the NOW.toLocalDate() calls in the date-only
test cases (Cases 5 and 6 in testDateTimeQueryParams) with
referenceTime.toLocalDate() to ensure consistent test behavior regardless of
execution time.
In
`@engine/src/main/java/com/redhat/cloud/notifications/processors/email/EmailAggregationProcessor.java`:
- Around line 172-185: Update the aggregation-key handling in
EmailAggregationProcessor around objectMapper.convertValue and
validator.validate so any key with constraint violations is cleared from command
before the null-key check. Ensure invalid deserializable keys are rejected and
skipped, and add a Kafka aggregation test covering a key that violates a
`@NotNull` field.
---
Nitpick comments:
In
`@backend/src/main/resources/META-INF/resources/internal/assets/index-Ct3h5WTo.js`:
- Around line 8-9: Treat the Biome noSwitchDeclarations findings in the minified
bundle as generated-code artifacts rather than source-level scoping defects.
Update the Biome/ESLint configuration to exclude generated bundle files,
including this asset, from linting; do not modify the minified React, lodash, or
OpenAPI output.
- Around line 76-77: Exclude this generated asset from the noUnsafeFinally
static-analysis rule instead of modifying its minified contents. Preserve the
upstream control flow in the immer finalize/scope helpers and zod safe-parse
wrappers, and configure the exclusion at the analyzer or project configuration
level for this generated path.
- Line 1: The Biome static analysis tool is incorrectly flagging legitimate `x
!== x` NaN-detection patterns in the minified bundle as self-compare bugs.
Rather than suppressing the noSelfCompare rule globally, configure Biome to
exclude the generated bundle file paths (like index-Ct3h5WTo.js and similar
build output) from static analysis in the Biome configuration file. This keeps
the rule active for source code while preventing false positives in minified
output.
In
`@engine/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.java`:
- Around line 59-61: Update the existing page-size validation in findEventsByIds
to reject values above the supported bind-parameter limit, while retaining the
current positive-integer check and fail-fast IllegalStateException behavior.
Ensure the upper-bound validation accounts for Hibernate query-plan-cache
padding rather than allowing page sizes that could exceed PostgreSQL’s parameter
limit.
In
`@engine/src/main/java/com/redhat/cloud/notifications/exports/transformers/ResultsTransformer.java`:
- Around line 26-33: Update the ResultsTransformer.finish() Javadoc contract to
state that the transformer is single-use: callers must not invoke finish more
than once or call addRecords after finish, because the transformer is finalized
and cannot accept further writes.
In
`@engine/src/main/java/com/redhat/cloud/notifications/processors/email/EmailAggregationProcessor.java`:
- Line 168: In EmailAggregationProcessor.processAggregationSync(), replace the
line that builds a new ValidatorFactory using
Validation.buildDefaultValidatorFactory().getValidator() with an injected
Quarkus Validator bean. Add a field annotated with `@Inject` to declare the
validator dependency, then use that injected instance for the aggregation key
validation instead of creating a new factory each time the method is called.
🪄 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 Plus
Run ID: 97a7f011-8530-4672-b644-5fb4d9422f91
⛔ Files ignored due to path filters (1)
admin-console/src/main/webapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (96)
.github/workflows/build.yml.github/workflows/codeql-analysis.yml.rhcicd/clowdapp-connector-email.yaml.rhcicd/clowdapp-engine.yamladmin-console/pom.xmladmin-console/src/main/webapp/.gitignoreadmin-console/src/main/webapp/.yarnrc.ymladmin-console/src/main/webapp/package.jsonadmin-console/src/main/webapp/src/app/App.tsxadmin-console/src/main/webapp/src/app/BundlesContext.tsadmin-console/src/main/webapp/src/app/Navigation.tsxadmin-console/src/main/webapp/src/components/Applications/CreateEditApplicationModal.tsxadmin-console/src/main/webapp/src/components/Bundles/CreateEditBundleModal.tsxadmin-console/src/main/webapp/src/components/EventTypes/EventTypeTable.tsxadmin-console/src/main/webapp/src/components/EventTypes/Table/EventTypeExpandableRow.test.tsxadmin-console/src/main/webapp/src/components/EventTypes/Table/EventTypeExpandableRow.tsxadmin-console/src/main/webapp/src/components/SystemBehaviorGroups/BehaviorGroupEventTypesPanel.test.tsxadmin-console/src/main/webapp/src/components/SystemBehaviorGroups/BehaviorGroupEventTypesPanel.tsxadmin-console/src/main/webapp/src/pages/BundlePage.tsxadmin-console/src/main/webapp/src/pages/BundlePage/BehaviorGroupTable.test.tsxadmin-console/src/main/webapp/src/pages/BundlePage/BehaviorGroupTable.tsxadmin-console/src/main/webapp/src/services/Applications/GetBundleById.tsadmin-console/src/main/webapp/src/services/Bundles/CreateBundle.test.tsadmin-console/src/main/webapp/src/services/Bundles/CreateBundle.tsadmin-console/src/main/webapp/src/services/EventTypes/GetBundles.tsadmin-console/src/main/webapp/src/services/SystemBehaviorGroups/GetBehaviorGroups.tsxadmin-console/src/main/webapp/src/services/SystemBehaviorGroups/LinkDefaultBehaviorToEventType.test.tsadmin-console/src/main/webapp/src/services/SystemBehaviorGroups/LinkDefaultBehaviorToEventType.tsadmin-console/src/main/webapp/src/services/SystemBehaviorGroups/UnlinkDefaultBehaviorToEventType.test.tsadmin-console/src/main/webapp/src/services/SystemBehaviorGroups/UnlinkDefaultBehaviorToEventType.tsadmin-console/src/main/webapp/src/types/Notifications.tsbackend/pom.xmlbackend/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.javabackend/src/main/java/com/redhat/cloud/notifications/models/dto/v2/subscriptions/ApplicationSubscriptionDTO.javabackend/src/main/java/com/redhat/cloud/notifications/models/dto/v2/subscriptions/ApplicationSubscriptionUpdateDTO.javabackend/src/main/java/com/redhat/cloud/notifications/models/dto/v2/subscriptions/BundleSubscriptionDTO.javabackend/src/main/java/com/redhat/cloud/notifications/models/dto/v2/subscriptions/BundleSubscriptionUpdateDTO.javabackend/src/main/java/com/redhat/cloud/notifications/models/dto/v2/subscriptions/EventTypeSubscriptionDTO.javabackend/src/main/java/com/redhat/cloud/notifications/models/dto/v2/subscriptions/EventTypeSubscriptionUpdateDTO.javabackend/src/main/java/com/redhat/cloud/notifications/models/dto/v2/subscriptions/SeverityDTO.javabackend/src/main/java/com/redhat/cloud/notifications/models/dto/v2/subscriptions/SubscriptionChannelDTO.javabackend/src/main/java/com/redhat/cloud/notifications/models/dto/v2/subscriptions/SubscriptionMapper.javabackend/src/main/java/com/redhat/cloud/notifications/models/dto/v2/subscriptions/SubscriptionTypeDTO.javabackend/src/main/java/com/redhat/cloud/notifications/oapi/OApiFilter.javabackend/src/main/java/com/redhat/cloud/notifications/routers/IncomingRequestInterceptor.javabackend/src/main/java/com/redhat/cloud/notifications/routers/handlers/event/EventResource.javabackend/src/main/java/com/redhat/cloud/notifications/routers/handlers/userconfig/UserConfigResourceV2.javabackend/src/main/resources/META-INF/resources/internal/assets/index-88ygW8gG.jsbackend/src/main/resources/META-INF/resources/internal/assets/index-CLX1xrMB.cssbackend/src/main/resources/META-INF/resources/internal/assets/index-Ct3h5WTo.jsbackend/src/main/resources/META-INF/resources/internal/assets/index-XQ7zc42e.cssbackend/src/main/resources/META-INF/resources/internal/index.htmlbackend/src/main/resources/ephemeral/ephemeral_data.jsonbackend/src/test/java/com/redhat/cloud/notifications/models/dto/v2/subscriptions/SubscriptionMapperTest.javabackend/src/test/java/com/redhat/cloud/notifications/oapi/OApiFilterTest.javabackend/src/test/java/com/redhat/cloud/notifications/routers/IncomingRequestInterceptorTest.javabackend/src/test/java/com/redhat/cloud/notifications/routers/handlers/event/EventResourceTest.javabackend/src/test/java/com/redhat/cloud/notifications/routers/handlers/userconfig/UserConfigResourceV2Test.javacommon-template/src/main/java/com/redhat/cloud/notifications/qute/templates/extensions/ActionExtension.javacommon-template/src/main/java/com/redhat/cloud/notifications/qute/templates/mapping/OpenShift.javacommon-template/src/main/resources/templates/email/Common/insightsDailyEmailBody.htmlcommon-template/src/main/resources/templates/email/Common/insightsEmailBody.htmlcommon-template/src/main/resources/templates/email/Errata/dailyEmailBody.htmlcommon-template/src/main/resources/templates/email/OCM/generalNotificationInstantEmailBody.htmlcommon-template/src/main/resources/templates/email/Oma/assessmentCreatedInstantEmailBody.htmlcommon-template/src/main/resources/templates/email/Oma/assessmentSharedInstantEmailBody.htmlcommon-template/src/main/resources/templates/email/Oma/partnershipRequestInstantEmailBody.htmlcommon-template/src/main/resources/templates/email/Oma/partnershipResponseInstantEmailBody.htmlcommon-template/src/main/resources/templates/email/Secure/Common/insightsDailyEmailBody.htmlcommon-template/src/main/resources/templates/email/Secure/Common/insightsEmailBody.htmlcommon-template/src/test/java/com/redhat/cloud/notifications/qute/templates/extensions/ActionExtensionTest.javacommon-template/src/test/java/email/TestDefaultTemplate.javacommon-template/src/test/java/email/TestEmailHeaderLogoSection.javacommon-template/src/test/java/email/TestErrataTemplate.javacommon-template/src/test/java/email/TestOmaTemplate.javaengine/src/main/java/com/redhat/cloud/notifications/config/EngineConfig.javaengine/src/main/java/com/redhat/cloud/notifications/db/repositories/EventRepository.javaengine/src/main/java/com/redhat/cloud/notifications/events/deduplication/EventDeduplicator.javaengine/src/main/java/com/redhat/cloud/notifications/exports/EventExporterService.javaengine/src/main/java/com/redhat/cloud/notifications/exports/transformers/ResultsTransformer.javaengine/src/main/java/com/redhat/cloud/notifications/exports/transformers/event/CSVEventTransformer.javaengine/src/main/java/com/redhat/cloud/notifications/exports/transformers/event/JSONEventTransformer.javaengine/src/main/java/com/redhat/cloud/notifications/processors/email/EmailActorsResolver.javaengine/src/main/java/com/redhat/cloud/notifications/processors/email/EmailAggregationProcessor.javaengine/src/main/java/com/redhat/cloud/notifications/processors/email/EmailAggregator.javaengine/src/main/java/com/redhat/cloud/notifications/processors/email/EmailPendoResolver.javaengine/src/main/java/com/redhat/cloud/notifications/processors/email/aggregators/AbstractEmailPayloadAggregator.javaengine/src/main/java/com/redhat/cloud/notifications/processors/email/aggregators/EmailPayloadAggregatorFactory.javaengine/src/test/java/com/redhat/cloud/notifications/db/repositories/EventRepositoryTest.javaengine/src/test/java/com/redhat/cloud/notifications/exports/ExportEventListenerMockServerTest.javaengine/src/test/java/com/redhat/cloud/notifications/exports/ExportEventListenerTest.javaengine/src/test/java/com/redhat/cloud/notifications/exports/transformers/event/CSVEventTransformerTest.javaengine/src/test/java/com/redhat/cloud/notifications/exports/transformers/event/JSONEventTransformerTest.javaengine/src/test/java/com/redhat/cloud/notifications/processors/email/EmailActorsResolverTest.javaengine/src/test/java/com/redhat/cloud/notifications/processors/email/EmailPendoResolverTest.javapom.xml
Comment on lines
+63
to
+72
| }).then((response) => { | ||
| if (response.error) { | ||
| setBundleCreateError('Failed to create bundle. Please check the values and try again.'); | ||
| } else { | ||
| setShowBundleModal(false); | ||
| bundles.query(); | ||
| const createdBundle = response.payload?.type === 'Bundle' ? response.payload.value : undefined; | ||
| if (createdBundle?.id) { | ||
| navigate(linkTo.bundle(createdBundle.id)); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Read the created bundle from the generated operation payload.
useCreateBundle returns the raw generated response. It does not add the type: 'Bundle' discriminator used on Line 69.
As a result, createdBundle remains undefined. A successful creation closes the modal but does not navigate to the new bundle.
Check the successful payload status and read its value directly.
Proposed fix
- const createdBundle = response.payload?.type === 'Bundle' ? response.payload.value : undefined;
+ const createdBundle = response.payload?.status === 200
+ ? response.payload.value
+ : undefined;📝 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
| }).then((response) => { | |
| if (response.error) { | |
| setBundleCreateError('Failed to create bundle. Please check the values and try again.'); | |
| } else { | |
| setShowBundleModal(false); | |
| bundles.query(); | |
| const createdBundle = response.payload?.type === 'Bundle' ? response.payload.value : undefined; | |
| if (createdBundle?.id) { | |
| navigate(linkTo.bundle(createdBundle.id)); | |
| } | |
| }).then((response) => { | |
| if (response.error) { | |
| setBundleCreateError('Failed to create bundle. Please check the values and try again.'); | |
| } else { | |
| setShowBundleModal(false); | |
| bundles.query(); | |
| const createdBundle = response.payload?.status === 200 | |
| ? response.payload.value | |
| : undefined; | |
| if (createdBundle?.id) { | |
| navigate(linkTo.bundle(createdBundle.id)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@admin-console/src/main/webapp/src/app/App.tsx` around lines 63 - 72, Update
the successful response handling in the useCreateBundle callback to validate the
generated operation payload’s success status and read its value directly,
without checking for a type: 'Bundle' discriminator. Preserve the existing modal
closure, refresh, and navigation flow, using the returned bundle value’s id for
navigate.
Contributor
Author
There was a problem hiding this comment.
not part of this PR concern, i will mark it separatedly
Contributor
There was a problem hiding this comment.
@bonscji1, acknowledged. This concern is out of scope for this pull request. Would you like me to create a follow-up GitHub issue for it?
🐇💭
You are interacting with an AI system.
Comment on lines
+42
to
+46
| React.useEffect(() => { | ||
| if (!selectedAppId) { | ||
| setEventTypes([]); | ||
| setCheckedIds(new Set()); | ||
| return; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reset request state when the application selection is cleared.
If the user clears the selection during a fetch, cleanup prevents the earlier finally block from clearing loading. This branch also retains a previous fetch error. The spinner can remain active and an obsolete error can remain visible with no selected application.
Add a regression test that clears the selection while the request is pending.
Proposed fix
if (!selectedAppId) {
+ setLoading(false);
+ setFetchError(null);
setEventTypes([]);
setCheckedIds(new Set());
return;📝 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
| React.useEffect(() => { | |
| if (!selectedAppId) { | |
| setEventTypes([]); | |
| setCheckedIds(new Set()); | |
| return; | |
| React.useEffect(() => { | |
| if (!selectedAppId) { | |
| setLoading(false); | |
| setFetchError(null); | |
| setEventTypes([]); | |
| setCheckedIds(new Set()); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@admin-console/src/main/webapp/src/components/SystemBehaviorGroups/BehaviorGroupEventTypesPanel.tsx`
around lines 42 - 46, Update the unselected branch in the React.useEffect of
BehaviorGroupEventTypesPanel to reset the pending request state, including
loading and fetch error, when selectedAppId is cleared; preserve the existing
eventTypes and checkedIds resets. Add a regression test covering selection
clearance while the request is pending and verify the spinner stops and obsolete
errors are removed.
Contributor
Author
There was a problem hiding this comment.
not part of the PR, will mark separatedly
Contributor
There was a problem hiding this comment.
@bonscji1 Understood. Please address this in the separate change.
Do you want me to open a GitHub follow-up issue with the regression-test requirement?
You are interacting with an AI system.
Comment on lines
+155
to
+178
| for (const et of eventTypes) { | ||
| const wasLinked = isEventTypeLinked(props.behaviorGroup, et.id); | ||
| const isChecked = checkedIds.has(et.id); | ||
|
|
||
| if (isChecked && !wasLinked) { | ||
| const success = await props.onLinkEventType(bgId, et.id); | ||
| if (!success) { | ||
| errors.push(et.displayName); | ||
| } | ||
| } else if (!isChecked && wasLinked) { | ||
| const success = await props.onUnlinkEventType(bgId, et.id); | ||
| if (!success) { | ||
| errors.push(et.displayName); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| setSaving(false); | ||
| if (errors.length > 0) { | ||
| setSaveError(`Failed to update: ${errors.join(', ')}`); | ||
| } else { | ||
| setSaveSuccess(true); | ||
| } | ||
| }, [ props, eventTypes, checkedIds ]); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Always clear saving when a mutation rejects.
A rejection from onLinkEventType or onUnlinkEventType exits handleUpdate before Line 172. The Update button then remains disabled and loading, with no error alert. Wrap the mutation batch in try/catch/finally, set saveError in catch, and reset saving in finally.
Add a rejected-promise test that verifies the user can retry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@admin-console/src/main/webapp/src/components/SystemBehaviorGroups/BehaviorGroupEventTypesPanel.tsx`
around lines 155 - 178, The handleUpdate callback must handle rejected promises
from onLinkEventType and onUnlinkEventType. Wrap the event-type mutation loop in
try/catch/finally, set saveError in catch, and reset saving in finally so the
Update button can be retried; add a test covering a rejected mutation and
verifying retry remains possible.
Comment on lines
+18
to
+39
| export const actionsToDropdownValue = (actions?: BehaviorGroupAction[] | null): string | undefined => { | ||
| if (!actions || actions.length === 0) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const action = actions[0]; | ||
| const properties = action.endpoint?.properties as Schemas.SystemSubscriptionProperties; | ||
| const endpointType = action.endpoint?.type; | ||
| if (!properties || !endpointType) { | ||
| return undefined; | ||
| } | ||
|
|
||
| if (endpointType === 'drawer') { | ||
| return properties.only_admins ? 'drawer-admin' : 'drawer-all'; | ||
| } | ||
|
|
||
| if (endpointType === 'email_subscription') { | ||
| return properties.only_admins ? 'email-admin' : 'email-all'; | ||
| } | ||
|
|
||
| return undefined; | ||
| }; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve all actions when editing a behavior group.
actionsToDropdownValue reads only actions[0]. The edit state then replaces the complete actions array with one dropdown value.
handleSubmit sends one action. Therefore, editing a behavior group that contains multiple actions can delete every action except the first one.
Update the editor to represent all actions. If the product permits only one action, reject or disable editing for records that already contain multiple actions.
Also applies to: 114-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@admin-console/src/main/webapp/src/pages/BundlePage/BehaviorGroupTable.tsx`
around lines 18 - 39, The function actionsToDropdownValue only extracts
information from actions[0], which causes the editor to represent and submit
only a single action. When a behavior group with multiple actions is edited and
submitted, all actions except the first are lost. Update the editor to either
preserve and support editing all actions in the actions array during edit and
submission, or detect when a behavior group contains multiple actions and reject
or disable editing for those records to prevent data loss.
Comment on lines
+81
to
+99
| const handleLinkEventType = React.useCallback(async (behaviorGroupId: string, eventTypeId: string) => { | ||
| const response = await linkMutation.mutate({ behaviorGroupId, eventTypeId }); | ||
| if (!response.error) { | ||
| const refreshResult = await getBehaviorGroups.query(); | ||
| return !refreshResult.error; | ||
| } | ||
|
|
||
| return false; | ||
| }, [ linkMutation.mutate, getBehaviorGroups.query ]); | ||
|
|
||
| const handleUnlinkEventType = React.useCallback(async (behaviorGroupId: string, eventTypeId: string) => { | ||
| const response = await unlinkMutation.mutate({ behaviorGroupId, eventTypeId }); | ||
| if (!response.error) { | ||
| const refreshResult = await getBehaviorGroups.query(); | ||
| return !refreshResult.error; | ||
| } | ||
|
|
||
| return false; | ||
| }, [ unlinkMutation.mutate, getBehaviorGroups.query ]); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file and related symbols"
git ls-files | rg 'BehaviorGroupTable\.tsx|BehaviorGroupEventTypesPanel|BehaviorGroups' || true
echo
echo "Inspect target file outline"
if [ -f admin-console/src/main/webapp/src/pages/BundlePage/BehaviorGroupTable.tsx ]; then
ast-grep outline admin-console/src/main/webapp/src/pages/BundlePage/BehaviorGroupTable.tsx --view compact || true
echo
echo "Lines 1-180"
sed -n '1,180p' admin-console/src/main/webapp/src/pages/BundlePage/BehaviorGroupTable.tsx | nl -ba
fi
echo
echo "Search for handleUpdate usage"
rg -n "handleUpdate|handleLinkEventType|handleUnlinkEventType|isSaving|setIsSaving|saving|mutate\\(" admin-console/src/main/webapp/src/pages/BundlePage -S || trueRepository: RedHatInsights/notifications-backend
Length of output: 1673
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect target file"
if [ -f admin-console/src/main/webapp/src/pages/BundlePage/BehaviorGroupTable.tsx ]; then
sed -n '1,180p' admin-console/src/main/webapp/src/pages/BundlePage/BehaviorGroupTable.tsx
fi
echo
echo "Inspect related file"
if [ -f admin-console/src/main/webapp/src/components/SystemBehaviorGroups/BehaviorGroupEventTypesPanel.tsx ]; then
sed -n '1,240p' admin-console/src/main/webapp/src/components/SystemBehaviorGroups/BehaviorGroupEventTypesPanel.tsx
fi
echo
echo "Search relevant usages"
rg -n "handleUpdate|handleLinkEventType|handleUnlinkEventType|isSaving|setIsSaving|saving|mutate\\(" admin-console/src/main/webapp/src/pages/BundlePage admin-console/src/main/webapp/src/components/SystemBehaviorGroups -S || trueRepository: RedHatInsights/notifications-backend
Length of output: 18565
Return false when link mutations reject.
handleLinkEventType and handleUnlinkEventType are awaited in BehaviorGroupEventTypesPanel.handleUpdate before setSaving(false). If linkMutation.mutate(), unlinkMutation.mutate(), or getBehaviorGroups.query() throws, handleUpdate rejects instead of recording the failed event types and clearing saving state. Wrap mutate() and query() in try/catch and return false on failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@admin-console/src/main/webapp/src/pages/BundlePage/BehaviorGroupTable.tsx`
around lines 81 - 99, Update handleLinkEventType and handleUnlinkEventType to
wrap both their mutation and getBehaviorGroups.query calls in try/catch blocks,
returning false whenever either operation throws; preserve the existing refresh
and success behavior when operations complete without errors.
Comment on lines
+1500
to
+1555
| @ParameterizedTest | ||
| @CsvSource({"false,false", "false,true", "true,false", "true,true"}) | ||
| void testDateTimeQueryParams(boolean kesselEnabled, boolean useNormalizedQueries) { | ||
| when(backendConfig.isKesselEnabled(anyString())).thenReturn(kesselEnabled); | ||
| when(backendConfig.isNormalizedQueriesEnabled(anyString())).thenReturn(useNormalizedQueries); | ||
| if (kesselEnabled) { | ||
| mockDefaultKesselPermission(EVENTS_VIEW, ALLOWED_TRUE); | ||
| } | ||
|
|
||
| Header identityHeader = mockRbac(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, DEFAULT_USER, FULL_ACCESS); | ||
|
|
||
| Bundle bundle = resourceHelpers.createBundle("bundle-dt", "Bundle DT"); | ||
| Application app = resourceHelpers.createApplication(bundle.getId(), "app-dt", "App DT"); | ||
| EventType eventType = resourceHelpers.createEventType(app.getId(), "et-dt", "ET DT", "ET DT"); | ||
|
|
||
| Event eventA = createEvent(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, bundle, app, eventType, NOW.minusHours(4L)); | ||
| Event eventB = createEvent(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, bundle, app, eventType, NOW.minusHours(2L)); | ||
| Event eventC = createEvent(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, bundle, app, eventType, NOW.minusMinutes(30L)); | ||
|
|
||
| // Case 1: datetime values on both bounds -- only eventB falls within the window | ||
| Page<EventLogEntry> page = getEventLogPageWithRawDateParams(identityHeader, | ||
| NOW.minusHours(3L).toString(), NOW.minusHours(1L).toString()); | ||
| assertEquals(1, page.getMeta().getCount()); | ||
| assertEquals(eventB.getId(), page.getData().get(0).getId()); | ||
|
|
||
| // Case 2: datetime start only -- open-ended forward range | ||
| page = getEventLogPageWithRawDateParams(identityHeader, NOW.minusHours(3L).toString(), null); | ||
| assertEquals(2, page.getMeta().getCount()); | ||
| assertTrue(page.getData().stream().anyMatch(e -> e.getId().equals(eventB.getId()))); | ||
| assertTrue(page.getData().stream().anyMatch(e -> e.getId().equals(eventC.getId()))); | ||
|
|
||
| // Case 3: datetime end only -- open-ended backward range | ||
| page = getEventLogPageWithRawDateParams(identityHeader, null, NOW.minusHours(1L).toString()); | ||
| assertEquals(2, page.getMeta().getCount()); | ||
| assertTrue(page.getData().stream().anyMatch(e -> e.getId().equals(eventA.getId()))); | ||
| assertTrue(page.getData().stream().anyMatch(e -> e.getId().equals(eventB.getId()))); | ||
|
|
||
| // Case 4: mixed formats -- start as datetime, end as date-only (expands to end of day) | ||
| page = getEventLogPageWithRawDateParams(identityHeader, NOW.minusHours(1L).toString(), NOW.toLocalDate().toString()); | ||
| assertEquals(1, page.getMeta().getCount()); | ||
| assertEquals(eventC.getId(), page.getData().get(0).getId()); | ||
|
|
||
| // Case 5: mixed formats -- start as date-only (expands to start of day), end as datetime | ||
| page = getEventLogPageWithRawDateParams(identityHeader, NOW.toLocalDate().toString(), NOW.minusHours(1L).toString()); | ||
| assertEquals(2, page.getMeta().getCount()); | ||
| assertTrue(page.getData().stream().anyMatch(e -> e.getId().equals(eventA.getId()))); | ||
| assertTrue(page.getData().stream().anyMatch(e -> e.getId().equals(eventB.getId()))); | ||
|
|
||
| // Case 6: date-only on both bounds (regression) -- all 3 events fall within today | ||
| page = getEventLogPageWithRawDateParams(identityHeader, NOW.toLocalDate().toString(), NOW.toLocalDate().toString()); | ||
| assertEquals(3, page.getMeta().getCount()); | ||
|
|
||
| // Case 7: no date params (regression) -- unbounded, all events returned | ||
| page = getEventLogPageWithRawDateParams(identityHeader, null, null); | ||
| assertEquals(3, page.getMeta().getCount()); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use a past midday reference time for the date-only cases.
NOW.minusHours(4L) enters the prior UTC date when this test starts before 04:00 UTC. Case 5 can then exclude its expected fixtures, and Case 6 no longer contains all three fixtures. The test result depends on execution time.
Proposed fix
+ LocalDateTime referenceTime = NOW.minusDays(1L)
+ .withHour(12)
+ .withMinute(0)
+ .withSecond(0)
+ .withNano(0);
+
- Event eventA = createEvent(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, bundle, app, eventType, NOW.minusHours(4L));
- Event eventB = createEvent(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, bundle, app, eventType, NOW.minusHours(2L));
- Event eventC = createEvent(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, bundle, app, eventType, NOW.minusMinutes(30L));
+ Event eventA = createEvent(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, bundle, app, eventType, referenceTime.minusHours(4L));
+ Event eventB = createEvent(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, bundle, app, eventType, referenceTime.minusHours(2L));
+ Event eventC = createEvent(DEFAULT_ACCOUNT_ID, DEFAULT_ORG_ID, bundle, app, eventType, referenceTime.minusMinutes(30L));Replace the remaining NOW query bounds in this test with referenceTime.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/src/test/java/com/redhat/cloud/notifications/routers/handlers/event/EventResourceTest.java`
around lines 1500 - 1555, The test uses NOW for date-only boundary comparisons
in Cases 5 and 6, which causes timezone-dependent failures if NOW occurs before
04:00 UTC (making eventA fall into a previous date). Define a referenceTime
variable set to a stable past midday value (such as a fixed hour offset like
NOW.minusHours(12L)) and replace the NOW.toLocalDate() calls in the date-only
test cases (Cases 5 and 6 in testDateTimeQueryParams) with
referenceTime.toLocalDate() to ensure consistent test behavior regardless of
execution time.
Comment on lines
+172
to
+185
| AggregationCommand command = objectMapper.convertValue(actionEvent.getPayload().getAdditionalProperties(), AggregationCommand.class); | ||
| try { | ||
| JsonObject aggregationKey = new JsonObject(actionEvent.getPayload().getAdditionalProperties()).getJsonObject("aggregationKey"); | ||
| EventAggregationCriterion key = objectMapper.convertValue(aggregationKey, EventAggregationCriterion.class); | ||
| Set<ConstraintViolation<EventAggregationCriterion>> constraintViolations = validator.validate(key); | ||
| if (constraintViolations.isEmpty()) { | ||
| command.setAggregationKey(key); | ||
| } | ||
| } catch (Exception e) { | ||
| Log.error("Kafka aggregation payload parsing key failed to be cast as 'EventAggregationCriteria' for event: " + event.getId() + ", aggregation: " + actionEvent.toString(), e); | ||
| } | ||
| if (command.getAggregationKey() == null) { | ||
| Log.warnf("Skipping aggregation command with null key for event: %s", event.getId()); | ||
| rejectedAggregationCommandCount.increment(); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
while IFS= read -r file; do
echo "--- $file ---"
ast-grep outline "$file" --items all
rg -n -C 8 'aggregationKey|setAggregationKey|getAggregationKey|JsonIgnore|JsonProperty' "$file" || true
done < <(fd -t f '^AggregationCommand\.java$' .)
rg -n -C 8 --glob '*.java' 'processAggregationSync|AggregationCommand|aggregationKey' engine/src/test || trueRepository: RedHatInsights/notifications-backend
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "files matching AggregationCommand.java:"
fd -t f 'AggregationCommand\.java$' . || true
echo
echo "target and related java files:"
while IFS= read -r file; do
echo "--- $file ---"
wc -l "$file"
ast-grep outline "$file" --items all || true
rg -n -C 8 'aggregationKey|setAggregationKey|getAggregationKey|`@JsonIgnore`|`@JsonProperty`' "$file" || true
done < <(fd -t f 'AggregationCommand\.java$' .)
echo
echo "test references:"
rg -n -C 8 --glob '*.java' '(processAggregationSync|AggregationCommand|aggregationKey|EventAggregationCriterion)' engine/src/test || trueRepository: RedHatInsights/notifications-backend
Length of output: 384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "target file:"
file="engine/src/main/java/com/redhat/cloud/notifications/processors/email/EmailAggregationProcessor.java"
if [ -f "$file" ]; then
wc -l "$file"
sed -n '140,210p' "$file" | cat -n
echo "--- symbols/usages in target ---"
ast-grep outline "$file" --items all || true
rg -n -C 6 'aggregationKey|setAggregationKey|getAggregationKey|convertValue|additionalProperties|processAggregationSync' "$file" || true
else
echo "missing $file"
fi
echo
echo "model:"
model="common/src/main/java/com/redhat/cloud/notifications/models/AggregationCommand.java"
if [ -f "$model" ]; then
wc -l "$model"
sed -n '1,220p' "$model" | cat -n
rg -n -C 6 'aggregationKey|setAggregationKey|getAggregationKey' "$model" || true
else
echo "model missing"
fi
echo
echo "tests:"
rg -n -C 8 --glob '*.java' '(processAggregationSync|AggregationCommand|aggregationKey|EventAggregationCriterion)' engine/src/test || trueRepository: RedHatInsights/notifications-backend
Length of output: 50392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "EventAggregationCriterion files:"
while IFS= read -r file; do
echo "--- $file ---"
wc -l "$file"
sed -n '1,220p' "$file" | cat -n
rg -n -C 6 '@|org|application|bundle|key|equals|hashCode|toString' "$file" || true
done < <(fd -t f 'EventAggregationCriterion\.java$' .)
echo
echo "Jackson/Jakarta validation dependencies:"
rg -n 'jackson|jakarta.validation|hibernate-validator|validation-api' pom.xml */pom.xml dependencies 2>/dev/null || true
echo
echo "config serializers/deserializers for AggregationCommand:EventAggregationCriterion:"
rg -n -C 6 'Jsonb|JsonbConfig|JsonArray|JsonObject|SerializationFeature|DeserializationFeature|EventAggregationCriterion|AggregationCommand|Jackson|Json' common/src/main/java engine/src/main/java || trueRepository: RedHatInsights/notifications-backend
Length of output: 260
🏁 Script executed:
#!/bin/bash
set -u
echo "EventAggregationCriterion.java list:"
fd -t f '^EventAggregationCriterion\.java$' . || true
echo
echo "EventAggregationCriterion.java contents (all files):"
while IFS= read -r file; do
file="${file#"$PWD"/}"
echo "--- $file ---"
wc -l "$file"
sed -n '1,220p' "$file" | cat -n
echo "-----"
done < <(fd -t f '^EventAggregationCriterion\.java$' .)
echo
echo "Jackson/Jakarta validation annotations in relevant models:"
rg -n '`@Json`|`@JsonIgnore`|`@Jsonb`|`@NotNull`|`@NotNull`|Constraint' common/src/main/java/com/redhat/cloud/notifications/models/AggregationCommand.java common/src/main/java/com/redhat/cloud/notifications/models/EventAg 2>/dev/null || trueRepository: RedHatInsights/notifications-backend
Length of output: 1039
🏁 Script executed:
#!/bin/bash
set -u
file="common/src/main/java/com/redhat/cloud/notifications/models/EventAggregationCriterion.java"
if [ -f "$file" ]; then
wc -l "$file"
cat -n "$file"
echo "--- Jackson/config search ---"
rg -n -C 6 'Jsonb|JsonbConfig|SerializationFeature|DeserializationFeature|PropertyNamingStrategy|EventAggregationCriterion|AggregationCommand' common/src/main/java engine/src/main/java pom.xml 2>/dev/null || true
else
echo "missing $file"
fiRepository: RedHatInsights/notifications-backend
Length of output: 50392
🏁 Script executed:
#!/bin/bash
set -u
echo "AggregationCommand.java:"
file="common/src/main/java/com/redhat/cloud/notifications/models/AggregationCommand.java"
cat -n "$file"
echo
echo "Jackson/Jsonb config and model annotations only:"
rg -n --glob '*.java' -C 3 'Jsonb|JsonbConfig|SerializationFeature|DeserializationFeature|PropertyNamingStrategy|`@Json`|`@JsonIgnore`|EventAggregationCriterion|AggregationCommand' common/src/main/java engine/src/main/java/engine -g '*.java' 2>/dev/null | head -n 220 || trueRepository: RedHatInsights/notifications-backend
Length of output: 26730
Reject deserialized invalid aggregation keys immediately.
objectMapper.convertValue(..., AggregationCommand.class) can populate command.aggregationKey with a deserializable EventAggregationCriterion before the separately deserialized key is validated. When that validation has violations, the current code keeps command.aggregationKey and adds the command to the aggregation list. Clear aggregationKey before validation or validate and reset the field on violations before reaching command.getAggregationKey() == null. Add a Kafka aggregation test using a deserializable key that violates a @NotNull field.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@engine/src/main/java/com/redhat/cloud/notifications/processors/email/EmailAggregationProcessor.java`
around lines 172 - 185, Update the aggregation-key handling in
EmailAggregationProcessor around objectMapper.convertValue and
validator.validate so any key with constraint violations is cleared from command
before the null-key check. Ensure invalid deserializable keys are rejected and
skipped, and add a Kafka aggregation test covering a key that violates a
`@NotNull` field.
Collaborator
|
❌ Performance Tests failed
Tests run: InsightsNotificationswebhook_runner · InsightsNotificationsgw_runner · InsightsNotificationsemail_runner 📊 Performance ResultsInsightsNotificationsemail_runner
InsightsNotificationswebhook_runner
InsightsNotificationsgw_runner
|
bonscji1
marked this pull request as draft
August 4, 2026 09:44
bonscji1
force-pushed
the
RHCLOUD-44097-kessel-api-migration
branch
from
August 4, 2026 09:59
1eb1550 to
b056682
Compare
bonscji1
marked this pull request as ready for review
August 4, 2026 10:01
Collaborator
|
❌ Performance Tests failed
Tests run: InsightsNotificationswebhook_runner · InsightsNotificationsgw_runner · InsightsNotificationsemail_runner 📊 Performance ResultsInsightsNotificationsemail_runner
InsightsNotificationswebhook_runner
InsightsNotificationsgw_runner
|
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
3 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.
Assisted-by: Claude Sonnet 5 (via Claude Code)
Summary by CodeRabbit