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

Projects
None yet
2 participants
@wubzz
Collaborator

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

@elhigu

elhigu approved these changes Mar 26, 2017

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

Collaborator

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

This comment has been minimized.

@wubzz

wubzz Mar 26, 2017

Collaborator

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

Collaborator

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 tgriesser:master Mar 26, 2017

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