Preserve code-only driver settings on DbConnection/DbDataSource paths

src/EFCore.ClickHouse/Storage/Internal/ClickHouseRelationalConnection.cs

@@ -45,39 +45,64 @@ protected override DbConnection CreateDbConnection()
     protected override bool SupportsAmbientTransactions => false;
 
     /// <summary>
-    /// Ensures <c>join_use_nulls=1</c> is present in the underlying connection's connection
-    /// string before it's opened. Required because ClickHouse's default (0) makes LEFT JOIN
-    /// return column defaults rather than NULL, which breaks EF Core's null-based navigation
-    /// detection.
+    /// Ensures <c>join_use_nulls=1</c> is applied to the underlying connection before it is
+    /// opened. Required because ClickHouse's default (0) makes LEFT JOIN return column defaults
+    /// rather than NULL, which breaks EF Core's null-based navigation detection. For a
+    /// <see cref="ClickHouseConnection"/> the setting is recorded on its
+    /// <see cref="ClickHouseConnection.Settings"/>; the connection-string fallback is used only
+    /// for a non-ClickHouse <see cref="DbConnection"/>.
     ///
     /// The ClickHouse.Driver HTTP protocol is stateless by default (<c>UseSession=False</c>):
     /// a standalone <c>SET join_use_nulls = 1</c> statement does not persist to subsequent
-    /// queries. Instead the driver applies <c>set_*</c> connection-string parameters as URL
-    /// parameters on every query, so we must mutate the connection string itself.
+    /// queries. Instead the driver applies <c>set_*</c> parameters (the driver's
+    /// <c>CustomSettings</c>) as URL parameters on every query, so we must record the setting on
+    /// the connection before it is opened.
     ///
     /// Applies on all paths. For the connection-string path the setting is already baked in by
     /// <see cref="ClickHouseDataSourceManager.EnsureDefaultSettings"/>; this override covers
     /// the <see cref="DbConnection"/> and <see cref="DbDataSource"/> paths where EF hands us
     /// connection objects we didn't construct. We only mutate when the connection is Closed
     /// and the user has not explicitly configured <c>join_use_nulls</c>.
+    ///
+    /// For a <see cref="ClickHouseConnection"/> we inject the setting by copying and reassigning
+    /// its <see cref="ClickHouseConnection.Settings"/> rather than rewriting
+    /// <see cref="DbConnection.ConnectionString"/>. Assigning the connection string rebuilds the
+    /// driver's settings purely from the string, silently discarding code-only settings such as
+    /// <c>SkipServerCertificateValidation</c> or a custom <c>HttpClient</c>. Only the string
+    /// fallback (for a non-ClickHouse <see cref="DbConnection"/>) mutates the connection string.
     /// </summary>
     public override bool Open(bool errorsExpected = false)
     {
-        EnsureJoinUseNullsInConnectionString();
+        EnsureJoinUseNulls();
         return base.Open(errorsExpected);
     }
 
     public override Task<bool> OpenAsync(CancellationToken cancellationToken, bool errorsExpected = false)
     {
-        EnsureJoinUseNullsInConnectionString();
+        EnsureJoinUseNulls();
         return base.OpenAsync(cancellationToken, errorsExpected);
     }
 
-    private void EnsureJoinUseNullsInConnectionString()
+    private void EnsureJoinUseNulls()
     {
         if (_joinNullSemanticsDisabled || DbConnection.State != ConnectionState.Closed)
             return;
 
+        // Prefer mutating the strongly-typed settings so that code-only settings (which have no
+        // connection-string equivalent) are preserved. Rewriting ConnectionString would rebuild
+        // the driver's settings from the string alone and drop them.
+        if (DbConnection is ClickHouseConnection clickHouseConnection)
+        {
+            var settings = clickHouseConnection.Settings;
+            if (settings.CustomSettings.ContainsKey(JoinUseNullsSetting))
+                return;
+
+            var updatedSettings = new ClickHouseClientSettings(settings);
+            updatedSettings.CustomSettings[JoinUseNullsSetting] = "1";
+            clickHouseConnection.Settings = updatedSettings;
+            return;
+        }
+
         var cs = DbConnection.ConnectionString;
         if (string.IsNullOrEmpty(cs)
             || cs.Contains("join_use_nulls", StringComparison.OrdinalIgnoreCase))
@@ -88,26 +113,59 @@ private void EnsureJoinUseNullsInConnectionString()
         DbConnection.ConnectionString = ClickHouseDataSourceManager.EnsureDefaultSettings(cs);
     }
 
+    private const string JoinUseNullsSetting = "join_use_nulls";
+
     public IClickHouseRelationalConnection CreateMasterConnection()
     {
-        var connectionStringBuilder = new ClickHouseConnectionStringBuilder(
-            _dataSource?.ConnectionString ?? ConnectionString)
-        {
-            Database = "default"
-        };
+        var optionsBuilder = new DbContextOptionsBuilder();
 
-        var masterConnectionString = connectionStringBuilder.ConnectionString;
+        if (TryGetClickHouseClientSettings(out var settings))
+        {
+            // Clone the driver settings and only swap the database. Round-tripping through a
+            // connection string here would drop code-only settings that have no connection-string
+            // representation (SkipServerCertificateValidation, BearerToken, HttpClient,
+            // HttpClientFactory, LoggerFactory, CustomHeaders, ApplicationInfo, EnableDebugMode,
+            // and the parameter/read converters), breaking database create/drop against servers
+            // that depend on them (e.g. a self-signed certificate). The connection is owned by the
+            // context so it is disposed with the master connection.
+            var masterSettings = new ClickHouseClientSettings(settings) { Database = "default" };
+            optionsBuilder.UseClickHouse(new ClickHouseConnection(masterSettings), contextOwnsConnection: true);
+        }
+        else
+        {
+            var masterConnectionString = new ClickHouseConnectionStringBuilder(
+                _dataSource?.ConnectionString ?? ConnectionString)
+            {
+                Database = "default"
+            }.ConnectionString;
 
-        var contextOptions = new DbContextOptionsBuilder()
-            .UseClickHouse(masterConnectionString)
-            .Options;
+            optionsBuilder.UseClickHouse(masterConnectionString);
+        }
 
         return new ClickHouseRelationalConnection(
-            Dependencies with { ContextOptions = contextOptions },
+            Dependencies with { ContextOptions = optionsBuilder.Options },
             dataSource: null,
             _joinNullSemanticsDisabled);
     }
 
+    private bool TryGetClickHouseClientSettings(out ClickHouseClientSettings settings)
+    {
+        if (_dataSource is ClickHouseDataSource clickHouseDataSource)
+        {
+            settings = clickHouseDataSource.Settings;
+            return true;
+        }
+
+        if (DbConnection is ClickHouseConnection clickHouseConnection)
+        {
+            settings = clickHouseConnection.Settings;
+            return true;
+        }
+
+        settings = null!;
+        return false;
+    }
+
     public override IDbContextTransaction BeginTransaction(IsolationLevel isolationLevel)
         => new ClickHouseTransaction();
 

test/EFCore.ClickHouse.Tests/ConnectionSettingsTests.cs

@@ -1,5 +1,7 @@
 using ClickHouse.Driver.ADO;
+using ClickHouse.EntityFrameworkCore.Storage.Internal;
 using Microsoft.EntityFrameworkCore;
+using Microsoft.EntityFrameworkCore.Infrastructure;
 using Xunit;
 
 namespace EFCore.ClickHouse.Tests;
@@ -96,6 +98,187 @@ public async Task UseClickHouse_DbConnection_RespectsUserOptOut()
         Assert.False(await GetJoinUseNullsAsync(ctx));
     }
 
+    [Fact]
+    public async Task UseClickHouse_DbConnection_PreservesCodeOnlySettings()
+    {
+        // Settings that only exist in code (no connection-string equivalent) must survive the
+        // join_use_nulls injection performed on Open. Previously
+        // the provider rewrote ConnectionString, which rebuilt the driver's settings from the
+        // string alone and silently dropped SkipServerCertificateValidation.
+        var settings = new ClickHouseClientSettings(_fixture.ConnectionString)
+        {
+            SkipServerCertificateValidation = true
+        };
+
+        await using var connection = new ClickHouseConnection(settings);
+        await using var ctx = new MinimalContext(o => o.UseClickHouse(connection));
+
+        Assert.True(await GetJoinUseNullsAsync(ctx));
+        Assert.True(connection.Settings.SkipServerCertificateValidation);
+    }
+
+    [Fact]
+    public async Task UseClickHouse_DbDataSource_PreservesCodeOnlySettings()
+    {
+        var settings = new ClickHouseClientSettings(_fixture.ConnectionString)
+        {
+            SkipServerCertificateValidation = true
+        };
+
+        await using var dataSource = new ClickHouseDataSource(settings);
+        await using var ctx = new MinimalContext(o => o.UseClickHouse(dataSource));
+
+        Assert.True(await GetJoinUseNullsAsync(ctx));
+
+        var connection = (ClickHouseConnection)ctx.Database.GetDbConnection();
+        Assert.True(connection.Settings.SkipServerCertificateValidation);
+    }
+
+    [Fact]
+    public void CreateMasterConnection_DbDataSource_PreservesCodeOnlySettings()
+    {
+        // The master connection (used for CREATE/DROP DATABASE) must not lose code-only settings
+        // by round-tripping through a connection string. Exercises several settings that have no
+        // connection-string representation, not just SkipServerCertificateValidation.
+        var settings = new ClickHouseClientSettings(_fixture.ConnectionString)
+        {
+            SkipServerCertificateValidation = true,
+            BearerToken = "token-abc",
+            CustomHeaders = new Dictionary<string, string> { ["X-Custom"] = "value" }
+        };
+
+        using var dataSource = new ClickHouseDataSource(settings);
+        using var ctx = new MinimalContext(o => o.UseClickHouse(dataSource));
+
+        var master = ctx.Database.GetService<IClickHouseRelationalConnection>().CreateMasterConnection();
+        try
+        {
+            var masterConnection = (ClickHouseConnection)master.DbConnection;
+            Assert.Equal("default", masterConnection.Settings.Database);
+            Assert.True(masterConnection.Settings.SkipServerCertificateValidation);
+            Assert.Equal("token-abc", masterConnection.Settings.BearerToken);
+            Assert.Equal("value", masterConnection.Settings.CustomHeaders["X-Custom"]);
+        }
+        finally
+        {
+            master.Dispose();
+        }
+    }
+
+    [Fact]
+    public void CreateMasterConnection_PreservesUserJoinUseNullsOptOut()
+    {
+        // If the user opted out via set_join_use_nulls=0, the master connection must carry that
+        // through so the injection on Open sees it and does not override the choice.
+        var optOutConnectionString = _fixture.ConnectionString
+            + (_fixture.ConnectionString.EndsWith(';') ? "" : ";")
+            + "set_join_use_nulls=0";
+
+        using var connection = new ClickHouseConnection(optOutConnectionString);
+        using var ctx = new MinimalContext(o => o.UseClickHouse(connection));
+
+        var master = ctx.Database.GetService<IClickHouseRelationalConnection>().CreateMasterConnection();
+        try
+        {
+            var masterConnection = (ClickHouseConnection)master.DbConnection;
+            Assert.Equal("0", masterConnection.Settings.CustomSettings["join_use_nulls"].ToString());
+        }
+        finally
+        {
+            master.Dispose();
+        }
+    }
+
+    [Fact]
+    public void CreateMasterConnection_DbConnection_PreservesCodeOnlySettings()
+    {
+        var settings = new ClickHouseClientSettings(_fixture.ConnectionString)
+        {
+            SkipServerCertificateValidation = true
+        };
+
+        using var connection = new ClickHouseConnection(settings);
+        using var ctx = new MinimalContext(o => o.UseClickHouse(connection));
+
+        var master = ctx.Database.GetService<IClickHouseRelationalConnection>().CreateMasterConnection();
+        try
+        {
+            var masterConnection = (ClickHouseConnection)master.DbConnection;
+            Assert.Equal("default", masterConnection.Settings.Database);
+            Assert.True(masterConnection.Settings.SkipServerCertificateValidation);
+        }
+        finally
+        {
+            master.Dispose();
+        }
+    }
+
+    [Fact]
+    public void EnsureJoinUseNulls_NonClickHouseConnection_FallsBackToConnectionString()
+    {
+        // Defensive fallback: for a DbConnection that is not a ClickHouseConnection we cannot
+        // mutate driver settings, so join_use_nulls is injected by rewriting the connection string.
+        using var connection = new FakeDbConnection("Host=localhost;Port=8123");
+        using var ctx = new MinimalContext(o => o.UseClickHouse(connection));
+
+        ctx.Database.OpenConnection();
+
+        Assert.Contains("set_join_use_nulls=1", connection.ConnectionString, StringComparison.OrdinalIgnoreCase);
+    }
+
+    [Fact]
+    public void EnsureJoinUseNulls_NonClickHouseConnection_LeavesExistingSettingUntouched()
+    {
+        // If the connection string already configures join_use_nulls, the fallback must not append.
+        const string configured = "Host=localhost;Port=8123;set_join_use_nulls=0";
+        using var connection = new FakeDbConnection(configured);
+        using var ctx = new MinimalContext(o => o.UseClickHouse(connection));
+
+        ctx.Database.OpenConnection();
+
+        Assert.Equal(configured, connection.ConnectionString);
+    }
+
+    [Fact]
+    public void CreateMasterConnection_NonClickHouseConnection_FallsBackToConnectionString()
+    {
+        // When neither a ClickHouseDataSource nor a ClickHouseConnection is available, the master
+        // connection is built from the connection string.
+        using var connection = new FakeDbConnection("Host=localhost;Port=8123;Database=app");
+        using var ctx = new MinimalContext(o => o.UseClickHouse(connection));
+
+        var master = ctx.Database.GetService<IClickHouseRelationalConnection>().CreateMasterConnection();
+        try
+        {
+            var masterConnection = (ClickHouseConnection)master.DbConnection;
+            Assert.Equal("default", masterConnection.Settings.Database);
+        }
+        finally
+        {
+            master.Dispose();
+        }
+    }
+
+    [Fact]
+    public void CreateMasterConnection_NonClickHouseDataSource_UsesDataSourceConnectionString()
+    {
+        // A non-ClickHouse DbDataSource does not set a connection string on the EF options, so the
+        // fallback must read it from the data source itself rather than the (empty) options value.
+        using var dataSource = new FakeDbDataSource("Host=localhost;Port=8123;Database=app");
+        using var ctx = new MinimalContext(o => o.UseClickHouse(dataSource));
+
+        var master = ctx.Database.GetService<IClickHouseRelationalConnection>().CreateMasterConnection();
+        try
+        {
+            var masterConnection = (ClickHouseConnection)master.DbConnection;
+            Assert.Equal("default", masterConnection.Settings.Database);
+        }
+        finally
+        {
+            master.Dispose();
+        }
+    }
+
     private static async Task<bool> GetJoinUseNullsAsync(DbContext ctx)
     {
         var connection = ctx.Database.GetDbConnection();
@@ -118,6 +301,57 @@ private static async Task<bool> GetJoinUseNullsAsync(DbContext ctx)
     }
 }
 
+/// <summary>
+/// Minimal non-ClickHouse <see cref="System.Data.Common.DbConnection"/> used to exercise the
+/// provider's defensive fallback paths that only run when the underlying connection is not a
+/// <see cref="ClickHouseConnection"/>.
+/// </summary>
+internal sealed class FakeDbConnection : System.Data.Common.DbConnection
+{
+    private System.Data.ConnectionState _state = System.Data.ConnectionState.Closed;
+
+    public FakeDbConnection(string connectionString) => ConnectionString = connectionString;
+
+    [System.Diagnostics.CodeAnalysis.AllowNull]
+    public override string ConnectionString { get; set; }
+
+    public override string Database => "app";
+
+    public override string DataSource => "fake";
+
+    public override string ServerVersion => "0.0";
+
+    public override System.Data.ConnectionState State => _state;
+
+    public override void Open() => _state = System.Data.ConnectionState.Open;
+
+    public override void Close() => _state = System.Data.ConnectionState.Closed;
+
+    public override void ChangeDatabase(string databaseName) { }
+
+    protected override System.Data.Common.DbTransaction BeginDbTransaction(System.Data.IsolationLevel isolationLevel)
+        => throw new NotSupportedException();
+
+    protected override System.Data.Common.DbCommand CreateDbCommand()
+        => throw new NotSupportedException();
+}
+
+/// <summary>
+/// Minimal non-ClickHouse <see cref="System.Data.Common.DbDataSource"/> used to verify that
+/// <c>CreateMasterConnection</c> falls back to the data source's own connection string.
+/// </summary>
+internal sealed class FakeDbDataSource : System.Data.Common.DbDataSource
+{
+    private readonly string _connectionString;
+
+    public FakeDbDataSource(string connectionString) => _connectionString = connectionString;
+
+    public override string ConnectionString => _connectionString;
+
+    protected override System.Data.Common.DbConnection CreateDbConnection()
+        => new FakeDbConnection(_connectionString);
+}
+
 public class ConnectionSettingsFixture : IAsyncLifetime
 {
     public string ConnectionString { get; private set; } = string.Empty;