Pass IServiceProvider to configureBuilder of AddKafkaConsumer and AddKafkaProducer by g7ed6e · Pull Request #4327 · microsoft/aspire

g7ed6e · 2024-05-29T15:04:00Z

Resolves #4323

I had to deal with RS0027 and RS0028 and finally moved from optional parameters to explicit parameters overloads as parameter types differ.

sample usage:

var builder = Host.CreateApplicationBuilder(args);

builder.AddServiceDefaults();

builder.Services.AddSingleton<ISchemaRegistryClient>(provider =>
{
    var schemaRegistryConfig = new SchemaRegistryConfig
    {
        Url = "http://localhost:8081"
    };
    return new CachedSchemaRegistryClient(schemaRegistryConfig);
});
builder.AddKafkaProducer<Null, User>("kafka", configureBuilder: (provider, producerBuilder) =>
{
    var schemaRegistryClient = provider.GetRequiredService<ISchemaRegistryClient>();
    producerBuilder.SetValueSerializer(new AvroSerializer<User>(schemaRegistryClient));
});

builder.Services.AddHostedService<ContinuousProducerWorker>();

builder.Build().Run();

Microsoft Reviewers: Open in CodeFlow

g7ed6e · 2024-05-30T19:37:08Z

The public API modification should be binary backward compatible I followed guidance from https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md.

eerhardt · 2024-06-04T15:18:13Z

Can you add some unit tests for all new API and functionality?

eerhardt · 2024-06-04T15:19:39Z

The public API modification should be binary backward compatible

@joperezr - thoughts on getting the package validation https://learn.microsoft.com/dotnet/fundamentals/apicompat/package-validation/overview hooked up in the repo now that we have shipped 8.0?

eerhardt · 2024-06-04T15:22:47Z

-static Microsoft.Extensions.Hosting.AspireKafkaConsumerExtensions.AddKeyedKafkaConsumer<TKey, TValue>(this Microsoft.Extensions.Hosting.IHostApplicationBuilder! builder, string! name, System.Action<Aspire.Confluent.Kafka.KafkaConsumerSettings!>? configureSettings = null, System.Action<Confluent.Kafka.ConsumerBuilder<TKey, TValue>!>? configureBuilder = null) -> void
-static Microsoft.Extensions.Hosting.AspireKafkaProducerExtensions.AddKafkaProducer<TKey, TValue>(this Microsoft.Extensions.Hosting.IHostApplicationBuilder! builder, string! connectionName, System.Action<Aspire.Confluent.Kafka.KafkaProducerSettings!>? configureSettings = null, System.Action<Confluent.Kafka.ProducerBuilder<TKey, TValue>!>? configureBuilder = null) -> void
-static Microsoft.Extensions.Hosting.AspireKafkaProducerExtensions.AddKeyedKafkaProducer<TKey, TValue>(this Microsoft.Extensions.Hosting.IHostApplicationBuilder! builder, string! name, System.Action<Aspire.Confluent.Kafka.KafkaProducerSettings!>? configureSettings = null, System.Action<Confluent.Kafka.ProducerBuilder<TKey, TValue>!>? configureBuilder = null) -> void
+static Microsoft.Extensions.Hosting.AspireKafkaConsumerExtensions.AddKafkaConsumer<TKey, TValue>(this Microsoft.Extensions.Hosting.IHostApplicationBuilder! builder, string! connectionName) -> void


This is a lot of new APIs (we went from 4 to 24). Are all these needed?

We previously have 4 APIs with 4 parameters from which 2 were optional. To ensure backwards compatibility I dropped the default values and exposed each combination of the previous APIs. we here fo from 4 to 4 * 4 APIs.

The new overloads replace Action<ProducerBuilder<TKey, TValue>> and Action<ConsumerBuilder<TKey, TValue>> parameters by Action<IServiceProvider, ProducerBuilder<TKey, TValue>> and Action<IServiceProvider, ConsumerBuilder<TKey, TValue>> which add 4 * 2 APIs.

At both side, consumer and producer, the APIs forward all the call to the same internal implementation.

g7ed6e · 2024-06-20T07:27:48Z

@mitchdenny / @eerhardt : PR is updated with latest changed from main :)

edit: build is broken stating that some shipped public API no longer exist. I've followed https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md but it looks that the analyzer is failing to map the new methods to existing ones. @joperezr may i ask some guidance about how to solve this.

eerhardt · 2024-06-20T18:45:33Z

@mitchdenny / @eerhardt : PR is updated with latest changed from main :)

edit: build is broken stating that some shipped public API no longer exist. I've followed https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md but it looks that the analyzer is failing to map the new methods to existing ones. @joperezr may i ask some guidance about how to solve this.

I resolved this build error by adding the *REMOVED* heading to the APIs. The analyzer considers this a change because the default values were dropped.

g7ed6e · 2024-06-21T08:03:09Z

@mitchdenny / @eerhardt : PR is updated with latest changed from main :)
edit: build is broken stating that some shipped public API no longer exist. I've followed https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md but it looks that the analyzer is failing to map the new methods to existing ones. @joperezr may i ask some guidance about how to solve this.

I resolved this build error by adding the *REMOVED* heading to the APIs. The analyzer considers this a change because the default values were dropped.

Thank you! i did not know that.

eerhardt · 2024-06-21T14:50:12Z

+    /// <inheritdoc cref="AddKafkaProducerInternal{TKey, TValue}(IHostApplicationBuilder, string, Action{KafkaProducerSettings}?, Action{IServiceProvider, ProducerBuilder{TKey, TValue}}?, string, string?)"/>
+    public static void AddKeyedKafkaProducer<TKey, TValue>(this IHostApplicationBuilder builder, string name, Action<ProducerBuilder<TKey, TValue>>? configureBuilder)
+    {
+        ArgumentException.ThrowIfNullOrEmpty(name);
+        AddKafkaProducerInternal<TKey, TValue>(builder, $"{DefaultConfigSectionName}:{name}", null, Wrap(configureBuilder), connectionName: name, serviceKey: name);
+    }
+
+    /// <inheritdoc cref="AddKafkaProducerInternal{TKey, TValue}(IHostApplicationBuilder, string, Action{KafkaProducerSettings}?, Action{IServiceProvider, ProducerBuilder{TKey, TValue}}?, string, string?)"/>
+    public static void AddKeyedKafkaProducer<TKey, TValue>(this IHostApplicationBuilder builder, string name, Action<IServiceProvider, ProducerBuilder<TKey, TValue>>? configureBuilder)
+    {
+        ArgumentException.ThrowIfNullOrEmpty(name);
+        AddKafkaProducerInternal<TKey, TValue>(builder, $"{DefaultConfigSectionName}:{name}", null, configureBuilder, connectionName: name, serviceKey: name);
+    }
+
+    /// <inheritdoc cref="AddKafkaProducerInternal{TKey, TValue}(IHostApplicationBuilder, string, Action{KafkaProducerSettings}?, Action{IServiceProvider, ProducerBuilder{TKey, TValue}}?, string, string?)"/>


You don't want to inhertdoc from AddKafkaProducerInternal. That method doesn't have any xml doc.

@eerhardt good catch. sorry for that. this is now fixed.

dotnet-bot · 2024-06-22T07:42:25Z

‼️ Found issues ‼️

Project	Coverage Type	Expected	Actual
Aspire.Confluent.Kafka	Line	90	87.63 🔻
Microsoft.Extensions.ServiceDiscovery	Branch	81	80.13 🔻
Microsoft.Extensions.ServiceDiscovery	Line	81	80.94 🔻

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=716654&view=codecoverage-tab

mitchdenny · 2024-06-24T07:27:56Z

Looks like we took a dip in code coverage.

g7ed6e · 2024-06-24T09:06:12Z

Looks like we took a dip in code coverage.

Hum, this PR replace methods with default parameters by methods with all combinations of those parameters. The code coverage was not really meaningful for some parameter combinations.
I can write test to reach 90% code coverage.

…hods to use IServiceProvider in configureBuilder param

Mark the public APIs with default values as removed to make PublicAPI analyzer happy.

g7ed6e · 2024-06-26T13:56:12Z

@eerhardt @mitchdenny i updated UT to cover all parameter combinations as it was before that PR.

mitchdenny · 2024-06-26T22:54:45Z

/azp run

azure-pipelines · 2024-06-26T22:54:57Z

Azure Pipelines successfully started running 1 pipeline(s).

mitchdenny · 2024-07-08T11:57:59Z

/azp run

azure-pipelines · 2024-07-08T11:58:10Z

Azure Pipelines successfully started running 1 pipeline(s).

mitchdenny

LGTM. The overload explosion is unfortunate but lessons learned about API design I say ;)

g7ed6e · 2024-07-08T12:14:06Z

LGTM. The overload explosion is unfortunate but lessons learned about API design I say ;)

I agree.. I guess this is the drawback of defaulting multiple parameters!

eerhardt

LGTM. Thanks for the fix @g7ed6e.

ghost added the area-integrations Issues pertaining to Aspire Integrations packages label May 29, 2024

dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label May 29, 2024

davidfowl requested a review from eerhardt June 4, 2024 07:27

eerhardt reviewed Jun 4, 2024

View reviewed changes

g7ed6e force-pushed the feature/pass-service-provider-to-configure-builder branch from 796ddc3 to 0f67a27 Compare June 5, 2024 09:26

mitchdenny assigned eerhardt Jun 6, 2024

g7ed6e force-pushed the feature/pass-service-provider-to-configure-builder branch from 0f67a27 to e8ec081 Compare June 20, 2024 07:20

g7ed6e requested a review from eerhardt June 20, 2024 07:26

eerhardt reviewed Jun 20, 2024

View reviewed changes

Comment thread src/Components/Aspire.Confluent.Kafka/AspireKafkaConsumerExtensions.cs Outdated

Comment thread tests/Aspire.Confluent.Kafka.Tests/ConsumerConfigurationTests.cs Outdated

Comment thread tests/Aspire.Confluent.Kafka.Tests/ProducerConfigurationTests.cs Outdated

g7ed6e requested review from radical and sebastienros as code owners June 21, 2024 08:00

eerhardt reviewed Jun 21, 2024

View reviewed changes

build-analysis Bot mentioned this pull request Jun 24, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

g7ed6e and others added 8 commits June 25, 2024 17:04

Add overloads to AddKafkaConsumer and AddKafkaProducer extensions met…

f6a7b34

…hods to use IServiceProvider in configureBuilder param

Add unit test to cover ConfigureBuilder params

a9d65a3

Update PublicAPI.Unshipped.txt

213f419

Fix build error

1a079fb

Mark the public APIs with default values as removed to make PublicAPI analyzer happy.

Use IServiceProvider in UT

df281cd

Sort overloads and deduplicate xml docs

a452772

Use Theory instead of ConditionalTheory

b23aec6

Fix incorrect inheritdoc cref

6dc1f1a

Increase code coverage

c368c79

g7ed6e force-pushed the feature/pass-service-provider-to-configure-builder branch from 059a6f3 to c368c79 Compare June 25, 2024 15:04

radical reviewed Jun 26, 2024

View reviewed changes

Comment thread tests/Aspire.Confluent.Kafka.Tests/ConsumerConfigurationTests.cs

radical reviewed Jun 27, 2024

View reviewed changes

Comment thread tests/Aspire.Confluent.Kafka.Tests/ConsumerConfigurationTests.cs Outdated

format parameterized tests to ease readability

6f5ec53

mitchdenny approved these changes Jul 8, 2024

View reviewed changes

eerhardt approved these changes Jul 8, 2024

View reviewed changes

eerhardt merged commit 3fb0716 into microsoft:main Jul 8, 2024

eerhardt mentioned this pull request Jul 8, 2024

Adds overload for AddSqlServerDbContext - Aspire.Microsoft.EntityFrameworkCore.SqlServer #4803

Closed

github-actions Bot locked and limited conversation to collaborators Aug 8, 2024

Conversation

g7ed6e commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

g7ed6e commented May 30, 2024

Uh oh!

eerhardt commented Jun 4, 2024

Uh oh!

eerhardt commented Jun 4, 2024

Uh oh!

eerhardt Jun 4, 2024

Choose a reason for hiding this comment

Uh oh!

g7ed6e Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

g7ed6e commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerhardt commented Jun 20, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

g7ed6e commented Jun 21, 2024

Uh oh!

eerhardt Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

g7ed6e Jun 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dotnet-bot commented Jun 22, 2024

Uh oh!

mitchdenny commented Jun 24, 2024

Uh oh!

g7ed6e commented Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

g7ed6e commented Jun 26, 2024

Uh oh!

mitchdenny commented Jun 26, 2024

Uh oh!

azure-pipelines Bot commented Jun 26, 2024

Uh oh!

Uh oh!

Uh oh!

mitchdenny commented Jul 8, 2024

Uh oh!

azure-pipelines Bot commented Jul 8, 2024

Uh oh!

mitchdenny left a comment

Choose a reason for hiding this comment

Uh oh!

g7ed6e commented Jul 8, 2024

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

g7ed6e commented May 29, 2024 •

edited

Loading

g7ed6e Jun 5, 2024 •

edited

Loading

g7ed6e commented Jun 20, 2024 •

edited

Loading

g7ed6e Jun 22, 2024 •

edited

Loading

g7ed6e commented Jun 24, 2024 •

edited

Loading