#3340 Connection settings can be an async function only for PG client

[Ali-Dalal]

No description provided.

[kibertoad]

Does it make sense to resolve settings on every acquisition? Why not store them after lazy loading originally?

[Ali-Dalal]

@kibertoad pls correct me if I am wrong. To my understanding, Node runtime will execute this function only one time and store the result in memory.
Is this what you asking for?

let tmpSettings;
if (typeof client.connectionSettings === 'function')
  if(! tmpSettings)
        tmpSettings = await client.connectionSettings();
  client.connectionSettings = tmpSettings;

[elhigu]

@Ali-Dalal node runtime will not automatically store anything to memory. Each call to client.connectionSettings() creates new promise that gets executed.

[Ali-Dalal]

@elhigu What about the above snippet?

[elhigu]

maybe connection settings should not be set before this point... is there any reason why they would need to be cloned already in client.js?

[Ali-Dalal]

I tried some examples and if you can have a look at the following output

The code:

//dialects/pg/index.js
  acquireRawConnection() {
    const client = this;
    return new Bluebird(async function(resolver, rejecter) {
      console.log("Executing function");
      if (typeof client.connectionSettings === 'function') {
        client.connectionSettings = await client.connectionSettings();
      }
//... rest of the code

//knexfile.js
//...settings
    production: {
        client: 'pg',
        connection: async function () {
            console.log("executing function in connection");
            if (!dbConfig) {
                let secret = await getSecretFromSecretManager(`KEY_NAME`);
                let connection = JSON.parse(secret.SecretString);
                dbConfig = {
                    host: connection.host,
                    user: connection.username,
                    password: connection.password,
                    database: connection.db_name
                };
            }
            return dbConfig;
        },
//...rest of settings

log:
for the first time:

Executing function
executing function in connection
getting secret from secret manager
  knex:client acquired connection from pool: __knexUid1 +0ms
  knex:query select **** undefined +0ms
  knex:bindings [ 'name', 1 ] undefined +0ms
  knex:client releasing connection to pool: __knexUid1 +403ms

for the 2nd time:

Executing function
  knex:client acquired connection from pool: __knexUid2 +2m
  knex:query select ***undefined +2m
  knex:bindings [ 'name', 1 ] undefined +2m
  knex:client releasing connection to pool: __knexUid2 +408ms

From the output above, the production connection config function is executed only 1 time and this mean fetching the db config from secret manager or any other provider will be called only 1 time.

To answer your question,

maybe connection settings should not be set before this point... is there any reason why they would need to be cloned already in client.js?

the reason is if we switch cleint.js to export an async function then every module that needs to use client.js has to require it like this.

require('client').then(function(){
//...rest of the code
})

This is not freidnly code :(

can you please give a resolution on the output above and tell me your thoughts?

I tried the code of this pull request on staging environment and it works like a charm.

I've seen around 6 people asking for this feature.

If you are ok wtih it then I can write integration test and setup the rest of dialects

Thanks.

[Ali-Dalal]

@elhigu What about the above snippet?

That doesn't work either, because you are creating new tmsSettings on every call to aquireRawConnection.

tmpSettings should be defined outside the scope

[elhigu]

maybe connection settings should not be set before this point... is there any reason why they would need to be cloned already in client.js?

I meant with that to use directly config.connection from acquireRawConnection, but I suppose it is not exposed or something. This was just something that really looks horrible:

client.connectionSettings = await client.connectionSettings();

So to prevent that + moving resolving real configuration to base implementation should be done by adding new async function to base client, which resolves configuration and which can be shared between every dialect.

Actually I don't thing there should be anything dialect specific in this feature (except for calling some base implementation function to get the config).

[Ali-Dalal]

@elhigu Thanks for the information.

So to prevent that + moving resolving real configuration to base implementation should be done by adding new async function to base client, which resolves configuration and which can be shared between every dialect.

Can you guide me more?

//client.js
function Client(config = {}) {
  this.config = config;
// How to resolve config.connectionSettings if it is async function?
  this.logger = new Logger(config);
//...rest of client code
module.exports = Client;

for example, if the client is a sync function and we need to convert it to an async function then everytime we need to use the client, we have to require in the following style

require('client').then(()=>{})

[elhigu]

By adding a new async method to the Client class. The same way that other methods of that class are implemented. https://github.com/tgriesser/knex/pull/3364/files#diff-50cfa59973c04321b5da0c6da0fdf4feL73

[kibertoad]

Needs integration test

[elhigu]

This relates to issue #3340

[kibertoad]

@Ali-Dalal Any updates?

[Ali-Dalal]

@kibertoad I haven't gotten the chance to modify the PR yet. for now, I am using Knex from my account's repo which I completely don't agree with.

I will try to modify the PR tomorrow and ask for the feed back again

[Ali-Dalal]

closing this PR as it is a duplication of #3403
#3403