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

Implement support for returning started transaction without using transaction() methods callback function #3099

Merged
merged 17 commits into from May 27, 2019

Conversation

Projects
None yet
3 participants
@kibertoad
Copy link
Collaborator

commented Mar 9, 2019

Original inspiration behind this change was discussion on implementing DDD pattern in Javascript that we had at our company. Currently there is no convenient way for implementation-level (repositories) entities to generate a transaction instance that could then be passed from domain-level (use cases) entities across all the business logic execution steps without knowing what exact db operations are going to be executed.

fixes #3095

@kibertoad kibertoad requested a review from elhigu Mar 9, 2019

@kibertoad

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 9, 2019

I'm not sure if it is confusing that meaning of a Promise that .transaction() returns is different based on provided params. It might be, and it may be a better solution to have separate method for this case, but I don't know what a good name that would be less confusing in that case would be.

@capaj

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2019

@kibertoad this looks useful. It will surely come in handy on our project at @LeapLabs. Thanks!

@kibertoad

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 10, 2019

@capaj You are welcome :)

@kibertoad kibertoad added the ready label Mar 21, 2019

Show resolved Hide resolved test/unit/knex.js
@elhigu
Copy link
Collaborator

left a comment

Couple of minor comments. I was surprised that getting transaction was non-async method, meaning, that at least it haven't had time to acquireConnection and initialize it before it is returned.

Also I would like to see how errors are propagated in this case if connection cannot be acquired because all connections from pool are in use. That is easy to test by calling knex.transaction() multiple times.

@elhigu

This comment has been minimized.

Copy link
Collaborator

commented Mar 30, 2019

Also might be a good idea to add to documentation something about availability of trx.initPromise for hooking functionality to transaction commit / rollback.

kibertoad and others added some commits Apr 11, 2019

Igor Savin
Merge branches 'feature/3095-direct-transaction' and 'master' of http…
…s://github.com/tgriesser/knex into feature/3095-direct-transaction

# Conflicts:
#	test/unit/knex.js
#	types/knex.d.ts
@kibertoad

This comment has been minimized.

Copy link
Collaborator Author

commented May 21, 2019

@elhigu Great point! There was, indeed, a gap in how errors were handled during transaction acquisition.

@kibertoad

This comment has been minimized.

Copy link
Collaborator Author

commented May 21, 2019

Also might be a good idea to add to documentation something about availability of trx.initPromise for hooking functionality to transaction commit / rollback.

Not really. initPromise is only used, as name implies, to track process of transaction itself getting prepared for usage. After this promise is resolved, it returns the transaction itself, and that's it. I don't think we should consider it a part of public api, as users are only supposed to consume it indirectly via knex.transaction()

@elhigu

This comment has been minimized.

Copy link
Collaborator

commented May 21, 2019

Also might be a good idea to add to documentation something about availability of trx.initPromise for hooking functionality to transaction commit / rollback.

Not really. initPromise is only used, as name implies, to track process of transaction itself getting prepared for usage. After this promise is resolved, it returns the transaction itself, and that's it. I don't think we should consider it a part of public api, as users are only supposed to consume it indirectly via knex.transaction()

Right. I thought that promise was the promise that get resolved / rejected when trx.commit() / trx.rollback(err) was called. Like the one that is returned when knex.transaction(trx => { ... }) is called.

Igor Savin added some commits May 21, 2019

@kibertoad

This comment has been minimized.

Copy link
Collaborator Author

commented May 21, 2019

@elhigu Hmmm, I can't find your other comment about your expectation for this not to start the actual transaction until someone actually uses it. I think that is doable, although would require slightly more intrusive changes. Do you think we should go for it within the scope of this PR?

Igor Savin added some commits May 21, 2019

@elhigu

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2019

@elhigu Hmmm, I can't find your other comment about your expectation for this not to start the actual transaction until someone actually uses it. I think that is doable, although would require slightly more intrusive changes. Do you think we should go for it within the scope of this PR?

I don't think that is in the scope of this PR. I'll just rename the issue to be less confusing :)

@elhigu elhigu changed the title Implement support for starting transactions without executing them Implement support for returning started transaction without using transaction() methods callback function May 23, 2019

Igor Savin

@kibertoad kibertoad requested a review from elhigu May 23, 2019

@kibertoad

This comment has been minimized.

Copy link
Collaborator Author

commented May 23, 2019

@elhigu It was a fairly trivial addition, though, so I ended up including it as a feature on top :)

@kibertoad

This comment has been minimized.

Copy link
Collaborator Author

commented May 23, 2019

@elhigu Phew, build finally passes again. Could you please rereview?

kibertoad added some commits May 26, 2019

Merge branches 'feature/3095-direct-transaction' and 'master' of http…
…s://github.com/tgriesser/knex into feature/3095-direct-transaction

# Conflicts:
#	types/index.d.ts

@kibertoad kibertoad merged commit 5307dac into master May 27, 2019

1 of 3 checks passed

continuous-integration/travis-ci/push The Travis CI build failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
coverage/coveralls Coverage remained the same at 88.665%
Details

@kibertoad kibertoad deleted the feature/3095-direct-transaction branch May 27, 2019

@kibertoad

This comment has been minimized.

Copy link
Collaborator Author

commented May 27, 2019

@capaj It was released as knex@0.17.0-next6, would be awesome if you could give it a try! Most likely final 0.17.0 stable will be published in a couple of days.

@@ -310,6 +310,40 @@ describe('knex', () => {
});
});

it('propagates error correctly when all connections are in use', function() {
this.timeout(30000);

This comment has been minimized.

Copy link
@elhigu

elhigu May 27, 2019

Collaborator

Why this needs 30secs timeout? 🤔

This comment has been minimized.

Copy link
@kibertoad

kibertoad May 27, 2019

Author Collaborator

It is unlikely to use any more than just a couple of seconds, it is going to crash and be handled way earlier than that.

var poolSqlite = {
min: 0,
max: 1,
acquireTimeoutMillis: 5000,

This comment has been minimized.

Copy link
@elhigu

elhigu May 27, 2019

Collaborator

If possible we could try to have subsecond timeouts in tests to prevent test runs to take too long just waiting around.

This comment has been minimized.

Copy link
@kibertoad

kibertoad May 27, 2019

Author Collaborator

Tried reducing it, but I think I was getting some flaky tests that way. Decreased it, let's see how it goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.