Conversation
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix issue #1153 by providing clearer error messages when ILoggerFactory is not registered before AddMediatR() is called. The PR aims to replace the cryptic "No constructor for type 'MediatR.Licensing.LicenseAccessor' can be instantiated" error with an actionable message directing users to call services.AddLogging().
Changes:
- Add factory lambdas for LicenseAccessor and LicenseValidator registration to detect missing ILoggerFactory early
- Simplify CheckLicense method by removing fallback instantiation logic
- Add Microsoft.Extensions.Logging import to ServiceRegistrar.cs
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/MediatR/Registration/ServiceRegistrar.cs | Adds factory lambdas for LicenseAccessor and LicenseValidator with explicit ILoggerFactory validation and clear error messages |
| src/MediatR/MicrosoftExtensionsDI/MediatRServiceCollectionExtensions.cs | Only contains a whitespace fix; intended simplification of CheckLicense method was not implemented |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| services.TryAddSingleton(static sp => | ||
| { | ||
| var loggerFactory = sp.GetService<ILoggerFactory>() | ||
| ?? throw new InvalidOperationException( | ||
| "MediatR requires ILoggerFactory to be registered. " + | ||
| "Call services.AddLogging() before services.AddMediatR()."); | ||
| var config = sp.GetService<MediatRServiceConfiguration>(); | ||
| return config != null | ||
| ? new LicenseAccessor(config, loggerFactory) | ||
| : new LicenseAccessor(loggerFactory); | ||
| }); |
There was a problem hiding this comment.
The TryAddSingleton call is missing the type parameter. This will not register LicenseAccessor in the DI container correctly. Change line 474 to: services.TryAddSingleton<LicenseAccessor>(static sp =>
| services.TryAddSingleton(static sp => | ||
| { | ||
| var loggerFactory = sp.GetService<ILoggerFactory>() | ||
| ?? throw new InvalidOperationException( | ||
| "MediatR requires ILoggerFactory to be registered. " + | ||
| "Call services.AddLogging() before services.AddMediatR()."); | ||
| return new LicenseValidator(loggerFactory); | ||
| }); |
There was a problem hiding this comment.
The TryAddSingleton call is missing the type parameter. This will not register LicenseValidator in the DI container correctly. Change line 485 to: services.TryAddSingleton<LicenseValidator>(static sp =>
| @@ -66,7 +66,7 @@ internal static void CheckLicense(this IServiceProvider serviceProvider) | |||
| serviceProvider.GetRequiredService<MediatRServiceConfiguration>(), | |||
| serviceProvider.GetRequiredService<ILoggerFactory>() | |||
| ); | |||
| var licenseValidator = serviceProvider.GetService<LicenseValidator>() | |||
| var licenseValidator = serviceProvider.GetService<LicenseValidator>() | |||
| ?? new LicenseValidator(serviceProvider.GetRequiredService<ILoggerFactory>()); | |||
There was a problem hiding this comment.
The PR description states that this method should be simplified to use GetRequiredService for both licenseAccessor and licenseValidator, removing the fallback logic with ?? new LicenseAccessor(...) and ?? new LicenseValidator(...). However, this change was not implemented - only a trailing whitespace was removed from line 69. The fallback logic on lines 65-70 should be replaced with: var licenseAccessor = serviceProvider.GetRequiredService<LicenseAccessor>(); and var licenseValidator = serviceProvider.GetRequiredService<LicenseValidator>(); as described in the PR description.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
…ogic Co-authored-by: jbogard <104498+jbogard@users.noreply.github.com>
Simplify CheckLicense to use GetRequiredService instead of fallback constructors
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var config = sp.GetService<MediatRServiceConfiguration>(); | ||
| return config != null | ||
| ? new LicenseAccessor(config, loggerFactory) | ||
| : new LicenseAccessor(loggerFactory); |
There was a problem hiding this comment.
MediatRServiceConfiguration is always registered just above via services.TryAddSingleton(serviceConfiguration), so sp.GetService<MediatRServiceConfiguration>() should never be null here. This makes the conditional and the new LicenseAccessor(loggerFactory) branch effectively dead code; consider switching to GetRequiredService<MediatRServiceConfiguration>() and always using the (config, loggerFactory) constructor to simplify.
| var config = sp.GetService<MediatRServiceConfiguration>(); | |
| return config != null | |
| ? new LicenseAccessor(config, loggerFactory) | |
| : new LicenseAccessor(loggerFactory); | |
| var config = sp.GetRequiredService<MediatRServiceConfiguration>(); | |
| return new LicenseAccessor(config, loggerFactory); |
| var loggerFactory = sp.GetService<ILoggerFactory>() | ||
| ?? throw new InvalidOperationException( | ||
| "MediatR requires ILoggerFactory to be registered. " + | ||
| "Call services.AddLogging() before services.AddMediatR()."); |
There was a problem hiding this comment.
New behavior is being introduced here (custom InvalidOperationException when ILoggerFactory is missing). There isn’t currently a test asserting this specific failure mode/message (the suite generally calls AddFakeLogging). Adding a focused test that omits logging and asserts the thrown message would help prevent regressions back to the cryptic "No constructor" error.
| var loggerFactory = sp.GetService<ILoggerFactory>() | ||
| ?? throw new InvalidOperationException( | ||
| "MediatR requires ILoggerFactory to be registered. " + | ||
| "Call services.AddLogging() before services.AddMediatR()."); |
There was a problem hiding this comment.
The exception message says to call services.AddLogging() before services.AddMediatR(), but registration order doesn’t actually matter as long as ILoggerFactory is registered before the provider is built/resolution happens (e.g., tests call AddRequiredServices before AddFakeLogging). Consider rewording to avoid implying a required ordering (e.g., “Ensure ILoggerFactory is registered (services.AddLogging())”).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fix: Cryptic "No constructor" Error When ILoggerFactory Is Not Registered (Issue #1153)
Context
After upgrading to MediatR 14.x, users get a confusing exception when resolving
IMediator:Why it happens:
ServiceRegistrar.AddRequiredServicesregistersLicenseAccessorandLicenseValidatorvia bare genericTryAddSingleton<T>()(lines 473–474 inServiceRegistrar.cs), with no factory lambda.ILoggerFactory— required by both ofLicenseAccessor's constructors — is not registered by the application.serviceProvider.GetService<LicenseAccessor>()inCheckLicensethrows (not returns null) because the type is registered but its constructor cannot be satisfied.services.AddLogging().Files to Modify
src/MediatR/Registration/ServiceRegistrar.cs— change howLicenseAccessorandLicenseValidatorare registered (lines 473–474)src/MediatR/MicrosoftExtensionsDI/MediatRServiceCollectionExtensions.cs— remove now-dead fallback code inCheckLicense(lines 65–70)Implementation
1. Replace bare registrations with factory lambdas in
ServiceRegistrar.csBefore (lines 473–474):
After:
This produces a clear, actionable error at
IMediatorresolution time instead of the internal-type-naming cryptic message.2. Simplify
CheckLicenseinMediatRServiceCollectionExtensions.csThe
?? new LicenseAccessor(...)and?? new LicenseValidator(...)fallbacks are now dead code: both types are always registered (viaTryAddSingleton), soGetService<T>()will never return null — it either succeeds or throws. Simplify toGetRequiredServicecalls.Before (lines 65–70):
After:
Verification
dotnet test test/MediatR.Tests/MediatR.Tests.csprojAll 166 existing tests pass (they already register
NullLoggerFactory.Instance). The fix only changes behavior whenILoggerFactoryis absent — producing a clear actionable message instead of the cryptic "No constructor" error.