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

Bugfixes on batchInsert and transactions for mysql/maria #1992

Merged
merged 1 commit into from Mar 26, 2017

Conversation

@wubzz
Copy link
Member

@wubzz wubzz commented Mar 25, 2017

I really don't like the code duplication between the base Transaction.query implementation and the ones for mysql, mysql2, and maria. For the sake of "keeping dialects separate", I left it as is. Perhaps a better idea just to listen on the query-error event and give the helpers.warn a call there instead?

@wubzz wubzz requested a review from elhigu Mar 25, 2017
@wubzz wubzz force-pushed the wubzz:bugfixes branch 7 times, most recently from 411cc2e to ef3ec87 Mar 25, 2017
@wubzz wubzz force-pushed the wubzz:bugfixes branch from ef3ec87 to 9dfcfb6 Mar 25, 2017
@elhigu
elhigu approved these changes Mar 26, 2017
Copy link
Member

@elhigu elhigu left a comment

Looks good


if(autoTransaction) {
return tr.commit()
//TODO: -- Oracle tr.commit() does not return a 'thenable' !? Ugly hack for now.
return (tr.commit(result) || Promise.resolve())

This comment has been minimized.

@elhigu

elhigu Mar 26, 2017
Member

Would it be difficult to fix oracle to return promise/thenable?

This comment has been minimized.

@wubzz

wubzz Mar 26, 2017
Author Member

Not too familiar with Oracles code base so not sure. The current state came as a shock to me. If commit does not return a promise, how can a standard transaction work at all for oracle?

knex.transaction((tr) => tr.raw('select version'))
.then(() => {...})
.catch((error) => {})

Really strange and confusing. Currently have no environment for testing oracle.

This comment has been minimized.

@elhigu

elhigu Mar 26, 2017
Member

I'm planning to setup docker container with postgres, mysql, oracle and mssql maybe with that it will be easier to test locally all dialects (hopefully I have time to do it before 2018 :p and permission from microsoft to bundle mssql there...).

@elhigu elhigu merged commit 138e13b into knex:master Mar 26, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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

2 participants