fix(graphQL): GraphQL subscription not notified on DialogActivityCreated #1187
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes streamline the event handling process in the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (1)
- src/Digdir.Domain.Dialogporten.Infrastructure/DomainEvents/Outbox/ConvertDomainEventsToOutboxMessagesInterceptor.cs (4 hunks)
...gporten.Infrastructure/DomainEvents/Outbox/ConvertDomainEventsToOutboxMessagesInterceptor.cs
Outdated
Show resolved
Hide resolved
...gporten.Infrastructure/DomainEvents/Outbox/ConvertDomainEventsToOutboxMessagesInterceptor.cs
Outdated
Show resolved
Hide resolved
|
I'm not sure I understand this change. I'm assuming the DialogActivityCreated is produced first as we walk up the aggregate, and that the DialogUpdated event effectively never gets produced whenever an activity is appended? Won't this break consumers who only listen for DialogUpdated events? |
You mean for Altinn Events? This does not affect that, only the GQL subscription. The old code only notified GQL-subs when DialogUpdated happened, so if a SO sends a patch which only adds a DialogActivity, AF would never be notified. This new version pings the GQL-sub on DialogActivityCreated and DialogUpdated, basically combining the two events. (And adding a short-lived cache so they don't get two events) The alternative is to have two separate pings for each of those, and having AF write two new queries, one that fetches the dialog without the activities, and one where they only fetch activities. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/DialogEntity.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/DomainEvents/Outbox/ConvertDomainEventsToOutboxMessagesInterceptor.cs (1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.Infrastructure/DomainEvents/Outbox/ConvertDomainEventsToOutboxMessagesInterceptor.cs (1)
Learnt from: oskogstad PR: digdir/dialogporten#1187 File: src/Digdir.Domain.Dialogporten.Infrastructure/DomainEvents/Outbox/ConvertDomainEventsToOutboxMessagesInterceptor.cs:105-122 Timestamp: 2024-09-24T22:26:57.178Z Learning: In the `ConvertDomainEventsToOutboxMessagesInterceptor` class, using a static in-memory cache (`UpdateEventThrottleCache`) for throttling in the `SendDialogUpdated` method is acceptable because only one instance of the WebAPI receives the REST request that generates these events. The throttling is meant to be local to prevent clients from receiving multiple notifications when both an update event and an activity created event are generated in the same transaction.
🔇 Additional comments (2)
src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/DialogEntity.cs (1)
77-78: SimplifiedOnUpdatemethod: Approve with concernsThe simplification of the
OnUpdatemethod improves code readability and maintainability. It ensures that aDialogUpdatedDomainEventis always published when the method is called, which aligns with the goal of combiningDialogActivityCreatedandDialogUpdatedevents as mentioned in the PR discussion.However, I have two concerns:
This change may lead to more frequent event publishing, which could potentially impact performance. Can you confirm if this increase in event frequency is intentional and has been considered?
The PR objectives mention implementing throttling for update events, but this change doesn't seem to directly address that. Could you clarify where the throttling logic is implemented, or if it's handled elsewhere in the system?
To better understand the impact of this change, could you run the following script to check for other usages of
DialogUpdatedDomainEventin the codebase?This will help us verify if there are any other places where the event is used or filtered, which might be affected by this change.
src/Digdir.Domain.Dialogporten.Infrastructure/DomainEvents/Outbox/ConvertDomainEventsToOutboxMessagesInterceptor.cs (1)
67-95:⚠️ Potential issueRemoval of Throttling May Lead to Duplicate Notifications
Based on previous learnings, a throttling mechanism using a static in-memory cache (
UpdateEventThrottleCache) was implemented to prevent clients from receiving multiple notifications when both an update event and aDialogActivityCreatedevent are generated in the same transaction. The current changes remove this throttling mechanism, which may reintroduce the issue of clients receiving duplicate notifications, potentially leading to unnecessary load and suboptimal user experience.Consider reintroducing the throttling mechanism or adjusting the event handling logic to ensure that clients receive only a single notification when related events occur in the same transaction.
⛔ Skipped due to learnings
Learnt from: oskogstad PR: digdir/dialogporten#1187 File: src/Digdir.Domain.Dialogporten.Infrastructure/DomainEvents/Outbox/ConvertDomainEventsToOutboxMessagesInterceptor.cs:105-122 Timestamp: 2024-09-24T22:26:57.178Z Learning: In the `ConvertDomainEventsToOutboxMessagesInterceptor` class, using a static in-memory cache (`UpdateEventThrottleCache`) for throttling in the `SendDialogUpdated` method is acceptable because only one instance of the WebAPI receives the REST request that generates these events. The throttling is meant to be local to prevent clients from receiving multiple notifications when both an update event and an activity created event are generated in the same transaction.
...gporten.Infrastructure/DomainEvents/Outbox/ConvertDomainEventsToOutboxMessagesInterceptor.cs
Show resolved
Hide resolved
|
|
Change suggestion from Magnus pushed |
oskogstad
changed the title
oskogstad
deleted the
feat/add-update-event-throttling-on-gql-subscription
branch
🤖 I have created a release *beep* *boop* --- ## [1.20.2](v1.20.1...v1.20.2) (2024-10-02) ### Bug Fixes * (webAPI): Add revision to search dto (ServiceOwner) ([#1216](#1216)) ([3b6d130](3b6d130)) * **graphQL:** GraphQL subscription not notified on DialogActivityCreated ([#1187](#1187)) ([f28e291](f28e291)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Removes limitations on creating DialogUpdatedEvent
(They where not created if the only thing changed in the aggregate was added activities)
We now create DialogUpdatedEvent for any change in the aggregate. When processing events after a successful DB save we check for either a DialogUpdated or DialogDeleted per dialogID, and notify each dialogEvents GraphQL topic.
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit