Merged
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughReplaced unconditional Configure registrations with TryConfigure to prevent duplicate option registrations; added MinimalJsonOptions and AddMinimalJsonOptions to map JsonFormatterOptions into JsonOptions; added tests verifying idempotent registrations and JsonOptions propagation. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer/Startup
participant SC as IServiceCollection
participant DI as DI Container
participant M as MinimalJsonOptions
participant JO as JsonOptions
Dev->>SC: Call AddMinimalJsonOptions(setup?)
SC->>SC: TryAddEnumerable(IConfigureOptions<JsonOptions>=MinimalJsonOptions)
SC->>SC: TryConfigure(JsonFormatterOptions) + AddJsonExceptionResponseFormatter
DI->>M: Resolve MinimalJsonOptions (IOptions<JsonFormatterOptions>)
M->>JO: Apply JsonFormatterOptions -> JsonOptions.SerializerOptions
JO-->>DI: Configured JsonOptions available to framework
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Cuemon.Extensions.DependencyInjection/ServiceCollectionExtensions.cs (1)
73-81:⚠️ Potential issue | 🔴 Critical
Add<TOptions>still usesConfigure— the new test expects idempotent behavior that this code does not provide.Line 79 calls
services.Configure(setup), which unconditionally adds anIConfigureOptions<TOptions>registration on every call. However, the new testAdd_ShouldOnlyRegisterOptionsOnce_WhenCalledMultipleTimesWithSameOptionsType(Line 627-642 in the test file) asserts that callingAddthree times with the sameTOptionsresults in only oneIConfigureOptions<FakeOptions>registration. This test will fail.Either the test expectation is wrong (should assert 3 registrations), or this line should also switch to
TryConfigure:Option A: Make Add idempotent for options too
services.AddServices(service, implementation, lifetime, false); - services.Configure(setup); + services.TryConfigure(setup); return services;Apply the same change to Line 149 (
Addwith factory overload).Option B: Fix the test to match current behavior
- Assert.Equal(1, configureOptionsCount); + Assert.Equal(3, configureOptionsCount);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Cuemon.Extensions.DependencyInjection/ServiceCollectionExtensions.cs` around lines 73 - 81, The Add<TOptions>(this IServiceCollection services, Type service, Type implementation, ServiceLifetime lifetime, Action<TOptions> setup) method currently calls services.Configure(setup) which registers IConfigureOptions<TOptions> on every call; change this to services.TryConfigure(setup) to make option registrations idempotent (so repeated calls only add one IConfigureOptions<TOptions>). Apply the same change to the other Add overload that accepts a factory (the overload referenced as the Add with factory) so both Add<TOptions> code paths use TryConfigure instead of Configure; keep existing null checks (Validator.ThrowIfNull) and service registration via AddServices unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@test/Cuemon.Extensions.DependencyInjection.Tests/ServiceCollectionExtensionsTest.cs`:
- Around line 627-642: The test fails because Add<TOptions> in
ServiceCollectionExtensions still calls services.Configure(setup), causing a new
IConfigureOptions<TOptions> to be registered on each call; change the
registration to use the "try" variant so the options setup is only registered
once — replace the Configure call with the idempotent TryConfigure (or the
equivalent TryAdd/TryAddEnumerable registration) inside the Add<TOptions>
implementation so multiple calls to Add<TOptions> only result in a single
IConfigureOptions<TOptions] registration.
---
Outside diff comments:
In `@src/Cuemon.Extensions.DependencyInjection/ServiceCollectionExtensions.cs`:
- Around line 73-81: The Add<TOptions>(this IServiceCollection services, Type
service, Type implementation, ServiceLifetime lifetime, Action<TOptions> setup)
method currently calls services.Configure(setup) which registers
IConfigureOptions<TOptions> on every call; change this to
services.TryConfigure(setup) to make option registrations idempotent (so
repeated calls only add one IConfigureOptions<TOptions>). Apply the same change
to the other Add overload that accepts a factory (the overload referenced as the
Add with factory) so both Add<TOptions> code paths use TryConfigure instead of
Configure; keep existing null checks (Validator.ThrowIfNull) and service
registration via AddServices unchanged.
test/Cuemon.Extensions.DependencyInjection.Tests/ServiceCollectionExtensionsTest.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This pull request enhances dependency injection options registration to prevent duplicate IConfigureOptions registrations when methods are called multiple times. The changes support minimal API scenarios where formatter and service options may be registered in multiple places. The PR modifies the TryAdd<TOptions> extension methods to use TryConfigure instead of Configure, and updates the AddJsonFormatterOptions and AddXmlFormatterOptions methods to ensure singleton registration behavior.
Changes:
- Modified
TryAdd<TOptions>methods inServiceCollectionExtensions.csto useTryConfigurefor preventing duplicate option registrations - Updated
AddJsonFormatterOptionsandAddXmlFormatterOptionsto useTryConfigurefor singleton registration behavior - Added comprehensive tests to verify single registration behavior for formatter options and service options
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Cuemon.Extensions.DependencyInjection/ServiceCollectionExtensions.cs | Updated two TryAdd<TOptions> overloads to use TryConfigure instead of Configure |
| src/Cuemon.Extensions.AspNetCore.Text.Json/Formatters/ServiceCollectionExtensions.cs | Changed AddJsonFormatterOptions to use TryConfigure and added required using statement |
| src/Cuemon.Extensions.AspNetCore.Xml/Formatters/ServiceCollectionExtensions.cs | Changed AddXmlFormatterOptions to use TryConfigure and added required using statement |
| test/Cuemon.Extensions.AspNetCore.Tests/Text.Json/Formatters/ServiceCollectionExtensionsTest.cs | New test file verifying AddJsonFormatterOptions only registers once |
| test/Cuemon.Extensions.AspNetCore.Tests/Xml/Formatters/ServiceCollectionExtensionsTest.cs | New test file verifying AddXmlFormatterOptions only registers once |
| test/Cuemon.Extensions.DependencyInjection.Tests/ServiceCollectionExtensionsTest.cs | Added three new tests verifying single registration for Add and TryAdd methods |
test/Cuemon.Extensions.DependencyInjection.Tests/ServiceCollectionExtensionsTest.cs
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #147 +/- ##
==========================================
+ Coverage 80.53% 80.59% +0.06%
==========================================
Files 598 600 +2
Lines 18839 18886 +47
Branches 1936 1937 +1
==========================================
+ Hits 15172 15222 +50
+ Misses 3601 3598 -3
Partials 66 66 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Cuemon.Extensions.AspNetCore.Text.Json/MinimalJsonOptions.cs (1)
14-17: Add<param>XML doc for theformatterOptionsconstructor parameter.The constructor is public API but the parameter lacks documentation.
As per coding guidelines, public and protected members must have XML doc comments following existing wording and style.
📝 Proposed doc addition
/// <summary> /// Creates a new <see cref="MinimalJsonOptions"/>. /// </summary> + /// <param name="formatterOptions">The <see cref="IOptions{JsonFormatterOptions}"/> to configure <see cref="JsonOptions"/> from.</param> public MinimalJsonOptions(IOptions<JsonFormatterOptions> formatterOptions) : base(mo =>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Cuemon.Extensions.AspNetCore.Text.Json/MinimalJsonOptions.cs` around lines 14 - 17, Add an XML <param> tag for the constructor parameter `formatterOptions` on the public constructor MinimalJsonOptions(IOptions<JsonFormatterOptions> formatterOptions), documenting that it supplies the JsonFormatterOptions via IOptions and describing the expected role (e.g., options used to configure JSON formatting behavior); ensure the wording and tag style match existing XML docs in the file for public members.test/Cuemon.Extensions.AspNetCore.Tests/Text.Json/MinimalJsonOptionsTest.cs (1)
34-47: Consider testingWriteIndented = trueto verify actual propagation.Setting
WriteIndented = falsetests the default value ofJsonSerializerOptions, so this test would pass even if propagation were broken. Setting it totruewould confirm the value is actually being applied.Proposed change
- o.Settings.WriteIndented = false; + o.Settings.WriteIndented = true; }); var provider = services.BuildServiceProvider(); var jsonOptions = provider.GetRequiredService<IOptions<JsonOptions>>().Value; - Assert.False(jsonOptions.SerializerOptions.WriteIndented); + Assert.True(jsonOptions.SerializerOptions.WriteIndented);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Cuemon.Extensions.AspNetCore.Tests/Text.Json/MinimalJsonOptionsTest.cs` around lines 34 - 47, Update the test MinimalJsonOptions_ShouldPropagateWriteIndented_FromJsonFormatterOptions to set o.Settings.WriteIndented = true instead of false and assert that jsonOptions.SerializerOptions.WriteIndented is true; locate the test method by name and the call to services.AddMinimalJsonOptions (and the retrieval via provider.GetRequiredService<IOptions<JsonOptions>>().Value) and change the arranged value and the Assert.False to Assert.True so the test verifies actual propagation of the non-default setting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Cuemon.Extensions.AspNetCore.Text.Json/MinimalJsonOptions.cs`:
- Around line 17-23: The Configure delegate in MinimalJsonOptions' constructor
mutates the shared formatterOptions.Value.Settings collection causing duplicate
converters; fix by cloning the JsonFormatterOptions.Settings (or the
Settings.Converters collection) before calling
AddHttpExceptionDescriptorConverter so you modify a per-call copy, then pass
that cloned converters list into
Decorator.Enclose(mo.SerializerOptions.Converters).AddRange(...); alternatively,
perform the AddHttpExceptionDescriptorConverter call once during initialization
(outside the Configure lambda) to avoid repeated mutation of
formatterOptions.Value; key symbols:
MinimalJsonOptions(IOptions<JsonFormatterOptions>),
formatterOptions.Value.Settings, AddHttpExceptionDescriptorConverter, and
Decorator.Enclose(...).AddRange.
In `@test/Cuemon.Extensions.AspNetCore.Tests/Text.Json/MinimalJsonOptionsTest.cs`:
- Around line 259-274: The test
MinimalJsonOptions_ShouldNotOverrideTypeInfoResolver_WhenNull only logs the
resolver state and has no assertion, so change it to assert the expected
behavior: after calling services.AddMinimalJsonOptions(o =>
o.Settings.TypeInfoResolver = null) and building provider, verify
jsonOptions.SerializerOptions.TypeInfoResolver was not overridden by asserting
it is not null (e.g.,
Assert.NotNull(jsonOptions.SerializerOptions.TypeInfoResolver)) so the test
actually fails if the resolver gets cleared.
---
Nitpick comments:
In `@src/Cuemon.Extensions.AspNetCore.Text.Json/MinimalJsonOptions.cs`:
- Around line 14-17: Add an XML <param> tag for the constructor parameter
`formatterOptions` on the public constructor
MinimalJsonOptions(IOptions<JsonFormatterOptions> formatterOptions), documenting
that it supplies the JsonFormatterOptions via IOptions and describing the
expected role (e.g., options used to configure JSON formatting behavior); ensure
the wording and tag style match existing XML docs in the file for public
members.
In `@test/Cuemon.Extensions.AspNetCore.Tests/Text.Json/MinimalJsonOptionsTest.cs`:
- Around line 34-47: Update the test
MinimalJsonOptions_ShouldPropagateWriteIndented_FromJsonFormatterOptions to set
o.Settings.WriteIndented = true instead of false and assert that
jsonOptions.SerializerOptions.WriteIndented is true; locate the test method by
name and the call to services.AddMinimalJsonOptions (and the retrieval via
provider.GetRequiredService<IOptions<JsonOptions>>().Value) and change the
arranged value and the Assert.False to Assert.True so the test verifies actual
propagation of the non-default setting.
src/Cuemon.Extensions.AspNetCore.Text.Json/MinimalJsonOptions.cs
Outdated
Show resolved
Hide resolved
test/Cuemon.Extensions.AspNetCore.Tests/Text.Json/MinimalJsonOptionsTest.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Cuemon.Extensions.AspNetCore.Text.Json/MinimalJsonOptions.cs`:
- Around line 15-17: Update the XML doc comment for the MinimalJsonOptions
constructor to include a <param name="formatterOptions"> entry describing the
constructor parameter; locate the public constructor for the MinimalJsonOptions
type (the constructor that accepts formatterOptions) and add a concise
description for formatterOptions consistent with existing doc wording/style so
the public member parameter is documented.
---
Duplicate comments:
In `@test/Cuemon.Extensions.AspNetCore.Tests/Text.Json/MinimalJsonOptionsTest.cs`:
- Around line 280-295: The test lacks an assertion and should verify that a
pre-existing target resolver is preserved when the source resolver is null;
before calling AddMinimalJsonOptions in
MinimalJsonOptions_ShouldNotOverrideTypeInfoResolver_WhenNull, register a known
sentinel resolver on JsonOptions (e.g., services.Configure<JsonOptions>(opts =>
opts.SerializerOptions.TypeInfoResolver = mySentinelResolver) or set it via
AddOptions), then call services.AddMinimalJsonOptions(o =>
o.Settings.TypeInfoResolver = null), build the provider, retrieve jsonOptions
via provider.GetRequiredService<IOptions<JsonOptions>>().Value and assert that
jsonOptions.SerializerOptions.TypeInfoResolver is the same sentinel instance (or
remains unchanged) to ensure the null source does not overwrite the target.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request ensures that options configuration for formatters and services is registered only once, even if the registration method is called multiple times. It updates the relevant extension methods to use
TryConfigureinstead ofConfigure, and adds tests to verify this singleton registration behavior.Dependency Injection Improvements:
AddJsonFormatterOptionsandAddXmlFormatterOptionsin their respectiveServiceCollectionExtensionsto useTryConfigure, preventing duplicateIConfigureOptionsregistrations for formatter options. [1] [2]TryAdd<TOptions>extension methods inServiceCollectionExtensionsto useTryConfigurefor options setup, ensuring only one registration per options type. [1] [2]Cuemon.Extensions.DependencyInjectionusing statements where needed to support these changes. [1] [2]Testing Enhancements:
AddJsonFormatterOptionsandAddXmlFormatterOptionsto confirm that multiple calls result in only a singleIConfigureOptionsregistration. [1] [2]AddandTryAddextension methods to verify that options are registered only once, regardless of how many times the methods are called with the same options type.Summary by CodeRabbit
New Features
Bug Fixes
Tests