eventbus fixed #40
eventbus fixed #40
Conversation
Added deserialization logic for the ExtraProperties field in both EfCoreInboxStore and EfCoreOutboxStore to properly load additional event metadata from the database. Also updated logging in OutboxProcessor to use LogInformation for successful message publication.
…ing-read-from-database-in-leasebatchasync-methods Handle ExtraProperties in inbox and outbox stores
Reviewer's GuideExtends inbox/outbox EF Core stores to persist and hydrate ExtraProperties from the database JSON column, and adjusts outbox processing logging level for successful publishes. ER diagram for inbox/outbox ExtraProperties JSON columnserDiagram
OutboxMessages {
uuid Id
int Status
varchar EventName
text EventData
timestamp CreatedAt
timestamp ProcessedAt
varchar LockedBy
timestamp LockedUntil
text LastError
int RetryCount
timestamp NextRetryAt
jsonb ExtraProperties
}
InboxMessages {
uuid Id
int Status
varchar EventName
text EventData
timestamp CreatedAt
timestamp HandledTime
varchar LockedBy
timestamp LockedUntil
int RetryCount
timestamp NextRetryTime
jsonb ExtraProperties
}
Class diagram for EF Core inbox/outbox stores ExtraProperties supportclassDiagram
class EfCoreOutboxStore {
+Task<List<OutboxMessage>> GetOutboxMessagesAsync(int batchSize, CancellationToken cancellationToken)
-void AddParameter(DbCommand command, string name, object value)
-Dictionary~string, object~ DeserializeExtraProperties(DbDataReader reader)
}
class EfCoreInboxStore {
+Task<List<InboxMessage>> GetInboxMessagesAsync(int batchSize, CancellationToken cancellationToken)
-void AddParameter(DbCommand command, string name, object value)
-Dictionary~string, object~ DeserializeExtraProperties(DbDataReader reader)
}
class OutboxMessage {
+Guid Id
+int Status
+string EventName
+string EventData
+DateTime CreatedAt
+DateTime? ProcessedAt
+string LockedBy
+DateTime? LockedUntil
+string LastError
+int RetryCount
+DateTime? NextRetryAt
+Dictionary~string, object~ ExtraProperties
}
class InboxMessage {
+Guid Id
+int Status
+string EventName
+string EventData
+DateTime CreatedAt
+DateTime? HandledTime
+string LockedBy
+DateTime? LockedUntil
+int RetryCount
+DateTime? NextRetryTime
+Dictionary~string, object~ ExtraProperties
}
EfCoreOutboxStore --> OutboxMessage : hydrates
EfCoreInboxStore --> InboxMessage : hydrates
Flow diagram for DeserializeExtraProperties helperflowchart TD
A[Read ExtraProperties column from DbDataReader] --> B{Is column DB null?}
B -- Yes --> C[Return new empty Dictionary]
B -- No --> D[Get string value as json]
D --> E{Is json null, whitespace, or empty JSON object?}
E -- Yes --> C
E -- No --> F[JsonSerializer.Deserialize to Dictionary]
F --> G{Deserialize result is null?}
G -- Yes --> C
G -- No --> H[Return deserialized Dictionary]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. Note
|
| Cohort / File(s) | Summary |
|---|---|
ExtraProperties JSON Support framework/src/BBT.Aether.Infrastructure/BBT/Aether/Events/EfCoreInboxStore.cs, framework/src/BBT.Aether.Infrastructure/BBT/Aether/Events/EfCoreOutboxStore.cs |
Both files introduce ExtraProperties as a JSON-serialized dictionary. A new DeserializeExtraProperties helper method converts JSON from the database into Dictionary<string, object>. SQL RETURN clauses updated to include the ExtraProperties column. OutboxStore populates ExtraProperties with metadata (TopicName, Version, Source, Subject), and OutboxMessage domain class gains a public ExtraProperties property. |
Logging Level Adjustment framework/src/BBT.Aether.Infrastructure/BBT/Aether/Events/Processing/OutboxProcessor.cs |
Changes log level for successful outbox message publishing from Debug to Information. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
🐰 With extra properties stored, our metadata's blessed,
JSON-serialized secrets in dictionaries rest,
From database to domain, the deserialization flows,
More info in the outbox—that's how our system grows! 📦✨
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
master
📜 Recent review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
framework/src/BBT.Aether.Infrastructure/BBT/Aether/Events/EfCoreInboxStore.csframework/src/BBT.Aether.Infrastructure/BBT/Aether/Events/EfCoreOutboxStore.csframework/src/BBT.Aether.Infrastructure/BBT/Aether/Events/Processing/OutboxProcessor.cs
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 @coderabbitai help to get the list of available commands and usage tips.
Summary of ChangesHello @yilmaztayfun, I'm Gemini Code Assist1! 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 resolves an issue where the 'ExtraProperties' associated with event messages were not being properly loaded from the database within the event bus infrastructure. The changes ensure that these crucial properties are correctly retrieved and deserialized for both incoming and outgoing messages, improving data integrity and completeness. Additionally, a minor logging verbosity adjustment was made for outbox message processing to provide more informative feedback. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to add support for ExtraProperties in the inbox and outbox stores and adjusts a log level. While the intent is good, the implementation for deserializing ExtraProperties introduces a critical issue. The use of System.Text.Json to deserialize into a Dictionary<string, object> results in values being JsonElement instances instead of primitive types, which will likely cause bugs in downstream consumers. Additionally, the new deserialization logic is duplicated in EfCoreInboxStore.cs and EfCoreOutboxStore.cs, which violates the DRY principle and should be refactored into a shared utility. The log level change from Debug to Information for successful outbox processing is a reasonable enhancement.
| private static Dictionary<string, object> DeserializeExtraProperties(DbDataReader reader) | ||
| { | ||
| var ordinal = reader.GetOrdinal("ExtraProperties"); | ||
| if (reader.IsDBNull(ordinal)) | ||
| return new Dictionary<string, object>(); | ||
|
|
||
| var json = reader.GetString(ordinal); | ||
| if (string.IsNullOrWhiteSpace(json) || json == "{}") | ||
| return new Dictionary<string, object>(); | ||
|
|
||
| return JsonSerializer.Deserialize<Dictionary<string, object>>(json) | ||
| ?? new Dictionary<string, object>(); | ||
| } |
There was a problem hiding this comment.
This new helper method has a few issues that should be addressed:
-
Critical Bug:
JsonSerializer.Deserialize<Dictionary<string, object>>(json)will deserialize JSON values intoSystem.Text.Json.JsonElementobjects, not primitive types. For example, a JSON string"my-value"becomes aJsonElementwhoseToString()method returns"my-value"(including the quotes). This will likely cause issues in consumer code likeOutboxProcessorwhich expects primitive values. You should ensure values are converted to their primitive types after deserialization. -
Code Duplication: This method is identical to the one added in
EfCoreOutboxStore.cs. To adhere to the DRY principle, this logic should be extracted into a shared utility class. -
Redundancy: The check
json == "{}"on line 252 is redundant.JsonSerializer.Deserializecorrectly handles an empty JSON object"{}"by returning an empty dictionary. You can simplify the condition tostring.IsNullOrWhiteSpace(json).
| private static Dictionary<string, object> DeserializeExtraProperties(DbDataReader reader) | ||
| { | ||
| var ordinal = reader.GetOrdinal("ExtraProperties"); | ||
| if (reader.IsDBNull(ordinal)) | ||
| return new Dictionary<string, object>(); | ||
|
|
||
| var json = reader.GetString(ordinal); | ||
| if (string.IsNullOrWhiteSpace(json) || json == "{}") | ||
| return new Dictionary<string, object>(); | ||
|
|
||
| return JsonSerializer.Deserialize<Dictionary<string, object>>(json) | ||
| ?? new Dictionary<string, object>(); | ||
| } |
There was a problem hiding this comment.
This new helper method has a few issues that should be addressed:
-
Critical Bug:
JsonSerializer.Deserialize<Dictionary<string, object>>(json)will deserialize JSON values intoSystem.Text.Json.JsonElementobjects, not primitive types. For example, a JSON string"my-value"becomes aJsonElementwhoseToString()method returns"my-value"(including the quotes). This will likely cause issues in consumer code likeOutboxProcessorwhich expects primitive values. You should ensure values are converted to their primitive types after deserialization. -
Code Duplication: This method is identical to the one added in
EfCoreInboxStore.cs. To adhere to the DRY principle, this logic should be extracted into a shared utility class. -
Redundancy: The check
json == "{}"on line 168 is redundant.JsonSerializer.Deserializecorrectly handles an empty JSON object"{}"by returning an empty dictionary. You can simplify the condition tostring.IsNullOrWhiteSpace(json).
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- Consider extracting the duplicated
DeserializeExtraPropertieslogic in the inbox and outbox stores into a shared helper to keep behavior consistent and reduce repetition. JsonSerializer.Deserialize<Dictionary<string, object>>will throw on malformed JSON, so you may want to wrap this in a try/catch and fall back to an empty dictionary to avoid breaking message fetching if the payload is corrupted.- The log level change from Debug to Information for successful outbox publishes may significantly increase log volume; verify this aligns with your logging strategy for high-throughput paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the duplicated `DeserializeExtraProperties` logic in the inbox and outbox stores into a shared helper to keep behavior consistent and reduce repetition.
- `JsonSerializer.Deserialize<Dictionary<string, object>>` will throw on malformed JSON, so you may want to wrap this in a try/catch and fall back to an empty dictionary to avoid breaking message fetching if the payload is corrupted.
- The log level change from Debug to Information for successful outbox publishes may significantly increase log volume; verify this aligns with your logging strategy for high-throughput paths.
## Individual Comments
### Comment 1
<location> `framework/src/BBT.Aether.Infrastructure/BBT/Aether/Events/EfCoreOutboxStore.cs:161` </location>
<code_context>
command.Parameters.Add(parameter);
}
+
+ private static Dictionary<string, object> DeserializeExtraProperties(DbDataReader reader)
+ {
+ var ordinal = reader.GetOrdinal("ExtraProperties");
</code_context>
<issue_to_address>
**suggestion:** The `DeserializeExtraProperties` helper is duplicated in both inbox and outbox stores and could be centralized.
The `DeserializeExtraProperties(DbDataReader)` implementation is duplicated in `EfCoreOutboxStore` and `EfCoreInboxStore`. Extract this into a shared helper (e.g., static utility or extension method) to remove duplication and keep deserialization behavior aligned across both stores.
Suggested implementation:
```csharp
: reader.GetDateTime(reader.GetOrdinal("NextRetryAt")),
ExtraProperties = MessageStoreSerializationHelper.DeserializeExtraProperties(reader)
};
messages.Add(message);
parameter.Value = value ?? DBNull.Value;
command.Parameters.Add(parameter);
}
```
To fully implement the suggestion and remove duplication:
1. Create a shared helper class (e.g., `MessageStoreSerializationHelper`) in a common namespace, for example:
- `framework/src/BBT.Aether.Infrastructure/BBT/Aether/Events/MessageStoreSerializationHelper.cs`
- Contents:
```csharp
using System;
using System.Collections.Generic;
using System.Data.Common;
using System.Text.Json;
namespace BBT.Aether.Events
{
internal static class MessageStoreSerializationHelper
{
public static Dictionary<string, object> DeserializeExtraProperties(DbDataReader reader)
{
var ordinal = reader.GetOrdinal("ExtraProperties");
if (reader.IsDBNull(ordinal))
return new Dictionary<string, object>();
var json = reader.GetString(ordinal);
if (string.IsNullOrWhiteSpace(json) || json == "{}")
return new Dictionary<string, object>();
return JsonSerializer.Deserialize<Dictionary<string, object>>(json)
?? new Dictionary<string, object>();
}
}
}
```
2. Add the appropriate `using` statement to both `EfCoreOutboxStore.cs` and `EfCoreInboxStore.cs`, e.g.:
```csharp
using BBT.Aether.Events;
```
3. In `EfCoreInboxStore.cs`, replace its local `DeserializeExtraProperties(DbDataReader)` method with calls to `MessageStoreSerializationHelper.DeserializeExtraProperties(reader)` and remove the duplicated method body, mirroring the change done in `EfCoreOutboxStore.cs`.
</issue_to_address>
### Comment 2
<location> `framework/src/BBT.Aether.Infrastructure/BBT/Aether/Events/EfCoreOutboxStore.cs:171-172` </location>
<code_context>
+ if (string.IsNullOrWhiteSpace(json) || json == "{}")
+ return new Dictionary<string, object>();
+
+ return JsonSerializer.Deserialize<Dictionary<string, object>>(json)
+ ?? new Dictionary<string, object>();
+ }
}
</code_context>
<issue_to_address>
**issue (bug_risk):** JSON deserialization of `ExtraProperties` can throw on malformed data, potentially breaking message processing.
Since `JsonSerializer.Deserialize` will throw on malformed or unexpected JSON, any bad `ExtraProperties` payload will currently abort `DeserializeExtraProperties` and may fail the whole batch. If `ExtraProperties` is meant to be best-effort metadata, consider catching deserialization exceptions, returning an empty dictionary as a fallback, and optionally logging the issue at a low log level so processing can continue.
</issue_to_address>
### Comment 3
<location> `framework/src/BBT.Aether.Infrastructure/BBT/Aether/Events/EfCoreInboxStore.cs:255-256` </location>
<code_context>
+ if (string.IsNullOrWhiteSpace(json) || json == "{}")
+ return new Dictionary<string, object>();
+
+ return JsonSerializer.Deserialize<Dictionary<string, object>>(json)
+ ?? new Dictionary<string, object>();
+ }
}
</code_context>
<issue_to_address>
**suggestion:** Apply the same defensive handling of malformed `ExtraProperties` JSON in the inbox store as suggested for the outbox.
Since this uses the same `DeserializeExtraProperties` logic, mirror any changes you make to the outbox helper here (exception handling, logging, serializer options) so malformed `ExtraProperties` JSON is handled consistently on both paths.
Suggested implementation:
```csharp
private static Dictionary<string, object> DeserializeExtraProperties(DbDataReader reader)
{
var ordinal = reader.GetOrdinal("ExtraProperties");
if (reader.IsDBNull(ordinal))
return new Dictionary<string, object>();
var json = reader.GetString(ordinal);
if (string.IsNullOrWhiteSpace(json) || json == "{}")
return new Dictionary<string, object>();
try
{
var result = JsonSerializer.Deserialize<Dictionary<string, object>>(json);
return result ?? new Dictionary<string, object>();
}
catch (JsonException)
{
// Malformed JSON in ExtraProperties should not break inbox processing.
// Treat it as empty to mirror outbox behavior.
return new Dictionary<string, object>();
}
}
```
1. Ensure `using System.Text.Json;` is present at the top of `EfCoreInboxStore.cs` (or wherever the other usings are declared) so `JsonSerializer` and `JsonException` resolve correctly.
2. To fully satisfy your comment about mirroring outbox behavior (including logging and serializer options), copy the exact `DeserializeExtraProperties` implementation from the outbox store (likely `EfCoreOutboxStore.cs`) into this inbox helper:
- Use the same `JsonSerializerOptions` instance (shared/static or injected) that is used in the outbox, instead of the default `JsonSerializer.Deserialize` overload if the outbox uses custom options.
- Use the same logging pattern as the outbox (e.g., `_logger.LogWarning(...)` or similar) inside the `catch (JsonException)` block so malformed JSON is logged consistently on both paths.
- If the outbox helper is refactored into a shared helper method or utility class, change this inbox method to delegate to that shared helper instead of duplicating the logic, to keep behavior in sync.
</issue_to_address>
### Comment 4
<location> `framework/src/BBT.Aether.Infrastructure/BBT/Aether/Events/Processing/OutboxProcessor.cs:100` </location>
<code_context>
}
- logger.LogDebug("Successfully published outbox message {MessageId}", message.Id);
+ logger.LogInformation("Successfully published outbox message {MessageId}", message.Id);
}
catch (Exception ex)
</code_context>
<issue_to_address>
**question (performance):** Raising this log to Information may generate a high volume of logs in busy systems.
If this is mainly for troubleshooting, consider keeping it at `Debug`, or make the level configurable/sampled so production environments aren’t flooded with routine success logs and associated storage/throughput costs.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| command.Parameters.Add(parameter); | ||
| } | ||
|
|
||
| private static Dictionary<string, object> DeserializeExtraProperties(DbDataReader reader) |
There was a problem hiding this comment.
suggestion: The DeserializeExtraProperties helper is duplicated in both inbox and outbox stores and could be centralized.
The DeserializeExtraProperties(DbDataReader) implementation is duplicated in EfCoreOutboxStore and EfCoreInboxStore. Extract this into a shared helper (e.g., static utility or extension method) to remove duplication and keep deserialization behavior aligned across both stores.
Suggested implementation:
: reader.GetDateTime(reader.GetOrdinal("NextRetryAt")),
ExtraProperties = MessageStoreSerializationHelper.DeserializeExtraProperties(reader)
};
messages.Add(message);
parameter.Value = value ?? DBNull.Value;
command.Parameters.Add(parameter);
}To fully implement the suggestion and remove duplication:
-
Create a shared helper class (e.g.,
MessageStoreSerializationHelper) in a common namespace, for example:framework/src/BBT.Aether.Infrastructure/BBT/Aether/Events/MessageStoreSerializationHelper.cs- Contents:
using System; using System.Collections.Generic; using System.Data.Common; using System.Text.Json; namespace BBT.Aether.Events { internal static class MessageStoreSerializationHelper { public static Dictionary<string, object> DeserializeExtraProperties(DbDataReader reader) { var ordinal = reader.GetOrdinal("ExtraProperties"); if (reader.IsDBNull(ordinal)) return new Dictionary<string, object>(); var json = reader.GetString(ordinal); if (string.IsNullOrWhiteSpace(json) || json == "{}") return new Dictionary<string, object>(); return JsonSerializer.Deserialize<Dictionary<string, object>>(json) ?? new Dictionary<string, object>(); } } }
-
Add the appropriate
usingstatement to bothEfCoreOutboxStore.csandEfCoreInboxStore.cs, e.g.:using BBT.Aether.Events;
-
In
EfCoreInboxStore.cs, replace its localDeserializeExtraProperties(DbDataReader)method with calls toMessageStoreSerializationHelper.DeserializeExtraProperties(reader)and remove the duplicated method body, mirroring the change done inEfCoreOutboxStore.cs.
| return JsonSerializer.Deserialize<Dictionary<string, object>>(json) | ||
| ?? new Dictionary<string, object>(); |
There was a problem hiding this comment.
issue (bug_risk): JSON deserialization of ExtraProperties can throw on malformed data, potentially breaking message processing.
Since JsonSerializer.Deserialize will throw on malformed or unexpected JSON, any bad ExtraProperties payload will currently abort DeserializeExtraProperties and may fail the whole batch. If ExtraProperties is meant to be best-effort metadata, consider catching deserialization exceptions, returning an empty dictionary as a fallback, and optionally logging the issue at a low log level so processing can continue.
| return JsonSerializer.Deserialize<Dictionary<string, object>>(json) | ||
| ?? new Dictionary<string, object>(); |
There was a problem hiding this comment.
suggestion: Apply the same defensive handling of malformed ExtraProperties JSON in the inbox store as suggested for the outbox.
Since this uses the same DeserializeExtraProperties logic, mirror any changes you make to the outbox helper here (exception handling, logging, serializer options) so malformed ExtraProperties JSON is handled consistently on both paths.
Suggested implementation:
private static Dictionary<string, object> DeserializeExtraProperties(DbDataReader reader)
{
var ordinal = reader.GetOrdinal("ExtraProperties");
if (reader.IsDBNull(ordinal))
return new Dictionary<string, object>();
var json = reader.GetString(ordinal);
if (string.IsNullOrWhiteSpace(json) || json == "{}")
return new Dictionary<string, object>();
try
{
var result = JsonSerializer.Deserialize<Dictionary<string, object>>(json);
return result ?? new Dictionary<string, object>();
}
catch (JsonException)
{
// Malformed JSON in ExtraProperties should not break inbox processing.
// Treat it as empty to mirror outbox behavior.
return new Dictionary<string, object>();
}
}- Ensure
using System.Text.Json;is present at the top ofEfCoreInboxStore.cs(or wherever the other usings are declared) soJsonSerializerandJsonExceptionresolve correctly. - To fully satisfy your comment about mirroring outbox behavior (including logging and serializer options), copy the exact
DeserializeExtraPropertiesimplementation from the outbox store (likelyEfCoreOutboxStore.cs) into this inbox helper:- Use the same
JsonSerializerOptionsinstance (shared/static or injected) that is used in the outbox, instead of the defaultJsonSerializer.Deserializeoverload if the outbox uses custom options. - Use the same logging pattern as the outbox (e.g.,
_logger.LogWarning(...)or similar) inside thecatch (JsonException)block so malformed JSON is logged consistently on both paths. - If the outbox helper is refactored into a shared helper method or utility class, change this inbox method to delegate to that shared helper instead of duplicating the logic, to keep behavior in sync.
- Use the same
| } | ||
|
|
||
| logger.LogDebug("Successfully published outbox message {MessageId}", message.Id); | ||
| logger.LogInformation("Successfully published outbox message {MessageId}", message.Id); |
There was a problem hiding this comment.
question (performance): Raising this log to Information may generate a high volume of logs in busy systems.
If this is mainly for troubleshooting, consider keeping it at Debug, or make the level configurable/sampled so production environments aren’t flooded with routine success logs and associated storage/throughput costs.
Summary by Sourcery
Handle extra event properties for inbox and outbox messages and adjust logging for successful outbox processing.
New Features:
Enhancements:
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.