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

Different txIf context when called from a transaction #479

Closed
nazarhussain opened this issue Mar 6, 2018 · 5 comments
Closed

Different txIf context when called from a transaction #479

nazarhussain opened this issue Mar 6, 2018 · 5 comments

Comments

@nazarhussain
Copy link

The documentation for txIf refers that:

The default condition is not in transaction, to start a transaction only if currently not in transaction, or else start a task

The phrase to start a transaction only if currently not in transaction provide impression that it will use the same transaction if called within a transaction context. Which is actually not the case.

Take this as example:

class DbReop {
   action() {
     return this.db.txIf('my-tx', t1 => t1.none(sql.someSQLFile));
   }
}

So if we use it with the following code:

db.tx('my-out-side-tx', function(t2) {
   return t2.DBRepo.action();
});

So logically t1 and t2 should refer to same transaction, but it actually not the case.

Its useful in many cases including:

  1. Writing unit tests related to transaction
  2. Extending active transaction to be used later used in repo method
  3. Spying/Stubbing transactions

We have many unit tests written for transaction related database repos, all of these start failing after upgrading to txIf. Please upgrade/fix the txIf with in context or suggest some way to unit test transactions with spying/stubbing.

@vitaly-t
Copy link
Owner

vitaly-t commented Mar 6, 2018

So logically t1 and t2 should refer to same transaction

No. Every task and transaction create their own connection context object with the protocol.

And the default logic for method txIf is to create a new task when already in transaction.

Your list of arguments isn't really valid...

Writing unit tests related to transaction

Nothing stops you from doing so.

Extending active transaction to be used later used in repo method

It is by design that you cannot use task/tx context outside of the callback. Also, creating the new context for each task/tx is what makes it possible to do independent extension of the protocol and/or implementation logic.

Spying/Stubbing transactions

You still can do so, just not for nested context.

@nazarhussain
Copy link
Author

nazarhussain commented Mar 6, 2018

@vitaly-t With the above example...

db.tx('my-out-side-tx', function *(t2) {
  
  sinonSandbox.spy(t2, 'none'); 
  
   yield t2.DBRepo.action();

  expect(t2.none).to.be.calledOnce; 
});

This test failed, as we spy t2 and none was called on t1 whereas both don't refer to same transaction/task object.

This is blocking us to write unit tests for transactions related code.

@vitaly-t
Copy link
Owner

vitaly-t commented Mar 6, 2018

This test failed, as we spy t2 and none was called on t1 whereas both don't refer to same transaction/task object.

And where is the t1 in your example? 😄

This code doesn't have it. And from the look of it, the example should work -

db.tx('my-out-side-tx', function *(t2) {
  
  sinonSandbox.spy(t2, 'none'); 
  
   yield t2.DBRepo.action();

  expect(t2.none).to.be.calledOnce; 
});

because this is no other task/tx context created there.

@vitaly-t vitaly-t changed the title Wrong context during txIf calls for an active transaction Different txIf context when called from a transaction Mar 6, 2018
@nazarhussain
Copy link
Author

@vitaly-t If you notice the code int he issue description, you will find t1 the code example just mentioned above should work, but its not. This line always failed expect(t2.none).to.be.calledOnce;

@vitaly-t
Copy link
Owner

vitaly-t commented Mar 7, 2018

Following this discussion, plus outside in Slack, opened new issue #480 as an improvement.

@vitaly-t vitaly-t closed this as completed Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants