SQLConnectionStringBuilder 5.0 is not Backwards Compatible with appSettings.json

[srothkin]

Describe the bug

With SqlClient 4, standard appSettings.json standard config file mapping from something like
{ "MyDb": { "Components": { "UserID": "blah", "Password": "foo", "DataSource": "hostname", "Encrypt": false } } }
to a SQLConnectionStringBuilder property
worked. With SqlClient 5.0, the resulting connection string is now returning Encrypt=True

I've tried changing encrypt value in json to "false", "False", "SqlConnectionEncryptOption.Optional", and "Optional". All of those still result in connection string with Encrypt=True.

To reproduce

    public class SqlConnectionString
    {
        public SqlConnectionStringBuilder Components { get; set; } = new();

        public override string ToString()
        {
            return Components.ConnectionString;
        }
    }

    public class Settings
    {
        [Required]
        public SqlConnectionString? MyDb { get; set; }
    }

            hostBuilder.ConfigureServices((ctx, services) =>
            {
                    var configuration = ctx.Configuration;
                    services.Configure<Settings>(configuration);
                    var settings = configuration.Get<Settings>();
                    Assert.Contains("Encrypt=false", settings.MyDb.ToString());
            }

Expected behavior

Expecting appSettings.json (or environment variable override of it) setting of "Encrypt": false to result in Encrypt=false in resulting connection string.

Further technical details

Microsoft.Data.SqlClient version: 5.0.1
.NET target: 6
SQL Server version: n/a
Operating system: Windows 10

Additional context
We're using the SqlConnectionStringBuilder to assemble/validate connection string components that are coming from different places (environment variables in helm chart, Azure keyvault, appSettings.json) into a single connection string. Our environment doesn't yet support encrypt=true/mandatory.

[lcheunglci]

Hi @srothkin, that sounds like a bug and will need to be investigated. As a workaround, could you try setting TrustServerCertificate=true from your appSettings.json? It's also possible that it's not actually reading the encrypt from the appSettings.json as the default value for encrypt is true.

[srothkin]

With SqlClient 4.0 it IS reading the encrypt value from appSettings.json. We have automated unit tests that verify this with SqlClient 4.0 and break with 5.0.1.

[srothkin]

Health check fails with TrustServerCertificate=true and Encrypt=True.

[srothkin]

So this blocks us from moving to SqlClient 5

[lcheunglci]

Thanks for the update. Were you able to connect without using the appSettings.json with that connection string with your health check failure with TrustServerCertificate=true on MDS 5.0 and what was the error message? Also, could you provide us a repro sample? Thanks!

[srothkin]

Actually TrustServerCertificate=true does work (there was another issue when I tested it yesterday). I'll get you a repro app later today.

[srothkin]

Automated unit tests in this sample all pass with SqlClient 4 but some fail with SqlClient 5.1
SqlConnectionStringBuilder5repro.zip

[lcheunglci]

Thanks @srothkin for providing the repro sample. I ran the tests, and it was reproducible. So far, I've briefly looked into the first failed test in the list i.e. ComponentsFromBothJsonAndEnvironmentVariablesTest and from LoadSettingsFromJsonFile method from my understand, it uses a builder that configures the custom SqlConnectionString class that uses the SqlConnectionStringBuilder, and from the context.Configuration, it looks like it does contain Encrypt property; however, after it is retrieve from the getter, I see that the connection string
{Data Source=c.local;Initial Catalog=MyDb;Persist Security Info=False;User ID=foo;Password=bar;Multiple Active Result Sets=True;Application Name=Replaced}
which does not contain the Encrypt=false meaning that the ConnectionString property from SqlConnectionStringBuilder may have considered false as the default value and hence not shown; however, with Encrypt being excluded in the connection string, it will be set to true/mandatory when used to open the connection. I'll have to spend some more time to investigate.

[lcheunglci]

Hi @srothkin , we did some investigation and tried to see if it invokes any of our conversion behavior in SqlConnectionStringBuilder, and it didn't seem to go into any of our logic. From a discussion with the team, it seems like the HostBuilderContext's Configuration might be using reflection to determine set the connection string parameters in SqlConnectionStringBuilder, and we suspect that it might be caused by our backward compatibility implementation using SqlConnectionEncryptOption to allow support for both the booland enum, but perhaps the default behavior from the configuration builder when failing to parse/convert it to this type is in omit the Encrypt parameter all together. Therefore, using a json configuration file regardless of what is set in Encrypt is ignored. Unfortunately, you're right that in this use case (i.e. using the configuration builder to set the SqlConnectionStringBuilder) broke between 4.0 and 5.0. We may need to coordinate with the ASP.NET team or the owner of that library to see how we can best address this implementation.

[DavoudEshtehari]

It's not reproducible using JsonXXX types to serialize and de-serialize into and from a stream. Basically, the underlying functions can't resolve or assign the Encrypt value and instead, silently use the default value by calling the Get<T>() extension.

cc @roji

[roji]

@DavoudEshtehari @lcheunglci am I understanding correctly, that the root cause here is that SqlConnectionStringBuilder.Encrypt is a now class (SqlConnectionEncryptOption), rather than a bool or enum, and therefore the deserialization of the options from appSettings.json isn't working properly?

I admit I haven't yet seen arbitrary classes as connection string components, I can imagine that would cause trouble in various systems interacting with connection strings...

[ErikEJ]

Could some serilisation hints be added?

[lcheunglci]

Thanks @ErikEJ and @roji for your responses. I'll do some investigation and see if we can introduce some json serialization attributes to serialize that particular class from json properly.

[roji]

Just curious: was there any specific reason to switch to a class rather than to an enum (which is what the class conceptually seems to be)? I didn't follow this change so am probably unaware of the details...

[lcheunglci]

Ironically, it was to add backward compatibility so that true and false can also be used instead a making it a breaking change and forcing the customer to use the enum types i.e. Mandatory, Optional and Strict from 4.0.x to 5.0, which seemed to work for most cases except this one.

[roji]

Oh I see, via the implicit casting operator.

I'm pretty sure you may find some other cases when arbitrary classes in connection strings won't work well. For example, people sometimes bind a Winforms PropertyGrid (I think that's what it's called) to a connection string builder, to allow users to manipulate the settings etc. This works with the primitive types, but I'm not sure it would work well with arbitrary classes. VS DDEX may be another problem.

[lcheunglci]

Thanks @roji , if that's the case, it looks like we may want to consider a different implementation to add backward compatibility support.