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

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

Merged
merged 8 commits into from Aug 29, 2018
Merged

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

merged 8 commits into from Aug 29, 2018

Conversation

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Aug 20, 2018

refs #2758

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

Copy link
Collaborator Author

@kibertoad kibertoad Aug 20, 2018

Choose a reason for hiding this comment

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

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
Copy link
Collaborator Author

@kibertoad kibertoad Aug 20, 2018

Choose a reason for hiding this comment

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

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]);
Copy link
Collaborator Author

@kibertoad kibertoad Aug 20, 2018

Choose a reason for hiding this comment

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

_.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';
Copy link
Member

@ricardograca ricardograca Aug 21, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@kibertoad kibertoad Aug 21, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ricardograca ricardograca Aug 21, 2018

Choose a reason for hiding this comment

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

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

@kibertoad
Copy link
Collaborator Author

@kibertoad kibertoad commented Aug 22, 2018

@ricardograca Did the change, anything else?..

@ricardograca
Copy link
Member

@ricardograca ricardograca commented Aug 22, 2018

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

src/client.js Outdated
@@ -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;
Copy link
Member

@elhigu elhigu Aug 22, 2018

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@kibertoad kibertoad Aug 22, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

@elhigu elhigu Aug 22, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

@elhigu elhigu Aug 22, 2018

Choose a reason for hiding this comment

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

@tgriesser @wubzz maybe has an opinion

Copy link
Member

@wubzz wubzz Aug 22, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@kibertoad kibertoad Aug 22, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

@wubzz wubzz Aug 22, 2018

Choose a reason for hiding this comment

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

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.

src/constants.js Outdated
'mssql',
'mysql',
'mysql2',
'oracle',
Copy link
Member

@elhigu elhigu Aug 22, 2018

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@kibertoad kibertoad Aug 22, 2018

Choose a reason for hiding this comment

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

Thanks, will remove

Copy link
Member

@elhigu elhigu Aug 22, 2018

Choose a reason for hiding this comment

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

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';
Copy link
Member

@elhigu elhigu Aug 22, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@kibertoad kibertoad Aug 22, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

@elhigu elhigu Aug 22, 2018

Choose a reason for hiding this comment

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

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
Copy link
Collaborator Author

@kibertoad kibertoad commented Aug 22, 2018

@elhigu Changes addressed.

@elhigu
Copy link
Member

@elhigu 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.

CHANGELOG.md Outdated
@@ -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.
Copy link
Member

@elhigu elhigu Aug 23, 2018

Choose a reason for hiding this comment

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

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

@kibertoad
Copy link
Collaborator Author

@kibertoad kibertoad commented Aug 23, 2018

@elhigu Fixed!

@kibertoad kibertoad closed this Aug 24, 2018
@kibertoad kibertoad reopened this Aug 24, 2018
@kibertoad
Copy link
Collaborator Author

@kibertoad kibertoad commented Aug 24, 2018

Aaaand it's green again.

@kibertoad
Copy link
Collaborator Author

@kibertoad kibertoad commented Aug 28, 2018

@elhigu Is anything currently missing?

@elhigu
Copy link
Member

@elhigu elhigu commented Aug 29, 2018

Looks good to me, thanks!

@elhigu elhigu merged commit 89d2b3a into knex:master Aug 29, 2018
2 checks passed
veikovx added a commit to veikovx/knex that referenced this issue Sep 17, 2018
* 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 pushed a commit that referenced this issue Sep 26, 2018
* 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants