dapr / dotnet-sdk

Dapr SDK for .NET
Apache License 2.0
1.12k stars 341 forks source link

Make AddDaprSecretStore compliant with DI #1115

Closed ChristPetitjean closed 1 month ago

ChristPetitjean commented 1 year ago

Describe the feature

The actual implementation need a DaprClient instance to work in API. But, because the mapping is performed somewhere before the app is built, we cannot use app.Services.GetRequiredService<DaprClient>. So this make this extension method using a new instance that is unmockable and so unusable during testing phase.

For example: Program.cs:

      builder.Configuration.AddDaprSecretStore("secretstore", new DaprClientBuilder().Build());

TestFile.cs

            var daprClientMock = new Mock<DaprClient>();
            daprClientMock
                .Setup(s => s.GetSecretAsync(
                           It.IsAny<string>(), 
                           It.IsAny<string>(), 
                           It.IsAny<IReadOnlyDictionary<string, string>>(), 
                           It.IsAny<CancellationToken>()))
                .ReturnsAsync(new Dictionary<string, string>
                {
                    { "ConnexionStrings:mongodb", this._mongoDbRunner.ConnectionString }
                });
            using var client = Application
                .WithWebHostBuilder(
                    builder =>
                    {
                        builder.ConfigureTestServices(
                            services =>
                            {
                                services.AddSingleton(_ => daprClientMock.Object);
                            });
                    })
                .CreateClient();

This code will not work because of the extension method that is creating its own instance instead of using the DI one.

It will be great to use DI for this method. Workaround is to not use it at all and only use secretAsync method from the client.

WhitWaldo commented 1 month ago

@ChristPetitjean While I agree that it would help to expand on the documentation to make this clear, you're not explicitly bound to using the static builder to make a DaprClient here, though be advised that you open yourself up to some real catch-22 resolution issues by taking this approach since Microsoft.Extensions.DependencyInjection does not support deferred resolution.

You might instead consider doing something like the following:

builder.Services.AddSingleton(c => new DaprClientBuilder().Build()); //Or whatever sort of mock you want to create
var app = builder.Build();

//And now, register the IConfiguration provider
await using var scope = app.Services.CreateAsyncScope();
var daprClient = scope.ServiceProvider.GetRequiredService<DaprClient>();

builder.Configuration.AddDaprSecretStore("secretstore", daprClient);

What you really need to be on the lookout for is that you're not injecting an IConfiguration during your preceding DI service registrations and relying on a value from the Dapr secret store given that it will not have been registered yet and today, you won't know this until it throws at runtime (albeit quite early during startup).

I'm going to go ahead and close this issue as the question is addressed, but if you have any follow-up questions, please don't hesitate to re-open.