Improve Npgsql health check design #2116
Conversation
- make NpgSqlHealthCheckOptions.DataSource internal, so the users can't make mistakes by assigning it to a new data source for every health check invocation - make sure not more than a single DataSource is created - when the user provides ConnectionString, try to resolve DataSource just once. If it's present and the connection string is the same, reuse it
- offer two public NpgSqlHealthCheckOptions ctors: one that requires ConnectionString and another that requires DataSource. This allows us to ensure that every instance of this type is valid. - make DataSource property public, but readonly. This allows us to ensure that the customers are not providing both.
adamsitnik
changed the title
|
|
||
| This health check verifies the ability to communicate with [PostgreSQL](https://www.postgresql.org/). It uses the [Npgsql](https://www.npgsql.org/) library. | ||
|
|
||
| ## NpgsqlDataSource |
There was a problem hiding this comment.
@roji @NinoFloris PTAL at what I wrote here and let me know if this is correct
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #2116 +/- ##
==========================================
- Coverage 64.94% 64.46% -0.49%
==========================================
Files 262 246 -16
Lines 8496 8318 -178
Branches 600 579 -21
==========================================
- Hits 5518 5362 -156
+ Misses 2828 2811 -17
+ Partials 150 145 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…ly a connection string was provided
|
I was unable to get a review, so I've eyeballed it multiple times. It LGTM, so I am merging it. In case I am wrong, we are going to release a fix later. |
There was a problem hiding this comment.
Sorry for the super late review, but overall everything here seems good! Yeah, the important thing is to have health checks automatically resolve a singleton NpgsqlDataSource from DI (previously registered via Npgsql.DependencyInjection), and for other cases, to provide ways to register with either a provided data source or a connection string.
|
|
||
| ## NpgsqlDataSource | ||
|
|
||
| Starting with Npgsql 7.0 (and .NET 7), the starting point for any database operation is [NpgsqlDataSource](https://www.npgsql.org/doc/api/Npgsql.NpgsqlDataSource.html). The data source represents your PostgreSQL database, and can hand out connections to it, or support direct execution of SQL against it. The data source encapsulates the various Npgsql configuration needed to connect to PostgreSQL, as well as the **connection pooling which makes Npgsql efficient**. |
There was a problem hiding this comment.
I'd maybe go softer and say "the recommended starting point", as connection strings are still fully supported etc.
| } | ||
| ``` | ||
|
|
||
| By default, the `NpgsqlDataSource` instance is resolved from service provider. If you need to access more than one PostgreSQL database, you can use keyed DI services to achieve that: |
| void Configure(IServiceCollection services) | ||
| { | ||
| services.AddNpgsqlDataSource("Host=pg_server;Username=test;Password=test;Database=test"); | ||
| services.AddHealthChecks().AddNpgSql(); |
There was a problem hiding this comment.
Yep, that's the ideal way things should work - the data source is registered independently as a singleton in DI (via Npgsql.DependencyInjection), and then is implicitly picked up by anyone needing it (health checks, EF Core...). A bit similar to an ILoggerFactory.
This PR implements #2113 for Npgsql:
NpgSqlHealthCheckOptionsctors: one that requiresConnectionStringand another that requiresDataSource. This allows us to ensure that every instance of this type is valid.ConnectionStringandDataSourceproperties readonly. This allows us to ensure that the customers are not providing both.