Fix return duplicate transaction promise for standalone transactions

[vonagam]

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]

@vonagam One test is failing.

[vonagam]

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]

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]

@vonagam A different test on postgreSQL is failing now.

[vonagam]

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]

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]

Fixed it.

[kibertoad]

Should this be case-sensitive?

[kibertoad]

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

[kibertoad]

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

[vonagam]

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

[kibertoad]

Fair enough!