-
Notifications
You must be signed in to change notification settings - Fork 10.4k
feat(DataProtection): add overloads for PersistKeysToStackExchangeRedis to accept IServiceProvider #62759
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
base: main
Are you sure you want to change the base?
Conversation
…is to accept IServiceProvider
…is factory method
Thanks for your PR, @@hangy. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
…to clarify usage with IServiceProvider
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
Adds new overloads for PersistKeysToStackExchangeRedis
that take an IServiceProvider
-based factory for the Redis IDatabase
, updates the internal wiring to use IConfigureOptions
, and surfaces the changes in the public API.
- Introduces two overloads accepting
Func<IServiceProvider, IDatabase>
with optional customRedisKey
- Modifies internal method to register a singleton
IConfigureOptions<KeyManagementOptions>
that defers to theIServiceProvider
- Updates
PublicAPI.Unshipped.txt
and adds a test for the factory-based overload
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/DataProtection/StackExchangeRedis/test/RedisDataProtectionBuilderExtensionsTest.cs | Adds a test for the IServiceProvider factory overload |
src/DataProtection/StackExchangeRedis/src/RedisDataProtectionBuilderExtensions.cs | Implements new overloads and adjusts internal configuration DI |
src/DataProtection/StackExchangeRedis/src/PublicAPI.Unshipped.txt | Exposes the new overloads in the public API surface |
Comments suppressed due to low confidence (3)
src/DataProtection/StackExchangeRedis/src/PublicAPI.Unshipped.txt:2
- The PublicAPI entry references
StackExchangeRedisDataProtectionBuilderExtensions
, but the actual static class in code is namedRedisDataProtectionBuilderExtensions
. Update the API file to the correct class name.
static Microsoft.AspNetCore.DataProtection.StackExchangeRedisDataProtectionBuilderExtensions.PersistKeysToStackExchangeRedis(this Microsoft.AspNetCore.DataProtection.IDataProtectionBuilder! builder, System.Func<System.IServiceProvider!, StackExchange.Redis.IDatabase!>! databaseFactory) -> Microsoft.AspNetCore.DataProtection.IDataProtectionBuilder!
src/DataProtection/StackExchangeRedis/test/RedisDataProtectionBuilderExtensionsTest.cs:32
- There's a factory-based overload test but no test for the overload that takes a custom
RedisKey
. Consider adding a test that passes a non-defaultRedisKey
and asserts theRedisXmlRepository
uses that key.
[Fact]
src/DataProtection/StackExchangeRedis/test/RedisDataProtectionBuilderExtensionsTest.cs:41
- The test invokes a database factory that depends on
IConnectionMultiplexer
, butIConnectionMultiplexer
is never registered in theServiceCollection
. Add something likeserviceCollection.AddSingleton<IConnectionMultiplexer>(connection);
before calling the builder method.
builder.PersistKeysToStackExchangeRedis(services => services.GetRequiredService<IConnectionMultiplexer>().GetDatabase());
builder.Services.AddSingleton<IConfigureOptions<KeyManagementOptions>>(services => | ||
{ | ||
options.XmlRepository = new RedisXmlRepository(databaseFactory, key); | ||
return new ConfigureOptions<KeyManagementOptions>(options => | ||
{ | ||
options.XmlRepository = new RedisXmlRepository(() => databaseFactory(services), key); |
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] The lambda parameter services
shadows the builder.Services
property and may confuse readers. Consider renaming the parameter to sp
or serviceProvider
to clarify its purpose.
Copilot uses AI. Check for mistakes.
Add additional overloads to
PersistKeysToStackExchangeRedis
Enhances the DataProtection library by enabling it to use an existing connection
Description
The additional overloads of the
PersistKeysToStackExchangeRedis
extension method allow an externally configured Redis connection from Aspire to be used to persist the data protection keys.Fixes #61768