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.

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 configuration provider - without changes to drivers #3497

Merged
merged 14 commits into from Oct 27, 2019

Conversation

@oranoran
Copy link
Contributor

oranoran commented Oct 25, 2019

This is a simplified PR to implement the same functionality as suggested in #3403, but with the following differences:

  1. All changes were made in client.js only, so they apply to all built-in and external drivers without any changes required in them, improving extensibility and maintainability.
  2. Provided connection configurations can include an expired() function, allowing configurations to expire after a while.

The expiration feature is useful, for example, for expiring Amazon RDS IAM access tokens after their lifespan of 15 minute is over, like so:

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

I wrote the tests relying on sqlite3, hope this makes sense.
Please let me know if this direction seems right, then I'll add a documentation PR.

lib/client.js Outdated Show resolved Hide resolved
lib/client.js Outdated Show resolved Hide resolved
lib/client.js Outdated Show resolved Hide resolved
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 25, 2019

TypeScript definitions probably need to be updated.

@oranoran

This comment has been minimized.

Copy link
Contributor Author

oranoran commented Oct 25, 2019

TypeScript definitions probably need to be updated.

I checked this, looks like the whole "connection" object is "any". Not changing anything else in the API, so didn't find anything that needs to be changed.

Oran Epelbaum added 4 commits Oct 25, 2019
…"undefined" so "null" will also be supported.
…ll DBs + removing expiryChecker from config being passed to DB drivers
lib/client.js Outdated Show resolved Hide resolved
Oran Epelbaum added 3 commits Oct 26, 2019
…oving it (and also initializing it to null if needed) - in order to maintain a stable set of properties in the client.
…fferent for different databases, and the tests seem to be just as valid wihtout them.
Oran Epelbaum
@oranoran

This comment has been minimized.

Copy link
Contributor Author

oranoran commented Oct 27, 2019

@kibertoad Ok so the only open issue AFAIK is that I get a test failing on Oracle when Travis runs automatically. I can't reproduce this because when I try to run Oracle tests locally according to the instructions, I get an error (the npm install oracledb below runs as part of the tests, not through anything I'm doing directly, and fails):

 > DB='oracledb' mocha  --grep "Connection configuration" --exit -t 10000 test/index.js
sqlite does not support inserting default values. Set the `useNullAsDefault` flag to hide this warning. (see docs http://knexjs.org/#Builder-insert).
sqlite does not support inserting default values. Set the `useNullAsDefault` flag to hide this warning. (see docs http://knexjs.org/#Builder-insert).
sqlite does not support inserting default values. Set the `useNullAsDefault` flag to hide this warning. (see docs http://knexjs.org/#Builder-insert).
Knex: run
$ npm install oracledb --save
Cannot find module 'oracledb'
Error: Cannot find module 'oracledb'
    at Function.Module._resolveFilename (module.js:548:15)
    at Function.Module._load (module.js:475:25)
    at Module.require (module.js:597:17)
    at require (internal/module.js:11:18)
    at Client_Oracledb._driver (/Users/oran/proj/knex/lib/dialects/oracledb/index.js:30:20)```
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 27, 2019

It is a flaky test, let me rerun it.

@oranoran

This comment has been minimized.

Copy link
Contributor Author

oranoran commented Oct 27, 2019

TypeScript definitions probably need to be updated.

I checked this, looks like the whole "connection" object is "any". Not changing anything else in the API, so didn't find anything that needs to be changed.

Actually, looking again at index.d.ts, I see there are ConnectionConfig interfaces defined for the various databases. Should I add an expiryChecker to each one?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 27, 2019

Would be helpful, yes.

…nction in connection objects.
@oranoran

This comment has been minimized.

Copy link
Contributor Author

oranoran commented Oct 27, 2019

The test is still failing:

  1) Integration Tests
       oracle | oracledb
         knex.migrate
           knex.migrate.up
             should only run the next migration that has not yet run if other migrations have already run:
      AssertionError: expected [ Array(1) ] to have a length of 2 but got 1

I can't reproduce it since I'm not able to run Oracle tests, any suggestions?

P.S the TS definitions I added aren't complete, I need to add a modified connection definition too. Will add later today.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 27, 2019

@oranoran It did pass after re-run, so as expected, flaky test, feel free to ignore.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 27, 2019

@oranoran Could you also create a documentation PR?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 27, 2019

@oranoran There is a conflict with types in master now.

Oran Epelbaum added 2 commits Oct 27, 2019
…nn-config-provider
…by provider. This is currently not supported since parsing a config string is done in knex.js while the connection config provider is implemented in client.js
oranoran pushed a commit to oranoran/documentation that referenced this pull request Oct 27, 2019
Take 2 - without the generated files
kibertoad added a commit to knex/documentation that referenced this pull request Oct 27, 2019
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 27, 2019

Thanks a lot for all the effort put into this!

@kibertoad kibertoad merged commit f4b6848 into knex:master Oct 27, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@oranoran

This comment has been minimized.

Copy link
Contributor Author

oranoran commented Oct 27, 2019

Thanks a lot for all the effort put into this!

Thanks for maintaining this awesome lib!

@oranoran oranoran deleted the oranoran:generic-conn-config-provider branch Oct 28, 2019
@Ali-Dalal

This comment has been minimized.

Copy link

Ali-Dalal commented Oct 29, 2019

@oranoran Thanks for finalizing this 👍

@kibertoad when can you release it the changes?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 29, 2019

@Ali-Dalal Released in 0.20.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.