Add NpgsqlDataSource.ReloadTypes{Async}#5919
Conversation
manandre
force-pushed
the
data-source-reload-types
branch
from
7ae26f7 to
8544c7c
Compare
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
Thanks @manandre, looks good overall! Can you please take a look at the comments?
.NET 9.0 is coming out on Tuesday, and we'll likely want to release Npgsql 9.0 very soon afterwards. So if you don't have time to quickly fix these comments, let me know and I'll take over this and complete it.
test/Npgsql.Tests/DataSourceTests.cs
Outdated
| } | ||
|
|
||
| [Test] | ||
| public async Task ReloadTypesAsync() |
There was a problem hiding this comment.
This test can be merged with the above, to exercise both sync and async in the same test (with a [Values] bool async parameter). Note that the only thing that needs to differ between the two tests is the invocation of ReloadTypes() vs. ReloadTypesAsync() - the rest of the code (e.g. the ExecuteScalarAsync()) can be async for both cases (that's not what we're testing here).
test/Npgsql.Tests/DataSourceTests.cs
Outdated
| Assert.DoesNotThrowAsync(async () => await connection2.ExecuteScalarAsync($"SELECT 'happy'::{type}")); | ||
| } | ||
|
|
||
| private NpgsqlDataSource CreateDataSourceWithType(string type) |
There was a problem hiding this comment.
I'd personally just inline this, doesn't seem worth it for 3 lines
test/Npgsql.Tests/TestUtil.cs
Outdated
| Unpooled | ||
| } | ||
|
|
||
| enum Mood { Sad, Ok, Happy } |
There was a problem hiding this comment.
I'd rather we kept this in the specific test classes rather than making it available in all classes, even if it's a bit of duplication...
src/Npgsql/NpgsqlDataSource.cs
Outdated
| /// Flushes the type cache for this data source. | ||
| /// Type changes will appear for connections only after they are re-opened from the pool. | ||
| /// </summary> | ||
| public async Task ReloadTypesAsync() |
There was a problem hiding this comment.
This is missing a CancellationToken parameter; I can see that the NpgsqlConnection.ReloadTypesAsync() overload is similarly missing it, I'm not sure why - maybe add it there as well as part of this PR?
src/Npgsql/PublicAPI.Shipped.txt
Outdated
| Npgsql.NpgsqlConnection.ProvidePasswordCallback.set -> void | ||
| Npgsql.NpgsqlConnection.ReloadTypes() -> void | ||
| Npgsql.NpgsqlConnection.ReloadTypesAsync() -> System.Threading.Tasks.Task! | ||
| Npgsql.NpgsqlConnection.ReloadTypesAsync(System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> System.Threading.Tasks.Task! |
There was a problem hiding this comment.
This is technically incorrect: the "Shipped" file shouldn't ever get modified (it represents the current release, 8.0) - I'll take care of it.
roji
force-pushed
the
data-source-reload-types
branch
from
a41623a to
2986ecd
Compare
2 participants
Fixes #5905