Currently inTransaction state on Connection is maintained based on the #461

tvrprasad · 2016-10-03T03:12:01Z

events 'beginTransaction', 'commitTransaction' and 'rollbackTransaction'.
These are all available only for TDS versions 7.2 and over. This is
causing several transaction integration tests to fail.

This PR attempts to fill in the gaps by managing inTransaction state based
on equivalent events and callbacks for TDS versions below 7.2. All the
tests pass now except for
transactionHelperMixedWithLowLevelTransactionMethods.

events 'beginTransaction', 'commitTransaction' and 'rollbackTransaction'. These are all available only for TDS versions 7.2 and over. This is causing several transaction integration tests to fail. This PR attempts to fill in the gaps by managing inTransaction state based on equivalent events and callbacks for TDS versions below 7.2. All the tests pass now except for transactionHelperMixedWithLowLevelTransactionMethods.

tvrprasad · 2016-10-03T03:15:55Z

@arthurschreiber This one ended as a bit of a wild goose chase me try to fake the 7.2+ events ('beginTransaction', 'commitTransaction' and 'rollbackTransaction') in 7.2- versions to manage inTransaction state. There is one test still failing - transactionHelperMixedWithLowLevelTransactionMethods. I'm just sharing the PR to get thoughts. I'm not sure if this is even the right approach. There's got to be a more official way of determining the transaction state in versions before 7.2.

I'll ask around but please take a look, if nothing else for my effort :)

tvrprasad · 2016-10-03T03:17:18Z

This PR is to help make progress on the remaining issues with #420.

Update transactionDepth for Savepoint in transaction(). With this all the tests pass for all versions TDS.

arthurschreiber · 2016-10-03T18:21:42Z

These changes look good so far, but I have a few questions.

Is there some way to get this solved without the 'transactionDepth' property? What's the reason for adding the 'isBatch' property? How will this interact with T-SQL transactions executed by the User manually?

arthurschreiber · 2016-10-03T18:25:47Z

src/connection.js

+        if (self.transactionDepth === 1) {
+          self.inTransaction = true;
+        }
+        return callback.apply(this, arguments);


This should be 'callback.apply(null, arguments)' I think. It works the way it is written now, because 'this' is equal to 'undefined' in the surrounding scope.

I think my change actually preserves the prior behavior in terms of how 'callback' is being invoked.

In the old code some code is calling 'callback' from somewhere and either giving it a 'this' value or not. We don't know that from here.

In the new code, new inline function() replaces 'callback' and hence should be called with the same 'this' value as above. All I'm doing is passing on that 'this' value to the invocation of 'callback', which preserves the old behavior.

Makes sense or am I missing something?

Ah, I understand your intention for this code now.

You're right, the callback given to new Request gets executed with this bound to the request instance, and by doing callback.apply(this, arguments) you're wrapping the callback while forwarding this this value correctly. 👍

I don't think this behaviour is necessarily right (or valid) for beginTransactions and friends, as the use of a Request object here is an implementation detail and should be of no concern to the user of the library, and we shouldn't expose this internal request object.

Also, if you check how callback is used a few lines below, it doesn't have a this binding set in that case. By using callback.apply(null, arguments), we would align both code paths.

That makes sense. Pushed new PR with 'this -> null' change.

tvrprasad · 2016-10-03T18:50:37Z

I don't know of anyway to do this without 'transactionDepth' property as yet. I'm asking around but TDS 7.1 is very old (SQL Server 2000) and not much knowledge around here any more on these specifics. I didn't see anything in the TDS protocol specification itself that gives us the equivalent of the new ENVCHANGE_TOKEN types - Begin Transaction, Rollback Transaction etc. for 7.1.

Reasons for 'isBatch':

Without the 'isBatch' property, transactionHelperBatchAbortingError fails with the following error: 'The ROLLBACK TRANSACTION request has no corresponding BEGIN TRANSACTION.'
There is this comment on 'rollbackTransaction' event handler in connection.js - "This is also fired if a batch aborting error happened that caused a rollback." That's consistent with the above bullet.

I'm not quite what you mean by this:
How will this interact with T-SQL transactions executed by the User manually?

Change callback invocations for TDS version < 7.2 in beingTransaction, commitTransaction, rollbackTransaction and saveTransaction to not expose the internal Request object instance created to send the TRANSACTION SQL commands.

…7.2.

arthurschreiber · 2016-10-05T07:46:31Z

src/connection.js

@@ -747,7 +779,11 @@ class Connection extends EventEmitter {
  saveTransaction(callback, name) {
    const transaction = new Transaction(name);
    if (this.config.options.tdsVersion < '7_2') {
-      return this.execSqlBatch(new Request('SAVE TRAN ' + transaction.name, callback));
+      const self = this;
+      return this.execSqlBatch(new Request('SAVE TRAN ' + transaction.name, function() {


In all these cases where you used const self = this; to capture the this context outside the callback, you can also use arrow functions now that you're no longer passing along the this context of the callback:

return this.execSqlBatch(new Request('SAVE TRAN ' + transaction.name, () => { this.transactionDepth++; return callback.apply(null, arguments); }));

Done. Pushed an update.

I made the change and discovered arrow functions don't have implicit 'arguments'. Added the rest parameters to the arrow function, like '(...args) => {'.

One day I hope not to be surprised by JavaScript anymore :)

arthurschreiber · 2016-10-05T07:52:10Z

If you can go and fix up that one suggestion of mine, I think you can merge whenever you feel this is ready.

I'd like to put some refactorings on top of these changes, but I want these to be committed to master first. Let's move in small but working steps. 👍

arthurschreiber · 2016-10-05T19:55:28Z

Oh, right, I completely forgot about that part... 🙈

tvrprasad · 2016-10-05T20:20:51Z

If you can go and fix up that one suggestion of mine, I think you can merge whenever you feel this is ready.
I'd like to put some refactorings on top of these changes, but I want these to be committed to master first. Let's move in small but working steps.

Are you asking that I merge my changes to master or to arthur/faster-appveyor-tests branch where we're doing this code review? Sounds like you want me to merge my changes to master. That'll require me to do a separate pull request with my changes cherry picked, right? Or is there a better way?

arthurschreiber · 2016-10-05T21:08:50Z

I meant that you can go ahead and merge this PR. 👍🏻 I'll do a final review of my PR including your changes tomorrow.

tvrprasad · 2016-10-05T22:35:54Z

Done merging. Thanks for review!

tvrprasad added 2 commits October 3, 2016 10:15

Fix test failure transactionHelperMixedWithLowLevelTransactionMethods.

8ada2df

Update transactionDepth for Savepoint in transaction(). With this all the tests pass for all versions TDS.

Fix typo. Change this -> self in one place in Connection.transaction().

38ecbd2

arthurschreiber reviewed Oct 3, 2016

View reviewed changes

tvrprasad added 3 commits October 3, 2016 11:54

Change == to === for SQL_BATCH comparison.

4c6916b

Set transactionDepth and isSqlBatch properties only for tdsVersion < …

1702a3e

…7.2.

arthurschreiber reviewed Oct 5, 2016

View reviewed changes

Switch from function() style to => in transaction method callbacks.

cd7f4b6

tvrprasad merged commit b7fd6cc into tediousjs:arthur/faster-appveyor-tests Oct 5, 2016

tvrprasad deleted the tds-multi-version branch October 5, 2016 22:35

arthurschreiber mentioned this pull request Nov 12, 2016

Release 1.15.0 #477

Closed

dazdaz mentioned this pull request Sep 30, 2024

[Snyk] Upgrade tedious from 1.14.0 to 1.15.0 dazdaz/developer-immersion-data#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Currently inTransaction state on Connection is maintained based on the #461

Currently inTransaction state on Connection is maintained based on the #461

tvrprasad commented Oct 3, 2016

tvrprasad commented Oct 3, 2016

tvrprasad commented Oct 3, 2016

arthurschreiber commented Oct 3, 2016

arthurschreiber Oct 3, 2016

tvrprasad Oct 3, 2016

arthurschreiber Oct 3, 2016

tvrprasad Oct 3, 2016

tvrprasad commented Oct 3, 2016

arthurschreiber Oct 5, 2016

tvrprasad Oct 5, 2016

arthurschreiber commented Oct 5, 2016

arthurschreiber commented Oct 5, 2016 via email

tvrprasad commented Oct 5, 2016

arthurschreiber commented Oct 5, 2016 via email

tvrprasad commented Oct 5, 2016

Currently inTransaction state on Connection is maintained based on the #461

Currently inTransaction state on Connection is maintained based on the #461

Conversation

tvrprasad commented Oct 3, 2016

tvrprasad commented Oct 3, 2016

tvrprasad commented Oct 3, 2016

arthurschreiber commented Oct 3, 2016

arthurschreiber Oct 3, 2016

Choose a reason for hiding this comment

tvrprasad Oct 3, 2016

Choose a reason for hiding this comment

arthurschreiber Oct 3, 2016

Choose a reason for hiding this comment

tvrprasad Oct 3, 2016

Choose a reason for hiding this comment

tvrprasad commented Oct 3, 2016

arthurschreiber Oct 5, 2016

Choose a reason for hiding this comment

tvrprasad Oct 5, 2016

Choose a reason for hiding this comment

arthurschreiber commented Oct 5, 2016

arthurschreiber commented Oct 5, 2016 via email

tvrprasad commented Oct 5, 2016

arthurschreiber commented Oct 5, 2016 via email

tvrprasad commented Oct 5, 2016