Allow sending queries from INpgsqlDatabaseInfoFactory.Load - #6634
Conversation
| /// database isn't of the correct type and isn't handled by this factory. | ||
| /// </returns> | ||
| Task<NpgsqlDatabaseInfo?> Load(NpgsqlConnector conn, NpgsqlTimeout timeout, bool async); | ||
| Task<NpgsqlDatabaseInfo?> Load(NpgsqlConnector conn, NpgsqlTimeout timeout, bool async, CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
This is a breaking change.
Do the strict rules also apply to members being experimental?
There was a problem hiding this comment.
This is indeed a breaking change, although given that this API was flawed for years and no one complained, I guess it's not really used by anyone other than us. But let's see what others also think.
There was a problem hiding this comment.
Yeah, this is a rarely-used extensibility API that's explicitly considered internal. On the other hand if anyone's using it, they're likely a library supporting some quirky database situation, and then it's not great to need to have two versions for different versions of Npgsql just because of this. I've done this kind of change in the past and regretted it, I'd personally probably just keep the old overload and calling into the new to preserve binary compat (there's no price to it really).
But no strong feelings from my side.
There was a problem hiding this comment.
Hmm, something like this I think should work?
There was a problem hiding this comment.
Pull request overview
Allows custom database-info factories to execute queries during database metadata loading.
Changes:
- Moves user-action ownership into PostgreSQL’s built-in loader.
- Propagates cancellation tokens through factory loading.
- Adds synchronous and asynchronous regression coverage.
Show a summary per file
| File | Description |
|---|---|
src/Npgsql/Internal/INpgsqlDatabaseInfoFactory.cs |
Adds cancellation-token support. |
src/Npgsql/Internal/NpgsqlDatabaseInfo.cs |
Forwards cancellation tokens to factories. |
src/Npgsql/NpgsqlDataSource.cs |
Removes the bootstrap-wide user action. |
src/Npgsql/PostgresDatabaseInfo.cs |
Scopes user action to built-in metadata queries. |
src/Npgsql/PostgresMinimalDatabaseInfo.cs |
Updates the factory signature. |
test/Npgsql.Tests/ConnectionTests.cs |
Tests querying during factory loading. |
test/Npgsql.Tests/TransactionTests.cs |
Updates the custom test factory. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Balanced
Fixes #6633