-
Notifications
You must be signed in to change notification settings - Fork 89
[UPDATED] Add NATS Options Pattern DI Support #888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR modernizes NATS DI configuration by adopting the Microsoft options pattern for NatsOpts, centralizing defaults and deprecating the older direct‐configuration methods.
- Introduces a new
NatsOptsBuildertype and wires it into the DI container viaIOptions<NatsOptsBuilder>. - Refactors all
ConfigureOptionsandConfigureConnectionoverloads to use anOptionsBuilder<NatsOptsBuilder>approach and marks the old APIs obsolete. - Updates the project file to reference
Microsoft.Extensions.Optionsand expands the test suite to cover the new options‐builder patterns and backward compatibility.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/NATS.Extensions.Microsoft.DependencyInjection.Tests/NatsHostingExtensionsTests.cs | Added tests for the new ConfigureOptions overloads and options‐builder variants, including backward compatibility checks. |
| src/NATS.Extensions.Microsoft.DependencyInjection/NatsBuilder.cs | Introduced NatsOptsBuilder and rewrote ConfigureOptions/ConfigureConnection to use OptionsBuilder<NatsOptsBuilder>. |
| src/NATS.Extensions.Microsoft.DependencyInjection/NATS.Extensions.Microsoft.DependencyInjection.csproj | Added Microsoft.Extensions.Options package references for .NET6/8 targets. |
Comments suppressed due to low confidence (1)
src/NATS.Extensions.Microsoft.DependencyInjection/NatsBuilder.cs:13
- Add XML documentation for
NatsOptsBuilderand its public members to explain their role in capturing and configuring NATS options via the options pattern.
public class NatsOptsBuilder
|
|
||
| return optsFactory(serviceProvider, opts); | ||
| }; | ||
| builder(_services.AddOptions<NatsOptsBuilder>()); |
Copilot
AI
Jul 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each call to ConfigureOptions invokes AddOptions<NatsOptsBuilder>() again, registering duplicate configuration actions. Consider storing the initial OptionsBuilder<NatsOptsBuilder> in a private field and reusing it for subsequent Configure calls to avoid redundant registrations.
| builder(_services.AddOptions<NatsOptsBuilder>()); | |
| builder(_optionsBuilder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this should be done build time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ive updated to cache the initial options builder in a field.
| private Func<IServiceProvider, int> _poolSizeConfigurer = _ => 1; | ||
| private Func<IServiceProvider, NatsOpts, NatsOpts>? _configureOpts; | ||
| private Action<IServiceProvider, NatsConnection>? _configureConnection; | ||
| private object? _diKey = null; |
Copilot
AI
Jul 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The private field _diKey is no longer referenced in the builder implementation. Remove it to clean up dead code and reduce maintenance overhead.
| private object? _diKey = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must be missing something. this is still used, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed it is still used. I didn't touch the usage around the DI key as it is a builder concern rather than a service configuration one
| }; | ||
| public NatsBuilder ConfigureConnection(Action<IServiceProvider, NatsConnection> configureConnection) => | ||
| ConfigureOptions(builder => builder.Configure<IServiceProvider>((opts, provider) => | ||
| opts.ConfigureConnection = configureConnection)); |
Copilot
AI
Jul 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This method overwrites any existing ConfigureConnection action rather than chaining multiple calls. If users may call this multiple times, consider merging the new delegate with the existing one to avoid losing previously configured behaviors.
| opts.ConfigureConnection = configureConnection)); | |
| { | |
| opts.ConfigureConnection = opts.ConfigureConnection == null | |
| ? configureConnection | |
| : (sp, connection) => | |
| { | |
| opts.ConfigureConnection?.Invoke(sp, connection); | |
| configureConnection(sp, connection); | |
| }; | |
| })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AdamSendible this one has some merit. do you mind if we include this change as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've applied a change to match the existing behaviour
|
thanks @AdamSendible. looks good. could you GPG sign your commits please? it's a project requirement we need to follow |
80fd6fb to
f2d2a83
Compare
f2d2a83 to
4b1ece3
Compare
That's done |
|
Hi @mtmk , Just checking in on the status of this PR. Seems that all the checks have passed now. Let me know if you need anything else from my side. Thanks! |
|
thanks for the nudge @AdamSendible, looks good to me actually. @rickdotnet do you have any objections? |
|
Looks good to me. Thanks @AdamSendible ! |
mtmk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks @AdamSendible
Description
This pull request introduces an enhancement to the NATS DI configuration framework by adopting the options pattern. The changes are designed to improve the readability, maintainability, and flexibility of options configuration in NATS projects.
Key Changes
Microsoft.Extensions.Optionspackage reference to support options pattern.NatsOptsBuilderto encapsulate configuration forNatsOptsand connection.OptionsBuilder<NatsOptsBuilder>.OptionsBuilderfor greater clarity and flexibility.IOptions<NatsOptsBuilder>.Related Issue