Allow passing a "connectionString key" as well as a full connectionstring on all setup overloads

[julealgon]

When configuring the provider, we currently have to pass the full connection string value. This can become fairly verbose:

    builder.Configuration
        .AddAzureAppConfiguration(builder.Configuration.GetConnectionString("AzureAppConfiguration"))
        .AddAzureAppConfiguration(builder.Configuration.GetConnectionString("AzureAppConfiguration-Common"));

Instead, I'd like to propose that every method that takes in a connection string value, also accepts just the key to the connection string in configuration. Calls would look like this using the same code as above:

    builder.Configuration
        .AddAzureAppConfiguration("AzureAppConfiguration")
        .AddAzureAppConfiguration("AzureAppConfiguration-Common");

The implementation would then check whether there are connectionstrings with the value as a key, and if not, try using the value directly as the connection string.

[amerjusupovic]

Hi @julealgon, thanks for the suggestion! That makes sense, and I'm open to the idea of pulling the connection string based on the key. I'm just thinking about whether this might be confusing on first glance since nothing explicitly changes about the parameter name or type, and the method is the same, but it has 2 functionalities. The parameter name is connectionString, but I feel like in that case it should really be something like connectionStringKey.

[julealgon]

@amerjusupovic the confusion aspect is indeed totally valid. I understand this can be weird since it would be "kinda like" an overload, but not really since it has to deal with the same type string: it would be like having 2 overloads in the same method signature.

I guess the only additional comments I can make here would be that EntityFramework Classic used to support something like this in the past: you could either provide the name of a ConnectionString key, or the connection string itself to the dbcontext:

• https://learn.microsoft.com/en-us/ef/ef6/fundamentals/configuring/connection-strings#use-code-first-with-connection-string-in-appconfigwebconfig-file

EFCore also has this "alternative syntax" where you specify the connection string key name as if it were a connectionstring property, like:

    services.AddDbContext<ApplicationDbContext>(
        options => options.UseSqlServer("name=ConnectionStrings:DefaultConnection"));

• https://learn.microsoft.com/en-us/ef/core/dbcontext-configuration/#dbcontext-in-dependency-injection-for-aspnet-core

Maybe that same design/principle could be applied here, considering we are talking about the exact same type of data (a connectionString)? I think that would, at the very least, increase consistency in the .NET ecosystem, even if the system itself is slighly convoluted (I'd honestly rather have name=DefaultConnection there without the ConnectionStrings part since that should be inferred... but anyways).

This more specific article also mentions the name=configKey pattern:

• https://learn.microsoft.com/en-us/ef/core/miscellaneous/connection-strings#aspnet-core

If the team feels umconfortable with the "magical" nature of these calls, perhaps some sort of Connect overload could be added or a builder-pattern method? It wouldn't help much with the verbosity aspect but would be less logic on the caller:

    builder.Configuration
        .AddAzureAppConfiguration(c => c.WithConnectionStringKey("AzureAppConfiguration"))
        .AddAzureAppConfiguration(c => c.WithConnectionStringKey("AzureAppConfiguration-Common"));

It's tough to suggest anything different with the 2 concepts being represented by the string type... in an ideal world, we'd have a proper native ConnectionString model/object to represent connection strings, and a ConfigurationKey object for the config keys, which would then allow for normal overloading to at least work at a basic level.

I'm not opposed to having native types to represent those 2 things but I do understand that would be a larger, potentiall framework-wide effort.

A design with dedicated strong types for each could allow one to overload the operation even if implicit conversion operators from string were defined:

public static IConfigurationBuilder AddAzureAppConfiguration(
    this IConfigurationBuilder configurationBuilder,
    ConnectionString connectionString)

public static IConfigurationBuilder AddAzureAppConfiguration(
    this IConfigurationBuilder configurationBuilder,
    ConfigurationKey configurationKey)

Assuming both ConnectionString and ConfigurationKey types had implicit conversions from string, you'd be able to do this:

    builder.Configuration
        .AddAzureAppConfiguration(configurationKey: "AzureAppConfiguration")
        .AddAzureAppConfiguration(connectionString: "Endpoint=https://common.azconfig.io;Id=myId;Secret=superSecret");

This would give us the same feel of "2 overloads on string" while still being 100% explicit because of the argument name.

[amerjusupovic]

@julealgon First off, I really appreciate you explaining the different options in your last comment. From discussions I've had so far with those involved in this repo, we don't feel great about the options mainly because the problem we're solving is already an established pattern to get connection strings from configuration. I can understand wanting to make the code a bit shorter, but some of these options add some extra code to write too, as you mentioned. The option of passing in a key with name= is interesting since it's also an established pattern, but I don't know if having name= instead of getting the string from configuration is significant enough. If you just get the connection strings as variables beforehand and pass them in you can space out the logic and make this look a bit simpler.

Overall, the sentiment I've gotten is to leave it as is for now. Let me know if there's anything else we should consider.

[julealgon]

From discussions I've had so far with those involved in this repo, we don't feel great about the options mainly because the problem we're solving is already an established pattern to get connection strings from configuration.

This is understandable. In a sense, it is a bit unfortunate when something less-than-ideal becomes so "invasive" that it is hard to move away from it.

And the only reason I state "less-than-ideal" is because the vast majority of the time, the intent is to grab the value from configuration, and that option is not "modeled" in the existing API.

I can understand wanting to make the code a bit shorter, but some of these options add some extra code to write too, as you mentioned.

Right. But do notice that the options where there are extra methods/extensions/etc still require less logic than the original version, even if they are comparable in length codewise. I think this is actually significant: being slightly verbose is not a big deal, but forcing more logic into the consumer is (in most cases).

The option of passing in a key with name= is interesting since it's also an established pattern, but I don't know if having name= instead of getting the string from configuration is significant enough.

I agree here... the name= pattern is a bit weird, and I'm not fond of the fact it's not strongly typed either. The only reason I brought it up was because it was an existing mechanism in EF for several years now and it would make sense to handle similar things (connection strings) in similar ways across the ecosystem.

If you just get the connection strings as variables beforehand and pass them in you can space out the logic and make this look a bit simpler.

That's obviously fair, I just try to avoid creating variables just for the sake of "looks" in general as variables introduce unneeded state that can by itself introduce other problems later. Code with more state is more error prone in general.

I try to reserve variable extraction for when something is used multiple times, or in cases where the variable adds significant semantic value to the code where having it helps explain what is happening and why.

Overall, the sentiment I've gotten is to leave it as is for now. Let me know if there's anything else we should consider.

It's your prerogative, of course. I appreciate you taking the suggestion, but for now I don't have anything else to propose.

From what I've shown above I'd actually like if the team went with the last option where we had stronger types to represent some of these concepts, which opens the door for stronger patterns without them feeling out of place (like the "2 overloads on string issue"). Of course, types like those would need to be defined in a very core abstraction dll, like Microsoft.Extensions.Configuration (for ConfigurationKey), and maybe some sort of generic Microsoft.Data.Abstractions for the ConnectionString type (or even Microsoft.Extensions.Primitives although that might be too general-purpose for this type), since that is not database-specific.

Anyways, feel free to close this if the team has decided not to approach it for now.

[julealgon]

@amerjusupovic you might want to use the "closed as not planned" instead of "closed as completed" mode to avoid giving the impression that this was actually implemented to folks going through the issues later.