fix: missing Kafka message deserialization in aggregator#2275
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR introduces a typed handler and factory for advisory update Kafka messages, ensuring they are deserialized into AdvisoryUpdateEvent objects before being processed in the aggregator, and updates the advisory subscription pipeline to use the new handler signature. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="base/mqueue/advisory_update_event.go" line_range="18-19" />
<code_context>
+func MakeAdvisoryUpdateHandler(handler AdvisoryUpdateEventHandler) MessageHandler {
+ return func(m KafkaMessage) error {
+ var event AdvisoryUpdateEvent
+ err := sonic.Unmarshal(m.Value, &event)
+ if err != nil {
+ utils.LogError("err", err, "Could not deserialize advisory update event")
+ return nil
</code_context>
<issue_to_address>
**issue (bug_risk):** Swallowing deserialization errors may hide malformed messages and mark them as successfully processed.
Because nil is returned on deserialization failure, the retrying handler will treat the message as successfully processed and never retry or reroute it. If malformed messages should be retried or sent to a dead-letter topic, return an error here (ideally a wrapped, typed error that the retry logic or higher-level handler can interpret) instead of nil.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| err := sonic.Unmarshal(m.Value, &event) | ||
| if err != nil { |
There was a problem hiding this comment.
issue (bug_risk): Swallowing deserialization errors may hide malformed messages and mark them as successfully processed.
Because nil is returned on deserialization failure, the retrying handler will treat the message as successfully processed and never retry or reroute it. If malformed messages should be retried or sent to a dead-letter topic, return an error here (ideally a wrapped, typed error that the retry logic or higher-level handler can interpret) instead of nil.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2275 +/- ##
==========================================
- Coverage 58.97% 58.92% -0.06%
==========================================
Files 147 147
Lines 9293 9301 +8
==========================================
Hits 5481 5481
- Misses 3238 3246 +8
Partials 574 574
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Add
MakeAdvisoryUpdateHandlerto deserialize raw Kafka messages intoAdvisoryUpdateEventobjects, mirroring the existingMakeMessageHandlerpattern.Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Add a dedicated advisory update event handler that deserializes Kafka messages before processing and wire it into the aggregator subscription flow.
New Features:
Bug Fixes:
Enhancements: