Preserve code-only driver settings on DbConnection/DbDataSource paths - #51
Preserve code-only driver settings on DbConnection/DbDataSource paths#51alex-clickhouse wants to merge 3 commits into
Conversation
Settings that exist only in code and have no connection-string representation (SkipServerCertificateValidation, BearerToken, HttpClient, HttpClientFactory, LoggerFactory, CustomHeaders, ApplicationInfo, EnableDebugMode, and the parameter/read converters) were silently dropped whenever the provider round-tripped a connection through its connection string. The driver's ClickHouseConnection.ConnectionString setter rebuilds Settings purely from the string, so any code-only setting is lost. Two sites in ClickHouseRelationalConnection did this: - EnsureJoinUseNulls (Open/OpenAsync): now injects join_use_nulls via a cloned Settings object for a ClickHouseConnection instead of rewriting ConnectionString. The string rewrite remains only as a fallback for a non-ClickHouse DbConnection. The user opt-out (set_join_use_nulls=0) is still honored. - CreateMasterConnection (CREATE/DROP DATABASE): now clones the driver settings and only swaps Database="default", handing back a context-owned ClickHouseConnection, instead of reconstructing from the connection string. ApplicationInfo (the lib User-Agent tag) is preserved via the copy constructor. Fixes #50. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR fixes a provider bug where configuring ClickHouse via UseClickHouse(DbConnection) or UseClickHouse(DbDataSource) could silently drop driver settings that have no connection-string representation (e.g., SkipServerCertificateValidation), by avoiding round-trips through DbConnection.ConnectionString and instead cloning/preserving ClickHouseClientSettings.
Changes:
- Update
ClickHouseRelationalConnection.Open/OpenAsyncto injectjoin_use_nullsviaClickHouseConnection.Settings.CustomSettings(with a connection-string fallback for non-ClickHouse connections). - Update
CreateMasterConnection()to clone driver settings and only switchDatabase = "default"instead of rebuilding settings from a connection string. - Add regression tests ensuring code-only settings and
join_use_nullsopt-out behavior are preserved across both DbConnection and DbDataSource configuration paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/EFCore.ClickHouse/Storage/Internal/ClickHouseRelationalConnection.cs |
Preserves code-only driver settings by cloning settings for join_use_nulls injection and master connections. |
test/EFCore.ClickHouse.Tests/ConnectionSettingsTests.cs |
Adds regression coverage for preserving code-only settings and join_use_nulls opt-out across configuration paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var masterConnectionString = new ClickHouseConnectionStringBuilder(ConnectionString) | ||
| { | ||
| Database = "default" | ||
| }.ConnectionString; |
There was a problem hiding this comment.
Good catch — fixed in 45edc2d. The fallback now uses _dataSource?.ConnectionString ?? ConnectionString, so a non-ClickHouse DbDataSource (which sets no connection string on the EF options) builds a valid master connection string. Added CreateMasterConnection_NonClickHouseDataSource_UsesDataSourceConnectionString to cover it.
| /// 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. |
There was a problem hiding this comment.
Fixed in 45edc2d — reworded the summary to state that the setting is recorded on ClickHouseConnection.Settings, with the connection-string path used only as a fallback for a non-ClickHouse DbConnection.
Adds a minimal fake DbConnection to exercise the defensive fallback branches in EnsureJoinUseNulls, CreateMasterConnection, and TryGetClickHouseClientSettings that only run when the underlying connection is not a ClickHouseConnection, bringing patch coverage to 100%. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- CreateMasterConnection fallback now uses `_dataSource?.ConnectionString ?? ConnectionString` so a non-ClickHouse DbDataSource (which sets no connection string on the EF options) builds a valid master connection string instead of an empty one. - Corrected the EnsureJoinUseNulls XML doc to reflect that the setting is recorded on ClickHouseConnection.Settings, with the connection-string path used only as a fallback. - Added a regression test covering the non-ClickHouse DbDataSource path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Fixes #50. When a user configures the provider via
UseClickHouse(DbConnection)orUseClickHouse(DbDataSource)using aClickHouseClientSettingsbuilt in code, settings that have no connection-string representation were silently dropped — most visiblySkipServerCertificateValidation, which is why connecting to a server with a self-signed certificate failed under EF Core even though the same settings work with a bareClickHouseClient.Root cause
The driver's
ClickHouseConnection.ConnectionStringsetter doesSettings = new ClickHouseClientSettings(value)— it rebuilds settings purely from the connection string. Anything not representable as a connection-string key is lost on a round-trip:SkipServerCertificateValidation,BearerToken,HttpClient,HttpClientFactory,HttpClientName,LoggerFactory,EnableDebugMode,CustomHeaders,ApplicationInfo, and the parameter/read converters.Two sites in
ClickHouseRelationalConnectionround-tripped through the string:EnsureJoinUseNulls(called fromOpen/OpenAsync) — rewroteConnectionStringto injectjoin_use_nulls=1. This hit both theDbConnectionandDbDataSourcepaths.CreateMasterConnection(used byClickHouseDatabaseCreatorforCREATE/DROP DATABASE, i.e.EnsureCreated/EnsureDeleted/migrations) — rebuilt the master connection from the connection string. The same self-signed-cert user would still fail on database create/drop even with add aggregate method translator #1 fixed.Fix
EnsureJoinUseNullsnow injectsjoin_use_nullsby cloningSettingsand settingCustomSettings["join_use_nulls"] = "1"for aClickHouseConnection, rather than rewritingConnectionString. The string rewrite remains only as a fallback for a non-ClickHouseDbConnection. The user opt-out (set_join_use_nulls=0) is still honored.CreateMasterConnectionnow clones the driver settings and only swapsDatabase = "default", handing back a context-ownedClickHouseConnection, instead of reconstructing from the connection string.ApplicationInfo(thelibUser-Agent tag added onmain) is preserved via the copy constructor.Testing
ConnectionSettingsTestscovering both fixed sites and both input paths, assertingSkipServerCertificateValidation,BearerToken, andCustomHeaderssurvive, plus that thejoin_use_nullsopt-out is preserved on the master path. These fail against pre-fix code.ConnectionSettingsTestspass against a real ClickHouse container.Notes
The durable guard against reintroducing this class of bug is the
TryGetClickHouseClientSettingshelper (clone-the-settings-object pattern). Any future code that reconstructs a connection from.ConnectionStringwould reintroduce the defect; a full-codebase review found no other current instances.🤖 Generated with Claude Code