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.
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
Connection config provider #3403
Changes from all commits
88f69a5
ccbde75
d2cbf1a
a8aa0af
79a2660
7d1b8f5
a2427b4
d219159
400b829
cc87967
4050772
ddae371
37c805c
1ecd885
a4df60d
1c6eff4
5484292
2b6460d
42d7d41
7059d5f
2837c15
6d0da47
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
const { isFunction, cloneDeep } = require('lodash'); | ||
|
||
class ConnectionConfigProvider { | ||
constructor(client) { | ||
this.client = client; | ||
this.resolvedConnectionSettings = null; | ||
} | ||
|
||
async resolveConnectionConfig() { | ||
if (this.resolvedConnectionSettings) return this.resolvedConnectionSettings; | ||
if (isFunction(this.client.config.connection)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I mean is this:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this line in
the lines after check for the type of the connection. if it is a function resolve it, other wise, deepclone it. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
// 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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -217,8 +217,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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in
will return the processed object direclty without processing anything anymore. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will it?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any recommendation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I've said, you can call |
||
.connectionConfigProvider() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When a new I tried it on my machine and the connection is resolved only one time and for the 2nd time it loads the config from
https://github.com/knex/knex/pull/3403/files#diff-cbc1ae6887774cf34c8d4bc73dece12fR10
can you give some examples how I can solve this? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I'll take a swing at the branch sometime this week to see how to improve the situation. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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();
}
}
What do you think? |
||
.resolveConnectionConfig(); | ||
settings.pool = this.mssqlPoolSettings; | ||
|
||
const connection = new this.driver.ConnectionPool(settings); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,29 +77,33 @@ 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do not use async inside new promise callback There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
// eslint-disable-next-line require-atomic-updates | ||
client.driver.fetchAsString = client.fetchAsString; | ||
|
||
client.driver.getConnection(oracleDbConfig, function(err, connection) { | ||
|
There was a problem hiding this comment.
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.