-
Notifications
You must be signed in to change notification settings - Fork 7
Preserve code-only driver settings on DbConnection/DbDataSource paths #51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
9193ba5
9ee68ed
45edc2d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,32 +52,55 @@ protected override DbConnection CreateDbConnection() | |
| /// | ||
| /// 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 +111,58 @@ 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(ConnectionString) | ||
| { | ||
| Database = "default" | ||
| }.ConnectionString; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — fixed in 45edc2d. The fallback now uses |
||
|
|
||
| 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(); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-ClickHouseDbConnection.