Handle ExtraProperties in inbox and outbox stores #39
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.
Reviewer's GuideAdds JSON deserialization of ExtraProperties for inbox/outbox EF Core stores so that metadata is loaded from the DB, and raises the log level to Information for successful outbox message publication. Sequence diagram for outbox message retrieval with ExtraProperties deserializationsequenceDiagram
actor Application
participant OutboxProcessor
participant EfCoreOutboxStore
participant DbConnection
participant DbCommand
participant DbDataReader
Application->>OutboxProcessor: ProcessOutboxMessagesAsync(cancellationToken)
OutboxProcessor->>EfCoreOutboxStore: GetUnprocessedMessagesAsync(batchSize, cancellationToken)
EfCoreOutboxStore->>DbConnection: CreateCommand()
EfCoreOutboxStore->>DbCommand: Configure SQL with RETURNING ExtraProperties
EfCoreOutboxStore->>DbCommand: AddParameter(@processing)
EfCoreOutboxStore->>DbCommand: AddParameter(@ready)
EfCoreOutboxStore->>DbCommand: AddParameter(@batchSize)
EfCoreOutboxStore->>DbDataReader: ExecuteReaderAsync()
loop For each row
DbDataReader-->>EfCoreOutboxStore: Read row
EfCoreOutboxStore->>EfCoreOutboxStore: DeserializeExtraProperties(reader)
EfCoreOutboxStore-->>OutboxProcessor: OutboxMessage with ExtraProperties
end
loop For each OutboxMessage
OutboxProcessor->>OutboxProcessor: Publish message
OutboxProcessor->>OutboxProcessor: Update status, clear lock
OutboxProcessor->>OutboxProcessor: LogInformation("Successfully published outbox message {MessageId}")
end
OutboxProcessor-->>Application: Completed
Class diagram for inbox/outbox EF Core stores ExtraProperties handlingclassDiagram
class EfCoreOutboxStore {
+Task<List<OutboxMessage>> GetUnprocessedMessagesAsync(int batchSize, CancellationToken cancellationToken)
+void AddParameter(DbCommand command, string name, object value)
-static Dictionary~string, object~ DeserializeExtraProperties(DbDataReader reader)
}
class EfCoreInboxStore {
+Task<List<IncomingEvent>> GetUnprocessedMessagesAsync(int batchSize, CancellationToken cancellationToken)
+void AddParameter(DbCommand command, string name, object value)
-static 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 IncomingEvent {
+Guid Id
+int Status
+string EventName
+string EventData
+DateTime CreatedAt
+DateTime? HandledTime
+string LockedBy
+DateTime? LockedUntil
+int RetryCount
+DateTime? NextRetryTime
+Dictionary~string, object~ ExtraProperties
}
class DbDataReader {
+int GetOrdinal(string name)
+bool IsDBNull(int ordinal)
+string GetString(int ordinal)
}
class JsonSerializer {
+static Dictionary~string, object~ Deserialize(Dictionary~string, object~ value)
}
EfCoreOutboxStore --> OutboxMessage : materializes
EfCoreOutboxStore --> DbDataReader : reads from
EfCoreOutboxStore --> JsonSerializer : uses
EfCoreInboxStore --> IncomingEvent : materializes
EfCoreInboxStore --> DbDataReader : reads from
EfCoreInboxStore --> JsonSerializer : uses
OutboxMessage --> "1" Dictionary~string, object~ : ExtraProperties
IncomingEvent --> "1" Dictionary~string, object~ : ExtraProperties
File-Level Changes
Assessment against linked issues
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 Deserialization Support framework/src/BBT.Aether.Infrastructure/BBT/Aether/Events/EfCoreInboxStore.cs, EfCoreOutboxStore.cs |
Added DeserializeExtraProperties() helper methods to parse JSON-serialized ExtraProperties from the database reader. Extended batch lease queries' RETURNING/SELECT clauses to include ExtraProperties columns. Modified materialization logic for both LeaseBatchAsync and pending retrieval paths to populate ExtraProperties on message instances. Added System.Text.Json using directive. |
Logging Enhancement framework/src/BBT.Aether.Infrastructure/BBT/Aether/Events/Processing/OutboxProcessor.cs |
Changed logging level for successful outbox message publication from Debug to Information. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
🐰 With whiskers twitched and eager hops,
We parse the properties that JSON drops,
ExtraFields now dance in morning light,
And logs ring clear, both loud and bright!
Hop, hop, hooray—our messages take flight! ✨
✨ 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
38-fix-extraproperties-not-being-read-from-database-in-leasebatchasync-methods
📜 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 enhances the event processing infrastructure by ensuring that additional event metadata, stored in the 'ExtraProperties' field, is correctly deserialized and loaded when retrieving messages from both the inbox and outbox stores. This change improves data integrity and completeness for event handling. Additionally, it refines the logging for successful outbox message processing to a more appropriate information level. 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
|
![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 2 issues, and left some high level feedback:
- The
DeserializeExtraPropertieshelper is duplicated in both inbox and outbox stores; consider extracting this into a shared utility or base class method to avoid duplication and keep serialization behavior consistent. - Deserializing
ExtraPropertiesintoDictionary<string, object>usingSystem.Text.Jsonwill yieldJsonElementinstances as values by default; if callers expect concrete primitive types, consider using a custom converter or aDictionary<string, JsonElement>(or a dedicated DTO) to avoid runtime surprises.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `DeserializeExtraProperties` helper is duplicated in both inbox and outbox stores; consider extracting this into a shared utility or base class method to avoid duplication and keep serialization behavior consistent.
- Deserializing `ExtraProperties` into `Dictionary<string, object>` using `System.Text.Json` will yield `JsonElement` instances as values by default; if callers expect concrete primitive types, consider using a custom converter or a `Dictionary<string, JsonElement>` (or a dedicated DTO) to avoid runtime surprises.
## Individual Comments
### Comment 1
<location> `framework/src/BBT.Aether.Infrastructure/BBT/Aether/Events/EfCoreOutboxStore.cs:144` </location>
<code_context>
? null
: reader.GetDateTime(reader.GetOrdinal("NextRetryTime")),
- ExtraProperties = new Dictionary<string, object>()
+ ExtraProperties = DeserializeExtraProperties(reader)
};
</code_context>
<issue_to_address>
**suggestion (bug_risk):** JSON deserialization from the DB can throw and currently isn’t guarded, which may break outbox processing for a single bad record.
If `ExtraProperties` contains malformed JSON, `JsonSerializer.Deserialize` will throw and stop the whole batch. Consider catching `JsonException` in `DeserializeExtraProperties`, logging it, and returning an empty dictionary so a single corrupt row doesn’t block processing.
Suggested implementation:
```csharp
private Dictionary<string, object> DeserializeExtraProperties(DbDataReader reader)
{
var extraPropertiesOrdinal = reader.GetOrdinal("ExtraProperties");
if (reader.IsDBNull(extraPropertiesOrdinal))
{
return new Dictionary<string, object>();
}
var json = reader.GetString(extraPropertiesOrdinal);
if (string.IsNullOrWhiteSpace(json))
{
return new Dictionary<string, object>();
}
try
{
var extraProperties =
JsonSerializer.Deserialize<Dictionary<string, object>>(json);
return extraProperties ?? new Dictionary<string, object>();
}
catch (JsonException ex)
{
_logger?.LogWarning(
ex,
"Failed to deserialize ExtraProperties for outbox message. Raw value: {ExtraPropertiesJson}",
json);
return new Dictionary<string, object>();
}
}
```
The above edit assumes:
1. There is already a `DeserializeExtraProperties(DbDataReader reader)` method matching the `SEARCH` block.
2. The class has an `_logger` field of type `ILogger<EfCoreOutboxStore>` (or compatible).
If these assumptions do not match your file:
- Remove `static` from the existing `DeserializeExtraProperties` definition (if it is static) so `_logger` is accessible, or adapt logging to your existing logging mechanism.
- Ensure `using System.Text.Json;` and `using Microsoft.Extensions.Logging;` are present at the top of the file.
- If `_logger` does not exist, inject an `ILogger<EfCoreOutboxStore>` into the class (constructor) and assign it to a private readonly `_logger` field, or replace the `_logger?.LogWarning(...)` call with your project's standard logging API.
</issue_to_address>
### Comment 2
<location> `framework/src/BBT.Aether.Infrastructure/BBT/Aether/Events/EfCoreInboxStore.cs:228` </location>
<code_context>
? null
: reader.GetDateTime(reader.GetOrdinal("NextRetryTime")),
- ExtraProperties = new Dictionary<string, object>()
+ ExtraProperties = DeserializeExtraProperties(reader)
};
</code_context>
<issue_to_address>
**suggestion:** The inbox and outbox stores now duplicate identical JSON deserialization logic that could be centralized.
`EfCoreInboxStore` and `EfCoreOutboxStore` both implement `DeserializeExtraProperties` identically. Please extract this into a shared helper (for example, an internal static utility in the same namespace) to remove duplication and keep JSON handling consistent across both stores.
Suggested implementation:
```csharp
ExtraProperties = EventStoreDataReaderHelper.DeserializeExtraProperties(reader)
```
```csharp
parameter.Value = value ?? DBNull.Value;
}
internal static class EventStoreDataReaderHelper
{
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))
{
return new Dictionary<string, object>();
}
Dictionary<string, object>? result;
try
{
result = JsonSerializer.Deserialize<Dictionary<string, object>>(json);
}
catch (JsonException)
{
// In case of malformed JSON, fallback to empty dictionary to avoid breaking processing.
result = null;
}
return result ?? new Dictionary<string, object>();
}
```
1. Ensure the following `using` directives exist at the top of this file (or are otherwise available in the project):
- `using System.Collections.Generic;`
- `using System.Data.Common;`
- `using System.Text.Json;`
2. In `EfCoreOutboxStore` (or the corresponding outbox store file), replace its local `DeserializeExtraProperties` implementation with calls to `EventStoreDataReaderHelper.DeserializeExtraProperties(reader)` so that both stores share the same deserialization logic.
3. Remove any now-unused instance methods named `DeserializeExtraProperties` from both `EfCoreInboxStore` and `EfCoreOutboxStore` once they are fully migrated to the shared helper.
4. If the project uses a custom `JsonSerializerOptions` (e.g., a shared options instance for polymorphic support or naming policies), adapt the `JsonSerializer.Deserialize` call to use that options object instead of the default.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ? null | ||
| : reader.GetDateTime(reader.GetOrdinal("NextRetryAt")) | ||
| : reader.GetDateTime(reader.GetOrdinal("NextRetryAt")), | ||
| ExtraProperties = DeserializeExtraProperties(reader) |
There was a problem hiding this comment.
suggestion (bug_risk): JSON deserialization from the DB can throw and currently isn’t guarded, which may break outbox processing for a single bad record.
If ExtraProperties contains malformed JSON, JsonSerializer.Deserialize will throw and stop the whole batch. Consider catching JsonException in DeserializeExtraProperties, logging it, and returning an empty dictionary so a single corrupt row doesn’t block processing.
Suggested implementation:
private Dictionary<string, object> DeserializeExtraProperties(DbDataReader reader)
{
var extraPropertiesOrdinal = reader.GetOrdinal("ExtraProperties");
if (reader.IsDBNull(extraPropertiesOrdinal))
{
return new Dictionary<string, object>();
}
var json = reader.GetString(extraPropertiesOrdinal);
if (string.IsNullOrWhiteSpace(json))
{
return new Dictionary<string, object>();
}
try
{
var extraProperties =
JsonSerializer.Deserialize<Dictionary<string, object>>(json);
return extraProperties ?? new Dictionary<string, object>();
}
catch (JsonException ex)
{
_logger?.LogWarning(
ex,
"Failed to deserialize ExtraProperties for outbox message. Raw value: {ExtraPropertiesJson}",
json);
return new Dictionary<string, object>();
}
}The above edit assumes:
- There is already a
DeserializeExtraProperties(DbDataReader reader)method matching theSEARCHblock. - The class has an
_loggerfield of typeILogger<EfCoreOutboxStore>(or compatible).
If these assumptions do not match your file:
- Remove
staticfrom the existingDeserializeExtraPropertiesdefinition (if it is static) so_loggeris accessible, or adapt logging to your existing logging mechanism. - Ensure
using System.Text.Json;andusing Microsoft.Extensions.Logging;are present at the top of the file. - If
_loggerdoes not exist, inject anILogger<EfCoreOutboxStore>into the class (constructor) and assign it to a private readonly_loggerfield, or replace the_logger?.LogWarning(...)call with your project's standard logging API.
| ? null | ||
| : reader.GetDateTime(reader.GetOrdinal("NextRetryTime")), | ||
| ExtraProperties = new Dictionary<string, object>() | ||
| ExtraProperties = DeserializeExtraProperties(reader) |
There was a problem hiding this comment.
suggestion: The inbox and outbox stores now duplicate identical JSON deserialization logic that could be centralized.
EfCoreInboxStore and EfCoreOutboxStore both implement DeserializeExtraProperties identically. Please extract this into a shared helper (for example, an internal static utility in the same namespace) to remove duplication and keep JSON handling consistent across both stores.
Suggested implementation:
ExtraProperties = EventStoreDataReaderHelper.DeserializeExtraProperties(reader) parameter.Value = value ?? DBNull.Value;
}
internal static class EventStoreDataReaderHelper
{
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))
{
return new Dictionary<string, object>();
}
Dictionary<string, object>? result;
try
{
result = JsonSerializer.Deserialize<Dictionary<string, object>>(json);
}
catch (JsonException)
{
// In case of malformed JSON, fallback to empty dictionary to avoid breaking processing.
result = null;
}
return result ?? new Dictionary<string, object>();
}- Ensure the following
usingdirectives exist at the top of this file (or are otherwise available in the project):using System.Collections.Generic;using System.Data.Common;using System.Text.Json;
- In
EfCoreOutboxStore(or the corresponding outbox store file), replace its localDeserializeExtraPropertiesimplementation with calls toEventStoreDataReaderHelper.DeserializeExtraProperties(reader)so that both stores share the same deserialization logic. - Remove any now-unused instance methods named
DeserializeExtraPropertiesfrom bothEfCoreInboxStoreandEfCoreOutboxStoreonce they are fully migrated to the shared helper. - If the project uses a custom
JsonSerializerOptions(e.g., a shared options instance for polymorphic support or naming policies), adapt theJsonSerializer.Deserializecall to use that options object instead of the default.
![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 introduces deserialization for ExtraProperties in the inbox and outbox stores, which is a good enhancement. However, the implementation introduces code duplication between EfCoreInboxStore and EfCoreOutboxStore. I've provided a suggestion to refactor this into a shared helper to improve maintainability. Additionally, the new deserialization logic lacks error handling for malformed JSON, which could cause processing to fail; I've included a code suggestion to make this more robust. Lastly, I've raised a point for consideration regarding a change in log level in the OutboxProcessor that might lead to excessive logging in production.
| 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 method introduces a couple of concerns:
-
Code Duplication: This logic is identical to the new method in
EfCoreOutboxStore.cs. To follow the DRY (Don't Repeat Yourself) principle, this should be extracted to a sharedinternal statichelper class. The existingAddParametermethod is also duplicated across both classes and could be moved there as well. -
Error Handling: The
JsonSerializer.Deserializecall can throw aJsonExceptionif the JSON string from the database is malformed. This would cause the entire batch processing to fail. It's safer to wrap this call in atry-catchblock and return an empty dictionary on failure, similar to the pattern inExtraPropertiesValueConverter.
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
{
return JsonSerializer.Deserialize<Dictionary<string, object>>(json)
?? new Dictionary<string, object>();
}
catch (JsonException)
{
// Consider logging this exception to aid in debugging malformed data.
return new Dictionary<string, object>();
}
}| 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 method is a duplicate of the one added to EfCoreInboxStore.cs. Please see my comment on that file for suggestions regarding error handling and refactoring to avoid code duplication.
| } | ||
|
|
||
| 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.
Changing this log level from Debug to Information might lead to excessive logging in a production environment, especially with a large batch size. This log is inside a loop that iterates over all leased messages. A LogInformation entry for every successfully published message could create a lot of noise. Consider if LogDebug is more appropriate for this fine-grained success message, or perhaps log a summary after the loop completes.
|
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.
Summary by Sourcery
Handle loading of ExtraProperties for inbox and outbox messages and adjust outbox success logging level.
New Features:
Enhancements:
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.