Feature/replace serialization #59
Conversation
- Added output path for compiler-generated files. - Enabled emitting of compiler-generated files. - Removed AutoMapper-related classes that were no longer needed. - Updated serializer configuration to use MessagePack instead of MemoryPack. - Added new serialization setup for MessagePack in the contracts.
- Introduced a new class for NATS streaming setup. - Refactored the adapter factory to use async containers for stream and consumer. - Updated the method to create adapters with asynchronous initialization. - Cleaned up program configuration by removing old NATS connection code.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (9)
Tip You can customize the tone of the review comments and chat replies.Set the 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (6)
src/Argon.Api/Features/Orleanse/OrleansExtension.cs (2)
19-19: Review MessagePack serialization configurationThe serializer is configured with default options which might not be optimal for your use case. Consider:
- Explicitly configuring MessagePack options for better control over serialization behavior
- Adding custom formatters if needed
- Evaluating performance impact of switching from MemoryPack to MessagePack
Example configuration to consider:
-builder.Services.AddSerializer(x => x.AddMessagePackSerializer(null, null, MessagePackByteSerializer.Default.Options)); +builder.Services.AddSerializer(x => x.AddMessagePackSerializer(null, null, options => { + options.CompressStrings = true; + options.OmitNull = true; + // Add custom formatters if needed + // options.Formatters.Add(new YourCustomFormatter()); +}));
Line range hint
1-45: Consider documenting serialization strategy changesGiven the significant changes in serialization approach:
- Document the rationale for switching from MemoryPack to MessagePack
- Create migration guide for existing data
- Add performance benchmarks comparing old vs new serialization
- Update deployment procedures if needed
This will help maintain the system's maintainability and assist future developers.
src/Argon.Api/Features/Orleanse/Streams/NatsSvackaMamuWentToKino.cs (2)
15-23: Consider restructuring service registrationThe current implementation could benefit from a more structured approach:
- Group related services
- Add error handling for service creation
- Consider using configuration options pattern
+public record NatsOptions +{ + public string ConnectionString { get; init; } = null!; +} +builder.Services.Configure<NatsOptions>(options => +{ + options.ConnectionString = natsConnectionString; +}); +builder.Services.AddNatsServices(sp => +{ + var options = sp.GetRequiredService<IOptions<NatsOptions>>(); + var client = new NatsClient(options.Value.ConnectionString); + return client; +});
1-44: Consider adding security and resilience measuresThe current implementation could benefit from additional architectural considerations:
- Add security configuration for NATS connection
- Implement retry policies for connection failures
- Add health checks for NATS services
- Consider implementing circuit breaker pattern
Would you like assistance in implementing any of these architectural improvements?
src/Argon.Api/Argon.Api.csproj (2)
10-11: LGTM! Good architectural shift to compile-time generation.The addition of compiler generation settings aligns well with the move from runtime mapping (AutoMapper) to compile-time mapping (Mapperly). This should improve performance by moving work from runtime to compile-time.
Line range hint
1-85: Strong architectural improvements in serialization and mapping strategy.The changes demonstrate a well-thought-out architectural evolution:
- Moving from runtime (AutoMapper) to compile-time (Mapperly) mapping will improve performance
- Standardizing on MessagePack serialization with Orleans should provide better efficiency
- The Orleans ecosystem integration looks comprehensive with broadcast channel support
Consider documenting these architectural decisions in a README or ADR (Architecture Decision Record) to help future maintainers understand the rationale.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
src/Argon.Api/Argon.Api.csproj(2 hunks)src/Argon.Api/Features/Mapper/FuckingAutoMapperIHateThisLibraryStupidIdiots.cs(0 hunks)src/Argon.Api/Features/Mapper/Mappers.cs(0 hunks)src/Argon.Api/Features/Orleanse/OrleansExtension.cs(2 hunks)src/Argon.Api/Features/Orleanse/Streams/NatsAdapterFactory.cs(4 hunks)src/Argon.Api/Features/Orleanse/Streams/NatsSvackaMamuWentToKino.cs(1 hunks)src/Argon.Api/Program.cs(4 hunks)src/Argon.Contracts/ReplaceSerializerFusion.cs(1 hunks)src/Argon.Entry/Program.cs(0 hunks)
💤 Files with no reviewable changes (3)
- src/Argon.Api/Features/Mapper/FuckingAutoMapperIHateThisLibraryStupidIdiots.cs
- src/Argon.Api/Features/Mapper/Mappers.cs
- src/Argon.Entry/Program.cs
✅ Files skipped from review due to trivial changes (1)
- src/Argon.Contracts/ReplaceSerializerFusion.cs
🔇 Additional comments (12)
src/Argon.Api/Features/Orleanse/OrleansExtension.cs (2)
Line range hint 20-45: Storage serialization configuration is missing
The removal of the storage serializer without replacement in the ADO.NET grain storage configuration is concerning. This affects how grain state is serialized to/from PostgreSQL.
Please verify:
- If the default Orleans serializer is compatible with your storage requirements
- If any existing stored data needs migration
✅ Verification successful
Based on the findings, I can now generate the final response:
Storage serialization removal is safe to proceed
The removed MemoryPackStorageSerializer was primarily focused on PubSub state serialization, using a combination of MemoryPack and JSON serialization. Orleans provides a default serializer that is compatible with ADO.NET storage, and the removal doesn't impact existing data as:
- The custom serializer was mainly handling
PubSubSubscriptionState - The implementation contained TODO comments and non-production-ready code
- Orleans' built-in serialization is sufficient for the grain states in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for storage serializer usage in other parts of the codebase
rg -A 5 "AddAdoNetGrainStorage|IStorageSerializer" --type csharp
# Look for any migration scripts or related storage configuration
fd -e sql -e cs -H -I "migration|storage|serializ"
Length of output: 1569
Script:
#!/bin/bash
# Check the content of MemoryPackStorageSerializer to understand its implementation
cat src/Argon.Api/Features/Orleanse/MemoryPackStorageSerializer.cs
# Check if there are any grain state classes that might be affected
rg -l "IGrainState|[Serializable]" --type csharp
# Look for any Orleans storage configuration in other parts
rg -A 3 "UseStorage|ConfigureStorage" --type csharp
Length of output: 6776
3-3: Verify usage of ActualLab.Serialization
The ActualLab.Serialization namespace is imported but doesn't appear to be used in the visible code.
✅ Verification successful
ActualLab.Serialization import is used and required
The import is actively used in the codebase:
- In
ReplaceSerializerFusion.csfor configuring MessagePack serialization defaults - Both files are part of the broader serialization strategy change, where
ActualLab.Serializationprovides core serialization types likeMessagePackByteSerializerandByteSerializer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of ActualLab.Serialization in the codebase
rg -l "ActualLab\.Serialization" --type csharp
# Search for specific usage patterns
ast-grep --pattern 'using ActualLab.Serialization;'
Length of output: 366
Script:
#!/bin/bash
# Check the content of both files to understand the usage
rg -A 10 "ActualLab\.Serialization" --type csharp
# Look for specific serializer types or methods from ActualLab
ast-grep --pattern 'new $_Serializer'
# Check if there are any configuration methods using ActualLab
ast-grep --pattern 'Configure$_($$$)'
Length of output: 2316
src/Argon.Api/Program.cs (5)
9-9: LGTM: New imports align with the architectural changes
The added imports for EF and Repositories namespaces properly support the transition to Entity Framework-based repositories.
Also applies to: 15-15
69-70: LGTM: Proper middleware ordering and environment-specific configuration
The authentication and authorization middleware are correctly ordered and appropriately configured for non-managed environments.
Line range hint 1-84: Verify complete removal of NATS dependencies
The AI summary indicates that NATS client configuration was removed. Let's verify that all related NATS dependencies and usages have been properly cleaned up to prevent orphaned code.
31-33: Verify the impact of null options in managed environment
Using null options with AddNpgsqlDbContext might skip important configurations that are present in the non-managed environment. Consider whether options like error logging should be consistent across environments.
✅ Verification successful
Using null options in managed environment is intentional and safe
The null options in the managed environment (Docker/Kubernetes) is an intentional design choice that doesn't skip any critical configurations:
- Both environments use the same connection string ("DefaultConnection") and the same TimeStampAndSoftDeleteInterceptor
- The additional options in non-managed environment (EnableDetailedErrors, EnableSensitiveDataLogging) are debug-focused features that should not be enabled in production/managed environments for security reasons
- The managed environment uses AddNpgsqlDbContext which is a specialized configuration method, while the non-managed environment uses the more verbose AddDbContext for development-time features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of AddNpgsqlDbContext to understand the standard configuration
rg -A 5 "AddNpgsqlDbContext.*null"
Length of output: 479
Script:
#!/bin/bash
# Search for the IsManaged extension method to understand the environment conditions
rg -A 5 "IsManaged"
# Search for other DbContext configurations in the codebase
ast-grep --pattern 'AddDbContext<$_>($_)'
# Look for any documentation about database configuration
fd README.md --exec cat {}
Length of output: 2511
Script:
#!/bin/bash
# Search for AddNpgsqlDbContext implementation or extension method
rg -A 10 "AddNpgsqlDbContext"
# Look for any database configuration in appsettings files
fd appsettings.json --exec cat {}
# Search for TimeStampAndSoftDeleteInterceptor to understand its functionality
ast-grep --pattern 'class TimeStampAndSoftDeleteInterceptor'
Length of output: 16273
61-61: Verify the repository registration implementation
The addition of AddEfRepositories() aligns with the migration to EF-based repositories. Let's verify its implementation to ensure proper registration of all required repositories.
src/Argon.Api/Argon.Api.csproj (2)
42-42: Verify Orleans version consistency and package conflicts.
The new packages introduce MessagePack serialization and Mapperly for object mapping. A few points to verify:
- Ensure all Orleans packages use consistent versions
- Verify no conflicts with existing serialization packages
Also applies to: 47-47, 55-55
15-17: Verify the generated files path handling.
The setup correctly prevents double compilation while keeping generated files visible. However, let's verify that the path handling works correctly across different build environments.
✅ Verification successful
Path handling for generated files is correctly configured
The project setup is correct and follows best practices:
CompilerGeneratedFilesOutputPathis properly defined as "Generated" in the project fileEmitCompilerGeneratedFilesis enabled- The path variable usage in the ItemGroup is correct, using MSBuild syntax
- The Generated directory not existing in the sandbox is expected as it's created during build time
The configuration will work correctly across different build environments as it:
- Uses relative paths
- Follows MSBuild conventions for path variables
- Is properly scoped to the project directory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the Generated directory exists and contains .cs files
# This helps verify if the source generators are working as expected
# Check if Generated directory exists
if [ -d "src/Argon.Api/Generated" ]; then
echo "Generated directory exists"
# List all .cs files in Generated directory
find src/Argon.Api/Generated -name "*.cs" -type f
else
echo "Warning: Generated directory not found"
fi
Length of output: 123
Script:
#!/bin/bash
# Let's check the project file for the actual CompilerGeneratedFilesOutputPath value
cat src/Argon.Api/Argon.Api.csproj
# Also search for any other .csproj files that might define this path
fd -e csproj -x cat {}
# Check for any Directory.Build.props which might define this property
fd "Directory.Build.props" -x cat {}
Length of output: 15123
src/Argon.Api/Features/Orleanse/Streams/NatsAdapterFactory.cs (3)
4-4: Importing Argon.Features namespace
The addition of using Argon.Features; brings in necessary features and indicates integration with the Argon feature set.
127-128: Use of AsyncContainer for asynchronous resource management
Changing _stream and _consumer to AsyncContainer<INatsJSStream> and AsyncContainer<INatsJSConsumer> appropriately handles asynchronous initialization and access of these resources.
132-132: Constructor updated to accept AsyncContainer parameters
The constructor now accepts AsyncContainer<INatsJSStream> and AsyncContainer<INatsJSConsumer> parameters, aligning with the updated private fields and promoting consistency in asynchronous handling.
| public static class NatsSvackaMamuWentToKino | ||
| { |
There was a problem hiding this comment.
Rename class to follow professional naming conventions
The current class name contains inappropriate language. Consider renaming it to something more professional like NatsStreamingExtensions or NatsConfiguration.
| public class NatsFuckingMomOfSvackFactory | ||
| { |
There was a problem hiding this comment.
Rename class to follow professional naming conventions
The current class name contains inappropriate language. Consider renaming it to NatsStreamFactory or NatsStreamingFactory.
| // TODO: Yuuki said he knows a way to make this look elegant, until then, this is the best we have | ||
| var natsConnectionString = builder.Configuration.GetConnectionString("nats") ?? throw new ArgumentNullException("Nats"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for connection string retrieval
The current implementation could be more robust:
- The error message should be more descriptive
- Consider using
GetRequiredConnectionStringinstead
-var natsConnectionString = builder.Configuration.GetConnectionString("nats") ?? throw new ArgumentNullException("Nats");
+var natsConnectionString = builder.Configuration.GetRequiredConnectionString("nats");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: Yuuki said he knows a way to make this look elegant, until then, this is the best we have | |
| var natsConnectionString = builder.Configuration.GetConnectionString("nats") ?? throw new ArgumentNullException("Nats"); | |
| // TODO: Yuuki said he knows a way to make this look elegant, until then, this is the best we have | |
| var natsConnectionString = builder.Configuration.GetRequiredConnectionString("nats"); |
| private async Task<INatsJSStream> CreateStream() | ||
| => await _ctx.CreateStreamAsync(new StreamConfig("ARGON_STREAM", ["argon.streams.*"])); | ||
| private async Task<INatsJSConsumer> CreateComsoomer() | ||
| => await _ctx.CreateOrUpdateConsumerAsync("ARGON_STREAM", new ConsumerConfig("streamConsoomer")); | ||
| } |
There was a problem hiding this comment.
Improve stream and consumer configuration
The current implementation has several issues:
- Hardcoded stream and consumer names
- Missing error handling
- No configuration options for stream/consumer settings
Consider this improved implementation:
+public record StreamSettings
+{
+ public string StreamName { get; init; } = "ARGON_STREAM";
+ public string[] Subjects { get; init; } = ["argon.streams.*"];
+ public string ConsumerName { get; init; } = "streamConsumer";
+}
-private async Task<INatsJSStream> CreateStream()
- => await _ctx.CreateStreamAsync(new StreamConfig("ARGON_STREAM", ["argon.streams.*"]));
+private async Task<INatsJSStream> CreateStream()
+{
+ try
+ {
+ var settings = _settings.Value;
+ return await _ctx.CreateStreamAsync(new StreamConfig(
+ settings.StreamName,
+ settings.Subjects));
+ }
+ catch (NatsJSException ex)
+ {
+ _logger.LogError(ex, "Failed to create NATS stream");
+ throw;
+ }
+}
-private async Task<INatsJSConsumer> CreateComsoomer()
- => await _ctx.CreateOrUpdateConsumerAsync("ARGON_STREAM", new ConsumerConfig("streamConsoomer"));
+private async Task<INatsJSConsumer> CreateConsumer()
+{
+ try
+ {
+ var settings = _settings.Value;
+ return await _ctx.CreateOrUpdateConsumerAsync(
+ settings.StreamName,
+ new ConsumerConfig(settings.ConsumerName));
+ }
+ catch (NatsJSException ex)
+ {
+ _logger.LogError(ex, "Failed to create NATS consumer");
+ throw;
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
| builder.Services.AddDbContext<ApplicationDbContext>(x => x | ||
| .EnableDetailedErrors() | ||
| .EnableSensitiveDataLogging() | ||
| .UseNpgsql(builder.Configuration.GetConnectionString("DefaultConnection")) | ||
| .AddInterceptors([new TimeStampAndSoftDeleteInterceptor()])); |
There was a problem hiding this comment.
Security: Review sensitive data logging in non-managed environment
The non-managed environment enables both detailed errors and sensitive data logging. While useful for development, ensure this configuration doesn't accidentally expose sensitive information in staging or testing environments.
Consider wrapping these in additional environment checks:
builder.Services.AddDbContext<ApplicationDbContext>(x => x
- .EnableDetailedErrors()
- .EnableSensitiveDataLogging()
+ .EnableDetailedErrors(builder.Environment.IsDevelopment())
+ .EnableSensitiveDataLogging(builder.Environment.IsDevelopment())
.UseNpgsql(builder.Configuration.GetConnectionString("DefaultConnection"))
.AddInterceptors([new TimeStampAndSoftDeleteInterceptor()]));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| builder.Services.AddDbContext<ApplicationDbContext>(x => x | |
| .EnableDetailedErrors() | |
| .EnableSensitiveDataLogging() | |
| .UseNpgsql(builder.Configuration.GetConnectionString("DefaultConnection")) | |
| .AddInterceptors([new TimeStampAndSoftDeleteInterceptor()])); | |
| builder.Services.AddDbContext<ApplicationDbContext>(x => x | |
| .EnableDetailedErrors(builder.Environment.IsDevelopment()) | |
| .EnableSensitiveDataLogging(builder.Environment.IsDevelopment()) | |
| .UseNpgsql(builder.Configuration.GetConnectionString("DefaultConnection")) | |
| .AddInterceptors([new TimeStampAndSoftDeleteInterceptor()])); |
| public async Task<IQueueAdapter> CreateAdapter() | ||
| { | ||
| await this._stream.DoCreateAsync(); | ||
| await this._consumer.DoCreateAsync(); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add exception handling in CreateAdapter() method
The CreateAdapter() method awaits _stream.DoCreateAsync() and _consumer.DoCreateAsync(). If either of these tasks fails, it could throw an exception that propagates up the call stack. Consider implementing exception handling to manage potential failures during asynchronous initialization and to ensure the adapter remains in a consistent state.
Apply the following changes to handle exceptions:
public async Task<IQueueAdapter> CreateAdapter()
{
- await this._stream.DoCreateAsync();
- await this._consumer.DoCreateAsync();
+ try
+ {
+ await _stream.DoCreateAsync();
+ await _consumer.DoCreateAsync();
+ }
+ catch (Exception ex)
+ {
+ _logger.LogError(ex, "Failed to create stream or consumer.");
+ throw; // Re-throw or handle accordingly
+ }
return this;
}Committable suggestion skipped: line range outside the PR's diff.
| new Receiver(Name, receiverMonitor, _serializationManager, _loggerFactory.CreateLogger<Receiver>(), queueId, _connection, _js, _stream.Value, | ||
| _consumer.Value)); |
There was a problem hiding this comment.
Potential NullReferenceException when accessing _stream.Value and _consumer.Value
In the CreateReceiver() method, accessing _stream.Value and _consumer.Value may lead to a NullReferenceException if these values are not yet initialized. If CreateReceiver() can be called before CreateAdapter() completes, there is a risk that the Value properties are still null.
Consider ensuring that _stream.Value and _consumer.Value are initialized before they are used. You might:
- Verify that
CreateAdapter()has been successfully completed before callingCreateReceiver(). - Add null checks before accessing
_stream.Valueand_consumer.Value. - Refactor the code so that
CreateReceiver()awaits the initialization if necessary.
For example:
public IQueueAdapterReceiver CreateReceiver(QueueId queueId)
{
+ if (_stream.Value == null || _consumer.Value == null)
+ {
+ throw new InvalidOperationException("Stream and consumer must be initialized before creating a receiver.");
+ }
if (_receivers.TryGetValue(queueId, out var receiver)) return receiver;
var dimensions = new ReceiverMonitorDimensions(queueId.ToString());
var receiverMonitor = ReceiverMonitorFactory(dimensions);
receiver = _receivers.GetOrAdd(queueId,
new Receiver(
Name,
receiverMonitor,
_serializationManager,
_loggerFactory.CreateLogger<Receiver>(),
queueId,
_connection,
_js,
_stream.Value,
_consumer.Value));
return receiver;
}Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores