Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Customize DaprClient from workflow registration #1411

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Customize DaprClient from workflow registration #1411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fbabbc3
Added means to customize the Dapr client from the Dapr Workflow regis…
WhitWaldo Nov 21, 2024
c6eafe6
Updated to favor the overload that provides an IServiceProvider
WhitWaldo Nov 21, 2024
ebedc41
Merge branch 'master' into customize-dapr-from-workflow-registration
WhitWaldo Nov 24, 2024
4f5457d
Merge branch 'master' into customize-dapr-from-workflow-registration
WhitWaldo Nov 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/Dapr.Workflow/WorkflowServiceCollectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

using System;
using System.Net.Http;
using Dapr.Client;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Extensions;
Expand All @@ -29,18 +30,29 @@ public static class WorkflowServiceCollectionExtensions
/// </summary>
/// <param name="serviceCollection">The <see cref="IServiceCollection"/>.</param>
/// <param name="configure">A delegate used to configure actor options and register workflow functions.</param>
/// <param name="configureDaprClient">A delegate used to optionally configure the Dapr client connection.</param>
/// <param name="lifetime">The lifetime of the registered services.</param>
public static IServiceCollection AddDaprWorkflow(
this IServiceCollection serviceCollection,
Action<WorkflowRuntimeOptions> configure,
Action<IServiceProvider, DaprClientBuilder>? configureDaprClient = null,
ServiceLifetime lifetime = ServiceLifetime.Singleton)
{
if (serviceCollection == null)
{
throw new ArgumentNullException(nameof(serviceCollection));
}

serviceCollection.AddDaprClient(lifetime: lifetime);

if (configureDaprClient is not null)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is probably ok, I wonder if, in this specific situation, the developer couldn't just call AddDaprClient() themselves before calling AddDaprWorkflow() (as the registration can only happen once)? This would save every derivative registration (e.g. workflow, AI, pub/sub, etc.) from having to add the complexity of additional lambdas in their APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to be able to have users install the various packages potentially independently of one another. If I want to connect to my local Dapr runtime for pubsub, but connect to a remote Dapr runtime for state management, that wouldn't be trivially done by having the developer call AddDaprClient() as it would supersede the per-client options.

Perhaps in the shorter term then, instead of what I've got here, I need to refactor it all to output a DaprConnectionOptions or the like and within each of the clients, stand up an unregistered instance of DaprClient using those options.

Then, if you want the DaprClient (as you would for most of today's Dapr functionality), you use AddDaprClient() and if you want any of the other clients, you use AddWhateverClient(), but the underlying DaprClients themselves are discrete from one another.

In the short term, it ensures you don't have lifecycle conflicts across the board. In the really long term, it means we can eventually drop AddDaprClient() altogether once all the existing functionality is migrated into the purpose-built clients.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking a little more about this - today, because all the clients use static builders, it's insufficient to rely on the previously created DaprClient instances to get the configuration as the builders are recreating the connections from scratch.

So while the configuration bits may be available in the IConfiguration, that's not necessarily going to cover all configuration scenarios (e.g. setting their own timeout, specifying their own gRPC options or their own JSON serialization settings), it'd at least handle setting endpoints (HTTP and gRPC) and the API token.

{
serviceCollection.AddDaprClient(configureDaprClient, lifetime: lifetime);
}
else
{
serviceCollection.AddDaprClient(lifetime: lifetime);
}

serviceCollection.AddHttpClient();
serviceCollection.AddHostedService<WorkflowLoggingService>();

Expand Down
41 changes: 37 additions & 4 deletions test/Dapr.Workflow.Test/WorkflowServiceCollectionExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,40 @@
using Microsoft.Extensions.DependencyInjection;
using System.Text.Json;
using Dapr.Client;
using Microsoft.Extensions.DependencyInjection;

namespace Dapr.Workflow.Test;

public class WorkflowServiceCollectionExtensionsTests
{
[Fact]
public void ConfigureDaprClient_FromWorkflowClientRegistration()
{
const int maxDepth = 6;
const bool writeIndented = true;

var services = new ServiceCollection();
services.AddSingleton(_ => new MyAwesomeType { MaxDepth = maxDepth });

services.AddDaprWorkflow(_ => { }, (serviceProvider, builder) =>
{
var myType = serviceProvider.GetRequiredService<MyAwesomeType>();

builder.UseJsonSerializationOptions(new JsonSerializerOptions { MaxDepth = myType.MaxDepth ?? 0, WriteIndented = writeIndented });
});

var serviceProvider = services.BuildServiceProvider();

var daprClient = serviceProvider.GetRequiredService<DaprClient>();
Assert.Equal(maxDepth, daprClient.JsonSerializerOptions.MaxDepth);
Assert.Equal(writeIndented, daprClient.JsonSerializerOptions.WriteIndented);
}

[Fact]
public void RegisterWorkflowClient_ShouldRegisterSingleton_WhenLifetimeIsSingleton()
{
var services = new ServiceCollection();

services.AddDaprWorkflow(options => { }, ServiceLifetime.Singleton);
services.AddDaprWorkflow(options => { }, lifetime: ServiceLifetime.Singleton);
var serviceProvider = services.BuildServiceProvider();

var daprWorkflowClient1 = serviceProvider.GetService<DaprWorkflowClient>();
Expand All @@ -26,7 +51,7 @@ public async Task RegisterWorkflowClient_ShouldRegisterScoped_WhenLifetimeIsScop
{
var services = new ServiceCollection();

services.AddDaprWorkflow(options => { }, ServiceLifetime.Scoped);
services.AddDaprWorkflow(options => { }, lifetime: ServiceLifetime.Scoped);
var serviceProvider = services.BuildServiceProvider();

await using var scope1 = serviceProvider.CreateAsyncScope();
Expand All @@ -45,7 +70,7 @@ public void RegisterWorkflowClient_ShouldRegisterTransient_WhenLifetimeIsTransie
{
var services = new ServiceCollection();

services.AddDaprWorkflow(options => { }, ServiceLifetime.Transient);
services.AddDaprWorkflow(options => { }, lifetime: ServiceLifetime.Transient);
var serviceProvider = services.BuildServiceProvider();

var daprWorkflowClient1 = serviceProvider.GetService<DaprWorkflowClient>();
Expand All @@ -55,4 +80,12 @@ public void RegisterWorkflowClient_ShouldRegisterTransient_WhenLifetimeIsTransie
Assert.NotNull(daprWorkflowClient2);
Assert.NotSame(daprWorkflowClient1, daprWorkflowClient2);
}

private sealed class MyAwesomeType
{
/// <summary>
/// The max depth value.
/// </summary>
public int? MaxDepth { get; init; }
}
}
Loading