Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions lib/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,12 @@ function Client(config = {}) {
if (config.version) {
this.version = config.version;
}

this.connectionSettings = cloneDeep(config.connection || {});
// if the type of connection settings is a function, pass it to the next layer
if (typeof config.connection === 'function') {
this.connectionSettings = config.connection;
} else {
this.connectionSettings = cloneDeep(config.connection || {});
}
if (this.driverName && config.connection) {
this.initializeDriver();
if (!config.pool || (config.pool && config.pool.max !== 0)) {
Expand Down
4 changes: 3 additions & 1 deletion lib/dialects/postgres/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,9 @@ Object.assign(Client_PG.prototype, {
// connection needs to be added to the pool.
acquireRawConnection() {
const client = this;
return new Bluebird(function(resolver, rejecter) {
return new Bluebird(async function(resolver, rejecter) {
if (typeof client.connectionSettings === 'function')
client.connectionSettings = await client.connectionSettings();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elhigu What about the above snippet?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@Ali-Dalal Ali-Dalal Jul 26, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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(()=>{})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const connection = new client.driver.Client(client.connectionSettings);
connection.connect(function(err, connection) {
if (err) {
Expand Down