fix: RPC event serialization error, dont send empty messages

[karel1980]

This PR

• adds a test for the notification error that I encountered in Caching problem(?) #1869

Related Issues

None that I'm aware of

Notes

vibe coding warning: I don't know go well enough to fix this, but I did use TDD and also did a manual test to verify that my problem went away.
If any part of my solution is technicaly or doesn't fit in the codebase for any reason: feel free to comment, I'll do my best to improve it.

How to test

Running the test suite should be sufficient

[netlify]

✅ Deploy Preview for polite-licorice-3db33c canceled.

Name	Link
🔨 Latest commit	c8b1e4d
🔍 Latest deploy log	https://app.netlify.com/projects/polite-licorice-3db33c/deploys/69a82b249e59ef0007d9224d

[gemini-code-assist]

Summary of Changes

Hello @karel1980, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical bug where the flag evaluation service could panic if a notification's data payload was nil during conversion to a Protobuf struct. The changes introduce robust nil checks before conversion and enhance the notification handling logic, ensuring stability and reliability of event streams. A new test has also been added to validate the correct behavior of notification data conversion.

Highlights

• Bug Fix: Nil Notification Data Panic: Implemented a fix to prevent panics when notification.Data is nil during structpb.NewStruct conversion within the event stream handlers for both OldFlagEvaluationService and FlagEvaluationService.
• New Test Case for Notification Data Conversion: Introduced a new test case (TestNotificationDataStructpbConversion) to validate the successful conversion of notification data to structpb.Struct without errors, ensuring robustness.
• Refactored Notification Data Handling: Adjusted type assertions and conversions in notification handling logic within connect_service_test.go and eventing.go for improved type safety and consistency across the codebase.

Changelog

• flagd/pkg/service/flag-evaluation/connect_service_test.go
  • Removed an unused import for notifications.
  • Updated the type assertion for notification data from notifications.Notifications to map[string]interface{}.
• flagd/pkg/service/flag-evaluation/eventing.go
  • Added an explicit type conversion map[string]interface{}(notifications) when assigning notification data.
• flagd/pkg/service/flag-evaluation/eventing_test.go
  • Imported google.golang.org/protobuf/types/known/structpb.
  • Added TestNotificationDataStructpbConversion to verify that iservice.Notification data can be converted to structpb.Struct without errors.
• flagd/pkg/service/flag-evaluation/flag_evaluator.go
  • Introduced a nil check for notification.Data before attempting structpb.NewStruct conversion to prevent panics.
• flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go
  • Introduced a nil check for notification.Data before attempting structpb.NewStruct conversion to prevent panics.

Activity

• The author identified a notification error that could lead to panics and implemented a fix.
• A new test was added to reproduce and verify the fix for the identified notification error.
• Manual testing was performed by the author to confirm the problem was resolved.
• The author encourages feedback on the solution's technical aspects or fit within the codebase.
• The author suggests running the test suite to verify the changes.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request effectively addresses a bug in notification sending by ensuring the data structure is correctly typed before protobuf conversion. The inclusion of a new test case to validate the fix is a great example of test-driven development. Additionally, the changes improve the robustness of the event stream handling by preventing potential panics from nil data and refining error management. I've included a couple of minor suggestions to enhance code readability in the event stream handlers.

[karel1980]

Changed the PR title (conventional commit)

[toddbaert]

/gemini review

[gemini-code-assist]

Code Review

This pull request fixes an error that occurred when sending notifications, by ensuring the notification data is a plain map[string]interface{} which is compatible with protobuf serialization. The change is correct and is accompanied by a regression test. I've added a couple of suggestions to further improve the code: one to avoid sending empty notifications, and another to improve test reliability by avoiding time.Sleep.

[toddbaert]

@karel1980 I made a change here to avoid sending empty notification messages entirely. If no flags are changed, there doesn't seem to be a point. I don't think this happens much in prod scenarios, to be honest.

[sonarqubecloud]

Quality Gate failed

Failed conditions
41.4% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud