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

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad 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
Copy link
Collaborator Author

@kibertoad kibertoad 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.

Loading

@capaj
Copy link
Contributor

@capaj capaj commented Mar 10, 2019

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

Loading

@kibertoad
Copy link
Collaborator Author

@kibertoad kibertoad commented Mar 10, 2019

@capaj You are welcome :)

Loading

src/util/make-knex.js Show resolved Hide resolved
Loading
test/unit/knex.js Show resolved Hide resolved
Loading
Copy link
Member

@elhigu elhigu 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.

Loading

@elhigu
Copy link
Member

@elhigu elhigu 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.

Loading

@kibertoad
Copy link
Collaborator Author

@kibertoad kibertoad commented May 21, 2019

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

Loading

@kibertoad
Copy link
Collaborator Author

@kibertoad kibertoad 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()

Loading

@elhigu
Copy link
Member

@elhigu elhigu 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.

Loading

@kibertoad
Copy link
Collaborator Author

@kibertoad kibertoad 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?

Loading

@elhigu
Copy link
Member

@elhigu elhigu 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 :)

Loading

@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
@kibertoad kibertoad requested a review from elhigu May 23, 2019
@kibertoad
Copy link
Collaborator Author

@kibertoad kibertoad commented May 23, 2019

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

Loading

@kibertoad
Copy link
Collaborator Author

@kibertoad kibertoad commented May 23, 2019

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

Loading

@kibertoad kibertoad merged commit 5307dac into master May 27, 2019
1 of 3 checks passed
Loading
@kibertoad kibertoad deleted the feature/3095-direct-transaction branch May 27, 2019
@kibertoad
Copy link
Collaborator Author

@kibertoad kibertoad 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.

Loading

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

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

@elhigu elhigu May 27, 2019

Choose a reason for hiding this comment

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

Why this needs 30secs timeout? 🤔

Loading

Copy link
Collaborator Author

@kibertoad kibertoad May 27, 2019

Choose a reason for hiding this comment

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

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.

Loading

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

@elhigu elhigu May 27, 2019

Choose a reason for hiding this comment

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

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

Loading

Copy link
Collaborator Author

@kibertoad kibertoad May 27, 2019

Choose a reason for hiding this comment

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

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants