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

Connection config provider #3403

Closed
wants to merge 22 commits into from
Closed

Connection config provider #3403

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
88f69a5
#3340 Connection settings can be an async function only for PG client
Ali-Dalal Jul 20, 2019
ccbde75
connection config provider
Ali-Dalal Aug 19, 2019
d2cbf1a
change default value of resolvedConnectionSettings to null in connect…
Ali-Dalal Aug 19, 2019
a8aa0af
Merge branch 'master' into connection-config-provider
Ali-Dalal Aug 19, 2019
79a2660
Rename connection.js to connectionProvider.js
Ali-Dalal Aug 26, 2019
7d1b8f5
fix an issue in ConnectionConfigProvider
Ali-Dalal Aug 26, 2019
a2427b4
remove console.log
Ali-Dalal Aug 26, 2019
d219159
enable configProvider for sqllite
Ali-Dalal Aug 26, 2019
400b829
enable config provider for mssql
Ali-Dalal Aug 26, 2019
cc87967
enable config provider for mysql
Ali-Dalal Aug 26, 2019
4050772
enable config provider for oracle
Ali-Dalal Aug 26, 2019
ddae371
enable config provider for oracledb
Ali-Dalal Aug 26, 2019
37c805c
Merge branch 'master' into connection-config-provider
Ali-Dalal Aug 26, 2019
1ecd885
remove comments from client.js
Ali-Dalal Aug 26, 2019
a4df60d
remove comments from mssql/index.js
Ali-Dalal Aug 26, 2019
1c6eff4
Merge branch 'master' into connection-config-provider
Ali-Dalal Oct 15, 2019
5484292
add await in oracledb dialects test
Ali-Dalal Oct 15, 2019
2b6460d
remove unused variables (clonedeeb) from client.js
Ali-Dalal Oct 15, 2019
42d7d41
fix eslint issues
Ali-Dalal Oct 15, 2019
7059d5f
add tests
Ali-Dalal Oct 16, 2019
2837c15
update postgres unit test
Ali-Dalal Oct 16, 2019
6d0da47
update postgres unit test
Ali-Dalal Oct 16, 2019
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
6 changes: 5 additions & 1 deletion lib/client.js
View file
Open in desktop
Expand Up @@ -15,6 +15,7 @@ const TableBuilder = require('./schema/tablebuilder');
const TableCompiler = require('./schema/tablecompiler');
const ColumnBuilder = require('./schema/columnbuilder');
const ColumnCompiler = require('./schema/columncompiler');
const ConnectionConfigProvider = require('./connectionProvider');

const { Pool, TimeoutError } = require('tarn');
const inherits = require('inherits');
Expand Down Expand Up @@ -54,7 +55,6 @@ function Client(config = {}) {
this.version = config.version;
}

this.connectionSettings = cloneDeep(config.connection || {});
if (this.driverName && config.connection) {
this.initializeDriver();
if (!config.pool || (config.pool && config.pool.max !== 0)) {
Expand Down Expand Up @@ -122,6 +122,10 @@ Object.assign(Client.prototype, {
return new Ref(this, ...arguments);
},

connectionConfigProvider() {
return new ConnectionConfigProvider(this);
},

_formatQuery(sql, bindings, timeZone) {
bindings = bindings == null ? [] : [].concat(bindings);
let index = 0;
Expand Down
25 changes: 25 additions & 0 deletions lib/connectionProvider.js
View file
Open in desktop
@@ -0,0 +1,25 @@
const { isFunction, cloneDeep } = require('lodash');

class ConnectionConfigProvider {
constructor(client) {
this.client = client;
this.resolvedConnectionSettings = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not resolve settings at this step already? At the very least we can definitely do that if it's a static config.

}

async resolveConnectionConfig() {
if (this.resolvedConnectionSettings) return this.resolvedConnectionSettings;
if (isFunction(this.client.config.connection)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can do the check once in constructor and set some flag after that.

Copy link
Author

Choose a reason for hiding this comment

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

Can you give more details about this, please?

if we need to manage this in ConnectionConfigProvider constructor, the how-to manipulate the promise as the type of the function is async?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I mean is this:

  1. In constructor check if it's a function. If it is - set this.isConfigSourceFunction = true and do nothing else. If it is not - set this.isConfigSourceFunction = false and this.resolvedConnectionSettings = cloneDeep <...>

  2. in resolveConnectionConfig check if this.resolvedConnectionSettings is set, if it is, return, if it is not and isConfigSourceFunction, resolve from function, if neither, probably throw an error.

Copy link
Author

Choose a reason for hiding this comment

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

I think this line in ConnectionConfigProvider class does the same, right?

  async resolveConnectionConfig() {
    if (this.resolvedConnectionSettings) return this.resolvedConnectionSettings;

the lines after check for the type of the connection. if it is a function resolve it, other wise, deepclone it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Partially. What I'm saying is that everything that can be moved from resolveConnectionConfig to constructor should ideally be moved, resolveConnectionConfig should be as simple as possible.
Current implementation is different from what I've described.

// if the config is a function, execute the function and resolve the connection settings
this.resolvedConnectionSettings = await this.client.config.connection();
this.client.connectionSettings = this.resolvedConnectionSettings;
} else {
this.client.connectionSettings = cloneDeep(
this.client.config.connection || {}
);
this.resolvedConnectionSettings = this.client.connectionSettings;
}
return this.resolvedConnectionSettings;
}
}

module.exports = ConnectionConfigProvider;
7 changes: 5 additions & 2 deletions lib/dialects/mssql/index.js
View file
Open in desktop
Expand Up @@ -213,8 +213,11 @@ Object.assign(Client_MSSQL.prototype, {
// Get a raw connection, called by the `pool` whenever a new
// connection needs to be added to the pool.
acquireRawConnection() {
return new Bluebird((resolver, rejecter) => {
const settings = Object.assign({}, this.connectionSettings);
const client = this;
return new Bluebird(async (resolver, rejecter) => {
const settings = await client
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why we might want to set connectionConfigProvider each time for a new connection anew and not just set it once on a client when it is created?

Copy link
Author

Choose a reason for hiding this comment

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

in ConnectionConfigProvider class I processed config.connection resolve the connection settings for only one time. So calling

const settings = await client
        .connectionConfigProvider()
        .resolveConnectionConfig();

will return the processed object direclty without processing anything anymore.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will it?

  connectionConfigProvider() {
    return new ConnectionConfigProvider(this);
  },

Copy link
Author

Choose a reason for hiding this comment

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

any recommendation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

As I've said, you can call connectionConfigProvider once after client is created and configuration is known. And probably it should be renamed to initConnectionConfigProvider to emphasise it mutates the state.

.connectionConfigProvider()
Copy link
Collaborator

@kibertoad kibertoad Oct 16, 2019 •
edited

Choose a reason for hiding this comment

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

So in this line we are still always initializing a new ConnectionConfigProvider, meaning we will resolve credentials again on each connection.
Can we adjust connectionConfigProvider() method to lazy-instantiate and not reset each time?

Copy link
Author

Choose a reason for hiding this comment

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

When a new knex instance defined, a new client is attached and a new ConnectionConfoProvider as well and once we call resolve connection, for the first time it will execute the function if the connection is a function then it will mutate the state of the client and set the connection to the resolved one.

I tried it on my machine and the connection is resolved only one time and for the 2nd time it loads the config from

async resolveConnectionConfig() {
    if (this.resolvedConnectionSettings) return this.resolvedConnectionSettings;

https://github.com/knex/knex/pull/3403/files#diff-cbc1ae6887774cf34c8d4bc73dece12fR10

Can we adjust connectionConfigProvider() method to lazy-instantiate and not reset each time?

can you give some examples how I can solve this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you are seeing connection being resolved only once because it is being reused from the connection pool and not recreated on every request - for new connections it is likely that same acquireRawConnection will be called again and you will have to resolve credentials again.

I'll take a swing at the branch sometime this week to see how to improve the situation.

Copy link
Member

Choose a reason for hiding this comment

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

Easy way to check with multiple connections is to create transactions and never commit / rollback them. In that way the old connections cannot be reused from pool.

Copy link
Contributor

@oranoran oranoran Oct 24, 2019 •
edited

Choose a reason for hiding this comment

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

Guys, sorry for coming in late in the process, but I noticed this PR when I was considering opening a similar issue, with the use case in mind being support of Amazon RDS IAM authentication.

For this use case though, the resolved configuration can't be cached indefinitely, because the password needs to be changed every 15 minutes.

How about adding an optional field to the returned configuration object, with a sync function determining whether or not the cached config can be reused?

So the connection config provider code can look something like this:

const { token, tokenExpiration } = await someCallToGetTheToken();
return {
    host : 'your_host',
    user : 'your_database_user',
    password : token,
    database : 'myapp_test',
    expired = () => {
        return tokenExpiration <= Date.now();
    }
}

expired is a synchronous function, returning a boolean determining whether the configuration has expired or or not. If it hasn't expired, it can be reused. If not specified, the previously-resolved configuration is always reused.

What do you think?

.resolveConnectionConfig();
settings.pool = this.mssqlPoolSettings;

const connection = new this.driver.ConnectionPool(settings);
Expand Down
8 changes: 6 additions & 2 deletions lib/dialects/mysql/index.js
View file
Open in desktop
Expand Up @@ -60,8 +60,12 @@ Object.assign(Client_MySQL.prototype, {
// Get a raw connection, called by the `pool` whenever a new
// connection needs to be added to the pool.
acquireRawConnection() {
return new Bluebird((resolver, rejecter) => {
const connection = this.driver.createConnection(this.connectionSettings);
const client = this;
return new Bluebird(async (resolver, rejecter) => {
const connectionSettings = await client
.connectionConfigProvider()
.resolveConnectionConfig();
const connection = this.driver.createConnection(connectionSettings);
connection.on('error', (err) => {
connection.__knex__disposed = err;
});
Expand Down
14 changes: 8 additions & 6 deletions lib/dialects/oracle/index.js
View file
Open in desktop
Expand Up @@ -79,14 +79,16 @@ Object.assign(Client_Oracle.prototype, {
// Get a raw connection, called by the `pool` whenever a new
// connection needs to be added to the pool.
acquireRawConnection() {
return new Bluebird((resolver, rejecter) => {
this.driver.connect(this.connectionSettings, (err, connection) => {
const client = this;
return new Bluebird(async (resolver, rejecter) => {
const connectionSettings = await client
.connectionConfigProvider()
.resolveConnectionConfig();
this.driver.connect(connectionSettings, (err, connection) => {
if (err) return rejecter(err);
Bluebird.promisifyAll(connection);
if (this.connectionSettings.prefetchRowCount) {
connection.setPrefetchRowCount(
this.connectionSettings.prefetchRowCount
);
if (connectionSettings.prefetchRowCount) {
connection.setPrefetchRowCount(connectionSettings.prefetchRowCount);
}
resolver(connection);
});
Expand Down
25 changes: 14 additions & 11 deletions lib/dialects/oracledb/index.js
View file
Open in desktop
Expand Up @@ -76,27 +76,30 @@ Client_Oracledb.prototype.prepBindings = function(bindings) {
// connection needs to be added to the pool.
Client_Oracledb.prototype.acquireRawConnection = function() {
const client = this;
const asyncConnection = new Bluebird(function(resolver, rejecter) {
const asyncConnection = new Bluebird(async function(resolver, rejecter) {
// If external authentication dont have to worry about username/password and
// if not need to set the username and password
const oracleDbConfig = client.connectionSettings.externalAuth
? { externalAuth: client.connectionSettings.externalAuth }
const connectionSettings = await client
Copy link
Collaborator

Choose a reason for hiding this comment

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

do not use async inside new promise callback
it will not catch errors, so you get unhandledRejection

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Ali-Dalal This needs to be addressed

.connectionConfigProvider()
.resolveConnectionConfig();
const oracleDbConfig = connectionSettings.externalAuth
? { externalAuth: connectionSettings.externalAuth }
: {
user: client.connectionSettings.user,
password: client.connectionSettings.password,
user: connectionSettings.user,
password: connectionSettings.password,
};

// In the case of external authentication connection string will be given
oracleDbConfig.connectString =
client.connectionSettings.connectString ||
client.connectionSettings.host + '/' + client.connectionSettings.database;
connectionSettings.connectString ||
connectionSettings.host + '/' + connectionSettings.database;

if (client.connectionSettings.prefetchRowCount) {
oracleDbConfig.prefetchRows = client.connectionSettings.prefetchRowCount;
if (connectionSettings.prefetchRowCount) {
oracleDbConfig.prefetchRows = connectionSettings.prefetchRowCount;
}

if (!_.isUndefined(client.connectionSettings.stmtCacheSize)) {
oracleDbConfig.stmtCacheSize = client.connectionSettings.stmtCacheSize;
if (!_.isUndefined(connectionSettings.stmtCacheSize)) {
oracleDbConfig.stmtCacheSize = connectionSettings.stmtCacheSize;
}

client.driver.fetchAsString = client.fetchAsString;
Expand Down
8 changes: 6 additions & 2 deletions lib/dialects/postgres/index.js
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@ function Client_PG(config) {
this.searchPath = config.searchPath;
}
}

inherits(Client_PG, Client);

Object.assign(Client_PG.prototype, {
Expand Down Expand Up @@ -105,8 +106,11 @@ Object.assign(Client_PG.prototype, {
// connection needs to be added to the pool.
acquireRawConnection() {
const client = this;
return new Bluebird(function(resolver, rejecter) {
const connection = new client.driver.Client(client.connectionSettings);
return new Bluebird(async function(resolver, rejecter) {
const connectionSettings = await client
.connectionConfigProvider()
.resolveConnectionConfig();
const connection = new client.driver.Client(connectionSettings);
connection.connect(function(err, connection) {
if (err) {
return rejecter(err);
Expand Down
8 changes: 6 additions & 2 deletions lib/dialects/sqlite3/index.js
View file
Open in desktop
Expand Up @@ -62,9 +62,13 @@ Object.assign(Client_SQLite3.prototype, {

// Get a raw connection from the database, returning a promise with the connection object.
acquireRawConnection() {
return new Bluebird((resolve, reject) => {
const client = this;
return new Bluebird(async (resolve, reject) => {
const connectionSettings = await client
.connectionConfigProvider()
.resolveConnectionConfig();
const db = new this.driver.Database(
this.connectionSettings.filename,
connectionSettings.filename,
(err) => {
if (err) {
return reject(err);
Expand Down