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

Fix return duplicate transaction promise for standalone transactions #3328

Merged
merged 1 commit into from Jul 4, 2019
Merged

Fix return duplicate transaction promise for standalone transactions #3328

merged 1 commit into from Jul 4, 2019

Conversation

vonagam
Copy link
Contributor

@vonagam vonagam commented Jul 3, 2019

So it seems like #3319 was a mistake and having a duplicate is needed because original execution promise does have catch attached so it will never produce unhandled rejections. So if you do not attach catch to executionPromise nobody will tell you that your transaction queries are failing (except this debug statement).

Solution: promise duplication but only for standalone transactions.

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Jul 3, 2019

@vonagam One test is failing.

@vonagam
Copy link
Contributor Author

@vonagam vonagam commented Jul 4, 2019

Should be fixed now. But executionPromise will behave differently for standalone and callback style transactions: for standalone you must catch rejections from executionPromise, while for callback style it is optional (since they have access to _promise itself which behaves the same).

@vonagam
Copy link
Contributor Author

@vonagam vonagam commented Jul 4, 2019

Fixed typo in transactionProvider.

Also transactionProvider returns a standalone transaction retriever and so executionPromise should be handled in tests where a provider is used otherwise they might produce unhandled rejections which are actually tests failures.

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Jul 4, 2019

@vonagam A different test on postgreSQL is failing now.

@vonagam
Copy link
Contributor Author

@vonagam vonagam commented Jul 4, 2019

Yes, i know. This test was failing before, but silently, it was producing unhandled rejections before, now the test properly fails because executionPromise is listened to.

Here is the link to an unhandled rejection warning for this test when it was introduced, so it was actually not passing from the start. (By the way i include line numbers in my links to travis logs, but unfortunately travis does not scroll to specified rows)

I can't run postgres tests (some issue with a connection... maybe figure it out later), i was running only sqlite tests on my machine before.

@vonagam
Copy link
Contributor Author

@vonagam vonagam commented Jul 4, 2019

Okay, managed to run this test. The issue is on this line. For me postgres res object does not have context property (it does have direct sql property equal to "ROLLBACK" though). So it fails the check and rejects with undefined.

I think this should be fixed together with first issue from #3309.

@vonagam
Copy link
Contributor Author

@vonagam vonagam commented Jul 4, 2019

Fixed it.

get(res, 'context.sql', '').toUpperCase() === 'ROLLBACK' &&
this.doNotRejectOnRollback
) {
if (this.doNotRejectOnRollback && /^ROLLBACK\b/i.test(sql)) {
Copy link
Collaborator

@kibertoad kibertoad Jul 4, 2019

Choose a reason for hiding this comment

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

Should this be case-sensitive?

Copy link
Collaborator

@kibertoad kibertoad Jul 4, 2019

Choose a reason for hiding this comment

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

This change is probably also needed for db dialect transaction implementations that have their own rollback handling.

Copy link
Collaborator

@kibertoad kibertoad Jul 4, 2019

Choose a reason for hiding this comment

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

I mean https://github.com/tgriesser/knex/blob/master/src/dialects/mysql/transaction.js, doesn't look like any other dialect has this logic.

Copy link
Contributor Author

@vonagam vonagam Jul 4, 2019

Choose a reason for hiding this comment

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

  • Current implementation is case-insensitive, so that's why i added /i flag to make the regexp case-insensitive too. But actually it does not need to be case-insensitive, since only valid way to rollback or commit is though trx.rollback() or trx.commit(), only they set status argument, so only they _resolve() or _reject(), so only they release a connection properly, and they use upper case, so actually there is no need for /i flag, i just kept it to be similar to current implementation.

  • Other dialects do usual rollback fine, since only postgres was failing the test. But i assume they do nested rollback badly (first issue). So yeah, other dialects should be updated to treat rollbackTo() queries the same way as rollback(), but it will require new tests for such cases and new PR. I just wanted to make current tests pass.

Copy link
Collaborator

@kibertoad kibertoad Jul 4, 2019

Choose a reason for hiding this comment

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

Fair enough!

@kibertoad kibertoad merged commit a0e9be5 into knex:master Jul 4, 2019
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants