Repository navigation
Fix ClickHouseDataSource: OpenConnectionAsync() without a token returns ClickHouseConnection - #688
polyglotAI-bot wants to merge 2 commits into
Conversation
…ns ClickHouseConnection
ClickHouseDataSource.OpenConnectionAsync(CancellationToken) had no default
value for its token. A call with no argument did not bind to it and bound to
the base DbDataSource.OpenConnectionAsync(CancellationToken = default), which
is typed ValueTask<DbConnection>. The documented ADO.NET examples that then
call connection.CreateCommand("...") did not compile (CS1501).
Declare the token as `= default`, as IClickHouseDataSource already does, and
update the PublicAPI files.
Fixes: #686
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The source-breaking return-type change needs user-facing migration guidance beyond the PR description.
1 open finding
What changed in this PR
Fixes #686 in the HTTP client's ADO.NET API so awaiting ClickHouseDataSource.OpenConnectionAsync() returns ClickHouseConnection.
Changes:
- Makes the cancellation token optional.
- Updates API records and adds a regression test.
- Adds a changelog fragment.
| File | Description |
|---|---|
| ClickHouse.Driver/PublicAPI/PublicAPI.Unshipped.txt | Records the optional token. |
| ClickHouse.Driver/PublicAPI/net8.0/PublicAPI.Shipped.txt | Updates the framework-specific signature. |
| ClickHouse.Driver/ADO/ClickHouseDataSource.cs | Defaults the cancellation token. |
| ClickHouse.Driver.Tests/ADO/DataSourceTests.cs | Tests the no-argument call and command execution. |
| changelog.d/686-datasource-openconnectionasync.fixes.md | Announces the fix. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| IClickHouseConnection IClickHouseDataSource.OpenConnection() => OpenConnection(); | ||
|
|
||
| public new async Task<ClickHouseConnection> OpenConnectionAsync(CancellationToken cancellationToken) | ||
| public new async Task<ClickHouseConnection> OpenConnectionAsync(CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Checked on this branch. The source change is real: ValueTask<DbConnection> t = ds.OpenConnectionAsync(); now fails with CS0029, and .AsTask() / .Preserve() on the call fail with CS1061. ((DbDataSource)ds).OpenConnectionAsync() still compiles and returns ValueTask<DbConnection>. DbConnection c = await ds.OpenConnectionAsync(); also still compiles.
- Changelog: done in ac7929b. The
686fragment now has a sub-bullet that names the new return type, the code that stops compiling, and the two ways to keep it compiling.changelog.cs --checkpasses. docs/http.mdx: no change. The docs page describes the current API, and the#ado-net-datasourceexamples are already correct for it. The page has no per-version compatibility notes; its only migration guide is for theClickHouse.Client→ClickHouse.Driverrename.CHANGELOG.mdis where this repo records source breaks for an upgrade (for example theLogger→LoggerFactoryentry). I left this thread open so a maintainer can decide whether they also want a docs note.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
A no-argument call on ClickHouseDataSource now returns Task<ClickHouseConnection>, not ValueTask<DbConnection>. Code that stores the call as ValueTask<DbConnection>, or calls .AsTask()/.Preserve() on it, no longer compiles. Tell users how to keep compiling. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Summary
Fixes #686 (solution 1, as the maintainer chose).
ClickHouseDataSource.OpenConnectionAsync(CancellationToken cancellationToken)had no default value for its token. So a call with no argument,await dataSource.OpenConnectionAsync(), did not bind to it. It bound to the baseDbDataSource.OpenConnectionAsync(CancellationToken = default), which is typedValueTask<DbConnection>. The documented ADO.NET examples indocs/http.mdxthen callconnection.CreateCommand("..."), which does not exist onDbConnection, and fail with CS1501. The fix declares the token asCancellationToken cancellationToken = default, asIClickHouseDataSource.OpenConnectionAsyncalready does. The no-argument call now binds to theClickHouseDataSourcemethod and is typedTask<ClickHouseConnection>.Changes
ClickHouse.Driver/ADO/ClickHouseDataSource.cs: add= defaultto the token ofOpenConnectionAsync.PublicAPI/PublicAPI.Unshipped.txtandPublicAPI/net8.0/PublicAPI.Shipped.txt: update the signature line (the only two files that list it).changelog.d/686-datasource-openconnectionasync.fixes.md: changelog fragment.docs/http.mdxsnippets are now correct as written.Test
DataSourceTests.OpenConnectionAsync_WithoutCancellationToken_ReturnsOpenClickHouseConnectionopens a connection with the no-argument call against the server. It assigns the result toClickHouseConnection, checks that the state isOpen, and runsCreateCommand("SELECT 1"). Without the fix the test project does not compile (CS0266: cannot convertDbConnectiontoClickHouseConnection). With the fix it passes.Also checked by hand against a 26.9 server: the three
docs/http.mdxsnippets from the issue (lines 1296, 1350 and 1556) now compile and run. These calls keep their previous types: an explicit token (Task<ClickHouseConnection>), a call throughIClickHouseDataSource(Task<IClickHouseConnection>), and a call through aDbDataSource-typed variable (ValueTask<DbConnection>).Compatibility
ClickHouseDataSource-typed variable is typedTask<ClickHouseConnection>, notValueTask<DbConnection>. Code that usesvar/awaitkeeps compiling. Code that stores the unawaited call in an explicitValueTask<DbConnection>, or calls.AsTask()/.Preserve()on it, stops compiling. Calls throughDbDataSource, throughIClickHouseDataSource, and method-group conversions do not change.Checklist
🤖 Generated with Claude Code