Add keyed services support #5387
Conversation
| /// Defaults to <see cref="ServiceLifetime.Singleton" />. | ||
| /// </param> | ||
| /// <returns>The same service collection so that multiple calls can be chained.</returns> | ||
| public static IServiceCollection AddNpgsqlDataSource( |
There was a problem hiding this comment.
I always get slightly nervous around object? parameters.. Specifically here, we have the future direction of doing away with connection strings (i.e. configuring everything via NpgsqlDataSourceBuilder or similar). When that happens, I'm not sure what the signature here will look like, and whether the object parameter will start to be ambiguous etc.
For example, do you think it makes sense to prefix overloads that accept a service key, e.g. AddKeyedNpgsqlDataSource? I know it becomes a mouthful with AddKeyedMultiHostNpgsqlSlimDataSourceCore - but I'm a bit worried.
There was a problem hiding this comment.
I don't think the intention is for libraries to add Keyed methods as well. Just as lifecycle singleton/etc should be a detail and not part of the naming pattern, while they are explicit methods on the servicecollection.
W.r.t. overload ambiguity I do agree we might have a problem here if we want to provide new overloads that don't require connection strings. I wasn't aware we had any intention to move into that direction.
An easy fix would be to move the service key argument to the end. We could even decide to remove the duplicate overloads and make it an optional argument instead.
There was a problem hiding this comment.
W.r.t. overload ambiguity I do agree we might have a problem here if we want to provide new overloads that don't require connection strings. I wasn't aware we had any intention to move into that direction.
Just trying to think about the future, assuming we do try to provide a path where connection strings are no longer used...
An easy fix would be to move the service key argument to the end. We could even decide to remove the duplicate overloads and make it an optional argument instead.
I like that, that makes it an optional aspect of the registration, just like the lifecycle (which it really is). Let's do that.
src/Npgsql.DependencyInjection/NpgsqlServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
src/Npgsql.DependencyInjection/NpgsqlServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
test/Npgsql.DependencyInjection.Tests/DependencyInjectionTests.cs
Outdated
Show resolved
Hide resolved
NinoFloris
force-pushed
the
keyed-services
branch
from
3913e12
to
66979ae
Compare
| /// <returns>The same service collection so that multiple calls can be chained.</returns> | ||
| public static IServiceCollection AddNpgsqlDataSource( | ||
| this IServiceCollection serviceCollection, | ||
| string connectionString, | ||
| Action<NpgsqlDataSourceBuilder> dataSourceBuilderAction, | ||
| ServiceLifetime connectionLifetime = ServiceLifetime.Transient, | ||
| ServiceLifetime dataSourceLifetime = ServiceLifetime.Singleton) | ||
| => AddNpgsqlDataSourceCore(serviceCollection, connectionString, dataSourceBuilderAction, connectionLifetime, dataSourceLifetime); | ||
| ServiceLifetime dataSourceLifetime = ServiceLifetime.Singleton, |
There was a problem hiding this comment.
This is a binary breaking change. Are you OK with making it?
There was a problem hiding this comment.
One recommendation would be to use https://learn.microsoft.com/en-us/dotnet/fundamentals/package-validation/baseline-version-validator (this exact scenario is listed in the example for why this tool is helpful).
There was a problem hiding this comment.
This is a binary breaking change. Are you OK with making it?
I am, these methods are rarely used outside app projects so binary breaks are not that impactful.
We could add back the original methods and move the serviceKey argument to the back of the required argument list but it'd be a fairly noisy overload set. @eerhardt do you think it's worth it?
One recommendation would be to use https://learn.microsoft.com/en-us/dotnet/fundamentals/package-validation/baseline-version-validator (this exact scenario is listed in the example for why this tool is helpful).
We're using https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md across Npgsql projects but it seems like it's not enabled in this one.
There was a problem hiding this comment.
@roji just pointed out we could reintroduce the original overloads and make all its optional parameters required. At that point we are able to keep serviceKey at the end of the new overload.
With that in place we could optionally mark the original method EditorBrowsable(Never) as well, to get rid of the noise...
If I have a moment before we release 8.0 I'll probably do something like that.
There was a problem hiding this comment.
My personal opinion is that AddKeyedNpgsqlDataSource makes it more understandable what this does. If I just happen to pass in a key into this method, I can no longer get my NpgsqlDataSource from DI the "typical" way of just GetService (or a ctor param). Now I need to call GetKeyedService.
I am, these methods are rarely used outside app projects so binary breaks are not that impactful.
Just note that this would break .NET Aspire if it was built earlier using the 7.x versions of Npgsql.DependencyInjection. .NET Aspire calls this method: https://github.com/dotnet/aspire/blob/8a7b625e0da646d12b272aa26e69c0abe4ac7439/src/Components/Aspire.Npgsql/AspirePostgreSqlNpgsqlExtensions.cs#L109. .NET Aspire's component library Aspire.Npgsql would no longer load if an application updated Npgsql.DependencyInjection to this new version. So I wouldn't underestimate how impactful this kind of change can be. (Again, it is the canonical scenario listed for why the package validation tool was written.)
There was a problem hiding this comment.
My personal opinion is that AddKeyedNpgsqlDataSource makes it more understandable what this does. If I just happen to pass in a key into this method, I can no longer get my NpgsqlDataSource from DI the "typical" way of just GetService (or a ctor param). Now I need to call GetKeyedService.
Right - but that's true of the other registration "aspect" - the lifetime.. i.e. if someone accidentally specifies Scoped instead of Singleton, the application will start misbehaving very badly. The service key doesn't seem very different from that conceptually - it's another aspect of the service registration; (it's true that it's an object rather than something typed, but IMHO it's reasonable to expect people to understand what a parameter means before passing in an argument...
We actually did consider putting Keyed in the method name (see #5387 (comment)), but there already are enough Npgsql-specific "facets" that this seemed to become a bit unmanageable. For example, a data source can be slim (for less size) and also multi-host (a special kind of data source for use with multiple database servers, failover etc.), which would bring us to AddKeyedMultiHostNpgsqlSlimDataSourceCore.
Does that make sense?
There was a problem hiding this comment.
We are aware of the changes that would be binary breaking. The primary cause here is not having the public api analyzer enabled and acting too quickly to get an 8.0 release out.
So in any case thanks for bringing it up, it might certainly save some pain.
Again, it is the canonical scenario listed for why the package validation tool was written
The public api analyzer may not spell out the rationale of why certain changes are binary breaks. However any change that needs you to mark an api as REMOVED is essentially binary breaking. It would have brought sufficient attention to it for us to consider it more carefully. Once I do make the change in this area I'll be sure to set up the public api analyzer.
Closes #5134