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

#2758: Implement fail-fast logic for dialect resolution #2776

Merged
merged 8 commits into from Aug 29, 2018

Conversation

Projects
None yet
4 participants
@kibertoad
Copy link
Collaborator

kibertoad commented Aug 20, 2018

refs #2758

@@ -1,73 +1,3 @@
import Raw from './raw';
import Client from './client';
const Knex = require('./knex');

This comment has been minimized.

@kibertoad

kibertoad Aug 20, 2018

Collaborator

Rationale behind this change: it it very counter-intuitive to write tests for a file named "index.js" which is in general supposed to be a catalogue and do not contain logic on its own.

import { SUPPORTED_CLIENTS, CLIENT_ALIASES } from './constants';

export default function Knex(config) {
// If config is string, try to parse it

This comment has been minimized.

@kibertoad

kibertoad Aug 20, 2018

Collaborator

added comment with explanation for each block, please verify the accuracy of them

export default function Knex(config) {
// If config is string, try to parse it
if (typeof config === 'string') {
const parsedConfig = Object.assign(parseConnection(config), arguments[2]);

This comment has been minimized.

@kibertoad

kibertoad Aug 20, 2018

Collaborator

_.assign replaced with Object.assign in this file since it is supported since Node 4.

src/knex.js Outdated
'Knex.VERSION is deprecated, you can get the module version' +
"by running require('knex/package').version"
);
return '0.12.6';

This comment has been minimized.

@ricardograca

ricardograca Aug 21, 2018

Collaborator

Looks like this return value hasn't been updated in some time 😆 Maybe it's time to remove this deprecation warning (and the associated code)? I know it's not related to the rest of the work done here, so feel free to ignore this comment.

This comment has been minimized.

@kibertoad

kibertoad Aug 21, 2018

Collaborator

So you suggest to remove entire

VERSION: {
    get() {
      console.warn(
        'Knex.VERSION is deprecated, you can get the module version' +
          "by running require('knex/package').version"
      );
      return '0.12.6';
}

block? Sure, I can do that.

This comment has been minimized.

@ricardograca

ricardograca Aug 21, 2018

Collaborator

Yes. The message has been there for almost 2 years now. I think that's enough time for a deprecation warning.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Aug 22, 2018

@ricardograca Did the change, anything else?..

@ricardograca

This comment has been minimized.

Copy link
Collaborator

ricardograca commented Aug 22, 2018

Cool! I'll take a look at the rest of changes later today.

@@ -38,7 +39,8 @@ function Client(config = {}) {
//Client is a required field, so throw error if it's not supplied.
//If 'this.dialect' is set, then this is a 'super()' call, in which case
//'client' does not have to be set as it's already assigned on the client prototype.
if (!this.config.client && !this.dialect) {
const dbClient = this.config.client || this.dialect;

This comment has been minimized.

@elhigu

elhigu Aug 22, 2018

Collaborator

Unrelated to this PR I would love to deprecate that dialect alias for client attribute.

This comment has been minimized.

@kibertoad

kibertoad Aug 22, 2018

Collaborator

Would you like me to log a deprecation warning when this.dialect is set?

This comment has been minimized.

@elhigu

elhigu Aug 22, 2018

Collaborator

That is a good idea, but we should discuss about it with other contributors too if they agree that alias is not wanted. My opinion is based on some previous problems with testing all these aliases and many times tests has been depending on .client and then they have failed when .dialect has been used by some other driver setup...

This comment has been minimized.

@elhigu

elhigu Aug 22, 2018

Collaborator

@tgriesser @wubzz maybe has an opinion

This comment has been minimized.

@wubzz

wubzz Aug 22, 2018

Collaborator

I think it's a good idea to remove "dialect" so long as the knex internals are also adjusted to reflect this. For example src/dialects -> src/clients.

This comment has been minimized.

@kibertoad

kibertoad Aug 22, 2018

Collaborator

Considering that there was no deprecation period with warnings prior to that, should we really just drop it immediately?

This comment has been minimized.

@wubzz

wubzz Aug 22, 2018

Collaborator

I meant in the long run, like when push comes to shove we shouldn't just remove the alias, but also adjust our own structure accordingly.

For now deprecation warning is best.

'mssql',
'mysql',
'mysql2',
'oracle',

This comment has been minimized.

@elhigu

elhigu Aug 22, 2018

Collaborator

I think that oracle is not actually supported by knex. It is just used by oracledb, which is supported.

This comment has been minimized.

@kibertoad

kibertoad Aug 22, 2018

Collaborator

Thanks, will remove

This comment has been minimized.

@elhigu

elhigu Aug 22, 2018

Collaborator

afaik. it is not tested either

'Knex.VERSION is deprecated, you can get the module version' +
"by running require('knex/package').version"
);
return '0.12.6';

This comment has been minimized.

@elhigu

elhigu Aug 22, 2018

Collaborator

Was this just dropped in this PR? It's hard to follow all the changes after renaming... I need to diff the files locally. If this was dropped, it must be notified in changelog as breaking change.

This comment has been minimized.

@kibertoad

kibertoad Aug 22, 2018

Collaborator

I've added comments in the PR for all the changes after renaming; this one was done after @ricardograca comment. If you are ok with this change, I'll add it to changelog.

This comment has been minimized.

@elhigu

elhigu Aug 22, 2018

Collaborator

Yep, I'm fine with it (even that the change should have been in a separate PR). I'll check out diff locally later on.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Aug 22, 2018

@elhigu Changes addressed.

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Aug 23, 2018

Looks like tests needs still some fixing so that they stop using .dialect too. Now at least custom logger tests are failing because of unexpected deprecation warnings.

@@ -4,6 +4,7 @@
### Breaking Changes:

- Use datetime2 for MSSQL datetime + timestamp types. This change is incompatible with MSSQL older than 2008 #2757
- Knex.VERSION() method was removed, run "require('knex/package').version" instead.

This comment has been minimized.

@elhigu

elhigu Aug 23, 2018

Collaborator

Please add also #2776 in the end of line so that this PR is referenced

kibertoad added some commits Aug 23, 2018

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Aug 23, 2018

@elhigu Fixed!

@kibertoad kibertoad closed this Aug 24, 2018

@kibertoad kibertoad reopened this Aug 24, 2018

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Aug 24, 2018

Aaaand it's green again.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Aug 28, 2018

@elhigu Is anything currently missing?

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Aug 29, 2018

Looks good to me, thanks!

@elhigu elhigu merged commit 89d2b3a into tgriesser:master Aug 29, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 85.598%
Details

veikovx added a commit to veikovx/knex that referenced this pull request Sep 17, 2018

tgriesser#2758: Implement fail-fast logic for dialect resolution (tgr…
…iesser#2776)

* Implement fail-fast logic for dialect resolution, clean-up code around.

* Remove method that was deprecated long time ago

* Address additional comments

* Try addressing comments

* Set client explicitly

* Fix compatibility with older Node versions

elhigu added a commit that referenced this pull request Sep 26, 2018

Kill queries after timeout for PostgreSQL (#2636)
* Kill queries after timeout for PostgreSQL

* Fix cancellation connection acquiring and test.

* Fix releasing connection in case query cancellation fails

* Add support for native enums on Postgres (#2632)

Reference https://www.postgresql.org/docs/current/static/sql-createtype.html

Closes #394

Signed-off-by: Will Soto <will.soto9@gmail.com>

* Removal of 'skim' (#2520)

* Allow overwriting log functions (#2625)

* Example build of custom log functions
* Handle logger object better for transactions
* Adjust test to ignore sqlite warning message

* Fixed onIn with empty values array (#2513)

* Drop support for strong-oracle (#2487)

* Remove babel-plugin-lodash (#2634)

While in theory, this may reduce the bundle size,
in practice it adds a ton of overhead during startup
due to the number of additional requires. Bundle
size also shouldn't matter for server side modules.

* Add json support to the query builder for mysql (#2635)

* Add json support to the query builder for mysql

refs #1036

Based on #1902

* Clarify supported version

* fix wrapIdentifier not being called in postgres alter column (#2612)

* fix wrapIdentifier not being called in postgres alter column

* add test for wrapIdentifier call in postgres alter column

* add comment regarding issue

* add issue & PR #'s in comment

* Remove readable-stream and safe-buffer (#2640)

* Add json support to the query builder for mysql

refs #1036

Based on #1902

* Clarify supported version

* Use native Buffer and Stream implementations

* fixes 2630 (#2642)

* Timeout errors shouldn't silently ignore the passed errors, but rather reject with original error. Fixes #2582 (#2626)

* chore: add Node.js 10 (#2594)

* chore: add Node.js 10

* chore: trigger new build

* chore: update lockfile

* chore: trigger new build

* fix: use npm i instead of npm ci

* Fix mssql driver crashing (#2637)

* Run SQL Server tests locally running SQL server in docker

* WIP mssql test stuff

* Patched MSSQL driver to not crash knex anymore

* Removed semicolon from rollback stmt for oracle (#2564)

* Remove WebSQL dialect (#2647)

* Add json support to the query builder for mysql

refs #1036

Based on #1902

* Clarify supported version

* Use native Buffer and Stream implementations

* Remove WebSQL dialect

* add homepage field to package.json (#2650)

* Make the stream catch errors in the query (#2638)

* Make the stream catch errors in the query

* Fix another case in which stream doesnt emits error

* Linter stuff

* Remove setTimeout in tests

* Make a test not to check the MySQL error code

* Fix stream error catching for MariaDB and PostgreSQL

* Fix stream error catching in Oracle

* Throw the error after emitting it to the stream

* Throw the error without instantiating a new Error

* Various fixes to mssql dialect (#2653)

* Fixed float type of mssql to be float

* Many tests where postgres test was not actually ran at all

* Migrations to be mssql compatible

Mssql driver doesn't handle if multiple queries are sent to same transaction concurrently.

* Prevented mssql failing when invalid schema builder was executed by accident

Instead of trying to generate sql from broken schema calls, just make exception to leak before query compiling is started

* Fixed mssql trx rollback to always throw an error

Also modified some connection test query to be mssql compatible

* Fixed various bugs from MSSQL driver to make tests run

* Fixed mssql unique index to be compatible with other dialect implementations

* Enable running mssql tests on CI

* Test for #2588

* Updated tests to not be dependend on tables left from previous test rans

* Trying to make mssql server work on travis

* Updated changelog and version

* Drop mariadb support  (#2681)

* Dropped support for mariasql

* ESLint

* Fixed docs to build again

* Fix knex.initialize() and adds test (#2477)

* Fix knex.initialize() and adds test

* Fix knex.initialize() and adds test

* Added test for reinitializing pool after destroy

* Fixed destroy / reinitialization test

* Fixed the fix

* Implement missing schema support for mssql dialect

* chore: Update dependencies. Remove estraverse (#2691)

* Update dependencies. Remove estraverse

* Fix compatibility with new Sinon

* Increase mssql timeout

* Normalized and validated driverNames of test db clients and fixed oracle test setup (#2692)

* Normalized and validated driverNames of test db clients and fixed oracle test setup

* Fixed failed queries from old query building tests which hadn't been ran in ages

* Allow selecting node version which is used to run oracledb docker tests

* Improved sql tester error messages

* Fixed rest of the oracledb tests

* Removed invalid flag from docker-compose

* Print mssql logs if initialization fails

* Fixed syntax error + final tests

* Added restart of failure for mssql DB initialization to try again if server was not ready

* Printout always mssql logs after container is started

* Fixed wait time printing after trying to connect

* Use npm run oracledb:test for testing oracle in travis

* Add Prettier (#2697)

* Add prettier
* Run files through prettier

* Istanbul -> NYC (#2700)

* istanbul -> nyc

* Update mocha

* Enforce code coverage (#2702)

* Enforce code coverage
* Update coverage numbers with current values

* version assignment on base class, copy on tx client, fix #2705

* Update changelog

* v0.15.1

* Added build step to test script. Fixes #2541 (#2712)

* Revert "Added build step to test script. Fixes #2541 (#2712)" (#2714)

This reverts commit 90ed8db.

* Allow oracle failures for now

* Fix issue with select(0) (#2711)

* Fix issue with select(0). Fixes #2658

* Refactor migrator (#2695)

* Refactor migrator

* Fix exports

* Fix ESLint

* Fix migrator

* Fix reference to config

* Split some more

* Fix table builder

* Fix argument order

* Merge branch 'master' of https://github.com/tgriesser/knex into feature/2690-support-multiple-migration-dirs

# Conflicts:
#	src/migrate/index.js
#	test/index.js
#	test/unit/migrate/migrator.js

* Fix #2715 (#2721)

* Fix #2715, explicitly set precision in datetime & timestamp
* Allow for precision in knex.fn.now, mysql time
* Add test for datetime with precision

* Bump changelog

* 0.15.2

* Fix issues with warnPromise when migration does not return a promise. Fixes #2725 (#2730)

* Add tests for multiple union arguments with callbacks and builders (#2749)

* Add tests for multiple union arguments

* Add some callback and raw tests to union unit tests

* Compile with before update so that bindings are put in correct order (#2733)

* Fix join using builder withSchema. (#2744)

* Use Datetime2 for MSSQL datetime + timestamp types (#2757)

* Use Datetime2 for MSSQL datetime + timestamp types

Datetime2 is now the recommended column type for new date work: https://docs.microsoft.com/en-us/sql/t-sql/data-types/datetime-transact-sql?view=sql-server-2017

* Add tests for MSSQL datetime2 changes

* General/document breaking change (#2774)

* Add changelog entry for a breaking change

* Improve entry

* Allow timestamp with timezone on mssql databases (#2724)

* Allow timestamp with timezone on mssql databases

* Change timestamp parameter to use object instead of boolean

* Update dependencies (#2772)

* Feature/2690: Multiple migration directories (#2735)

* Implement support for multiple migration directories

* Add tests for new functionality

* Fix tape tests

* Pass migration directory upwards

* Fix multiple directory support

* Fix bugs

* Rollback correctly

* Fix remaining tests

* Address comments

* #2758: Implement fail-fast logic for dialect resolution (#2776)

* Implement fail-fast logic for dialect resolution, clean-up code around.

* Remove method that was deprecated long time ago

* Address additional comments

* Try addressing comments

* Set client explicitly

* Fix compatibility with older Node versions

* Use columnize instead of wrap in using(). (#2713)

* Use columnize instead of wrap in using().

This is an attempt to fix #2136. Also added an integration test, couldn't find any existing.

* Exclude MSSQL from test as it does not support .using

* Change test to not use subquery

* Change test

* Introduced abstraction for getting migrations (#2775)

* Introduced abstraction for getting migrations

This would allow a webpack migration source which is compatible with bundling.

* Fixed migration validation with custom migration source

* Fixed issues after rebasing on muti directory PR

* Renamed parameter and fixed error message

* Addressed some PR comments

* Finished comment

* Moved filename extension filtering into fs-migrator

* Added test showing in memory custom migration source

* Cleaned up how to get config

* Fixed failing test

* Hopefully fix tests

* Fix Node.js 10 support in tests

* Test for correctly releasing cancel query connection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment