Proposal for supporting dynamic connection settings

[aivopaas]

Knex has automatic pool management and reconnection, but it doesn't provide an easy way to provide dynamic connection settings. That becomes an issue when the DB server IP changes (would have to restart service or at least handle it manually and create new Knex instance), or when we want the pool to connect to different physical DB instances.

My proposed solution is to add an optional configuration option named getConnection which accepts a function that returns either the connection settings object or a promise that resolves to the connection settings.

That function (if provided) is called every time when getting a new raw connection from the dialect/driver.

Sample usage:

const knex = require('knex')({
  client: 'mysql',
  connection: {
    database: 'db_name'
  },
  getConnection: async (settings) => {
    // Get instance of the DB asynchronously (from Consul for example)
    const {host, port, user, password} = await getDbInstance();
    // `settings` is the `connection` options property provided to Knex
    return { ...settings, host, port, user, password };
  }
});

That would probably also solve the problem described in #2732

[elhigu]

I don't think this is a good API to implemented this kind of feature. It is confusing to have 2 different places where configuration is fetched.

Better way would be to allow to pass function to connection attribute, which will return the connection attributes dynamically.

Other that that this needs to be well tested with integration tests to make sure that this works in the use case that this was meant to be used. I'm pretty sure that currently old database connections are destroyed / cleaned up properly when configuration is changed.

How this feature should exactly work should be defined in a much more clear manner before implementing it.

Currently I see this potentially causing lots of problems in future unless always when configuration is changed, stuff is released and recreated properly. Tests should make also sure that different event handlers are bound correctly to the newly created connections.

[aivopaas]

There are properties used from config.connection (database name for example) in some places other than where connection is created. That makes it impossible to replace connection with the function without causing braking changes and/or changing the options structure (splitting properties that is not needed for only creating connection out of that object).

The dynamic settings should not be able to change any other settings than what is used to determine and connect to the instance of the DB. Database name, collations and whatever other settings there might be must stay the same. I didn't find a good way to force it though, because it might be too dialect-dependant which properties can and which can not change.

[elhigu]

There are properties used from config.connection (database name for example) in some places other than where connection is created. That makes it impossible to replace connection with the function without causing braking changes and/or changing the options structure (splitting properties that is not needed for only creating connection out of that object).

I don't see why that would cause breaking changes. We would just need to write those parts of code a bit different way, where the values are used (checking if we are dealing with function or an object or string first).

The dynamic settings should not be able to change any other settings than what is used to determine and connect to the instance of the DB. Database name, collations and whatever other settings there might be must stay the same.

Why it shouldn't be able to change anything else? Sounds like really opinionated feature if it handles only some parameters that you need in your specific use-case.

[aivopaas]

I don't see why that would cause breaking changes. We would just need to write those parts of code a bit different way, where the values are used (checking if we are dealing with function or an object or string first).

Database field is used in compiler.columInfo() for example. If I'm not mistaken, using dynamic settings function there would cause async settings fetch for every query if not even multiple times per query. And if database name could be changed dynamically, that code would have to be completely changed to take into account the actual instance in the pool against which the query is executed. So it is not that simple to make everything dynamic without rethinking how query builder is working.

Why it shouldn't be able to change anything else? Sounds like really opinionated feature if it handles only some parameters that you need in your specific use-case.

It's not about my specific use case. I have to change everything except the database name and I can't think o f a good reason for even being able to change the database name. So for the majority it would be ok to always get the settings dynamically (object vs function) but that would indeed require some major refactoring of where database name is currently used.

[elhigu]

I don't see why that would cause breaking changes. We would just need to write those parts of code a bit different way, where the values are used (checking if we are dealing with function or an object or string first).

Database field is used in compiler.columInfo() for example. If I'm not mistaken, using dynamic settings function there would cause async settings fetch for every query if not even multiple times per query. And if database name could be changed dynamically, that code would have to be completely changed to take into account the actual instance in the pool against which the query is executed. So it is not that simple to make everything dynamic without rethinking how query builder is working.

Calling that dynamic getter multiple times is not a problem, one can do that in a efficient manner if one decides to use the feature of getting configuration in a dynamic way (usually just once when app is starting).

I still don't like the proposed API at all. Would be better to open feature request with more discussion why this is needed etc. I don't think that just changing password / host etc. is enough. If you like that you can easily create wrapper to get knex instance with correct parameters (even though in that case there would be multiple connection pools, which really doesn't matter if there are multiple database servers). It also ignores the fact that connection attribute might be just a string.

Why it shouldn't be able to change anything else? Sounds like really opinionated feature if it handles only some parameters that you need in your specific use-case.

It's not about my specific use case. I have to change everything except the database name and I can't think o f a good reason for even being able to change the database name.

That second sentence really sounds like very specific use. I'm 100% sure that other people will come up to think of many reasons why one would like to change also some other connection setting which are not included in this PR.

So for the majority it would be ok to always get the settings dynamically (object vs function) but that would indeed require some major refactoring of where database name is currently used.

I still don't think that just resolving connection object every time before using the database name is necessary a big refactoring (I cannot say if it really is without digging in to the code of those parts, if current compiler flow is completely synchronous, it might need some pre-compilation pass to be added, which resolves configuration just once before starting). After that its only up to the one who implements the dynamic getter to make sure that it works fast enough.

Anyways I still stand by my opinion that if this is going to be included to knex core it should allow dynamically to get the whole connection details object / string. Otherwise I would recommend to implement this kind of functionality outside of knex by re-creating knex instance with new settings.

For me it would be better to just have that dynamic setup to be resolved once when knex instance is created to help the #2732 issue to prevent knex from needing to watch those changes and to make sure that old pool is torn down and new created when setup is changed (and having to deal all errors that might happen during that). This thing might introduce some pretty nasty bugs if not done properly.

When I think about this more I'm starting to incline that maybe I wouldn't like this to be included to knex at all and leave this to be handled outside of knex. Maybe by writing a knex-async-setup package which would implement this externally (it would not be so convenient to use though, since invalidating old query builders when connection setup is changed doesn't sound so trivial).

[aivopaas]

I think you are a bit overthinking what I'm trying to solve here.

The problem with current Knex implementation is that for the connection pool it always uses the same backend. I just need a way to make sure every time a connection is added to the pool it is with fresh configuration (an instance that I know should accept the connection, not an old host that might already be teared down.

Having said that, I solved the problem by extending the mysql client with my own patched
acquireRawConnection which does exactly what I would have been able with that PR here, but with a lot less effort. SO I don't really see a point fighting the stone wall right now trying to explain the problem further if there's no strong interest in getting that fixed in Knex.

I understand the concern about maybe needing to replace the whole connection object and it being potentially a string, but I also don't have the time to dig too deep into it right now.

[elhigu]

I think you are a bit overthinking what I'm trying to solve here.

Not really. I just did not want that kind of really opiniated feature to knex just to solve that single case.

Having your own workaround is better solution this time. It will be also easier for you to modify to your future needs later on.