hangfire-postgres / Hangfire.PostgreSql

PostgreSql Storage Provider for Hangfire
Other
358 stars 132 forks source link

NpgsqlConnectionStringBuilder is not giving updated builder instance for new connection string value. #343

Closed svetrivel closed 7 months ago

svetrivel commented 7 months ago

Issue : NpgsqlInstanceConnectionFactoryBase.SetupConnectionStringBuilder method always returns same builder instance even if we are passing different connection string values. This causes a DB connection error if db connection parameters changes while the application is running.

Proposed Solution : Instead of checking builder instance exists, system should check the given connection string is different from the previous value and gives updated builder instance.

Mentioned issue in Line 26 at https://github.com/hangfire-postgres/Hangfire.PostgreSql/blob/master/src/Hangfire.PostgreSql/Factories/NpgsqlInstanceConnectionFactoryBase.cs

I have raised PR for this issue https://github.com/hangfire-postgres/Hangfire.PostgreSql/pull/344

azygis commented 7 months ago

I'm curious, what is the actual use case? How and why does the connection string change at runtime, once the server is already up and running? How are you setting up the Hangfire server instance?

svetrivel commented 7 months ago

Hi @azygis, Thanks for the quick response!.

Use case : We rotate secrets(db password) in a periodic manner for security purposes without redeploying the hosted web api application. This is how the connection string changes at runtime even though the server is already up and running.

Hangfire server setup We have configured Hangfire server as background process in the web api application, hence we don't have a separate server and database for Hangfire.

azygis commented 7 months ago

Can you show the AddHangfire call that you use? Reason I'm asking is, I do not think your proposed PR solves your issue, as at least built in connection factories run the method in the constructor, meaning at that point they would be null either way. Aka the method runs once and never again.

Unless you have a custom-made IConnectionFactory implementation.

svetrivel commented 7 months ago

Yes @azygis , you are right. The proposed solution wouldn't help our case like you said. so I have modified the PR where PostgreSqlBootstrapperOptions will provide functionality to create new NpgsqlConnection based on the user given function to fetch latest connection string.

Thought this functionality would be useful for other users who face this problem, so that they don't need to implement a custom IConnectionFactory, instead they can use the proposed overloaded method UseNpgsqlConnection in PostgreSqlBootstrapperOptions by providing the function to fetch the updated connection string as below

services.AddHangfire((provider, config) =>
{
    config.UsePostgreSqlStorage(
        options =>
        {
            options.UseNpgsqlConnection(() => Configuration.GetConnectionString());
        }, new PostgreSqlStorageOptions { InvisibilityTimeout = TimeSpan.FromMinutes(90) });
});

Configuration.GetConnectionString() is a custom method which gives latest connection string.

Please let me know your thoughts on this.

Thank you.

azygis commented 7 months ago

Fine by me for now. Be aware that the method is called pretty frequently, so your mileage may vary performance-wise (although trivially).

azygis commented 7 months ago

Published, thank you for your contribution!